Re: [PATCH] drm: Clean up pending events in the core

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Monday 11 January 2016 14:51:46 Daniel Stone wrote:
> On 10 January 2016 at 23:48, Laurent Pinchart wrote:
> > On Saturday 09 January 2016 14:28:46 Daniel Vetter wrote:
> >> @@ -353,18 +354,16 @@ static void drm_events_release(struct drm_file
> >> *file_priv) {
> >> 
> >>       struct drm_device *dev = file_priv->minor->dev;
> >>       struct drm_pending_event *e, *et;
> >> 
> >> -     struct drm_pending_vblank_event *v, *vt;
> >> 
> >>       unsigned long flags;
> >>       
> >>       spin_lock_irqsave(&dev->event_lock, flags);
> >> 
> >> -     /* Remove pending flips */
> >> -     list_for_each_entry_safe(v, vt, &dev->vblank_event_list, base.link)
> >> -             if (v->base.file_priv == file_priv) {
> >> -                     list_del(&v->base.link);
> >> -                     drm_vblank_put(dev, v->pipe);
> >> -                     v->base.destroy(&v->base);
> >> -             }
> > 
> > Where does this code go ?
> 
> It doesn't: instead of deleting the events, the helpers to either
> cancel or send the event just notice that file_priv is NULL and bail
> out early.

Thank you for the explanation. What bothered me was that the drm_vblank_put() 
call got dropped. With v2 of this patch series that splits that change to a 
separate patch it's easier to understand.

> >> +     /* Unlink pending events */
> >> +     list_for_each_entry_safe(e, et, &file_priv->pending_event_list,
> >> +                              pending_link) {
> >> +             list_del(&e->pending_link);
> >> +             e->file_priv = NULL;
> >> +     }
> 
> file_priv gets reset here ...
> 
> >> @@ -736,7 +736,10 @@ void drm_event_cancel_free(struct drm_device *dev,
> >> 
> >>  {
> >>  
> >>       unsigned long flags;
> >>       spin_lock_irqsave(&dev->event_lock, flags);
> >> 
> >> -     p->file_priv->event_space += p->event->length;
> >> +     if (p->file_priv) {
> >> +             p->file_priv->event_space += p->event->length;
> >> +             list_del(&p->pending_link);
> >> +     }
> >> 
> >>       spin_unlock_irqrestore(&dev->event_lock, flags);
> >>       p->destroy(p);
> 
> Allowing us to DTRT here ...
> 
> >>  void drm_send_event_locked(struct drm_device *dev, struct
> >>  drm_pending_event
> >> 
> >> *e) {
> >> 
> >>       assert_spin_locked(&dev->event_lock);
> >> 
> >> +     if (!e->file_priv) {
> > 
> > I don't think this could happen before this patch as e->file_priv is
> > dereferenced below, and I don't see anything in this patch that makes the
> > condition possible.
> > 
> >> +             e->destroy(e);
> >> +             return;
> >> +     }
> 
> ... and now here.
> 
> This looks good to me, and a good sight better than doing it in every
> driver. Still drowning in stuff after three weeks off though, so the
> best I can offer for the series right now is:
> Acked-by: Daniel Stone <daniels@collabora.com>

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx