[PATCH 1/4] drm/msm/gpu: add submit flag to hint which buffers should be dumped

[PATCH 1/4] drm/msm/gpu: add submit flag to hint which buffers should be dumped

[Rob Clark]

To lower CPU  overhead, future userspace will be switching to pinning
iova and avoiding the use of relocs, and only include cmds table entries
for IB1 level cmdstream (but not IB2 or state-groups).

This leaves the kernel unsure what to dump for rd/hangrd cmdstream
dumping.  So add a MSM_SUBMIT_BO_DUMP flag so userspace can indicate
buffers that contain cmdstream (or are otherwise important to dump).

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_gem_submit.c |  5 ++++-
 drivers/gpu/drm/msm/msm_rd.c         | 13 ++++++++++---
 include/uapi/drm/msm_drm.h           |  5 ++++-
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index ddd95a078ec4..eab638011f4c 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -114,8 +114,11 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
 			pagefault_disable();
 		}
 
+/* at least one of READ and/or WRITE flags should be set: */
+#define MANDATORY_FLAGS (MSM_SUBMIT_BO_READ | MSM_SUBMIT_BO_WRITE)
+
 		if ((submit_bo.flags & ~MSM_SUBMIT_BO_FLAGS) ||
-			!(submit_bo.flags & MSM_SUBMIT_BO_FLAGS)) {
+			!(submit_bo.flags & MANDATORY_FLAGS)) {
 			DRM_ERROR("invalid flags: %x\n", submit_bo.flags);
 			ret = -EINVAL;
 			goto out_unlock;
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
index 0c2c8d2c631f..90e9d0a48dc0 100644
--- a/drivers/gpu/drm/msm/msm_rd.c
+++ b/drivers/gpu/drm/msm/msm_rd.c
@@ -348,6 +348,12 @@ static void snapshot_buf(struct msm_rd_state *rd,
 	msm_gem_put_vaddr(&obj->base);
 }
 
+static bool
+should_dump(struct msm_gem_submit *submit, int idx)
+{
+	return rd_full || (submit->bos[idx].flags & MSM_SUBMIT_BO_DUMP);
+}
+
 /* called under struct_mutex */
 void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit,
 		const char *fmt, ...)
