Am 23.06.21 um 09:48 schrieb Pekka Paalanen:
> On Tue, 22 Jun 2021 11:57:53 +0200
> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote:
>
>> Am 22.06.21 um 09:25 schrieb Pekka Paalanen:
>>> On Fri, 18 Jun 2021 11:11:14 +0200
>>> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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.
> As long as you keep the old KMS property as is, just moving code so it
> is used by more drivers, this is fine and one can't do otherwise anyway.
>
> (The rest of this email is merely pondering the future, so not about
> this patch in particular.)
>
>
> But if we had a new, more general property for the range reported to
> monitors via infoframes, then it would be worth to re-visit the design.
> The HDR properties only set the infoframe and expect userspace to make
> sure that the pixels actually correspond to what the infoframes tell
> the monitor. One can't do HDR tone mapping automatically in the kernel,
> so in that sense the HDR property behaviour is obvious. But which
> behaviour would fit range property or others better, I'm not sure.
>
> Generally there seems to be two approaches to designing KMS properties:
>
> - Let userspace describe what data it has and what data should be sent
> to a monitor, and let the kernel driver magically come up with a
> conversion.
>
> - Only userspace understands how the pixel data is encoded, and
> programs the transformations (DEGAMMA/CTM/GAMMA etc.) such, that the
> result is what a monitor expects based on e.g. infoframes.
Why not both?
This patchset is thought to control what's happening "on the cable", so if the input data is in a different format, the
kernel has to convert it.
Maybe in the future there could be an additional set of "input-" properties. aka "input bpc", "input color format", and
"input color range". With an additional constraint that if "input-" property == "active-" property the kernel is not
allowed to do any conversion regarding this aspect, giving userspace full control if wanted.
>
> Doing the former requires policy in the kernel. If there is a
> specification that uniquely defines what the conversion is, this is
> good. But if not or if there are artistic decisions to be made, like
> with HDR tone mapping, then it doesn't work.
>
> OTOH, the former approach allows the driver to use any and all hardware
> features it has to realize the conversion, perhaps taking advantage of
> even fixed-function hardware blocks. The latter approach is much harder
> to map to hardware features.
>
> This dilemma has been discussed in length in
> https://lists.freedesktop.org/archives/dri-devel/2021-June/311689.html
>
>
> Thanks,
> pq
