> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville
> Syrjälä
> Sent: Tuesday, November 21, 2023 7:21 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
> > + /* 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.
> >
> > 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
--------------------
