From: Werner Sembach <wse@tuxedocomputers.com>
To: Pekka Paalanen <ppaalanen@gmail.com>
Cc: amd-gfx@lists.freedesktop.org, tzimmermann@suse.de,
intel-gfx@lists.freedesktop.org, sunpeng.li@amd.com,
dri-devel@lists.freedesktop.org, joonas.lahtinen@linux.intel.com,
maarten.lankhorst@linux.intel.com, linux-kernel@vger.kernel.org,
mripard@kernel.org, airlied@linux.ie,
jani.nikula@linux.intel.com, daniel@ffwll.ch,
rodrigo.vivi@intel.com, alexander.deucher@amd.com,
harry.wentland@amd.com, christian.koenig@amd.com
Subject: Re: [PATCH v4 15/17] drm/uAPI: Move "Broadcast RGB" property from driver specific to general context
Date: Tue, 22 Jun 2021 11:57:53 +0200 [thread overview]
Message-ID: <70d89fe2-4e45-7ea9-2509-15257ef222f8@tuxedocomputers.com> (raw)
In-Reply-To: <20210622102529.5266e87b@eldfell>
Am 22.06.21 um 09:25 schrieb Pekka Paalanen:
> On Fri, 18 Jun 2021 11:11:14 +0200
> Werner Sembach <wse@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2021-06-22 9:58 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-18 9:10 [PATCH v4 00/17] New uAPI drm properties for color management Werner Sembach
2021-06-18 9:11 ` [PATCH v4 01/17] drm/amd/display: Remove unnecessary SIGNAL_TYPE_HDMI_TYPE_A check Werner Sembach
2021-06-18 9:11 ` [PATCH v4 02/17] drm/amd/display: Add missing cases convert_dc_color_depth_into_bpc Werner Sembach
2021-06-18 9:11 ` [PATCH v4 03/17] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property Werner Sembach
2021-06-22 6:46 ` Pekka Paalanen
2021-06-28 17:03 ` Werner Sembach
2021-06-29 11:02 ` Werner Sembach
2021-06-30 8:21 ` Pekka Paalanen
2021-06-30 9:42 ` Werner Sembach
2021-07-01 7:42 ` Pekka Paalanen
2021-07-01 11:30 ` Werner Sembach
2021-07-14 18:18 ` Werner Sembach
2021-07-15 9:10 ` Pekka Paalanen
2021-06-18 9:11 ` [PATCH v4 04/17] drm/amd/display: Add handling for new "active bpc" property Werner Sembach
2021-06-18 9:11 ` [PATCH v4 05/17] drm/i915/display: " Werner Sembach
2021-06-18 9:11 ` [PATCH v4 06/17] drm/uAPI: Add "active color format" drm property as feedback for userspace Werner Sembach
2021-06-22 6:48 ` Pekka Paalanen
2021-06-18 9:11 ` [PATCH v4 07/17] drm/amd/display: Add handling for new "active color format" property Werner Sembach
2021-06-18 9:11 ` [PATCH v4 08/17] drm/i915/display: " Werner Sembach
2021-06-18 9:11 ` [PATCH v4 09/17] drm/uAPI: Add "active color range" drm property as feedback for userspace Werner Sembach
2021-06-22 7:00 ` Pekka Paalanen
2021-06-22 9:50 ` Werner Sembach
2021-06-22 11:48 ` Simon Ser
2021-06-23 7:32 ` Pekka Paalanen
2021-06-23 10:17 ` Werner Sembach
2021-06-23 11:14 ` Pekka Paalanen
2021-06-23 11:19 ` Werner Sembach
2021-06-18 9:11 ` [PATCH v4 10/17] drm/amd/display: Add handling for new "active color range" property Werner Sembach
2021-06-18 9:11 ` [PATCH v4 11/17] drm/i915/display: " Werner Sembach
2021-06-18 9:11 ` [PATCH v4 12/17] drm/uAPI: Add "preferred color format" drm property as setting for userspace Werner Sembach
2021-06-22 7:15 ` Pekka Paalanen
2021-06-29 8:12 ` Simon Ser
2021-06-29 11:17 ` Pekka Paalanen
2021-06-29 11:39 ` Werner Sembach
2021-06-30 8:41 ` Pekka Paalanen
2021-06-30 9:20 ` Werner Sembach
2021-07-01 8:07 ` Pekka Paalanen
2021-07-01 12:50 ` Werner Sembach
2021-07-01 13:24 ` Pekka Paalanen
2021-07-05 15:49 ` Werner Sembach
2021-07-06 7:09 ` Pekka Paalanen
2021-07-14 17:59 ` Werner Sembach
2021-06-18 9:11 ` [PATCH v4 13/17] drm/amd/display: Add handling for new "preferred color format" property Werner Sembach
2021-06-18 9:11 ` [PATCH v4 14/17] drm/i915/display: " Werner Sembach
2021-06-18 9:11 ` [PATCH v4 15/17] drm/uAPI: Move "Broadcast RGB" property from driver specific to general context Werner Sembach
2021-06-22 7:25 ` Pekka Paalanen
2021-06-22 9:57 ` Werner Sembach [this message]
2021-06-23 7:48 ` Pekka Paalanen
2021-06-23 10:10 ` Werner Sembach
2021-06-23 11:26 ` Pekka Paalanen
2021-06-25 8:48 ` Werner Sembach
2021-06-18 9:11 ` [PATCH v4 16/17] drm/i915/display: Use the general "Broadcast RGB" implementation Werner Sembach
2021-06-18 9:11 ` [PATCH v4 17/17] drm/amd/display: Add handling for new "Broadcast RGB" property Werner Sembach
2021-06-22 7:29 ` Pekka Paalanen
2021-06-22 9:28 ` Werner Sembach
2021-06-23 8:01 ` Pekka Paalanen
2021-06-23 9:58 ` Werner Sembach
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=70d89fe2-4e45-7ea9-2509-15257ef222f8@tuxedocomputers.com \
--to=wse@tuxedocomputers.com \
--cc=airlied@linux.ie \
--cc=alexander.deucher@amd.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=harry.wentland@amd.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=ppaalanen@gmail.com \
--cc=rodrigo.vivi@intel.com \
--cc=sunpeng.li@amd.com \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).