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

[Arnd Bergmann]

On Fri, Nov 30, 2018 at 4:31 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> On Fri, Nov 30, 2018 at 10:12 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >
> > On Fri, Nov 30, 2018 at 4:02 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
> > >
> > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
> > > ---
> > >  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