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 <[email protected]>
> > ---
> > 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;
> > }
> >
