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

[Alex Deucher]

On Thu, Dec 15, 2022 at 4:17 AM Pekka Paalanen <ppaalanen at gmail.com> wrote:
>
> On Wed, 14 Dec 2022 10:46:55 -0500
> Alex Deucher <alexdeucher at gmail.com> wrote:
>
> > On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen <ppaalanen at gmail.com> 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.
> > >
> >
> > 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.
>
> Hi Alex,
>
> we already have userspace in explicit control of the refresh rate.
> Userspace picks the refresh rate first, then the driver silently checks
> what bpc is possible. Userspace preference wins, so bpc is chosen to
> produce the desired refresh rate.
>
> In what cases does the driver really pick a refresh rate on its own?

It didn't, but IIRC, at the time the driver filtered out modes that it
could not support with the max bpc.  It looks like that has been fixed
now, so maybe this whole discussion is moot.

Alex

>
> Or is this about display latency more than throughput, to send a full
> frame in shorter time while not actually increasing refresh rate?
>
> Even for VRR, userspace already explicitly chooses the max refresh
> rate, I believe.
>
> I suppose optimising power consumption by choosing a lower bpc than
> what would work is a use case, but that's a bit awkward currently
> because there is no way for userspace to opt-out of that. OTOH,
> userspace can already opt-in for that by lowering max_bpc.
>
> >  I guess the driver
> > can select a lower bpc at its discretion to produce what it thinks is
> > the best default, but what about users that don't want the default?
>
> That's what we need explicit bpc control KMS properties for, e.g.
> min_bpc to complement max_bpc (I recall old discussions about this).
> Only userspace could know what a particular end user wants.
>
> To ask this differently: in what cases would max_bpc become the wanted
> bpc in a way that it overrides the explicitly set video mode (refresh
> rate), or would cause a modeset to fail if a lower bpc would make the
> modeset succeed?
>
> The very definition of max_bpc is that it's the upper limit, and not the
> desired bpc. The UAPI documentation says:
>
> max bpc:
>
>     This range property is used by userspace to limit the bit depth.
>     When used the driver would limit the bpc in accordance with the
>     valid range supported by the hardware and sink. Drivers to use the
>     function drm_connector_attach_max_bpc_property() to create and
>     attach the property to the connector during initialization.
>
> Git archaeology revealed that this property was first intended to work
> around broken sinks and not for user preferences.
>
>
> Thanks,
> pq
>
> >
> > Alex
> >
> >
> > > 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.
> > >
> > > 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
>