Re: [PATCH v4 03/17] drm/uAPI: Add "active bpc" as feedback channel for "max bpc" drm property

[Werner Sembach]

Am 01.07.21 um 09:42 schrieb Pekka Paalanen:
> On Wed, 30 Jun 2021 11:42:10 +0200
> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote:
>
>> Am 30.06.21 um 10:21 schrieb Pekka Paalanen:
>>> On Tue, 29 Jun 2021 13:02:05 +0200
>>> Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx> wrote:
>>>  
>>>> Am 28.06.21 um 19:03 schrieb Werner Sembach:  
>>>>> Am 18.06.21 um 11:11 schrieb Werner Sembach:  
>>>>>> Add a new general drm property "active bpc" which can be used by graphic
>>>>>> drivers to report the applied bit depth per pixel back to userspace.
>>>>>>
>>>>>> While "max bpc" can be used to change the color depth, there was no way to
>>>>>> check which one actually got used. While in theory the driver chooses the
>>>>>> best/highest color depth within the max bpc setting a user might not be
>>>>>> fully aware what his hardware is or isn't capable off. This is meant as a
>>>>>> quick way to double check the setup.
>>>>>>
>>>>>> In the future, automatic color calibration for screens might also depend on
>>>>>> this information being available.
>>>>>>
>>>>>> Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>   drivers/gpu/drm/drm_connector.c | 51 +++++++++++++++++++++++++++++++++
>>>>>>   include/drm/drm_connector.h     |  8 ++++++
>>>>>>   2 files changed, 59 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>>>>> index da39e7ff6965..943f6b61053b 100644
>>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>>> @@ -1197,6 +1197,14 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>>>>>>    *	drm_connector_attach_max_bpc_property() to create and attach the
>>>>>>    *	property to the connector during initialization.
>>>>>>    *
>>>>>> + * active bpc:
>>>>>> + *	This read-only range property tells userspace the pixel color bit depth
>>>>>> + *	actually used by the hardware display engine on "the cable" on a
>>>>>> + *	connector. The chosen value depends on hardware capabilities, both
>>>>>> + *	display engine and connected monitor, and the "max bpc" property.
>>>>>> + *	Drivers shall use drm_connector_attach_active_bpc_property() to install
>>>>>> + *	this property.
>>>>>> + *  
>>>>> Regarding "on the cable" and dithering: As far as I can tell, what the dithering option does, is setting a hardware
>>>>> register here:
>>>>>
>>>>> - https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/intel_display.c#L4534
>>>>>
>>>>> - https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/intel_display.c#L4571
>>>>>
>>>>> So dithering seems to be calculated by fixed purpose hardware/firmware outside of the driver?
>>>>>
>>>>> The Intel driver does not seem to set a target bpc/bpp for this hardware so I guess it defaults to 6 or 8 bpc?  
>>>> Never mind it does. This switch-case does affect the dithering output:
>>>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/i915/display/intel_display.c#L4537  
>>> Hi,
>>>
>>> I obviously do not know the intel driver or hardware at all, but
>>> to me that just looks like translating from bits per pixel to bits per
>>> channel in RGB mapping?  
>> No, if i understand the documentation correctly: Writing bit depth here 
>> with dithering enabled sets the dithering target bpc.
>>>  
>>>> As found in this documentation p.548:
>>>> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-lkf-vol02c-commandreference-registers-part2.pdf
>>>>
>>>> So max bpc and active bpc are affecting/affected by the bpc after dithering.  
>>> By definition, if the cable carries N bpc, then dithering does not
>>> change that. The cable still carries N bpc, but due to spatial or
>>> temporal dithering, the *observed* color resolution may or may not be
>>> higher than the cable bpc.  
>> Yes, and max bpc and active bpc tell the cable bpc ist not the 
>> *observed* bpc.
>>> Of course, if the cable bpc is 8, and dithering targets 6 bpc, then 2
>>> LSB on the cable are always zero, right?  
>> I would assume that in this case only 6 bpc are actually send? Isn't the 
>> whole thing of dithering that you can't send, for example, 8 bpc?
>>> Maybe one would want to do that if the monitor has a 6 bit panel and it
>>> simply ignored the 2 LSB, and the cable cannot go down to 6 bpc.  
>> Is there dithering actually doing this? aka is my assumption above wrong?
>>
>> AMD code that confused me before, is hinting that you might be right: 
>> https://elixir.bootlin.com/linux/v5.13/source/drivers/gpu/drm/amd/display/dc/dce/dce_transform.c#L826
>>
>> there is a set_clamp depth and a separate DCP_SPATIAL_DITHER_DEPTH_30BPP
>>
>>> So, what does "max bpc" mean right now?
>>>
>>> It seems like dither on/off is insufficient information, one would also
>>> need to control the dithering target bpc. I suppose the driver has a
>>> policy on how it chooses the target bpc, but what is that policy? Is
>>> the dither target bpc the cable bpc or the sink bpc?
>>>
>>> Needless to say, I'm quite confused.  
>> ... We need someone who knows what dithering on intel and amd gpu 
>> actually means.
>>
>> But I don't want this to become a blocker for this patchset, because if 
>> there is no dithering, which seems to be the norm, the active bpc 
>> property is already really usefull as it is. So add a note to the docs 
>> that the value might be invalid when dithering is active for now?
> Hi,
>
> not necessarily invalid. It all depends on how "max bpc" and "active
> bpc" are defined.
>
> If they are defined and implemented as "on the cable", then they are
> both well-defined and always valid, regardless of what dithering or bit
> clamping does, so this is the semantics I'd would prefer. It's clear,
> but of course does not tell full story.
>
> When better properties for dithering are added, those can then define
> how it works on top of cable bpc, with no impact on "max bpc" or
> "active bpc" properties.
>
> So if we cannot tell what "max bpc" is, then "active bpc" should just
> be defined as the same thing as "max bpc" affects, and leave the
> precise definition of both for later.
But as long as I don't know exactly how dithering is affected by max bpc I can't tell for sure if active bpc is acting
the same. That's why I wrote it is "undefined" to not run into a trap where the actual behavior have to change after the
fact.
>
> If the definition was observed bpc, then we would have problems and
> would need to know everything right now. But you can't really make
> promises of observed bpc anyway, because you don't know what the
> monitor does to the video signal I suppose. Unless you define it "as if
> observed through an ideal theoretical monitor" which then gets awkward
> to explain.
Yes, that's why I think describing the "raw" signal is the best and leave it to the user to know what his or her Monitor
is making out of it.
>
>
> Thanks,
> pq