[Intel-gfx] [PATCH 0/2] Add DP MST DSC support to i915

[Lyude Paul]

On Thu, 2022-08-11 at 10:33 +0300, Lisovskiy, Stanislav wrote:
> On Wed, Aug 10, 2022 at 04:02:08PM -0400, Lyude Paul wrote:
> > Btw, what's the plan for this? Figured I'd ask since I noticed this on the ML,
> > nd I'm now finishing up getting the atomic only MST patches I've been working
> > on merged :)
> 
> Current plan is that I need to fix this, as current implementation doesn't
> seem to work because of my wrong assumption that drm_dp_mst_find_vcpi_slots
> will fail if no slots are available and then we can fallback to DSC.
> 
> In reality that function can return whatever bogus value it wants, like
> 71 slots, while you have only 63 available. The real check is done in
> drm_dp_mst_atomic_check, which would of course reject that configuration,
> however by that moment its going to be too late for swithcing to DSC.
> 

Yeah - the drm_dp_mst_find_vcpi_slots() bit is intentional, as we need to be
able to ensure that calls to it are idempotent since it may be called multiple
times. We also don't actually know whether or not we've gone past the max
number of slots until atomic check since there's not necessarily a guarantee
towards which order we process disabling CRTCs and process enabling them,
hence why we wait towards the end to check this.

The eventual plan that I'd like to see happen is to basically teach
drm_dp_mst_atomic_check() to indicate when a display state needs to be
recalculated in order to fit into the current bandwidth limitations. There's a
number of ways we could accomplish this, one I've been thinking about in
particular is maybe having drivers provide a minimum and maximum bandwidth
value, so that we could also have the helpers indicate to the driver which
sinks can be recalculated.

Also FWIW: the reason I've been working on:

https://patchwork.freedesktop.org/series/107073/

Is to make all of this easier by making sure all of the payload info is in the
MST atomic state.

> So looke like I will have to move that check at least partly to where DSC/no DSC decision is done. However if there are multiple displays we get
> another problem, lets say we have 2 displays requiring 40 vcpi slots each in DSC
> mode with certain input bpp.
> We have now either option to reject the whole config or go back and try with
> another bpp to check if we can reduce amount of slots.
> Because by default we choose the first one which fits, however by the time when 
> compute_config is called, we still don't have all config computed, which might
> lead to that last crtc can either run our of vcpi slots or we will have to 
> go back and try recalculating with higher compression ratio.

Having to go back and recalculate with a higher compression ratio is to be
expected to some extent I'm fairly sure, as I don't really think there's any
other sensible way we could figure this out besides recalculating the state
multiple times. Keep in mind too - doing stuff like calling a CRTC's atomic
check multiple times is totally normal and expected to work in DRM anyhow.

> 
> My other question was that DSC was supposed to be "visually" lossless, wondering
> why we are still trying with different bpps? Could have just set highest
> compression ratio right away.

I think this is a misunderstanding, at least while talking to AMD the
impression I've gotten is that DSC isn't actually totally loseless - which is
why their driver currently tries to use the least amount of compression
possible. Otherwise I would say we should just enable it at max all of the
time. Also, annoyingly most of the logic for doing this is currently stuck in
amdgpu's driver. but please feel free to move it out into helpers! If we want
good DSC support that's definitely going to help quite a bunch since they've
worked out a lot of this already, and there's probably even improvements we
can make upon their logic.

> 
> So need to sort this out first before floating new series.
> 
> Stan
> 
> > 
> > On Wed, 2022-08-10 at 11:17 +0300, Stanislav Lisovskiy wrote:
> > > Currently we have only DSC support for DP SST.
> > > 
> > > Stanislav Lisovskiy (2):
> > >   drm: Add missing DP DSC extended capability definitions.
> > >   drm/i915: Add DSC support to MST path
> > > 
> > >  drivers/gpu/drm/i915/display/intel_dp.c     |  76 +++++-----
> > >  drivers/gpu/drm/i915/display/intel_dp.h     |  17 +++
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 145 ++++++++++++++++++++
> > >  include/drm/display/drm_dp.h                |  10 +-
> > >  4 files changed, 203 insertions(+), 45 deletions(-)
> > > 
> > 
> > -- 
> > Cheers,
> >  Lyude Paul (she/her)
> >  Software Engineer at Red Hat
> > 
> 

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