Re: [PATCH libdrm] headers: Sync with drm-next

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 10, 2019 at 09:49:33PM -0400, Rob Clark wrote:
> On Tue, Apr 9, 2019 at 8:27 AM Eric Engestrom <eric.engestrom@intel.com> wrote:
> >
> > On Tuesday, 2019-04-09 12:59:13 +0100, Eric Engestrom wrote:
> > > On Tuesday, 2019-04-09 11:35:14 +0000, Ayan Halder wrote:
> > > > Generated using make headers_install from the drm-next
> > > > tree - git://anongit.freedesktop.org/drm/drm
> > > > branch - drm-next
> > > > commit - 14d2bd53a47a7e1cb3e03d00a6b952734cf90f3f
> > > >
> > > > The changes were as follows :-
> > > >
> > > > core: (drm.h, drm_fourcc.h, drm_mode.h)
> > > > - Added 'struct drm_syncobj_transfer', 'struct drm_syncobj_timeline_wait' and 'struct drm_syncobj_timeline_array'
> > > > - Added various DRM_IOCTL_SYNCOBJ_ ioctls
> > > > - Added some new RGB and YUV formats
> > > > - Added 'DRM_FORMAT_MOD_VENDOR_ALLWINNER'
> > > > - Added 'SAMSUNG' and Arm's 'AFBC' and 'ALLWINNER' format modifiers
> > > > - Added 'struct drm_mode_rect'
> > > >
> > > > i915:
> > > > - Added struct 'struct i915_user_extension' and various 'struct drm_i915_gem_context_'
> > > > - Added different modes of per-process Graphics Translation Table
> > > >
> > > > msm:
> > > > - Added various get or set GEM buffer info flags
> > > > - Added some MSM_SUBMIT_BO_ macros
> > > > - Modified 'struct drm_msm_gem_info'
> > > >
> > > > Signed-off-by: Ayan Kumar halder <ayan.halder@arm.com>
> > >
> > > This looks sane, and applies cleanly :)
> > > Acked-by: Eric Engestrom <eric.engestrom@intel.com>
> >
> > Actually, revoking that, as this patch breaks the build; see below.
> >
> > >
> > > > ---
> > > >  include/drm/drm.h        |  36 +++++++
> > > >  include/drm/drm_fourcc.h | 136 +++++++++++++++++++++++++++
> > > >  include/drm/drm_mode.h   |  23 ++++-
> > > >  include/drm/i915_drm.h   | 237 ++++++++++++++++++++++++++++++++++++++++-------
> > > >  include/drm/msm_drm.h    |  25 +++--
> > > >  5 files changed, 415 insertions(+), 42 deletions(-)
> > > >
> > [snip]
> > > > diff --git a/include/drm/msm_drm.h b/include/drm/msm_drm.h
> > > > index c06d0a5..91a16b3 100644
> > > > --- a/include/drm/msm_drm.h
> > > > +++ b/include/drm/msm_drm.h
> > > > @@ -105,14 +105,24 @@ 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 */
> > > > +#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 */
> > > > -   __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 */
> > > > +   __u32 pad;
> >
> > freedreno/msm/msm_bo.c needs to be updated to reflect those changes.
> 
> 
> I think you can just rename flags->info and offset->value, the rest of
> the struct should be zero-initialized.. if in doubt you can check
> $mesa/src/freedreno/drm/msm_bo.c
> 
> side-note:  the libdrm_freedreno code was folded into mesa in 19.0, so
> at *some* point we can probably disable libdrm_freedreno build by
> default.  (I'd kinda still like to keep the code around for some misc
> standalone tools I have, but that is the sort of thing where I can fix
> libdrm if it gets broken).  When to switch to disabled by default I
> guess comes down to how long we want to support mesa 18.x with latest
> libdrm??  Maybe after 19.1, since (selfishly motivated) that gives me
> a long enough window back in case I find myself needing to bisect for
> some regression..

Sidenote: renaming fields breaks uapi (but not uabi), so not really great.
Even if it doesn't matter for you since you copypasted the headers into
mesa ... Dont do this, it's regrets :-)

If you want to do rename uapi without breaking stuff, create a new
structure with new field names, that happens to match the old one. Plus
add a comment. So

struct drm_msm_gem_info2 {
   __u32 handle;         /* in */
   __u32 info;           /* in - one of MSM_INFO_* */
   __u64 value;          /* in or out */
   __u32 len;            /* in or out */
   __u32 pad;
};

You can just use that struct in the IOCTL number define, stuff will work
out correctly (we don't typecheck these in userspace, and lenght
mismatches are handled by the kernel automatically anyway).

-Daniel

> 
> BR,
> -R
> 
> >
> > > >  };
> > > >
> > > >  #define MSM_PREP_READ        0x01
> > > > @@ -188,8 +198,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.7.4
> > > >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch