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@xxxxxxxx> wrote:
> > From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx> (v2)
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
>
> 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@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