@@ -389,15 +395,16 @@ void msm_rd_dump_submit(struct msm_rd_state *rd, struct msm_gem_submit *submit,
 
 	rd_write_section(rd, RD_CMD, msg, ALIGN(n, 4));
 
-	for (i = 0; rd_full && i < submit->nr_bos; i++)
-		snapshot_buf(rd, submit, i, 0, 0);
+	for (i = 0; i < submit->nr_bos; i++)
+		if (should_dump(submit, i))
+			snapshot_buf(rd, submit, i, 0, 0);
 
 	for (i = 0; i < submit->nr_cmds; i++) {
 		uint64_t iova = submit->cmd[i].iova;
 		uint32_t szd  = submit->cmd[i].size; /* in dwords */
 
 		/* snapshot cmdstream bo's (if we haven't already): */
-		if (!rd_full) {
+		if (!should_dump(submit, i)) {
 			snapshot_buf(rd, submit, submit->cmd[i].idx,
 					submit->cmd[i].iova, szd * 4);
 		}
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index c06d0a5bdd80..3c3af92c4b3e 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -188,8 +188,11 @@ struct drm_msm_gem_submit_cmd {
  */
 #define MSM_SUBMIT_BO_READ             0x0001
 #define MSM_SUBMIT_BO_WRITE            0x0002
+#define MSM_SUBMIT_BO_DUMP             0x0004
 
-#define MSM_SUBMIT_BO_FLAGS            (MSM_SUBMIT_BO_READ | MSM_SUBMIT_BO_WRITE)
+#define MSM_SUBMIT_BO_FLAGS            (MSM_SUBMIT_BO_READ | \
+					MSM_SUBMIT_BO_WRITE | \
+					MSM_SUBMIT_BO_DUMP)
 
 struct drm_msm_gem_submit_bo {
 	__u32 flags;          /* in, mask of MSM_SUBMIT_BO_x */
-- 
2.19.2

[PATCH 2/4] drm/msm: rework GEM_INFO ioctl

[Rob Clark]

Prep work to add a way to get/set the GEM objects debug name.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 25 ++++++++++++++++---------
 include/uapi/drm/msm_drm.h    | 17 ++++++++++++-----
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9f823bf8d312..913f5b3642b5 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -863,21 +863,28 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 	struct drm_gem_object *obj;
 	int ret = 0;
 
-	if (args->flags & ~MSM_INFO_FLAGS)
+	switch (args->info) {
+	case MSM_INFO_GET_OFFSET:
+	case MSM_INFO_GET_IOVA:
+		/* value returned as immediate, not pointer, so len==0: */
+		if (args->len)
+			return -EINVAL;
+		break;
+	default:
 		return -EINVAL;
+	}
 
 	obj = drm_gem_object_lookup(file, args->handle);
 	if (!obj)
 		return -ENOENT;
 
-	if (args->flags & MSM_INFO_IOVA) {
-		uint64_t iova;
-
-		ret = msm_ioctl_gem_info_iova(dev, obj, &iova);
-		if (!ret)
-			args->offset = iova;
-	} else {
-		args->offset = msm_gem_mmap_offset(obj);
+	switch (args->info) {
+	case MSM_INFO_GET_OFFSET:
+		args->value = msm_gem_mmap_offset(obj);
+		break;
+	case MSM_INFO_GET_IOVA:
+		ret = msm_ioctl_gem_info_iova(dev, obj, &args->value);
+		break;
 	}
 
 	drm_gem_object_put_unlocked(obj);
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index 3c3af92c4b3e..bc1757848c7c 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -105,14 +105,21 @@ struct drm_msm_gem_new {
 	__u32 handle;         /* out */
 };
 
-#define MSM_INFO_IOVA	0x01
-
-#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 */
 
 struct drm_msm_gem_info {
 	__u32 handle;         /* in */
-	__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 */
 };
 
 #define MSM_PREP_READ        0x01
-- 
2.19.2

[PATCH 3/4] drm/msm: add uapi to get/set debug name

[Rob Clark]

Add UAPI to get/set GEM objects' debug name.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 36 ++++++++++++++++++++++++++++++++++-
 include/uapi/drm/msm_drm.h    |  2 ++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 913f5b3642b5..6ebbd5010722 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -23,6 +23,7 @@
 #include "msm_drv.h"
 #include "msm_debugfs.h"
 #include "msm_fence.h"
+#include "msm_gem.h"
 #include "msm_gpu.h"
 #include "msm_kms.h"
 
@@ -861,7 +862,8 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 {
 	struct drm_msm_gem_info *args = data;
 	struct drm_gem_object *obj;
-	int ret = 0;
+	struct msm_gem_object *msm_obj;
+	int i, ret = 0;
 
 	switch (args->info) {
 	case MSM_INFO_GET_OFFSET:
@@ -870,6 +872,9 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 		if (args->len)
 			return -EINVAL;
 		break;
+	case MSM_INFO_SET_NAME:
+	case MSM_INFO_GET_NAME:
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -878,6 +883,8 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOENT;
 
+	msm_obj = to_msm_bo(obj);
+
 	switch (args->info) {
 	case MSM_INFO_GET_OFFSET:
 		args->value = msm_gem_mmap_offset(obj);
@@ -885,6 +892,33 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
 	case MSM_INFO_GET_IOVA:
 		ret = msm_ioctl_gem_info_iova(dev, obj, &args->value);
 		break;
+	case MSM_INFO_SET_NAME:
+		/* length check should leave room for terminating null: */
+		if (args->len >= sizeof(msm_obj->name)) {
+			ret = -EINVAL;
+			break;
+		}
+		ret = copy_from_user(msm_obj->name,
+			u64_to_user_ptr(args->value), args->len);
+		msm_obj->name[args->len] = '\0';
+		for (i = 0; i < args->len; i++) {
+			if (!isprint(msm_obj->name[i])) {
+				msm_obj->name[i] = '\0';
+				break;
+			}
+		}
+		break;
+	case MSM_INFO_GET_NAME:
+		if (args->value && (args->len < strlen(msm_obj->name))) {
+			ret = -EINVAL;
+			break;
+		}
+		args->len = strlen(msm_obj->name);
+		if (args->value) {
+			ret = copy_to_user(u64_to_user_ptr(args->value),
+					msm_obj->name, args->len);
+		}
+		break;
 	}
 
 	drm_gem_object_put_unlocked(obj);
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index bc1757848c7c..09f16fd7beda 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -114,6 +114,8 @@ struct drm_msm_gem_new {
  */
 #define MSM_INFO_GET_OFFSET	0x00   /* get mmap() offset, returned by value */
 #define MSM_INFO_GET_IOVA	0x01   /* get iova, returned by value */
+#define MSM_INFO_SET_NAME	0x02   /* set the debug name (by pointer) */
+#define MSM_INFO_GET_NAME	0x03   /* get debug name, returned by pointer */
 
 struct drm_msm_gem_info {
 	__u32 handle;         /* in */
-- 
2.19.2

[PATCH 4/4] drm/msm: bump UAPI version

[Rob Clark]

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/msm/msm_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 6ebbd5010722..782cc33916d6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -36,9 +36,11 @@
  * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +
  *           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
 
 static const struct drm_mode_config_funcs mode_config_funcs = {
-- 
2.19.2

Re: [PATCH 4/4] drm/msm: bump UAPI version

[Arnd Bergmann]

On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote:
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 6ebbd5010722..782cc33916d6 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -36,9 +36,11 @@
>   * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +
>   *           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?

      Arnd

Re: [PATCH 2/4] drm/msm: rework GEM_INFO ioctl

[Arnd Bergmann]

On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote:
>

> -
> -#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 */
>
>  struct drm_msm_gem_info {
>         __u32 handle;         /* in */
> -       __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

Re: [PATCH 2/4] drm/msm: rework GEM_INFO ioctl

[Rob Clark]

On Fri, Nov 30, 2018 at 10:14 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote:
> >
>
> > -
> > -#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 */
> >
> >  struct drm_msm_gem_info {
> >         __u32 handle;         /* in */
> > -       __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.

hmm, right, pad field is a good idea.  As far as compat, drm_ioctl()
handles zero-padding so adding new ioctl struct members at the end is
safe (as long as a zero value somehow results in previous behavior)

BR,
-R

>
>      Arnd

Re: [PATCH 4/4] drm/msm: bump UAPI version

[Rob Clark]

On Fri, Nov 30, 2018 at 10:12 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/msm_drv.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 6ebbd5010722..782cc33916d6 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -36,9 +36,11 @@
> >   * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +
> >   *           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?
>

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));
}


(The old userspace on new kernel case is handled by zero padding in drm_ioctl())


BR,
-R

Re: [PATCH 4/4] drm/msm: bump UAPI version

[Arnd Bergmann]

On Fri, Nov 30, 2018 at 4:31 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Nov 30, 2018 at 10:12 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > ---
> > >  drivers/gpu/drm/msm/msm_drv.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > index 6ebbd5010722..782cc33916d6 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -36,9 +36,11 @@
> > >   * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +
> > >   *           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?
> >
>
> 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.

> 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));
> }

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.

      Arnd

Re: [PATCH 4/4] drm/msm: bump UAPI version

[Rob Clark]

On Fri, Nov 30, 2018 at 10:36 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Nov 30, 2018 at 4:31 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Fri, Nov 30, 2018 at 10:12 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/msm/msm_drv.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > index 6ebbd5010722..782cc33916d6 100644
> > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > @@ -36,9 +36,11 @@
> > > >   * - 1.3.0 - adds GMEM_BASE + NR_RINGS params, SUBMITQUEUE_NEW +
> > > >   *           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?
> > >
> >
> > 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.
>
> > 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));
> > }
>
> 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.

In this case, yes, probably a better example would be v1.1.0 when
support for >4 cmd buffers was added.  For older kernels userspace has
to structure the cmdstream in a less efficient way to work around that
limitation.

I guess these are things where a dummy ioctl call to probe whether
kernel returns an error could be used.. but that gets awkward.

BR,
-R