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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux