[PATCH 00/14] drm: Some more vblank timestampi changes

[PATCH 00/14] drm: Some more vblank timestampi changes

[ville.syrjala]

So I took another look at the vblank timestamping code, and got a bit
excited. The result is this patchset.

Summary of changes:
- kill crtc->hwmode dependency
- eliminate a bunch of 64bit math
- fix timestamps for stereo and interlaced modes (on i915 at least)
- move the "early vbl irq" hack into radeon code
- add a similar hack to i915, but make it as finely targeted
  as possibly to minimize the chance of accidentally
  applying it in the wrong place

The s/clock/crtc_clock change could use some radeon people to verify
whether changing radeon_atom_get_tv_timings() is enough to make
crtc_clock always populated.

This series applies on top of Mario's
"Vblank timestamping improvements/fixes for Linux drm." series.

Ville Syrjälä (14):
      drm: Pass the display mode to drm_calc_timestamping_constants()
      drm: Pass the display mode to drm_calc_vbltimestamp_from_scanoutpos()
      drm/i915: Kill hwmode save/restore
      drm/i915: Call drm_calc_timestamping_constants() earlier
      drm: Improve drm_calc_timestamping_constants() documentation
      drm: Simplify the math in drm_calc_timestamping_constants()
      drm/radeon: Populate crtc_clock in radeon_atom_get_tv_timings()
      drm: Use crtc_clock in drm_calc_timestamping_constants()
      drm: Change {pixel,line,frame}dur_ns from s64 to int
      drm/i915: Fix scanoutpos calculations for interlaced modes
      drm: Fix vblank timestamping constants for interlaced modes
      drm: Pass 'flags' from the caller to .get_scanout_position()
      drm/radeon: Move the early vblank IRQ fixup to radeon_get_crtc_scanoutpos()
      drm/i915: Add a kludge for DSL incrementing too late and ISR not working

 drivers/gpu/drm/drm_crtc_helper.c        |   2 +-
 drivers/gpu/drm/drm_irq.c                | 109 ++++++++++++-------------------
 drivers/gpu/drm/i915/i915_irq.c          |  91 ++++++++++++++------------
 drivers/gpu/drm/i915/intel_display.c     |  29 ++++----
 drivers/gpu/drm/radeon/radeon_atombios.c |   6 +-
 drivers/gpu/drm/radeon/radeon_display.c  |  29 +++++++-
 drivers/gpu/drm/radeon/radeon_kms.c      |   2 +-
 drivers/gpu/drm/radeon/radeon_mode.h     |   1 +
 drivers/gpu/drm/radeon/radeon_pm.c       |   2 +-
 include/drm/drmP.h                       |   8 ++-
 include/drm/drm_crtc.h                   |   2 +-
 11 files changed, 143 insertions(+), 138 deletions(-)

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

[PATCH 01/14] drm: Pass the display mode to drm_calc_timestamping_constants()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't really use hwmode anymore in i915, so eliminating its use
from the core code seems prudent. Just pass the appropriate mode
to drm_calc_timestamping_constants().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc_helper.c    |  2 +-
 drivers/gpu/drm/drm_irq.c            | 17 +++++++++--------
 drivers/gpu/drm/i915/intel_display.c |  3 ++-
 include/drm/drmP.h                   |  3 ++-
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index dbcd687..fce1cff 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -533,7 +533,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 	 * are later needed by vblank and swap-completion
 	 * timestamping. They are derived from true hwmode.
 	 */
-	drm_calc_timestamping_constants(crtc);
+	drm_calc_timestamping_constants(crtc, &crtc->hwmode);
 
 	/* FIXME: add subpixel order */
 done:
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 2250724..679417d 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -445,20 +445,22 @@ int drm_control(struct drm_device *dev, void *data,
  * adjustments into account.
  *
  * @crtc drm_crtc whose timestamp constants should be updated.
+ * @mode display mode containing the scanout timings
  *
  */
-void drm_calc_timestamping_constants(struct drm_crtc *crtc)
+void drm_calc_timestamping_constants(struct drm_crtc *crtc,
+				     const struct drm_display_mode *mode)
 {
 	s64 linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0;
 	u64 dotclock;
 
 	/* Dot clock in Hz: */
-	dotclock = (u64) crtc->hwmode.clock * 1000;
+	dotclock = (u64) mode->clock * 1000;
 
 	/* Fields of interlaced scanout modes are only halve a frame duration.
 	 * Double the dotclock to get halve the frame-/line-/pixelduration.
 	 */
-	if (crtc->hwmode.flags & DRM_MODE_FLAG_INTERLACE)
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		dotclock *= 2;
 
 	/* Valid dotclock? */
@@ -469,10 +471,9 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc)
 		 * nanoseconds:
 		 */
 		pixeldur_ns = (s64) div64_u64(1000000000, dotclock);
-		linedur_ns  = (s64) div64_u64(((u64) crtc->hwmode.crtc_htotal *
+		linedur_ns  = (s64) div64_u64(((u64) mode->crtc_htotal *
 					      1000000000), dotclock);
-		frame_size = crtc->hwmode.crtc_htotal *
-				crtc->hwmode.crtc_vtotal;
+		frame_size = mode->crtc_htotal * mode->crtc_vtotal;
 		framedur_ns = (s64) div64_u64((u64) frame_size * 1000000000,
 					      dotclock);
 	} else
@@ -484,8 +485,8 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc)
 	crtc->framedur_ns = framedur_ns;
 
 	DRM_DEBUG("crtc %d: hwmode: htotal %d, vtotal %d, vdisplay %d\n",
-		  crtc->base.id, crtc->hwmode.crtc_htotal,
-		  crtc->hwmode.crtc_vtotal, crtc->hwmode.crtc_vdisplay);
+		  crtc->base.id, mode->crtc_htotal,
+		  mode->crtc_vtotal, mode->crtc_vdisplay);
 	DRM_DEBUG("crtc %d: clock %d kHz framedur %d linedur %d, pixeldur %d\n",
 		  crtc->base.id, (int) dotclock/1000, (int) framedur_ns,
 		  (int) linedur_ns, (int) pixeldur_ns);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0c2e83c..7ea20b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9364,7 +9364,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		 * are later needed by vblank and swap-completion
 		 * timestamping. They are derived from true hwmode.
 		 */
