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@xxxxxx>
>> ---
>> 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]
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
