drm/msm: uapi updates

include/uapi/drm/msm_drm.h +8 -2

-
-#define MSM_INFO_FLAGS (MSM_INFO_IOVA)
+/* Get or set GEM buffer info.  The requested value can be passed
+ * directly in 'value', or for data larger than 64b 'value' is a
+ * pointer to userspace buffer, with 'len' specifying the number of
+ * bytes copied into that buffer.  For info returned by pointer,
+ * calling the GEM_INFO ioctl with null 'value' will return the
+ * required buffer size in 'len'
+ */
+#define MSM_INFO_GET_OFFSET    0x00   /* get mmap() offset, returned by value */

Quoted 14 lines · Rob Clark

include/uapi/drm/msm_drm.h +9 -2

-
-#define MSM_INFO_FLAGS (MSM_INFO_IOVA)
+/* Get or set GEM buffer info.  The requested value can be passed
+ * directly in 'value', or for data larger than 64b 'value' is a
+ * pointer to userspace buffer, with 'len' specifying the number of
+ * bytes copied into that buffer.  For info returned by pointer,
+ * calling the GEM_INFO ioctl with null 'value' will return the
+ * required buffer size in 'len'
+ */
+#define MSM_INFO_GET_OFFSET    0x00   /* get mmap() offset, returned by value */
+#define MSM_INFO_GET_IOVA      0x01   /* get iova, returned by value */

include/uapi/drm/msm_drm.h struct drm_msm_gem_new { · line 123

Quoted 8 lines

 struct drm_msm_gem_info {
        __u32 handle;         /* in */

include/uapi/drm/msm_drm.h +3 -2

-       __u32 flags;          /* in - combination of MSM_INFO_* flags */
-       __u64 offset;         /* out, mmap() offset or iova */
+       __u32 info;           /* in - one of MSM_INFO_* */
+       __u64 value;          /* in or out */
+       __u32 len;            /* in or out */
 };

As structure with implicit padding has the problem of possibly leaking kernel stack data. It's better to make the padding explicit here so you can zero it from the kernel. Also, as I mentioned in the other patch, you probably need a new data structure and ioctl command number to keep compatiblity with the old interface.

Arnd

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/msm_drv.c +0 -0

index 6ebbd5010722..782cc33916d6 100644
@@ -36,9 +36,11 @@
  * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +

Quoted 12 lines · Rob Clark

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/msm_drv.c +0 -0

index 6ebbd5010722..782cc33916d6 100644
@@ -36,9 +36,11 @@
  * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +

drivers/gpu/drm/msm/msm_drv.c line 45

Quoted 10 lines

drivers/gpu/drm/msm/msm_drv.c +3 -1

  *           SUBMITQUEUE_CLOSE ioctls, and MSM_INFO_IOVA flag for
  *           MSM_GEM_INFO ioctl.
+ * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
+ *           GEM object's debug name
  */
 #define MSM_VERSION_MAJOR      1
-#define MSM_VERSION_MINOR      3
+#define MSM_VERSION_MINOR      4
 #define MSM_VERSION_PATCHLEVEL 0
 

I don't know the background here, but generally speaking we don't have version numbers for ioctls in kernel drivers. Instead, the old ioctls need to remain functional, but you can add new ioctl commands in addition.

Is there something that makes this driver special?

Quoted 30 lines · Arnd Bergmann

Quoted 12 lines · Rob Clark

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/msm_drv.c +0 -0

index 6ebbd5010722..782cc33916d6 100644
@@ -36,9 +36,11 @@
  * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +

drivers/gpu/drm/msm/msm_drv.c line 45

Quoted 10 lines

drivers/gpu/drm/msm/msm_drv.c +3 -1

  *           SUBMITQUEUE_CLOSE ioctls, and MSM_INFO_IOVA flag for
  *           MSM_GEM_INFO ioctl.
+ * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
+ *           GEM object's debug name
  */
 #define MSM_VERSION_MAJOR      1
-#define MSM_VERSION_MINOR      3
+#define MSM_VERSION_MINOR      4
 #define MSM_VERSION_PATCHLEVEL 0
 

I don't know the background here, but generally speaking we don't have version numbers for ioctls in kernel drivers. Instead, the old ioctls need to remain functional, but you can add new ioctl commands in addition.

Quoted 2 lines

Is there something that makes this driver special?

The version # indicates to userspace that some new features are supported, so that new userspace on kernel can work. For example, the userspace side of setting a GEM obj debug name is:

static void msm_bo_set_name(struct fd_bo *bo, const char *fmt, va_list ap)

{
    struct drm_msm_gem_info req = {
            .handle = bo->handle,
            .info = MSM_INFO_SET_NAME,
    };
    char buf[32];
    int sz;

    /* bail if kernel doesn't support this: */
    if (bo->dev->version < FD_VERSION_SOFTPIN)
        return;

    sz = vsnprintf(buf, sizeof(buf), fmt, ap);

    req.value = VOID2U64(buf);
    req.len = MIN2(sz, sizeof(buf));

    drmCommandWrite(bo->dev->fd, DRM_MSM_GEM_INFO, &req, sizeof(req));
}

Quoted 32 lines · Rob Clark

Quoted 30 lines · Arnd Bergmann

Quoted 12 lines · Rob Clark

Signed-off-by Rob Clark <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/msm_drv.c +0 -0

index 6ebbd5010722..782cc33916d6 100644
@@ -36,9 +36,11 @@
  * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +

drivers/gpu/drm/msm/msm_drv.c line 45

Quoted 10 lines

drivers/gpu/drm/msm/msm_drv.c +3 -1

  *           SUBMITQUEUE_CLOSE ioctls, and MSM_INFO_IOVA flag for
  *           MSM_GEM_INFO ioctl.
+ * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get
+ *           GEM object's debug name
  */
 #define MSM_VERSION_MAJOR      1
-#define MSM_VERSION_MINOR      3
+#define MSM_VERSION_MINOR      4
 #define MSM_VERSION_PATCHLEVEL 0
 

I don't know the background here, but generally speaking we don't have version numbers for ioctls in kernel drivers. Instead, the old ioctls need to remain functional, but you can add new ioctl commands in addition.

Quoted 6 lines

Quoted 2 lines

Is there something that makes this driver special?

The version # indicates to userspace that some new features are supported, so that new userspace on kernel can work. For example, the userspace side of setting a GEM obj debug name is:

Ok, got it.

Quoted 18 lines

static void msm_bo_set_name(struct fd_bo *bo, const char *fmt, va_list ap)

{
    struct drm_msm_gem_info req = {
            .handle = bo->handle,
            .info = MSM_INFO_SET_NAME,
    };
    char buf[32];
    int sz;

    /* bail if kernel doesn't support this: */
    if (bo->dev->version < FD_VERSION_SOFTPIN)
        return;

    sz = vsnprintf(buf, sizeof(buf), fmt, ap);

    req.value = VOID2U64(buf);
    req.len = MIN2(sz, sizeof(buf));

Quoted 2 lines

    drmCommandWrite(bo->dev->fd, DRM_MSM_GEM_INFO, &req, sizeof(req));
}

So that version check seems harmless, but also not necessary, at least in this case, right? I would assume that calling into drmCommandWrite() with an invalid command will only return an error, which then gets ignored, where with the check, we would skip the call, knowing that it wont't work.