[PATCH v4 09/17] drm/uAPI: Add "active color range" drm property as feedback for userspace
Werner Sembach
wse at tuxedocomputers.com
Tue Jun 22 09:50:23 UTC 2021
Am 22.06.21 um 09:00 schrieb Pekka Paalanen:
> On Fri, 18 Jun 2021 11:11:08 +0200
> Werner Sembach <wse at tuxedocomputers.com> wrote:
>
>> Add a new general drm property "active color range" which can be used by
>> graphic drivers to report the used color range back to userspace.
>>
>> There was no way to check which color range got actually used on a given
>> monitor. To surely predict this, one must know the exact capabilities of
>> the monitor and what the default behaviour of the used driver is. This
>> property helps eliminating the guessing at this point.
>>
>> In the future, automatic color calibration for screens might also depend on
>> this information being available.
>>
>> Signed-off-by: Werner Sembach <wse at tuxedocomputers.com>
>> ---
>> drivers/gpu/drm/drm_connector.c | 59 +++++++++++++++++++++++++++++++++
>> include/drm/drm_connector.h | 27 +++++++++++++++
>> 2 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index 684d7abdf0eb..818de58d972f 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -897,6 +897,12 @@ static const struct drm_prop_enum_list drm_active_color_format_enum_list[] = {
>> { DRM_COLOR_FORMAT_YCRCB420, "ycbcr420" },
>> };
>>
>> +static const struct drm_prop_enum_list drm_active_color_range_enum_list[] = {
>> + { DRM_MODE_COLOR_RANGE_UNSET, "Unknown" },
>> + { DRM_MODE_COLOR_RANGE_FULL, "Full" },
>> + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" },
> Doesn't "limited" mean different numbers on RGB vs. Y vs. CbCr? I have
> a vague recollection that at least one of them was different from the
> others.
Yes, seems like it does:
https://www.kernel.org/doc/html/v5.12/userspace-api/media/v4l/colorspaces-defs.html#c.V4L.v4l2_quantization
I carried the option names over from "Broadcast RGB", see my other e-mail for more details.
>
> Documenting DRM_MODE_COLOR_RANGE_UNSET as "unspecified/default" while
> the string for it is "Unknown" seems inconsistent to me. I would
> recommend to avoid the word "default" because "reset to defaults" might
> become a thing one day, and that probably is not the same default as
> here.
>
> Is there actually a case for "unknown"? How can it be not known? Or
> does it mean "not applicable"?
Unknown is when no monitor is connected or is when the connector/monitor is disabled.
It also is the initial value when the driver fails to correctly set the property. This shouldn't happen, but I'm
wondering if I should still introduce an _ERROR state instead for this case?
I will rename it, maybe "unset" to match the enum? "not applicable" also fits if either the error state is defined or
not necessary.
>
> Otherwise looks good to me.
>
>
> Thanks,
> pq
>
>
>> +};
>> +
>> DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
>> drm_dp_subconnector_enum_list)
>>
>> @@ -1221,6 +1227,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>> * drm_connector_attach_active_color_format_property() to install this
>> * property.
>> *
>> + * active color range:
>> + * This read-only property tells userspace the color range actually used by
>> + * the hardware display engine on "the cable" on a connector. The chosen
>> + * value depends on hardware capabilities of the monitor and the used color
>> + * format. Drivers shall use
>> + * drm_connector_attach_active_color_range_property() to install this
>> + * property.
>> + *
>> * Connectors also have one standardized atomic property:
>> *
>> * CRTC_ID:
>> @@ -2264,6 +2278,51 @@ void drm_connector_set_active_color_format_property(struct drm_connector *connec
>> }
>> EXPORT_SYMBOL(drm_connector_set_active_color_format_property);
>>
>> +/**
>> + * drm_connector_attach_active_color_range_property - attach "active color range" property
>> + * @connector: connector to attach active color range property on.
>> + *
>> + * This is used to check the applied color range on a connector.
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int drm_connector_attach_active_color_range_property(struct drm_connector *connector)
>> +{
>> + struct drm_device *dev = connector->dev;
>> + struct drm_property *prop;
>> +
>> + if (!connector->active_color_range_property) {
>> + prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE, "active color range",
>> + drm_active_color_range_enum_list,
>> + ARRAY_SIZE(drm_active_color_range_enum_list));
>> + if (!prop)
>> + return -ENOMEM;
>> +
>> + connector->active_color_range_property = prop;
>> + drm_object_attach_property(&connector->base, prop, DRM_MODE_COLOR_RANGE_UNSET);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_active_color_range_property);
>> +
>> +/**
>> + * drm_connector_set_active_color_range_property - sets the active color range property for a
>> + * connector
>> + * @connector: drm connector
>> + * @active_color_range: color range for the connector currently active on "the cable"
>> + *
>> + * Should be used by atomic drivers to update the active color range over a connector.
>> + */
>> +void drm_connector_set_active_color_range_property(struct drm_connector *connector,
>> + enum drm_mode_color_range active_color_range)
>> +{
>> + drm_object_property_set_value(&connector->base, connector->active_color_range_property,
>> + active_color_range);
>> +}
>> +EXPORT_SYMBOL(drm_connector_set_active_color_range_property);
>> +
>> /**
>> * drm_connector_attach_hdr_output_metadata_property - attach "HDR_OUTPUT_METADA" property
>> * @connector: connector to attach the property on.
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 8a5197f14e87..9fb7119b7a02 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -648,6 +648,24 @@ struct drm_tv_connector_state {
>> unsigned int hue;
>> };
>>
>> +/**
>> + * enum drm_mode_color_range - color_range info for &drm_connector
>> + *
>> + * This enum is used to represent full or limited color range on the display
>> + * connector signal.
>> + *
>> + * @DRM_MODE_COLOR_RANGE_UNSET: Color range is unspecified/default.
>> + * @DRM_MODE_COLOR_RANGE_FULL: Color range is full range, 0-255 for
>> + * 8-Bit color depth.
>> + * DRM_MODE_COLOR_RANGE_LIMITED_16_235: Color range is limited range, 16-235 for
>> + * 8-Bit color depth.
>> + */
>> +enum drm_mode_color_range {
>> + DRM_MODE_COLOR_RANGE_UNSET,
>> + DRM_MODE_COLOR_RANGE_FULL,
>> + DRM_MODE_COLOR_RANGE_LIMITED_16_235,
>> +};
>> +
>> /**
>> * struct drm_connector_state - mutable connector state
>> */
>> @@ -1392,6 +1410,12 @@ struct drm_connector {
>> */
>> struct drm_property *active_color_format_property;
>>
>> + /**
>> + * @active_color_range_property: Default connector property for the
>> + * active color range to be driven out of the connector.
>> + */
>> + struct drm_property *active_color_range_property;
>> +
>> #define DRM_CONNECTOR_POLL_HPD (1 << 0)
>> #define DRM_CONNECTOR_POLL_CONNECT (1 << 1)
>> #define DRM_CONNECTOR_POLL_DISCONNECT (1 << 2)
>> @@ -1719,6 +1743,9 @@ void drm_connector_set_active_bpc_property(struct drm_connector *connector, int
>> int drm_connector_attach_active_color_format_property(struct drm_connector *connector);
>> void drm_connector_set_active_color_format_property(struct drm_connector *connector,
>> u32 active_color_format);
>> +int drm_connector_attach_active_color_range_property(struct drm_connector *connector);
>> +void drm_connector_set_active_color_range_property(struct drm_connector *connector,
>> + enum drm_mode_color_range active_color_range);
>>
>> /**
>> * struct drm_tile_group - Tile group metadata
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the dri-devel
mailing list