diff mbox

[RFC,5/8] drm/fence: add fence to drm_pending_event

Message ID 1460683781-22535-6-git-send-email-gustavo@padovan.org (mailing list archive)
State New, archived
Headers show

Commit Message

Gustavo Padovan April 15, 2016, 1:29 a.m. UTC
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.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
 drivers/gpu/drm/drm_fops.c   |  5 +++--
 drivers/gpu/drm/drm_irq.c    |  7 +++++++
 include/drm/drmP.h           |  1 +
 4 files changed, 24 insertions(+), 8 deletions(-)

Comments

Daniel Vetter April 15, 2016, 8:09 a.m. UTC | #1
On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan 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.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Do you have atomic igt testcase that exercise all the combinations of
drm_event and in/out-fences and make sure it all keeps working? Tomeu
already converted that over to be a generic testcase.

> ---
>  drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
>  drivers/gpu/drm/drm_fops.c   |  5 +++--
>  drivers/gpu/drm/drm_irq.c    |  7 +++++++
>  include/drm/drmP.h           |  1 +
>  4 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 6702502..0b95526 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_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;
> @@ -1444,12 +1445,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;
>  }
>  
> @@ -1676,7 +1682,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 aeef58e..38def49 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
>  {
>  	assert_spin_locked(&dev->event_lock);
>  
> -	if (!e->file_priv) {
> -		e->destroy(e);
> +	if (!e->file_priv || !e->event) {

This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
this happen?

> +		if (e->destroy)
> +			e->destroy(e);
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 3c1a6f1..0c5d7cb 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -41,6 +41,7 @@
>  
>  #include <linux/vgaarb.h>
>  #include <linux/export.h>
> +#include <linux/fence.h>
>  
>  /* Access macro for slots in vblank timestamp ringbuffer. */
>  #define vblanktimestamp(dev, pipe, count) \
> @@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
>  		now = get_drm_timestamp();
>  	}
>  	e->pipe = pipe;
> +
> +	if (e->base.fence) {
> +		fence_signal(e->base.fence);
> +		fence_put(e->base.fence);
> +	}

I'd put this into drm_send_event_locked even. In case we send out fences
for other events too (e.g. exynos uses drm_event for rendering ...).
-Daniel

> +
>  	send_vblank_event(dev, e, seq, &now);
>  }
>  EXPORT_SYMBOL(drm_send_vblank_event);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c..8f83c2a 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -282,6 +282,7 @@ 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;
> -- 
> 2.5.5
>
Gustavo Padovan April 15, 2016, 6:59 p.m. UTC | #2
2016-04-15 Daniel Vetter <daniel@ffwll.ch>:

> On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan 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.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Do you have atomic igt testcase that exercise all the combinations of
> drm_event and in/out-fences and make sure it all keeps working? Tomeu
> already converted that over to be a generic testcase.

Not yet. I'll check for test cases.

> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 19 +++++++++++++------
> >  drivers/gpu/drm/drm_fops.c   |  5 +++--
> >  drivers/gpu/drm/drm_irq.c    |  7 +++++++
> >  include/drm/drmP.h           |  1 +
> >  4 files changed, 24 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 6702502..0b95526 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1431,7 +1431,8 @@ EXPORT_SYMBOL(drm_atomic_async_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;
> > @@ -1444,12 +1445,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;
> >  }
> >  
> > @@ -1676,7 +1682,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 aeef58e..38def49 100644
> > --- a/drivers/gpu/drm/drm_fops.c
> > +++ b/drivers/gpu/drm/drm_fops.c
> > @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
> >  {
> >  	assert_spin_locked(&dev->event_lock);
> >  
> > -	if (!e->file_priv) {
> > -		e->destroy(e);
> > +	if (!e->file_priv || !e->event) {
> 
> This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
> this happen?

Not sure now. But I needed this to prevent a crash, I don't have logs of
it anymore, I'll check this again.

> 
> > +		if (e->destroy)
> > +			e->destroy(e);
> >  		return;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 3c1a6f1..0c5d7cb 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -41,6 +41,7 @@
> >  
> >  #include <linux/vgaarb.h>
> >  #include <linux/export.h>
> > +#include <linux/fence.h>
> >  
> >  /* Access macro for slots in vblank timestamp ringbuffer. */
> >  #define vblanktimestamp(dev, pipe, count) \
> > @@ -1124,6 +1125,12 @@ void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
> >  		now = get_drm_timestamp();
> >  	}
> >  	e->pipe = pipe;
> > +
> > +	if (e->base.fence) {
> > +		fence_signal(e->base.fence);
> > +		fence_put(e->base.fence);
> > +	}
> 
> I'd put this into drm_send_event_locked even. In case we send out fences
> for other events too (e.g. exynos uses drm_event for rendering ...).

Okay. Looking into that.

	Gustavo
Daniel Vetter April 15, 2016, 7:31 p.m. UTC | #3
On Fri, Apr 15, 2016 at 11:59:00AM -0700, Gustavo Padovan wrote:
> 2016-04-15 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Apr 14, 2016 at 06:29:38PM -0700, Gustavo Padovan wrote:
> > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> > > index aeef58e..38def49 100644
> > > --- a/drivers/gpu/drm/drm_fops.c
> > > +++ b/drivers/gpu/drm/drm_fops.c
> > > @@ -801,8 +801,9 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
> > >  {
> > >  	assert_spin_locked(&dev->event_lock);
> > >  
> > > -	if (!e->file_priv) {
> > > -		e->destroy(e);
> > > +	if (!e->file_priv || !e->event) {
> > 
> > This would be a bug: e->file_priv != NULL iff e->event != NULL. How did
> > this happen?
> 
> Not sure now. But I needed this to prevent a crash, I don't have logs of
> it anymore, I'll check this again.

There was a massive irc discussion with Daniel Stone, so I'll try to
summarize it here. There are 3 possible cases:

e->file_priv == NULL && e->event == NULL:
This is a drm_event without a drm_event. Probably e->fence is set, if not
then it's a completeley useless thing (but not forbidden).

e->file_priv != NULL && e->event != NULL:
drm_event with an event for the given file_priv attached.

e->file_priv == NULL && e->event != NULL:
Above case, but with the file_priv closed and unlinked from the event.

The 4th case, which is the only case things will change with the above
hunk, is not allowed. If you hit it, there's a bug somewhere.

This is all completely idependent of e->fence, which this patch adds.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 6702502..0b95526 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1431,7 +1431,8 @@  EXPORT_SYMBOL(drm_atomic_async_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;
@@ -1444,12 +1445,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;
 }
 
@@ -1676,7 +1682,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 aeef58e..38def49 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -801,8 +801,9 @@  void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
 {
 	assert_spin_locked(&dev->event_lock);
 
-	if (!e->file_priv) {
-		e->destroy(e);
+	if (!e->file_priv || !e->event) {
+		if (e->destroy)
+			e->destroy(e);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 3c1a6f1..0c5d7cb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -41,6 +41,7 @@ 
 
 #include <linux/vgaarb.h>
 #include <linux/export.h>
+#include <linux/fence.h>
 
 /* Access macro for slots in vblank timestamp ringbuffer. */
 #define vblanktimestamp(dev, pipe, count) \
@@ -1124,6 +1125,12 @@  void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
 		now = get_drm_timestamp();
 	}
 	e->pipe = pipe;
+
+	if (e->base.fence) {
+		fence_signal(e->base.fence);
+		fence_put(e->base.fence);
+	}
+
 	send_vblank_event(dev, e, seq, &now);
 }
 EXPORT_SYMBOL(drm_send_vblank_event);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c8422c..8f83c2a 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -282,6 +282,7 @@  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;