On 29/10/13 19:06, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrj?l?
>
> 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 at gmail.com
> Signed-off-by: Ville Syrj?l?
> ---
> 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