Re: [PATCH 38/50] drm/i915/dsi: Move initialization of DSI DSC streams-per-pipe to fill_dsc()

[Imre Deak <imre.deak@intel.com>]

On Tue, Dec 09, 2025 at 10:47:35AM +0200, Jouni Hogander wrote:
> On Thu, 2025-11-27 at 19:50 +0200, Imre Deak wrote:
> > Move the initialization of the DSI DSC streams-per-pipe value to
> > fill_dsc() next to where the corresponding (per-line) slice_count
> > value
> > is initialized. This allows converting the initialization to use the
> > detailed slice configuration state in follow-up changes.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/icl_dsi.c    | 6 ------
> >  drivers/gpu/drm/i915/display/intel_bios.c | 5 +++++
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> > b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index 90076839e7152..9aba3d813daae 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > @@ -1624,12 +1624,6 @@ static int gen11_dsi_dsc_compute_config(struct
> > intel_encoder *encoder,
> >  	if (crtc_state->pipe_bpp < 8 * 3)
> >  		return -EINVAL;
> >  
> > -	/* FIXME: split only when necessary */
> > -	if (crtc_state->dsc.slice_count > 1)
> > -		crtc_state->dsc.slice_config.streams_per_pipe = 2;
> > -	else
> > -		crtc_state->dsc.slice_config.streams_per_pipe = 1;
> > -
> >  	/* FIXME: initialize from VBT */
> >  	vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> > b/drivers/gpu/drm/i915/display/intel_bios.c
> > index a639c5eb32459..e69fac4f5bdfe 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -3516,10 +3516,14 @@ static void fill_dsc(struct intel_crtc_state *crtc_state,
> >  	 * throughput etc. into account.
> >  	 *
> >  	 * Also, per spec DSI supports 1, 2, 3 or 4 horizontal slices.
> > +	 *
> > +	 * FIXME: split only when necessary
> >  	 */
> >  	if (dsc->slices_per_line & BIT(2)) {
> > +		crtc_state->dsc.slice_config.streams_per_pipe = 2;
> >  		crtc_state->dsc.slice_count = 4;
> >  	} else if (dsc->slices_per_line & BIT(1)) {
> > +		crtc_state->dsc.slice_config.streams_per_pipe = 2;
> 
> fill_dsc is called by intel_bios_get_dsc_params. Is streams_per_pipe
> really bios parameter? I see slices_per_line is in VBT.
> Streams_per_pipe and existing slice_count are decided based on that.

The slices_per_line computed in fill_dsc() at the moment
(crtc_state->dsc.slice_count) is not exactly what is in VBT. VBT
indicates what slices_per_line counts the sink supports, not what the
selected slices_per_line count should be (which would be a single
integer parameter in VBT, not a mask).

> Is that right place to make that decisions or should we leave that
> decision to caller of intel_bios_get_dsc_params?

I think the computation of the slices_per_line value (for which the
sink's slice_per_line capability mask is only one criteria) should be at
the same spot where the closely related pipes_per_line, streams_per_pipe
and slices_per_stream are computed as well. In fact at the end of the
patchset only these latter 3 params are computed and the slices_per_line
value is derived from these using a helper function.

I agree with you that fill_dsc() should not do the actual state
computation (like it does atm selecting slices_per_line aka
dsc.slice_count), rather this should be done by the DSI encoder state
computation in gen11_dsi_dsc_compute_config(), fill_dsc() only returning
a mask of the slices_per_line counts supported by the sink. Would you be
ok to do this as a follow-up?

> BR,
> 
> Jouni Högander
> 
> >  		crtc_state->dsc.slice_count = 2;
> >  	} else {
> >  		/* FIXME */
> > @@ -3527,6 +3531,7 @@ static void fill_dsc(struct intel_crtc_state *crtc_state,
> >  			drm_dbg_kms(display->drm,
> >  				    "VBT: Unsupported DSC slice count for DSI\n");
> >  
> > +		crtc_state->dsc.slice_config.streams_per_pipe = 1;
> >  		crtc_state->dsc.slice_count = 1;
> >  	}
> >