-		drm_calc_timestamping_constants(crtc);
+		drm_calc_timestamping_constants(crtc,
+						&pipe_config->adjusted_mode);
 	}
 
 	/* FIXME: add subpixel order */
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 8d4e10d..99f49ea 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1419,7 +1419,8 @@ extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 						 struct timeval *vblank_time,
 						 unsigned flags,
 						 struct drm_crtc *refcrtc);
-extern void drm_calc_timestamping_constants(struct drm_crtc *crtc);
+extern void drm_calc_timestamping_constants(struct drm_crtc *crtc,
+					    const struct drm_display_mode *mode);
 
 extern bool
 drm_mode_parse_command_line_for_connector(const char *mode_option,
-- 
1.8.1.5

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

[PATCH 02/14] drm: Pass the display mode to drm_calc_vbltimestamp_from_scanoutpos()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Rather than using crtc->hwmode, just pass the relevant mode to
drm_calc_vbltimestamp_from_scanoutpos(). This removes the last hwmode
usage from core drm.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c           | 6 +++---
 drivers/gpu/drm/i915/i915_irq.c     | 3 ++-
 drivers/gpu/drm/radeon/radeon_kms.c | 2 +-
 include/drm/drmP.h                  | 3 ++-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 679417d..7702614 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -522,6 +522,7 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants);
  *         0 = Default.
  *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler.
  * @refcrtc: drm_crtc* of crtc which defines scanout timing.
+ * @mode: mode which defines the scanout timings
  *
  * Returns negative value on error, failure or if not supported in current
  * video mode:
@@ -541,11 +542,11 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 					  int *max_error,
 					  struct timeval *vblank_time,
 					  unsigned flags,
-					  struct drm_crtc *refcrtc)
+					  const struct drm_crtc *refcrtc,
+					  const struct drm_display_mode *mode)
 {
 	ktime_t stime, etime, mono_time_offset;
 	struct timeval tv_etime;
-	struct drm_display_mode *mode;
 	int vbl_status, vtotal, vdisplay;
 	int vpos, hpos, i;
 	s64 framedur_ns, linedur_ns, pixeldur_ns, delta_ns, duration_ns;
@@ -562,7 +563,6 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 		return -EIO;
 	}
 
-	mode = &refcrtc->hwmode;
 	vtotal = mode->crtc_vtotal;
 	vdisplay = mode->crtc_vdisplay;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5a60d89..2b19989 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -790,7 +790,8 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
 	/* Helper routine in DRM core does all the work: */
 	return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error,
 						     vblank_time, flags,
-						     crtc);
+						     crtc,
+						     &to_intel_crtc(crtc)->config.adjusted_mode);
 }
 
 static bool intel_hpd_irq_event(struct drm_device *dev,
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index d6b3676..80896a1 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -670,7 +670,7 @@ int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc,
 	/* Helper routine in DRM core does all the work: */
 	return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc, max_error,
 						     vblank_time, flags,
-						     drmcrtc);
+						     drmcrtc, &drmcrtc->hwmode);
 }
 
 #define KMS_INVALID_IOCTL(name)						\
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 99f49ea..14d4046 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1418,7 +1418,8 @@ extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
 						 int crtc, int *max_error,
 						 struct timeval *vblank_time,
 						 unsigned flags,
-						 struct drm_crtc *refcrtc);
+						 const struct drm_crtc *refcrtc,
+						 const struct drm_display_mode *mode);
 extern void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 					    const struct drm_display_mode *mode);
 
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/14] drm/i915: Kill hwmode save/restore

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm core no longer uses crtc->hwmode, and neither does i915, so we can totally ignore it
in i915.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7ea20b2..2b35b29 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9283,21 +9283,19 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct drm_display_mode *saved_mode, *saved_hwmode;
+	struct drm_display_mode *saved_mode;
 	struct intel_crtc_config *pipe_config = NULL;
 	struct intel_crtc *intel_crtc;
 	unsigned disable_pipes, prepare_pipes, modeset_pipes;
 	int ret = 0;
 
-	saved_mode = kcalloc(2, sizeof(*saved_mode), GFP_KERNEL);
+	saved_mode = kmalloc(sizeof(*saved_mode), GFP_KERNEL);
 	if (!saved_mode)
 		return -ENOMEM;
-	saved_hwmode = saved_mode + 1;
 
 	intel_modeset_affected_pipes(crtc, &modeset_pipes,
 				     &prepare_pipes, &disable_pipes);
 
-	*saved_hwmode = crtc->hwmode;
 	*saved_mode = crtc->mode;
 
 	/* Hack: Because we don't (yet) support global modeset on multiple
@@ -9357,9 +9355,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		dev_priv->display.crtc_enable(&intel_crtc->base);
 
 	if (modeset_pipes) {
-		/* Store real post-adjustment hardware mode. */
-		crtc->hwmode = pipe_config->adjusted_mode;
-
 		/* Calculate and store various constants which
 		 * are later needed by vblank and swap-completion
 		 * timestamping. They are derived from true hwmode.
@@ -9370,10 +9365,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 
 	/* FIXME: add subpixel order */
 done:
-	if (ret && crtc->enabled) {
-		crtc->hwmode = *saved_hwmode;
+	if (ret && crtc->enabled)
 		crtc->mode = *saved_mode;
-	}
 
 out:
 	kfree(pipe_config);
-- 
1.8.1.5

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

[PATCH 04/14] drm/i915: Call drm_calc_timestamping_constants() earlier

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Update the pixel/line/frame duration information when we switch to the
new pipe config. This will keep the timestamping constants in better
sync with the real hardware state.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2b35b29..1b8ed0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9331,6 +9331,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 		/* mode_set/enable/disable functions rely on a correct pipe
 		 * config. */
 		to_intel_crtc(crtc)->config = *pipe_config;
