[Intel-gfx] [PATCH 1/2] drm/i915: Stop accessing crtc->state from the flip done irq

[Ville Syrjälä]

On Tue, Dec 05, 2023 at 11:16:58PM +0000, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> > Syrjälä
> > Sent: Tuesday, November 21, 2023 7:21 PM
> > To: intel-gfx at lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Stop accessing crtc->state from
> > the flip done irq
> > 
> > On Thu, Sep 28, 2023 at 06:24:49PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > >
> > > Assuming crtc->state is pointing at the correct thing for the async
> > > flip commit is nonsense. If we had already queued up multiple commits
> > > this would point at the very lates crtc state even if the older
> > > commits hadn't even happened yet.
> > >
> > > Instead properly stage/arm the event like we do for async flips.
> > > Since we don't need to arm multiple of these at the same time we don't
> > > need a list like the normal vblank even processing uses.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_crtc.c          | 9 ++++++++-
> > >  drivers/gpu/drm/i915/display/intel_display_irq.c   | 9 ++++-----
> > >  drivers/gpu/drm/i915/display/intel_display_types.h | 3 +++
> > >  3 files changed, 15 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index 1fd068e6e26c..8a84a31c7b48 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -553,8 +553,15 @@ void intel_pipe_update_start(struct
> > > intel_atomic_state *state,
> > >
> > >  	intel_psr_lock(new_crtc_state);
> > >
> > > -	if (new_crtc_state->do_async_flip)
> > > +	if (new_crtc_state->do_async_flip) {
> > > +		spin_lock_irq(&crtc->base.dev->event_lock);
> 
> 
> Would it be better to use irqsave since we are dealing with events.

One uses irqsave/restore when the we must protect against irq handlers,
and the code can be called both with irqs enabled and irqs disabled.
Here we are always called with irqs enabled, so the save/restore would
be pointless.

> 
> > > +		/* arm the event for the flip done irq handler */
> > > +		crtc->flip_done_event = new_crtc_state->uapi.event;
> > > +		spin_unlock_irq(&crtc->base.dev->event_lock);
> > > +
> > > +		new_crtc_state->uapi.event = NULL;
> > >  		return;
> > > +	}
> > >
> > >  	if (intel_crtc_needs_vblank_work(new_crtc_state))
> > >  		intel_crtc_vblank_work_init(new_crtc_state);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > index bff4a76310c0..d3df615f0e48 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> > > @@ -340,16 +340,15 @@ static void flip_done_handler(struct
> > drm_i915_private *i915,
> > >  			      enum pipe pipe)
> > >  {
> > >  	struct intel_crtc *crtc = intel_crtc_for_pipe(i915, pipe);
> > > -	struct drm_crtc_state *crtc_state = crtc->base.state;
> > > -	struct drm_pending_vblank_event *e = crtc_state->event;
> > >  	struct drm_device *dev = &i915->drm;
> > >  	unsigned long irqflags;
> > >
> > >  	spin_lock_irqsave(&dev->event_lock, irqflags);
> > >
> > > -	crtc_state->event = NULL;
> > > -
> > > -	drm_crtc_send_vblank_event(&crtc->base, e);
> > > +	if (crtc->flip_done_event) {
> > > +		drm_crtc_send_vblank_event(&crtc->base, crtc-
> > >flip_done_event);
> > > +		crtc->flip_done_event = NULL;
> > > +	}
> > 
> > I just observed an oops here due to e==NULL with the current code.
> > I *think* I've seen it once before as well. Pstore also caught what seemed to
> > some kind of spurious DE interrupt, which might explain the oops. But not
> > really sure what happened as the machine died before I could poke at it more.
> > 
> 
> Earlier the event was set to NULL and then drm_crtc_send_vblank_event() was called.

The question is "how was this called when the event was NULL?".

The possible answers are:
- spurious flip done irq
- some kind of race with multiple commits, but can't immediately
  think how that would happen as we still signal hw_done after
  flip_done, and drm_atomic_helper_swap_state() will block on
  hw_done, and the flip_done irq will not be enabled otherwise

> 
> > >
> > >  	spin_unlock_irqrestore(&dev->event_lock, irqflags);  } diff --git
> > > a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 8d8b2f8d37a9..a8ae1a25a550 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1461,6 +1461,9 @@ struct intel_crtc {
> > >
> > >  	struct intel_crtc_state *config;
> > >
> > > +	/* armed event for async flip */
> > > +	struct drm_pending_vblank_event *flip_done_event;
> > > +
> > >  	/* Access to these should be protected by dev_priv->irq_lock. */
> > >  	bool cpu_fifo_underrun_disabled;
> > >  	bool pch_fifo_underrun_disabled;
> > > --
> > > 2.41.0
> > 
> > --
> > Ville Syrjälä
> > Intel
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------

-- 
Ville Syrjälä
Intel