Re: [PATCH v2 3/4] drm/i915/dsc: Add helper for writing DSC Selective Update ET parameters

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2026-03-04 at 14:06 +0530, Nautiyal, Ankit K wrote:
> 
> On 3/3/2026 6:24 PM, Jouni Högander wrote:
> > There are slice row per frame and pic height configuration in DSC
> > Selective
> > Update Parameter Set 1 register. Add helper for configuring these.
> > 
> > Bspec: 71709
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_vdsc.c | 22
> > ++++++++++++++++++++++
> >   drivers/gpu/drm/i915/display/intel_vdsc.h |  3 +++
> >   2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > index 7e53201b3cb1..ae3af3c2e41a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > @@ -820,6 +820,28 @@ void intel_dsc_dp_pps_write(struct
> > intel_encoder *encoder,
> >   				  sizeof(dp_dsc_pps_sdp));
> >   }
> >   
> > +void intel_dsc_su_et_parameters_configure(struct intel_dsb *dsb,
> > struct intel_encoder *encoder,
> > +					  const struct
> > intel_crtc_state *crtc_state, int su_lines)
> > +{
> > +	struct intel_display *display =
> > to_intel_display(crtc_state);
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state-
> > >uapi.crtc);
> > +	const struct drm_dsc_config *vdsc_cfg = &crtc_state-
> > >dsc.config;
> > +	enum pipe pipe = crtc->pipe;
> > +	int vdsc_instances_per_pipe =
> > intel_dsc_get_vdsc_per_pipe(crtc_state);
> > +	int slice_row_per_frame = su_lines / vdsc_cfg-
> > >slice_height;
> > +	u32 val;
> > +
> > +	drm_WARN_ON_ONCE(display->drm, su_lines % vdsc_cfg-
> > >slice_height);
> > +
> > +	val =
> > DSC_SUPS0_SU_SLICE_ROW_PER_FRAME(slice_row_per_frame);
> > +	val |= DSC_SUPS0_SU_PIC_HEIGHT(su_lines);
> > +
> > +	intel_de_write_dsb(display, dsb,
> > LNL_DSC0_SU_PARAMETER_SET_0(pipe), val);
> > +
> > +	if (vdsc_instances_per_pipe > 1)
> > +		intel_de_write_dsb(display, dsb,
> > LNL_DSC1_SU_PARAMETER_SET_0(pipe), val);
> 
> Currently 3 DSC engines per pipe is only supported on BMG, which
> doesn't 
> have eDP, so vdsc_instances_per_pipe would never be 3 for now.

Yes, but it can be two. Thus > 1.
> 
> Furthermore we do not support these registers for BMG AFAICS.
> 
> However later some platform may have 3 VDSC engines and who knows may
> need the Selective Update ET configuration for DSC.
> 
> Since we do not have those registers defined, lets make this
> condition 
> specifically check for `vdsc_instances_per_pipe == 1`

We can't do that because instances can be > 1. Actually when running
kms_psr2_sf on setup where panel is supporting selective update and dsc
this is the case.

> 
> We can have perhaps have WARN_ON if vdsc_instances_per_pipe > 2, at
> the 
> start, as we do not expect the SU ET configuration for 3rd VDSC
> engine yet.

I see everywhere else in intel_vdsc.c same convention is used. I don't
understand why this helper for PSR code should be made different and be
responsible for identifying possible DSC configuration issue?

BR,
Jouni Högander

> 
> 
> Regards,
> 
> Ankit
> 
> 
> > +}
> > +
> >   static i915_reg_t dss_ctl1_reg(struct intel_crtc *crtc, enum
> > transcoder cpu_transcoder)
> >   {
> >   	return is_pipe_dsc(crtc, cpu_transcoder) ?
> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.h
> > b/drivers/gpu/drm/i915/display/intel_vdsc.h
> > index f4d5b37293cf..3372f8694054 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.h
> > @@ -14,6 +14,7 @@ enum transcoder;
> >   struct intel_crtc;
> >   struct intel_crtc_state;
> >   struct intel_display;
> > +struct intel_dsb;
> >   struct intel_dsc_slice_config;
> >   struct intel_encoder;
> >   
> > @@ -37,6 +38,8 @@ void intel_dsc_dsi_pps_write(struct intel_encoder
> > *encoder,
> >   			     const struct intel_crtc_state
> > *crtc_state);
> >   void intel_dsc_dp_pps_write(struct intel_encoder *encoder,
> >   			    const struct intel_crtc_state
> > *crtc_state);
> > +void intel_dsc_su_et_parameters_configure(struct intel_dsb *dsb,
> > struct intel_encoder *encoder,
> > +					  const struct
> > intel_crtc_state *crtc_state, int su_lines);
> >   void intel_vdsc_state_dump(struct drm_printer *p, int indent,
> >   			   const struct intel_crtc_state
> > *crtc_state);
> >   int intel_vdsc_min_cdclk(const struct intel_crtc_state
> > *crtc_state);