Re: [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2 (v1.5)

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 28, 2015 at 01:46:53PM -0500, Rob Clark wrote:
> On Wed, Jan 28, 2015 at 12:37 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > From: Rob Clark <robdclark@gmail.com>
> >
> > In DRM/KMS we are lacking a good way to deal with tiled/compressed
> > formats.  Especially in the case of dmabuf/prime buffer sharing, where
> > we cannot always rely on under-the-hood flags passed to driver specific
> > gem-create ioctl to pass around these extra flags.
> >
> > The proposal is to add a per-plane format modifier.  This allows to, if
> > necessary, use different tiling patters for sub-sampled planes, etc.
> > The format modifiers are added at the end of the ioctl struct, so for
> > legacy userspace it will be zero padded.
> >
> > TODO how best to deal with assignment of modifier token values?  The
> > rough idea was to namespace things with an 8bit vendor-id, and then
> > beyond that it is treated as an opaque value.  But that was a relatively
> > arbitrary choice.  There are cases where same tiling pattern and/or
> > compression is supported by various different vendors.  So we should
> > standardize to use the vendor-id and value of the first one who
> > documents the format?
> 
> iirc, I think we decided that drm_fourcc.h in drm-next is a sufficient
> authority for coordinating assignment of modifier token values, so we
> could probably drop this TODO.  Perhaps it wouldn't hurt to document
> somewhere that, as with fourcc/format values, new additions are
> expected to come with some description of the format?

Oh right forgotten to drop the TODO. I'll add a comment to the header
file.
> 
> >
> > v1: original
> > v1.5: increase modifier to 64b
> >
> > v2: Incorporate review comments from the big thread, plus a few more.
> >
> > - Add a getcap so that userspace doesn't have to jump through hoops.
> > - Allow modifiers only when a flag is set. That way drivers know when
> >   they're dealing with old userspace and need to fish out e.g. tiling
> >   from other information.
> > - After rolling out checks for ->modifier to all drivers I've decided
> >   that this is way too fragile and needs an explicit opt-in flag. So
> >   do that instead.
> > - Add a define (just for documentation really) for the "NONE"
> >   modifier. Imo we don't need to add mask #defines since drivers
> >   really should only do exact matches against values defined with
> >   fourcc_mod_code.
> > - Drop the Samsung tiling modifier on Rob's request since he's not yet
> >   sure whether that one is accurate.
> >
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Daniel Stone <daniel@fooishbar.org>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Signed-off-by: Rob Clark <robdclark@gmail.com> (v1.5)
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >
> 
> you forgot to change subject line to (v2).. but other than that, looks good

Ah, I generally don't keep a patch revision in the subject and forgot to
update it ;-)

> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> 
> > --
> >
> > I've picked this up since we want to push out some fancy new tiling
> > modes soonish. No defines yet, but Tvrkto is working on the i915 parts
> > of this.
> >
> > I think the only part I haven't done from the discussion is exposing a
> > list of supported modifiers. Not sure that's really useful though
> > since all this is highly hw specific.
> >
> > And a note to driver writes: They need to check or the flag and in its
> > absence make a reasonable choice about the internal layet (e.g. for
> > i915 consult the tiling mode of the underlying bo).
> > -Daniel
> > ---
> >  drivers/gpu/drm/drm_crtc.c    | 14 +++++++++++++-
> >  drivers/gpu/drm/drm_ioctl.c   |  3 +++
> >  include/drm/drm_crtc.h        |  3 +++
> >  include/uapi/drm/drm.h        |  1 +
> >  include/uapi/drm/drm_fourcc.h | 26 ++++++++++++++++++++++++++
> >  include/uapi/drm/drm_mode.h   |  9 +++++++++
> >  6 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 419f9d915c78..8090e3d64f67 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3324,6 +3324,12 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
> >                         DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
> >                         return -EINVAL;
> >                 }
> > +
> > +               if (r->modifier[i] && !(r->flags & DRM_MODE_FB_MODIFIERS)) {
> > +                       DRM_DEBUG_KMS("bad fb modifier %llu for plane %d\n",
> > +                                     r->modifier[i], i);
> > +                       return -EINVAL;
> > +               }
> >         }
> >
> >         return 0;
> > @@ -3337,7 +3343,7 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> >         struct drm_framebuffer *fb;
> >         int ret;
> >
> > -       if (r->flags & ~DRM_MODE_FB_INTERLACED) {
> > +       if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
> >                 DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
> >                 return ERR_PTR(-EINVAL);
> >         }
> > @@ -3353,6 +3359,12 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> >                 return ERR_PTR(-EINVAL);
> >         }
> >
> > +       if (r->flags & DRM_MODE_FB_MODIFIERS &&
> > +           !dev->mode_config.allow_fb_modifiers) {
> > +               DRM_DEBUG_KMS("driver does not support fb modifiers\n");
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> >         ret = framebuffer_check(r);
> >         if (ret)
> >                 return ERR_PTR(ret);
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 3785d66721f2..a6d773a61c2d 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -321,6 +321,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
> >                 else
> >                         req->value = 64;
> >                 break;
> > +       case DRM_CAP_ADDFB2_MODIFIERS:
> > +               req->value = dev->mode_config.allow_fb_modifiers;
> > +               break;
> >         default:
> >                 return -EINVAL;
> >         }
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 019c9b562144..767f7d4cab63 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1153,6 +1153,9 @@ struct drm_mode_config {
> >         /* whether async page flip is supported or not */
> >         bool async_page_flip;
> >
> > +       /* whether the driver supports fb modifiers */
> > +       bool allow_fb_modifiers;
> > +
> >         /* cursor size */
> >         uint32_t cursor_width, cursor_height;
> >  };
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 01b2d6d0e355..ff6ef62d084b 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -630,6 +630,7 @@ struct drm_gem_open {
> >   */
> >  #define DRM_CAP_CURSOR_WIDTH           0x8
> >  #define DRM_CAP_CURSOR_HEIGHT          0x9
> > +#define DRM_CAP_ADDFB2_MODIFIERS       0x10
> >
> >  /** DRM_IOCTL_GET_CAP ioctl argument type */
> >  struct drm_get_cap {
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index 646ae5f39f42..bf99557f1624 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -132,4 +132,30 @@
> >  #define DRM_FORMAT_YUV444      fourcc_code('Y', 'U', '2', '4') /* non-subsampled Cb (1) and Cr (2) planes */
> >  #define DRM_FORMAT_YVU444      fourcc_code('Y', 'V', '2', '4') /* non-subsampled Cr (1) and Cb (2) planes */
> >
> > +
> > +/*
> > + * Format Modifiers:
> > + *
> > + * Format modifiers describe, typically, a re-ordering or modification
> > + * of the data in a plane of an FB.  This can be used to express tiled/
> > + * swizzled formats, or compression, or a combination of the two.
> > + *
> > + * The upper 8 bits of the format modifier are a vendor-id as assigned
> > + * below.  The lower 56 bits are assigned as vendor sees fit.
> > + */
> > +
> > +/* Vendor Ids: */
> > +#define DRM_FORMAT_MOD_NONE           0
> > +#define DRM_FORMAT_MOD_VENDOR_INTEL   0x01
> > +#define DRM_FORMAT_MOD_VENDOR_AMD     0x02
> > +#define DRM_FORMAT_MOD_VENDOR_NV      0x03
> > +#define DRM_FORMAT_MOD_VENDOR_SAMSUNG 0x04
> > +#define DRM_FORMAT_MOD_VENDOR_QCOM    0x05
> > +/* add more to the end as needed */
> > +
> > +#define fourcc_mod_code(vendor, val) \
> > +       ((((u64)DRM_FORMAT_MOD_VENDOR_## vendor) << 56) | (val & 0x00ffffffffffffffL)
> > +
> > +/* Format Modifier tokens: */
> > +
> >  #endif /* DRM_FOURCC_H */
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index ca788e01dab2..dbeba949462a 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -336,6 +336,7 @@ struct drm_mode_fb_cmd {
> >  };
> >
> >  #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */
> > +#define DRM_MODE_FB_MODIFIERS  (1<<1) /* enables ->modifer[] */
> >
> >  struct drm_mode_fb_cmd2 {
> >         __u32 fb_id;
> > @@ -356,10 +357,18 @@ struct drm_mode_fb_cmd2 {
> >          * So it would consist of Y as offsets[0] and UV as
> >          * offsets[1].  Note that offsets[0] will generally
> >          * be 0 (but this is not required).
> > +        *
> > +        * To accommodate tiled, compressed, etc formats, a per-plane
> > +        * modifier can be specified.  The default value of zero
> > +        * indicates "native" format as specified by the fourcc.
> > +        * Vendor specific modifier token.  This allows, for example,
> > +        * different tiling/swizzling pattern on different planes.
> > +        * See discussion above of DRM_FORMAT_MOD_xxx.
> >          */
> >         __u32 handles[4];
> >         __u32 pitches[4]; /* pitch for each plane */
> >         __u32 offsets[4]; /* offset of each plane */
> > +       __u64 modifier[4]; /* ie, tiling, compressed (per plane) */
> >  };
> >
> >  #define DRM_MODE_FB_DIRTY_ANNOTATE_COPY 0x01
> > --
> > 2.1.4
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx