Re: [PATCH v3 2/3] drm/i915/psr: Perform full frame update on async flip

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

On Wed, Dec 03, 2025 at 03:13:11PM +0000, Hogander, Jouni wrote:
> On Wed, 2025-12-03 at 17:08 +0200, Ville Syrjälä wrote:
> > On Wed, Dec 03, 2025 at 01:58:23PM +0000, Hogander, Jouni wrote:
> > > On Wed, 2025-12-03 at 15:22 +0200, Ville Syrjälä wrote:
> > > > On Mon, Dec 01, 2025 at 03:24:56PM +0200, Jouni Högander wrote:
> > > > > According to bspec selective fetch is not supported with async
> > > > > flips and
> > > > > instructing full frame update on async flip.
> > > > > 
> > > > > v3:
> > > > >   - rebase
> > > > >   - fix old_crtc_state->pipe_srcsz_early_tpt
> > > > >   - fix using intel_atomic_get_new_crtc_state
> > > > > v2:
> > > > >   - check also crtc_state->async_flip_planes in
> > > > >     psr2_sel_fetch_plane_state_supported
> > > > > 
> > > > > Bspec: 55229
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_psr.c | 72 ++++++++++++++--
> > > > > ----
> > > > > ----
> > > > >  1 file changed, 41 insertions(+), 31 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > index 15ef3b6caad6..53cf292247d7 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > @@ -2728,13 +2728,20 @@
> > > > > intel_psr2_sel_fetch_et_alignment(struct
> > > > > intel_atomic_state *state,
> > > > >   * Plane scaling and rotation is not supported by selective
> > > > > fetch
> > > > > and both
> > > > >   * properties can change without a modeset, so need to be
> > > > > check at
> > > > > every
> > > > >   * atomic commit.
> > > > > + *
> > > > > + * If plane was having async flip previously we can't use
> > > > > selective
> > > > > + * fetch as we don't know if the flip is completed.
> > > > >   */
> > > > > -static bool psr2_sel_fetch_plane_state_supported(const struct
> > > > > intel_plane_state *plane_state)
> > > > > +static bool psr2_sel_fetch_plane_state_supported(const struct
> > > > > intel_crtc_state *old_crtc_state,
> > > > > +						 const struct
> > > > > intel_plane_state *plane_state)
> > > > >  {
> > > > > +	struct intel_plane *plane =
> > > > > to_intel_plane(plane_state-
> > > > > > uapi.plane);
> > > > > +
> > > > >  	if (plane_state->uapi.dst.y1 < 0 ||
> > > > >  	    plane_state->uapi.dst.x1 < 0 ||
> > > > >  	    plane_state->scaler_id >= 0 ||
> > > > > -	    plane_state->hw.rotation != DRM_MODE_ROTATE_0)
> > > > > +	    plane_state->hw.rotation != DRM_MODE_ROTATE_0 ||
> > > > > +	    old_crtc_state->async_flip_planes & plane->id)
> > > > 
> > > > Why are you looking at the old crtc state? There should be
> > > > nothing of
> > > > interest to us there.
> > > 
> > > To continue keeping CFF bit set if previous update was async flip.
> > > This
> > > is how I understood it (please correct):
> > > 
> > > 0. syncronous update
> > > 
> > > 1. async_flip:
> > >    new_crtc_state->async_flip_planes != 0
> > >    old_crtc_state->async_flip_planes == 0
> > >    crtc_state->uapi.async_flip == true)
> > >    -> full frame update
> > > 
> > > 2. async_flip:
> > >    new_crtc_state->async_flip_planes != 0
> > >    old_crtc_state->async_flip_planes != 0
> > >    crtc_state->uapi.async_flip == true
> > >    -> full frame update
> > > 
> > > 3. syncronous update
> > >    new_crtc_state->async_flip_planes == 0
> > >    old_crtc_state->async_flip_planes != 0
> > >    crtc_state->uapi.async_flip == false
> > >    -> full frame update
> > > 
> > > 4. syncronous update
> > >    new_crtc_state->async_flip_planes == 0
> > >    old_crtc_state->async_flip_planes == 0
> > >    crtc_state->uapi.async_flip == false
> > >    -> selective update
> > > 
> > > > 
> > > > >  		return false;
> > > > >  
> > > > >  	return true;
> > > > > @@ -2749,7 +2756,8 @@ static bool
> > > > > psr2_sel_fetch_plane_state_supported(const struct
> > > > > intel_plane_state
> > > > >   */
> > > > >  static bool psr2_sel_fetch_pipe_state_supported(const struct
> > > > > intel_crtc_state *crtc_state)
> > > > >  {
> > > > > -	if (crtc_state->scaler_state.scaler_id >= 0)
> > > > > +	if (crtc_state->scaler_state.scaler_id >= 0 ||
> > > > > +	    crtc_state->uapi.async_flip)
> > > > 
> > > > I think just checking crtc_state->async_flip_planes!=0 here
> > > > should be
> > > > sufficient.
> > > 
> > > I'm doing this to handle step 1. above. Alternatively I could check
> > > both new_crtc_state->async_flip_planes and old_crtc_state-
> > > > async_flip_planes. When using crtc_state->uapi.async_flip I can
> > > > decide
> > > earlier.
> > > 
> > > > The rest of the patch seems unnecessary.
> > > 
> > > No need to handle selective update where planes having pending
> > > async
> > > flip are not involved?
> > 
> > There won't be pending flips of any kind. We whole CRTC commit queue
> > is handled in a strict FIFO order (apart from the legacy cursor
> > special case).
> > 
> 
> Ok, so it's not possible to take in new CRTC commit before async flip
> is completed?

More or less. The new request can come in before that, but it won't
be commited to the hardware until the previous one has finished.

-- 
Ville Syrjälä
Intel