| Message ID | 24ec5f778bf658c11561d36b416523ac2b978f7c.1493881618.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: > Add standard optinal property blobs for YCbCr to RGB conversion CSC > matrix and YCbCr preoffset vector in DRM planes. New enums are defined > to YCBCR_ENCODING property to activate the CSC and preoffset > properties. For simplicity the new properties are stored in the > drm_plane object, because the YCBCR_ENCODING is already there. The > blob contents are defined in the uapi/drm/drm_mode.h header. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> Not sure we want to make this yuv2rgb specific, plenty of planes have generic degamma/offset, csc, gamme/offset hw. I think what we want instead is the generic csc support, plus a ycbcr2rgb mode of "bypass". And as usual, this needs some userspace compositor which actually uses it (not just a demo, since there's a huge difference between a demo and something that has to Just Work like a real compositor). -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++ > drivers/gpu/drm/drm_color_mgmt.c | 55 +++++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/drm_plane.c | 6 ++++ > include/drm/drm_color_mgmt.h | 3 ++ > include/drm/drm_plane.h | 4 +++ > include/uapi/drm/drm_mode.h | 12 ++++++++ > 7 files changed, 106 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index bcef93d..87a2d55 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -733,6 +733,7 @@ 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); > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > state->zpos = val; > } else if (property == plane->ycbcr_encoding_property) { > state->ycbcr_encoding = val; > + } else if (property == plane->ycbcr_decode_csc_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ycbcr_decode_csc, val, > + sizeof(struct drm_ycbcr_decode_csc), > + &dummy); > + return ret; > + } else if (property == plane->ycbcr_csc_preoffset_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ycbcr_csc_preoffset, val, > + sizeof(struct drm_ycbcr_csc_preoffset), > + &dummy); > + return ret; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > *val = state->zpos; > } else if (property == plane->ycbcr_encoding_property) { > *val = state->ycbcr_encoding; > + } else if (property == plane->ycbcr_decode_csc_property) { > + *val = state->ycbcr_decode_csc ? > + state->ycbcr_decode_csc->base.id : 0; > + } else if (property == plane->ycbcr_csc_preoffset_property) { > + *val = state->ycbcr_csc_preoffset ? > + state->ycbcr_csc_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 8be9719..6ecc32f 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > { > memcpy(state, plane->state, sizeof(*state)); > > + if (state->ycbcr_decode_csc) > + drm_property_blob_get(state->ycbcr_decode_csc); > + > + if (state->ycbcr_csc_preoffset) > + drm_property_blob_get(state->ycbcr_csc_preoffset); > + > if (state->fb) > drm_framebuffer_get(state->fb); > > @@ -3278,6 +3284,9 @@ struct drm_plane_state * > */ > void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > { > + drm_property_blob_put(state->ycbcr_decode_csc); > + drm_property_blob_put(state->ycbcr_csc_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 245b14a..319ed46 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -95,8 +95,23 @@ > * > * "YCBCR_ENCODING" > * Optional plane enum property to control YCbCr to RGB > - * conversion. The driver provides a subset of standard > - * enum values supported by the DRM plane. > + * conversion. The driver provides a subset of standard enum > + * values supported by the DRM plane. The possible encodings > + * include the standard conversions and a possibility to select a > + * custom conversion matrix and preoffset vector. > + * > + * "YCBCR_DECODE_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 > + * used depends on the value of YCBCR_ENCODING property. > + * > + * "YCBCR_CSC_PREOFFSET" > + * Optional plane property blob to configure YCbCr offset before > + * YCbCr to RGB CSC is applied. The blob contains struct > + * drm_ycbcr_csc_preoffset which is defined in > + * uapi/drm/drm_mode.h. Whether this property is used depends on > + * the value of YCBCR_ENCODING property. > */ > > /** > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > [DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range", > [DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range", > [DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range", > + [DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range", > + [DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range", > + [DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector", > }; > > /** > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > * drm_plane object. The supported encodings should be provided in the > * enum_list parameter. The enum_list parameter should not contain the > * enum names, because the standard names are added by this function. > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created > + * based on the provided enum_list. > */ > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > struct drm_prop_enum_list *enum_list, > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > { > struct drm_device *dev = plane->dev; > struct drm_property *prop; > + bool ycbcr_decode_csc_create = false; > + bool ycbcr_csc_preoffset_create = false; > unsigned int i; > > if (WARN_ON(plane->ycbcr_encoding_property != NULL)) > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; > > enum_list[i].name = ycbcr_encoding_name[encoding]; > + > + if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE || > + encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE) > + ycbcr_decode_csc_create = true; > + > + if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) { > + ycbcr_decode_csc_create = true; > + ycbcr_csc_preoffset_create = true; > + } > } > > prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > plane->ycbcr_encoding_property = prop; > drm_object_attach_property(&plane->base, prop, default_mode); > > + if (ycbcr_decode_csc_create) { > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > + DRM_MODE_PROP_BLOB, > + "YCBCR_DECODE_CSC", 0); > + if (!prop) > + return -ENOMEM; > + plane->ycbcr_decode_csc_property = prop; > + drm_object_attach_property(&plane->base, prop, 0); > + } > + > + if (ycbcr_csc_preoffset_create) { > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > + DRM_MODE_PROP_BLOB, > + "YCBCR_CSC_PREOFFSET", 0); > + if (!prop) > + return -ENOMEM; > + plane->ycbcr_csc_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 007c4d7..d10c942 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane) > if (plane->ycbcr_encoding_property) > drm_property_destroy(dev, plane->ycbcr_encoding_property); > > + if (plane->ycbcr_decode_csc_property) > + drm_property_destroy(dev, plane->ycbcr_decode_csc_property); > + > + if (plane->ycbcr_csc_preoffset_property) > + drm_property_destroy(dev, plane->ycbcr_csc_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 1771394..bdfd37e 100644 > --- a/include/drm/drm_color_mgmt.h > +++ b/include/drm/drm_color_mgmt.h > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding { > DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, > DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, > DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, > + DRM_PLANE_YCBCR_CSC_FULL_RANGE, > + DRM_PLANE_YCBCR_CSC_LIMITED_RANGE, > + DRM_PLANE_YCBCR_CSC_PREOFFSET, > }; > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 4d0510f..63b1e0c 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -115,6 +115,8 @@ struct drm_plane_state { > > /* YCbCr to RGB conversion */ > enum drm_plane_ycbcr_encoding ycbcr_encoding; > + struct drm_property_blob *ycbcr_decode_csc; > + struct drm_property_blob *ycbcr_csc_preoffset; > > /* Clipped coordinates */ > struct drm_rect src, dst; > @@ -529,6 +531,8 @@ struct drm_plane { > struct drm_property *rotation_property; > > struct drm_property *ycbcr_encoding_property; > + struct drm_property *ycbcr_decode_csc_property; > + struct drm_property *ycbcr_csc_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..8c9568d 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_decode_csc { > + /* Conversion matrix in 2-complement s32.32 format. */ > + __s64 ry, rcb, rcr; > + __s64 gy, gcb, gcr; > + __s64 by, bcb, bcr; > +}; > + > +struct drm_ycbcr_csc_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 Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: > > Add standard optinal property blobs for YCbCr to RGB conversion CSC > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined > > to YCBCR_ENCODING property to activate the CSC and preoffset > > properties. For simplicity the new properties are stored in the > > drm_plane object, because the YCBCR_ENCODING is already there. The > > blob contents are defined in the uapi/drm/drm_mode.h header. > > > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > Not sure we want to make this yuv2rgb specific, plenty of planes have > generic degamma/offset, csc, gamme/offset hw. I think what we want instead > is the generic csc support, plus a ycbcr2rgb mode of "bypass". My idea is to not expose this. And instead just expose a normal ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm that can't be done by the hw. > > And as usual, this needs some userspace compositor which actually uses it > (not just a demo, since there's a huge difference between a demo and > something that has to Just Work like a real compositor). > -Daniel > > > --- > > drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++ > > drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++ > > drivers/gpu/drm/drm_color_mgmt.c | 55 +++++++++++++++++++++++++++++++++++-- > > drivers/gpu/drm/drm_plane.c | 6 ++++ > > include/drm/drm_color_mgmt.h | 3 ++ > > include/drm/drm_plane.h | 4 +++ > > include/uapi/drm/drm_mode.h | 12 ++++++++ > > 7 files changed, 106 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index bcef93d..87a2d55 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -733,6 +733,7 @@ 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); > > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > state->zpos = val; > > } else if (property == plane->ycbcr_encoding_property) { > > state->ycbcr_encoding = val; > > + } else if (property == plane->ycbcr_decode_csc_property) { > > + ret = drm_atomic_replace_property_blob_from_id(dev, > > + &state->ycbcr_decode_csc, val, > > + sizeof(struct drm_ycbcr_decode_csc), > > + &dummy); > > + return ret; > > + } else if (property == plane->ycbcr_csc_preoffset_property) { > > + ret = drm_atomic_replace_property_blob_from_id(dev, > > + &state->ycbcr_csc_preoffset, val, > > + sizeof(struct drm_ycbcr_csc_preoffset), > > + &dummy); > > + return ret; > > } else if (plane->funcs->atomic_set_property) { > > return plane->funcs->atomic_set_property(plane, state, > > property, val); > > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > *val = state->zpos; > > } else if (property == plane->ycbcr_encoding_property) { > > *val = state->ycbcr_encoding; > > + } else if (property == plane->ycbcr_decode_csc_property) { > > + *val = state->ycbcr_decode_csc ? > > + state->ycbcr_decode_csc->base.id : 0; > > + } else if (property == plane->ycbcr_csc_preoffset_property) { > > + *val = state->ycbcr_csc_preoffset ? > > + state->ycbcr_csc_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 8be9719..6ecc32f 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > { > > memcpy(state, plane->state, sizeof(*state)); > > > > + if (state->ycbcr_decode_csc) > > + drm_property_blob_get(state->ycbcr_decode_csc); > > + > > + if (state->ycbcr_csc_preoffset) > > + drm_property_blob_get(state->ycbcr_csc_preoffset); > > + > > if (state->fb) > > drm_framebuffer_get(state->fb); > > > > @@ -3278,6 +3284,9 @@ struct drm_plane_state * > > */ > > void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > { > > + drm_property_blob_put(state->ycbcr_decode_csc); > > + drm_property_blob_put(state->ycbcr_csc_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 245b14a..319ed46 100644 > > --- a/drivers/gpu/drm/drm_color_mgmt.c > > +++ b/drivers/gpu/drm/drm_color_mgmt.c > > @@ -95,8 +95,23 @@ > > * > > * "YCBCR_ENCODING" > > * Optional plane enum property to control YCbCr to RGB > > - * conversion. The driver provides a subset of standard > > - * enum values supported by the DRM plane. > > + * conversion. The driver provides a subset of standard enum > > + * values supported by the DRM plane. The possible encodings > > + * include the standard conversions and a possibility to select a > > + * custom conversion matrix and preoffset vector. > > + * > > + * "YCBCR_DECODE_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 > > + * used depends on the value of YCBCR_ENCODING property. > > + * > > + * "YCBCR_CSC_PREOFFSET" > > + * Optional plane property blob to configure YCbCr offset before > > + * YCbCr to RGB CSC is applied. The blob contains struct > > + * drm_ycbcr_csc_preoffset which is defined in > > + * uapi/drm/drm_mode.h. Whether this property is used depends on > > + * the value of YCBCR_ENCODING property. > > */ > > > > /** > > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > > [DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range", > > [DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range", > > [DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range", > > + [DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range", > > + [DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range", > > + [DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector", > > }; > > > > /** > > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > > * drm_plane object. The supported encodings should be provided in the > > * enum_list parameter. The enum_list parameter should not contain the > > * enum names, because the standard names are added by this function. > > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created > > + * based on the provided enum_list. > > */ > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > struct drm_prop_enum_list *enum_list, > > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > { > > struct drm_device *dev = plane->dev; > > struct drm_property *prop; > > + bool ycbcr_decode_csc_create = false; > > + bool ycbcr_csc_preoffset_create = false; > > unsigned int i; > > > > if (WARN_ON(plane->ycbcr_encoding_property != NULL)) > > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; > > > > enum_list[i].name = ycbcr_encoding_name[encoding]; > > + > > + if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE || > > + encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE) > > + ycbcr_decode_csc_create = true; > > + > > + if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) { > > + ycbcr_decode_csc_create = true; > > + ycbcr_csc_preoffset_create = true; > > + } > > } > > > > prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, > > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > plane->ycbcr_encoding_property = prop; > > drm_object_attach_property(&plane->base, prop, default_mode); > > > > + if (ycbcr_decode_csc_create) { > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > > + DRM_MODE_PROP_BLOB, > > + "YCBCR_DECODE_CSC", 0); > > + if (!prop) > > + return -ENOMEM; > > + plane->ycbcr_decode_csc_property = prop; > > + drm_object_attach_property(&plane->base, prop, 0); > > + } > > + > > + if (ycbcr_csc_preoffset_create) { > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > > + DRM_MODE_PROP_BLOB, > > + "YCBCR_CSC_PREOFFSET", 0); > > + if (!prop) > > + return -ENOMEM; > > + plane->ycbcr_csc_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 007c4d7..d10c942 100644 > > --- a/drivers/gpu/drm/drm_plane.c > > +++ b/drivers/gpu/drm/drm_plane.c > > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane) > > if (plane->ycbcr_encoding_property) > > drm_property_destroy(dev, plane->ycbcr_encoding_property); > > > > + if (plane->ycbcr_decode_csc_property) > > + drm_property_destroy(dev, plane->ycbcr_decode_csc_property); > > + > > + if (plane->ycbcr_csc_preoffset_property) > > + drm_property_destroy(dev, plane->ycbcr_csc_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 1771394..bdfd37e 100644 > > --- a/include/drm/drm_color_mgmt.h > > +++ b/include/drm/drm_color_mgmt.h > > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding { > > DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, > > DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, > > DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, > > + DRM_PLANE_YCBCR_CSC_FULL_RANGE, > > + DRM_PLANE_YCBCR_CSC_LIMITED_RANGE, > > + DRM_PLANE_YCBCR_CSC_PREOFFSET, > > }; > > > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > index 4d0510f..63b1e0c 100644 > > --- a/include/drm/drm_plane.h > > +++ b/include/drm/drm_plane.h > > @@ -115,6 +115,8 @@ struct drm_plane_state { > > > > /* YCbCr to RGB conversion */ > > enum drm_plane_ycbcr_encoding ycbcr_encoding; > > + struct drm_property_blob *ycbcr_decode_csc; > > + struct drm_property_blob *ycbcr_csc_preoffset; > > > > /* Clipped coordinates */ > > struct drm_rect src, dst; > > @@ -529,6 +531,8 @@ struct drm_plane { > > struct drm_property *rotation_property; > > > > struct drm_property *ycbcr_encoding_property; > > + struct drm_property *ycbcr_decode_csc_property; > > + struct drm_property *ycbcr_csc_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..8c9568d 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_decode_csc { > > + /* Conversion matrix in 2-complement s32.32 format. */ > > + __s64 ry, rcb, rcr; > > + __s64 gy, gcb, gcr; > > + __s64 by, bcb, bcr; > > +}; > > + > > +struct drm_ycbcr_csc_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 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On 05/04/17 17:51, Ville Syrjälä wrote: > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: >> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: >>> Add standard optinal property blobs for YCbCr to RGB conversion CSC >>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined >>> to YCBCR_ENCODING property to activate the CSC and preoffset >>> properties. For simplicity the new properties are stored in the >>> drm_plane object, because the YCBCR_ENCODING is already there. The >>> blob contents are defined in the uapi/drm/drm_mode.h header. >>> >>> Signed-off-by: Jyri Sarha <jsarha@ti.com> >> Not sure we want to make this yuv2rgb specific, plenty of planes have >> generic degamma/offset, csc, gamme/offset hw. I think what we want instead >> is the generic csc support, plus a ycbcr2rgb mode of "bypass". > My idea is to not expose this. And instead just expose a normal > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm > that can't be done by the hw. > So, pulling the suggestions together the properties would now look like this: YCBCR_RANGE - Full - Limited YCBCR_ENCODING - BT.601 - BT.709 - BT.2020 CTM (blob) And all these properties could be added to individual planes. I'll try to come up with a new patch to add simple enum properties for YCBCR_PREOFFSET and YCBCR_ENCODING for planes. Should we need any more enum values for those? The only functional thing that is missing from the above proposal is the possibility to define an arbitrary YCbCr preoffset vector, but that can be added later if there ever is any real need. The other thing that could be added for special purposes would be adding an "Identity" enum to YCBCR_ENCODING, that would allow setting the YCbCr CSC trough CTM without any driver level multiplication getting in way, but that can also be added if there is a need. Best regards, Jyri
On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote: > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: > > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: > > > Add standard optinal property blobs for YCbCr to RGB conversion CSC > > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined > > > to YCBCR_ENCODING property to activate the CSC and preoffset > > > properties. For simplicity the new properties are stored in the > > > drm_plane object, because the YCBCR_ENCODING is already there. The > > > blob contents are defined in the uapi/drm/drm_mode.h header. > > > > > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > > > Not sure we want to make this yuv2rgb specific, plenty of planes have > > generic degamma/offset, csc, gamme/offset hw. I think what we want instead > > is the generic csc support, plus a ycbcr2rgb mode of "bypass". > > My idea is to not expose this. And instead just expose a normal > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm > that can't be done by the hw. But that means we'd need to have a bit-perfect match with a canonical conversion matrix (even if maybe the hw has a rounding bug and implements something slightly different than the standard). That seems a bit an awkward interface. Or is your idea that hw with a ctm would simply not expose the colorspace enum, if it doesn't also have fixed-function bits on top of the ctm? -Daniel > > > > > And as usual, this needs some userspace compositor which actually uses it > > (not just a demo, since there's a huge difference between a demo and > > something that has to Just Work like a real compositor). > > -Daniel > > > > > --- > > > drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++ > > > drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++ > > > drivers/gpu/drm/drm_color_mgmt.c | 55 +++++++++++++++++++++++++++++++++++-- > > > drivers/gpu/drm/drm_plane.c | 6 ++++ > > > include/drm/drm_color_mgmt.h | 3 ++ > > > include/drm/drm_plane.h | 4 +++ > > > include/uapi/drm/drm_mode.h | 12 ++++++++ > > > 7 files changed, 106 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index bcef93d..87a2d55 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -733,6 +733,7 @@ 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); > > > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > > state->zpos = val; > > > } else if (property == plane->ycbcr_encoding_property) { > > > state->ycbcr_encoding = val; > > > + } else if (property == plane->ycbcr_decode_csc_property) { > > > + ret = drm_atomic_replace_property_blob_from_id(dev, > > > + &state->ycbcr_decode_csc, val, > > > + sizeof(struct drm_ycbcr_decode_csc), > > > + &dummy); > > > + return ret; > > > + } else if (property == plane->ycbcr_csc_preoffset_property) { > > > + ret = drm_atomic_replace_property_blob_from_id(dev, > > > + &state->ycbcr_csc_preoffset, val, > > > + sizeof(struct drm_ycbcr_csc_preoffset), > > > + &dummy); > > > + return ret; > > > } else if (plane->funcs->atomic_set_property) { > > > return plane->funcs->atomic_set_property(plane, state, > > > property, val); > > > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > > *val = state->zpos; > > > } else if (property == plane->ycbcr_encoding_property) { > > > *val = state->ycbcr_encoding; > > > + } else if (property == plane->ycbcr_decode_csc_property) { > > > + *val = state->ycbcr_decode_csc ? > > > + state->ycbcr_decode_csc->base.id : 0; > > > + } else if (property == plane->ycbcr_csc_preoffset_property) { > > > + *val = state->ycbcr_csc_preoffset ? > > > + state->ycbcr_csc_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 8be9719..6ecc32f 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > > { > > > memcpy(state, plane->state, sizeof(*state)); > > > > > > + if (state->ycbcr_decode_csc) > > > + drm_property_blob_get(state->ycbcr_decode_csc); > > > + > > > + if (state->ycbcr_csc_preoffset) > > > + drm_property_blob_get(state->ycbcr_csc_preoffset); > > > + > > > if (state->fb) > > > drm_framebuffer_get(state->fb); > > > > > > @@ -3278,6 +3284,9 @@ struct drm_plane_state * > > > */ > > > void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > > { > > > + drm_property_blob_put(state->ycbcr_decode_csc); > > > + drm_property_blob_put(state->ycbcr_csc_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 245b14a..319ed46 100644 > > > --- a/drivers/gpu/drm/drm_color_mgmt.c > > > +++ b/drivers/gpu/drm/drm_color_mgmt.c > > > @@ -95,8 +95,23 @@ > > > * > > > * "YCBCR_ENCODING" > > > * Optional plane enum property to control YCbCr to RGB > > > - * conversion. The driver provides a subset of standard > > > - * enum values supported by the DRM plane. > > > + * conversion. The driver provides a subset of standard enum > > > + * values supported by the DRM plane. The possible encodings > > > + * include the standard conversions and a possibility to select a > > > + * custom conversion matrix and preoffset vector. > > > + * > > > + * "YCBCR_DECODE_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 > > > + * used depends on the value of YCBCR_ENCODING property. > > > + * > > > + * "YCBCR_CSC_PREOFFSET" > > > + * Optional plane property blob to configure YCbCr offset before > > > + * YCbCr to RGB CSC is applied. The blob contains struct > > > + * drm_ycbcr_csc_preoffset which is defined in > > > + * uapi/drm/drm_mode.h. Whether this property is used depends on > > > + * the value of YCBCR_ENCODING property. > > > */ > > > > > > /** > > > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > > > [DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range", > > > [DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range", > > > [DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range", > > > + [DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range", > > > + [DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range", > > > + [DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector", > > > }; > > > > > > /** > > > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > > > * drm_plane object. The supported encodings should be provided in the > > > * enum_list parameter. The enum_list parameter should not contain the > > > * enum names, because the standard names are added by this function. > > > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created > > > + * based on the provided enum_list. > > > */ > > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > struct drm_prop_enum_list *enum_list, > > > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > { > > > struct drm_device *dev = plane->dev; > > > struct drm_property *prop; > > > + bool ycbcr_decode_csc_create = false; > > > + bool ycbcr_csc_preoffset_create = false; > > > unsigned int i; > > > > > > if (WARN_ON(plane->ycbcr_encoding_property != NULL)) > > > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; > > > > > > enum_list[i].name = ycbcr_encoding_name[encoding]; > > > + > > > + if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE || > > > + encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE) > > > + ycbcr_decode_csc_create = true; > > > + > > > + if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) { > > > + ycbcr_decode_csc_create = true; > > > + ycbcr_csc_preoffset_create = true; > > > + } > > > } > > > > > > prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, > > > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > plane->ycbcr_encoding_property = prop; > > > drm_object_attach_property(&plane->base, prop, default_mode); > > > > > > + if (ycbcr_decode_csc_create) { > > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > > > + DRM_MODE_PROP_BLOB, > > > + "YCBCR_DECODE_CSC", 0); > > > + if (!prop) > > > + return -ENOMEM; > > > + plane->ycbcr_decode_csc_property = prop; > > > + drm_object_attach_property(&plane->base, prop, 0); > > > + } > > > + > > > + if (ycbcr_csc_preoffset_create) { > > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > > > + DRM_MODE_PROP_BLOB, > > > + "YCBCR_CSC_PREOFFSET", 0); > > > + if (!prop) > > > + return -ENOMEM; > > > + plane->ycbcr_csc_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 007c4d7..d10c942 100644 > > > --- a/drivers/gpu/drm/drm_plane.c > > > +++ b/drivers/gpu/drm/drm_plane.c > > > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane) > > > if (plane->ycbcr_encoding_property) > > > drm_property_destroy(dev, plane->ycbcr_encoding_property); > > > > > > + if (plane->ycbcr_decode_csc_property) > > > + drm_property_destroy(dev, plane->ycbcr_decode_csc_property); > > > + > > > + if (plane->ycbcr_csc_preoffset_property) > > > + drm_property_destroy(dev, plane->ycbcr_csc_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 1771394..bdfd37e 100644 > > > --- a/include/drm/drm_color_mgmt.h > > > +++ b/include/drm/drm_color_mgmt.h > > > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding { > > > DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, > > > DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, > > > DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, > > > + DRM_PLANE_YCBCR_CSC_FULL_RANGE, > > > + DRM_PLANE_YCBCR_CSC_LIMITED_RANGE, > > > + DRM_PLANE_YCBCR_CSC_PREOFFSET, > > > }; > > > > > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > > index 4d0510f..63b1e0c 100644 > > > --- a/include/drm/drm_plane.h > > > +++ b/include/drm/drm_plane.h > > > @@ -115,6 +115,8 @@ struct drm_plane_state { > > > > > > /* YCbCr to RGB conversion */ > > > enum drm_plane_ycbcr_encoding ycbcr_encoding; > > > + struct drm_property_blob *ycbcr_decode_csc; > > > + struct drm_property_blob *ycbcr_csc_preoffset; > > > > > > /* Clipped coordinates */ > > > struct drm_rect src, dst; > > > @@ -529,6 +531,8 @@ struct drm_plane { > > > struct drm_property *rotation_property; > > > > > > struct drm_property *ycbcr_encoding_property; > > > + struct drm_property *ycbcr_decode_csc_property; > > > + struct drm_property *ycbcr_csc_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..8c9568d 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_decode_csc { > > > + /* Conversion matrix in 2-complement s32.32 format. */ > > > + __s64 ry, rcb, rcr; > > > + __s64 gy, gcb, gcr; > > > + __s64 by, bcb, bcr; > > > +}; > > > + > > > +struct drm_ycbcr_csc_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 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel OTC
Regards Shashank On 5/5/2017 12:28 PM, Daniel Vetter wrote: > On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote: >> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: >>> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: >>>> Add standard optinal property blobs for YCbCr to RGB conversion CSC >>>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined >>>> to YCBCR_ENCODING property to activate the CSC and preoffset >>>> properties. For simplicity the new properties are stored in the >>>> drm_plane object, because the YCBCR_ENCODING is already there. The >>>> blob contents are defined in the uapi/drm/drm_mode.h header. >>>> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com> >>> Not sure we want to make this yuv2rgb specific, plenty of planes have >>> generic degamma/offset, csc, gamme/offset hw. I think what we want instead >>> is the generic csc support, plus a ycbcr2rgb mode of "bypass". >> My idea is to not expose this. And instead just expose a normal >> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm >> that can't be done by the hw. > But that means we'd need to have a bit-perfect match with a canonical > conversion matrix (even if maybe the hw has a rounding bug and implements > something slightly different than the standard). That seems a bit an > awkward interface. Or is your idea that hw with a ctm would simply not > expose the colorspace enum, if it doesn't also have fixed-function bits on > top of the ctm? > -Daniel I think the idea is to have separate properties for CTM and Gamut Mapping, so that we can have more control over linear and non-linear data blocks. All transformation should happen in one property whereas all gamut mapping should go into other, which IMHO seems to be the correct way to operate. - Shashank >>> And as usual, this needs some userspace compositor which actually uses it >>> (not just a demo, since there's a huge difference between a demo and >>> something that has to Just Work like a real compositor). >>> -Daniel >>> >>>> --- >>>> drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++ >>>> drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++ >>>> drivers/gpu/drm/drm_color_mgmt.c | 55 +++++++++++++++++++++++++++++++++++-- >>>> drivers/gpu/drm/drm_plane.c | 6 ++++ >>>> include/drm/drm_color_mgmt.h | 3 ++ >>>> include/drm/drm_plane.h | 4 +++ >>>> include/uapi/drm/drm_mode.h | 12 ++++++++ >>>> 7 files changed, 106 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>> index bcef93d..87a2d55 100644 >>>> --- a/drivers/gpu/drm/drm_atomic.c >>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>> @@ -733,6 +733,7 @@ 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); >>>> @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >>>> state->zpos = val; >>>> } else if (property == plane->ycbcr_encoding_property) { >>>> state->ycbcr_encoding = val; >>>> + } else if (property == plane->ycbcr_decode_csc_property) { >>>> + ret = drm_atomic_replace_property_blob_from_id(dev, >>>> + &state->ycbcr_decode_csc, val, >>>> + sizeof(struct drm_ycbcr_decode_csc), >>>> + &dummy); >>>> + return ret; >>>> + } else if (property == plane->ycbcr_csc_preoffset_property) { >>>> + ret = drm_atomic_replace_property_blob_from_id(dev, >>>> + &state->ycbcr_csc_preoffset, val, >>>> + sizeof(struct drm_ycbcr_csc_preoffset), >>>> + &dummy); >>>> + return ret; >>>> } else if (plane->funcs->atomic_set_property) { >>>> return plane->funcs->atomic_set_property(plane, state, >>>> property, val); >>>> @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, >>>> *val = state->zpos; >>>> } else if (property == plane->ycbcr_encoding_property) { >>>> *val = state->ycbcr_encoding; >>>> + } else if (property == plane->ycbcr_decode_csc_property) { >>>> + *val = state->ycbcr_decode_csc ? >>>> + state->ycbcr_decode_csc->base.id : 0; >>>> + } else if (property == plane->ycbcr_csc_preoffset_property) { >>>> + *val = state->ycbcr_csc_preoffset ? >>>> + state->ycbcr_csc_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 8be9719..6ecc32f 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>> @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >>>> { >>>> memcpy(state, plane->state, sizeof(*state)); >>>> >>>> + if (state->ycbcr_decode_csc) >>>> + drm_property_blob_get(state->ycbcr_decode_csc); >>>> + >>>> + if (state->ycbcr_csc_preoffset) >>>> + drm_property_blob_get(state->ycbcr_csc_preoffset); >>>> + >>>> if (state->fb) >>>> drm_framebuffer_get(state->fb); >>>> >>>> @@ -3278,6 +3284,9 @@ struct drm_plane_state * >>>> */ >>>> void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >>>> { >>>> + drm_property_blob_put(state->ycbcr_decode_csc); >>>> + drm_property_blob_put(state->ycbcr_csc_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 245b14a..319ed46 100644 >>>> --- a/drivers/gpu/drm/drm_color_mgmt.c >>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c >>>> @@ -95,8 +95,23 @@ >>>> * >>>> * "YCBCR_ENCODING" >>>> * Optional plane enum property to control YCbCr to RGB >>>> - * conversion. The driver provides a subset of standard >>>> - * enum values supported by the DRM plane. >>>> + * conversion. The driver provides a subset of standard enum >>>> + * values supported by the DRM plane. The possible encodings >>>> + * include the standard conversions and a possibility to select a >>>> + * custom conversion matrix and preoffset vector. >>>> + * >>>> + * "YCBCR_DECODE_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 >>>> + * used depends on the value of YCBCR_ENCODING property. >>>> + * >>>> + * "YCBCR_CSC_PREOFFSET" >>>> + * Optional plane property blob to configure YCbCr offset before >>>> + * YCbCr to RGB CSC is applied. The blob contains struct >>>> + * drm_ycbcr_csc_preoffset which is defined in >>>> + * uapi/drm/drm_mode.h. Whether this property is used depends on >>>> + * the value of YCBCR_ENCODING property. >>>> */ >>>> >>>> /** >>>> @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, >>>> [DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range", >>>> [DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range", >>>> [DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range", >>>> + [DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range", >>>> + [DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range", >>>> + [DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector", >>>> }; >>>> >>>> /** >>>> @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, >>>> * drm_plane object. The supported encodings should be provided in the >>>> * enum_list parameter. The enum_list parameter should not contain the >>>> * enum names, because the standard names are added by this function. >>>> + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created >>>> + * based on the provided enum_list. >>>> */ >>>> int drm_plane_create_ycbcr_properties(struct drm_plane *plane, >>>> struct drm_prop_enum_list *enum_list, >>>> @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, >>>> { >>>> struct drm_device *dev = plane->dev; >>>> struct drm_property *prop; >>>> + bool ycbcr_decode_csc_create = false; >>>> + bool ycbcr_csc_preoffset_create = false; >>>> unsigned int i; >>>> >>>> if (WARN_ON(plane->ycbcr_encoding_property != NULL)) >>>> @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, >>>> enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; >>>> >>>> enum_list[i].name = ycbcr_encoding_name[encoding]; >>>> + >>>> + if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE || >>>> + encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE) >>>> + ycbcr_decode_csc_create = true; >>>> + >>>> + if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) { >>>> + ycbcr_decode_csc_create = true; >>>> + ycbcr_csc_preoffset_create = true; >>>> + } >>>> } >>>> >>>> prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, >>>> @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, >>>> plane->ycbcr_encoding_property = prop; >>>> drm_object_attach_property(&plane->base, prop, default_mode); >>>> >>>> + if (ycbcr_decode_csc_create) { >>>> + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | >>>> + DRM_MODE_PROP_BLOB, >>>> + "YCBCR_DECODE_CSC", 0); >>>> + if (!prop) >>>> + return -ENOMEM; >>>> + plane->ycbcr_decode_csc_property = prop; >>>> + drm_object_attach_property(&plane->base, prop, 0); >>>> + } >>>> + >>>> + if (ycbcr_csc_preoffset_create) { >>>> + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | >>>> + DRM_MODE_PROP_BLOB, >>>> + "YCBCR_CSC_PREOFFSET", 0); >>>> + if (!prop) >>>> + return -ENOMEM; >>>> + plane->ycbcr_csc_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 007c4d7..d10c942 100644 >>>> --- a/drivers/gpu/drm/drm_plane.c >>>> +++ b/drivers/gpu/drm/drm_plane.c >>>> @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane) >>>> if (plane->ycbcr_encoding_property) >>>> drm_property_destroy(dev, plane->ycbcr_encoding_property); >>>> >>>> + if (plane->ycbcr_decode_csc_property) >>>> + drm_property_destroy(dev, plane->ycbcr_decode_csc_property); >>>> + >>>> + if (plane->ycbcr_csc_preoffset_property) >>>> + drm_property_destroy(dev, plane->ycbcr_csc_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 1771394..bdfd37e 100644 >>>> --- a/include/drm/drm_color_mgmt.h >>>> +++ b/include/drm/drm_color_mgmt.h >>>> @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding { >>>> DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, >>>> DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, >>>> DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, >>>> + DRM_PLANE_YCBCR_CSC_FULL_RANGE, >>>> + DRM_PLANE_YCBCR_CSC_LIMITED_RANGE, >>>> + DRM_PLANE_YCBCR_CSC_PREOFFSET, >>>> }; >>>> >>>> int drm_plane_create_ycbcr_properties(struct drm_plane *plane, >>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >>>> index 4d0510f..63b1e0c 100644 >>>> --- a/include/drm/drm_plane.h >>>> +++ b/include/drm/drm_plane.h >>>> @@ -115,6 +115,8 @@ struct drm_plane_state { >>>> >>>> /* YCbCr to RGB conversion */ >>>> enum drm_plane_ycbcr_encoding ycbcr_encoding; >>>> + struct drm_property_blob *ycbcr_decode_csc; >>>> + struct drm_property_blob *ycbcr_csc_preoffset; >>>> >>>> /* Clipped coordinates */ >>>> struct drm_rect src, dst; >>>> @@ -529,6 +531,8 @@ struct drm_plane { >>>> struct drm_property *rotation_property; >>>> >>>> struct drm_property *ycbcr_encoding_property; >>>> + struct drm_property *ycbcr_decode_csc_property; >>>> + struct drm_property *ycbcr_csc_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..8c9568d 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_decode_csc { >>>> + /* Conversion matrix in 2-complement s32.32 format. */ >>>> + __s64 ry, rcb, rcr; >>>> + __s64 gy, gcb, gcr; >>>> + __s64 by, bcb, bcr; >>>> +}; >>>> + >>>> +struct drm_ycbcr_csc_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 >>>> >>> -- >>> Daniel Vetter >>> Software Engineer, Intel Corporation >>> http://blog.ffwll.ch >> -- >> Ville Syrjälä >> Intel OTC
On Fri, May 5, 2017 at 9:06 AM, Sharma, Shashank <shashank.sharma@intel.com> wrote: > On 5/5/2017 12:28 PM, Daniel Vetter wrote: >> >> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote: >>> >>> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: >>>> >>>> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: >>>>> >>>>> Add standard optinal property blobs for YCbCr to RGB conversion CSC >>>>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined >>>>> to YCBCR_ENCODING property to activate the CSC and preoffset >>>>> properties. For simplicity the new properties are stored in the >>>>> drm_plane object, because the YCBCR_ENCODING is already there. The >>>>> blob contents are defined in the uapi/drm/drm_mode.h header. >>>>> >>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com> >>>> >>>> Not sure we want to make this yuv2rgb specific, plenty of planes have >>>> generic degamma/offset, csc, gamme/offset hw. I think what we want >>>> instead >>>> is the generic csc support, plus a ycbcr2rgb mode of "bypass". >>> >>> My idea is to not expose this. And instead just expose a normal >>> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm >>> that can't be done by the hw. >> >> But that means we'd need to have a bit-perfect match with a canonical >> conversion matrix (even if maybe the hw has a rounding bug and implements >> something slightly different than the standard). That seems a bit an >> awkward interface. Or is your idea that hw with a ctm would simply not >> expose the colorspace enum, if it doesn't also have fixed-function bits on >> top of the ctm? >> -Daniel > > I think the idea is to have separate properties for CTM and Gamut Mapping, > so that we can have more control > over linear and non-linear data blocks. All transformation should happen in > one property whereas all gamut > mapping should go into other, which IMHO seems to be the correct way to > operate. Yeah, for the programmable hw we should probably go with the degamma/ctm/gamma combo, like we have on the CRTC already. My question was how this interactions with some fixed-function color space conversion the hw might also have, and how these two sets of properties are mean to interact. On that topic, I think it'd be good if we put the helper functions and property documentation into drm_color_mgmt.c, so that it is all in one place, and to make sure we don't accidentally have different meanings for gamma table and ctm blobs. -Daniel
Regards Shashank On 5/5/2017 12:43 PM, Daniel Vetter wrote: > On Fri, May 5, 2017 at 9:06 AM, Sharma, Shashank > <shashank.sharma@intel.com> wrote: >> On 5/5/2017 12:28 PM, Daniel Vetter wrote: >>> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote: >>>> On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: >>>>> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: >>>>>> Add standard optinal property blobs for YCbCr to RGB conversion CSC >>>>>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined >>>>>> to YCBCR_ENCODING property to activate the CSC and preoffset >>>>>> properties. For simplicity the new properties are stored in the >>>>>> drm_plane object, because the YCBCR_ENCODING is already there. The >>>>>> blob contents are defined in the uapi/drm/drm_mode.h header. >>>>>> >>>>>> Signed-off-by: Jyri Sarha <jsarha@ti.com> >>>>> Not sure we want to make this yuv2rgb specific, plenty of planes have >>>>> generic degamma/offset, csc, gamme/offset hw. I think what we want >>>>> instead >>>>> is the generic csc support, plus a ycbcr2rgb mode of "bypass". >>>> My idea is to not expose this. And instead just expose a normal >>>> ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm >>>> that can't be done by the hw. >>> But that means we'd need to have a bit-perfect match with a canonical >>> conversion matrix (even if maybe the hw has a rounding bug and implements >>> something slightly different than the standard). That seems a bit an >>> awkward interface. Or is your idea that hw with a ctm would simply not >>> expose the colorspace enum, if it doesn't also have fixed-function bits on >>> top of the ctm? >>> -Daniel >> I think the idea is to have separate properties for CTM and Gamut Mapping, >> so that we can have more control >> over linear and non-linear data blocks. All transformation should happen in >> one property whereas all gamut >> mapping should go into other, which IMHO seems to be the correct way to >> operate. > Yeah, for the programmable hw we should probably go with the > degamma/ctm/gamma combo, like we have on the CRTC already. My question > was how this interactions with some fixed-function color space > conversion the hw might also have, and how these two sets of > properties are mean to interact. Even for the fixed function HW we can have this segregation, and the core driver can take care of understanding the enum, and conversion into driving a fix-function block. I had proposed one block diagram, which was once prepared by Ville / Me to handle this situation, but unfortunately we could not discus much there. https://patchwork.kernel.org/patch/9695875/ > On that topic, I think it'd be good if we put the helper functions and > property documentation into drm_color_mgmt.c, so that it is all in one > place, and to make sure we don't accidentally have different meanings > for gamma table and ctm blobs. I agree, that would be good place. - Shashank > -Daniel
On Fri, May 05, 2017 at 08:58:17AM +0200, Daniel Vetter wrote: > On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote: > > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: > > > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: > > > > Add standard optinal property blobs for YCbCr to RGB conversion CSC > > > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined > > > > to YCBCR_ENCODING property to activate the CSC and preoffset > > > > properties. For simplicity the new properties are stored in the > > > > drm_plane object, because the YCBCR_ENCODING is already there. The > > > > blob contents are defined in the uapi/drm/drm_mode.h header. > > > > > > > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > > > > > Not sure we want to make this yuv2rgb specific, plenty of planes have > > > generic degamma/offset, csc, gamme/offset hw. I think what we want instead > > > is the generic csc support, plus a ycbcr2rgb mode of "bypass". > > > > My idea is to not expose this. And instead just expose a normal > > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm > > that can't be done by the hw. > > But that means we'd need to have a bit-perfect match with a canonical > conversion matrix (even if maybe the hw has a rounding bug and implements > something slightly different than the standard). I don't see what this has to do with bit correctness. Different hw is likely to use different precision for this stuff anyway, so I'm 100% sure we'll not get the same results from all hardware. And really, getting 100% same output doesn't seem all that important here anyway. Anyway, all I'm saying is that we should expose a standard pipeline: ycbcr->rgb -> degamma -> ctm -> gamma And each driver can then expose whatever parts of that they wish. If you want to expoe a programmable matrix you expose it as the ctm. I don't think having a programmable matrix as both ycbcr->rgb and ctm would really benefit us. > That seems a bit an > awkward interface. Or is your idea that hw with a ctm would simply not > expose the colorspace enum, if it doesn't also have fixed-function bits on > top of the ctm? > -Daniel > > > > > > > > > And as usual, this needs some userspace compositor which actually uses it > > > (not just a demo, since there's a huge difference between a demo and > > > something that has to Just Work like a real compositor). > > > -Daniel > > > > > > > --- > > > > drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++ > > > > drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++ > > > > drivers/gpu/drm/drm_color_mgmt.c | 55 +++++++++++++++++++++++++++++++++++-- > > > > drivers/gpu/drm/drm_plane.c | 6 ++++ > > > > include/drm/drm_color_mgmt.h | 3 ++ > > > > include/drm/drm_plane.h | 4 +++ > > > > include/uapi/drm/drm_mode.h | 12 ++++++++ > > > > 7 files changed, 106 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > > index bcef93d..87a2d55 100644 > > > > --- a/drivers/gpu/drm/drm_atomic.c > > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > > @@ -733,6 +733,7 @@ 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); > > > > @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > > > state->zpos = val; > > > > } else if (property == plane->ycbcr_encoding_property) { > > > > state->ycbcr_encoding = val; > > > > + } else if (property == plane->ycbcr_decode_csc_property) { > > > > + ret = drm_atomic_replace_property_blob_from_id(dev, > > > > + &state->ycbcr_decode_csc, val, > > > > + sizeof(struct drm_ycbcr_decode_csc), > > > > + &dummy); > > > > + return ret; > > > > + } else if (property == plane->ycbcr_csc_preoffset_property) { > > > > + ret = drm_atomic_replace_property_blob_from_id(dev, > > > > + &state->ycbcr_csc_preoffset, val, > > > > + sizeof(struct drm_ycbcr_csc_preoffset), > > > > + &dummy); > > > > + return ret; > > > > } else if (plane->funcs->atomic_set_property) { > > > > return plane->funcs->atomic_set_property(plane, state, > > > > property, val); > > > > @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, > > > > *val = state->zpos; > > > > } else if (property == plane->ycbcr_encoding_property) { > > > > *val = state->ycbcr_encoding; > > > > + } else if (property == plane->ycbcr_decode_csc_property) { > > > > + *val = state->ycbcr_decode_csc ? > > > > + state->ycbcr_decode_csc->base.id : 0; > > > > + } else if (property == plane->ycbcr_csc_preoffset_property) { > > > > + *val = state->ycbcr_csc_preoffset ? > > > > + state->ycbcr_csc_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 8be9719..6ecc32f 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > > > { > > > > memcpy(state, plane->state, sizeof(*state)); > > > > > > > > + if (state->ycbcr_decode_csc) > > > > + drm_property_blob_get(state->ycbcr_decode_csc); > > > > + > > > > + if (state->ycbcr_csc_preoffset) > > > > + drm_property_blob_get(state->ycbcr_csc_preoffset); > > > > + > > > > if (state->fb) > > > > drm_framebuffer_get(state->fb); > > > > > > > > @@ -3278,6 +3284,9 @@ struct drm_plane_state * > > > > */ > > > > void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > > > { > > > > + drm_property_blob_put(state->ycbcr_decode_csc); > > > > + drm_property_blob_put(state->ycbcr_csc_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 245b14a..319ed46 100644 > > > > --- a/drivers/gpu/drm/drm_color_mgmt.c > > > > +++ b/drivers/gpu/drm/drm_color_mgmt.c > > > > @@ -95,8 +95,23 @@ > > > > * > > > > * "YCBCR_ENCODING" > > > > * Optional plane enum property to control YCbCr to RGB > > > > - * conversion. The driver provides a subset of standard > > > > - * enum values supported by the DRM plane. > > > > + * conversion. The driver provides a subset of standard enum > > > > + * values supported by the DRM plane. The possible encodings > > > > + * include the standard conversions and a possibility to select a > > > > + * custom conversion matrix and preoffset vector. > > > > + * > > > > + * "YCBCR_DECODE_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 > > > > + * used depends on the value of YCBCR_ENCODING property. > > > > + * > > > > + * "YCBCR_CSC_PREOFFSET" > > > > + * Optional plane property blob to configure YCbCr offset before > > > > + * YCbCr to RGB CSC is applied. The blob contains struct > > > > + * drm_ycbcr_csc_preoffset which is defined in > > > > + * uapi/drm/drm_mode.h. Whether this property is used depends on > > > > + * the value of YCBCR_ENCODING property. > > > > */ > > > > > > > > /** > > > > @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > > > > [DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range", > > > > [DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range", > > > > [DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range", > > > > + [DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range", > > > > + [DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range", > > > > + [DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector", > > > > }; > > > > > > > > /** > > > > @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > > > > * drm_plane object. The supported encodings should be provided in the > > > > * enum_list parameter. The enum_list parameter should not contain the > > > > * enum names, because the standard names are added by this function. > > > > + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created > > > > + * based on the provided enum_list. > > > > */ > > > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > > struct drm_prop_enum_list *enum_list, > > > > @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > > { > > > > struct drm_device *dev = plane->dev; > > > > struct drm_property *prop; > > > > + bool ycbcr_decode_csc_create = false; > > > > + bool ycbcr_csc_preoffset_create = false; > > > > unsigned int i; > > > > > > > > if (WARN_ON(plane->ycbcr_encoding_property != NULL)) > > > > @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > > enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; > > > > > > > > enum_list[i].name = ycbcr_encoding_name[encoding]; > > > > + > > > > + if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE || > > > > + encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE) > > > > + ycbcr_decode_csc_create = true; > > > > + > > > > + if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) { > > > > + ycbcr_decode_csc_create = true; > > > > + ycbcr_csc_preoffset_create = true; > > > > + } > > > > } > > > > > > > > prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, > > > > @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > > plane->ycbcr_encoding_property = prop; > > > > drm_object_attach_property(&plane->base, prop, default_mode); > > > > > > > > + if (ycbcr_decode_csc_create) { > > > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > > > > + DRM_MODE_PROP_BLOB, > > > > + "YCBCR_DECODE_CSC", 0); > > > > + if (!prop) > > > > + return -ENOMEM; > > > > + plane->ycbcr_decode_csc_property = prop; > > > > + drm_object_attach_property(&plane->base, prop, 0); > > > > + } > > > > + > > > > + if (ycbcr_csc_preoffset_create) { > > > > + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | > > > > + DRM_MODE_PROP_BLOB, > > > > + "YCBCR_CSC_PREOFFSET", 0); > > > > + if (!prop) > > > > + return -ENOMEM; > > > > + plane->ycbcr_csc_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 007c4d7..d10c942 100644 > > > > --- a/drivers/gpu/drm/drm_plane.c > > > > +++ b/drivers/gpu/drm/drm_plane.c > > > > @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane) > > > > if (plane->ycbcr_encoding_property) > > > > drm_property_destroy(dev, plane->ycbcr_encoding_property); > > > > > > > > + if (plane->ycbcr_decode_csc_property) > > > > + drm_property_destroy(dev, plane->ycbcr_decode_csc_property); > > > > + > > > > + if (plane->ycbcr_csc_preoffset_property) > > > > + drm_property_destroy(dev, plane->ycbcr_csc_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 1771394..bdfd37e 100644 > > > > --- a/include/drm/drm_color_mgmt.h > > > > +++ b/include/drm/drm_color_mgmt.h > > > > @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding { > > > > DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, > > > > DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, > > > > DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, > > > > + DRM_PLANE_YCBCR_CSC_FULL_RANGE, > > > > + DRM_PLANE_YCBCR_CSC_LIMITED_RANGE, > > > > + DRM_PLANE_YCBCR_CSC_PREOFFSET, > > > > }; > > > > > > > > int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > > > index 4d0510f..63b1e0c 100644 > > > > --- a/include/drm/drm_plane.h > > > > +++ b/include/drm/drm_plane.h > > > > @@ -115,6 +115,8 @@ struct drm_plane_state { > > > > > > > > /* YCbCr to RGB conversion */ > > > > enum drm_plane_ycbcr_encoding ycbcr_encoding; > > > > + struct drm_property_blob *ycbcr_decode_csc; > > > > + struct drm_property_blob *ycbcr_csc_preoffset; > > > > > > > > /* Clipped coordinates */ > > > > struct drm_rect src, dst; > > > > @@ -529,6 +531,8 @@ struct drm_plane { > > > > struct drm_property *rotation_property; > > > > > > > > struct drm_property *ycbcr_encoding_property; > > > > + struct drm_property *ycbcr_decode_csc_property; > > > > + struct drm_property *ycbcr_csc_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..8c9568d 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_decode_csc { > > > > + /* Conversion matrix in 2-complement s32.32 format. */ > > > > + __s64 ry, rcb, rcr; > > > > + __s64 gy, gcb, gcr; > > > > + __s64 by, bcb, bcr; > > > > +}; > > > > + > > > > +struct drm_ycbcr_csc_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 > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Fri, May 5, 2017 at 12:45 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, May 05, 2017 at 08:58:17AM +0200, Daniel Vetter wrote: >> On Thu, May 04, 2017 at 05:51:45PM +0300, Ville Syrjälä wrote: >> > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: >> > > On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: >> > > > Add standard optinal property blobs for YCbCr to RGB conversion CSC >> > > > matrix and YCbCr preoffset vector in DRM planes. New enums are defined >> > > > to YCBCR_ENCODING property to activate the CSC and preoffset >> > > > properties. For simplicity the new properties are stored in the >> > > > drm_plane object, because the YCBCR_ENCODING is already there. The >> > > > blob contents are defined in the uapi/drm/drm_mode.h header. >> > > > >> > > > Signed-off-by: Jyri Sarha <jsarha@ti.com> >> > > >> > > Not sure we want to make this yuv2rgb specific, plenty of planes have >> > > generic degamma/offset, csc, gamme/offset hw. I think what we want instead >> > > is the generic csc support, plus a ycbcr2rgb mode of "bypass". >> > >> > My idea is to not expose this. And instead just expose a normal >> > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm >> > that can't be done by the hw. >> >> But that means we'd need to have a bit-perfect match with a canonical >> conversion matrix (even if maybe the hw has a rounding bug and implements >> something slightly different than the standard). > > I don't see what this has to do with bit correctness. Different hw is > likely to use different precision for this stuff anyway, so I'm 100% > sure we'll not get the same results from all hardware. And really, > getting 100% same output doesn't seem all that important here > anyway. > > Anyway, all I'm saying is that we should expose a standard pipeline: > ycbcr->rgb -> degamma -> ctm -> gamma > > And each driver can then expose whatever parts of that they wish. If you > want to expoe a programmable matrix you expose it as the ctm. I don't > think having a programmable matrix as both ycbcr->rgb and ctm would > really benefit us. Ok, that makes sense. I was somehow assuming you're suggesting we only expose a colors -> degamma -> ctm -> gamma pipeline and drivers with only fixed function ycbcr2rbg conversion would then need to infer which exact btsomethingsomething standard it should pick. The above makes perfect sense, sorry for the confusion. -Daniel
Hi Jyri, On Thursday 04 May 2017 18:23:21 Jyri Sarha wrote: > On 05/04/17 17:51, Ville Syrjälä wrote: > > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote: > >> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote: > >>> Add standard optinal property blobs for YCbCr to RGB conversion CSC > >>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined > >>> to YCBCR_ENCODING property to activate the CSC and preoffset > >>> properties. For simplicity the new properties are stored in the > >>> drm_plane object, because the YCBCR_ENCODING is already there. The > >>> blob contents are defined in the uapi/drm/drm_mode.h header. > >>> > >>> Signed-off-by: Jyri Sarha <jsarha@ti.com> > >> > >> Not sure we want to make this yuv2rgb specific, plenty of planes have > >> generic degamma/offset, csc, gamme/offset hw. I think what we want > >> instead is the generic csc support, plus a ycbcr2rgb mode of "bypass". > > > > My idea is to not expose this. And instead just expose a normal > > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm > > that can't be done by the hw. > > So, pulling the suggestions together the properties would now look like > this: > > YCBCR_RANGE > - Full > - Limited > > YCBCR_ENCODING > - BT.601 > - BT.709 > - BT.2020 > > CTM (blob) > > And all these properties could be added to individual planes. I'll try > to come up with a new patch to add simple enum properties for > YCBCR_PREOFFSET and YCBCR_ENCODING for planes. Should we need any more > enum values for those? > > The only functional thing that is missing from the above proposal is the > possibility to define an arbitrary YCbCr preoffset vector, but that can > be added later if there ever is any real need. > > The other thing that could be added for special purposes would be adding > an "Identity" enum to YCBCR_ENCODING, that would allow setting the YCbCr > CSC trough CTM without any driver level multiplication getting in way, > but that can also be added if there is a need. I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the content of the framebuffer is encoded. If I understand correctly, you're proposing adding an enumeration value that tells the driver not to try to be clever and multiply the CTM matrix by the CSC matrix corresponding to the encoding. That would probably work in most cases, but it would combine two pieces of information in a single property. The driver would then lose the knowledge of how the plane framebuffer is encoded. Couldn't there be cases where that knowledge is needed for other purposes than picking the right CSC matrix ? If so, it might be better to always set the YCBCR_ENCODING property to the actual encoding, and have another property to tell the driver to skip multiplication by the CSC matrix. Or could that be conveyed through the CTM blob property ? Some kind of flag that would essentially tell that the CTM matrix has been pre-multiplied already ? Before I forget, how do you plan to handle backward compatibility with userspace that won't set the YCBCR_ENCODING property ? Is that done by picking a driver-specific default value ? Do you think there would be a need for drivers to know that the property has not been set ?
On Fri, May 5, 2017 at 4:36 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the > content of the framebuffer is encoded. If I understand correctly, you're > proposing adding an enumeration value that tells the driver not to try to be > clever and multiply the CTM matrix by the CSC matrix corresponding to the > encoding. That would probably work in most cases, but it would combine two > pieces of information in a single property. The driver would then lose the > knowledge of how the plane framebuffer is encoded. Couldn't there be cases > where that knowledge is needed for other purposes than picking the right CSC > matrix ? If so, it might be better to always set the YCBCR_ENCODING property > to the actual encoding, and have another property to tell the driver to skip > multiplication by the CSC matrix. Or could that be conveyed through the CTM > blob property ? Some kind of flag that would essentially tell that the CTM > matrix has been pre-multiplied already ? > > Before I forget, how do you plan to handle backward compatibility with > userspace that won't set the YCBCR_ENCODING property ? Is that done by picking > a driver-specific default value ? Do you think there would be a need for > drivers to know that the property has not been set ? Where do we need this? Afaik the encoding is to spec the yuv2rbg conversion function, and that's it. But I'm fairly ignorant about video and yuv and all these things. so does this specify something else? If not, I don't see any possibilities that someone could retrofit more meaning onto these conversion rules. -Daniel
On Fri, May 05, 2017 at 07:17:43PM +0200, Daniel Vetter wrote: > On Fri, May 5, 2017 at 4:36 PM, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the > > content of the framebuffer is encoded. If I understand correctly, you're > > proposing adding an enumeration value that tells the driver not to try to be > > clever and multiply the CTM matrix by the CSC matrix corresponding to the > > encoding. That would probably work in most cases, but it would combine two > > pieces of information in a single property. The driver would then lose the > > knowledge of how the plane framebuffer is encoded. Couldn't there be cases > > where that knowledge is needed for other purposes than picking the right CSC > > matrix ? If so, it might be better to always set the YCBCR_ENCODING property > > to the actual encoding, and have another property to tell the driver to skip > > multiplication by the CSC matrix. Or could that be conveyed through the CTM > > blob property ? Some kind of flag that would essentially tell that the CTM > > matrix has been pre-multiplied already ? > > > > Before I forget, how do you plan to handle backward compatibility with > > userspace that won't set the YCBCR_ENCODING property ? Is that done by picking > > a driver-specific default value ? Do you think there would be a need for > > drivers to know that the property has not been set ? > > Where do we need this? Afaik the encoding is to spec the yuv2rbg > conversion function, and that's it. But I'm fairly ignorant about > video and yuv and all these things. so does this specify something > else? If not, I don't see any possibilities that someone could > retrofit more meaning onto these conversion rules. The question, I believe, is how do we deal with existing userspace that doesn't know about this knob. I think BT.709 is probably what we should use as the default since I'm expecting that would match most non-ancient source material out there. i915 currently uses BT.601 for no particular good reason, but I'm very happy to change that to BT.709. I think for the old video overlay (which isn't exposed as a plane currently) we actually default to BT.709. The only problatic case is when the hardware can't to BT.709 but I'm not sure if that's relevant for any currently supported hardware. I guess we could have the core pick the default based on some priority list eg. BT.709->BT.601->BT.2020. The other option of course being that we just let each driver pick their own default. I guess that might make sense if there's some userspace already out there that expects eg. BT.601. But frankly the difference between 601 and 709 is small enough that I wouldn't expect anyone to scream too loudly if we end up changing the default for someone.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index bcef93d..87a2d55 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -733,6 +733,7 @@ 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); @@ -777,6 +778,18 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, state->zpos = val; } else if (property == plane->ycbcr_encoding_property) { state->ycbcr_encoding = val; + } else if (property == plane->ycbcr_decode_csc_property) { + ret = drm_atomic_replace_property_blob_from_id(dev, + &state->ycbcr_decode_csc, val, + sizeof(struct drm_ycbcr_decode_csc), + &dummy); + return ret; + } else if (property == plane->ycbcr_csc_preoffset_property) { + ret = drm_atomic_replace_property_blob_from_id(dev, + &state->ycbcr_csc_preoffset, val, + sizeof(struct drm_ycbcr_csc_preoffset), + &dummy); + return ret; } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); @@ -839,6 +852,12 @@ int drm_atomic_plane_set_property(struct drm_plane *plane, *val = state->zpos; } else if (property == plane->ycbcr_encoding_property) { *val = state->ycbcr_encoding; + } else if (property == plane->ycbcr_decode_csc_property) { + *val = state->ycbcr_decode_csc ? + state->ycbcr_decode_csc->base.id : 0; + } else if (property == plane->ycbcr_csc_preoffset_property) { + *val = state->ycbcr_csc_preoffset ? + state->ycbcr_csc_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 8be9719..6ecc32f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3238,6 +3238,12 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, { memcpy(state, plane->state, sizeof(*state)); + if (state->ycbcr_decode_csc) + drm_property_blob_get(state->ycbcr_decode_csc); + + if (state->ycbcr_csc_preoffset) + drm_property_blob_get(state->ycbcr_csc_preoffset); + if (state->fb) drm_framebuffer_get(state->fb); @@ -3278,6 +3284,9 @@ struct drm_plane_state * */ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) { + drm_property_blob_put(state->ycbcr_decode_csc); + drm_property_blob_put(state->ycbcr_csc_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 245b14a..319ed46 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -95,8 +95,23 @@ * * "YCBCR_ENCODING" * Optional plane enum property to control YCbCr to RGB - * conversion. The driver provides a subset of standard - * enum values supported by the DRM plane. + * conversion. The driver provides a subset of standard enum + * values supported by the DRM plane. The possible encodings + * include the standard conversions and a possibility to select a + * custom conversion matrix and preoffset vector. + * + * "YCBCR_DECODE_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 + * used depends on the value of YCBCR_ENCODING property. + * + * "YCBCR_CSC_PREOFFSET" + * Optional plane property blob to configure YCbCr offset before + * YCbCr to RGB CSC is applied. The blob contains struct + * drm_ycbcr_csc_preoffset which is defined in + * uapi/drm/drm_mode.h. Whether this property is used depends on + * the value of YCBCR_ENCODING property. */ /** @@ -351,6 +366,9 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, [DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range", [DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range", [DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range", + [DRM_PLANE_YCBCR_CSC_FULL_RANGE] = "YCbCr CSC full range", + [DRM_PLANE_YCBCR_CSC_LIMITED_RANGE] = "YCbCr CSC limited range", + [DRM_PLANE_YCBCR_CSC_PREOFFSET] = "YCbCr CSC and preoffset vector", }; /** @@ -363,6 +381,8 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, * drm_plane object. The supported encodings should be provided in the * enum_list parameter. The enum_list parameter should not contain the * enum names, because the standard names are added by this function. + * YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties are created + * based on the provided enum_list. */ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, struct drm_prop_enum_list *enum_list, @@ -371,6 +391,8 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, { struct drm_device *dev = plane->dev; struct drm_property *prop; + bool ycbcr_decode_csc_create = false; + bool ycbcr_csc_preoffset_create = false; unsigned int i; if (WARN_ON(plane->ycbcr_encoding_property != NULL)) @@ -380,6 +402,15 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; enum_list[i].name = ycbcr_encoding_name[encoding]; + + if (encoding == DRM_PLANE_YCBCR_CSC_FULL_RANGE || + encoding == DRM_PLANE_YCBCR_CSC_LIMITED_RANGE) + ycbcr_decode_csc_create = true; + + if (encoding == DRM_PLANE_YCBCR_CSC_PREOFFSET) { + ycbcr_decode_csc_create = true; + ycbcr_csc_preoffset_create = true; + } } prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, @@ -390,5 +421,25 @@ int drm_plane_create_ycbcr_properties(struct drm_plane *plane, plane->ycbcr_encoding_property = prop; drm_object_attach_property(&plane->base, prop, default_mode); + if (ycbcr_decode_csc_create) { + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | + DRM_MODE_PROP_BLOB, + "YCBCR_DECODE_CSC", 0); + if (!prop) + return -ENOMEM; + plane->ycbcr_decode_csc_property = prop; + drm_object_attach_property(&plane->base, prop, 0); + } + + if (ycbcr_csc_preoffset_create) { + prop = drm_property_create(dev, DRM_MODE_PROP_ATOMIC | + DRM_MODE_PROP_BLOB, + "YCBCR_CSC_PREOFFSET", 0); + if (!prop) + return -ENOMEM; + plane->ycbcr_csc_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 007c4d7..d10c942 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -247,6 +247,12 @@ void drm_plane_cleanup(struct drm_plane *plane) if (plane->ycbcr_encoding_property) drm_property_destroy(dev, plane->ycbcr_encoding_property); + if (plane->ycbcr_decode_csc_property) + drm_property_destroy(dev, plane->ycbcr_decode_csc_property); + + if (plane->ycbcr_csc_preoffset_property) + drm_property_destroy(dev, plane->ycbcr_csc_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 1771394..bdfd37e 100644 --- a/include/drm/drm_color_mgmt.h +++ b/include/drm/drm_color_mgmt.h @@ -44,6 +44,9 @@ enum drm_plane_ycbcr_encoding { DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, + DRM_PLANE_YCBCR_CSC_FULL_RANGE, + DRM_PLANE_YCBCR_CSC_LIMITED_RANGE, + DRM_PLANE_YCBCR_CSC_PREOFFSET, }; int drm_plane_create_ycbcr_properties(struct drm_plane *plane, diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 4d0510f..63b1e0c 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -115,6 +115,8 @@ struct drm_plane_state { /* YCbCr to RGB conversion */ enum drm_plane_ycbcr_encoding ycbcr_encoding; + struct drm_property_blob *ycbcr_decode_csc; + struct drm_property_blob *ycbcr_csc_preoffset; /* Clipped coordinates */ struct drm_rect src, dst; @@ -529,6 +531,8 @@ struct drm_plane { struct drm_property *rotation_property; struct drm_property *ycbcr_encoding_property; + struct drm_property *ycbcr_decode_csc_property; + struct drm_property *ycbcr_csc_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..8c9568d 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_decode_csc { + /* Conversion matrix in 2-complement s32.32 format. */ + __s64 ry, rcb, rcr; + __s64 gy, gcb, gcr; + __s64 by, bcb, bcr; +}; + +struct drm_ycbcr_csc_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 optinal property blobs for YCbCr to RGB conversion CSC matrix and YCbCr preoffset vector in DRM planes. New enums are defined to YCBCR_ENCODING property to activate the CSC and preoffset properties. For simplicity the new properties are stored in the drm_plane object, because the YCBCR_ENCODING is already there. 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 | 19 +++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 9 ++++++ drivers/gpu/drm/drm_color_mgmt.c | 55 +++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/drm_plane.c | 6 ++++ include/drm/drm_color_mgmt.h | 3 ++ include/drm/drm_plane.h | 4 +++ include/uapi/drm/drm_mode.h | 12 ++++++++ 7 files changed, 106 insertions(+), 2 deletions(-)