On Wed, Sep 25, 2013 at 05:12:54AM +0200, Mario Kleiner wrote:
>
>
> On 23.09.13 12:02, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
