drm/probe_helper, i915: Validate MST modes against PBN limits

[Lyude Paul]

I'm going to cc this to dri-devel, because this is something that would be of
interest to upstream (you also should be doing the same, since this
development should be happening upstream).

Anyway, some more comments below:

On Mon, 2020-11-09 at 11:49 -0500, Lyude Paul wrote:
> On Mon, 2020-11-09 at 15:43 +0800, Koba Ko wrote:
> Link: https://patchwork.freedesktop.org/series/77670/
> ---
> Hi Lyude,
> I'm encountering some issues for the lack of bandwidth on the mst hub.
> The valid mode is shown in the edid list but after choosing it and the error
> is prompted(Due to NOSPC).
> I found your patches should be helpful for this kind of issue.
> But I mentioned,
> 1. Why does the patch choose "min_bpp=18" but not to choose the crtc_state-
> >pipe_bpp
> 
> because I haven't gotten around to adding the proper logic to the MST
> helpers to actually optimize the bpp depending on how much bandwidth is
> available, along with other constraints such as max_bpc. This is next up on
> my todo list (we also need this for nouveau), but I've been a bit busy over
> the last two weeks trying to complete some high priority stuff at work. In
> general, there's a good number of things that still need to be done for MST:
> we also don't support DSC for MST and need to add support for that into the
> helpers, and as well we also need to write up support for fallback link
> retraining (when we train an MST link at a certain link rate, but that
> connection proves to be unreliable so we have to lower the link rate and
> recalculate the settings for the display accordingly).
> 
> 2. For the vcpi number, the patch doesn't check it
> in intel_dp_mst_mode_valid_ctx().
> Even the mode needs the vcpi greater than 63 and it would be listed in
> available EDID.
> 
> Would the PBN check not handle this? Maybe I overlooked VCPI checks when I
> wrote this, but generally if there's no possible way a mode could fit in 63
> VCPI, it also shouldn't be able to pass the PBN check. The VCPI checks in
> the atomic check are moreso to prevent there from being too much bandwidth
> being used on a single hub with multiple displays (which would be dependent
> on the current state of other displays), but I'd be interested to know if
> you found some situations where we're not filtering out always impossible
> modes from intel_dp_mst_mode_valid_ctx().
> 
> Also, I think I'll have some time a little later this week so I start
> writing up support for dynamic bpc selection if this is something you guys
> need right now. (hopefully, at least :)

actually-I just realized it might take me a little longer to get to it
depending on what happens, as I've currently got some other stuff that's
higher priority (getting DPCD backlights working on nouveau mainly, which I
unfortunately have a deadline for).

If you have the time to write the patches sooner I could probably help with
reviewing them and other things - although it's a good bit more complicated
then it might look, and will require having to pull a lot of logic out of
amdgpu. amdgpu currently supports these things, but the agreement they had
with upstream is that we'd eventually be moving out code for dynamically
chosing bpc/dsc status for MST into the MST helpers so that both i915 and
nouveau can benefit.

The basic idea would be making it so that a driver first sets up it's initial
VCPI allocations during the atomic check, using it's "ideal" settings by
calling drm_dp_mst_atomic_find_vcpi_slots() and
drm_dp_mst_atomic_release_vcpi_slots(). "Settings" here are vaguely defined as
anything on the connector that would affect the bandwidth consumption of the
mode we want to set, which in this case would just be the bpc and whether or
not dsc is enabled. It probably would also make sense to start piping these to
find_vcpi_slots()/release_vcpi_slots() for reasons I'll clarify in a moment.

>From there, the driver would figure out what it wants for bpc/dsc as per
usual, then run through the rest of the atomic check until the driver calls
drm_dp_mst_atomic_check(). At this point if we're using too much bandwidth,
drm_dp_mst_atomic_check() would let us know by returning -ENOSPC in order to
indicate that we need to retry with more bandwidth conservative settings if
possible. So, we'd redo the atomic check over again with a more conservative
bpc while maybe also enabling dsc, see if that fits, then try raising the
bandwidth usage of each connector gradually until we fail the
drm_dp_mst_atomic_check() and come up with the most preferable settings we
found. Then we'd trigger modesets on connectors that need to have modesets
performed, etc. etc. Keep in mind that this might not be the exact algorithm
we end up using, as other approaches such as starting with a display
configuration using the smallest bandwidth possible then moving up from there
in the atomic check could make more sense.

Additionally, remember that dsc would definitely have to be taken into
consideration here, since we'd want amdgpu to be using these helpers and we
also don't want to suddenly drop DSC support for MST on their side (even if we
don't have the code for DSC ready yet in drivers like nouveau). Just in case
you're confused on why I keep mentioning it when you originally asked about
bpp :).

My hope would be that additionally by providing bpc and dsc settings through
drm_dp_mst_atomic_find_vcpi_slots() we might be able to also teach the MST
helpers to potentially suggest the most optimal display configuration given
the bandwidth constraints, so that each driver using the helpers doesn't need
to reimplement this. I'm not sure if this is a good idea yet though, and it'll
likely be a lot more obvious once I've started writing the code and have more
then just one driver using it.

As well, there's some resources already for what we might want some of this
behavior to look like. 8c20a1ed9b4ff25c67f6c1771a4e1195b5221cce, where amdgpu
adds DSC fair share computation, does a pretty good job of describing the
algorithm that amdgpu follows. We'll likely want to implement something
similar to that, or just reuse the algorithm as-is in the DP MST helpers.

Anyway-like I said it's a lot of work, if you're interested in getting a head
start I'd be fine with that. But I'll definitely be getting to it in the near
future, as it's something my employer needs me to implement :).

Also if you do that, please put your WIP code on github or somewhere like that
so I can keep up with it. I want to get working on this once I have the time
asap, but I don't want to accidentally end up redoing your work. As well, we'd
definitely want to make sure we're running our ideas by folks on intel-gfx as
well to make sure we do this the right way.

> 
> 
> Thanks
> 
> -- 
> Cheers,
> 	Lyude Paul (she/her)
> 	Software Engineer at Red Hat

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat