| Message ID | 3f25532ae9b05c34996fc9657261b204c627e232.1492768073.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index f881319..d1512aa 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > { > struct drm_device *dev = plane->dev; > struct drm_mode_config *config = &dev->mode_config; > + int ret; > + bool dummy; > > if (property == config->prop_fb_id) { > struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); > @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > state->rotation = val; > } else if (property == plane->zpos_property) { > state->zpos = val; > + } else if (property == plane->ycbcr_to_rgb_mode_property) { > + state->ycbcr_to_rgb_mode = val; > + } else if (property == plane->ycbcr_to_rgb_csc_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ycbcr_to_rgb_csc, > + val, > + sizeof(struct drm_ycbcr_to_rgb_csc), > + &dummy); > + return ret; > + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ycbcr_to_rgb_preoffset, > + val, > + sizeof(struct drm_ycbcr_to_rgb_preoffset), > + &dummy); > + return ret; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > *val = state->rotation; > } else if (property == plane->zpos_property) { > *val = state->zpos; > + } else if (property == plane->ycbcr_to_rgb_mode_property) { > + *val = state->ycbcr_to_rgb_mode; > + } else if (property == plane->ycbcr_to_rgb_csc_property) { > + *val = state->ycbcr_to_rgb_csc ? > + state->ycbcr_to_rgb_csc->base.id : 0; > + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { > + *val = state->ycbcr_to_rgb_preoffset ? > + state->ycbcr_to_rgb_preoffset->base.id : 0; > } else if (plane->funcs->atomic_get_property) { > return plane->funcs->atomic_get_property(plane, state, property, val); > } else { > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index c3994b4..89fd826 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > { > memcpy(state, plane->state, sizeof(*state)); > > + if (state->ycbcr_to_rgb_csc) > + drm_property_blob_get(state->ycbcr_to_rgb_csc); > + > + if (state->ycbcr_to_rgb_preoffset) > + drm_property_blob_get(state->ycbcr_to_rgb_preoffset); > + > if (state->fb) > drm_framebuffer_get(state->fb); > > @@ -3236,6 +3242,9 @@ struct drm_plane_state * > */ > void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > { > + drm_property_blob_put(state->ycbcr_to_rgb_csc); > + drm_property_blob_put(state->ycbcr_to_rgb_preoffset); > + > if (state->fb) > drm_framebuffer_put(state->fb); > > 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. 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. > + [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; > + > + 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; > +} > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index bc71aa2..4c7e827 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane) > > kfree(plane->name); > > + if (plane->ycbcr_to_rgb_mode_property) > + drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property); > + > + if (plane->ycbcr_to_rgb_csc_property) > + drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property); > + > + if (plane->ycbcr_to_rgb_preoffset_property) > + drm_property_destroy(dev, > + plane->ycbcr_to_rgb_preoffset_property); > + > memset(plane, 0, sizeof(*plane)); > } > EXPORT_SYMBOL(drm_plane_cleanup); > 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 > @@ -26,6 +26,8 @@ > #include <linux/ctype.h> > > struct drm_crtc; > +struct drm_plane; > +struct drm_prop_enum_list; > > uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision); > > @@ -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, > +}; > + > +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 > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 9ab3e70..41dcde2 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -26,6 +26,7 @@ > #include <linux/list.h> > #include <linux/ctype.h> > #include <drm/drm_mode_object.h> > +#include <drm/drm_color_mgmt.h> > > struct drm_crtc; > struct drm_printer; > @@ -112,6 +113,11 @@ struct drm_plane_state { > unsigned int zpos; > unsigned int normalized_zpos; > > + /* YCbCr to RGB conversion */ > + enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode; > + struct drm_property_blob *ycbcr_to_rgb_csc; > + struct drm_property_blob *ycbcr_to_rgb_preoffset; > + > /* Clipped coordinates */ > struct drm_rect src, dst; > > @@ -523,6 +529,10 @@ struct drm_plane { > > struct drm_property *zpos_property; > struct drm_property *rotation_property; > + > + struct drm_property *ycbcr_to_rgb_mode_property; > + struct drm_property *ycbcr_to_rgb_csc_property; > + struct drm_property *ycbcr_to_rgb_preoffset_property; > }; > > #define obj_to_plane(x) container_of(x, struct drm_plane, base) > 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 > -- > 1.9.1
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@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. > > + [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 Shashank On 4/21/2017 4:47 PM, 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@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(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index f881319..d1512aa 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >> { >> struct drm_device *dev = plane->dev; >> struct drm_mode_config *config = &dev->mode_config; >> + int ret; >> + bool dummy; >> >> if (property == config->prop_fb_id) { >> struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); >> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >> state->rotation = val; >> } else if (property == plane->zpos_property) { >> state->zpos = val; >> + } else if (property == plane->ycbcr_to_rgb_mode_property) { >> + state->ycbcr_to_rgb_mode = val; >> + } else if (property == plane->ycbcr_to_rgb_csc_property) { >> + ret = drm_atomic_replace_property_blob_from_id(dev, >> + &state->ycbcr_to_rgb_csc, >> + val, >> + sizeof(struct drm_ycbcr_to_rgb_csc), >> + &dummy); >> + return ret; >> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { >> + ret = drm_atomic_replace_property_blob_from_id(dev, >> + &state->ycbcr_to_rgb_preoffset, >> + val, >> + sizeof(struct drm_ycbcr_to_rgb_preoffset), >> + &dummy); >> + return ret; >> } else if (plane->funcs->atomic_set_property) { >> return plane->funcs->atomic_set_property(plane, state, >> property, val); >> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >> *val = state->rotation; >> } else if (property == plane->zpos_property) { >> *val = state->zpos; >> + } else if (property == plane->ycbcr_to_rgb_mode_property) { >> + *val = state->ycbcr_to_rgb_mode; >> + } else if (property == plane->ycbcr_to_rgb_csc_property) { >> + *val = state->ycbcr_to_rgb_csc ? >> + state->ycbcr_to_rgb_csc->base.id : 0; >> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { >> + *val = state->ycbcr_to_rgb_preoffset ? >> + state->ycbcr_to_rgb_preoffset->base.id : 0; >> } else if (plane->funcs->atomic_get_property) { >> return plane->funcs->atomic_get_property(plane, state, property, val); >> } else { >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index c3994b4..89fd826 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >> { >> memcpy(state, plane->state, sizeof(*state)); >> >> + if (state->ycbcr_to_rgb_csc) >> + drm_property_blob_get(state->ycbcr_to_rgb_csc); >> + >> + if (state->ycbcr_to_rgb_preoffset) >> + drm_property_blob_get(state->ycbcr_to_rgb_preoffset); >> + >> if (state->fb) >> drm_framebuffer_get(state->fb); >> >> @@ -3236,6 +3242,9 @@ struct drm_plane_state * >> */ >> void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >> { >> + drm_property_blob_put(state->ycbcr_to_rgb_csc); >> + drm_property_blob_put(state->ycbcr_to_rgb_preoffset); >> + >> if (state->fb) >> drm_framebuffer_put(state->fb); >> >> 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. 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. Ville has already conveyed the zest of this design. When we want to blend the data targeting various framebuffers, we have to consider their color spaces too. So we should actually come up with a set of properties, which in a combination (and in sequence) provide the capability to blend a Rec 709 and Rec 2020 buffer. I was supposed to publish a design RFC doc for the same, but it got delayed from my side. Will publish that in parallel, to see if we can merge these two thoughts. - Shashank >> + [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; >> + >> + 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; >> +} >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c >> index bc71aa2..4c7e827 100644 >> --- a/drivers/gpu/drm/drm_plane.c >> +++ b/drivers/gpu/drm/drm_plane.c >> @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane) >> >> kfree(plane->name); >> >> + if (plane->ycbcr_to_rgb_mode_property) >> + drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property); >> + >> + if (plane->ycbcr_to_rgb_csc_property) >> + drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property); >> + >> + if (plane->ycbcr_to_rgb_preoffset_property) >> + drm_property_destroy(dev, >> + plane->ycbcr_to_rgb_preoffset_property); >> + >> memset(plane, 0, sizeof(*plane)); >> } >> EXPORT_SYMBOL(drm_plane_cleanup); >> 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 >> @@ -26,6 +26,8 @@ >> #include <linux/ctype.h> >> >> struct drm_crtc; >> +struct drm_plane; >> +struct drm_prop_enum_list; >> >> uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision); >> >> @@ -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, >> +}; >> + >> +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 >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index 9ab3e70..41dcde2 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -26,6 +26,7 @@ >> #include <linux/list.h> >> #include <linux/ctype.h> >> #include <drm/drm_mode_object.h> >> +#include <drm/drm_color_mgmt.h> >> >> struct drm_crtc; >> struct drm_printer; >> @@ -112,6 +113,11 @@ struct drm_plane_state { >> unsigned int zpos; >> unsigned int normalized_zpos; >> >> + /* YCbCr to RGB conversion */ >> + enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode; >> + struct drm_property_blob *ycbcr_to_rgb_csc; >> + struct drm_property_blob *ycbcr_to_rgb_preoffset; >> + >> /* Clipped coordinates */ >> struct drm_rect src, dst; >> >> @@ -523,6 +529,10 @@ struct drm_plane { >> >> struct drm_property *zpos_property; >> struct drm_property *rotation_property; >> + >> + struct drm_property *ycbcr_to_rgb_mode_property; >> + struct drm_property *ycbcr_to_rgb_csc_property; >> + struct drm_property *ycbcr_to_rgb_preoffset_property; >> }; >> >> #define obj_to_plane(x) container_of(x, struct drm_plane, base) >> 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 >> -- >> 1.9.1
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@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
On 04/21/17 14:17, Ville Syrjälä wrote: >> +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. > I do not really have the expertise to argue with that. I merely copied the idea from the mail thread I referred to in the cover letter. However, there are several display HWs out there that do not have all bolts and knobs to make the color-space conversion in exactly the ideal order, omap DSS being one of them. > 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. OTOH I guess this property should be called YCBCR_DECODING. > 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. In my first version it was called just CSC, but then I wanted to make it explicit what this CSC was used for to avoid mixing the YCbCr decoding matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces of HW that can do only one or the other, e.g. the offset calculations are supported only to one direction. > I don't think most people are in the habit if cooking up new ways to > encode their pixel data. > In the embedded side I can imagine there could be some custom appliances where one may want to do some custom thing with the CSC and not needing a custom kernel for that could make a life easier... but then again I am not really an expert in this area. Best regards, Jyri
On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: > On 04/21/17 14:17, Ville Syrjälä wrote: > >> +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. > > > > I do not really have the expertise to argue with that. I merely copied > the idea from the mail thread I referred to in the cover letter. > However, there are several display HWs out there that do not have all > bolts and knobs to make the color-space conversion in exactly the ideal > order, omap DSS being one of them. Yeah. Intel hardware is in the same boat for the time being. On current hw I think we can only really expose the YCbCr->RGB and degamma stages. On some limited set of platforms we could expose a blob as well, and I suppose it would then be possible to use it for color space conversion if you ignore gamma and/or only deal with linear RGB data. But it's such a limited subset of hardware for us that I don't think I'm interested in exposing it. In the future we should be getting a more fully fleged pipeline. > > > 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. OTOH > > I guess this property should be called YCBCR_DECODING. Only if you think of it as a verb. > > > 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. > > In my first version it was called just CSC, but then I wanted to make it > explicit what this CSC was used for to avoid mixing the YCbCr decoding > matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces > of HW that can do only one or the other, e.g. the offset calculations > are supported only to one direction. Are you planning to do RGB->YCbCr conversion in the plane as well? I think we'll be only doing that at crtc/connector level. > > > I don't think most people are in the habit if cooking up new ways to > > encode their pixel data. > > > > In the embedded side I can imagine there could be some custom appliances > where one may want to do some custom thing with the CSC and not needing > a custom kernel for that could make a life easier... but then again I am > not really an expert in this area. I would assume most customy things you'd do in the crtc (eg. color correction and whatnot). But could be that I just lack imagination.
Hi, Thanks for picking this up Jyri. On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote: >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: >> On 04/21/17 14:17, Ville Syrjälä wrote: >> >> +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. >> > I thought the conclusion of the last round was that on some hardware this would be conflated as a conscious choice. If the HW doesn't have the required degamma->csc->gamma stages, and the implementor/user doesn't care about being a little incorrect, then it can all be described in this single property. If there is more capable hardware with the additional stages, then they should expose additional properties (in pipeline order): YCBCR_TO_RGB_*: Does YCbCr->RGB conversion on non-linear YCbCr data, only the enum values which don't have a CSC are exposed. DEGAMMA: Does the non-linear to linear conversion on the RGB data output from the YCBCR_TO_RGB stage. RGB_TO_RGB_*: Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC on the linear data. GAMMA: Convert the CSC'd RGB data back in to non-linear for blending (if blending is to be done with non-linear data). Drivers can expose as many or as few of the above properties as their hardware supports. >> >> I do not really have the expertise to argue with that. I merely copied >> the idea from the mail thread I referred to in the cover letter. >> However, there are several display HWs out there that do not have all >> bolts and knobs to make the color-space conversion in exactly the ideal >> order, omap DSS being one of them. > >Yeah. Intel hardware is in the same boat for the time being. On current >hw I think we can only really expose the YCbCr->RGB and degamma stages. > >On some limited set of platforms we could expose a blob as well, and I >suppose it would then be possible to use it for color space conversion >if you ignore gamma and/or only deal with linear RGB data. But it's such >a limited subset of hardware for us that I don't think I'm interested >in exposing it. > I'm not sure the custom blob is worth having either. It can easily be added later if we decide we want it after all. Thanks, -Brian >In the future we should be getting a more fully fleged pipeline. > >> >> > 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. OTOH >> >> I guess this property should be called YCBCR_DECODING. > >Only if you think of it as a verb. > >> >> > 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. >> >> In my first version it was called just CSC, but then I wanted to make it >> explicit what this CSC was used for to avoid mixing the YCbCr decoding >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces >> of HW that can do only one or the other, e.g. the offset calculations >> are supported only to one direction. > >Are you planning to do RGB->YCbCr conversion in the plane as well? I >think we'll be only doing that at crtc/connector level. > >> >> > I don't think most people are in the habit if cooking up new ways to >> > encode their pixel data. >> > >> >> In the embedded side I can imagine there could be some custom appliances >> where one may want to do some custom thing with the CSC and not needing >> a custom kernel for that could make a life easier... but then again I am >> not really an expert in this area. > >I would assume most customy things you'd do in the crtc (eg. color >correction and whatnot). But could be that I just lack imagination. > >-- >Ville Syrjälä >Intel OTC
On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote: > Hi, > > Thanks for picking this up Jyri. > > On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote: > >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: > >> On 04/21/17 14:17, Ville Syrjälä wrote: > >> >> +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. > >> > > > I thought the conclusion of the last round was that on some hardware > this would be conflated as a conscious choice. If the HW doesn't have > the required degamma->csc->gamma stages, and the implementor/user > doesn't care about being a little incorrect, then it can all be > described in this single property. I was proposing the single prop approach initially, but now I think it might just lead to more confusion. So a dedicated property for each stage is the clearer design I think. We do lose potentially a bit of discoverability when not all combinations are supported, but we have that problem in many other places as well, so not a big deal I think. > > If there is more capable hardware with the additional stages, then > they should expose additional properties (in pipeline order): > > YCBCR_TO_RGB_*: > Does YCbCr->RGB conversion on non-linear YCbCr data, > only the enum values which don't have a CSC are exposed. Not sure what you mean but that last part. > > DEGAMMA: > Does the non-linear to linear conversion on the RGB data output from > the YCBCR_TO_RGB stage. > > RGB_TO_RGB_*: > Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC > on the linear data. We may want to call this just CTM to match the matching crtc prop. > > GAMMA: > Convert the CSC'd RGB data back in to non-linear for blending (if > blending is to be done with non-linear data). > > Drivers can expose as many or as few of the above properties as their > hardware supports. > > >> > >> I do not really have the expertise to argue with that. I merely copied > >> the idea from the mail thread I referred to in the cover letter. > >> However, there are several display HWs out there that do not have all > >> bolts and knobs to make the color-space conversion in exactly the ideal > >> order, omap DSS being one of them. > > > >Yeah. Intel hardware is in the same boat for the time being. On current > >hw I think we can only really expose the YCbCr->RGB and degamma stages. > > > >On some limited set of platforms we could expose a blob as well, and I > >suppose it would then be possible to use it for color space conversion > >if you ignore gamma and/or only deal with linear RGB data. But it's such > >a limited subset of hardware for us that I don't think I'm interested > >in exposing it. > > > > I'm not sure the custom blob is worth having either. It can easily be > added later if we decide we want it after all. > > Thanks, > -Brian > > >In the future we should be getting a more fully fleged pipeline. > > > >> > >> > 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. OTOH > >> > >> I guess this property should be called YCBCR_DECODING. > > > >Only if you think of it as a verb. > > > >> > >> > 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. > >> > >> In my first version it was called just CSC, but then I wanted to make it > >> explicit what this CSC was used for to avoid mixing the YCbCr decoding > >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces > >> of HW that can do only one or the other, e.g. the offset calculations > >> are supported only to one direction. > > > >Are you planning to do RGB->YCbCr conversion in the plane as well? I > >think we'll be only doing that at crtc/connector level. > > > >> > >> > I don't think most people are in the habit if cooking up new ways to > >> > encode their pixel data. > >> > > >> > >> In the embedded side I can imagine there could be some custom appliances > >> where one may want to do some custom thing with the CSC and not needing > >> a custom kernel for that could make a life easier... but then again I am > >> not really an expert in this area. > > > >I would assume most customy things you'd do in the crtc (eg. color > >correction and whatnot). But could be that I just lack imagination. > > > >-- > >Ville Syrjälä > >Intel OTC
On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote: >On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote: >> Hi, >> >> Thanks for picking this up Jyri. >> >> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote: >> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: >> >> On 04/21/17 14:17, Ville Syrjälä wrote: >> >> >> +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. >> >> > >> >> I thought the conclusion of the last round was that on some hardware >> this would be conflated as a conscious choice. If the HW doesn't have >> the required degamma->csc->gamma stages, and the implementor/user >> doesn't care about being a little incorrect, then it can all be >> described in this single property. > >I was proposing the single prop approach initially, but now I think it >might just lead to more confusion. So a dedicated property for each stage >is the clearer design I think. We do lose potentially a bit of >discoverability when not all combinations are supported, but we have >that problem in many other places as well, so not a big deal I think. > Yeah you lose discoverability, but do you also lose the ability to do the non-perfect, single-stage conversions? For HW that only has a matrix, is the driver expected to combine all of the separated stages down into a single matrix? Or it wouldn't expose the other properties, only a matrix, and userspace has to come up with a blob that does the (approximate) right thing? >> >> If there is more capable hardware with the additional stages, then >> they should expose additional properties (in pipeline order): >> >> YCBCR_TO_RGB_*: >> Does YCbCr->RGB conversion on non-linear YCbCr data, >> only the enum values which don't have a CSC are exposed. > >Not sure what you mean but that last part. > Just that the enum list should only contain things that are: YCbCr BT.601 -> RGB BT.601 YCbCr BT.709 -> RGB BT.709 YcbCr BT.2020 -> RGB BT.2020 and not a those including a color space change, e.g.: YCbCr BT.601 -> RGB BT.709 because an RGB BT.601 -> RGB BT.709 conversion can be performed later with the RGB_TO_RGB/CTM/whatever property. >> >> DEGAMMA: >> Does the non-linear to linear conversion on the RGB data output from >> the YCBCR_TO_RGB stage. >> >> RGB_TO_RGB_*: >> Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC >> on the linear data. > >We may want to call this just CTM to match the matching crtc prop. > Good call. -Brian >> >> GAMMA: >> Convert the CSC'd RGB data back in to non-linear for blending (if >> blending is to be done with non-linear data). >> >> Drivers can expose as many or as few of the above properties as their >> hardware supports. >> >> >> >> >> I do not really have the expertise to argue with that. I merely copied >> >> the idea from the mail thread I referred to in the cover letter. >> >> However, there are several display HWs out there that do not have all >> >> bolts and knobs to make the color-space conversion in exactly the ideal >> >> order, omap DSS being one of them. >> > >> >Yeah. Intel hardware is in the same boat for the time being. On current >> >hw I think we can only really expose the YCbCr->RGB and degamma stages. >> > >> >On some limited set of platforms we could expose a blob as well, and I >> >suppose it would then be possible to use it for color space conversion >> >if you ignore gamma and/or only deal with linear RGB data. But it's such >> >a limited subset of hardware for us that I don't think I'm interested >> >in exposing it. >> > >> >> I'm not sure the custom blob is worth having either. It can easily be >> added later if we decide we want it after all. >> >> Thanks, >> -Brian >> >> >In the future we should be getting a more fully fleged pipeline. >> > >> >> >> >> > 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. OTOH >> >> >> >> I guess this property should be called YCBCR_DECODING. >> > >> >Only if you think of it as a verb. >> > >> >> >> >> > 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. >> >> >> >> In my first version it was called just CSC, but then I wanted to make it >> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding >> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces >> >> of HW that can do only one or the other, e.g. the offset calculations >> >> are supported only to one direction. >> > >> >Are you planning to do RGB->YCbCr conversion in the plane as well? I >> >think we'll be only doing that at crtc/connector level. >> > >> >> >> >> > I don't think most people are in the habit if cooking up new ways to >> >> > encode their pixel data. >> >> > >> >> >> >> In the embedded side I can imagine there could be some custom appliances >> >> where one may want to do some custom thing with the CSC and not needing >> >> a custom kernel for that could make a life easier... but then again I am >> >> not really an expert in this area. >> > >> >I would assume most customy things you'd do in the crtc (eg. color >> >correction and whatnot). But could be that I just lack imagination. >> > >> >-- >> >Ville Syrjälä >> >Intel OTC > >-- >Ville Syrjälä >Intel OTC
On Fri, Apr 21, 2017 at 04:34:00PM +0100, Brian Starkey wrote: > On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote: > >On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote: > >> Hi, > >> > >> Thanks for picking this up Jyri. > >> > >> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote: > >> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: > >> >> On 04/21/17 14:17, Ville Syrjälä wrote: > >> >> >> +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. > >> >> > > >> > >> I thought the conclusion of the last round was that on some hardware > >> this would be conflated as a conscious choice. If the HW doesn't have > >> the required degamma->csc->gamma stages, and the implementor/user > >> doesn't care about being a little incorrect, then it can all be > >> described in this single property. > > > >I was proposing the single prop approach initially, but now I think it > >might just lead to more confusion. So a dedicated property for each stage > >is the clearer design I think. We do lose potentially a bit of > >discoverability when not all combinations are supported, but we have > >that problem in many other places as well, so not a big deal I think. > > > > Yeah you lose discoverability, but do you also lose the ability to do > the non-perfect, single-stage conversions? Not sure why one vs. multiple props should matter here. Either you accept the less than perfect pipeline or you don't. Whether you ask it via one of multiple props doesn't seem all that different to me. > > For HW that only has a matrix, is the driver expected to combine all > of the separated stages down into a single matrix? Or it wouldn't > expose the other properties, only a matrix, and userspace has to come > up with a blob that does the (approximate) right thing? If you're not happy with exposing just one or the other, then I guess you would expose both but indicate that there's no degamma in between and userspace can then choose whether it's happy with that solution or or not. > > >> > >> If there is more capable hardware with the additional stages, then > >> they should expose additional properties (in pipeline order): > >> > >> YCBCR_TO_RGB_*: > >> Does YCbCr->RGB conversion on non-linear YCbCr data, > >> only the enum values which don't have a CSC are exposed. > > > >Not sure what you mean but that last part. > > > > Just that the enum list should only contain things that are: > YCbCr BT.601 -> RGB BT.601 > YCbCr BT.709 -> RGB BT.709 > YcbCr BT.2020 -> RGB BT.2020 > > and not a those including a color space change, e.g.: > YCbCr BT.601 -> RGB BT.709 Right. We probably shouldn't even have the "BT.whatever" on both sides since it's not really a colorspace, just the encoding, and once you decoded the YCbCr into RGB it's just RGB. We could actually just define the enum values as "BT.601","BT.709" etc. > > because an RGB BT.601 -> RGB BT.709 conversion can be performed > later with the RGB_TO_RGB/CTM/whatever property. > > >> > >> DEGAMMA: > >> Does the non-linear to linear conversion on the RGB data output from > >> the YCBCR_TO_RGB stage. > >> > >> RGB_TO_RGB_*: > >> Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC > >> on the linear data. > > > >We may want to call this just CTM to match the matching crtc prop. > > > > Good call. > > -Brian > > >> > >> GAMMA: > >> Convert the CSC'd RGB data back in to non-linear for blending (if > >> blending is to be done with non-linear data). > >> > >> Drivers can expose as many or as few of the above properties as their > >> hardware supports. > >> > >> >> > >> >> I do not really have the expertise to argue with that. I merely copied > >> >> the idea from the mail thread I referred to in the cover letter. > >> >> However, there are several display HWs out there that do not have all > >> >> bolts and knobs to make the color-space conversion in exactly the ideal > >> >> order, omap DSS being one of them. > >> > > >> >Yeah. Intel hardware is in the same boat for the time being. On current > >> >hw I think we can only really expose the YCbCr->RGB and degamma stages. > >> > > >> >On some limited set of platforms we could expose a blob as well, and I > >> >suppose it would then be possible to use it for color space conversion > >> >if you ignore gamma and/or only deal with linear RGB data. But it's such > >> >a limited subset of hardware for us that I don't think I'm interested > >> >in exposing it. > >> > > >> > >> I'm not sure the custom blob is worth having either. It can easily be > >> added later if we decide we want it after all. > >> > >> Thanks, > >> -Brian > >> > >> >In the future we should be getting a more fully fleged pipeline. > >> > > >> >> > >> >> > 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. OTOH > >> >> > >> >> I guess this property should be called YCBCR_DECODING. > >> > > >> >Only if you think of it as a verb. > >> > > >> >> > >> >> > 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. > >> >> > >> >> In my first version it was called just CSC, but then I wanted to make it > >> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding > >> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces > >> >> of HW that can do only one or the other, e.g. the offset calculations > >> >> are supported only to one direction. > >> > > >> >Are you planning to do RGB->YCbCr conversion in the plane as well? I > >> >think we'll be only doing that at crtc/connector level. > >> > > >> >> > >> >> > I don't think most people are in the habit if cooking up new ways to > >> >> > encode their pixel data. > >> >> > > >> >> > >> >> In the embedded side I can imagine there could be some custom appliances > >> >> where one may want to do some custom thing with the CSC and not needing > >> >> a custom kernel for that could make a life easier... but then again I am > >> >> not really an expert in this area. > >> > > >> >I would assume most customy things you'd do in the crtc (eg. color > >> >correction and whatnot). But could be that I just lack imagination. > >> > > >> >-- > >> >Ville Syrjälä > >> >Intel OTC > > > >-- > >Ville Syrjälä > >Intel OTC
On Fri, Apr 21, 2017 at 07:49:04PM +0300, Ville Syrjälä wrote: >On Fri, Apr 21, 2017 at 04:34:00PM +0100, Brian Starkey wrote: >> On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote: >> >On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote: >> >> Hi, >> >> >> >> Thanks for picking this up Jyri. >> >> >> >> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote: >> >> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: >> >> >> On 04/21/17 14:17, Ville Syrjälä wrote: >> >> >> >> +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. >> >> >> > >> >> >> >> I thought the conclusion of the last round was that on some hardware >> >> this would be conflated as a conscious choice. If the HW doesn't have >> >> the required degamma->csc->gamma stages, and the implementor/user >> >> doesn't care about being a little incorrect, then it can all be >> >> described in this single property. >> > >> >I was proposing the single prop approach initially, but now I think it >> >might just lead to more confusion. So a dedicated property for each stage >> >is the clearer design I think. We do lose potentially a bit of >> >discoverability when not all combinations are supported, but we have >> >that problem in many other places as well, so not a big deal I think. >> > >> >> Yeah you lose discoverability, but do you also lose the ability to do >> the non-perfect, single-stage conversions? > >Not sure why one vs. multiple props should matter here. Either you >accept the less than perfect pipeline or you don't. Whether you ask it >via one of multiple props doesn't seem all that different to me. > >> >> For HW that only has a matrix, is the driver expected to combine all >> of the separated stages down into a single matrix? Or it wouldn't >> expose the other properties, only a matrix, and userspace has to come >> up with a blob that does the (approximate) right thing? > >If you're not happy with exposing just one or the other, then I guess >you would expose both but indicate that there's no degamma in between >and userspace can then choose whether it's happy with that solution or >or not. > I might understand/explain better if we take a concrete example. Let's assume I have a hardware pipeline with the following bits: Input -> 3x3 + 3 matrix -> degamma LUT -> CRTC My input is non-linear YCbCr BT.601 full-range, and I want linear RGB BT.709, full-range to reach the CRTC. Which properties would my driver expose, and which values should be set on them? My assumption /was/ that: 3x3 + 3 matrix: Exposed as YCBCR_TO_RGB_MODE = "YCbCr BT.601 full-range to RGB BT.709 full-range" degamma LUT: Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma curve Are you suggesting instead that: 3x3 + 3 matrix: Exposed as YCBCR_TO_RGB_MODE = "YCbCr TO RGB CSC full range preoffset" with YCBCR_TO_RGB_CSC = <userspace-derived-matrix> degamma LUT: Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma curve Thanks, Brian >> >> >> >> >> If there is more capable hardware with the additional stages, then >> >> they should expose additional properties (in pipeline order): >> >> >> >> YCBCR_TO_RGB_*: >> >> Does YCbCr->RGB conversion on non-linear YCbCr data, >> >> only the enum values which don't have a CSC are exposed. >> > >> >Not sure what you mean but that last part. >> > >> >> Just that the enum list should only contain things that are: >> YCbCr BT.601 -> RGB BT.601 >> YCbCr BT.709 -> RGB BT.709 >> YcbCr BT.2020 -> RGB BT.2020 >> >> and not a those including a color space change, e.g.: >> YCbCr BT.601 -> RGB BT.709 > >Right. We probably shouldn't even have the "BT.whatever" on both sides >since it's not really a colorspace, just the encoding, and once you >decoded the YCbCr into RGB it's just RGB. We could actually just define >the enum values as "BT.601","BT.709" etc. > >> >> because an RGB BT.601 -> RGB BT.709 conversion can be performed >> later with the RGB_TO_RGB/CTM/whatever property. >> >> >> >> >> DEGAMMA: >> >> Does the non-linear to linear conversion on the RGB data output from >> >> the YCBCR_TO_RGB stage. >> >> >> >> RGB_TO_RGB_*: >> >> Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC >> >> on the linear data. >> > >> >We may want to call this just CTM to match the matching crtc prop. >> > >> >> Good call. >> >> -Brian >> >> >> >> >> GAMMA: >> >> Convert the CSC'd RGB data back in to non-linear for blending (if >> >> blending is to be done with non-linear data). >> >> >> >> Drivers can expose as many or as few of the above properties as their >> >> hardware supports. >> >> >> >> >> >> >> >> I do not really have the expertise to argue with that. I merely copied >> >> >> the idea from the mail thread I referred to in the cover letter. >> >> >> However, there are several display HWs out there that do not have all >> >> >> bolts and knobs to make the color-space conversion in exactly the ideal >> >> >> order, omap DSS being one of them. >> >> > >> >> >Yeah. Intel hardware is in the same boat for the time being. On current >> >> >hw I think we can only really expose the YCbCr->RGB and degamma stages. >> >> > >> >> >On some limited set of platforms we could expose a blob as well, and I >> >> >suppose it would then be possible to use it for color space conversion >> >> >if you ignore gamma and/or only deal with linear RGB data. But it's such >> >> >a limited subset of hardware for us that I don't think I'm interested >> >> >in exposing it. >> >> > >> >> >> >> I'm not sure the custom blob is worth having either. It can easily be >> >> added later if we decide we want it after all. >> >> >> >> Thanks, >> >> -Brian >> >> >> >> >In the future we should be getting a more fully fleged pipeline. >> >> > >> >> >> >> >> >> > 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. OTOH >> >> >> >> >> >> I guess this property should be called YCBCR_DECODING. >> >> > >> >> >Only if you think of it as a verb. >> >> > >> >> >> >> >> >> > 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. >> >> >> >> >> >> In my first version it was called just CSC, but then I wanted to make it >> >> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding >> >> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces >> >> >> of HW that can do only one or the other, e.g. the offset calculations >> >> >> are supported only to one direction. >> >> > >> >> >Are you planning to do RGB->YCbCr conversion in the plane as well? I >> >> >think we'll be only doing that at crtc/connector level. >> >> > >> >> >> >> >> >> > I don't think most people are in the habit if cooking up new ways to >> >> >> > encode their pixel data. >> >> >> > >> >> >> >> >> >> In the embedded side I can imagine there could be some custom appliances >> >> >> where one may want to do some custom thing with the CSC and not needing >> >> >> a custom kernel for that could make a life easier... but then again I am >> >> >> not really an expert in this area. >> >> > >> >> >I would assume most customy things you'd do in the crtc (eg. color >> >> >correction and whatnot). But could be that I just lack imagination. >> >> > >> >> >-- >> >> >Ville Syrjälä >> >> >Intel OTC >> > >> >-- >> >Ville Syrjälä >> >Intel OTC > >-- >Ville Syrjälä >Intel OTC
On 04/21/17 16:52, Ville Syrjälä wrote: > On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: >> On 04/21/17 14:17, Ville Syrjälä wrote: >>>> +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. >>> >> >> I do not really have the expertise to argue with that. I merely copied >> the idea from the mail thread I referred to in the cover letter. >> However, there are several display HWs out there that do not have all >> bolts and knobs to make the color-space conversion in exactly the ideal >> order, omap DSS being one of them. > > Yeah. Intel hardware is in the same boat for the time being. On current > hw I think we can only really expose the YCbCr->RGB and degamma stages. > > On some limited set of platforms we could expose a blob as well, and I > suppose it would then be possible to use it for color space conversion > if you ignore gamma and/or only deal with linear RGB data. But it's such > a limited subset of hardware for us that I don't think I'm interested > in exposing it. > > In the future we should be getting a more fully fleged pipeline. > If going forward with these patches, then maybe it is better to stick with the enum options that we are sure of, e.g. BT.601, BT.709, and BT.2020 to full range RGB. It is easy enough to add more enum values in the future. What is more important is the naming approach, whether we keep the conversion mode naming explicit about the output format, or just specify the output format implicitly to be always full range (non linear) RGB? >> >>> 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. OTOH >> >> I guess this property should be called YCBCR_DECODING. > > Only if you think of it as a verb. > No strong opinions here. I am fine with any of the names that have been suggested so far. >> >>> 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. >> >> In my first version it was called just CSC, but then I wanted to make it >> explicit what this CSC was used for to avoid mixing the YCbCr decoding >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces >> of HW that can do only one or the other, e.g. the offset calculations >> are supported only to one direction. > > Are you planning to do RGB->YCbCr conversion in the plane as well? I > think we'll be only doing that at crtc/connector level. > No, but we need such a property to CRTC output, and there we certainly need to expose the CSC, because we do not have CTM (before gamma) matrix, and may have to use the output CSC in its place. >> >>> I don't think most people are in the habit if cooking up new ways to >>> encode their pixel data. >>> >> >> In the embedded side I can imagine there could be some custom appliances >> where one may want to do some custom thing with the CSC and not needing >> a custom kernel for that could make a life easier... but then again I am >> not really an expert in this area. > > I would assume most customy things you'd do in the crtc (eg. color > correction and whatnot). But could be that I just lack imagination. > I am on thin ice here :), but surely there is no harm in specifying an optional property for exposing the CSC as many display HWs provide a way to set an arbitrary CSC. What about the uapi structs? In the patch there is an explicit struct naming each coefficient for what they are for in YCbCr to RGB conversion. Is this Ok, or should we go with a generic (CTM style) matrix, that could be used for RGB to YCbCr conversion too? Best regards, Jyri
On Mon, Apr 24, 2017 at 04:21:39PM +0300, Jyri Sarha wrote: > On 04/21/17 16:52, Ville Syrjälä wrote: > > On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote: > >> On 04/21/17 14:17, Ville Syrjälä wrote: > >>>> +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. > >>> > >> > >> I do not really have the expertise to argue with that. I merely copied > >> the idea from the mail thread I referred to in the cover letter. > >> However, there are several display HWs out there that do not have all > >> bolts and knobs to make the color-space conversion in exactly the ideal > >> order, omap DSS being one of them. > > > > Yeah. Intel hardware is in the same boat for the time being. On current > > hw I think we can only really expose the YCbCr->RGB and degamma stages. > > > > On some limited set of platforms we could expose a blob as well, and I > > suppose it would then be possible to use it for color space conversion > > if you ignore gamma and/or only deal with linear RGB data. But it's such > > a limited subset of hardware for us that I don't think I'm interested > > in exposing it. > > > > In the future we should be getting a more fully fleged pipeline. > > > > If going forward with these patches, then maybe it is better to stick > with the enum options that we are sure of, e.g. BT.601, BT.709, and > BT.2020 to full range RGB. It is easy enough to add more enum values in > the future. > > What is more important is the naming approach, whether we keep the > conversion mode naming explicit about the output format, or just specify > the output format implicitly to be always full range (non linear) RGB? > > >> > >>> 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. OTOH > >> > >> I guess this property should be called YCBCR_DECODING. > > > > Only if you think of it as a verb. > > > > No strong opinions here. I am fine with any of the names that have been > suggested so far. > > >> > >>> 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. > >> > >> In my first version it was called just CSC, but then I wanted to make it > >> explicit what this CSC was used for to avoid mixing the YCbCr decoding > >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces > >> of HW that can do only one or the other, e.g. the offset calculations > >> are supported only to one direction. > > > > Are you planning to do RGB->YCbCr conversion in the plane as well? I > > think we'll be only doing that at crtc/connector level. > > > > No, but we need such a property to CRTC output, and there we certainly > need to expose the CSC, because we do not have CTM (before gamma) > matrix, and may have to use the output CSC in its place. We're sort of in the same boat. We have just one matrix currently, but the gamma can be either before, after, or both. So mixing CTM and YCbCr output is a bit of a challenge, especially when gamma is involved. Shashank has recently posted a patch set to do YCbCr output with i915. So what we're going to be doing is using our matrix as the output CSC or CTM as needed. And if the user asks to use both, well, then we get to either multiply the matrices together (assuming user didn't want some gamma in between) or we just refuse the entire thing. So I think better to just expose the standard properties and map the hardware resources to those dynamically. Otherwise there's going to be too many ways to do things, and that just leads to confusion for userspace. > > >> > >>> I don't think most people are in the habit if cooking up new ways to > >>> encode their pixel data. > >>> > >> > >> In the embedded side I can imagine there could be some custom appliances > >> where one may want to do some custom thing with the CSC and not needing > >> a custom kernel for that could make a life easier... but then again I am > >> not really an expert in this area. > > > > I would assume most customy things you'd do in the crtc (eg. color > > correction and whatnot). But could be that I just lack imagination. > > > > I am on thin ice here :), but surely there is no harm in specifying an > optional property for exposing the CSC as many display HWs provide a way > to set an arbitrary CSC. Well, if we don't have a clear use case we're more likely to specify it wrong. And when the real use case appears we might discover that what we specified isn't good enough. Hence I would avoid leaning forward too heavily. > > What about the uapi structs? In the patch there is an explicit struct > naming each coefficient for what they are for in YCbCr to RGB > conversion. Is this Ok, or should we go with a generic (CTM style) > matrix, that could be used for RGB to YCbCr conversion too? Not sure what we're talking about here, but like I said I think we should stick to a fairly limited set of standard props and just have each driver map the hardware resources to them as best they can. If you just have csc+(de)gamma then I guess it might make sense to just expose the YCbCr->RGB and degamma. If you have degamma+csc+gamma then it might make sense to expose both YCbCr->RGB, degamma, CTM, and gamma, and just refuse any combination that can't be done. Eg. can't do YCbCr->RGB if degamma is used, and YCbCr->RGB + CTM would require multiplying the matrices together which you may or may not want to bother with, I guess we could try to put some matrix math helpers into the core to make such things less painful for drivers?
On 04/24/17 18:13, Ville Syrjälä wrote: >> What about the uapi structs? In the patch there is an explicit struct >> naming each coefficient for what they are for in YCbCr to RGB >> conversion. Is this Ok, or should we go with a generic (CTM style) >> matrix, that could be used for RGB to YCbCr conversion too? > Not sure what we're talking about here, but like I said I think we > should stick to a fairly limited set of standard props and just have > each driver map the hardware resources to them as best they can. > Just about the implementation detail, if we should have a separate uapi struct for YCbCr to RGB CSC and RGB to YCbCr CSC. If we are going to use the same struct, then we could as well use the already existing CTM struct. > If you just have csc+(de)gamma then I guess it might make sense > to just expose the YCbCr->RGB and degamma. If you have > degamma+csc+gamma then it might make sense to expose both > YCbCr->RGB, degamma, CTM, and gamma, and just refuse any > combination that can't be done. Eg. can't do YCbCr->RGB if > degamma is used, and YCbCr->RGB + CTM would require multiplying > the matrices together which you may or may not want to bother > with, I guess we could try to put some matrix math helpers into > the core to make such things less painful for drivers? In fact we have plane specific YCbCr to RGB CSC (only preoffset possible), then (per crtc) gamma table, and finally a (per crtc) RGB to YCbCr CSC with optional post offset (so it can be used either as CSC or CTM). Cheers, Jyri
On Mon, Apr 24, 2017 at 06:49:04PM +0300, Jyri Sarha wrote: > On 04/24/17 18:13, Ville Syrjälä wrote: > >> What about the uapi structs? In the patch there is an explicit struct > >> naming each coefficient for what they are for in YCbCr to RGB > >> conversion. Is this Ok, or should we go with a generic (CTM style) > >> matrix, that could be used for RGB to YCbCr conversion too? > > Not sure what we're talking about here, but like I said I think we > > should stick to a fairly limited set of standard props and just have > > each driver map the hardware resources to them as best they can. > > > > Just about the implementation detail, if we should have a separate uapi > struct for YCbCr to RGB CSC and RGB to YCbCr CSC. If we are going to use > the same struct, then we could as well use the already existing CTM struct. > > > If you just have csc+(de)gamma then I guess it might make sense > > to just expose the YCbCr->RGB and degamma. If you have > > degamma+csc+gamma then it might make sense to expose both > > YCbCr->RGB, degamma, CTM, and gamma, and just refuse any > > combination that can't be done. Eg. can't do YCbCr->RGB if > > degamma is used, and YCbCr->RGB + CTM would require multiplying > > the matrices together which you may or may not want to bother > > with, I guess we could try to put some matrix math helpers into > > the core to make such things less painful for drivers? > > In fact we have plane specific YCbCr to RGB CSC (only preoffset > possible), then (per crtc) gamma table, and finally a (per crtc) RGB to > YCbCr CSC with optional post offset (so it can be used either as CSC or > CTM). So with that plane hw you could perhaps do: - YCbCr->RGB if you input is not linear, but then you must blend using non-linear data - colorspace conversion if your input is alredy linear And with your crtc hw you could do: - degamma + CTM - gamma + RGB->YCbCr
On 04/24/17 19:55, Ville Syrjälä wrote: >> In fact we have plane specific YCbCr to RGB CSC (only preoffset >> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to >> YCbCr CSC with optional post offset (so it can be used either as CSC or >> CTM). > So with that plane hw you could perhaps do: > - YCbCr->RGB if you input is not linear, but then you must > blend using non-linear data > - colorspace conversion if your input is alredy linear > > And with your crtc hw you could do: > - degamma + CTM > - gamma + RGB->YCbCr Just a generic question. Shouldn't - in an ideal HW - the degamma phase and the CTM be a plane specific property? I mean, isn't the purpose of normalizing the non linear RGB to linear (and possibly converting the color space) to have same format for all plane data before blending and composing them together? Of course it does not matter if all the planes use the same color space, which then should be converted to something else for the output. ... or have I misunderstood something? Cheers, Jyri
Regards Shashank On 4/26/2017 6:26 PM, Jyri Sarha wrote: > On 04/24/17 19:55, Ville Syrjälä wrote: >>> In fact we have plane specific YCbCr to RGB CSC (only preoffset >>> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to >>> YCbCr CSC with optional post offset (so it can be used either as CSC or >>> CTM). >> So with that plane hw you could perhaps do: >> - YCbCr->RGB if you input is not linear, but then you must >> blend using non-linear data >> - colorspace conversion if your input is alredy linear >> >> And with your crtc hw you could do: >> - degamma + CTM >> - gamma + RGB->YCbCr > > Just a generic question. Shouldn't - in an ideal HW - the degamma phase > and the CTM be a plane specific property? > > I mean, isn't the purpose of normalizing the non linear RGB to linear > (and possibly converting the color space) to have same format for all > plane data before blending and composing them together? I think Ville's point is something else. There are two types of color operations possible: - color space matching so that you can blend the plane data accurately. - color correction or transformation to enhance display, or change output from one format to other format. Now, when you want to blend, you have to make sure that, you blend among the same, that means: - all the framebuffers should be in one state, either linear or non-linear - all the framebuffers should be in one color space (REC 709 or 601 or 2020), else you have to do gamut mapping - Gamut mapping is possible on Linear data only. Else, if we try to blend one REC2020 buffer and one REC709 buffer, its like adding 2CM + 3MM and making = 5CM So, we can use the plane level properties in such a way that, you should have similar data to the input of the blender (linear Or non-linear) And then, once blending is done, you can use CRTC level operations for color correction and transformation (Like RGB->YCBCR conversion using CRTC level CSC, for YCBCR420 kind of outputs) Does it helps in explanation ? - Shashank > > Of course it does not matter if all the planes use the same color space, > which then should be converted to something else for the output. > > ... or have I misunderstood something? > > Cheers, > Jyri
On Wed, Apr 26, 2017 at 03:56:54PM +0300, Jyri Sarha wrote: > On 04/24/17 19:55, Ville Syrjälä wrote: > >> In fact we have plane specific YCbCr to RGB CSC (only preoffset > >> possible), then (per crtc) gamma table, and finally a (per crtc) RGB to > >> YCbCr CSC with optional post offset (so it can be used either as CSC or > >> CTM). > > So with that plane hw you could perhaps do: > > - YCbCr->RGB if you input is not linear, but then you must > > blend using non-linear data > > - colorspace conversion if your input is alredy linear > > > > And with your crtc hw you could do: > > - degamma + CTM > > - gamma + RGB->YCbCr > > > Just a generic question. Shouldn't - in an ideal HW - the degamma phase > and the CTM be a plane specific property? > > I mean, isn't the purpose of normalizing the non linear RGB to linear > (and possibly converting the color space) to have same format for all > plane data before blending and composing them together? > > Of course it does not matter if all the planes use the same color space, > which then should be converted to something else for the output. > > ... or have I misunderstood something? I think the pipeline we want to expose is planes: crtc: YCbCr->RGB -> degamma -> CTM -> (gamma) \ YCbCr->RGB -> degamma -> CTM -> (gamma) -> (degamma) -> CTM -> gamma -> RGB->YCbCr ... / I put the plane gamma and crtc degamma in parentheses because ideally you perhaps wouldn't need them (since you want to blend with linear data). But we have hardware which has them, and might be lacking some of the other stages so they can actually be useful. Eg. if you don't have plane gamma/degamma and you want to use the crtc CTM to change the colorspace you would also use the crtc degamma.
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@ti.com> Just to make sure there's no surprises: We need the userspace for this too. -modesetting (for Xv maybe), some wayland compositor (weston, ozone, whatever you feel like) or drm_hwcomposer. But yeah might be good to bikeshed the uabi first a bit more and get at least some agreement on that. Thanks, Daniel > --- > 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(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index f881319..d1512aa 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > { > struct drm_device *dev = plane->dev; > struct drm_mode_config *config = &dev->mode_config; > + int ret; > + bool dummy; > > if (property == config->prop_fb_id) { > struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); > @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > state->rotation = val; > } else if (property == plane->zpos_property) { > state->zpos = val; > + } else if (property == plane->ycbcr_to_rgb_mode_property) { > + state->ycbcr_to_rgb_mode = val; > + } else if (property == plane->ycbcr_to_rgb_csc_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ycbcr_to_rgb_csc, > + val, > + sizeof(struct drm_ycbcr_to_rgb_csc), > + &dummy); > + return ret; > + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ycbcr_to_rgb_preoffset, > + val, > + sizeof(struct drm_ycbcr_to_rgb_preoffset), > + &dummy); > + return ret; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > *val = state->rotation; > } else if (property == plane->zpos_property) { > *val = state->zpos; > + } else if (property == plane->ycbcr_to_rgb_mode_property) { > + *val = state->ycbcr_to_rgb_mode; > + } else if (property == plane->ycbcr_to_rgb_csc_property) { > + *val = state->ycbcr_to_rgb_csc ? > + state->ycbcr_to_rgb_csc->base.id : 0; > + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { > + *val = state->ycbcr_to_rgb_preoffset ? > + state->ycbcr_to_rgb_preoffset->base.id : 0; > } else if (plane->funcs->atomic_get_property) { > return plane->funcs->atomic_get_property(plane, state, property, val); > } else { > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index c3994b4..89fd826 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > { > memcpy(state, plane->state, sizeof(*state)); > > + if (state->ycbcr_to_rgb_csc) > + drm_property_blob_get(state->ycbcr_to_rgb_csc); > + > + if (state->ycbcr_to_rgb_preoffset) > + drm_property_blob_get(state->ycbcr_to_rgb_preoffset); > + > if (state->fb) > drm_framebuffer_get(state->fb); > > @@ -3236,6 +3242,9 @@ struct drm_plane_state * > */ > void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > { > + drm_property_blob_put(state->ycbcr_to_rgb_csc); > + drm_property_blob_put(state->ycbcr_to_rgb_preoffset); > + > if (state->fb) > drm_framebuffer_put(state->fb); > > 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", > + [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; > + > + 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; > +} > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index bc71aa2..4c7e827 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane) > > kfree(plane->name); > > + if (plane->ycbcr_to_rgb_mode_property) > + drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property); > + > + if (plane->ycbcr_to_rgb_csc_property) > + drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property); > + > + if (plane->ycbcr_to_rgb_preoffset_property) > + drm_property_destroy(dev, > + plane->ycbcr_to_rgb_preoffset_property); > + > memset(plane, 0, sizeof(*plane)); > } > EXPORT_SYMBOL(drm_plane_cleanup); > 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 > @@ -26,6 +26,8 @@ > #include <linux/ctype.h> > > struct drm_crtc; > +struct drm_plane; > +struct drm_prop_enum_list; > > uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision); > > @@ -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, > +}; > + > +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 > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 9ab3e70..41dcde2 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -26,6 +26,7 @@ > #include <linux/list.h> > #include <linux/ctype.h> > #include <drm/drm_mode_object.h> > +#include <drm/drm_color_mgmt.h> > > struct drm_crtc; > struct drm_printer; > @@ -112,6 +113,11 @@ struct drm_plane_state { > unsigned int zpos; > unsigned int normalized_zpos; > > + /* YCbCr to RGB conversion */ > + enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode; > + struct drm_property_blob *ycbcr_to_rgb_csc; > + struct drm_property_blob *ycbcr_to_rgb_preoffset; > + > /* Clipped coordinates */ > struct drm_rect src, dst; > > @@ -523,6 +529,10 @@ struct drm_plane { > > struct drm_property *zpos_property; > struct drm_property *rotation_property; > + > + struct drm_property *ycbcr_to_rgb_mode_property; > + struct drm_property *ycbcr_to_rgb_csc_property; > + struct drm_property *ycbcr_to_rgb_preoffset_property; > }; > > #define obj_to_plane(x) container_of(x, struct drm_plane, base) > 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 > -- > 1.9.1 >
On 05/02/17 11:33, Daniel Vetter 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@ti.com> > > Just to make sure there's no surprises: We need the userspace for this > too. -modesetting (for Xv maybe), some wayland compositor (weston, ozone, > whatever you feel like) or drm_hwcomposer. > > But yeah might be good to bikeshed the uabi first a bit more and get at > least some agreement on that. > In the first phase I have been using kms++ [1]. And when/if we have an agreement about the API, I will push my patches there. With X, wayland, and other compositors, we would in the end need some video player supporting the different YCbCr encodings according to the video stream. This sounds like a relatively big task, but surely I volunteer to assist, when we get there. Cheer, Jyri [1] https://github.com/tomba/kmsxx/ > Thanks, Daniel >> --- >> 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(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index f881319..d1512aa 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >> { >> struct drm_device *dev = plane->dev; >> struct drm_mode_config *config = &dev->mode_config; >> + int ret; >> + bool dummy; >> >> if (property == config->prop_fb_id) { >> struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); >> @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >> state->rotation = val; >> } else if (property == plane->zpos_property) { >> state->zpos = val; >> + } else if (property == plane->ycbcr_to_rgb_mode_property) { >> + state->ycbcr_to_rgb_mode = val; >> + } else if (property == plane->ycbcr_to_rgb_csc_property) { >> + ret = drm_atomic_replace_property_blob_from_id(dev, >> + &state->ycbcr_to_rgb_csc, >> + val, >> + sizeof(struct drm_ycbcr_to_rgb_csc), >> + &dummy); >> + return ret; >> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { >> + ret = drm_atomic_replace_property_blob_from_id(dev, >> + &state->ycbcr_to_rgb_preoffset, >> + val, >> + sizeof(struct drm_ycbcr_to_rgb_preoffset), >> + &dummy); >> + return ret; >> } else if (plane->funcs->atomic_set_property) { >> return plane->funcs->atomic_set_property(plane, state, >> property, val); >> @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >> *val = state->rotation; >> } else if (property == plane->zpos_property) { >> *val = state->zpos; >> + } else if (property == plane->ycbcr_to_rgb_mode_property) { >> + *val = state->ycbcr_to_rgb_mode; >> + } else if (property == plane->ycbcr_to_rgb_csc_property) { >> + *val = state->ycbcr_to_rgb_csc ? >> + state->ycbcr_to_rgb_csc->base.id : 0; >> + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { >> + *val = state->ycbcr_to_rgb_preoffset ? >> + state->ycbcr_to_rgb_preoffset->base.id : 0; >> } else if (plane->funcs->atomic_get_property) { >> return plane->funcs->atomic_get_property(plane, state, property, val); >> } else { >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index c3994b4..89fd826 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >> { >> memcpy(state, plane->state, sizeof(*state)); >> >> + if (state->ycbcr_to_rgb_csc) >> + drm_property_blob_get(state->ycbcr_to_rgb_csc); >> + >> + if (state->ycbcr_to_rgb_preoffset) >> + drm_property_blob_get(state->ycbcr_to_rgb_preoffset); >> + >> if (state->fb) >> drm_framebuffer_get(state->fb); >> >> @@ -3236,6 +3242,9 @@ struct drm_plane_state * >> */ >> void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >> { >> + drm_property_blob_put(state->ycbcr_to_rgb_csc); >> + drm_property_blob_put(state->ycbcr_to_rgb_preoffset); >> + >> if (state->fb) >> drm_framebuffer_put(state->fb); >> >> 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", >> + [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; >> + >> + 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; >> +} >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c >> index bc71aa2..4c7e827 100644 >> --- a/drivers/gpu/drm/drm_plane.c >> +++ b/drivers/gpu/drm/drm_plane.c >> @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane) >> >> kfree(plane->name); >> >> + if (plane->ycbcr_to_rgb_mode_property) >> + drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property); >> + >> + if (plane->ycbcr_to_rgb_csc_property) >> + drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property); >> + >> + if (plane->ycbcr_to_rgb_preoffset_property) >> + drm_property_destroy(dev, >> + plane->ycbcr_to_rgb_preoffset_property); >> + >> memset(plane, 0, sizeof(*plane)); >> } >> EXPORT_SYMBOL(drm_plane_cleanup); >> 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 >> @@ -26,6 +26,8 @@ >> #include <linux/ctype.h> >> >> struct drm_crtc; >> +struct drm_plane; >> +struct drm_prop_enum_list; >> >> uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision); >> >> @@ -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, >> +}; >> + >> +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 >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index 9ab3e70..41dcde2 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -26,6 +26,7 @@ >> #include <linux/list.h> >> #include <linux/ctype.h> >> #include <drm/drm_mode_object.h> >> +#include <drm/drm_color_mgmt.h> >> >> struct drm_crtc; >> struct drm_printer; >> @@ -112,6 +113,11 @@ struct drm_plane_state { >> unsigned int zpos; >> unsigned int normalized_zpos; >> >> + /* YCbCr to RGB conversion */ >> + enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode; >> + struct drm_property_blob *ycbcr_to_rgb_csc; >> + struct drm_property_blob *ycbcr_to_rgb_preoffset; >> + >> /* Clipped coordinates */ >> struct drm_rect src, dst; >> >> @@ -523,6 +529,10 @@ struct drm_plane { >> >> struct drm_property *zpos_property; >> struct drm_property *rotation_property; >> + >> + struct drm_property *ycbcr_to_rgb_mode_property; >> + struct drm_property *ycbcr_to_rgb_csc_property; >> + struct drm_property *ycbcr_to_rgb_preoffset_property; >> }; >> >> #define obj_to_plane(x) container_of(x, struct drm_plane, base) >> 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 >> -- >> 1.9.1 >> >
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index f881319..d1512aa 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -732,6 +732,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, { struct drm_device *dev = plane->dev; struct drm_mode_config *config = &dev->mode_config; + int ret; + bool dummy; if (property == config->prop_fb_id) { struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); @@ -774,6 +776,22 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, state->rotation = val; } else if (property == plane->zpos_property) { state->zpos = val; + } else if (property == plane->ycbcr_to_rgb_mode_property) { + state->ycbcr_to_rgb_mode = val; + } else if (property == plane->ycbcr_to_rgb_csc_property) { + ret = drm_atomic_replace_property_blob_from_id(dev, + &state->ycbcr_to_rgb_csc, + val, + sizeof(struct drm_ycbcr_to_rgb_csc), + &dummy); + return ret; + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { + ret = drm_atomic_replace_property_blob_from_id(dev, + &state->ycbcr_to_rgb_preoffset, + val, + sizeof(struct drm_ycbcr_to_rgb_preoffset), + &dummy); + return ret; } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); @@ -834,6 +852,14 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, *val = state->rotation; } else if (property == plane->zpos_property) { *val = state->zpos; + } else if (property == plane->ycbcr_to_rgb_mode_property) { + *val = state->ycbcr_to_rgb_mode; + } else if (property == plane->ycbcr_to_rgb_csc_property) { + *val = state->ycbcr_to_rgb_csc ? + state->ycbcr_to_rgb_csc->base.id : 0; + } else if (property == plane->ycbcr_to_rgb_preoffset_property) { + *val = state->ycbcr_to_rgb_preoffset ? + state->ycbcr_to_rgb_preoffset->base.id : 0; } else if (plane->funcs->atomic_get_property) { return plane->funcs->atomic_get_property(plane, state, property, val); } else { diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index c3994b4..89fd826 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3196,6 +3196,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, { memcpy(state, plane->state, sizeof(*state)); + if (state->ycbcr_to_rgb_csc) + drm_property_blob_get(state->ycbcr_to_rgb_csc); + + if (state->ycbcr_to_rgb_preoffset) + drm_property_blob_get(state->ycbcr_to_rgb_preoffset); + if (state->fb) drm_framebuffer_get(state->fb); @@ -3236,6 +3242,9 @@ struct drm_plane_state * */ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) { + drm_property_blob_put(state->ycbcr_to_rgb_csc); + drm_property_blob_put(state->ycbcr_to_rgb_preoffset); + if (state->fb) drm_framebuffer_put(state->fb); 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", + [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; + + 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; +} diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index bc71aa2..4c7e827 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -244,6 +244,16 @@ void drm_plane_cleanup(struct drm_plane *plane) kfree(plane->name); + if (plane->ycbcr_to_rgb_mode_property) + drm_property_destroy(dev, plane->ycbcr_to_rgb_mode_property); + + if (plane->ycbcr_to_rgb_csc_property) + drm_property_destroy(dev, plane->ycbcr_to_rgb_csc_property); + + if (plane->ycbcr_to_rgb_preoffset_property) + drm_property_destroy(dev, + plane->ycbcr_to_rgb_preoffset_property); + memset(plane, 0, sizeof(*plane)); } EXPORT_SYMBOL(drm_plane_cleanup); 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 @@ -26,6 +26,8 @@ #include <linux/ctype.h> struct drm_crtc; +struct drm_plane; +struct drm_prop_enum_list; uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision); @@ -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, +}; + +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 diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 9ab3e70..41dcde2 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -26,6 +26,7 @@ #include <linux/list.h> #include <linux/ctype.h> #include <drm/drm_mode_object.h> +#include <drm/drm_color_mgmt.h> struct drm_crtc; struct drm_printer; @@ -112,6 +113,11 @@ struct drm_plane_state { unsigned int zpos; unsigned int normalized_zpos; + /* YCbCr to RGB conversion */ + enum drm_plane_ycbcr_to_rgb_mode ycbcr_to_rgb_mode; + struct drm_property_blob *ycbcr_to_rgb_csc; + struct drm_property_blob *ycbcr_to_rgb_preoffset; + /* Clipped coordinates */ struct drm_rect src, dst; @@ -523,6 +529,10 @@ struct drm_plane { struct drm_property *zpos_property; struct drm_property *rotation_property; + + struct drm_property *ycbcr_to_rgb_mode_property; + struct drm_property *ycbcr_to_rgb_csc_property; + struct drm_property *ycbcr_to_rgb_preoffset_property; }; #define obj_to_plane(x) container_of(x, struct drm_plane, base) 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
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@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(-)