[PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Laurent Pinchart]

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 at 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]

-- 
Regards,

Laurent Pinchart