[PATCH] drm: add some comments to drm_wait_vblank and drm_queue_vblank_event

[PATCH] drm: add some comments to drm_wait_vblank and drm_queue_vblank_event

[Ilija Hadzic]

during the review of the fix for locks problems in drm_wait_vblank,
a couple of false concerns were raised about how the drm_vblank_get
and drm_vblank_put are used in this function; it turned out that the
code is correct and that it cannot be simplified

add a few comments to explain non-obvious flows in the code,
to prevent "false alarms" in the future

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_irq.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8b4da8..e9dd19d 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1065,6 +1065,10 @@ out:
 	return ret;
 }
 
+/* must acquire vblank reference count (call drm_vblank_get) */
+/* before calling this function; the matching drm_vblank_put */
+/* will either be issued here or in drm_handle_vblank_events */
+/* after the vblank is signaled */
 static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 				  union drm_wait_vblank *vblwait,
 				  struct drm_file *file_priv)
@@ -1124,6 +1128,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 		trace_drm_vblank_event_delivered(current->pid, pipe,
 						 vblwait->request.sequence);
 	} else {
+		/* can't call drm_vblank_put here because interrupt */
+		/* must remain enabled until the event occurs */
+		/* drm_handle_vblank_events will do this for us */
 		list_add_tail(&e->base.link, &dev->vblank_event_list);
 		vblwait->reply.sequence = vblwait->request.sequence;
 	}
@@ -1215,6 +1222,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 
 	if (flags & _DRM_VBLANK_EVENT) {
 		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+		/* drm_queue_vblank_event() will call drm_vblank_put() */
 		return drm_queue_vblank_event(dev, crtc, vblwait, file_priv);
 	}
 
-- 
1.7.7

Re: [PATCH] drm: add some comments to drm_wait_vblank and drm_queue_vblank_event

[Daniel Vetter]

On Fri, Oct 28, 2011 at 05:44:23PM -0400, Ilija Hadzic wrote:
> during the review of the fix for locks problems in drm_wait_vblank,
> a couple of false concerns were raised about how the drm_vblank_get
> and drm_vblank_put are used in this function; it turned out that the
> code is correct and that it cannot be simplified
> 
> add a few comments to explain non-obvious flows in the code,
> to prevent "false alarms" in the future
> 
> Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
> ---
>  drivers/gpu/drm/drm_irq.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c8b4da8..e9dd19d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1065,6 +1065,10 @@ out:
>  	return ret;
>  }
>  
> +/* must acquire vblank reference count (call drm_vblank_get) */
> +/* before calling this function; the matching drm_vblank_put */
> +/* will either be issued here or in drm_handle_vblank_events */
> +/* after the vblank is signaled */
>  static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
>  				  union drm_wait_vblank *vblwait,
>  				  struct drm_file *file_priv)
> @@ -1124,6 +1128,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
>  		trace_drm_vblank_event_delivered(current->pid, pipe,
>  						 vblwait->request.sequence);
>  	} else {
> +		/* can't call drm_vblank_put here because interrupt */
> +		/* must remain enabled until the event occurs */
> +		/* drm_handle_vblank_events will do this for us */
>  		list_add_tail(&e->base.link, &dev->vblank_event_list);
>  		vblwait->reply.sequence = vblwait->request.sequence;
>  	}
> @@ -1215,6 +1222,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  
>  	if (flags & _DRM_VBLANK_EVENT) {
>  		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +		/* drm_queue_vblank_event() will call drm_vblank_put() */

I think three comments for this is a bit overkill. Imo one here at the
only callsite of queue_vblank_event saying that it needs to hold onto the
vblank ref untill the event fires and hence will call drm_vblank_put
asynchronously is good enought.

>  		return drm_queue_vblank_event(dev, crtc, vblwait, file_priv);
>  	}
>  
> -- 
> 1.7.7
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48