linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Werner Sembach <wse@tuxedocomputers.com>
To: Pekka Paalanen <ppaalanen@gmail.com>
Cc: sunpeng.li@amd.com, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
	airlied@linux.ie, dri-devel@lists.freedesktop.org,
	tzimmermann@suse.de, rodrigo.vivi@intel.com,
	alexander.deucher@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: Fri, 25 Jun 2021 10:48:27 +0200	[thread overview]
Message-ID: <62df9501-1ac5-8e1f-b0e2-c9cbfd534395@tuxedocomputers.com> (raw)
In-Reply-To: <20210623142659.16672192@eldfell>


Am 23.06.21 um 13:26 schrieb Pekka Paalanen:
> On Wed, 23 Jun 2021 12:10:14 +0200
> Werner Sembach <wse@tuxedocomputers.com> wrote:
>
>> Am 23.06.21 um 09:48 schrieb Pekka Paalanen:
>>> On Tue, 22 Jun 2021 11:57:53 +0200
>>> Werner Sembach <wse@tuxedocomputers.com> wrote:
>>>  
>>>> 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.  
>>> 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?
> Because "both" means you have overlapping sets of properties that might
> contradict each other. Or then you need a switch between the two models.
>
>> 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.
> Right, if that is the desired semantics.
>
> That's not how the HDR property works. Kernel does not convert there.
> The HDR property only sets infoframes that the monitor interprets.
>
>> 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.
> If by "input" you mean "the result from userspace provided content
> going through the configured KMS pixel pipeline", then yes. But it's
> hard to put it into words accurately.
>
> The FB could contain whatever which userspace then programs DEGAMMA and
> CTM to produce what would be the "input" pixels for example.
>
> This is getting closer to the "abstract KMS pipeline" idea which has
> been predicted to fall apart in the email thread linked below.
Ok, I'm not deep enough in the topic. For this patchset anyways I try to stick to the preexisting behavior of "max bpc"
and "Broadcast RGB", which I believe is the "on the cable/kernel does the conversion" discussed. I will double check on
that before I submit the next revision.
>
>>> 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

  reply	other threads:[~2021-06-25  8:48 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
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 [this message]
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=62df9501-1ac5-8e1f-b0e2-c9cbfd534395@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=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linux-kernel@vger.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).