[PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Apr 21 13:08:29 UTC 2017
On Fri, Apr 21, 2017 at 03:10:31PM +0300, Laurent Pinchart wrote:
> Hello,
>
> CC'ing Hans Verkuil for his knowledge on colorspace.
>
> On Friday 21 Apr 2017 14:17:56 Ville Syrjälä wrote:
> > On Fri, Apr 21, 2017 at 12:51:14PM +0300, Jyri Sarha wrote:
> > > Add standard properties to control YCbCr to RGB conversion in DRM
> > > planes. The created properties are stored to drm_plane object to allow
> > > different set of supported conversion modes for different planes on
> > > the device. For simplicity the related property blobs for CSC matrix
> > > and YCbCr preoffsets are also stored in the same place. The blob
> > > contents are defined in the uapi/drm/drm_mode.h header.
> > >
> > > Signed-off-by: Jyri Sarha <jsarha at ti.com>
> > > ---
> > >
> > > drivers/gpu/drm/drm_atomic.c | 26 +++++++
> > > drivers/gpu/drm/drm_atomic_helper.c | 9 +++
> > > drivers/gpu/drm/drm_color_mgmt.c | 136 ++++++++++++++++++++++++++++++-
> > > drivers/gpu/drm/drm_plane.c | 10 +++
> > > include/drm/drm_color_mgmt.h | 23 ++++++
> > > include/drm/drm_plane.h | 10 +++
> > > include/uapi/drm/drm_mode.h | 12 ++++
> > > 7 files changed, 223 insertions(+), 3 deletions(-)
>
> [snip]
>
> > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> > > b/drivers/gpu/drm/drm_color_mgmt.c index cc23b9a..badaddd 100644
> > > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > > @@ -29,9 +29,14 @@
> > > /**
> > > * DOC: overview
> > > *
> > > - * Color management or color space adjustments is supported through a set
> > > of 5
> > > - * properties on the &drm_crtc object. They are set up by calling
> > > - * drm_crtc_enable_color_mgmt().
> > > + * Color management or color space adjustments in CRTCs is supported
> > > + * through a set of 5 properties on the &drm_crtc object. They are set
> > > + * up by calling drm_crtc_enable_color_mgmt().
> > > + *
> > > + * Color space conversions from YCbCr to RGB color space in planes is
> > > + * supporter trough 3 optional properties in &drm_plane object.
> > > + *
> > > + * The &drm_crtc object's properties are:
> > > *
> > > * "DEGAMMA_LUT”:
> > > * Blob property to set the degamma lookup table (LUT) mapping pixel data
> > > @@ -85,6 +90,28 @@
> > > * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> > > * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp
> > > with the
> > > * "GAMMA_LUT" property above.
> > >
> > > + *
> > > + * The &drm_plane object's properties are:
> > > + *
> > > + * "YCBCR_TO_RGB_MODE"
> > > + * Optional plane enum property to configure YCbCr to RGB
> > > + * conversion. The possible modes include a number of standard
> > > + * conversions and a possibility to select custom conversion
> > > + * matrix and preoffset vector. The driver should select the
> > > + * supported subset of of the modes.
> > > + *
> > > + * "YCBCR_TO_RGB_CSC"
> > > + * Optional plane property blob to set YCbCr to RGB conversion
> > > + * matrix. The blob contains struct drm_ycbcr_to_rgb_csc which is
> > > + * defined in uapi/drm/drm_mode.h. Whether this property is
> > > + * active dependent on YCBCR_TO_RGB_MODE property.
> > > + *
> > > + * "YCBCR_TO_RGB_PREOFFSET"
> > > + * Optional plane property blob to configure YCbCr offset before
> > > + * YCbCr to RGB CSC is applied. The blob contains struct
> > > + * drm_ycbcr_to_rgb_preoffset which is defined in
> > > + * uapi/drm/drm_mode.h. Whether this property is active dependent
> > > + * on YCBCR_TO_RGB_MODE property.
> > > */
> > >
> > > /**
> > > @@ -330,3 +357,106 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> > > drm_modeset_unlock_all(dev);
> > > return ret;
> > > }
> > > +
> > > +static char *ycbcr_to_rgb_mode_name[] = {
> > > + [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
> > > + "YCbCr BT.601 limited range TO RGB BT.601 full range",
> > > + [DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
> > > + "YCbCr BT.601 full range TO RGB BT.601 full range",
> > > + [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
> > > + "YCbCr BT.709 limited range TO RGB BT.709 full range",
> > > + [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
> > > + "YCbCr BT.2020 limited range TO RGB BT.2020 full range",
> > > + [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
> > > + "YCbCr BT.601 limited range TO RGB BT.709 full range",
> >
> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
> > conversion because the YCbCr->RGB part should be performed on gamma encoded
> > data and the colorspace conversion on linear data. So we need a degamma
> > stage in between. At least that seemed to be the general concencus after
> > the last round of mails on this topic.
> >
> > After staring at the v4l docs on this stuff I kinda like their
> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
> > little partial to calling the prop something like YCBCR_ENCODING.
>
> For quite obvious reasons I agree with this partial reasoning :-) I would also
> copy how V4L2 splits color space information into a transfer function, an
> encoding and a quantization. If you group all three in a single enum you will
> end up with lots of possible combinations.
>
> > OTOH if we want to expose the raw matrix as a blob then maybe calling it a
> > CSC might be better. Not sure there's much point in exposing it though.
> > I don't think most people are in the habit if cooking up new ways to
> > encode their pixel data.
>
> I believe it would be more about supporting weird color encodings that are in
> the wild out there in various media sources. I'm not an expert in the field of
> color spaces though.
We can always add more values to the enum if new useful encodings crop
up. I'd be more inclined to exposing the blob if some media formats
would allow you to specify a custom matrix as well. Not sure any do.
I guess maybe the only real benefit from exposing the blob would be that
you could then combine the encoding and colorspace conversion stages
to the one matrix if you for some reason decide that degamma is not your
thing. Otherwise we'd have to multiply the matrices in the kernel, which
I guess we may have to anyway in some cases. At least i915 tries to do
that (unsuccesfully I might add) for the output CSC stuff.
Anyways, I just wanted to point out that I think exposing the blob
propably won't have any real users, so not sure it's worth the
hassle. But if people find it useful I won't object to having it.
>
> > > + [DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL] =
> > > + "YCbCr BT.601 limited range TO RGB BT.2020 full range",
> > > + [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL] =
> > > + "YCbCr BT.709 limited range TO RGB BT.601 full range",
> > > + [DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL] =
> > > + "YCbCr BT.709 limited range TO RGB BT.2020 full range",
> > > + [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL] =
> > > + "YCbCr BT.2020 limited range TO RGB BT.601 full range",
> > > + [DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL] =
> > > + "YCbCr BT.2020 limited range TO RGB BT.709 full range",
> > > + [DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET] =
> > > + "YCbCr TO RGB CSC limited range preoffset",
> > > + [DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET] =
> > > + "YCbCr TO RGB CSC full range preoffset",
> > > + [DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR] =
> > > + "YCBCR TO RGB CSC preoffset vector",
> > > +};
> > > +
> > > +/**
> > > + * drm_plane_create_ycbcr_to_rgb_properties - ycbcr to rgb related
> > > properties
> > > + * @enum_list: drm_prop_enum_list array of supported modes without names
> > > + * @enum_list_len: length of enum_list array
> > > + * @default_mode: default csc mode
> > > + *
> > > + * Create and attach plane specific YCbCr to RGB conversion related
> > > + * properties to to the drm_plane object. The YCBCR_TO_RGB_MODE propety
> > > + * created and an the supported modes should be provided the enum_list
> > > + * parameter. YCBCR_TO_RGB_CSC and YCBCR_TO_RGB_PREOFFSET properties are
> > > + * created based on supported conversion modes. The enum_list parameter
> > > + * should not contain the enum names, because the standard names are
> > > + * added by this function.
> > > + */
> > > +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> > > + struct drm_prop_enum_list *enum_list,
> > > + unsigned int enum_list_len,
> > > + enum drm_plane_ycbcr_to_rgb_mode default_mode)
> > > +{
> > > + struct drm_device *dev = plane->dev;
> > > + struct drm_property *prop;
> > > + bool ycbcr_to_rgb_csc_create = false;
> > > + bool ycbcr_to_rgb_preoffset_create = false;
> > > + int i;
>
> i takes positive values only, you can make it an unsigned int.
>
> > > +
> > > + if (WARN_ON(plane->ycbcr_to_rgb_mode_property != NULL))
> > > + return -EEXIST;
> > > +
> > > + for (i = 0; i < enum_list_len; i++) {
> > > + enum drm_plane_ycbcr_to_rgb_mode mode = enum_list[i].type;
> > > +
> > > + enum_list[i].name = ycbcr_to_rgb_mode_name[mode];
> > > +
> > > + if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET ||
> > > + mode == DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET)
> > > + ycbcr_to_rgb_csc_create = true;
> > > +
> > > + if (mode == DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR) {
> > > + ycbcr_to_rgb_csc_create = true;
> > > + ycbcr_to_rgb_preoffset_create = true;
> > > + }
> > > + }
> > > +
> > > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> > > + "YCBCR_TO_RGB_MODE",
> > > + enum_list, enum_list_len);
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + plane->ycbcr_to_rgb_mode_property = prop;
> > > + drm_object_attach_property(&plane->base, prop, default_mode);
> > > +
> > > + if (ycbcr_to_rgb_csc_create) {
> > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > + DRM_MODE_PROP_BLOB,
> > > + "YCBCR_TO_RGB_CSC", 0);
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + plane->ycbcr_to_rgb_csc_property = prop;
> > > + drm_object_attach_property(&plane->base, prop, 0);
> > > + }
> > > +
> > > + if (ycbcr_to_rgb_preoffset_create) {
> > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC |
> > > + DRM_MODE_PROP_BLOB,
> > > + "YCBCR_TO_RGB_PREOFFSET", 0);
> > > + if (!prop)
> > > + return -ENOMEM;
> > > + plane->ycbcr_to_rgb_preoffset_property = prop;
> > > + drm_object_attach_property(&plane->base, prop, 0);
> > > + }
> > > +
> > > + return 0;
> > > +}
>
> [snip]
>
> > > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > > index 03a59cb..a20b3ff 100644
> > > --- a/include/drm/drm_color_mgmt.h
> > > +++ b/include/drm/drm_color_mgmt.h
>
> [snip]
>
> > > @@ -37,4 +39,25 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> > > int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
> > > int gamma_size);
> > >
> > > +enum drm_plane_ycbcr_to_rgb_mode {
> > > + DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL = 0,
> > > + DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL,
> > > + DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL,
> > > + DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL,
> > > + DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL,
> > > + DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT2020_FULL,
> > > + DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT601_FULL,
> > > + DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT2020_FULL,
> > > + DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT601_FULL,
> > > + DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT709_FULL,
> > > + DRM_PLANE_YCBCR_TO_RGB_CSC_LIM_PREOFFSET,
> > > + DRM_PLANE_YCBCR_TO_RGB_CSC_FULL_PREOFFSET,
> > > + DRM_PLANE_YCBCR_TO_RGB_CSC_PREOFFSET_VECTOR,
>
> All those values should be documented.
>
> Should the CSC coefficient tables be centralized in core code ? Patch 5/6 adds
> tables to the omapdrm driver, for standard conversions it would make sense to
> share that data.
>
> > > +};
> > > +
> > > +int drm_plane_create_ycbcr_to_rgb_properties(struct drm_plane *plane,
> > > + struct drm_prop_enum_list *enum_list,
> > > + unsigned int enum_list_len,
> > > + enum drm_plane_ycbcr_to_rgb_mode default_mode);
> > > +
> > > #endif
>
> [snip]
>
> > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > > index 8c67fc0..27e0bee 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -543,6 +543,18 @@ struct drm_color_lut {
> > > __u16 reserved;
> > > };
> > >
> > > +struct drm_ycbcr_to_rgb_csc {
> > > + /* Conversion matrix in 2-complement s32.32 format. */
> > > + __s64 ry, rcb, rcr;
> > > + __s64 gy, gcb, gcr;
> > > + __s64 by, bcb, bcr;
> > > +};
> > > +
> > > +struct drm_ycbcr_to_rgb_preoffset {
> > > + /* Offset vector in 2-complement s.32 format. */
> > > + __s32 y, cb, cr;
> > > +};
> > > +
> > >
> > > #define DRM_MODE_PAGE_FLIP_EVENT 0x01
> > > #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> > > #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
>
> --
> Regards,
>
> Laurent Pinchart
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list