Re: [PATCH 4/5] drm/i915/psr: DSC configuration for Early Transport

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

On Wed, 2026-02-25 at 17:36 +0530, Nautiyal, Ankit K wrote:
> 
> On 2/19/2026 6:37 PM, Jouni Högander wrote:
> > There is Selective Update slice row per frame and picture height
> > configurations needed on DSC when using Selective Update Early
> > Transport. Calculate and configure these when using Early
> > Transport.
> > 
> > Bspec: 68927
> > Fixes: 467e4e061c44 ("drm/i915/psr: Enable psr2 early transport as
> > possible")
> > Cc: <stable@vger.kernel.org> # v6.9+
> 
> 
> This patch needs the other patch where registers are defined. I am
> not 
> sure if stable will only pick this patch or will try to find out the 
> dependency patch.
> 
> We need to check if there is a way to tell the dependency
> patch/commit 
> to stable, so that both patches are applied together.
> 
> If we want this change to get ported to older kernels, we might need
> to 
> squash the register definition patch with this patch.
> 
> 
> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > ---
> >   .../drm/i915/display/intel_display_types.h    |  1 +
> >   drivers/gpu/drm/i915/display/intel_psr.c      | 24
> > +++++++++++++++++++
> >   2 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index e8e4af03a6a6..8903804c04b1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1381,6 +1381,7 @@ struct intel_crtc_state {
> >   	u32 psr2_man_track_ctl;
> >   
> >   	u32 pipe_srcsz_early_tpt;
> > +	u32 dsc_su_parameter_set_0_calc;
> 
> I think let's just have a bool parameter something like 
> psr_su_update_dsc_pps.
> 
> We can set this bool variable during intel_psr2_sel_fetch_update()

You mean calculating value for the register when writing it? I think
for that purpose we can rely on crtc_state->enable_psr2_su_region_et
and crtc_state->dsc.compression_enable. No need to add new boolean.
Let's do it that way.
> 
> 
> >   
> >   	struct drm_rect psr2_su_area;
> >   
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 331645a2c9f6..0a2948ec308d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -2618,6 +2618,11 @@ void
> > intel_psr2_program_trans_man_trk_ctl(struct intel_dsb *dsb,
> >   
> >   	intel_de_write_dsb(display, dsb, PIPE_SRCSZ_ERLY_TPT(crtc-
> > >pipe),
> >   			   crtc_state->pipe_srcsz_early_tpt);
> > +	intel_de_write_dsb(display, dsb,
> > DSC_SU_PARAMETER_SET_0_DSC0(crtc->pipe),
> > +			   crtc_state-
> > >dsc_su_parameter_set_0_calc);
> > +	if (intel_dsc_get_vdsc_per_pipe(crtc_state) > 1)
> > +		intel_de_write_dsb(display, dsb,
> > DSC_SU_PARAMETER_SET_0_DSC1(crtc->pipe),
> > +				   crtc_state-
> > >dsc_su_parameter_set_0_calc);
> >   }
> >   
> >   static void psr2_man_trk_ctl_calc(struct intel_crtc_state
> > *crtc_state,
> > @@ -2668,6 +2673,23 @@ static u32
> > psr2_pipe_srcsz_early_tpt_calc(struct intel_crtc_state *crtc_state,
> >   	return PIPESRC_WIDTH(width - 1) | PIPESRC_HEIGHT(height -
> > 1);
> >   }
> >   
> > +static u32 psr2_dsc_su_parameter_set_0_calc(struct
> > intel_crtc_state *crtc_state,
> > +					    bool full_update)
> > +{
> > +	const struct drm_dsc_config *vdsc_cfg = &crtc_state-
> > >dsc.config;
> > +	int slice_row_per_frame, pic_height;
> > +
> > +	if (!crtc_state->enable_psr2_su_region_et || full_update
> > ||
> > +	    !crtc_state->dsc.compression_enable)
> > +		return 0;
> > +
> 
> Although we are making sure that height of the psr2_su_area is a 
> multiple of the slice_height, perhaps it would be good to have a 
> drm_WARN here to flag any misconfiguration i.e. if height is not a 
> multiple of slice_height.

I will add that warning and move these to intel_vdsc.c. I will also
move those register definitions you commented in patch 2.

> 
> 
> > +	slice_row_per_frame = drm_rect_height(&crtc_state-
> > >psr2_su_area) / vdsc_cfg->slice_height;
> > +	pic_height = slice_row_per_frame * vdsc_cfg->slice_height;
> > +
> > +	return
> > DSC_SU_PARAMETER_SET_0_SU_SLICE_ROW_PER_FRAME(slice_row_per_frame)
> > |
> > +		DSC_SU_PARAMETER_SET_0_SU_PIC_HEIGHT(pic_height);
> > +}
> 
> Since this writes a DSC register belonging to PPS Set 0, this
> function 
> should be moved to intel_vdsc.c.
> 
> Also, based on the boolean flag (psr_su_update_dsc_pps) discussed
> above, 
> this function should simply retrieve the required fields from
> crtc_state 
> and program the register.

Now as you pointed this out I see there is no real reason follow what
is done for PSR2_MAN_TRK_CTL.

> 
> Such a helper function should then be called from 
> intel_psr2_program_trans_man_trk_ctl() in place of the direct 
> intel_reg_write() call.
> 
> IMO, all register reads/writes, along with the wrappers/helpers
> around 
> them, should live in the file corresponding to the block that owns
> those 
> registers, based on context.

Ok, you convinced me. I will move these.

BR,
Jouni Högander

> 
> 
> Regards,
> 
> Ankit
> 
> > +
> >   static void clip_area_update(struct drm_rect
> > *overlap_damage_area,
> >   			     struct drm_rect *damage_area,
> >   			     struct drm_rect *pipe_src)
> > @@ -3026,6 +3048,8 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >   	psr2_man_trk_ctl_calc(crtc_state, full_update);
> >   	crtc_state->pipe_srcsz_early_tpt =
> >   		psr2_pipe_srcsz_early_tpt_calc(crtc_state,
> > full_update);
> > +	crtc_state->dsc_su_parameter_set_0_calc =
> > psr2_dsc_su_parameter_set_0_calc(crtc_state,
> > +								
> > 		   full_update);
> >   	return 0;
> >   }
> >