[PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid()

[Ilia Mirkin]

On Fri, Sep 25, 2020 at 6:08 PM Lyude Paul <lyude at redhat.com> wrote:
>
> On Tue, 2020-09-22 at 17:22 -0400, Ilia Mirkin wrote:
> > On Tue, Sep 22, 2020 at 5:14 PM Lyude Paul <lyude at redhat.com> wrote:
> > > On Tue, 2020-09-22 at 17:10 -0400, Ilia Mirkin wrote:
> > > > Can we use 6bpc on arbitrary DP monitors, or is there a capability for
> > > > it? Maybe only use 6bpc if display_info.bpc == 6 and otherwise use 8?
> > >
> > > I don't think that display_info.bpc actually implies a minimum bpc, only a
> > > maximum bpc iirc (Ville would know the answer to this one). The other thing
> > > to
> > > note here is that we want to assume the lowest possible bpc here since we're
> > > only concerned if the mode passed to ->mode_valid can be set under -any-
> > > conditions (including those that require lowering the bpc beyond it's
> > > maximum
> > > value), so we definitely do want to always use 6bpc here even once we get
> > > support for optimizing the bpc based on the available display bandwidth.
> >
> > Yeah, display_info is the max bpc. But would an average monitor
> > support 6bpc? And if it does, does the current link training code even
> > try that when display_info.bpc != 6?
>
> So I did confirm that 6bpc support is mandatory for DP, so yes-6 bpc will always
> work.
>
> But also, your second comment doesn't really apply here. So: to be clear, we're
> not really concerned here about whether nouveau will actually use 6bpc or not.
> In truth I'm not actually sure either if we have any code that uses 6bpc (iirc
> we don't), since we don't current optimize bpc. I think it's very possible for
> us to use 6bpc for eDP displays if I recall though, but I'm not sure on that.
>
> But that's also not the point of this code. ->mode_valid() is only used in two
> situations in DRM modesetting: when probing connector modes, and when checking
> if a mode is valid or not during the atomic check for atomic modesetting. Its
> purpose is only to reject display modes that are physically impossible to set in
> hardware due to static hardware constraints. Put another way, we only check the
> given mode against constraints which will always remain constant regardless of
> the rest of the display state. An example of a static constraint would be the
> max pixel clock supported by the hardware, since on sensible hardware this never
> changes. A dynamic constraint would be something like how much bandwidth is
> currently unused on an MST topology, since that value is entirely dependent on
> the rest of the display state.
>
> So - with that said, bpc is technically a dynamic constraint because while a
> sink and source both likely have their own bpc limits, any bpc which is equal or
> below that limit can be used depending on what the driver decides - which will
> be based on the max_bpc property, and additionally for MST displays it will also
> depend on the available bandwidth on the topology. The only non-dynamic thing
> about bpc is that at a minimum, it will be 6 - so any mode that doesn't fit on
> the link with a bpc of 6 is guaranteed to be a mode that we'll never be able to
> set and therefore want to prune.
>
> So, even if we're not using 6 in the majority of situations, I'm fairly
> confident it's the right value here. It's also what i915 does as well (and they
> previously had to fix a bug that was the result of assuming a minimum of 8bpc
> instead of 6).

Here's the situation I'm trying to avoid:

1. Mode is considered "OK" from a bandwidth perspective @6bpc
2. Modesetting logic never tries 6bpc and the modeset fails

As long as the two bits of logic agree on what is settable, I'm happy.

Cheers,

  -ilia