[Intel-gfx] [PATCH v4 15/17] drm/uAPI: Move "Broadcast RGB" property from driver specific to general context
Werner Sembach
wse at tuxedocomputers.com
Tue Jun 22 09:57:53 UTC 2021
Am 22.06.21 um 09:25 schrieb Pekka Paalanen:
> On Fri, 18 Jun 2021 11:11:14 +0200
> Werner Sembach <wse at tuxedocomputers.com> wrote:
>
>> Add "Broadcast RGB" to general drm context so that more drivers besides
>> i915 and gma500 can implement it without duplicating code.
>>
>> Userspace can use this property to tell the graphic driver to use full or
>> limited color range for a given connector, overwriting the default
>> behaviour/automatic detection.
>>
>> Possible options are:
>> - Automatic (default/current behaviour)
>> - Full
>> - Limited 16:235
>>
>> In theory the driver should be able to automatically detect the monitors
>> capabilities, but because of flawed standard implementations in Monitors,
>> this might fail. In this case a manual overwrite is required to not have
>> washed out colors or lose details in very dark or bright scenes.
>>
>> Signed-off-by: Werner Sembach <wse at tuxedocomputers.com>
>> ---
>> drivers/gpu/drm/drm_atomic_helper.c | 4 +++
>> drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
>> drivers/gpu/drm/drm_connector.c | 43 +++++++++++++++++++++++++++++
>> include/drm/drm_connector.h | 16 +++++++++++
>> 4 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 90d62f305257..0c89d32efbd0 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -691,6 +691,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>> if (old_connector_state->preferred_color_format !=
>> new_connector_state->preferred_color_format)
>> new_crtc_state->connectors_changed = true;
>> +
>> + if (old_connector_state->preferred_color_range !=
>> + new_connector_state->preferred_color_range)
>> + new_crtc_state->connectors_changed = true;
>> }
>>
>> if (funcs->atomic_check)
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index c536f5e22016..c589bb1a8163 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -798,6 +798,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>> state->max_requested_bpc = val;
>> } else if (property == connector->preferred_color_format_property) {
>> state->preferred_color_format = val;
>> + } else if (property == connector->preferred_color_range_property) {
>> + state->preferred_color_range = val;
>> } else if (connector->funcs->atomic_set_property) {
>> return connector->funcs->atomic_set_property(connector,
>> state, property, val);
>> @@ -877,6 +879,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>> *val = state->max_requested_bpc;
>> } else if (property == connector->preferred_color_format_property) {
>> *val = state->preferred_color_format;
>> + } else if (property == connector->preferred_color_range_property) {
>> + *val = state->preferred_color_range;
>> } else if (connector->funcs->atomic_get_property) {
>> return connector->funcs->atomic_get_property(connector,
>> state, property, val);
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index aea03dd02e33..9bc596638613 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -905,6 +905,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_preferred_color_range_enum_list[] = {
>> + { DRM_MODE_COLOR_RANGE_UNSET, "Automatic" },
>> + { DRM_MODE_COLOR_RANGE_FULL, "Full" },
>> + { DRM_MODE_COLOR_RANGE_LIMITED_16_235, "Limited 16:235" },
> Hi,
>
> the same question here about these numbers as I asked on the "active
> color range" property.
>
>> +};
>> +
>> 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" },
>> @@ -1243,6 +1249,13 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>> * drm_connector_attach_active_color_format_property() to install this
>> * property.
>> *
>> + * Broadcast RGB:
>> + * This property is used by userspace to change the used color range. When
>> + * used the driver will use the selected range if valid for the current
>> + * color format. Drivers to use the function
>> + * drm_connector_attach_preferred_color_format_property() to create and
>> + * attach the property to the connector during initialization.
> An important detail to document here is: does userspace need to
> take care that pixel data at the connector will already match the set
> range, or will the driver program the hardware to produce the set range?
Since until now, the userspace didn't even know for sure if RGB or YCbCr and therefore which full/limited format was
used I guess it's all kernel space conversion.
>
> If the former, then I'm afraid the preference/active property pair
> design does not work. Userspace needs to make sure the content is in
> the right range, so the driver cannot second-guess that afterwards.
>
> If the latter, then what does the driver assume about color range just
> before the automatic conversion to the final color range, and does the
> range conversion happen as the final step in the color pipeline?
>
> If I remember the discussion about Intel right, then the driver does
> the latter and assume that userspace programs KMS to always produce
> full range pixels. There is no provision for userspace to produce
> limited range pixels, IIRC.
I think I remember this too from an answer to one of the revisions of this patchset.
>
>
> Thanks,
> pq
>
>> + *
>> * 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
>> @@ -2324,6 +2337,36 @@ void drm_connector_set_active_color_format_property(struct drm_connector *connec
>> }
>> EXPORT_SYMBOL(drm_connector_set_active_color_format_property);
>>
>> +/**
>> + * drm_connector_attach_preferred_color_range_property - attach "Broadcast RGB" property
>> + * @connector: connector to attach preferred color range property on.
>> + *
>> + * This is used to add support for selecting a color range on a connector.
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int drm_connector_attach_preferred_color_range_property(struct drm_connector *connector)
>> +{
>> + struct drm_device *dev = connector->dev;
>> + struct drm_property *prop;
>> +
>> + if (!connector->preferred_color_range_property) {
>> + prop = drm_property_create_enum(dev, 0, "Broadcast RGB",
>> + drm_preferred_color_range_enum_list,
>> + ARRAY_SIZE(drm_preferred_color_range_enum_list));
>> + if (!prop)
>> + return -ENOMEM;
>> +
>> + connector->preferred_color_range_property = prop;
>> + drm_object_attach_property(&connector->base, prop, DRM_MODE_COLOR_RANGE_UNSET);
>> + connector->state->preferred_color_range = DRM_MODE_COLOR_RANGE_UNSET;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_connector_attach_preferred_color_range_property);
>> +
>> /**
>> * drm_connector_attach_active_color_range_property - attach "active color range" property
>> * @connector: connector to attach active color range property on.
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 7b85407ba45c..b319760d4a8c 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -809,6 +809,15 @@ struct drm_connector_state {
>> */
>> u32 preferred_color_format;
>>
>> + /**
>> + * preferred_color_range: Property set by userspace via "Broadcast RGB"
>> + * property to tell the GPU driver which color range to use. It
>> + * overwrites existing automatic detection mechanisms, if set and valid
>> + * for the current color format. Userspace can check for (un-)successful
>> + * application via the "active color range" property.
>> + */
>> + enum drm_mode_color_range preferred_color_range;
>> +
>> /**
>> * @hdr_output_metadata:
>> * DRM blob property for HDR output metadata
>> @@ -1426,6 +1435,12 @@ struct drm_connector {
>> */
>> struct drm_property *active_color_format_property;
>>
>> + /**
>> + * @preferred_color_range_property: Default connector property for the
>> + * preferred color range to be driven out of the connector.
>> + */
>> + struct drm_property *preferred_color_range_property;
>> +
>> /**
>> * @active_color_range_property: Default connector property for the
>> * active color range to be driven out of the connector.
>> @@ -1760,6 +1775,7 @@ int drm_connector_attach_preferred_color_format_property(struct drm_connector *c
>> 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_preferred_color_range_property(struct drm_connector *connector);
>> 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);
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the Intel-gfx
mailing list