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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Vidya Srinivas <vidya.srinivas@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
Date: Fri, 8 Sep 2017 17:55:24 +0300	[thread overview]
Message-ID: <20170908145524.GI4914@intel.com> (raw)
In-Reply-To: <20170908144759.GH4914@intel.com>

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.

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx