Re: [PATCH 12/38] drm/fence: add fence to drm_pending_event

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 02, 2016 at 02:49:13PM -0400, Sean Paul wrote:
> On Wed, Jun 1, 2016 at 6:06 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > Now a drm_pending_event can either send a real drm_event or signal a
> > fence, or both. It allow us to signal via fences when the buffer is
> > displayed on the screen. Which in turn means that the previous buffer
> > is not in use anymore and can be freed or sent back to another driver
> > for processing.
> >
> > v2: Comments from Daniel Vetter
> >         - call fence_signal in drm_send_event_locked()
> >         - remove unneeded !e->event check
> >
> > v3: Remove drm_pending_event->destroy to fix a leak when e->file_priv
> > is not set.
> >
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> (v2)
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> I suppose one could argue that the s/destroy/kfree/ change might
> warrant a separate change, but the patch is very readable as-is, so:
 
Yeah I got somewhat lazy with this one ;-)

> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Thanks for the review, patch merged to drm-misc.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c                | 19 +++++++++++++------
> >  drivers/gpu/drm/drm_fops.c                  | 16 +++++++++-------
> >  drivers/gpu/drm/nouveau/nouveau_usif.c      |  1 -
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  2 +-
> >  include/drm/drmP.h                          |  3 ++-
> >  5 files changed, 25 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 5093d81f60f7..5e4b820a977c 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1413,7 +1413,8 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
> >   */
> >
> >  static struct drm_pending_vblank_event *create_vblank_event(
> > -               struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data)
> > +               struct drm_device *dev, struct drm_file *file_priv,
> > +               struct fence *fence, uint64_t user_data)
> >  {
> >         struct drm_pending_vblank_event *e = NULL;
> >         int ret;
> > @@ -1426,12 +1427,17 @@ static struct drm_pending_vblank_event *create_vblank_event(
> >         e->event.base.length = sizeof(e->event);
> >         e->event.user_data = user_data;
> >
> > -       ret = drm_event_reserve_init(dev, file_priv, &e->base, &e->event.base);
> > -       if (ret) {
> > -               kfree(e);
> > -               return NULL;
> > +       if (file_priv) {
> > +               ret = drm_event_reserve_init(dev, file_priv, &e->base,
> > +                                            &e->event.base);
> > +               if (ret) {
> > +                       kfree(e);
> > +                       return NULL;
> > +               }
> >         }
> >
> > +       e->base.fence = fence;
> > +
> >         return e;
> >  }
> >
> > @@ -1671,7 +1677,8 @@ retry:
> >                 for_each_crtc_in_state(state, crtc, crtc_state, i) {
> >                         struct drm_pending_vblank_event *e;
> >
> > -                       e = create_vblank_event(dev, file_priv, arg->user_data);
> > +                       e = create_vblank_event(dev, file_priv, NULL,
> > +                                               arg->user_data);
> >                         if (!e) {
> >                                 ret = -ENOMEM;
> >                                 goto out;
> > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > index 868871068956..4c4b30f7a9f2 100644
> > --- a/drivers/gpu/drm/drm_fops.c
> > +++ b/drivers/gpu/drm/drm_fops.c
> > @@ -294,7 +294,7 @@ static void drm_events_release(struct drm_file *file_priv)
> >         /* Remove unconsumed events */
> >         list_for_each_entry_safe(e, et, &file_priv->event_list, link) {
> >                 list_del(&e->link);
> > -               e->destroy(e);
> > +               kfree(e);
> >         }
> >
> >         spin_unlock_irqrestore(&dev->event_lock, flags);
> > @@ -525,7 +525,7 @@ put_back_event:
> >                         }
> >
> >                         ret += length;
> > -                       e->destroy(e);
> > +                       kfree(e);
> >                 }
> >         }
> >         mutex_unlock(&file_priv->event_read_lock);
> > @@ -602,9 +602,6 @@ int drm_event_reserve_init_locked(struct drm_device *dev,
> >         list_add(&p->pending_link, &file_priv->pending_event_list);
> >         p->file_priv = file_priv;
> >
> > -       /* we *could* pass this in as arg, but everyone uses kfree: */
> > -       p->destroy = (void (*) (struct drm_pending_event *)) kfree;
> > -
> >         return 0;
> >  }
> >  EXPORT_SYMBOL(drm_event_reserve_init_locked);
> > @@ -667,7 +664,7 @@ void drm_event_cancel_free(struct drm_device *dev,
> >                 list_del(&p->pending_link);
> >         }
> >         spin_unlock_irqrestore(&dev->event_lock, flags);
> > -       p->destroy(p);
> > +       kfree(p);
> >  }
> >  EXPORT_SYMBOL(drm_event_cancel_free);
> >
> > @@ -689,8 +686,13 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
> >  {
> >         assert_spin_locked(&dev->event_lock);
> >
> > +       if (e->fence) {
> > +               fence_signal(e->fence);
> > +               fence_put(e->fence);
> > +       }
> > +
> >         if (!e->file_priv) {
> > -               e->destroy(e);
> > +               kfree(e);
> >                 return;
> >         }
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_usif.c b/drivers/gpu/drm/nouveau/nouveau_usif.c
> > index 675e9e077a95..08f9c6fa0f7f 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_usif.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_usif.c
> > @@ -212,7 +212,6 @@ usif_notify_get(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
> >         ntfy->p->base.event = &ntfy->p->e.base;
> >         ntfy->p->base.file_priv = f;
> >         ntfy->p->base.pid = current->pid;
> > -       ntfy->p->base.destroy =(void(*)(struct drm_pending_event *))kfree;
> >         ntfy->p->e.base.type = DRM_NOUVEAU_EVENT_NVIF;
> >         ntfy->p->e.base.length = sizeof(ntfy->p->e.base) + ntfy->reply;
> >
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index 1c4d5b5a70a2..5567fb43e674 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -889,7 +889,7 @@ static void vop_crtc_cancel_pending_vblank(struct drm_crtc *crtc,
> >         if (e && e->base.file_priv == file_priv) {
> >                 vop->event = NULL;
> >
> > -               e->base.destroy(&e->base);
> > +               kfree(&e->base);
> >                 file_priv->event_space += sizeof(e->event);
> >         }
> >         spin_unlock_irqrestore(&drm->event_lock, flags);
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index f51fa4328494..51f751d1c8a4 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -57,6 +57,7 @@
> >  #include <linux/types.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/workqueue.h>
> > +#include <linux/fence.h>
> >
> >  #include <asm/mman.h>
> >  #include <asm/pgalloc.h>
> > @@ -283,12 +284,12 @@ struct drm_ioctl_desc {
> >  /* Event queued up for userspace to read */
> >  struct drm_pending_event {
> >         struct drm_event *event;
> > +       struct fence *fence;
> >         struct list_head link;
> >         struct list_head pending_link;
> >         struct drm_file *file_priv;
> >         pid_t pid; /* pid of requester, no guarantee it's valid by the time
> >                       we deliver the event, for tracing only */
> > -       void (*destroy)(struct drm_pending_event *event);
> >  };
> >
> >  /* initial implementaton using a linked list - todo hashtab */
> > --
> > 2.8.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx