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