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

[Mario Kleiner <mario.kleiner.de@gmail.com>]

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