[11/11] drm/omap: page-flip fixes

Message ID 1349725849-22433-12-git-send-email-rob.clark@linaro.org
State New
Headers show

Commit Message

Rob Clark Oct. 8, 2012, 7:50 p.m.
From: Rob Clark <rob@ti.com>

Userspace might not request a vblank event.  So it is not an error
for 'event' to be NULL, and we shouldn't use it to determine if
there is a pending flip already.

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/staging/omapdrm/omap_crtc.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Imre Deak Oct. 9, 2012, 9:35 a.m. | #1
On Mon, 2012-10-08 at 14:50 -0500, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> Userspace might not request a vblank event.  So it is not an error
> for 'event' to be NULL, and we shouldn't use it to determine if
> there is a pending flip already.
> 
> Signed-off-by: Rob Clark <rob@ti.com>
> ---
>  drivers/staging/omapdrm/omap_crtc.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c
> index 74e019a..317b854 100644
> --- a/drivers/staging/omapdrm/omap_crtc.c
> +++ b/drivers/staging/omapdrm/omap_crtc.c
> @@ -119,7 +119,6 @@ static void vblank_cb(void *arg)
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>  	unsigned long flags;
>  
> -	WARN_ON(!event);
>  	spin_lock_irqsave(&dev->event_lock, flags);
>  
>  	/* wakeup userspace */
> @@ -127,6 +126,7 @@ static void vblank_cb(void *arg)
>  		drm_send_vblank_event(dev, -1, omap_crtc->event);
>  
>  	omap_crtc->event = NULL;
> +	omap_crtc->old_fb = NULL;

Is old_fb used anywhere? If not we could just remove it.

Otherwise nice work! On the series:

Reviewed-by: Imre Deak <imre.deak@intel.com>

>  
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>  }
> @@ -138,8 +138,6 @@ static void page_flip_cb(void *arg)
>  	struct drm_framebuffer *old_fb = omap_crtc->old_fb;
>  	struct drm_gem_object *bo;
>  
> -	omap_crtc->old_fb = NULL;
> -
>  	omap_crtc_mode_set_base(crtc, crtc->x, crtc->y, old_fb);
>  
>  	/* really we'd like to setup the callback atomically w/ setting the
> @@ -162,7 +160,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
>  
>  	DBG("%d -> %d", crtc->fb ? crtc->fb->base.id : -1, fb->base.id);
>  
> -	if (omap_crtc->event) {
> +	if (omap_crtc->old_fb) {
>  		dev_err(dev->dev, "already a pending flip\n");
>  		return -EINVAL;
>  	}
Imre Deak Oct. 9, 2012, 9:38 a.m. | #2
On Tue, 2012-10-09 at 12:35 +0300, Imre Deak wrote:
> On Mon, 2012-10-08 at 14:50 -0500, Rob Clark wrote:
> > From: Rob Clark <rob@ti.com>
> > 
> > Userspace might not request a vblank event.  So it is not an error
> > for 'event' to be NULL, and we shouldn't use it to determine if
> > there is a pending flip already.
> > 
> > Signed-off-by: Rob Clark <rob@ti.com>
> > ---
> >  drivers/staging/omapdrm/omap_crtc.c |    6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c
> > index 74e019a..317b854 100644
> > --- a/drivers/staging/omapdrm/omap_crtc.c
> > +++ b/drivers/staging/omapdrm/omap_crtc.c
> > @@ -119,7 +119,6 @@ static void vblank_cb(void *arg)
> >  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >  	unsigned long flags;
> >  
> > -	WARN_ON(!event);
> >  	spin_lock_irqsave(&dev->event_lock, flags);
> >  
> >  	/* wakeup userspace */
> > @@ -127,6 +126,7 @@ static void vblank_cb(void *arg)
> >  		drm_send_vblank_event(dev, -1, omap_crtc->event);
> >  
> >  	omap_crtc->event = NULL;
> > +	omap_crtc->old_fb = NULL;
> 
> Is old_fb used anywhere? If not we could just remove it.
> 
> Otherwise nice work! On the series:
> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  
> >  	spin_unlock_irqrestore(&dev->event_lock, flags);
> >  }
> > @@ -138,8 +138,6 @@ static void page_flip_cb(void *arg)
> >  	struct drm_framebuffer *old_fb = omap_crtc->old_fb;
> >  	struct drm_gem_object *bo;
> >  
> > -	omap_crtc->old_fb = NULL;
> > -
> >  	omap_crtc_mode_set_base(crtc, crtc->x, crtc->y, old_fb);
> >  
> >  	/* really we'd like to setup the callback atomically w/ setting the
> > @@ -162,7 +160,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
> >  
> >  	DBG("%d -> %d", crtc->fb ? crtc->fb->base.id : -1, fb->base.id);
> >  
> > -	if (omap_crtc->event) {
> > +	if (omap_crtc->old_fb) {

Ah, just noticed this adds the use for it :) So ignore my comment above.

--Imre

> >  		dev_err(dev->dev, "already a pending flip\n");
> >  		return -EINVAL;
> >  	}
>

Patch

diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c
index 74e019a..317b854 100644
--- a/drivers/staging/omapdrm/omap_crtc.c
+++ b/drivers/staging/omapdrm/omap_crtc.c
@@ -119,7 +119,6 @@  static void vblank_cb(void *arg)
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 	unsigned long flags;
 
-	WARN_ON(!event);
 	spin_lock_irqsave(&dev->event_lock, flags);
 
 	/* wakeup userspace */
@@ -127,6 +126,7 @@  static void vblank_cb(void *arg)
 		drm_send_vblank_event(dev, -1, omap_crtc->event);
 
 	omap_crtc->event = NULL;
+	omap_crtc->old_fb = NULL;
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
@@ -138,8 +138,6 @@  static void page_flip_cb(void *arg)
 	struct drm_framebuffer *old_fb = omap_crtc->old_fb;
 	struct drm_gem_object *bo;
 
-	omap_crtc->old_fb = NULL;
-
 	omap_crtc_mode_set_base(crtc, crtc->x, crtc->y, old_fb);
 
 	/* really we'd like to setup the callback atomically w/ setting the
@@ -162,7 +160,7 @@  static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
 
 	DBG("%d -> %d", crtc->fb ? crtc->fb->base.id : -1, fb->base.id);
 
-	if (omap_crtc->event) {
+	if (omap_crtc->old_fb) {
 		dev_err(dev->dev, "already a pending flip\n");
 		return -EINVAL;
 	}