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@xxxxxxxxxxx> 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@xxxxxxxxxx> -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
