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?
>
> 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
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
