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

[Daniel Stone <daniel@fooishbar.org>]

Hi,

On 10 January 2016 at 23:48, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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.

>> +     /* 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>

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel