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@xxxxxxxx> wrote:
> > From: Rob Clark <robdclark@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Cc: Daniel Stone <daniel@xxxxxxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Michel Dänzer <michel@xxxxxxxxxxx>
> > Signed-off-by: Rob Clark <robdclark@xxxxxxxxx> (v1.5)
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >
>
> 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@xxxxxxxxx>
>
> > --
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
