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

[Rob Clark]

On Fri, Nov 30, 2018 at 10:36 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> 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.

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