[Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi

Mon Sep 11 18:21:01 UTC 2017

On Mon, Sep 11, 2017 at 01:19:15PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> >Daniel Vetter
> >Sent: Saturday, September 9, 2017 1:15 AM
> >To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >Cc: intel-gfx at lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas at intel.com>
> >Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi
> >
> >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 at 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 at intel.com>
> >> > > Signed-off-by: Chandra Konduru <chandra.konduru at intel.com>
> >> > > Signed-off-by: Vidya Srinivas <vidya.srinivas at 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.
> >
> 
> As per spec, it says BDW+. Will rename them using BDW as prefix.

You're not looking at the right spec then. Looks like ctg/elk is
the right answer indeed. The gen4 spec is a bit confused in that it
doesn't correctly state whether some of the regs apply to gen4 or
ctg/elk. But testing on actual gen4 hardware gives me 0s from all the
pipe timestamp registers, whereas elk gives sane looking value. The
TIMESTAMP register did exist on gen4 already.

There have been some changes in the TIMESTAMP register(s) throughout
the years though. I'll try to summarize below:

bw/cl: TIMESTAMP/0x2358. 64bit register that increments every 16 hclks
ctg/elk: TIMESTAMP/0x2358. 64bit register where the upper 32 bits increment
         every 1.024us, the lower part has more 12 bits which means the whole
         value increments every 1/4 ns
ilk/snb: TIMESTAMP/0x2358 "64bit" register where the upprt 32 bits increment
         every 1 us. Lower part is documented as MBZ. The upprt part is
         aliased as TIMESTAMP_HI high at offset 0x70070
ivb+: TIMESTAMP_HI got renamed to TIMESTAMP_CTR and moved to 0x44070

The pipe flip/frame timestamp registers seem to have remained
unchanged ever since ctg/elk.

-- 
Ville Syrjälä
Intel OTC