[Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi
Shankar, Uma
uma.shankar at intel.com
Wed Sep 13 08:24:38 UTC 2017
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>Sent: Tuesday, September 12, 2017 8:36 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: intel-gfx at lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas at intel.com>
>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>
>On Tue, Sep 12, 2017 at 02:21:42PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> >Sent: Tuesday, September 12, 2017 7:43 PM
>> >To: Shankar, Uma <uma.shankar at intel.com>
>> >Cc: intel-gfx at lists.freedesktop.org; Srinivas, Vidya
>> ><vidya.srinivas at intel.com>
>> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >
>> >On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote:
>> >>
>> >>
>> >> >-----Original Message-----
>> >> >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> >> >Sent: Tuesday, September 12, 2017 7:04 PM
>> >> >To: Shankar, Uma <uma.shankar at intel.com>
>> >> >Cc: intel-gfx at lists.freedesktop.org; Srinivas, Vidya
>> >> ><vidya.srinivas at intel.com>
>> >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >> >
>> >> >> >>>
>> >> >> >>> >-----Original Message-----
>> >> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> >> >> >>> >Sent: Friday, September 8, 2017 8:18 PM
>> >> >> >>> >To: Srinivas, Vidya <vidya.srinivas at intel.com>
>> >> >> >>> >Cc: intel-gfx at lists.freedesktop.org; Kahola, Mika
>> >> >> >>> ><mika.kahola at intel.com>; Kamath, Sunil
>> >> >> >>> ><sunil.kamath at intel.com>; Shankar, Uma
>> >> >> >>> ><uma.shankar at intel.com>; Konduru, Chandra
>> >> >> >>> ><chandra.konduru at intel.com>
>> >> >> >>> >Subject: Re: [PATCH] drm/i915: Enable scanline read for
>> >> >> >>> >gen9 dsi
>> >> >> >>> >
>> >> >> >>> >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.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Will try to find an alternate way to do this.
>> >> >> >>>
>> >> >> >>> >> + 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.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Will add that.
>> >> >> >>>
>> >> >> >>> >> +
>> >> >> >>> >> /* BXT MIPI clock controls */
>> >> >> >>> >> #define BXT_MAX_VAR_OUTPUT_KHZ 39500
>> >> >> >>> >>
>> >> >> >>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> b/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> index 2a0f5d3..d145ba4 100644
>> >> >> >>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >> >> >>> >> @@ -1621,6 +1621,52 @@ static int
>> >> >> >>> >> intel_dsi_get_modes(struct
>> >> >> >>> >drm_connector *connector)
>> >> >> >>> >> return 1;
>> >> >> >>> >> }
>> >> >> >>> >>
>> >> >> >>> >> +/*
>> >> >> >>> >> + * For Gen9 DSI, pipe scanline register will not
>> >> >> >>> >> + * work to get the scanline since the timings
>> >> >> >>> >> + * are driven from the PORT (unlike DDI encoders).
>> >> >> >>> >> + * This function will use Framestamp and current
>> >> >> >>> >> + * timestamp registers to calculate the scanline.
>> >> >> >>> >> + */
>> >> >> >>> >> +u32 bxt_dsi_get_scanline(struct intel_crtc *crtc) {
>> >> >> >>> >> + struct drm_device *dev = crtc->base.dev;
>> >> >> >>> >> + struct drm_i915_private *dev_priv = to_i915(dev);
>> >> >> >>> >> + u32 vrefresh = crtc->base.mode.vrefresh;
>> >> >> >>> >> + u32 ulPrevTime, ulCurrTime, vtotal, ulScanlineNo2 = 0;
>> >> >> >>> >
>> >> >> >>> >Please get rid of the hungarian notation.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Yes, will fix this.
>> >> >> >>>
>> >> >> >>> >> + uint_fixed_16_16_t ulScanlineTime;
>> >> >> >>> >> +
>> >> >> >>> >> + /*
>> >> >> >>> >> + * This field provides read back of the display
>> >> >> >>> >> + * pipe frame time stamp. The time stamp value
>> >> >> >>> >> + * is sampled at every start of vertical blank.
>> >> >> >>> >> + */
>> >> >> >>> >> + ulPrevTime =
>I915_READ_FW(BXT_PIPE_FRMTMSTMP_A);
>> >> >> >>> >> +
>> >> >> >>> >> + /*
>> >> >> >>> >> + * The TIMESTAMP_CTR register has the current
>> >> >> >>> >> + * time stamp value.
>> >> >> >>> >> + */
>> >> >> >>> >> + ulCurrTime = I915_READ_FW(BXT_TIMESTAMP_CTR);
>> >> >> >>> >> +
>> >> >> >>> >> + /* The PORT for DSI will always be 0 since
>> >> >> >>> >> + * isolated PORTC cannot be enabled for Gen9
>> >> >> >>> >> + * DSI. Hence using PORT_A i.e 0 to extract
>> >> >> >>> >> + * the VTOTAL value.
>> >> >> >>> >> + */
>> >> >> >>> >> + vtotal = I915_READ_FW(BXT_MIPI_TRANS_VTOTAL(0));
>> >> >> >>> >
>> >> >> >>> >This value can be dug out from the hwmode.
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Yes, will get it from hwmode and drop this change.
>> >> >> >>>
>> >> >> >>> >> + WARN_ON(!vtotal);
>> >> >> >>> >> + if (!vtotal)
>> >> >> >>> >> + return ulScanlineNo2;
>> >> >> >>> >> +
>> >> >> >>> >> + ulScanlineTime = div_fixed16(1000000, vtotal *
>vrefresh);
>> >> >> >>> >> + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime
>-
>> >> >ulPrevTime),
>> >> >> >>> >> +
> ulScanlineTime);
>> >> >> >>> >
>> >> >> >>> >Something like:
>> >> >> >>> >scanline = div_u64(mul_u32_u32(curr - prev, crtc_clock),
>> >> >> >>> > 1000 * crtc_htotal);
>> >> >> >>> >
>> >> >> >>> >> + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal;
>> >> >> >>> >
>> >> >> >>> >I think that would have to be something like:
>> >> >> >>> >return (scanline + vblank_start) % vtotal;
>> >> >> >>> >
>> >> >> >>>
>> >> >> >>> Yes you are right. It should be vblank_start. Will fix this.
>> >> >> >>>
>> >> >> >>> >All in all this looks like a pretty decent approach to the DSI problem.
>> >> >> >>> >
>> >> >> >>> >One concern here is rounding issues and inaccuracies in our
>> >> >> >>> >crtc_clock. But since the frame timestamp is sampled at
>> >> >> >>> >vblank start I guess we can't accidentally get an answer
>> >> >> >>> >that's earlier than vblank_start as long as we really
>> >> >> >>> >passed vblank start already. That should
>> >> >> >>make this at least suitable for vblank timestamps.
>> >> >> >>>
>> >> >> >>> I also feel the same, this situation should never occur.
>> >> >> >>>
>> >> >> >>> >And for
>> >> >> >>> >the atomic evade, I guess if we clamp our the scanline
>> >> >> >>> >before the
>> >> >> >>> >+vblank_start such that it never reaches vtotal, we can't
>> >> >> >>> >+be sure that
>> >> >> >>> >our vblank evade never indicates that we already reached
>> >> >> >>> >the start of vblank prematurely.
>> >> >> >>> >
>> >> >> >>> >So maybe something like:
>> >> >> >>> >scaline = div_u64(...);
>> >> >> >>> >scanline = min(scanline, vtotal - 1);
>> >> >> >>>
>> >> >> >>> I am not sure if the value of scanline returned can ever be
>> >> >> >>> greater than vtotal -
>> >> >> >>1.
>> >> >> >>> But we can have a check just to be safe. Not sure if I fully
>> >> >> >>> got your point
>> >> >here.
>> >> >> >>
>> >> >> >>The point is that the timestamp counter might tick at a
>> >> >> >>slightly faster rate than we might think. Thus we might end up
>> >> >> >>with more ticks in one frame than what we calculated as the
>> >> >> >>maximum fom crtc_clock etc. But if we clamp the value like I
>> >> >> >>suggested then at least we should never get an answer that
>> >> >> >>tells us we're already past the start of vblank when in
>> >> >> >>reality
>> >> >> >we're not.
>> >> >> >>
>> >> >> >>Of course as Daniel pointed out we might also get into trouble
>> >> >> >>if the counter ticks slower than expected. That could lead us
>> >> >> >>to think that we don't need to do the vblank evade when in fact we do.
>> >> >> >>
>> >> >>
>> >> >> Hi Ville,
>> >> >> We tried to test with this condition and are calculating wrong scanlines.
>> >> >> For ex:
>> >> >> [ 79.418943] [drm:bxt_dsi_get_scanline] *ERROR* scanline = 22534,
>> >> >crtc_vtotal-1 = 1211, min of two = 1211
>> >> >
>> >> >Well, that scanline number looks totally bogus. How did you
>> >> >calculate it
>> >exactly?
>> >> >
>> >>
>> >> If we have multiple scans on the same frame (no new flip being
>> >> issued). Prev timestamp value which is read from Frametime Stamp
>> >> will remain same, but current time stamp will keep on incrementing.
>> >
>> >The frame timestamp should get sampled on every vblank, whereas the
>> >flip timestamp only when a flip occurs. Are you using the correct timestamp
>register?
>> >
>>
>> Yes, we are using what is there in the patch.
>> Name Pipe A Frame Time Stamp
>> Symbol PIPE_FRMTMSTMP_A
>> Start 0x70048
>> End 0x7004B
>>
>> Its behaving as FLIP Timestamp though (not being updated on every
>vblank_start).
>> Atleast with the readback what we get on APL.
>
>Then it's broken and probably can't be used without having a decent idea of how
>long the frame actually is. Which probably means we'd need something like what
>Chris suggested.
>
Hi Ville,
On further experiments we figured out that, frame time stamp is not updated if the
vblank interrupt gets disabled (which is currently controlled through vblank get and put).
We tried to forcefully enable vblank interrupt by doing an extra vblank get during crtc_enable.
By doing this, we see that frame timestamp is updating at every vblank.
So not sure what should be the best approach to deal with this. I don't think, keeping
vblank interrupt enabled always is a good idea. Ideally frame time stamp should get updated
even if no physical vblank interrupt is coming, or is the behavior expected ?
>Hmm. It's not a command mode display is it?
>
No, it's a single link video mode panel. Command Mode is not even enabled in upstream as of now.
Regards,
Uma Shankar
>--
>Ville Syrjälä
>Intel OTC
More information about the Intel-gfx
mailing list