+
+		/*
+		 * Calculate and store various constants which
+		 * are later needed by vblank and swap-completion
+		 * timestamping. They are derived from true hwmode.
+		 */
+		drm_calc_timestamping_constants(crtc,
+						&pipe_config->adjusted_mode);
 	}
 
 	/* Only after disabling all output pipelines that will be changed can we
@@ -9354,15 +9362,6 @@ static int __intel_set_mode(struct drm_crtc *crtc,
 	for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc)
 		dev_priv->display.crtc_enable(&intel_crtc->base);
 
-	if (modeset_pipes) {
-		/* Calculate and store various constants which
-		 * are later needed by vblank and swap-completion
-		 * timestamping. They are derived from true hwmode.
-		 */
-		drm_calc_timestamping_constants(crtc,
-						&pipe_config->adjusted_mode);
-	}
-
 	/* FIXME: add subpixel order */
 done:
 	if (ret && crtc->enabled)
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/14] drm: Improve drm_calc_timestamping_constants() documentation

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the long blurp to into the body of the comment, leaving only
a short summary line at the top.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 7702614..48bf91f 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -436,17 +436,16 @@ int drm_control(struct drm_device *dev, void *data,
 }
 
 /**
- * drm_calc_timestamping_constants - Calculate and
- * store various constants which are later needed by
- * vblank and swap-completion timestamping, e.g, by
- * drm_calc_vbltimestamp_from_scanoutpos().
- * They are derived from crtc's true scanout timing,
- * so they take things like panel scaling or other
- * adjustments into account.
+ * drm_calc_timestamping_constants - Calculate vblank timestamp constants
  *
  * @crtc drm_crtc whose timestamp constants should be updated.
  * @mode display mode containing the scanout timings
  *
+ * Calculate and store various constants which are later
+ * needed by vblank and swap-completion timestamping, e.g,
+ * by drm_calc_vbltimestamp_from_scanoutpos(). They are
+ * derived from crtc's true scanout timing, so they take
+ * things like panel scaling or other adjustments into account.
  */
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode)
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/14] drm: Simplify the math in drm_calc_timestamping_constants()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_calc_timestamping_constants() makes the math more complex
than necessary.
- multipying the dotclock by 1000 is pointless, just makes all the
  numbers bigger
- div64_u64() is also pointless, div_u64 is enough
- pixeldur_ns doesn't need any 64bit math

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 48bf91f..d6ef034 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -451,10 +451,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode)
 {
 	s64 linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0;
-	u64 dotclock;
-
-	/* Dot clock in Hz: */
-	dotclock = (u64) mode->clock * 1000;
+	int dotclock = mode->clock;
 
 	/* Fields of interlaced scanout modes are only halve a frame duration.
 	 * Double the dotclock to get halve the frame-/line-/pixelduration.
@@ -464,17 +461,16 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 
 	/* Valid dotclock? */
 	if (dotclock > 0) {
-		int frame_size;
-		/* Convert scanline length in pixels and video dot clock to
-		 * line duration, frame duration and pixel duration in
-		 * nanoseconds:
+		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
+
+		/*
+		 * Convert scanline length in pixels and video
+		 * dot clock to line duration, frame duration
+		 * and pixel duration in nanoseconds:
 		 */
-		pixeldur_ns = (s64) div64_u64(1000000000, dotclock);
-		linedur_ns  = (s64) div64_u64(((u64) mode->crtc_htotal *
-					      1000000000), dotclock);
-		frame_size = mode->crtc_htotal * mode->crtc_vtotal;
-		framedur_ns = (s64) div64_u64((u64) frame_size * 1000000000,
-					      dotclock);
+		pixeldur_ns = 1000000 / dotclock;
+		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
+		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
 	} else
 		DRM_ERROR("crtc %d: Can't calculate constants, dotclock = 0!\n",
 			  crtc->base.id);
@@ -487,7 +483,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 		  crtc->base.id, mode->crtc_htotal,
 		  mode->crtc_vtotal, mode->crtc_vdisplay);
 	DRM_DEBUG("crtc %d: clock %d kHz framedur %d linedur %d, pixeldur %d\n",
-		  crtc->base.id, (int) dotclock/1000, (int) framedur_ns,
+		  crtc->base.id, dotclock, (int) framedur_ns,
 		  (int) linedur_ns, (int) pixeldur_ns);
 }
 EXPORT_SYMBOL(drm_calc_timestamping_constants);
-- 
1.8.1.5

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

[PATCH 07/14] drm/radeon: Populate crtc_clock in radeon_atom_get_tv_timings()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

crtc_clock is now supposed to be the actual pixel clock corresponding to
the other crtc_ timing values. Populate crtc_clock appropriately in
radeon_atom_get_tv_timings().

This was the only obvious place where we frob with the crtc_ timigns
directly instead of calling drm_mode_set_crtcinfo() which would also
update crtc_clock.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/radeon/radeon_atombios.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c
index f79ee18..229ef65 100644
--- a/drivers/gpu/drm/radeon/radeon_atombios.c
+++ b/drivers/gpu/drm/radeon/radeon_atombios.c
@@ -1809,7 +1809,8 @@ bool radeon_atom_get_tv_timings(struct radeon_device *rdev, int index,
 		if (misc & ATOM_DOUBLE_CLOCK_MODE)
 			mode->flags |= DRM_MODE_FLAG_DBLSCAN;
 
-		mode->clock = le16_to_cpu(tv_info->aModeTimings[index].usPixelClock) * 10;
+		mode->crtc_clock = mode->clock =
+			le16_to_cpu(tv_info->aModeTimings[index].usPixelClock) * 10;
 
 		if (index == 1) {
 			/* PAL timings appear to have wrong values for totals */
@@ -1852,7 +1853,8 @@ bool radeon_atom_get_tv_timings(struct radeon_device *rdev, int index,
 		if (misc & ATOM_DOUBLE_CLOCK_MODE)
 			mode->flags |= DRM_MODE_FLAG_DBLSCAN;
 
-		mode->clock = le16_to_cpu(dtd_timings->usPixClk) * 10;
+		mode->crtc_clock = mode->clock =
+			le16_to_cpu(dtd_timings->usPixClk) * 10;
 		break;
 	}
 	return true;
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/14] drm: Use crtc_clock in drm_calc_timestamping_constants()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm_calc_timestamping_constants() computes the pixel/line/frame
durations based on the crtc_ timing values. The corresponding pixel
clock is in mode->crtc_clock, so we need to use that instead of
mode->clock.

This should fix drm_calc_timestamping_constants() for frame packing
stereo modes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index d6ef034..77be2fb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -451,7 +451,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode)
 {
 	s64 linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0;
-	int dotclock = mode->clock;
+	int dotclock = mode->crtc_clock;
 
 	/* Fields of interlaced scanout modes are only halve a frame duration.
 	 * Double the dotclock to get halve the frame-/line-/pixelduration.
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/14] drm: Change {pixel,line,frame}dur_ns from s64 to int

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Using s64 for the timestamping constants is wasteful. Signed 32bit
integers get us a range of over +-2 seconds. Presuming that no-one
wants to a vrefresh rate less than 0.5, we can switch to using int
for the timestamping constants. We save a few bytes in drm_crtc and
avoid a bunch of 64bit math.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 18 +++++++++---------
 include/drm/drm_crtc.h    |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 77be2fb..b21a226 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -450,7 +450,7 @@ int drm_control(struct drm_device *dev, void *data,
 void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 				     const struct drm_display_mode *mode)
 {
-	s64 linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0;
+	int linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0;
 	int dotclock = mode->crtc_clock;
 
 	/* Fields of interlaced scanout modes are only halve a frame duration.
@@ -483,8 +483,8 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 		  crtc->base.id, mode->crtc_htotal,
 		  mode->crtc_vtotal, mode->crtc_vdisplay);
 	DRM_DEBUG("crtc %d: clock %d kHz framedur %d linedur %d, pixeldur %d\n",
-		  crtc->base.id, dotclock, (int) framedur_ns,
-		  (int) linedur_ns, (int) pixeldur_ns);
+		  crtc->base.id, dotclock, framedur_ns,
+		  linedur_ns, pixeldur_ns);
 }
 EXPORT_SYMBOL(drm_calc_timestamping_constants);
 
@@ -544,7 +544,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 	struct timeval tv_etime;
 	int vbl_status, vtotal, vdisplay;
 	int vpos, hpos, i;
-	s64 framedur_ns, linedur_ns, pixeldur_ns, delta_ns, duration_ns;
+	int framedur_ns, linedur_ns, pixeldur_ns, delta_ns, duration_ns;
 	bool invbl;
 
 	if (crtc < 0 || crtc >= dev->num_crtcs) {
@@ -605,18 +605,18 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 		duration_ns = ktime_to_ns(etime) - ktime_to_ns(stime);
 
 		/* Accept result with <  max_error nsecs timing uncertainty. */
-		if (duration_ns <= (s64) *max_error)
+		if (duration_ns <= *max_error)
 			break;
 	}
 
 	/* Noisy system timing? */
 	if (i == DRM_TIMESTAMP_MAXRETRIES) {
 		DRM_DEBUG("crtc %d: Noisy timestamp %d us > %d us [%d reps].\n",
-			  crtc, (int) duration_ns/1000, *max_error/1000, i);
+			  crtc, duration_ns/1000, *max_error/1000, i);
 	}
 
 	/* Return upper bound of timestamp precision error. */
-	*max_error = (int) duration_ns;
+	*max_error = duration_ns;
 
 	/* Check if in vblank area:
 	 * vpos is >=0 in video scanout area, but negative
@@ -629,7 +629,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 	 * since start of scanout at first display scanline. delta_ns
 	 * can be negative if start of scanout hasn't happened yet.
 	 */
-	delta_ns = (s64) vpos * linedur_ns + (s64) hpos * pixeldur_ns;
+	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?
@@ -667,7 +667,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 		  crtc, (int)vbl_status, hpos, vpos,
 		  (long)tv_etime.tv_sec, (long)tv_etime.tv_usec,
 		  (long)vblank_time->tv_sec, (long)vblank_time->tv_usec,
-		  (int)duration_ns/1000, i);
+		  duration_ns/1000, i);
 
 	vbl_status = DRM_VBLANKTIME_SCANOUTPOS_METHOD;
 	if (invbl)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index d3a91ad..d94c25f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -447,7 +447,7 @@ struct drm_crtc {
 	uint16_t *gamma_store;
 
 	/* Constants needed for precise vblank and swap timestamping. */
-	s64 framedur_ns, linedur_ns, pixeldur_ns;
+	int framedur_ns, linedur_ns, pixeldur_ns;
 
 	/* if you are using the helper */
 	void *helper_private;
-- 
1.8.1.5

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

[PATCH 10/14] drm/i915: Fix scanoutpos calculations for interlaced modes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The scanline counter counts lines in the current field, not the entire
frame. But the crtc_ timings are the values for the entire frame. Divide
the vertical timings by 2 to make them match the scanline counter.

The rounding was carefully chosen to make it do the right thing wrt. the
observed scanline counter and ISR vblank bit behaviour.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2b19989..f6b3206 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -680,6 +680,12 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 	vbl_start = mode->crtc_vblank_start;
 	vbl_end = mode->crtc_vblank_end;
 
+	if (mode->flags & DRM_MODE_FLAG_INTERLACE) {
+		vbl_start = DIV_ROUND_UP(vbl_start, 2);
+		vbl_end /= 2;
+		vtotal /= 2;
+	}
+
 	ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE;
 
 	/* Lock uncore.lock, as we will do multiple timing critical raw
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/14] drm: Fix vblank timestamping constants for interlaced modes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We're currently miscalculating the line and pixel durations for
interlaced modes. crtc_htotal and crtc_vtotal are the full frame
timings, and so is crtc_clock, so we can compute the line
and pixel durations from those w/o any extra adjustments. But
we actually want framedur_ns to be the field, not frame, duration,
so we must divide it by two.

This should make the scanout based vblank timestamp corrections
work correctly with interlaced modes, at least for i915. It all
depends whether we keep the field or frame timings in the display
mode crtc_ timings.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b21a226..b5c4d42 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -453,12 +453,6 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 	int linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0;
 	int dotclock = mode->crtc_clock;
 
-	/* Fields of interlaced scanout modes are only halve a frame duration.
-	 * Double the dotclock to get halve the frame-/line-/pixelduration.
-	 */
-	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
-		dotclock *= 2;
-
 	/* Valid dotclock? */
 	if (dotclock > 0) {
 		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
@@ -471,6 +465,12 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
 		pixeldur_ns = 1000000 / dotclock;
 		linedur_ns  = div_u64((u64) mode->crtc_htotal * 1000000, dotclock);
 		framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
+
+		/*
+		 * Fields of interlaced scanout modes are only halve a frame duration.
+		 */
+		if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+			framedur_ns /= 2;
 	} else
 		DRM_ERROR("crtc %d: Can't calculate constants, dotclock = 0!\n",
 			  crtc->base.id);
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/14] drm: Pass 'flags' from the caller to .get_scanout_position()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Preparation for moving the early vblank IRQ logic into
radeon_get_crtc_scanoutpos().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c               | 2 +-
 drivers/gpu/drm/i915/i915_irq.c         | 3 ++-
 drivers/gpu/drm/radeon/radeon_display.c | 7 ++++---
 drivers/gpu/drm/radeon/radeon_mode.h    | 1 +
 drivers/gpu/drm/radeon/radeon_pm.c      | 2 +-
 include/drm/drmP.h                      | 2 ++
 6 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b5c4d42..b39255f 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -585,7 +585,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
 		/* Get vertical and horizontal scanout position vpos, hpos,
 		 * and bounding timestamps stime, etime, pre/post query.
 		 */
-		vbl_status = dev->driver->get_scanout_position(dev, crtc, &vpos,
+		vbl_status = dev->driver->get_scanout_position(dev, crtc, flags, &vpos,
 							       &hpos, &stime, &etime);
 
 		/* Get correction for CLOCK_MONOTONIC -> CLOCK_REALTIME if
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f6b3206..70daf3c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -657,7 +657,8 @@ static bool intel_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
-			     int *vpos, int *hpos, ktime_t *stime, ktime_t *etime)
+				    unsigned int flags, int *vpos, int *hpos,
+				    ktime_t *stime, ktime_t *etime)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index ccd8751..3581570 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -305,7 +305,7 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
 	 * to complete in this vblank?
 	 */
 	if (update_pending &&
-	    (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id,
+	    (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id, 0,
 							       &vpos, &hpos, NULL, NULL)) &&
 	    ((vpos >= (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100) ||
 	     (vpos < 0 && !ASIC_IS_AVIVO(rdev)))) {
@@ -1544,6 +1544,7 @@ bool radeon_crtc_scaling_mode_fixup(struct drm_crtc *crtc,
  *
  * \param dev Device to query.
  * \param crtc Crtc to query.
+ * \param flags Flags from caller (DRM_CALLED_FROM_VBLIRQ or 0).
  * \param *vpos Location where vertical scanout position should be stored.
  * \param *hpos Location where horizontal scanout position should go.
  * \param *stime Target location for timestamp taken immediately before
@@ -1565,8 +1566,8 @@ bool radeon_crtc_scaling_mode_fixup(struct drm_crtc *crtc,
  * unknown small number of scanlines wrt. real scanout position.
  *
  */
-int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, int *vpos, int *hpos,
-			       ktime_t *stime, ktime_t *etime)
+int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int flags,
+			       int *vpos, int *hpos, ktime_t *stime, ktime_t *etime)
 {
 	u32 stat_crtc = 0, vbl = 0, position = 0;
 	int vbl_start, vbl_end, vtotal, ret = 0;
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index 3bfa910..c4016dc 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -758,6 +758,7 @@ extern int radeon_crtc_cursor_move(struct drm_crtc *crtc,
 				   int x, int y);
 
 extern int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc,
+				      unsigned int flags,
 				      int *vpos, int *hpos, ktime_t *stime,
 				      ktime_t *etime);
 
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index 98bf63b..a394049 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -1468,7 +1468,7 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev)
 	 */
 	for (crtc = 0; (crtc < rdev->num_crtc) && in_vbl; crtc++) {
 		if (rdev->pm.active_crtcs & (1 << crtc)) {
-			vbl_status = radeon_get_crtc_scanoutpos(rdev->ddev, crtc, &vpos, &hpos, NULL, NULL);
+			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))
 				in_vbl = false;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 14d4046..13c7942 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -840,6 +840,7 @@ struct drm_driver {
 	 *
 	 * \param dev  DRM device.
 	 * \param crtc Id of the crtc to query.
+	 * \param flags Flags from the caller (DRM_CALLED_FROM_VBLIRQ or 0).
 	 * \param *vpos Target location for current vertical scanout position.
 	 * \param *hpos Target location for current horizontal scanout position.
 	 * \param *stime Target location for timestamp taken immediately before
@@ -862,6 +863,7 @@ struct drm_driver {
 	 *
 	 */
 	int (*get_scanout_position) (struct drm_device *dev, int crtc,
+				     unsigned int flags,
 				     int *vpos, int *hpos, ktime_t *stime,
 				     ktime_t *etime);
 
-- 
1.8.1.5

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

Re: [PATCH 12/14] drm: Pass 'flags' from the caller to .get_scanout_position()

[Mario Kleiner]

On 29/10/13 19:06, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Preparation for moving the early vblank IRQ logic into
> radeon_get_crtc_scanoutpos().
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Tiny compile fix needed for this one. The function prototype for 
radeon_get_crtc_scanoutpos() is also defined in radeon_drv.c, so it 
needs the same update as the one in radeon_mode.h

Other than that

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

-mario


> ---
>   drivers/gpu/drm/drm_irq.c               | 2 +-
>   drivers/gpu/drm/i915/i915_irq.c         | 3 ++-
>   drivers/gpu/drm/radeon/radeon_display.c | 7 ++++---
>   drivers/gpu/drm/radeon/radeon_mode.h    | 1 +
>   drivers/gpu/drm/radeon/radeon_pm.c      | 2 +-
>   include/drm/drmP.h                      | 2 ++
>   6 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index b5c4d42..b39255f 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -585,7 +585,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int crtc,
>   		/* Get vertical and horizontal scanout position vpos, hpos,
>   		 * and bounding timestamps stime, etime, pre/post query.
>   		 */
> -		vbl_status = dev->driver->get_scanout_position(dev, crtc, &vpos,
> +		vbl_status = dev->driver->get_scanout_position(dev, crtc, flags, &vpos,
>   							       &hpos, &stime, &etime);
>
>   		/* Get correction for CLOCK_MONOTONIC -> CLOCK_REALTIME if
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index f6b3206..70daf3c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -657,7 +657,8 @@ static bool intel_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
>   }
>
>   static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
> -			     int *vpos, int *hpos, ktime_t *stime, ktime_t *etime)
> +				    unsigned int flags, int *vpos, int *hpos,
> +				    ktime_t *stime, ktime_t *etime)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index ccd8751..3581570 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -305,7 +305,7 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
>   	 * to complete in this vblank?
>   	 */
>   	if (update_pending &&
> -	    (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id,
> +	    (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id, 0,
>   							       &vpos, &hpos, NULL, NULL)) &&
>   	    ((vpos >= (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100) ||
>   	     (vpos < 0 && !ASIC_IS_AVIVO(rdev)))) {
> @@ -1544,6 +1544,7 @@ bool radeon_crtc_scaling_mode_fixup(struct drm_crtc *crtc,
>    *
>    * \param dev Device to query.
>    * \param crtc Crtc to query.
> + * \param flags Flags from caller (DRM_CALLED_FROM_VBLIRQ or 0).
>    * \param *vpos Location where vertical scanout position should be stored.
>    * \param *hpos Location where horizontal scanout position should go.
>    * \param *stime Target location for timestamp taken immediately before
> @@ -1565,8 +1566,8 @@ bool radeon_crtc_scaling_mode_fixup(struct drm_crtc *crtc,
>    * unknown small number of scanlines wrt. real scanout position.
>    *
>    */
> -int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, int *vpos, int *hpos,
> -			       ktime_t *stime, ktime_t *etime)
> +int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int flags,
> +			       int *vpos, int *hpos, ktime_t *stime, ktime_t *etime)
>   {
>   	u32 stat_crtc = 0, vbl = 0, position = 0;
>   	int vbl_start, vbl_end, vtotal, ret = 0;
> diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
> index 3bfa910..c4016dc 100644
> --- a/drivers/gpu/drm/radeon/radeon_mode.h
> +++ b/drivers/gpu/drm/radeon/radeon_mode.h
> @@ -758,6 +758,7 @@ extern int radeon_crtc_cursor_move(struct drm_crtc *crtc,
>   				   int x, int y);
>
>   extern int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc,
> +				      unsigned int flags,
>   				      int *vpos, int *hpos, ktime_t *stime,
>   				      ktime_t *etime);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index 98bf63b..a394049 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -1468,7 +1468,7 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev)
>   	 */
>   	for (crtc = 0; (crtc < rdev->num_crtc) && in_vbl; crtc++) {
>   		if (rdev->pm.active_crtcs & (1 << crtc)) {
> -			vbl_status = radeon_get_crtc_scanoutpos(rdev->ddev, crtc, &vpos, &hpos, NULL, NULL);
> +			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))
>   				in_vbl = false;
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 14d4046..13c7942 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -840,6 +840,7 @@ struct drm_driver {
>   	 *
>   	 * \param dev  DRM device.
>   	 * \param crtc Id of the crtc to query.
> +	 * \param flags Flags from the caller (DRM_CALLED_FROM_VBLIRQ or 0).
>   	 * \param *vpos Target location for current vertical scanout position.
>   	 * \param *hpos Target location for current horizontal scanout position.
>   	 * \param *stime Target location for timestamp taken immediately before
> @@ -862,6 +863,7 @@ struct drm_driver {
>   	 *
>   	 */
>   	int (*get_scanout_position) (struct drm_device *dev, int crtc,
> +				     unsigned int flags,
>   				     int *vpos, int *hpos, ktime_t *stime,
>   				     ktime_t *etime);
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[ville.syrjala]

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.

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;
+		vtotal = rdev->mode_info.crtcs[crtc]->base.hwmode.crtc_vtotal;
+
+		if (vbl_start - *vpos < vtotal / 100) {
+			vpos -= vtotal;
+
+			/* Signal this correction as "applied". */
+			ret |= 0x8;
+		}
+	}
+
 	return ret;
 }
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mario Kleiner]

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

[PATCH 14/14] drm/i915: Add a kludge for DSL incrementing too late and ISR not working

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On pre-PCH platforms ISR doesn't seem to be an actual ISR, at least as
far as display interrupts are concerned. Instead it sort of looks like
some ISR bits just directly reflect the corresponding bit from PIPESTAT.
The bit appears in the ISR only if the PIPESTAT interrupt is enabled. So
in that sense it sort of looks a bit like the south interrupt scheme on
PCH platforms. So it goes something a bit like this:
PIPESTAT.status & PIPESTAT.enable -> ISR -> IMR -> IIR -> IER -> actual
interrupt

In any case that means the intel_pipe_in_vblank_locked() doesn't actually
work for pre-PCH platforms. As a last resort, add a similar kludge as radeon
has that fixes things up if we got called from the vblank interrupt,
but the scanline counter value indicates that we're not quite there yet.
We know that the scanline counter increments at hsync but is otherwise
accurate, so we can limit the kludge to the line just prior to vblank
start, instead of the relative distance that radeon uses.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 79 ++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 70daf3c..b93f39c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -603,36 +603,15 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe)
 #define __raw_i915_read32(dev_priv__, reg__) readl((dev_priv__)->regs + (reg__))
 #define __raw_i915_read16(dev_priv__, reg__) readw((dev_priv__)->regs + (reg__))
 
-static bool intel_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
+static bool ilk_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t status;
-	int reg;
 
-	if (IS_VALLEYVIEW(dev)) {
-		status = pipe == PIPE_A ?
-			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
-			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
-
-		reg = VLV_ISR;
-	} else if (IS_GEN2(dev)) {
-		status = pipe == PIPE_A ?
-			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
-			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
-
-		reg = ISR;
-	} else if (INTEL_INFO(dev)->gen < 5) {
-		status = pipe == PIPE_A ?
-			I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT :
-			I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
-
-		reg = ISR;
-	} else if (INTEL_INFO(dev)->gen < 7) {
+	if (INTEL_INFO(dev)->gen < 7) {
 		status = pipe == PIPE_A ?
 			DE_PIPEA_VBLANK :
 			DE_PIPEB_VBLANK;
-
-		reg = DEISR;
 	} else {
 		switch (pipe) {
 		default:
@@ -646,14 +625,9 @@ static bool intel_pipe_in_vblank_locked(struct drm_device *dev, enum pipe pipe)
 			status = DE_PIPEC_VBLANK_IVB;
 			break;
 		}
-
-		reg = DEISR;
 	}
 
-	if (IS_GEN2(dev))
-		return __raw_i915_read16(dev_priv, reg) & status;
-	else
-		return __raw_i915_read32(dev_priv, reg) & status;
+	return __raw_i915_read32(dev_priv, DEISR) & status;
 }
 
 static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
@@ -710,17 +684,42 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe,
 		else
 			position = __raw_i915_read32(dev_priv, PIPEDSL(pipe)) & DSL_LINEMASK_GEN3;
 
-		/*
-		 * The scanline counter increments at the leading edge
-		 * of hsync, ie. it completely misses the active portion
-		 * of the line. Fix up the counter at both edges of vblank
-		 * to get a more accurate picture whether we're in vblank
-		 * or not.
-		 */
-		in_vbl = intel_pipe_in_vblank_locked(dev, pipe);
-		if ((in_vbl && position == vbl_start - 1) ||
-		    (!in_vbl && position == vbl_end - 1))
-			position = (position + 1) % vtotal;
+		if (HAS_PCH_SPLIT(dev)) {
+			/*
+			 * The scanline counter increments at the leading edge
+			 * of hsync, ie. it completely misses the active portion
+			 * of the line. Fix up the counter at both edges of vblank
+			 * to get a more accurate picture whether we're in vblank
+			 * or not.
+			 */
+			in_vbl = ilk_pipe_in_vblank_locked(dev, pipe);
+			if ((in_vbl && position == vbl_start - 1) ||
+			    (!in_vbl && position == vbl_end - 1))
+				position = (position + 1) % vtotal;
+		} else {
+			/*
+			 * ISR vblank status bits don't work the way we'd want
+			 * them to work on non-PCH platforms (for
+			 * ilk_pipe_in_vblank_locked()), and there doesn't
+			 * appear any other way to determine if we're currently
+			 * in vblank.
+			 *
+			 * Instead let's assume that we're already in vblank if
+			 * we got called from the vblank interrupt and the
+			 * scanline counter value indicates that we're on the
+			 * line just prior to vblank start. This should result
+			 * in the correct answer, unless the vblank interrupt
+			 * delivery really got delayed for almost exactly one
+			 * full frame/field.
+			 */
+			if (flags & DRM_CALLED_FROM_VBLIRQ &&
+			    position == vbl_start - 1) {
+				position = (position + 1) % vtotal;
+
+				/* Signal this correction as "applied". */
+				ret |= 0x8;
+			}
+		}
 	} else {
 		/* Have access to pixelcount since start of frame.
 		 * We can split this into vertical and horizontal
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/14] drm: Some more vblank timestampi changes

[Dave Airlie]

On Wed, Oct 30, 2013 at 4:06 AM,  <ville.syrjala@linux.intel.com> wrote:
> So I took another look at the vblank timestamping code, and got a bit
> excited. The result is this patchset.

I'd like to merge this, I was hoping Mario could ack it at least as it
seems mostly sane to my eyes.

Dave.

Re: [PATCH 00/14] drm: Some more vblank timestampi changes

[Ville Syrjälä]

On Wed, Nov 06, 2013 at 01:46:41PM +1000, Dave Airlie wrote:
> On Wed, Oct 30, 2013 at 4:06 AM,  <ville.syrjala@linux.intel.com> wrote:
> > So I took another look at the vblank timestamping code, and got a bit
> > excited. The result is this patchset.
> 
> I'd like to merge this, I was hoping Mario could ack it at least as it
> seems mostly sane to my eyes.

So we missed that boat, but maybe we'll get the next one...

Pinging Mario. Any chance you can take a look at this stuff at some
point?

Hmm. Do I have the wrong email addres for Mario? Adding the other one
too just to make sure...

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 00/14] drm: Some more vblank timestampi changes

[Mario Kleiner]

On 29/11/13 14:36, Ville Syrjälä wrote:
> On Wed, Nov 06, 2013 at 01:46:41PM +1000, Dave Airlie wrote:
>> On Wed, Oct 30, 2013 at 4:06 AM,  <ville.syrjala@linux.intel.com> wrote:
>>> So I took another look at the vblank timestamping code, and got a bit
>>> excited. The result is this patchset.
>>
>> I'd like to merge this, I was hoping Mario could ack it at least as it
>> seems mostly sane to my eyes.
>
> So we missed that boat, but maybe we'll get the next one...
>
> Pinging Mario. Any chance you can take a look at this stuff at some
> point?
>

I will, including testing. Hopefully within the coming week, but 
definitely safely before christmas.

> Hmm. Do I have the wrong email addres for Mario? Adding the other one
> too just to make sure...
>

Both work, but the tuebingen.mpg.de one will probably soon turn into a 
pure forward to the gmail one.

-mario

Re: [PATCH 00/14] drm: Some more vblank timestampi changes

[Mario Kleiner]

On 29/10/13 19:06, ville.syrjala@linux.intel.com wrote:
 > So I took another look at the vblank timestamping code, and got a bit
 > excited. The result is this patchset.
 >
 > Summary of changes:
 > - kill crtc->hwmode dependency
 > - eliminate a bunch of 64bit math
 > - fix timestamps for stereo and interlaced modes (on i915 at least)
 > - move the "early vbl irq" hack into radeon code
 > - add a similar hack to i915, but make it as finely targeted
 >    as possibly to minimize the chance of accidentally
 >    applying it in the wrong place
 >
 > The s/clock/crtc_clock change could use some radeon people to verify
 > whether changing radeon_atom_get_tv_timings() is enough to make
 > crtc_clock always populated.
 >
 > This series applies on top of Mario's
 > "Vblank timestamping improvements/fixes for Linux drm." series.
 >
 > Ville Syrjälä (14):
 >        drm: Pass the display mode to drm_calc_timestamping_constants()
 >        drm: Pass the display mode to 
drm_calc_vbltimestamp_from_scanoutpos()
 >        drm/i915: Kill hwmode save/restore
 >        drm/i915: Call drm_calc_timestamping_constants() earlier
 >        drm: Improve drm_calc_timestamping_constants() documentation
 >        drm: Simplify the math in drm_calc_timestamping_constants()
 >        drm/radeon: Populate crtc_clock in radeon_atom_get_tv_timings()
 >        drm: Use crtc_clock in drm_calc_timestamping_constants()
 >        drm: Change {pixel,line,frame}dur_ns from s64 to int
 >        drm/i915: Fix scanoutpos calculations for interlaced modes
 >        drm: Fix vblank timestamping constants for interlaced modes
 >        drm: Pass 'flags' from the caller to .get_scanout_position()
 >        drm/radeon: Move the early vblank IRQ fixup to 
radeon_get_crtc_scanoutpos()
 >        drm/i915: Add a kludge for DSL incrementing too late and ISR 
not working
 >

Hi Ville,

sorry this took way longer than expected. I've reviewed all of your 
patches. Nice cleanups, nice improvements!

You can add a ...

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

... to all of them.

Patches 0 - 11 and 14 are fine as they are. Only tiny formatting/comment 
fixes needed so they apply cleanly against the current drm-next.

Patch 12 and 13 need some small fixes, after applying those i'm fine 
with them. I'll send separate e-mails for those.

As far as testing goes, i had more encounters with Murphy's law in the 
last weeks than ever before, hence the long delay. You can add

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

to the drm core and intel patches with the following restrictions:

I was able to "sort of" test the patchset on Intel GMA-950 (Gen-3 hw).

- I didn't test if your interlaced scanout patches 10 and 11 work as 
expected, because i was testing the patches first, then reviewing them, 
so i didn't realize at that point testing interlaced mode would be 
neccessary. The patches look correct to me though. I no longer have easy 
access to that machine.

- My photodiode test equipment, which i need for Intel testing 
malfunctioned. Not sure if my testing hardware is dying, or if it is a 
bug in the kernels usb or serial/tty stack, or some kernel 
misconfiguration wrt. low-latency, but there was so much timing noise in 
my equipment that i couldn't test with it.

- As a workaround I ran the kms-timestamping for regular non-interlaced 
mode against the original userspace implementation of the same code in 
my own toolkit Psychtoolbox, which itself was verified with testing 
equipment to do the right thing on that GMA-950 netbook earlier this 
year. Difference was less than 40 microseconds and more likely caused 
due to userspace noisyness and off-by-one errors in Psychtoolbox than 
your code, so i assume that your code is essentially correct at least 
for non-interlaced scanout, and that the DRM core changes are therefore 
also correct. If you or somebody would want to try this test yourself i 
can guide you through the steps. Psychtoolbox is easily apt-get'able for 
Debian and at least Ubuntu.

- The next limitation of my testing is wrt. to your "early vbl irq 
handling" improvements (patch 14). I currently only have Gen3 hardware 
which doesn't exercise those code path at all, so while the patch looks 
correct, it's not really tested by me.

As far as Radeon testing goes, i can't test it at all atm. After already 
not working very stable at all for the last half year, my last machine 
with an AMD card died during bootup for this test, but not without 
trying to corrupt the filesystem on my development drive as a little 
post-christmas gift to me. If somebody has a AMD card and wants to test 
this, it could be tested against the Psychtoolbox userspace reference 
implementation, which was verified with very precise external hardware 
last time a couple of months ago. However, patch 13 needs some fixes or 
it would crash. The now dead PC wasn't mine, but i still have the AMD card.

I will try to hunt for a new PC soon, and hopefully will get your 
patches better tested during the -rc phase if they get merged into 3.14.

Apart from a NVidia card, my 2010 MacBookPro also has an integrated 
Gen-4 Intel card, connected to the internal panel via hardware mux, but 
so far i wasn't successfull at all to make use of it under Ubuntu 13.10. 
I can power on the card, make it detected by vgaswitcheroo/kms and 
manually switch the mux via some hacks, but it never boots into a 
functional desktop :(. I haven't tried very hard though with more recent 
kernels.

-mario

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx