On Wed, Apr 16, 2025 at 04:58:18PM -0400, Alyssa Rosenzweig wrote:
> > - spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > if (crtc->state->event) {
> > - drm_crtc_vblank_get(crtc);
> > - adp->event = crtc->state->event;
> > + spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > +
> > + if (drm_crtc_vblank_get(crtc) != 0)
> > + drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > + else
> > + adp->event = crtc->state->event;
> > +
> > + spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > }
> > crtc->state->event = NULL;
> > - spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>
> Kind of confused about
>
> > crtc->state->event = NULL;
>
> now being out of the lock. Should we set to NULL in the if, since
> if we don't take the if, we know event is already NULL? Or should we
> hold the lock for the whole time, the way the code did before your
> change? I'm not sure between the two, but the in-between here smells
> wrong.
I struggled with this as well. To my understanding event_lock is
necessary for drm_crtc_send_vblank_event(), adp->event and
drm_crtc_vblank_get(). The first according to event_lock's
documentation, the second to avoid avoid races with the irq handler and
the third to ensure vblank interrupts are not disabled.
Based on examples in other drivers I assumed `crtc->state->event` is
protected by another lock held externally. Not sure about that now. To
my understanding sruct drm_crtc::mutex protects `crtc->state`.
I did not move "crtc->state->event = NULL;" to avoid churn. No point
setting it to NULL if it is already NULL.
I'll look tomorrow if the locking for crtc->state->event is sufficient.
Janne
