Re: [PATCH v6 07/12] drm/i915/psr: Changes for PSR2_MAN_TRK_CTL handling when DSB is in use

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 11, 2025 at 06:24:24AM +0000, Hogander, Jouni wrote:
> On Mon, 2025-02-10 at 18:26 +0200, Ville Syrjälä wrote:
> > On Mon, Jan 27, 2025 at 12:28:41PM +0200, Jouni Högander wrote:
> > > Do needed changes to handle PSR2_MAN_TRK_CTL correctly when DSB is
> > > in use:
> > > 
> > > 1. Write PSR2_MAN_TRK_CTL in commit_pipe_pre_planes only when not
> > > using
> > >    DSB.
> > > 2. Add PSR2_MAN_TRK_CTL writing into DSB commit in
> > >    intel_atomic_dsb_finish.
> > > 
> > > Taking PSR lock over DSB commit is not needed because
> > > PSR2_MAN_TRK_CTL is
> > > now written only by DSB.
> > > 
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > Reviewed-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index aed35f203fd8d..5db2af86d0c8a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -7143,7 +7143,8 @@ static void commit_pipe_pre_planes(struct
> > > intel_atomic_state *state,
> > >  			intel_pipe_fastset(old_crtc_state,
> > > new_crtc_state);
> > >  	}
> > >  
> > > -	intel_psr2_program_trans_man_trk_ctl(NULL,
> > > new_crtc_state);
> > > +	if (!new_crtc_state->use_dsb)
> > > +		intel_psr2_program_trans_man_trk_ctl(NULL,
> > > new_crtc_state);
> > 
> > commit_pipe_pre_planes() is not called when use_dsb==true.
> 
> Couple of lines earlier in same function there is this:
> 
> 	if (!modeset && !new_crtc_state->use_dsb) {
> 
> I followed that in here. Do you still think I should remove checking
> use_dsb from my patch?

Hmm, I guess it was some leftover from some of my earlier attempts
at reusing more of the existing commit path. We could remove it from
there as well for the time being. I suppose we could stick a few
drm_WARN_ON(use_dsb)s into commit_pipe_{pre,post}_planes() and
intel_pipe_update_{start,end}() to make it clear they aren't used
by the full DSB path.

At some point we need to attempt to refactor this stuff into some
kind of more sensible form. But as long as we can't do all the
relevant programming on the DSB it's a bit hard to see what the
end result should look like.

-- 
Ville Syrjälä
Intel