[PATCH] drm/i915/dp_mst: Fix DSC input BPP computation

[Luca Coelho]

On Wed, 2024-06-12 at 18:22 +0300, Imre Deak wrote:
> On Wed, Jun 12, 2024 at 04:35:36PM +0300, Luca Coelho wrote:
> > Hi Imre,
> > 
> > On Tue, 2024-06-11 at 18:33 +0300, Imre Deak wrote:
> > > The branch or sink device decompressing a stream may have a limitation
> > > on the input/uncompressed BPP, which is lower than the base line BPP
> > > (determined by the sink's EDID). In some cases a stream with an input
> > > BPP higher than this limit will be converted automatically by the device
> > > decompressing the stream, by truncating the BPP, however in some cases
> > > - seen at least in Dell dock's DP->HDMI converters - the decompression
> > > will fail.
> > > 
> > > Fix the above by limiting the input BPP correctly. This is done already
> > > correctly for SST outputs.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index 00fdcbc28e9b7..15c20bedea091 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -349,6 +349,8 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder,
> > >  	if (max_bpp > sink_max_bpp)
> > >  		max_bpp = sink_max_bpp;
> > >  
> > > +	crtc_state->pipe_bpp = max_bpp;
> > > +
> > >  	max_compressed_bpp = intel_dp_dsc_sink_max_compressed_bpp(connector,
> > >  								  crtc_state,
> > >  								  max_bpp / 3);
> > 
> > Wouldn't it be better to make the assignment in
> > intel_dp_dsc_sink_max_compressed_bpp(), since that function is already
> > making modifications to crtc_state?
> 
> Agreed, this is less than ideal atm. The whole MST DSC state computation
> would need a refactor, at least to sanitize it wrt. the MST/non-DSC and
> the SST DSC counterparts (also planned by others). Hence, it made sense
> to keep this fix simple.

I'm totally for simple fixes!


> Also, the rational I saw for this way was that the input/uncompressed
> bpp (max_bpp above), which isn't relavant in the non-DSC case, is
> computed in intel_dp_dsc_mst_compute_link_config() and
> intel_dp_mst_find_vcpi_slots_for_bpp() would only compute the
> output/compressed bpp (relevant to both DSC and non-DSC case). Imo in
> the end the computation for these two cases should be separated into
> their own functions, instead of passing a 'bool dsc' param to a common
> function handling both cases.

Makes sense.  It's best to avoid this sort of boolean that changes the
function's semantics.


> > There is another caller, but I think it may benefit from the same check.
> 
> For the non-DSC case the above limit doesn't apply (at least I'm not
> aware of it).

Yeah, but AFAICT, in this case, it wouldn't really matter, right? The
remote's max_bpp would never be greater than the current one in this
case.


> > But this is just a nitpick.  Either way, you have:
> > 
> > Reviewed-by: Luca Coelho <luciano.coelho at intel.com>
> 
> Thanks!

--
Cheers,
Luca.