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

[Michel Dänzer]

On 12/23/22 20:10, Harry Wentland wrote:
> On 12/14/22 04:01, Pekka Paalanen wrote:
>> On Tue, 13 Dec 2022 18:20:59 +0100
>> Michel Dänzer <michel.daenzer at 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.

Thanks for double-checking! This patch is

Reviewed-by: Michel Dänzer <mdaenzer at redhat.com>


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer