[PATCH 1/4] drm: Really never disable vblank irqs for offdelay==0

[PATCH 1/4] drm: Really never disable vblank irqs for offdelay==0

[Daniel Vetter]

With the new support for immediate vblank disabling we always disabled
the vblank interrupt right away, irrespective of the vblank offdelay
setting.

But being able to let vblanks run forever is fairly useful for
debugging, so restore that behaviour.

Suggested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_irq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index a75da075927c..6eb015020af2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1024,9 +1024,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 
 	/* Last user schedules interrupt disable */
 	if (atomic_dec_and_test(&vblank->refcount)) {
-		if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
+		if (drm_vblank_offdelay == 0)
+			return;
+		else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
 			vblank_disable_fn((unsigned long)vblank);
-		else if (drm_vblank_offdelay > 0)
+		else
 			mod_timer(&vblank->disable_timer,
 				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
 	}
-- 
1.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm: Only update final vblank count when precise ts is available

[Daniel Vetter]

Drivers without a hardware vblank counter simply can't account for the
vblanks that happened while the vblank interrupt was off. To check
this grab a vblank timestamp and if the result is dubious follow the
normal save-and-disable logic.

Drivers should prevent this by setting vblank_disable_allowed = false,
but since running vblank interrupts constantly is not good for power
consumption most drivers lie. Testing for precise vblank timestamps is
the next best thing we can check for.

Suggested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_irq.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 6eb015020af2..922721ead29a 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -163,8 +163,15 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * has been ticking all along until this time. This makes the
 	 * count account for the entire time between drm_vblank_on() and
 	 * drm_vblank_off().
+	 *
+	 * But only do this if precise vblank timestamps are available.
+	 * Otherwise we might read a totally bogus timestamp since drivers
+	 * lacking precise timestamp support rely upon sampling the system clock
+	 * at vblank interrupt time. Which obviously won't work out well if the
+	 * vblank interrupt is disabled.
 	 */
-	if (!vblank->enabled) {
+	if (!vblank->enabled &&
+	    drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0) > 0) {
 		drm_update_vblank_count(dev, crtc);
 		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 		return;
-- 
1.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm: Simplify return value of drm_get_last_vbltimestamp

[Daniel Vetter]

Imo u32 hints at a register value, but in reality all callers only
care whether the sampled timestamp is precise or not. So give them
just a bool.

Also move the declaration out of drmP.h, it's only used in drm_irq.c.

Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_irq.c | 24 +++++++++++++++---------
 include/drm/drmP.h        |  2 --
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 922721ead29a..b16f0bcef959 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -70,6 +70,10 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
 module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600);
 module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
 
+static bool
+drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
+			  struct timeval *tvblank, unsigned flags);
+
 /**
  * drm_update_vblank_count - update the master vblank counter
  * @dev: DRM device
@@ -89,7 +93,8 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
 static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
-	u32 cur_vblank, diff, tslot, rc;
+	u32 cur_vblank, diff, tslot;
+	bool rc;
 	struct timeval t_vblank;
 
 	/*
@@ -147,7 +152,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	unsigned long irqflags;
 	u32 vblcount;
 	s64 diff_ns;
-	int vblrc;
+	bool vblrc;
 	struct timeval tvblank;
 	int count = DRM_TIMESTAMP_MAXRETRIES;
 
@@ -171,7 +176,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * vblank interrupt is disabled.
 	 */
 	if (!vblank->enabled &&
-	    drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0) > 0) {
+	    drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0)) {
 		drm_update_vblank_count(dev, crtc);
 		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 		return;
@@ -219,7 +224,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * available. In that case we can't account for this and just
 	 * hope for the best.
 	 */
-	if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) {
+	if (vblrc && (abs64(diff_ns) > 1000000)) {
 		/* Store new timestamp in ringbuffer. */
 		vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
 
@@ -786,10 +791,11 @@ static struct timeval get_drm_timestamp(void)
  * call, i.e., it isn't very precisely locked to the true vblank.
  *
  * Returns:
- * Non-zero if timestamp is considered to be very precise, zero otherwise.
+ * True if timestamp is considered to be very precise, false otherwise.
  */
-u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
-			      struct timeval *tvblank, unsigned flags)
+static bool
+drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
+			  struct timeval *tvblank, unsigned flags)
 {
 	int ret;
 
@@ -801,7 +807,7 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 		ret = dev->driver->get_vblank_timestamp(dev, crtc, &max_error,
 							tvblank, flags);
 		if (ret > 0)
-			return (u32) ret;
+			return true;
 	}
 
 	/* GPU high precision timestamp query unsupported or failed.
@@ -809,7 +815,7 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
 	 */
 	*tvblank = get_drm_timestamp();
 
-	return 0;
+	return false;
 }
 EXPORT_SYMBOL(drm_get_last_vbltimestamp);
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index ad952b08711e..2ccb0e715569 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1004,8 +1004,6 @@ extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
 extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
 extern void drm_vblank_cleanup(struct drm_device *dev);
 
-extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
-				     struct timeval *tvblank, unsigned flags);
 extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 						 int crtc, int *max_error,
 						 struct timeval *vblank_time,
-- 
1.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm: Clarify vblank ts/scanoutpos sampling #defines

[Daniel Vetter]

I've read INVBL as "invalid backlight" and got mightly confused.
The #defines are already fairly long and we can afford to extend
them a bit more without resulting in ugly code all over.

I'm not sure how useful the complicated bitmask return value of these
functions really are since no one checks them. But for now let's keep
things as is.

Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c                 | 4 ++--
 drivers/gpu/drm/i915/i915_irq.c           | 2 +-
 drivers/gpu/drm/nouveau/nouveau_display.c | 2 +-
 drivers/gpu/drm/radeon/radeon_display.c   | 2 +-
 drivers/gpu/drm/radeon/radeon_pm.c        | 2 +-
 include/drm/drmP.h                        | 4 ++--
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b16f0bcef959..15f7bdebc00c 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -729,7 +729,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 	 * within vblank area, counting down the number of lines until
 	 * start of scanout.
 	 */
-	invbl = vbl_status & DRM_SCANOUTPOS_INVBL;
+	invbl = vbl_status & DRM_SCANOUTPOS_IN_VBLANK;
 
 	/* Convert scanout position into elapsed time at raw_time query
 	 * since start of scanout at first display scanline. delta_ns
@@ -759,7 +759,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 
 	vbl_status = DRM_VBLANKTIME_SCANOUTPOS_METHOD;
 	if (invbl)
-		vbl_status |= DRM_VBLANKTIME_INVBL;
+		vbl_status |= DRM_VBLANKTIME_IN_VBLANK;
 
 	return vbl_status;
 }
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0760b91687a0..998dad9365d5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1020,7 +1020,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 
 	/* In vblank? */
 	if (in_vbl)
-		ret |= DRM_SCANOUTPOS_INVBL;
+		ret |= DRM_SCANOUTPOS_IN_VBLANK;
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index a9ec525c0994..6d0a3cdc752b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -126,7 +126,7 @@ nouveau_display_scanoutpos_head(struct drm_crtc *crtc, int *vpos, int *hpos,
 	if (etime) *etime = ns_to_ktime(args.scan.time[1]);
 
 	if (*vpos < 0)
-		ret |= DRM_SCANOUTPOS_INVBL;
+		ret |= DRM_SCANOUTPOS_IN_VBLANK;
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index bc894c17b2f9..4eb37976f879 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1914,7 +1914,7 @@ int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int fl
 
 	/* In vblank? */
 	if (in_vbl)
-		ret |= DRM_SCANOUTPOS_INVBL;
+		ret |= DRM_SCANOUTPOS_IN_VBLANK;
 
 	/* Is vpos outside nominal vblank area, but less than
 	 * 1/100 of a frame height away from start of vblank?
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 164898b0010c..32522cc940a1 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -1556,7 +1556,7 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev)
 		if (rdev->pm.active_crtcs & (1 << crtc)) {
 			vbl_status = radeon_get_crtc_scanoutpos(rdev->ddev, crtc, 0, &vpos, &hpos, NULL, NULL);
 			if ((vbl_status & DRM_SCANOUTPOS_VALID) &&
-			    !(vbl_status & DRM_SCANOUTPOS_INVBL))
+			    !(vbl_status & DRM_SCANOUTPOS_IN_VBLANK))
 				in_vbl = false;
 		}
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2ccb0e715569..21c88370663e 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -448,11 +448,11 @@ struct drm_master {
 /* Flags and return codes for get_vblank_timestamp() driver function. */
 #define DRM_CALLED_FROM_VBLIRQ 1
 #define DRM_VBLANKTIME_SCANOUTPOS_METHOD (1 << 0)
-#define DRM_VBLANKTIME_INVBL             (1 << 1)
+#define DRM_VBLANKTIME_IN_VBLANK         (1 << 1)
 
 /* get_scanout_position() return flags */
 #define DRM_SCANOUTPOS_VALID        (1 << 0)
-#define DRM_SCANOUTPOS_INVBL        (1 << 1)
+#define DRM_SCANOUTPOS_IN_VBLANK    (1 << 1)
 #define DRM_SCANOUTPOS_ACCURATE     (1 << 2)
 
 /**
-- 
1.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Really never disable vblank irqs for offdelay==0

[Matt Roper]

On Wed, Sep 10, 2014 at 05:36:08PM +0200, Daniel Vetter wrote:
> With the new support for immediate vblank disabling we always disabled
> the vblank interrupt right away, irrespective of the vblank offdelay
> setting.
> 
> But being able to let vblanks run forever is fairly useful for
> debugging, so restore that behaviour.
> 
> Suggested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/drm_irq.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index a75da075927c..6eb015020af2 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1024,9 +1024,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  
>  	/* Last user schedules interrupt disable */
>  	if (atomic_dec_and_test(&vblank->refcount)) {
> -		if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
> +		if (drm_vblank_offdelay == 0)
> +			return;
> +		else if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
>  			vblank_disable_fn((unsigned long)vblank);
> -		else if (drm_vblank_offdelay > 0)
> +		else
>  			mod_timer(&vblank->disable_timer,
>  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
>  	}
> -- 
> 1.9.3
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 3/4] drm: Simplify return value of drm_get_last_vbltimestamp

[Mario Kleiner]

On 09/10/2014 05:36 PM, Daniel Vetter wrote:
> Imo u32 hints at a register value, but in reality all callers only
> care whether the sampled timestamp is precise or not. So give them
> just a bool.
>
> Also move the declaration out of drmP.h, it's only used in drm_irq.c.

All good. Maybe then also remove

EXPORT_SYMBOL(drm_get_last_vbltimestamp);

in this patch if the method is now static to drm_irq.c ? Up to you.

For all 4 patches...

Reviewed-by: Mario Kleiner <mario.kleiner.de@gmail.com>

-mario



> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>   drivers/gpu/drm/drm_irq.c | 24 +++++++++++++++---------
>   include/drm/drmP.h        |  2 --
>   2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 922721ead29a..b16f0bcef959 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -70,6 +70,10 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600);
>   module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600);
>   module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
>   
> +static bool
> +drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
> +			  struct timeval *tvblank, unsigned flags);
> +
>   /**
>    * drm_update_vblank_count - update the master vblank counter
>    * @dev: DRM device
> @@ -89,7 +93,8 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
>   static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>   {
>   	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> -	u32 cur_vblank, diff, tslot, rc;
> +	u32 cur_vblank, diff, tslot;
> +	bool rc;
>   	struct timeval t_vblank;
>   
>   	/*
> @@ -147,7 +152,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>   	unsigned long irqflags;
>   	u32 vblcount;
>   	s64 diff_ns;
> -	int vblrc;
> +	bool vblrc;
>   	struct timeval tvblank;
>   	int count = DRM_TIMESTAMP_MAXRETRIES;
>   
> @@ -171,7 +176,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>   	 * vblank interrupt is disabled.
>   	 */
>   	if (!vblank->enabled &&
> -	    drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0) > 0) {
> +	    drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0)) {
>   		drm_update_vblank_count(dev, crtc);
>   		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>   		return;
> @@ -219,7 +224,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>   	 * available. In that case we can't account for this and just
>   	 * hope for the best.
>   	 */
> -	if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) {
> +	if (vblrc && (abs64(diff_ns) > 1000000)) {
>   		/* Store new timestamp in ringbuffer. */
>   		vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
>   
> @@ -786,10 +791,11 @@ static struct timeval get_drm_timestamp(void)
>    * call, i.e., it isn't very precisely locked to the true vblank.
>    *
>    * Returns:
> - * Non-zero if timestamp is considered to be very precise, zero otherwise.
> + * True if timestamp is considered to be very precise, false otherwise.
>    */
> -u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
> -			      struct timeval *tvblank, unsigned flags)
> +static bool
> +drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
> +			  struct timeval *tvblank, unsigned flags)
>   {
>   	int ret;
>   
> @@ -801,7 +807,7 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
>   		ret = dev->driver->get_vblank_timestamp(dev, crtc, &max_error,
>   							tvblank, flags);
>   		if (ret > 0)
> -			return (u32) ret;
> +			return true;
>   	}
>   
>   	/* GPU high precision timestamp query unsupported or failed.
> @@ -809,7 +815,7 @@ u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
>   	 */
>   	*tvblank = get_drm_timestamp();
>   
> -	return 0;
> +	return false;
>   }
>   EXPORT_SYMBOL(drm_get_last_vbltimestamp);
>   
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index ad952b08711e..2ccb0e715569 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1004,8 +1004,6 @@ extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
>   extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
>   extern void drm_vblank_cleanup(struct drm_device *dev);
>   
> -extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
> -				     struct timeval *tvblank, unsigned flags);
>   extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>   						 int crtc, int *max_error,
>   						 struct timeval *vblank_time,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: Simplify return value of drm_get_last_vbltimestamp

[Daniel Vetter]

On Thu, Sep 11, 2014 at 1:28 PM, Mario Kleiner
<mario.kleiner.de@gmail.com> wrote:
> On 09/10/2014 05:36 PM, Daniel Vetter wrote:
>>
>> Imo u32 hints at a register value, but in reality all callers only
>> care whether the sampled timestamp is precise or not. So give them
>> just a bool.
>>
>> Also move the declaration out of drmP.h, it's only used in drm_irq.c.
>
>
> All good. Maybe then also remove
>
> EXPORT_SYMBOL(drm_get_last_vbltimestamp);

Oh, I've missed that one when grepping. Will fix when applying.

> in this patch if the method is now static to drm_irq.c ? Up to you.
>
> For all 4 patches...
>
> Reviewed-by: Mario Kleiner <mario.kleiner.de@gmail.com>

Thanks a lot, I plan to send the pull request to Dave tomorrow.
Presuming nothing else fails meanwhile ;-)

Re: [PATCH 2/4] drm: Only update final vblank count when precise ts is available

[Matt Roper]

On Wed, Sep 10, 2014 at 05:36:09PM +0200, Daniel Vetter wrote:
> Drivers without a hardware vblank counter simply can't account for the
> vblanks that happened while the vblank interrupt was off. To check
> this grab a vblank timestamp and if the result is dubious follow the
> normal save-and-disable logic.
> 
> Drivers should prevent this by setting vblank_disable_allowed = false,
> but since running vblank interrupts constantly is not good for power
> consumption most drivers lie. Testing for precise vblank timestamps is
> the next best thing we can check for.
> 
> Suggested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/drm_irq.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 6eb015020af2..922721ead29a 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -163,8 +163,15 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	 * has been ticking all along until this time. This makes the
>  	 * count account for the entire time between drm_vblank_on() and
>  	 * drm_vblank_off().
> +	 *
> +	 * But only do this if precise vblank timestamps are available.
> +	 * Otherwise we might read a totally bogus timestamp since drivers
> +	 * lacking precise timestamp support rely upon sampling the system clock
> +	 * at vblank interrupt time. Which obviously won't work out well if the
> +	 * vblank interrupt is disabled.
>  	 */
> -	if (!vblank->enabled) {
> +	if (!vblank->enabled &&
> +	    drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0) > 0) {
>  		drm_update_vblank_count(dev, crtc);
>  		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  		return;
> -- 
> 1.9.3
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795