Re: [PATCH 2/6] drm/i915: Reject async flips if we need to change DDB/watermarks

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

On Fri, Apr 19, 2024 at 04:27:53AM +0000, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Wednesday, March 20, 2024 9:34 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [PATCH 2/6] drm/i915: Reject async flips if we need to change
> > DDB/watermarks
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > DDB/watermarks are always double buffered on the vblank, so we can't safely
> > change them during async flips. Currently this never happens, but we'll be
> > making changing between sync and async flips a bit more flexible, in which case
> > we can actually end up here.
> 
> Rather on getting wm/DDB changes should we switch from async to sync flip to honour the wm/DDB changes else might end up in underrun or flicker/corruption.
> Spec is also aligned to this approach.

I can't really parse what you're saying.

The sequence of events that can lead us here are:
1. start in sync flip mode
2. userspace asks for an async flip (potentially asking for a
   different modifier)
   - we convert it to a sync flip and proceed
3. userspace asks for another async flip
   either:
   - the format/modifier (and thus wm/ddb) stays the same all
     is good and we do the async flip
   - the modifier changes we will now reject the request due to
     wm/ddb needing to change

We don't want to convert step 3 also to a sync flip because userspace
could just keep pingponging between two buffers with different modifiers
and we'd never actually get into proper async flip mode, and would just
keep doing sync flips. That would completely defat the purpose of async
flips.

And we do have to reject the request here in the wm code because
otherwise we'll clear the do_async_flip flag and the later
intel_async_flip_check_hw() wouldn't refuse the request even though
the modifier is changing. The other option would be to move some/all
of intel_async_flip_check_hw() into some earlier phase of
atomic_check(), but that would require some actual thought.


> Thanks and Regards,
> Arun R Murthy
> --------------------
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/skl_watermark.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index bc341abcab2f..1fa416a70d51 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -2540,6 +2540,12 @@ skl_ddb_add_affected_planes(const struct
> > intel_crtc_state *old_crtc_state,
> >  					&new_crtc_state-
> > >wm.skl.plane_ddb_y[plane_id]))
> >  			continue;
> > 
> > +		if (new_crtc_state->do_async_flip) {
> > +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> > change DDB during async flip\n",
> > +				    plane->base.base.id, plane->base.name);
> > +			return -EINVAL;
> > +		}
> > +
> >  		plane_state = intel_atomic_get_plane_state(state, plane);
> >  		if (IS_ERR(plane_state))
> >  			return PTR_ERR(plane_state);
> > @@ -2906,6 +2912,12 @@ static int skl_wm_add_affected_planes(struct
> > intel_atomic_state *state,
> >  						 &new_crtc_state-
> > >wm.skl.optimal))
> >  			continue;
> > 
> > +		if (new_crtc_state->do_async_flip) {
> > +			drm_dbg_kms(&i915->drm, "[PLANE:%d:%s] Can't
> > change watermarks during async flip\n",
> > +				    plane->base.base.id, plane->base.name);
> > +			return -EINVAL;
> > +		}
> > +
> >  		plane_state = intel_atomic_get_plane_state(state, plane);
> >  		if (IS_ERR(plane_state))
> >  			return PTR_ERR(plane_state);
> > --
> > 2.43.2
> 

-- 
Ville Syrjälä
Intel