Re: [PATCH 16/16] drm/amd/display: Don't restrict bpc to 8 bpc

[Harry Wentland <harry.wentland@amd.com>]

On 12/14/22 04:01, Pekka Paalanen wrote:
> On Tue, 13 Dec 2022 18:20:59 +0100
> Michel Dänzer <michel.daenzer@mailbox.org> wrote:
> 
>> On 12/12/22 19:21, Harry Wentland wrote:
>>> This will let us pass kms_hdr.bpc_switch.
>>>
>>> I don't see any good reasons why we still need to
>>> limit bpc to 8 bpc and doing so is problematic when
>>> we enable HDR.
>>>
>>> If I remember correctly there might have been some
>>> displays out there where the advertised link bandwidth
>>> was not large enough to drive the default timing at
>>> max bpc. This would leave to an atomic commit/check
>>> failure which should really be handled in compositors
>>> with some sort of fallback mechanism.
>>>
>>> If this somehow turns out to still be an issue I
>>> suggest we add a module parameter to allow users to
>>> limit the max_bpc to a desired value.  
>>
>> While leaving the fallback for user space to handle makes some sense
>> in theory, in practice most KMS display servers likely won't handle
>> it.
>>
>> Another issue is that if mode validation is based on the maximum bpc
>> value, it may reject modes which would work with lower bpc.
>>
>>
>> What Ville (CC'd) suggested before instead (and what i915 seems to be
>> doing already) is that the driver should do mode validation based on
>> the *minimum* bpc, and automatically make the effective bpc lower
>> than the maximum as needed to make the rest of the atomic state work.
> 
> A driver is always allowed to choose a bpc lower than max_bpc, so it
> very well should do so when necessary due to *known* hardware etc.
> limitations.
> 

I spent a bunch of time to figure out how this actually pans out in
amdgpu and it looks like we're doing the right thing, i.e. if bandwidth
limitations require it we'll downgrade bpc appropriately. These changes
happened over the last couple years or so. So while raising the default
max_bpc wasn't safe in amdgpu years ago it is completely fine now.

As for the relevant code it's mostly handled in create_validate_stream_for_sink
in amdgpu_dm.c where we iterate over a stream's mode validation with
decreasing bpc if it fails (down to a bpc of 6).

For HDMI we also have a separate adjust_colour_depth_from_display_info
function that downgrades bpc in order to fit within the max_tmds_clock.

So, in short, this change should not lead to displays not lighting up
because we no longer force a given bpc.

> So things like mode validation cannot just look at a single max or min
> bpc, but it needs to figure out if there is any usable bpc value that
> makes the mode work.
> 
> The max_bpc knob exists only for the cases where the sink undetectably
> malfunctions unless the bpc is artificially limited more than seems
> necessary. That malfunction requires a human to detect, and reconfigure
> their system as we don't have a quirk database for this I think.
> 
> The question of userspace wanting a specific bpc is a different matter
> and an unsolved one. It also ties to userspace wanting to use the
> current mode to avoid a mode switch between e.g. hand-off from firmware
> boot splash to proper userspace. That's also unsolved AFAIK.
> 

Agreed, the current "max bpc" just sets a max. We'd probably want a
"min bpc" if userspace needs a minimum (e.g., for HDR).

Harry

> OTOH, we have the discussion that concluded as
> https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898
> which really puts userspace in charge of max_bpc, so the driver-chosen
> default value does not have much impact as long as it makes the
> firmware-chosen video mode to continue, as requested in
> https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995
> given that userspace cannot know what the actual bpc currently is nor
> set the exact bpc to keep it the same.
> 
> 
> Thanks,
> pq