Re: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor updates

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

On Wed, Dec 20, 2023 at 11:45:44AM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Shankar, Uma
> > Sent: Wednesday, December 20, 2023 5:11 PM
> > To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org
> > Subject: RE: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy cursor
> > updates
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > > Ville Syrjala
> > > Sent: Wednesday, December 13, 2023 3:55 PM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [PATCH 8/9] drm/i915: Perform vblank evasion around legacy
> > > cursor updates
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Our legacy cursor updates are actually mailbox updates.
> > > Ie. the hardware latches things once per frame on start of vblank, but
> > > we issue an number of updates per frame, withough any attempt to
> > > synchronize against the vblank in software. So in theory only the last
> > > update issued during the frame will latch, and the previous ones are discarded.
> > >
> > > However this can lead to problems with maintaining the ggtt/iommu
> > > mappings as we have no idea which updates will actually latch.
> > >
> > > The problem is exacerbated by the hardware's annoying disarming
> > > behaviour; any non-arming register write will disarm an already armed
> > > update, only to be rearmed later by the arming register (CURBASE in
> > > case of cursors). If a disarming write happens just before the start
> > > of vblank, and the arming write happens after start of vblank we have
> > > effectively prevented the hardware from latching anything. And if we
> > > manage to straddle multiple sequential vblank starts in this manner we
> > > effectively prevent the hardware from latching any new registers for
> > > an arbitrary amount of time. This provides more time for the (potentially still in
> > use by the hardware) gtt/iommu mappings to be torn down.
> > >
> > > A partial solution, of course, is to use vblank evasion to avoid the
> > > register writes from spreading on both sides of the start of vblank.
> > >
> > > I've previously highlighted this problem as a general issue affecting
> > > mailbox updates. I even added some notes to the
> > > {i9xx,skl}_crtc_planes_update_arm() to remind us that the noarm and
> > > arm phases both need to pulled into the vblank evasion critical
> > > section if we actually decided to implement mailbox updates in
> > > general. But as I never impelemented the
> > > noarm+arm split for cursors we don't have to worry about that for the moment.
> > >
> > > We've been lucky enough so far that this hasn't really caused
> > > problems. One thing that does help is that Xorg generally sticks to
> > > the same cursor BO. But igt seems pretty good at hitting this on MTL
> > > now, so apparently we have to start thinking about this.
> > 
> > Was not aware that a disarming update will disarm an armed update and make
> > the vblank sync irrelevant. Thanks for a good writeup highlighting the issue, really
> > helps.
> > 
> > Yeah, this should help maintain consistency with cursor updates and ensure the
> > sync at vblank. Change looks Good to me.
> > Reviewed-by: Uma Shankar <uma.shankar@intel.com>
> > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cursor.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > index 926e2de00eb5..77531838001f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > @@ -22,6 +22,7 @@
> > >  #include "intel_frontbuffer.h"
> > >  #include "intel_psr.h"
> > >  #include "intel_psr_regs.h"
> > > +#include "intel_vblank.h"
> > >  #include "skl_watermark.h"
> > >
> > >  #include "gem/i915_gem_object.h"
> > > @@ -647,12 +648,14 @@ intel_legacy_cursor_update(struct drm_plane
> > > *_plane, {
> > >  	struct intel_plane *plane = to_intel_plane(_plane);
> > >  	struct intel_crtc *crtc = to_intel_crtc(_crtc);
> > > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > >  	struct intel_plane_state *old_plane_state =
> > >  		to_intel_plane_state(plane->base.state);
> > >  	struct intel_plane_state *new_plane_state;
> > >  	struct intel_crtc_state *crtc_state =
> > >  		to_intel_crtc_state(crtc->base.state);
> > >  	struct intel_crtc_state *new_crtc_state;
> > > +	struct intel_vblank_evade_ctx evade;
> > >  	int ret;
> > >
> > >  	/*
> > > @@ -745,14 +748,15 @@ intel_legacy_cursor_update(struct drm_plane
> > *_plane,
> > >  	 */
> > >  	crtc_state->active_planes = new_crtc_state->active_planes;
> > >
> > > -	/*
> > > -	 * Technically we should do a vblank evasion here to make
> > > -	 * sure all the cursor registers update on the same frame.
> > > -	 * For now just make sure the register writes happen as
> > > -	 * quickly as possible to minimize the race window.
> > > -	 */
> > > +	intel_vblank_evade_init(crtc_state, crtc_state, &evade);
> 
> Missed to update: 
> Should the 2nd argument not be new_crtc_state ?

We'll discard 'new_crtc_state' at the end and we just update
'crtc_state' in place (only active_planes actually). So essentially
'crtc_state' here is the new state already. That should be perfectly
fine for the purposes of vblank evasion since that only cares about
the old state when modesets are involved (which is never the case here).

> 
> > >  	local_irq_disable();
> > >
> > > +	if (!drm_WARN_ON(&i915->drm, drm_crtc_vblank_get(&crtc->base))) {
> > > +		intel_vblank_evade(&evade);
> > > +		drm_crtc_vblank_put(&crtc->base);
> > > +	}
> > > +
> > >  	if (new_plane_state->uapi.visible) {
> > >  		intel_plane_update_noarm(plane, crtc_state, new_plane_state);
> > >  		intel_plane_update_arm(plane, crtc_state, new_plane_state);
> > > --
> > > 2.41.0
> 

-- 
Ville Syrjälä
Intel