Re: [PATCH 13/14] drm/radeon: Move the early vblank IRQ fixup to radeon_get_crtc_scanoutpos()

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

On 29/10/13 19:06, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> i915 doesn't need this kludge for most platforms. Although we do
> appear to need something similar on certain platforms, but we can
> be more accurate when we apply the adjustment since we know exactly
> why the scanline counter doesn't always quite match the vblank
> status.
>
> Also the current code doesn't handle interlaced modes correctly,
> and we already deal with interlaced modes in i915 code.
>
> So let's just move the current code to radeon_get_crtc_scanoutpos()
> since that's why it was added. For i915 we'll add a more finely
> targeted variant.
>

The logic itself looks correct and should work, although i couldn't test 
it because of the dying PC.

But see below for some bugfix and some little nit-pick.

Other than that

Reviewed-by: mario.kleiner.de@gmail.com


> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/drm_irq.c               | 25 ++-----------------------
>   drivers/gpu/drm/radeon/radeon_display.c | 22 ++++++++++++++++++++++
>   2 files changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index b39255f..a1cc1a3 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -542,7 +542,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   {
>   	ktime_t stime, etime, mono_time_offset;
>   	struct timeval tv_etime;
> -	int vbl_status, vtotal, vdisplay;
> +	int vbl_status;
>   	int vpos, hpos, i;
>   	int framedur_ns, linedur_ns, pixeldur_ns, delta_ns, duration_ns;
>   	bool invbl;
> @@ -558,9 +558,6 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   		return -EIO;
>   	}
>
> -	vtotal = mode->crtc_vtotal;
> -	vdisplay = mode->crtc_vdisplay;
> -
>   	/* Durations of frames, lines, pixels in nanoseconds. */
>   	framedur_ns = refcrtc->framedur_ns;
>   	linedur_ns  = refcrtc->linedur_ns;
> @@ -569,7 +566,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   	/* If mode timing undefined, just return as no-op:
>   	 * Happens during initial modesetting of a crtc.
>   	 */
> -	if (vtotal <= 0 || vdisplay <= 0 || framedur_ns == 0) {
> +	if (framedur_ns == 0) {
>   		DRM_DEBUG("crtc %d: Noop due to uninitialized mode.\n", crtc);
>   		return -EAGAIN;
>   	}
> @@ -631,24 +628,6 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   	 */
>   	delta_ns = vpos * linedur_ns + hpos * pixeldur_ns;
>
> -	/* Is vpos outside nominal vblank area, but less than
> -	 * 1/100 of a frame height away from start of vblank?
> -	 * If so, assume this isn't a massively delayed vblank
> -	 * interrupt, but a vblank interrupt that fired a few
> -	 * microseconds before true start of vblank. Compensate
> -	 * by adding a full frame duration to the final timestamp.
> -	 * Happens, e.g., on ATI R500, R600.
> -	 *
> -	 * We only do this if DRM_CALLED_FROM_VBLIRQ.
> -	 */
> -	if ((flags & DRM_CALLED_FROM_VBLIRQ) && !invbl &&
> -	    ((vdisplay - vpos) < vtotal / 100)) {
> -		delta_ns = delta_ns - framedur_ns;
> -
> -		/* Signal this correction as "applied". */
> -		vbl_status |= 0x8;
> -	}
> -
>   	if (!drm_timestamp_monotonic)
>   		etime = ktime_sub(etime, mono_time_offset);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 3581570..9d02fa7 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -1709,5 +1709,27 @@ int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int fl
>   	if (in_vbl)
>   		ret |= DRM_SCANOUTPOS_INVBL;
>
> +	/* Is vpos outside nominal vblank area, but less than
> +	 * 1/100 of a frame height away from start of vblank?
> +	 * If so, assume this isn't a massively delayed vblank
> +	 * interrupt, but a vblank interrupt that fired a few
> +	 * microseconds before true start of vblank. Compensate
> +	 * by adding a full frame duration to the final timestamp.
> +	 * Happens, e.g., on ATI R500, R600.
> +	 *
> +	 * We only do this if DRM_CALLED_FROM_VBLIRQ.
> +	 */
> +	if ((flags & DRM_CALLED_FROM_VBLIRQ) && !in_vbl) {
> +		vbl_start = rdev->mode_info.crtcs[crtc]->base.hwmode.crtc_vdisplay;

vbl_start gets already initialized by the code above, so the vbl_start 
assignment here shouldn't be neccessary. Only the vtotal assignment 
below is really needed.

> +		vtotal = rdev->mode_info.crtcs[crtc]->base.hwmode.crtc_vtotal;
> +
> +		if (vbl_start - *vpos < vtotal / 100) {
> +			vpos -= vtotal;

Here vpos is an int*, so the following line will corrupt kernel memory 
and die. Obviously then this

 > +			*vpos -= vtotal;

instead.

> +
> +			/* Signal this correction as "applied". */
> +			ret |= 0x8;

ret |= 0x8;

This is not really defined as return flag for the 
driver->get_scanout_position() hook, but would work.

The value was only used in its original function for diagnostic 
DRM_DEBUG output in the DRM core at higher drm debug levels, but has 
some use for debugging.

Now that it is also used in kms drivers, maybe define it properly as

#define DRM_SCANOUTPOS_NUDGED     (1 << 3)

or something like that in include/drm/drmP.h to mark value 0x8 as used? 
Or drop the 0x8 return code and add some DRM_DEBUG statement instead?

Same for patch 14 in the i915 driver.

thanks,
-mario

> +		}
> +	}
> +
>   	return ret;
>   }
>

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