* [PATCH 0/3] drm/i915: i915_get_crtc_scanoutpos improvements
@ 2013-09-23 10:02 ville.syrjala
2013-09-23 10:02 ` [PATCH 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() ville.syrjala
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: ville.syrjala @ 2013-09-23 10:02 UTC (permalink / raw)
To: intel-gfx; +Cc: Mario Kleiner
These patches make some fixes and improvements to the get_scanoutpos() code.
They've been sitting on my disk for a while now, but I haven't found
the time to really test them properly. But as the subject of eliminating
the useless register reads came up recently, I decided that it's better
to post the patches to the list to avoid someone else duplicating the
effort.
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-23 10:02 [PATCH 0/3] drm/i915: i915_get_crtc_scanoutpos improvements ville.syrjala @ 2013-09-23 10:02 ` ville.syrjala 2013-09-23 10:30 ` Chris Wilson 2013-09-23 10:02 ` [PATCH 2/3] drm/i915: Fix scanoutpos calculations ville.syrjala 2013-09-23 10:02 ` [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ ville.syrjala 2 siblings, 1 reply; 22+ messages in thread From: ville.syrjala @ 2013-09-23 10:02 UTC (permalink / raw) To: intel-gfx; +Cc: Mario Kleiner From: Ville Syrjälä <ville.syrjala@linux.intel.com> We have all the information we need in the mode structure, so going and reading it from the hardware is pointless, and slower. For the !kms case, leave the register reads in place. Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 51 +++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b356dc1..697d62c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -571,12 +571,10 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, int *vpos, int *hpos) { drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; - u32 vbl = 0, position = 0; + u32 position; int vbl_start, vbl_end, htotal, vtotal; bool in_vbl = true; int ret = 0; - enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, - pipe); if (!i915_pipe_enabled(dev, pipe)) { DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " @@ -584,10 +582,36 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, return 0; } - /* Get vtotal. */ - vtotal = 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode; - if (INTEL_INFO(dev)->gen >= 4) { + htotal = mode->crtc_htotal; + vtotal = mode->crtc_vtotal; + vbl_start = mode->crtc_vblank_start; + vbl_end = mode->crtc_vblank_end; + + ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; + } else { + enum transcoder cpu_transcoder = + intel_pipe_to_cpu_transcoder(dev_priv, pipe); + u32 vbl; + + htotal = 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); + vtotal = 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); + + vbl = I915_READ(VBLANK(cpu_transcoder)); + + vbl_start = 1 + (vbl & 0x1fff); + vbl_end = 1 + ((vbl >> 16) & 0x1fff); + + /* Readouts valid? */ + if (vbl > 0) + ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; + } + + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { /* No obvious pixelcount register. Only query vertical * scanout position from Display scan line register. */ @@ -605,29 +629,16 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, */ position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; - htotal = 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); *vpos = position / htotal; *hpos = position - (*vpos * htotal); } - /* Query vblank area. */ - vbl = I915_READ(VBLANK(cpu_transcoder)); - - /* Test position against vblank region. */ - vbl_start = vbl & 0x1fff; - vbl_end = (vbl >> 16) & 0x1fff; - - if ((*vpos < vbl_start) || (*vpos > vbl_end)) - in_vbl = false; + in_vbl = *vpos >= vbl_start && *vpos < vbl_end; /* Inside "upper part" of vblank area? Apply corrective offset: */ if (in_vbl && (*vpos >= vbl_start)) *vpos = *vpos - vtotal; - /* Readouts valid? */ - if (vbl > 0) - ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; - /* In vblank? */ if (in_vbl) ret |= DRM_SCANOUTPOS_INVBL; -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-23 10:02 ` [PATCH 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() ville.syrjala @ 2013-09-23 10:30 ` Chris Wilson 2013-09-23 11:48 ` [PATCH v2 " ville.syrjala 0 siblings, 1 reply; 22+ messages in thread From: Chris Wilson @ 2013-09-23 10:30 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx, Mario Kleiner On Mon, Sep 23, 2013 at 01:02:05PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We have all the information we need in the mode structure, so going and > reading it from the hardware is pointless, and slower. > > For the !kms case, leave the register reads in place. dev->driver->get_scanout_position [i915_get_crtc_scanoutpos] is only called by i915_get_vblank_timestamp() which is only used by KMS. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-23 10:30 ` Chris Wilson @ 2013-09-23 11:48 ` ville.syrjala 2013-09-24 9:11 ` Daniel Vetter 2013-09-25 2:35 ` Mario Kleiner 0 siblings, 2 replies; 22+ messages in thread From: ville.syrjala @ 2013-09-23 11:48 UTC (permalink / raw) To: intel-gfx; +Cc: Mario Kleiner From: Ville Syrjälä <ville.syrjala@linux.intel.com> We have all the information we need in the mode structure, so going and reading it from the hardware is pointless, and slower. We never populated ->get_vblank_timestamp() in the UMS case, and as that is the only way we'd ever call ->get_scanout_position(), we can completely ignore UMS in i915_get_crtc_scanoutpos(). Also reorganize intel_irq_init() a bit to clarify the KMS vs. UMS situation. v2: Drop UMS code Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 43 ++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b356dc1..058f099 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -570,24 +570,29 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, int *vpos, int *hpos) { - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; - u32 vbl = 0, position = 0; + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode; + u32 position; int vbl_start, vbl_end, htotal, vtotal; bool in_vbl = true; int ret = 0; - enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, - pipe); - if (!i915_pipe_enabled(dev, pipe)) { + if (!intel_crtc->active) { DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " "pipe %c\n", pipe_name(pipe)); return 0; } - /* Get vtotal. */ - vtotal = 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); + htotal = mode->crtc_htotal; + vtotal = mode->crtc_vtotal; + vbl_start = mode->crtc_vblank_start; + vbl_end = mode->crtc_vblank_end; - if (INTEL_INFO(dev)->gen >= 4) { + ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; + + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { /* No obvious pixelcount register. Only query vertical * scanout position from Display scan line register. */ @@ -605,29 +610,16 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, */ position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; - htotal = 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); *vpos = position / htotal; *hpos = position - (*vpos * htotal); } - /* Query vblank area. */ - vbl = I915_READ(VBLANK(cpu_transcoder)); - - /* Test position against vblank region. */ - vbl_start = vbl & 0x1fff; - vbl_end = (vbl >> 16) & 0x1fff; - - if ((*vpos < vbl_start) || (*vpos > vbl_end)) - in_vbl = false; + in_vbl = *vpos >= vbl_start && *vpos < vbl_end; /* Inside "upper part" of vblank area? Apply corrective offset: */ if (in_vbl && (*vpos >= vbl_start)) *vpos = *vpos - vtotal; - /* Readouts valid? */ - if (vbl > 0) - ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; - /* In vblank? */ if (in_vbl) ret |= DRM_SCANOUTPOS_INVBL; @@ -3148,11 +3140,10 @@ void intel_irq_init(struct drm_device *dev) dev->driver->get_vblank_counter = gm45_get_vblank_counter; } - if (drm_core_check_feature(dev, DRIVER_MODESET)) + if (drm_core_check_feature(dev, DRIVER_MODESET)) { dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; - else - dev->driver->get_vblank_timestamp = NULL; - dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; + dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; + } if (IS_VALLEYVIEW(dev)) { dev->driver->irq_handler = valleyview_irq_handler; -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-23 11:48 ` [PATCH v2 " ville.syrjala @ 2013-09-24 9:11 ` Daniel Vetter 2013-09-25 4:45 ` Mario Kleiner 2013-09-25 2:35 ` Mario Kleiner 1 sibling, 1 reply; 22+ messages in thread From: Daniel Vetter @ 2013-09-24 9:11 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx, Mario Kleiner On Mon, Sep 23, 2013 at 02:48:50PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We have all the information we need in the mode structure, so going and > reading it from the hardware is pointless, and slower. > > We never populated ->get_vblank_timestamp() in the UMS case, and as that > is the only way we'd ever call ->get_scanout_position(), we can > completely ignore UMS in i915_get_crtc_scanoutpos(). > > Also reorganize intel_irq_init() a bit to clarify the KMS vs. UMS > situation. > > v2: Drop UMS code > > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 43 ++++++++++++++++------------------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index b356dc1..058f099 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -570,24 +570,29 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) > static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > int *vpos, int *hpos) > { > - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > - u32 vbl = 0, position = 0; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode; > + u32 position; > int vbl_start, vbl_end, htotal, vtotal; > bool in_vbl = true; > int ret = 0; > - enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, > - pipe); Hooray, this rips out the racy pipe_to_cpu_transcoder deref, so I'm all in favour \o/ Of course I still have that ugly itme on my todo about "fix the locking for kms vblank stuff" ;-) Mario, can you please take a look at these patches and ack them? I'd like to slurp them in for -next. -Daniel > > - if (!i915_pipe_enabled(dev, pipe)) { > + if (!intel_crtc->active) { > DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " > "pipe %c\n", pipe_name(pipe)); > return 0; > } > > - /* Get vtotal. */ > - vtotal = 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); > + htotal = mode->crtc_htotal; > + vtotal = mode->crtc_vtotal; > + vbl_start = mode->crtc_vblank_start; > + vbl_end = mode->crtc_vblank_end; > > - if (INTEL_INFO(dev)->gen >= 4) { > + ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; > + > + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { > /* No obvious pixelcount register. Only query vertical > * scanout position from Display scan line register. > */ > @@ -605,29 +610,16 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > */ > position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; > > - htotal = 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); > *vpos = position / htotal; > *hpos = position - (*vpos * htotal); > } > > - /* Query vblank area. */ > - vbl = I915_READ(VBLANK(cpu_transcoder)); > - > - /* Test position against vblank region. */ > - vbl_start = vbl & 0x1fff; > - vbl_end = (vbl >> 16) & 0x1fff; > - > - if ((*vpos < vbl_start) || (*vpos > vbl_end)) > - in_vbl = false; > + in_vbl = *vpos >= vbl_start && *vpos < vbl_end; > > /* Inside "upper part" of vblank area? Apply corrective offset: */ > if (in_vbl && (*vpos >= vbl_start)) > *vpos = *vpos - vtotal; > > - /* Readouts valid? */ > - if (vbl > 0) > - ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; > - > /* In vblank? */ > if (in_vbl) > ret |= DRM_SCANOUTPOS_INVBL; > @@ -3148,11 +3140,10 @@ void intel_irq_init(struct drm_device *dev) > dev->driver->get_vblank_counter = gm45_get_vblank_counter; > } > > - if (drm_core_check_feature(dev, DRIVER_MODESET)) > + if (drm_core_check_feature(dev, DRIVER_MODESET)) { > dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; > - else > - dev->driver->get_vblank_timestamp = NULL; > - dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; > + dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; > + } > > if (IS_VALLEYVIEW(dev)) { > dev->driver->irq_handler = valleyview_irq_handler; > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-24 9:11 ` Daniel Vetter @ 2013-09-25 4:45 ` Mario Kleiner 2013-09-25 7:52 ` Daniel Vetter 0 siblings, 1 reply; 22+ messages in thread From: Mario Kleiner @ 2013-09-25 4:45 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Mario Kleiner On 24.09.13 11:11, Daniel Vetter wrote: ... > > Hooray, this rips out the racy pipe_to_cpu_transcoder deref, so I'm all in > favour \o/ Of course I still have that ugly itme on my todo about "fix the > locking for kms vblank stuff" ;-) > See the other e-mail i sent. Maybe pushing the time read into the i915_get_crtc_scanoutpos() as Ville proposed would not require that extra lock. That would need a change of drm core and both intel-kms and radeon-kms within one kernel release due to the internal api change. > Mario, can you please take a look at these patches and ack them? I'd like > to slurp them in for -next. Done in the other e-mails, with some comments. However, i'd like to test these a bit and it might take a week or two before i can get my hands on the machine with the intel card and the measurement equipment. Are we still quite far away from the next merge window? -mario > -Daniel > >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-25 4:45 ` Mario Kleiner @ 2013-09-25 7:52 ` Daniel Vetter 0 siblings, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2013-09-25 7:52 UTC (permalink / raw) To: Mario Kleiner; +Cc: intel-gfx On Wed, Sep 25, 2013 at 6:45 AM, Mario Kleiner <mario.kleiner@tuebingen.mpg.de> wrote: > >> Mario, can you please take a look at these patches and ack them? I'd like >> to slurp them in for -next. > > > Done in the other e-mails, with some comments. However, i'd like to test > these a bit and it might take a week or two before i can get my hands on the > machine with the intel card and the measurement equipment. > > Are we still quite far away from the next merge window? Yeah, we still have plenty time. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-23 11:48 ` [PATCH v2 " ville.syrjala 2013-09-24 9:11 ` Daniel Vetter @ 2013-09-25 2:35 ` Mario Kleiner 2013-09-25 8:14 ` Ville Syrjälä 1 sibling, 1 reply; 22+ messages in thread From: Mario Kleiner @ 2013-09-25 2:35 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On 23.09.13 13:48, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We have all the information we need in the mode structure, so going and > reading it from the hardware is pointless, and slower. > > We never populated ->get_vblank_timestamp() in the UMS case, and as that > is the only way we'd ever call ->get_scanout_position(), we can > completely ignore UMS in i915_get_crtc_scanoutpos(). > > Also reorganize intel_irq_init() a bit to clarify the KMS vs. UMS > situation. > > v2: Drop UMS code > > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 43 ++++++++++++++++------------------------- > 1 file changed, 17 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index b356dc1..058f099 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -570,24 +570,29 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) > static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > int *vpos, int *hpos) > { > - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > - u32 vbl = 0, position = 0; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode; > + u32 position; > int vbl_start, vbl_end, htotal, vtotal; > bool in_vbl = true; > int ret = 0; > - enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, > - pipe); > > - if (!i915_pipe_enabled(dev, pipe)) { > + if (!intel_crtc->active) { > DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " > "pipe %c\n", pipe_name(pipe)); > return 0; > } > > - /* Get vtotal. */ > - vtotal = 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); > + htotal = mode->crtc_htotal; > + vtotal = mode->crtc_vtotal; > + vbl_start = mode->crtc_vblank_start; > + vbl_end = mode->crtc_vblank_end; > > - if (INTEL_INFO(dev)->gen >= 4) { > + ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; > + > + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { > /* No obvious pixelcount register. Only query vertical > * scanout position from Display scan line register. > */ > @@ -605,29 +610,16 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > */ > position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; > > - htotal = 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); > *vpos = position / htotal; > *hpos = position - (*vpos * htotal); > } > > - /* Query vblank area. */ > - vbl = I915_READ(VBLANK(cpu_transcoder)); > - > - /* Test position against vblank region. */ > - vbl_start = vbl & 0x1fff; > - vbl_end = (vbl >> 16) & 0x1fff; > - > - if ((*vpos < vbl_start) || (*vpos > vbl_end)) > - in_vbl = false; > + in_vbl = *vpos >= vbl_start && *vpos < vbl_end; I think this should be a <= instead of < in *vpos < vbl_end, if it is meant to be equal to the line it replaces (not > is <=), unless the original comparison was off-by-one? > + in_vbl = *vpos >= vbl_start && *vpos <= vbl_end; Other than that, it looks good. Reviewed-by: mario.kleiner.de@gmail.com > > /* Inside "upper part" of vblank area? Apply corrective offset: */ > if (in_vbl && (*vpos >= vbl_start)) > *vpos = *vpos - vtotal; > > - /* Readouts valid? */ > - if (vbl > 0) > - ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; > - > /* In vblank? */ > if (in_vbl) > ret |= DRM_SCANOUTPOS_INVBL; > @@ -3148,11 +3140,10 @@ void intel_irq_init(struct drm_device *dev) > dev->driver->get_vblank_counter = gm45_get_vblank_counter; > } > > - if (drm_core_check_feature(dev, DRIVER_MODESET)) > + if (drm_core_check_feature(dev, DRIVER_MODESET)) { > dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; > - else > - dev->driver->get_vblank_timestamp = NULL; > - dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; > + dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; > + } > > if (IS_VALLEYVIEW(dev)) { > dev->driver->irq_handler = valleyview_irq_handler; > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-25 2:35 ` Mario Kleiner @ 2013-09-25 8:14 ` Ville Syrjälä 2013-09-26 16:12 ` Mario Kleiner 0 siblings, 1 reply; 22+ messages in thread From: Ville Syrjälä @ 2013-09-25 8:14 UTC (permalink / raw) To: Mario Kleiner; +Cc: intel-gfx On Wed, Sep 25, 2013 at 04:35:56AM +0200, Mario Kleiner wrote: > On 23.09.13 13:48, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > We have all the information we need in the mode structure, so going and > > reading it from the hardware is pointless, and slower. > > > > We never populated ->get_vblank_timestamp() in the UMS case, and as that > > is the only way we'd ever call ->get_scanout_position(), we can > > completely ignore UMS in i915_get_crtc_scanoutpos(). > > > > Also reorganize intel_irq_init() a bit to clarify the KMS vs. UMS > > situation. > > > > v2: Drop UMS code > > > > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 43 ++++++++++++++++------------------------- > > 1 file changed, 17 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index b356dc1..058f099 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -570,24 +570,29 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) > > static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > > int *vpos, int *hpos) > > { > > - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > > - u32 vbl = 0, position = 0; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > + const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode; > > + u32 position; > > int vbl_start, vbl_end, htotal, vtotal; > > bool in_vbl = true; > > int ret = 0; > > - enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, > > - pipe); > > > > - if (!i915_pipe_enabled(dev, pipe)) { > > + if (!intel_crtc->active) { > > DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " > > "pipe %c\n", pipe_name(pipe)); > > return 0; > > } > > > > - /* Get vtotal. */ > > - vtotal = 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); > > + htotal = mode->crtc_htotal; > > + vtotal = mode->crtc_vtotal; > > + vbl_start = mode->crtc_vblank_start; > > + vbl_end = mode->crtc_vblank_end; > > > > - if (INTEL_INFO(dev)->gen >= 4) { > > + ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; > > + > > + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { > > /* No obvious pixelcount register. Only query vertical > > * scanout position from Display scan line register. > > */ > > @@ -605,29 +610,16 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > > */ > > position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; > > > > - htotal = 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); > > *vpos = position / htotal; > > *hpos = position - (*vpos * htotal); > > } > > > > - /* Query vblank area. */ > > - vbl = I915_READ(VBLANK(cpu_transcoder)); > > - > > - /* Test position against vblank region. */ > > - vbl_start = vbl & 0x1fff; > > - vbl_end = (vbl >> 16) & 0x1fff; > > - > > - if ((*vpos < vbl_start) || (*vpos > vbl_end)) > > - in_vbl = false; > > + in_vbl = *vpos >= vbl_start && *vpos < vbl_end; > > I think this should be a <= instead of < in *vpos < vbl_end, if it is > meant to be equal to the line it replaces (not > is <=), unless the > original comparison was off-by-one? Yeah, I think the original was wrong, in more ways than one. It forgot to add +1 to vbl_start/end, and then it did the comparison wrong as well. > > > + in_vbl = *vpos >= vbl_start && *vpos <= vbl_end; > > Other than that, it looks good. > > Reviewed-by: mario.kleiner.de@gmail.com > > > > > /* Inside "upper part" of vblank area? Apply corrective offset: */ > > if (in_vbl && (*vpos >= vbl_start)) > > *vpos = *vpos - vtotal; > > > > - /* Readouts valid? */ > > - if (vbl > 0) > > - ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; > > - > > /* In vblank? */ > > if (in_vbl) > > ret |= DRM_SCANOUTPOS_INVBL; > > @@ -3148,11 +3140,10 @@ void intel_irq_init(struct drm_device *dev) > > dev->driver->get_vblank_counter = gm45_get_vblank_counter; > > } > > > > - if (drm_core_check_feature(dev, DRIVER_MODESET)) > > + if (drm_core_check_feature(dev, DRIVER_MODESET)) { > > dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; > > - else > > - dev->driver->get_vblank_timestamp = NULL; > > - dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; > > + dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; > > + } > > > > if (IS_VALLEYVIEW(dev)) { > > dev->driver->irq_handler = valleyview_irq_handler; > > -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() 2013-09-25 8:14 ` Ville Syrjälä @ 2013-09-26 16:12 ` Mario Kleiner 0 siblings, 0 replies; 22+ messages in thread From: Mario Kleiner @ 2013-09-26 16:12 UTC (permalink / raw) Cc: intel-gfx, Mario Kleiner On 25.09.13 10:14, Ville Syrjälä wrote: > On Wed, Sep 25, 2013 at 04:35:56AM +0200, Mario Kleiner wrote: >> On 23.09.13 13:48, ville.syrjala@linux.intel.com wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> We have all the information we need in the mode structure, so going and >>> reading it from the hardware is pointless, and slower. >>> >>> We never populated ->get_vblank_timestamp() in the UMS case, and as that >>> is the only way we'd ever call ->get_scanout_position(), we can >>> completely ignore UMS in i915_get_crtc_scanoutpos(). >>> >>> Also reorganize intel_irq_init() a bit to clarify the KMS vs. UMS >>> situation. >>> >>> v2: Drop UMS code >>> >>> Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_irq.c | 43 ++++++++++++++++------------------------- >>> 1 file changed, 17 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >>> index b356dc1..058f099 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -570,24 +570,29 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) >>> static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, >>> int *vpos, int *hpos) >>> { >>> - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; >>> - u32 vbl = 0, position = 0; >>> + struct drm_i915_private *dev_priv = dev->dev_private; >>> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; >>> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>> + const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode; >>> + u32 position; >>> int vbl_start, vbl_end, htotal, vtotal; >>> bool in_vbl = true; >>> int ret = 0; >>> - enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, >>> - pipe); >>> >>> - if (!i915_pipe_enabled(dev, pipe)) { >>> + if (!intel_crtc->active) { >>> DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " >>> "pipe %c\n", pipe_name(pipe)); >>> return 0; >>> } >>> >>> - /* Get vtotal. */ >>> - vtotal = 1 + ((I915_READ(VTOTAL(cpu_transcoder)) >> 16) & 0x1fff); >>> + htotal = mode->crtc_htotal; >>> + vtotal = mode->crtc_vtotal; >>> + vbl_start = mode->crtc_vblank_start; >>> + vbl_end = mode->crtc_vblank_end; >>> >>> - if (INTEL_INFO(dev)->gen >= 4) { >>> + ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; >>> + >>> + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { >>> /* No obvious pixelcount register. Only query vertical >>> * scanout position from Display scan line register. >>> */ >>> @@ -605,29 +610,16 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, >>> */ >>> position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; >>> >>> - htotal = 1 + ((I915_READ(HTOTAL(cpu_transcoder)) >> 16) & 0x1fff); >>> *vpos = position / htotal; >>> *hpos = position - (*vpos * htotal); >>> } >>> >>> - /* Query vblank area. */ >>> - vbl = I915_READ(VBLANK(cpu_transcoder)); >>> - >>> - /* Test position against vblank region. */ >>> - vbl_start = vbl & 0x1fff; >>> - vbl_end = (vbl >> 16) & 0x1fff; >>> - >>> - if ((*vpos < vbl_start) || (*vpos > vbl_end)) >>> - in_vbl = false; >>> + in_vbl = *vpos >= vbl_start && *vpos < vbl_end; >> >> I think this should be a <= instead of < in *vpos < vbl_end, if it is >> meant to be equal to the line it replaces (not > is <=), unless the >> original comparison was off-by-one? > > Yeah, I think the original was wrong, in more ways than one. It forgot > to add +1 to vbl_start/end, and then it did the comparison wrong as > well. > Ah ok, that's possible. Then you have my blessing :). On the Intel side i only had and have sporadic access to an old Intel GMA-950 (Gen-3?) when writing that function, so i could only really test one half of the code-path in that function. Also that card only has a VGA output, which limits my actual measurements to use of a photo-diode attached to a CRT monitor. That means i can only verify the accuracy of timestamping down to about 0.2 msecs variability and 0.5 msecs bias due to the limitations/noise of the measurement setup (depending how close i get the photo-diode to the corner of the monitor, how dark it is, etc.). So i know that the jitter in the timestamps is very low, less than 1 usec standard deviation iirc, and that the absolute error wrt. reality is lower than 0.2 msecs, but i wouldn't be able to detect absolute errors of a few scanlines. -mario >> >> > + in_vbl = *vpos >= vbl_start && *vpos <= vbl_end; >> >> Other than that, it looks good. >> >> Reviewed-by: mario.kleiner.de@gmail.com >> >>> >>> /* Inside "upper part" of vblank area? Apply corrective offset: */ >>> if (in_vbl && (*vpos >= vbl_start)) >>> *vpos = *vpos - vtotal; >>> >>> - /* Readouts valid? */ >>> - if (vbl > 0) >>> - ret |= DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE; >>> - >>> /* In vblank? */ >>> if (in_vbl) >>> ret |= DRM_SCANOUTPOS_INVBL; >>> @@ -3148,11 +3140,10 @@ void intel_irq_init(struct drm_device *dev) >>> dev->driver->get_vblank_counter = gm45_get_vblank_counter; >>> } >>> >>> - if (drm_core_check_feature(dev, DRIVER_MODESET)) >>> + if (drm_core_check_feature(dev, DRIVER_MODESET)) { >>> dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; >>> - else >>> - dev->driver->get_vblank_timestamp = NULL; >>> - dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; >>> + dev->driver->get_scanout_position = i915_get_crtc_scanoutpos; >>> + } >>> >>> if (IS_VALLEYVIEW(dev)) { >>> dev->driver->irq_handler = valleyview_irq_handler; >>> > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] drm/i915: Fix scanoutpos calculations 2013-09-23 10:02 [PATCH 0/3] drm/i915: i915_get_crtc_scanoutpos improvements ville.syrjala 2013-09-23 10:02 ` [PATCH 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() ville.syrjala @ 2013-09-23 10:02 ` ville.syrjala 2013-09-25 2:39 ` Mario Kleiner 2013-10-11 14:31 ` Mario Kleiner 2013-09-23 10:02 ` [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ ville.syrjala 2 siblings, 2 replies; 22+ messages in thread From: ville.syrjala @ 2013-09-23 10:02 UTC (permalink / raw) To: intel-gfx; +Cc: Mario Kleiner From: Ville Syrjälä <ville.syrjala@linux.intel.com> The reported scanout position must be relative to the end of vblank. Currently we manage to fumble that in a few ways. First we don't consider the case when vtotal != vbl_end. While that isn't very common (happens maybe only w/ old panel fitting hardware), we can fix it easily enough. The second issue is that on pre-CTG hardware we convert the pixel count to horizontal/vertical components at the very beginning, and then forget to adjust the horizontal component to be relative to vbl_end. So instead we should keep our numbers in the pixel count domain while we're adjusting the position to be relative to vbl_end. Then when we do the conversion in the end, both vertical _and_ horizontal components will come out correct. Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 697d62c..4f74f0c 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -615,13 +615,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, /* No obvious pixelcount register. Only query vertical * scanout position from Display scan line register. */ - position = I915_READ(PIPEDSL(pipe)); - - /* Decode into vertical scanout position. Don't have - * horizontal scanout position. - */ - *vpos = position & 0x1fff; - *hpos = 0; + position = I915_READ(PIPEDSL(pipe)) & 0x1fff; } else { /* Have access to pixelcount since start of frame. * We can split this into vertical and horizontal @@ -629,15 +623,32 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, */ position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; - *vpos = position / htotal; - *hpos = position - (*vpos * htotal); + /* convert to pixel counts */ + vbl_start *= htotal; + vbl_end *= htotal; + vtotal *= htotal; } - in_vbl = *vpos >= vbl_start && *vpos < vbl_end; + in_vbl = position >= vbl_start && position < vbl_end; - /* Inside "upper part" of vblank area? Apply corrective offset: */ - if (in_vbl && (*vpos >= vbl_start)) - *vpos = *vpos - vtotal; + /* + * While in vblank, position will be negative + * counting up towards 0 at vbl_end. And outside + * vblank, position will be positive counting + * up since vbl_end. + */ + if (position >= vbl_start) + position -= vbl_end; + else + position += vtotal - vbl_end; + + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { + *vpos = position; + *hpos = 0; + } else { + *vpos = position / htotal; + *hpos = position - (*vpos * htotal); + } /* In vblank? */ if (in_vbl) -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fix scanoutpos calculations 2013-09-23 10:02 ` [PATCH 2/3] drm/i915: Fix scanoutpos calculations ville.syrjala @ 2013-09-25 2:39 ` Mario Kleiner 2013-10-11 14:31 ` Mario Kleiner 1 sibling, 0 replies; 22+ messages in thread From: Mario Kleiner @ 2013-09-25 2:39 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx On 23.09.13 12:02, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The reported scanout position must be relative to the end of vblank. > Currently we manage to fumble that in a few ways. > > First we don't consider the case when vtotal != vbl_end. While that > isn't very common (happens maybe only w/ old panel fitting hardware), > we can fix it easily enough. > > The second issue is that on pre-CTG hardware we convert the pixel count > to horizontal/vertical components at the very beginning, and then forget > to adjust the horizontal component to be relative to vbl_end. So instead > we should keep our numbers in the pixel count domain while we're > adjusting the position to be relative to vbl_end. Then when we do the > conversion in the end, both vertical _and_ horizontal components will > come out correct. > > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 697d62c..4f74f0c 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -615,13 +615,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > /* No obvious pixelcount register. Only query vertical > * scanout position from Display scan line register. > */ > - position = I915_READ(PIPEDSL(pipe)); > - > - /* Decode into vertical scanout position. Don't have > - * horizontal scanout position. > - */ > - *vpos = position & 0x1fff; > - *hpos = 0; > + position = I915_READ(PIPEDSL(pipe)) & 0x1fff; > } else { > /* Have access to pixelcount since start of frame. > * We can split this into vertical and horizontal > @@ -629,15 +623,32 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > */ > position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; > > - *vpos = position / htotal; > - *hpos = position - (*vpos * htotal); > + /* convert to pixel counts */ > + vbl_start *= htotal; > + vbl_end *= htotal; > + vtotal *= htotal; > } > > - in_vbl = *vpos >= vbl_start && *vpos < vbl_end; > + in_vbl = position >= vbl_start && position < vbl_end; > Carried forward from patch 1/3: position <= vbl_end Everything else is fine. Nice improvement :) Reviewed-by: mario.kleiner.de@gmail.com > - /* Inside "upper part" of vblank area? Apply corrective offset: */ > - if (in_vbl && (*vpos >= vbl_start)) > - *vpos = *vpos - vtotal; > + /* > + * While in vblank, position will be negative > + * counting up towards 0 at vbl_end. And outside > + * vblank, position will be positive counting > + * up since vbl_end. > + */ > + if (position >= vbl_start) > + position -= vbl_end; > + else > + position += vtotal - vbl_end; > + > + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { > + *vpos = position; > + *hpos = 0; > + } else { > + *vpos = position / htotal; > + *hpos = position - (*vpos * htotal); > + } > > /* In vblank? */ > if (in_vbl) > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fix scanoutpos calculations 2013-09-23 10:02 ` [PATCH 2/3] drm/i915: Fix scanoutpos calculations ville.syrjala 2013-09-25 2:39 ` Mario Kleiner @ 2013-10-11 14:31 ` Mario Kleiner 2013-10-11 16:10 ` [PATCH v2 " ville.syrjala 2013-10-11 23:19 ` [PATCH " Daniel Vetter 1 sibling, 2 replies; 22+ messages in thread From: Mario Kleiner @ 2013-10-11 14:31 UTC (permalink / raw) To: ville.syrjala, intel-gfx, Daniel Vetter; +Cc: Mario Kleiner Daniel, Ville, i tested Ville's patch series for the scanoutpos improvements on a GMA-950, on top of airlied's current drm-next branch. There's one issue: The variable "position" in i915_get_crtc_scanoutpos() must be turned from a u32 into a int, otherwise funny sign errors happen and we end up with *vpos being off by multiple million scanlines and timestamps being off by over 60 seconds. Other than that looks good. Execution time is now better: Before uncore.lock addition: 3-4 usecs execution time for the scanoutpos query on my machine. After uncore.lock addition (3.12.0-rc3) 9-20 usecs, sometimes repetition of the timing loop triggered. After Ville's patches down to typically 3-8 usecs, occassionally spiking to almost 20 usecs. I'll make my patches for the realtime kernel + increased accuracy on top of drm-next + Ville's patches. thanks, -mario On 09/23/2013 12:02 PM, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The reported scanout position must be relative to the end of vblank. > Currently we manage to fumble that in a few ways. > > First we don't consider the case when vtotal != vbl_end. While that > isn't very common (happens maybe only w/ old panel fitting hardware), > we can fix it easily enough. > > The second issue is that on pre-CTG hardware we convert the pixel count > to horizontal/vertical components at the very beginning, and then forget > to adjust the horizontal component to be relative to vbl_end. So instead > we should keep our numbers in the pixel count domain while we're > adjusting the position to be relative to vbl_end. Then when we do the > conversion in the end, both vertical _and_ horizontal components will > come out correct. > > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 37 ++++++++++++++++++++++++------------- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 697d62c..4f74f0c 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -615,13 +615,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > /* No obvious pixelcount register. Only query vertical > * scanout position from Display scan line register. > */ > - position = I915_READ(PIPEDSL(pipe)); > - > - /* Decode into vertical scanout position. Don't have > - * horizontal scanout position. > - */ > - *vpos = position & 0x1fff; > - *hpos = 0; > + position = I915_READ(PIPEDSL(pipe)) & 0x1fff; > } else { > /* Have access to pixelcount since start of frame. > * We can split this into vertical and horizontal > @@ -629,15 +623,32 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > */ > position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; > > - *vpos = position / htotal; > - *hpos = position - (*vpos * htotal); > + /* convert to pixel counts */ > + vbl_start *= htotal; > + vbl_end *= htotal; > + vtotal *= htotal; > } > > - in_vbl = *vpos >= vbl_start && *vpos < vbl_end; > + in_vbl = position >= vbl_start && position < vbl_end; > > - /* Inside "upper part" of vblank area? Apply corrective offset: */ > - if (in_vbl && (*vpos >= vbl_start)) > - *vpos = *vpos - vtotal; > + /* > + * While in vblank, position will be negative > + * counting up towards 0 at vbl_end. And outside > + * vblank, position will be positive counting > + * up since vbl_end. > + */ > + if (position >= vbl_start) > + position -= vbl_end; > + else > + position += vtotal - vbl_end; > + > + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { > + *vpos = position; > + *hpos = 0; > + } else { > + *vpos = position / htotal; > + *hpos = position - (*vpos * htotal); > + } > > /* In vblank? */ > if (in_vbl) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/3] drm/i915: Fix scanoutpos calculations 2013-10-11 14:31 ` Mario Kleiner @ 2013-10-11 16:10 ` ville.syrjala 2013-10-11 23:19 ` [PATCH " Daniel Vetter 1 sibling, 0 replies; 22+ messages in thread From: ville.syrjala @ 2013-10-11 16:10 UTC (permalink / raw) To: intel-gfx; +Cc: Mario Kleiner From: Ville Syrjälä <ville.syrjala@linux.intel.com> The reported scanout position must be relative to the end of vblank. Currently we manage to fumble that in a few ways. First we don't consider the case when vtotal != vbl_end. While that isn't very common (happens maybe only w/ old panel fitting hardware), we can fix it easily enough. The second issue is that on pre-CTG hardware we convert the pixel count to horizontal/vertical components at the very beginning, and then forget to adjust the horizontal component to be relative to vbl_end. So instead we should keep our numbers in the pixel count domain while we're adjusting the position to be relative to vbl_end. Then when we do the conversion in the end, both vertical _and_ horizontal components will come out correct. v2: Change position to int from u32 to avoid sign issues Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 92a37ee..c9d2ab7 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -574,7 +574,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); const struct drm_display_mode *mode = &intel_crtc->config.adjusted_mode; - u32 position; + int position; int vbl_start, vbl_end, htotal, vtotal; bool in_vbl = true; int ret = 0; @@ -596,13 +596,7 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, /* No obvious pixelcount register. Only query vertical * scanout position from Display scan line register. */ - position = I915_READ(PIPEDSL(pipe)); - - /* Decode into vertical scanout position. Don't have - * horizontal scanout position. - */ - *vpos = position & 0x1fff; - *hpos = 0; + position = I915_READ(PIPEDSL(pipe)) & 0x1fff; } else { /* Have access to pixelcount since start of frame. * We can split this into vertical and horizontal @@ -610,15 +604,32 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, */ position = (I915_READ(PIPEFRAMEPIXEL(pipe)) & PIPE_PIXEL_MASK) >> PIPE_PIXEL_SHIFT; - *vpos = position / htotal; - *hpos = position - (*vpos * htotal); + /* convert to pixel counts */ + vbl_start *= htotal; + vbl_end *= htotal; + vtotal *= htotal; } - in_vbl = *vpos >= vbl_start && *vpos < vbl_end; + in_vbl = position >= vbl_start && position < vbl_end; - /* Inside "upper part" of vblank area? Apply corrective offset: */ - if (in_vbl && (*vpos >= vbl_start)) - *vpos = *vpos - vtotal; + /* + * While in vblank, position will be negative + * counting up towards 0 at vbl_end. And outside + * vblank, position will be positive counting + * up since vbl_end. + */ + if (position >= vbl_start) + position -= vbl_end; + else + position += vtotal - vbl_end; + + if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) { + *vpos = position; + *hpos = 0; + } else { + *vpos = position / htotal; + *hpos = position - (*vpos * htotal); + } /* In vblank? */ if (in_vbl) -- 1.8.1.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fix scanoutpos calculations 2013-10-11 14:31 ` Mario Kleiner 2013-10-11 16:10 ` [PATCH v2 " ville.syrjala @ 2013-10-11 23:19 ` Daniel Vetter 2013-10-11 23:22 ` Mario Kleiner 1 sibling, 1 reply; 22+ messages in thread From: Daniel Vetter @ 2013-10-11 23:19 UTC (permalink / raw) To: Mario Kleiner; +Cc: Daniel Vetter, intel-gfx, Mario Kleiner On Fri, Oct 11, 2013 at 04:31:38PM +0200, Mario Kleiner wrote: > Daniel, Ville, > > i tested Ville's patch series for the scanoutpos improvements on a > GMA-950, on top of airlied's current drm-next branch. > > There's one issue: The variable "position" in > i915_get_crtc_scanoutpos() must be turned from a u32 into a int, > otherwise funny sign errors happen and we end up with *vpos being > off by multiple million scanlines and timestamps being off by over > 60 seconds. > > Other than that looks good. Execution time is now better: > > Before uncore.lock addition: 3-4 usecs execution time for the > scanoutpos query on my machine. After uncore.lock addition > (3.12.0-rc3) 9-20 usecs, sometimes repetition of the timing loop > triggered. After Ville's patches down to typically 3-8 usecs, > occassionally spiking to almost 20 usecs. > > I'll make my patches for the realtime kernel + increased accuracy on > top of drm-next + Ville's patches. So official reviewed-by/tested-by from you on Ville's latest patches in this thread? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fix scanoutpos calculations 2013-10-11 23:19 ` [PATCH " Daniel Vetter @ 2013-10-11 23:22 ` Mario Kleiner 2013-10-14 15:13 ` Daniel Vetter 0 siblings, 1 reply; 22+ messages in thread From: Mario Kleiner @ 2013-10-11 23:22 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, Mario Kleiner [-- Attachment #1.1: Type: text/plain, Size: 1287 bytes --] Yes. On Oct 12, 2013 1:18 AM, "Daniel Vetter" <daniel@ffwll.ch> wrote: > > On Fri, Oct 11, 2013 at 04:31:38PM +0200, Mario Kleiner wrote: > > Daniel, Ville, > > > > i tested Ville's patch series for the scanoutpos improvements on a > > GMA-950, on top of airlied's current drm-next branch. > > > > There's one issue: The variable "position" in > > i915_get_crtc_scanoutpos() must be turned from a u32 into a int, > > otherwise funny sign errors happen and we end up with *vpos being > > off by multiple million scanlines and timestamps being off by over > > 60 seconds. > > > > Other than that looks good. Execution time is now better: > > > > Before uncore.lock addition: 3-4 usecs execution time for the > > scanoutpos query on my machine. After uncore.lock addition > > (3.12.0-rc3) 9-20 usecs, sometimes repetition of the timing loop > > triggered. After Ville's patches down to typically 3-8 usecs, > > occassionally spiking to almost 20 usecs. > > > > I'll make my patches for the realtime kernel + increased accuracy on > > top of drm-next + Ville's patches. > > So official reviewed-by/tested-by from you on Ville's latest patches in > this thread? Yes. -mario > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch [-- Attachment #1.2: Type: text/html, Size: 1787 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] drm/i915: Fix scanoutpos calculations 2013-10-11 23:22 ` Mario Kleiner @ 2013-10-14 15:13 ` Daniel Vetter 0 siblings, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2013-10-14 15:13 UTC (permalink / raw) To: Mario Kleiner; +Cc: intel-gfx, Mario Kleiner, Daniel Vetter On Sat, Oct 12, 2013 at 01:22:40AM +0200, Mario Kleiner wrote: > Yes. > On Oct 12, 2013 1:18 AM, "Daniel Vetter" <daniel@ffwll.ch> wrote: > > > > On Fri, Oct 11, 2013 at 04:31:38PM +0200, Mario Kleiner wrote: > > > Daniel, Ville, > > > > > > i tested Ville's patch series for the scanoutpos improvements on a > > > GMA-950, on top of airlied's current drm-next branch. > > > > > > There's one issue: The variable "position" in > > > i915_get_crtc_scanoutpos() must be turned from a u32 into a int, > > > otherwise funny sign errors happen and we end up with *vpos being > > > off by multiple million scanlines and timestamps being off by over > > > 60 seconds. > > > > > > Other than that looks good. Execution time is now better: > > > > > > Before uncore.lock addition: 3-4 usecs execution time for the > > > scanoutpos query on my machine. After uncore.lock addition > > > (3.12.0-rc3) 9-20 usecs, sometimes repetition of the timing loop > > > triggered. After Ville's patches down to typically 3-8 usecs, > > > occassionally spiking to almost 20 usecs. > > > > > > I'll make my patches for the realtime kernel + increased accuracy on > > > top of drm-next + Ville's patches. > > > > So official reviewed-by/tested-by from you on Ville's latest patches in > > this thread? > > Yes. Ok, slurped in the entire series, thanks for the review and testing. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ 2013-09-23 10:02 [PATCH 0/3] drm/i915: i915_get_crtc_scanoutpos improvements ville.syrjala 2013-09-23 10:02 ` [PATCH 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() ville.syrjala 2013-09-23 10:02 ` [PATCH 2/3] drm/i915: Fix scanoutpos calculations ville.syrjala @ 2013-09-23 10:02 ` ville.syrjala 2013-09-25 3:12 ` Mario Kleiner 2 siblings, 1 reply; 22+ messages in thread From: ville.syrjala @ 2013-09-23 10:02 UTC (permalink / raw) To: intel-gfx; +Cc: Mario Kleiner From: Ville Syrjälä <ville.syrjala@linux.intel.com> The DSL register increments at the start of horizontal sync, so it manages to miss the entire active portion of the current line. Improve the get_scanoutpos accuracy a bit when the scanout position is close to the start or end of vblank. We can do that by double checking the DSL value against the vblank status bit from ISR. Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 53 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 4f74f0c..14b42d9 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -567,6 +567,47 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) return I915_READ(reg); } +static bool g4x_pipe_in_vblank(struct drm_device *dev, enum pipe pipe) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + uint32_t status; + + if (IS_VALLEYVIEW(dev)) { + status = pipe == PIPE_A ? + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; + + return I915_READ(VLV_ISR) & status; + } else if (IS_G4X(dev)) { + status = pipe == PIPE_A ? + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; + + return I915_READ(ISR) & status; + } else if (INTEL_INFO(dev)->gen < 7) { + status = pipe == PIPE_A ? + DE_PIPEA_VBLANK : + DE_PIPEB_VBLANK; + + return I915_READ(DEISR) & status; + } else { + switch (pipe) { + default: + case PIPE_A: + status = DE_PIPEA_VBLANK_IVB; + break; + case PIPE_B: + status = DE_PIPEB_VBLANK_IVB; + break; + case PIPE_C: + status = DE_PIPEC_VBLANK_IVB; + break; + } + + return I915_READ(DEISR) & status; + } +} + static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, int *vpos, int *hpos) { @@ -616,6 +657,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, * scanout position from Display scan line register. */ position = I915_READ(PIPEDSL(pipe)) & 0x1fff; + + /* + * 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 = g4x_pipe_in_vblank(dev, pipe); + if ((in_vbl && position == vbl_start - 1) || + (!in_vbl && position == vbl_end - 1)) + position = (position + 1) % vtotal; } 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ 2013-09-23 10:02 ` [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ ville.syrjala @ 2013-09-25 3:12 ` Mario Kleiner 2013-09-25 8:11 ` Ville Syrjälä 0 siblings, 1 reply; 22+ messages in thread From: Mario Kleiner @ 2013-09-25 3:12 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx, Mario Kleiner On 23.09.13 12:02, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The DSL register increments at the start of horizontal sync, so it > manages to miss the entire active portion of the current line. > > Improve the get_scanoutpos accuracy a bit when the scanout position is > close to the start or end of vblank. We can do that by double checking > the DSL value against the vblank status bit from ISR. > > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_irq.c | 53 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 4f74f0c..14b42d9 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -567,6 +567,47 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) > return I915_READ(reg); > } > > +static bool g4x_pipe_in_vblank(struct drm_device *dev, enum pipe pipe) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + uint32_t status; > + > + if (IS_VALLEYVIEW(dev)) { > + status = pipe == PIPE_A ? > + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : > + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; > + > + return I915_READ(VLV_ISR) & status; > + } else if (IS_G4X(dev)) { > + status = pipe == PIPE_A ? > + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : > + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; > + > + return I915_READ(ISR) & status; > + } else if (INTEL_INFO(dev)->gen < 7) { > + status = pipe == PIPE_A ? > + DE_PIPEA_VBLANK : > + DE_PIPEB_VBLANK; > + > + return I915_READ(DEISR) & status; > + } else { > + switch (pipe) { > + default: > + case PIPE_A: > + status = DE_PIPEA_VBLANK_IVB; > + break; > + case PIPE_B: > + status = DE_PIPEB_VBLANK_IVB; > + break; > + case PIPE_C: > + status = DE_PIPEC_VBLANK_IVB; > + break; > + } > + > + return I915_READ(DEISR) & status; > + } > +} > + > static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > int *vpos, int *hpos) > { > @@ -616,6 +657,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > * scanout position from Display scan line register. > */ > position = I915_READ(PIPEDSL(pipe)) & 0x1fff; > + > + /* > + * 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 = g4x_pipe_in_vblank(dev, pipe); > + if ((in_vbl && position == vbl_start - 1) || > + (!in_vbl && position == vbl_end - 1)) > + position = (position + 1) % vtotal; > } else { > /* Have access to pixelcount since start of frame. > * We can split this into vertical and horizontal > This one i don't know. I think i can't follow the logic, but i don't know enough about the way the intel hw counts. Do you mean the counter increments when the scanline is over, instead of when it begins? With this correction by +1 at the edges of vblank, the scanlines at vbl_start and vbl_end would be reported twice, for two successive scanline durations, that seems a bit weird and asymmetric to the rest of the scanline positions. Wouldn't it make more sense to simply always add 1 for a smaller overall error, given that hblank is shorter than the active scanout part of a scanline? Also it adds back one lock protected, therefore potentially slow, register read into the time critical code. -mario _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ 2013-09-25 3:12 ` Mario Kleiner @ 2013-09-25 8:11 ` Ville Syrjälä 2013-09-25 8:46 ` Daniel Vetter 2013-09-26 17:04 ` Mario Kleiner 0 siblings, 2 replies; 22+ messages in thread From: Ville Syrjälä @ 2013-09-25 8:11 UTC (permalink / raw) To: Mario Kleiner; +Cc: intel-gfx On Wed, Sep 25, 2013 at 05:12:54AM +0200, Mario Kleiner wrote: > > > On 23.09.13 12:02, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The DSL register increments at the start of horizontal sync, so it > > manages to miss the entire active portion of the current line. > > > > Improve the get_scanoutpos accuracy a bit when the scanout position is > > close to the start or end of vblank. We can do that by double checking > > the DSL value against the vblank status bit from ISR. > > > > Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_irq.c | 53 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 53 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 4f74f0c..14b42d9 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -567,6 +567,47 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) > > return I915_READ(reg); > > } > > > > +static bool g4x_pipe_in_vblank(struct drm_device *dev, enum pipe pipe) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + uint32_t status; > > + > > + if (IS_VALLEYVIEW(dev)) { > > + status = pipe == PIPE_A ? > > + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : > > + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; > > + > > + return I915_READ(VLV_ISR) & status; > > + } else if (IS_G4X(dev)) { > > + status = pipe == PIPE_A ? > > + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : > > + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; > > + > > + return I915_READ(ISR) & status; > > + } else if (INTEL_INFO(dev)->gen < 7) { > > + status = pipe == PIPE_A ? > > + DE_PIPEA_VBLANK : > > + DE_PIPEB_VBLANK; > > + > > + return I915_READ(DEISR) & status; > > + } else { > > + switch (pipe) { > > + default: > > + case PIPE_A: > > + status = DE_PIPEA_VBLANK_IVB; > > + break; > > + case PIPE_B: > > + status = DE_PIPEB_VBLANK_IVB; > > + break; > > + case PIPE_C: > > + status = DE_PIPEC_VBLANK_IVB; > > + break; > > + } > > + > > + return I915_READ(DEISR) & status; > > + } > > +} > > + > > static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > > int *vpos, int *hpos) > > { > > @@ -616,6 +657,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > > * scanout position from Display scan line register. > > */ > > position = I915_READ(PIPEDSL(pipe)) & 0x1fff; > > + > > + /* > > + * 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 = g4x_pipe_in_vblank(dev, pipe); > > + if ((in_vbl && position == vbl_start - 1) || > > + (!in_vbl && position == vbl_end - 1)) > > + position = (position + 1) % vtotal; > > } else { > > /* Have access to pixelcount since start of frame. > > * We can split this into vertical and horizontal > > > > This one i don't know. I think i can't follow the logic, but i don't > know enough about the way the intel hw counts. > > Do you mean the counter increments when the scanline is over, instead of > when it begins? Let me draw a picture of the scanline (not to scale): |XXXXXXXXXXXXX|-----|___________|---| horiz. active horiz. sync ^ ^ | | first pixel this is where the of the line scanline counter increments > With this correction by +1 at the edges of vblank, the scanlines at > vbl_start and vbl_end would be reported twice, for two successive > scanline durations, that seems a bit weird and asymmetric to the rest of > the scanline positions. Wouldn't it make more sense to simply always add > 1 for a smaller overall error, given that hblank is shorter than the > active scanout part of a scanline? Since the counter increments too late, drm_handle_vblank() may get the wrong idea ie. something like this may happen: 1. vblank irq triggered 2. drm_handle_vblank() gets called 3. i915_get_crtc_scanoutpos() returns vbl_start-1 as the scanline 4. delta_ns calculation gets confused and tries to correct for it Now, the correction you do for delta_ns should handle this, but I don't like having such kludges in common code, and we can handle it in the driver as I've demonstrated. But yeah, I suppose it can make the error slightly less stable. For some other uses (atomic page flip stuff) of the scanline position, I definitely want this correction since I need accurate information whether the position has passed vblank start. > Also it adds back one lock protected, therefore potentially slow, > register read into the time critical code. I don't think a single register read should be _that_ slow even with all the extra junk we do. And of course we can fix that problem. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ 2013-09-25 8:11 ` Ville Syrjälä @ 2013-09-25 8:46 ` Daniel Vetter 2013-09-26 17:04 ` Mario Kleiner 1 sibling, 0 replies; 22+ messages in thread From: Daniel Vetter @ 2013-09-25 8:46 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Mario Kleiner On Wed, Sep 25, 2013 at 11:11:30AM +0300, Ville Syrjälä wrote: > On Wed, Sep 25, 2013 at 05:12:54AM +0200, Mario Kleiner wrote: > > This one i don't know. I think i can't follow the logic, but i don't > > know enough about the way the intel hw counts. > > > > Do you mean the counter increments when the scanline is over, instead of > > when it begins? > > Let me draw a picture of the scanline (not to scale): > > |XXXXXXXXXXXXX|-----|___________|---| > horiz. active horiz. sync > ^ ^ > | | > first pixel this is where the > of the line scanline counter increments > > > With this correction by +1 at the edges of vblank, the scanlines at > > vbl_start and vbl_end would be reported twice, for two successive > > scanline durations, that seems a bit weird and asymmetric to the rest of > > the scanline positions. Wouldn't it make more sense to simply always add > > 1 for a smaller overall error, given that hblank is shorter than the > > active scanout part of a scanline? > > Since the counter increments too late, drm_handle_vblank() > may get the wrong idea ie. something like this may happen: > > 1. vblank irq triggered > 2. drm_handle_vblank() gets called > 3. i915_get_crtc_scanoutpos() returns vbl_start-1 as the scanline > 4. delta_ns calculation gets confused and tries to correct for it > > Now, the correction you do for delta_ns should handle this, but > I don't like having such kludges in common code, and we can handle > it in the driver as I've demonstrated. But yeah, I suppose it can > make the error slightly less stable. > > For some other uses (atomic page flip stuff) of the scanline position, > I definitely want this correction since I need accurate information > whether the position has passed vblank start. At least on modern platforms I think we should take a good look at the timestamp registers. With those the only thing we essentially need to do is to correct the spot where the timestamp is taken to make it suitable for OML_sync ... But doing such a switch is something for future work ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ 2013-09-25 8:11 ` Ville Syrjälä 2013-09-25 8:46 ` Daniel Vetter @ 2013-09-26 17:04 ` Mario Kleiner 1 sibling, 0 replies; 22+ messages in thread From: Mario Kleiner @ 2013-09-26 17:04 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Mario Kleiner On 25.09.13 10:11, Ville Syrjälä wrote: > On Wed, Sep 25, 2013 at 05:12:54AM +0200, Mario Kleiner wrote: >> >> >> On 23.09.13 12:02, ville.syrjala@linux.intel.com wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> The DSL register increments at the start of horizontal sync, so it >>> manages to miss the entire active portion of the current line. >>> >>> Improve the get_scanoutpos accuracy a bit when the scanout position is >>> close to the start or end of vblank. We can do that by double checking >>> the DSL value against the vblank status bit from ISR. >>> >>> Cc: Mario Kleiner <mario.kleiner@tuebingen.mpg.de> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_irq.c | 53 +++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 53 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >>> index 4f74f0c..14b42d9 100644 >>> --- a/drivers/gpu/drm/i915/i915_irq.c >>> +++ b/drivers/gpu/drm/i915/i915_irq.c >>> @@ -567,6 +567,47 @@ static u32 gm45_get_vblank_counter(struct drm_device *dev, int pipe) >>> return I915_READ(reg); >>> } >>> >>> +static bool g4x_pipe_in_vblank(struct drm_device *dev, enum pipe pipe) >>> +{ >>> + struct drm_i915_private *dev_priv = dev->dev_private; >>> + uint32_t status; >>> + >>> + if (IS_VALLEYVIEW(dev)) { >>> + status = pipe == PIPE_A ? >>> + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : >>> + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; >>> + >>> + return I915_READ(VLV_ISR) & status; >>> + } else if (IS_G4X(dev)) { >>> + status = pipe == PIPE_A ? >>> + I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT : >>> + I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT; >>> + >>> + return I915_READ(ISR) & status; >>> + } else if (INTEL_INFO(dev)->gen < 7) { >>> + status = pipe == PIPE_A ? >>> + DE_PIPEA_VBLANK : >>> + DE_PIPEB_VBLANK; >>> + >>> + return I915_READ(DEISR) & status; >>> + } else { >>> + switch (pipe) { >>> + default: >>> + case PIPE_A: >>> + status = DE_PIPEA_VBLANK_IVB; >>> + break; >>> + case PIPE_B: >>> + status = DE_PIPEB_VBLANK_IVB; >>> + break; >>> + case PIPE_C: >>> + status = DE_PIPEC_VBLANK_IVB; >>> + break; >>> + } >>> + >>> + return I915_READ(DEISR) & status; >>> + } >>> +} >>> + >>> static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, >>> int *vpos, int *hpos) >>> { >>> @@ -616,6 +657,18 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, >>> * scanout position from Display scan line register. >>> */ >>> position = I915_READ(PIPEDSL(pipe)) & 0x1fff; >>> + >>> + /* >>> + * 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 = g4x_pipe_in_vblank(dev, pipe); >>> + if ((in_vbl && position == vbl_start - 1) || >>> + (!in_vbl && position == vbl_end - 1)) >>> + position = (position + 1) % vtotal; >>> } else { >>> /* Have access to pixelcount since start of frame. >>> * We can split this into vertical and horizontal >>> >> >> This one i don't know. I think i can't follow the logic, but i don't >> know enough about the way the intel hw counts. >> >> Do you mean the counter increments when the scanline is over, instead of >> when it begins? > > Let me draw a picture of the scanline (not to scale): > > |XXXXXXXXXXXXX|-----|___________|---| > horiz. active horiz. sync > ^ ^ > | | > first pixel this is where the > of the line scanline counter increments > >> With this correction by +1 at the edges of vblank, the scanlines at >> vbl_start and vbl_end would be reported twice, for two successive >> scanline durations, that seems a bit weird and asymmetric to the rest of >> the scanline positions. Wouldn't it make more sense to simply always add >> 1 for a smaller overall error, given that hblank is shorter than the >> active scanout part of a scanline? > > Since the counter increments too late, drm_handle_vblank() > may get the wrong idea ie. something like this may happen: > > 1. vblank irq triggered > 2. drm_handle_vblank() gets called > 3. i915_get_crtc_scanoutpos() returns vbl_start-1 as the scanline > 4. delta_ns calculation gets confused and tries to correct for it > > Now, the correction you do for delta_ns should handle this, but > I don't like having such kludges in common code, and we can handle > it in the driver as I've demonstrated. But yeah, I suppose it can > make the error slightly less stable. > The kludges are also needed for other drivers, especially some radeon gpu's which can fire their vblank interrupts multiple scanlines before the vblank, and iirc nouveau with the prototype patches we had. I like that catch-all for robustness, especially on gpu's whose behaviour is not that well documented as in case of intel, but of course it's better if a driver does the right thing from the start. > For some other uses (atomic page flip stuff) of the scanline position, > I definitely want this correction since I need accurate information > whether the position has passed vblank start. > Ok, i can live with that. >> Also it adds back one lock protected, therefore potentially slow, >> register read into the time critical code. > > I don't think a single register read should be _that_ slow even > with all the extra junk we do. And of course we can fix that problem. > Ja. If you make g4x_pipe_in_vblank() or maybe a helper to just return the register and status mask, we can do the actual register read also as a __raw read together with the raw read of scanout position regs, inside that ktime_get() enclosed section. Then it's cheap and rt compatible and done with that one uncore.lock lock/unlock and everybody's happy. -mario ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-10-14 15:12 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-23 10:02 [PATCH 0/3] drm/i915: i915_get_crtc_scanoutpos improvements ville.syrjala 2013-09-23 10:02 ` [PATCH 1/3] drm/i915: Skip register reads in i915_get_crtc_scanoutpos() ville.syrjala 2013-09-23 10:30 ` Chris Wilson 2013-09-23 11:48 ` [PATCH v2 " ville.syrjala 2013-09-24 9:11 ` Daniel Vetter 2013-09-25 4:45 ` Mario Kleiner 2013-09-25 7:52 ` Daniel Vetter 2013-09-25 2:35 ` Mario Kleiner 2013-09-25 8:14 ` Ville Syrjälä 2013-09-26 16:12 ` Mario Kleiner 2013-09-23 10:02 ` [PATCH 2/3] drm/i915: Fix scanoutpos calculations ville.syrjala 2013-09-25 2:39 ` Mario Kleiner 2013-10-11 14:31 ` Mario Kleiner 2013-10-11 16:10 ` [PATCH v2 " ville.syrjala 2013-10-11 23:19 ` [PATCH " Daniel Vetter 2013-10-11 23:22 ` Mario Kleiner 2013-10-14 15:13 ` Daniel Vetter 2013-09-23 10:02 ` [PATCH 3/3] drm/i915: Improve the accuracy of get_scanout_pos on CTG+ ville.syrjala 2013-09-25 3:12 ` Mario Kleiner 2013-09-25 8:11 ` Ville Syrjälä 2013-09-25 8:46 ` Daniel Vetter 2013-09-26 17:04 ` Mario Kleiner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox