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@xxxxxxxxxxxxx>
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
