diff mbox series

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

Message ID 20181130150050.13762-5-robdclark@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm/msm: uapi updates | expand

Commit Message

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

Comments

Arnd Bergmann Nov. 30, 2018, 3:12 p.m. UTC | #1 
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
Rob Clark Nov. 30, 2018, 3:31 p.m. UTC | #2 
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
Arnd Bergmann Nov. 30, 2018, 3:36 p.m. UTC | #3 
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
Rob Clark Nov. 30, 2018, 3:43 p.m. UTC | #4 
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
diff mbox series

Patch

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 = {