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

["Michel Dänzer" <michel.daenzer@mailbox.org>]

On 12/15/22 10:07, Michel Dänzer wrote:
> On 12/14/22 16:46, Alex Deucher wrote:
>> On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen@gmail.com> 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.
>>>
>>
>> In the amdgpu case, it's more of a preference thing.  The driver would
>> enable higher bpcs at the expense of refresh rate and it seemed most
>> users want higher refresh rates than higher bpc. 
> 
> I wrote the above because I thought that this patch might result in some modes getting pruned because they can't work with the max bpc. However, I see now that cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color") should prevent that AFAICT.
> 
> The question then is: What happens if user space tries to use a mode which doesn't work with the max bpc? Does the driver automatically lower the effective bpc as needed, or does the atomic commit (check) fail? The latter would seem bad.

Per my previous post in the other sub-thread, cbd14ae7ea93 ("drm/amd/display: Fix incorrectly pruned modes with deep color") seems to do the former. The commit log of this patch should probably be changed to reflect that.


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