Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	Vidya Srinivas <vidya.srinivas@intel.com>
Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
Date: Fri, 8 Sep 2017 21:45:11 +0200	[thread overview]
Message-ID: <20170908194511.3riwekli6zfatt4e@phenom.ffwll.local> (raw)
In-Reply-To: <20170908145524.GI4914@intel.com>

On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote:
> On Fri, Sep 08, 2017 at 05:47:59PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 08, 2017 at 07:18:55PM +0530, Vidya Srinivas wrote:
> > > From: Uma Shankar <uma.shankar@intel.com>
> > > 
> > > For gen9 platforms, dsi timings are driven from port instead of pipe
> > > (unlike ddi). Thus, we can't rely on pipe registers to get the timing
> > > information. Even scanline register read will not be functional.
> > > This is causing vblank evasion logic to fail since it relies on
> > > scanline, causing atomic update failure warnings.
> > > 
> > > This patch uses pipe framestamp and current timestamp registers
> > > to calculate scanline. This is an indirect way to get the scanline.
> > > It helps resolve atomic update failure for gen9 dsi platforms.
> > > 
> > > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
> > >  drivers/gpu/drm/i915/i915_irq.c  |  5 +++++
> > >  drivers/gpu/drm/i915/i915_reg.h  |  3 +++
> > >  drivers/gpu/drm/i915/intel_dsi.c | 46 ++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 56 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index d07d110..4213b54 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -4077,6 +4077,8 @@ void intel_sbi_write(struct drm_i915_private *dev_priv, u16 reg, u32 value,
> > >  u32 vlv_flisdsi_read(struct drm_i915_private *dev_priv, u32 reg);
> > >  void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
> > >  
> > > +u32 bxt_dsi_get_scanline(struct intel_crtc *crtc);
> > > +
> > >  /* intel_dpio_phy.c */
> > >  void bxt_port_to_phy_channel(struct drm_i915_private *dev_priv, enum port port,
> > >  			     enum dpio_phy *phy, enum dpio_channel *ch);
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 5d391e6..31aa7f0 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -781,6 +781,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> > >  	struct drm_vblank_crtc *vblank;
> > >  	enum pipe pipe = crtc->pipe;
> > >  	int position, vtotal;
> > > +	enum transcoder cpu_transcoder;
> > >  
> > >  	if (!crtc->active)
> > >  		return -1;
> > > @@ -792,6 +793,10 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> > >  	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > >  		vtotal /= 2;
> > >  
> > > +	cpu_transcoder = crtc->config->cpu_transcoder;
> > 
> > Humm. Would be nice to be able to do this without adding more
> > crtc->config uses. We're pretty much trying to get rid of that guy.
> > 
> > > +	if (IS_BROXTON(dev_priv) && transcoder_is_dsi(cpu_transcoder))
> > > +		return bxt_dsi_get_scanline(crtc);
> > > +
> > >  	if (IS_GEN2(dev_priv))
> > >  		position = I915_READ_FW(PIPEDSL(pipe)) & DSL_LINEMASK_GEN2;
> > >  	else
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 9a73ea0..54582de 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -8802,6 +8802,9 @@ enum skl_power_gate {
> > >  #define MIPIO_TXESC_CLK_DIV2			_MMIO(0x160008)
> > >  #define  GLK_TX_ESC_CLK_DIV2_MASK			0x3FF
> > >  
> > > +#define BXT_TIMESTAMP_CTR	_MMIO(0x44070)
> > > +#define BXT_PIPE_FRMTMSTMP_A	_MMIO(0x70048)
> > 
> > Please add proper parametrized define that works for all pipes.
> 
> Oh, and these shouldn't be called BXT_something. I don't recall when
> they got added to the hardware, but I'm pretty sure it was way before
> BXT came out.

gen5 or maybe gen4.5 iirc.

> Another thought that just occurred to me: Maybe we could use these
> timestamps as a workaround for the DDI "scanline reads as 0 at the
> wrong time" problem. What we could do is check of the scanline counter
> reads as 0, and if it does we could switch over to checking the
> timestamps instead. Not sure if we should just do the full timestamp
> based scanline read like you do here, or we could just check that if the
> timestamps look like they're close to vblank_start we just return
> vblank_start-1. This could then remove the obnoxious retry loop from the
> scanline counter read.

Another concern I have on this is timeframe jitter. If the vblank
timestamp stuff isnt' perfectly accurately spaced, or we have a mismatch
in clocks, then we might think there's still plenty of time before vblank
while we're already racing.

Just a bit of testing won't catch that all that easily unfortunately, and
I'm not sure we have any good igts to stress-test this stuff. I'd advocate
we're playing it defensive and increase the vblank evasion window for this
trick quite a bit to stay on the safe side (maybe 1 full ms or so). Or
much, much better testing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx