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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxx> > > Thanks! -- Cheers, Luca.
