On Mon, May 15, 2017 at 05:25:02PM +0300, Jyri Sarha wrote:
> On 05/15/17 16:34, Ville Syrjälä wrote:
> > On Mon, May 15, 2017 at 02:19:09PM +0300, Jyri Sarha wrote:
> >> Add a standard optinal properties to support different non RGB color
> >> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
> >> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
> >> the value ranges within the selected color encoding. The properties
> >> are stored to drm_plane object to allow different set of supported
> >> encoding for different planes on the device.
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
> >> ---
> >> drivers/gpu/drm/drm_atomic.c | 8 ++++
> >> drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
> >> drivers/gpu/drm/drm_plane.c | 6 +++
> >> include/drm/drm_color_mgmt.h | 20 +++++++++
> >> include/drm/drm_plane.h | 8 ++++
> >> 5 files changed, 139 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 4033384..f341dda 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -774,6 +774,10 @@ 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->color_encoding_property) {
> >> + state->color_encoding = val;
> >> + } else if (property == plane->color_range_property) {
> >> + state->color_range = val;
> >> } else if (plane->funcs->atomic_set_property) {
> >> return plane->funcs->atomic_set_property(plane, state,
> >> property, val);
> >> @@ -834,6 +838,10 @@ 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->color_encoding_property) {
> >> + *val = state->color_encoding;
> >> + } else if (property == plane->color_range_property) {
> >> + *val = state->color_range;
> >> } 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..2e04ebb 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 non RGB color encodings is controlled trough
> >> + * plane specific COLOR_ENCODING and COLOR_RANGE properties.
> >
> > Putting this comment in middle of the crtc stuff seems a bit strange
> > when the rest of the plane stuff is at the end.
> >
>
> Ok. I remove them. Of course in the future some one may want to attach
> those proerties to crtcs too, but that is a different story.
True. But at that point I guess we'll have to reword the comment anyway.
>
> > This should specify somewhere that these plane properties control
> > the plane input and not the output.
> >
>
> Ok. I'll make the documentation more verbose.
>
> >> + *
> >> + * 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,18 @@
> >> * 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:
> >> + *
> >> + * "COLOR_ENCODING"
> >> + * Optional plane enum property to support different non RGB
> >> + * color encodings. The driver can provide a subset of standard
> >> + * enum values supported by the DRM plane.
> >> + *
> >> + * "COLOR_RANGE"
> >> + * Optional plane enum property to support different non RGB
> >> + * color parameter ranges. The driver can provide a subset of
> >> + * standard enum values supported by the DRM plane.
> >> */
> >>
> >> /**
> >> @@ -333,3 +350,83 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >> drm_modeset_unlock(&crtc->mutex);
> >> return ret;
> >> }
> >> +
> >> +static char *color_encoding_name[] = {
> >
> > Missing a few consts.
> >
> >> + [DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
> >> + [DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
> >> + [DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
> >> +};
> >> +
> >> +static char *color_range_name[] = {
> >
> > ditto.
> >
>
> Will fix.
>
> >> + [DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr FULL RANGE",
> >> + [DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr LIMITED RANGE",
> >
> > Lowercase for the strings maybe? I guess we don't have any consistent
> > style in this regard.
> >
>
> Lower case of everything but YCbCr, right?
That would work for me. But as stated, we already totally lack a
consistent style so I guess it doesn't matter too much in the end.
Same goes for the prop names, basic atomic props are all caps,
zpos/rotation are all lowercase, and many other props are somewhere
in between.
>
> >> +};
> >> +
> >> +/**
> >> + * drm_plane_create_color_properties - color encoding related plane properties
> >> + * @supported_encodings: bitfield indicating supported color encdings
> >> + * @supported_ranges: bitfileld indicating supported color ranges
> >> + * @default_encoding: default color encoding
> >> + * @default_range: default color range
> >> + *
> >> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
> >> + * properties to to the drm_plane object. The supported encodings and
> >> + * ranges should be provided in supported_encodings and
> >> + * supported_ranges bitmasks. Each bit set in the bitmast indicates
> >> + * the its number as enum value being supported.
> >> + */
> >> +int drm_plane_create_color_properties(struct drm_plane *plane,
> >> + u32 supported_encodings,
> >> + u32 supported_ranges,
> >> + enum drm_color_encoding default_encoding,
> >> + enum drm_color_range default_range)
> >> +{
> >> + struct drm_device *dev = plane->dev;
> >> + struct drm_property *prop;
> >> + struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
> >> + DRM_COLOR_RANGE_MAX)];
> >> + unsigned int i, len;
> >
> > 'unsigned int i' might surprise someone if they ever try to change the
> > loop to go backwards. Better make it signed IMO.
> >
>
> Ok, will fix.
>
> >> +
> >> +
> >> + if (WARN_ON(plane->color_encoding_property != NULL) ||
> >> + WARN_ON(plane->color_range_property != NULL))
> >> + return -EEXIST;
> >
> > I think I already stated that we don't have these sorts of checks
> > elsewhere, so IMO no point in adding them here either unless you
> > go and change all the other places too.
> >
> > Just feels like accumulating random differences that will confuse
> > people in the future.
> >
> >> +
> >> + len = 0;
> >> + for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> >> + if ((BIT(i) & supported_encodings) == 0)
> >
> > 'foo & BIT(i)' would be the more conventional order.
> >
>
> Ok, I'll swap the order.
>
> >> + continue;
> >> +
> >> + enum_list[i].type = i;
> >> + enum_list[i].name = color_encoding_name[i];
> >
> > enum_list[len] ?
> >
>
> Argh. That's a real bug. Thanks, I'll fix that.
>
> >> + len++;
> >> + }
> >> +
> >> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> >> + "COLOR_ENCODING",
> >> + enum_list, len);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + plane->color_encoding_property = prop;
> >> + drm_object_attach_property(&plane->base, prop, default_encoding);
> >
> > Missing the plane->state->whatever setup here.
>
> What do you mean? Initialize the plane->state->color_encoding with the
> default value?
>
> There is no state allocated for a plane at initialization phase, where I
> attach the properties, at least in omapdrm. I somehow expected this to
> be taken care by the default value in the attach call, but now I see it
> does not do that.
Yeah. For the props we just do 'if (state) state->foo = bar;'.
>
> >
> >> +
> >> + len = 0;
> >> + for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> >> + if ((BIT(i) & supported_ranges) == 0)
> >> + continue;
> >> +
> >> + enum_list[i].type = i;
> >> + enum_list[i].name = color_range_name[i];
> >> + len++;
> >> + }
> >> +
> >> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> >> + "COLOR_RANGE",
> >> + enum_list, len);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + plane->color_range_property = prop;
> >> + drm_object_attach_property(&plane->base, prop, default_range);
> >
> > Same issues here
> >
> > I was somewhat wondering if we should even have these two props handled
> > in the same function, but I suppose it does make some sense. Not much
> > point in adding just one but not the other.
> >
>
> I was thinking about the same thing, and decided that if only one range
> is supported, it is better to expose that as a singleton enum as well.
>
> >> +
> >> + return 0;
> >> +}
> >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >> index fedd4d6..a6762e8 100644
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >> @@ -244,6 +244,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >>
> >> kfree(plane->name);
> >>
> >> + if (plane->color_encoding_property)
> >> + drm_property_destroy(dev, plane->color_encoding_property);
> >> +
> >> + if (plane->color_range_property)
> >> + drm_property_destroy(dev, plane->color_range_property);
> >
> > Not needed.
> >
>
> Is that some late addition? I remember having some trouble earlier and
> added these because of that. Maybe I remember wrong...
I believe all props should end up on some global list that gets
cleaned up by the core.
>
> >> +
> >> 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..7a1af63 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,22 @@ 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_color_encoding {
> >> + DRM_COLOR_YCBCR_BT601 = 0,
> >
> > = 0 seems pointless.
> >
>
> I added them just for stating that the code breaks if the enums do not
> start from 0, but I can remove that.
I'm pretty sure enums always start at 0.
>
> >> + DRM_COLOR_YCBCR_BT709,
> >> + DRM_COLOR_YCBCR_BT2020,
> >> + DRM_COLOR_ENCODING_MAX,
> >> +};
> >> +
> >> +enum drm_color_range {
> >> + DRM_COLOR_YCBCR_LIMITED_RANGE = 0,
> >
> > ditto
> >
> >> + DRM_COLOR_YCBCR_FULL_RANGE,
> >> + DRM_COLOR_RANGE_MAX,
> >> +};
> >> +
> >> +int drm_plane_create_color_properties(struct drm_plane *plane,
> >> + u32 supported_encodings,
> >> + u32 supported_ranges,
> >> + enum drm_color_encoding default_encoding,
> >> + enum drm_color_range default_range);
> >> #endif
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index 9ab3e70..f4de5f9 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,10 @@ struct drm_plane_state {
> >> unsigned int zpos;
> >> unsigned int normalized_zpos;
> >>
> >> + /* Color encoding for non RGB formats */
> >> + enum drm_color_encoding color_encoding;
> >> + enum drm_color_range color_range;
> >> +
> >> /* Clipped coordinates */
> >> struct drm_rect src, dst;
> >>
> >> @@ -523,6 +528,9 @@ struct drm_plane {
> >>
> >> struct drm_property *zpos_property;
> >> struct drm_property *rotation_property;
> >> +
> >> + struct drm_property *color_encoding_property;
> >> + struct drm_property *color_range_property;
> >> };
> >>
> >> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> >> --
> >> 1.9.1
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
