| Message ID | 7e2f54feee6108f859a11ac08e0377592d21e2e3.1493881618.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jyri, On 05/04/17 09:14, Jyri Sarha wrote: > Add a standard optinal property to control YCbCr conversion in DRM > planes. The property is stored to drm_plane object to allow different > set of supported conversion modes for different planes on the device. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/drm_atomic.c | 5 ++++ > drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_plane.c | 3 ++ > include/drm/drm_color_mgmt.h | 14 ++++++++++ > include/drm/drm_plane.h | 6 ++++ > 5 files changed, 87 insertions(+) > <snip> > @@ -37,4 +39,16 @@ 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_encoding { > + DRM_PLANE_YCBCR_BT601_FULL_RANGE = 0, > + DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, > + DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, > + DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, > +}; I did a lot of work and research into colorspace handling for V4L2, and I strongly recommend that you do not combine the ycbcr encoding with the quantization range setting. I.e., make this two properties. This is a good reference (but I'm biased, since I wrote it :-) ): https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/colorspaces.html (and the two following sections as well). The four 'parameters' that control how to interpret colors (colorspace, transfer function, ycbcr encoding, quantization range) are all independent of one another, and combining two or more into one property will become a hassle to maintain. Regards, Hans
On Thu, May 04, 2017 at 10:14:25AM +0300, Jyri Sarha wrote: > Add a standard optinal property to control YCbCr conversion in DRM > planes. The property is stored to drm_plane object to allow different > set of supported conversion modes for different planes on the device. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/drm_atomic.c | 5 ++++ > drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_plane.c | 3 ++ > include/drm/drm_color_mgmt.h | 14 ++++++++++ > include/drm/drm_plane.h | 6 ++++ > 5 files changed, 87 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 4033384..bcef93d 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -732,6 +732,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; > > if (property == config->prop_fb_id) { > struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); > @@ -774,6 +775,8 @@ 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_encoding_property) { > + state->ycbcr_encoding = val; > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > @@ -834,6 +837,8 @@ 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_encoding_property) { > + *val = state->ycbcr_encoding; > } 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_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > index 533f3a3..245b14a 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -33,6 +33,11 @@ > * properties on the &drm_crtc object. They are set up by calling > * drm_crtc_enable_color_mgmt(). > * > + * Support for different YCbCr color encodings is controlled trough > + * plane specific YCBCR_ENCODING property. > + * > + * The &drm_crtc object's properties are: > + * > * "DEGAMMA_LUT”: > * Blob property to set the degamma lookup table (LUT) mapping pixel data > * from the framebuffer before it is given to the transformation matrix. > @@ -85,6 +90,13 @@ > * 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_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. > */ > > /** > @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > drm_modeset_unlock(&crtc->mutex); > return ret; > } > + > +static char *ycbcr_encoding_name[] = { > + [DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range", Why full range for BT.601 but not the others? I presume the full range here refers to the YCbCr data, and RGB will always be full range? > + [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_create_ycbcr_properties - ycbcr related plane properties > + * @enum_list: drm_prop_enum_list array of supported modes without names > + * @enum_list_len: length of enum_list array > + * @default_mode: default YCbCr encoding > + * > + * Create and attach plane specific YCBCR_ENCODING property to to the > + * 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. > + */ > +int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > + struct drm_prop_enum_list *enum_list, > + unsigned int enum_list_len, This API doesn't seem very nice because you have to pass in a non-const enum list. Could we perhaps pass the supported values as a bitmask instead? Or const int []? > + enum drm_plane_ycbcr_encoding default_mode) > +{ > + struct drm_device *dev = plane->dev; > + struct drm_property *prop; > + unsigned int i; > + > + if (WARN_ON(plane->ycbcr_encoding_property != NULL)) > + return -EEXIST; We don't have those kinds of checks elsewhere, so why would we want it here? > + > + for (i = 0; i < enum_list_len; i++) { > + enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; > + > + enum_list[i].name = ycbcr_encoding_name[encoding]; > + } > + > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, > + "YCBCR_ENCODING", > + enum_list, enum_list_len); > + if (!prop) > + return -ENOMEM; > + plane->ycbcr_encoding_property = prop; > + drm_object_attach_property(&plane->base, prop, default_mode); Missing the state->ycbcr_encoding setup. > + > + return 0; > +} > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index fedd4d6..007c4d7 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane) > > kfree(plane->name); > > + if (plane->ycbcr_encoding_property) > + drm_property_destroy(dev, plane->ycbcr_encoding_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..1771394 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,16 @@ 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_encoding { > + DRM_PLANE_YCBCR_BT601_FULL_RANGE = 0, > + DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, > + DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, > + DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, > +}; > + > +int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > + struct drm_prop_enum_list *enum_list, > + unsigned int enum_list_len, > + enum drm_plane_ycbcr_encoding default_mode); > + > #endif > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 9ab3e70..4d0510f 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,9 @@ struct drm_plane_state { > unsigned int zpos; > unsigned int normalized_zpos; > > + /* YCbCr to RGB conversion */ > + enum drm_plane_ycbcr_encoding ycbcr_encoding; > + > /* Clipped coordinates */ > struct drm_rect src, dst; > > @@ -523,6 +527,8 @@ struct drm_plane { > > struct drm_property *zpos_property; > struct drm_property *rotation_property; > + > + struct drm_property *ycbcr_encoding_property; > }; > > #define obj_to_plane(x) container_of(x, struct drm_plane, base) > -- > 1.9.1
Hi Jyri, Thank you for the patch. On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote: > Add a standard optinal property to control YCbCr conversion in DRM > planes. The property is stored to drm_plane object to allow different > set of supported conversion modes for different planes on the device. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/drm_atomic.c | 5 ++++ > drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_plane.c | 3 ++ > include/drm/drm_color_mgmt.h | 14 ++++++++++ > include/drm/drm_plane.h | 6 ++++ > 5 files changed, 87 insertions(+) [snip] > diff --git a/drivers/gpu/drm/drm_color_mgmt.c > b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c [snip] > @@ -85,6 +90,13 @@ > * 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_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. > */ As already mentioned by Hans Verkuil, I also recommend not mixing the encoding and quantization in a single property. If you split them, I would then drop the YCBCR_ prefix (or replace it by something more generic) at least for the quantization property, as it would apply to RGB as well. For the encoding property, we have support in V4L2 for a two HSV encodings, so it could make sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt we'll see any display hardware with native support for HSV :-) > /** > @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > drm_modeset_unlock(&crtc->mutex); > return ret; > } > + > +static char *ycbcr_encoding_name[] = { > + [DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range", > + [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_create_ycbcr_properties - ycbcr related plane properties > + * @enum_list: drm_prop_enum_list array of supported modes without names > + * @enum_list_len: length of enum_list array > + * @default_mode: default YCbCr encoding > + * > + * Create and attach plane specific YCBCR_ENCODING property to to the > + * 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. > + */ > +int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > + struct drm_prop_enum_list *enum_list, > + unsigned int enum_list_len, > + enum drm_plane_ycbcr_encoding default_mode) I wonder whether we shouldn't simplify the API by passing a bitmask of supported encodings. Sure, you would have to allocate the array of drm_prop_enum_list internally in the function, but driver code would be simpler. Even if you don't like bitmasks, I think we should pass a const pointer and duplicate the array internally to fill the names. Drivers will in many cases pass the same array for all planes, modifying it in the function is asking for trouble (even if it should be OK with the current implementation). By the way, for drivers that support the same encodings for all planes, would it be worth it to support allocation of a single property instead of allocating one per plane ? > +{ > + struct drm_device *dev = plane->dev; > + struct drm_property *prop; > + unsigned int i; > + > + if (WARN_ON(plane->ycbcr_encoding_property != NULL)) > + return -EEXIST; > + > + for (i = 0; i < enum_list_len; i++) { > + enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; > + > + enum_list[i].name = ycbcr_encoding_name[encoding]; > + } > + > + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, > + "YCBCR_ENCODING", > + enum_list, enum_list_len); > + if (!prop) > + return -ENOMEM; > + plane->ycbcr_encoding_property = prop; > + drm_object_attach_property(&plane->base, prop, default_mode); > + > + return 0; > +} > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index fedd4d6..007c4d7 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane) > > kfree(plane->name); > > + if (plane->ycbcr_encoding_property) > + drm_property_destroy(dev, plane->ycbcr_encoding_property); There's lots of similar code all over the place, I wonder whether we shouldn't move the NULL check to drm_property_destroy(). > + > memset(plane, 0, sizeof(*plane)); > } > EXPORT_SYMBOL(drm_plane_cleanup); [snip]
On 05/05/17 12:07, Laurent Pinchart wrote: > Hi Jyri, > > Thank you for the patch. > > On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote: >> Add a standard optinal property to control YCbCr conversion in DRM >> planes. The property is stored to drm_plane object to allow different >> set of supported conversion modes for different planes on the device. >> >> Signed-off-by: Jyri Sarha <jsarha@ti.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 5 ++++ >> drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_plane.c | 3 ++ >> include/drm/drm_color_mgmt.h | 14 ++++++++++ >> include/drm/drm_plane.h | 6 ++++ >> 5 files changed, 87 insertions(+) > > [snip] > >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c >> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644 >> --- a/drivers/gpu/drm/drm_color_mgmt.c >> +++ b/drivers/gpu/drm/drm_color_mgmt.c > > [snip] > >> @@ -85,6 +90,13 @@ >> * 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_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. >> */ > > As already mentioned by Hans Verkuil, I also recommend not mixing the encoding > and quantization in a single property. If you split them, I would then drop > the YCBCR_ prefix (or replace it by something more generic) at least for the > quantization property, as it would apply to RGB as well. For the encoding > property, we have support in V4L2 for a two HSV encodings, so it could make > sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt > we'll see any display hardware with native support for HSV :-) > COLOR_ENCODING? Why not, the YCbCr could then be in the enum names. >> /** >> @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, >> drm_modeset_unlock(&crtc->mutex); >> return ret; >> } >> + >> +static char *ycbcr_encoding_name[] = { >> + [DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range", >> + [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_create_ycbcr_properties - ycbcr related plane properties >> + * @enum_list: drm_prop_enum_list array of supported modes without names >> + * @enum_list_len: length of enum_list array >> + * @default_mode: default YCbCr encoding >> + * >> + * Create and attach plane specific YCBCR_ENCODING property to to the >> + * 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. >> + */ >> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane, >> + struct drm_prop_enum_list *enum_list, >> + unsigned int enum_list_len, >> + enum drm_plane_ycbcr_encoding default_mode) > > I wonder whether we shouldn't simplify the API by passing a bitmask of > supported encodings. Sure, you would have to allocate the array of > drm_prop_enum_list internally in the function, but driver code would be > simpler. Even if you don't like bitmasks, I think we should pass a const > pointer and duplicate the array internally to fill the names. Drivers will in > many cases pass the same array for all planes, modifying it in the function is > asking for trouble (even if it should be OK with the current implementation). > I used a bitmask property first, but abandoned it because the encodings do not behave like bitmasks. You can not have BT.601 | BT.709 at the same time. A bitmask in the function API would certainly work and be probably better, but I've tried to keep the implementation simple, while we are still discussing what we should actually do. > By the way, for drivers that support the same encodings for all planes, would > it be worth it to support allocation of a single property instead of > allocating one per plane ? I was thinking of that, but AFAIK there is really not that many planes on the know HW that it would justify the complexity. > >> +{ >> + struct drm_device *dev = plane->dev; >> + struct drm_property *prop; >> + unsigned int i; >> + >> + if (WARN_ON(plane->ycbcr_encoding_property != NULL)) >> + return -EEXIST; >> + >> + for (i = 0; i < enum_list_len; i++) { >> + enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; >> + >> + enum_list[i].name = ycbcr_encoding_name[encoding]; >> + } >> + >> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, >> + "YCBCR_ENCODING", >> + enum_list, enum_list_len); >> + if (!prop) >> + return -ENOMEM; >> + plane->ycbcr_encoding_property = prop; >> + drm_object_attach_property(&plane->base, prop, default_mode); >> + >> + return 0; >> +} >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c >> index fedd4d6..007c4d7 100644 >> --- a/drivers/gpu/drm/drm_plane.c >> +++ b/drivers/gpu/drm/drm_plane.c >> @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane) >> >> kfree(plane->name); >> >> + if (plane->ycbcr_encoding_property) >> + drm_property_destroy(dev, plane->ycbcr_encoding_property); > > There's lots of similar code all over the place, I wonder whether we shouldn't > move the NULL check to drm_property_destroy(). > Absolutely. >> + >> memset(plane, 0, sizeof(*plane)); >> } >> EXPORT_SYMBOL(drm_plane_cleanup); > > [snip] >
Hi Jyri, On Friday 05 May 2017 15:11:06 Jyri Sarha wrote: > On 05/05/17 12:07, Laurent Pinchart wrote: > > On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote: > >> Add a standard optinal property to control YCbCr conversion in DRM > >> planes. The property is stored to drm_plane object to allow different > >> set of supported conversion modes for different planes on the device. > >> > >> Signed-off-by: Jyri Sarha <jsarha@ti.com> > >> --- > >> > >> drivers/gpu/drm/drm_atomic.c | 5 ++++ > >> drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/drm_plane.c | 3 ++ > >> include/drm/drm_color_mgmt.h | 14 ++++++++++ > >> include/drm/drm_plane.h | 6 ++++ > >> 5 files changed, 87 insertions(+) > > > > [snip] > > > >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c > >> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644 > >> --- a/drivers/gpu/drm/drm_color_mgmt.c > >> +++ b/drivers/gpu/drm/drm_color_mgmt.c > > > > [snip] > > > >> @@ -85,6 +90,13 @@ > >> * 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_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. > >> */ > > > > As already mentioned by Hans Verkuil, I also recommend not mixing the > > encoding and quantization in a single property. If you split them, I > > would then drop the YCBCR_ prefix (or replace it by something more > > generic) at least for the quantization property, as it would apply to RGB > > as well. For the encoding property, we have support in V4L2 for a two HSV > > encodings, so it could make sense to drop or replace the YCBCR_ prefix, > > but on the other hand I doubt we'll see any display hardware with native > > support for HSV :-) > > COLOR_ENCODING? Why not, the YCbCr could then be in the enum names. Sounds good to me. > >> /** > >> > >> @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, > >> drm_modeset_unlock(&crtc->mutex); > >> return ret; > >> } > >> + > >> +static char *ycbcr_encoding_name[] = { > >> + [DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range", > >> + [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_create_ycbcr_properties - ycbcr related plane properties > >> + * @enum_list: drm_prop_enum_list array of supported modes without names > >> + * @enum_list_len: length of enum_list array > >> + * @default_mode: default YCbCr encoding > >> + * > >> + * Create and attach plane specific YCBCR_ENCODING property to to the > >> + * 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. > >> + */ > >> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane, > >> + struct drm_prop_enum_list *enum_list, > >> + unsigned int enum_list_len, > >> + enum drm_plane_ycbcr_encoding default_mode) > > > > I wonder whether we shouldn't simplify the API by passing a bitmask of > > supported encodings. Sure, you would have to allocate the array of > > drm_prop_enum_list internally in the function, but driver code would be > > simpler. Even if you don't like bitmasks, I think we should pass a const > > pointer and duplicate the array internally to fill the names. Drivers will > > in many cases pass the same array for all planes, modifying it in the > > function is asking for trouble (even if it should be OK with the current > > implementation). > > I used a bitmask property first, but abandoned it because the encodings > do not behave like bitmasks. You can not have BT.601 | BT.709 at the > same time. Sorry, I should have expressed myself more clearly, I meant an enum property with a bitmask in the function API. I agree that a bitmask property isn't a good match. > A bitmask in the function API would certainly work and be probably > better, but I've tried to keep the implementation simple, while we are > still discussing what we should actually do. I think we're getting there with 1/2, so feel free to update that in the next version :-) > > By the way, for drivers that support the same encodings for all planes, > > would it be worth it to support allocation of a single property instead > > of allocating one per plane ? > > I was thinking of that, but AFAIK there is really not that many planes > on the know HW that it would justify the complexity. > > >> +{ > >> + struct drm_device *dev = plane->dev; > >> + struct drm_property *prop; > >> + unsigned int i; > >> + > >> + if (WARN_ON(plane->ycbcr_encoding_property != NULL)) > >> + return -EEXIST; > >> + > >> + for (i = 0; i < enum_list_len; i++) { > >> + enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; > >> + > >> + enum_list[i].name = ycbcr_encoding_name[encoding]; > >> + } > >> + > >> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, > >> + "YCBCR_ENCODING", > >> + enum_list, enum_list_len); > >> + if (!prop) > >> + return -ENOMEM; > >> + plane->ycbcr_encoding_property = prop; > >> + drm_object_attach_property(&plane->base, prop, default_mode); > >> + > >> + return 0; > >> +} > >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > >> index fedd4d6..007c4d7 100644 > >> --- a/drivers/gpu/drm/drm_plane.c > >> +++ b/drivers/gpu/drm/drm_plane.c > >> @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane) > >> > >> kfree(plane->name); > >> > >> + if (plane->ycbcr_encoding_property) > >> + drm_property_destroy(dev, plane->ycbcr_encoding_property); > > > > There's lots of similar code all over the place, I wonder whether we > > shouldn't move the NULL check to drm_property_destroy(). > > Absolutely. > > >> + > >> > >> memset(plane, 0, sizeof(*plane)); > >> > >> } > >> EXPORT_SYMBOL(drm_plane_cleanup); > > > > [snip]
On 05/05/17 16:03, Laurent Pinchart wrote: > Hi Jyri, > > On Friday 05 May 2017 15:11:06 Jyri Sarha wrote: >> On 05/05/17 12:07, Laurent Pinchart wrote: >>> On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote: >>>> Add a standard optinal property to control YCbCr conversion in DRM >>>> planes. The property is stored to drm_plane object to allow different >>>> set of supported conversion modes for different planes on the device. >>>> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com> >>>> --- >>>> >>>> drivers/gpu/drm/drm_atomic.c | 5 ++++ >>>> drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/drm_plane.c | 3 ++ >>>> include/drm/drm_color_mgmt.h | 14 ++++++++++ >>>> include/drm/drm_plane.h | 6 ++++ >>>> 5 files changed, 87 insertions(+) >>> >>> [snip] >>> >>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c >>>> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644 >>>> --- a/drivers/gpu/drm/drm_color_mgmt.c >>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c >>> >>> [snip] >>> >>>> @@ -85,6 +90,13 @@ >>>> * 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_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. >>>> */ >>> >>> As already mentioned by Hans Verkuil, I also recommend not mixing the >>> encoding and quantization in a single property. If you split them, I >>> would then drop the YCBCR_ prefix (or replace it by something more >>> generic) at least for the quantization property, as it would apply to RGB >>> as well. For the encoding property, we have support in V4L2 for a two HSV >>> encodings, so it could make sense to drop or replace the YCBCR_ prefix, >>> but on the other hand I doubt we'll see any display hardware with native >>> support for HSV :-) >> >> COLOR_ENCODING? Why not, the YCbCr could then be in the enum names. > > Sounds good to me. > I am now implementing this. However, there is a logical challenge in putting the two suggestions together, splitting the encoding and range to separate properties, and changing the property names to generic COLOR_ENCODING and COLOR_RANGE. In general COLOR_RANGE enum values are only defined within the selected COLOR_ENCODING. With the standard YCbCr encodings this is not a problem, since they all define a "full range" and a "limted range". But if we are preparing for some unknown color ecodings, I am not sure how likely it is that "full range" and "limited range" options make sense there. I can implement the properties for currently known YCbCr color encodings either way, either as YCbCr specific properties, or as generic COLOR_* properties for all non RGB encodings. I am just not sure if defining the generic properties would make any sense now that we (or at least I) have no idea what the other non RGB encodings could be. What do you think? Cheers, Jyri
Hi Jyri, On Thursday 11 May 2017 17:30:01 Jyri Sarha wrote: > On 05/05/17 16:03, Laurent Pinchart wrote: > > On Friday 05 May 2017 15:11:06 Jyri Sarha wrote: > >> On 05/05/17 12:07, Laurent Pinchart wrote: > >>> On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote: > >>>> Add a standard optinal property to control YCbCr conversion in DRM > >>>> planes. The property is stored to drm_plane object to allow different > >>>> set of supported conversion modes for different planes on the device. > >>>> > >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com> > >>>> --- > >>>> > >>>> drivers/gpu/drm/drm_atomic.c | 5 ++++ > >>>> drivers/gpu/drm/drm_color_mgmt.c | 59 +++++++++++++++++++++++++++++++ > >>>> drivers/gpu/drm/drm_plane.c | 3 ++ > >>>> include/drm/drm_color_mgmt.h | 14 ++++++++++ > >>>> include/drm/drm_plane.h | 6 ++++ > >>>> 5 files changed, 87 insertions(+) > >>> > >>> [snip] > >>> > >>>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c > >>>> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644 > >>>> --- a/drivers/gpu/drm/drm_color_mgmt.c > >>>> +++ b/drivers/gpu/drm/drm_color_mgmt.c > >>> > >>> [snip] > >>> > >>>> @@ -85,6 +90,13 @@ > >>>> * 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_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. > >>>> */ > >>> > >>> As already mentioned by Hans Verkuil, I also recommend not mixing the > >>> encoding and quantization in a single property. If you split them, I > >>> would then drop the YCBCR_ prefix (or replace it by something more > >>> generic) at least for the quantization property, as it would apply to > >>> RGB as well. For the encoding property, we have support in V4L2 for a > >>> two HSV encodings, so it could make sense to drop or replace the YCBCR_ > >>> prefix, but on the other hand I doubt we'll see any display hardware > >>> with native support for HSV :-) > >> > >> COLOR_ENCODING? Why not, the YCbCr could then be in the enum names. > > > > Sounds good to me. > > I am now implementing this. However, there is a logical challenge in > putting the two suggestions together, splitting the encoding and range > to separate properties, and changing the property names to generic > COLOR_ENCODING and COLOR_RANGE. > > In general COLOR_RANGE enum values are only defined within the selected > COLOR_ENCODING. With the standard YCbCr encodings this is not a problem, > since they all define a "full range" and a "limted range". But if we are > preparing for some unknown color ecodings, I am not sure how likely it > is that "full range" and "limited range" options make sense there. > > I can implement the properties for currently known YCbCr color encodings > either way, either as YCbCr specific properties, or as generic COLOR_* > properties for all non RGB encodings. I am just not sure if defining the > generic properties would make any sense now that we (or at least I) have > no idea what the other non RGB encodings could be. What do you think? I won't claim to know what quantization methods will make sense for non-YCbCr encodings in the future (or, for that matter, even for YCbCr encodings). We might even not need any new quantization method. However I don't think this is an argument to not standardize a quantization method property. We can start with an enumerated property with two values, clearly defined in the API documentation as applying to YCbCr only. If and when we'll need quantization methods for other encodings, we can just add values to that enumeration. I would have proposed to even reuse the enumerated values for a different purpose depending on the colour encoding, but as the DRM API exposes enumerated values as strings, we unfortunately can't do that.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 4033384..bcef93d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -732,6 +732,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; if (property == config->prop_fb_id) { struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); @@ -774,6 +775,8 @@ 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_encoding_property) { + state->ycbcr_encoding = val; } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); @@ -834,6 +837,8 @@ 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_encoding_property) { + *val = state->ycbcr_encoding; } 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_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -33,6 +33,11 @@ * properties on the &drm_crtc object. They are set up by calling * drm_crtc_enable_color_mgmt(). * + * Support for different YCbCr color encodings is controlled trough + * plane specific YCBCR_ENCODING property. + * + * The &drm_crtc object's properties are: + * * "DEGAMMA_LUT”: * Blob property to set the degamma lookup table (LUT) mapping pixel data * from the framebuffer before it is given to the transformation matrix. @@ -85,6 +90,13 @@ * 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_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. */ /** @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev, drm_modeset_unlock(&crtc->mutex); return ret; } + +static char *ycbcr_encoding_name[] = { + [DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range", + [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_create_ycbcr_properties - ycbcr related plane properties + * @enum_list: drm_prop_enum_list array of supported modes without names + * @enum_list_len: length of enum_list array + * @default_mode: default YCbCr encoding + * + * Create and attach plane specific YCBCR_ENCODING property to to the + * 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. + */ +int drm_plane_create_ycbcr_properties(struct drm_plane *plane, + struct drm_prop_enum_list *enum_list, + unsigned int enum_list_len, + enum drm_plane_ycbcr_encoding default_mode) +{ + struct drm_device *dev = plane->dev; + struct drm_property *prop; + unsigned int i; + + if (WARN_ON(plane->ycbcr_encoding_property != NULL)) + return -EEXIST; + + for (i = 0; i < enum_list_len; i++) { + enum drm_plane_ycbcr_encoding encoding = enum_list[i].type; + + enum_list[i].name = ycbcr_encoding_name[encoding]; + } + + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC, + "YCBCR_ENCODING", + enum_list, enum_list_len); + if (!prop) + return -ENOMEM; + plane->ycbcr_encoding_property = prop; + drm_object_attach_property(&plane->base, prop, default_mode); + + return 0; +} diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index fedd4d6..007c4d7 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane) kfree(plane->name); + if (plane->ycbcr_encoding_property) + drm_property_destroy(dev, plane->ycbcr_encoding_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..1771394 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,16 @@ 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_encoding { + DRM_PLANE_YCBCR_BT601_FULL_RANGE = 0, + DRM_PLANE_YCBCR_BT601_LIMITED_RANGE, + DRM_PLANE_YCBCR_BT709_LIMITED_RANGE, + DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE, +}; + +int drm_plane_create_ycbcr_properties(struct drm_plane *plane, + struct drm_prop_enum_list *enum_list, + unsigned int enum_list_len, + enum drm_plane_ycbcr_encoding default_mode); + #endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 9ab3e70..4d0510f 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,9 @@ struct drm_plane_state { unsigned int zpos; unsigned int normalized_zpos; + /* YCbCr to RGB conversion */ + enum drm_plane_ycbcr_encoding ycbcr_encoding; + /* Clipped coordinates */ struct drm_rect src, dst; @@ -523,6 +527,8 @@ struct drm_plane { struct drm_property *zpos_property; struct drm_property *rotation_property; + + struct drm_property *ycbcr_encoding_property; }; #define obj_to_plane(x) container_of(x, struct drm_plane, base)
Add a standard optinal property to control YCbCr conversion in DRM planes. The property is stored to drm_plane object to allow different set of supported conversion modes for different planes on the device. Signed-off-by: Jyri Sarha <jsarha@ti.com> --- drivers/gpu/drm/drm_atomic.c | 5 ++++ drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_plane.c | 3 ++ include/drm/drm_color_mgmt.h | 14 ++++++++++ include/drm/drm_plane.h | 6 ++++ 5 files changed, 87 insertions(+)