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

[Sebastian Wick]

On Fri, Dec 23, 2022 at 8:10 PM Harry Wentland <harry.wentland at amd.com> 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.

Very good!

>
> > 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).

To be clear: we need this. I've argued before that we need a min bpc
setting because we'd rather not enable HDR if we can't get the bit
depth required for it and there is no other mechanism to control this.
The other remaining kernel problem for HDR is that we still have no
control over YCC/RGB selection on the cable and thus can't choose the
correct color space infoframe.

>
> 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
>