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 15:56 +0530, Nautiyal, Ankit K wrote:
> 
> On 3/4/2026 2:43 PM, Hogander, Jouni wrote:
> > 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.
> 
> 
> Sorry I meant `vdsc_instances_per_pipe == 2`.

Ok I will change it to this check. Alltought I don't know if it's any
better to configure only DSC0 if we reach here and
vdsc_instances_per_pipe > 2.

I will also add WARN_ON_ONCE if vdsc_instances_per_pipe > 2.

BR,
Jouni Högander

> 
> 
> > 
> > > 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?
> 
> For all the PPS registers we are using, intel_dsc_pps_write() and 
> intel_dsc_get_pps_reg() which take care of this.
> 
> But yes, I can see there is a miss in 2 places:RC_BUF_THRESH
> registers 
> and RC_RANGE_PARAMETER registers, for which 3rd DSC engine thing is 
> missing. (I'll check if there are any more instances and rectify this
> soon).
> 
> So I recommend filling this register only for 2 vdsc slices per pipe.
> 
> 
> Regards,
> 
> Ankit
> 
> 
> > 
> > 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);