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

["Shankar, Uma" <uma.shankar@intel.com>]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, September 11, 2017 11:51 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: Daniel Vetter <daniel@ffwll.ch>; intel-gfx@lists.freedesktop.org; Srinivas,
>Vidya <vidya.srinivas@intel.com>
>Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Mon, Sep 11, 2017 at 01:19:15PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>> >Behalf Of Daniel Vetter
>> >Sent: Saturday, September 9, 2017 1:15 AM
>> >To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya
>> ><vidya.srinivas@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@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.
>> >
>>
>> 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.
>

Thanks Ville for this info. Will update the Current Timestamp register for Gen4 and Gen7 (declare
as separate macros). Also will define Frame Timestamp  generically.

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