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

Mon Sep 11 13:19:15 UTC 2017

>-----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.

>> 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.

We tried to use kms_plane_multiple test from IGT to validate this. Atomic update failure
can easily be reproduced using this. With the above approach, we are not seeing those
failures. Also to try to check for traditional method of detecting scanline (using Pipe scanline)
which works for DDI interfaces. We tried to compare the results from both ways, readback from 
this register and the calculated one from timestamps on HDMI. The results were pretty similar with a delta of 
1. We will try to do more experiments and stress test to get more confidence on the timestamp method.
But till now, it looks pretty promising.

If we increase evade time, it may have an adverse impact on fps. So if scanline reported gives promising
data (similar to what pipe scanline register returns), I think we should stick to that approach.

Regards,
Uma Shankar

>-Daniel
>--
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx