[Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi
Shankar, Uma
uma.shankar at intel.com
Tue Sep 12 13:40:58 UTC 2017
>-----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
>
>On Tue, Sep 12, 2017 at 01:23:39PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On
>> >Behalf Of Shankar, Uma
>> >Sent: Tuesday, September 12, 2017 3:20 PM
>> >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
>> >
>> >
>> >
>> >>-----Original Message-----
>> >>From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
>> >>Sent: Monday, September 11, 2017 11:20 PM
>> >>To: Shankar, Uma <uma.shankar at intel.com>
>> >>Cc: Srinivas, Vidya <vidya.srinivas at intel.com>;
>> >>intel-gfx at lists.freedesktop.org; Kahola, Mika
>> >><mika.kahola at intel.com>; Kamath, Sunil <sunil.kamath at intel.com>;
>> >>Konduru, Chandra <chandra.konduru at intel.com>
>> >>Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi
>> >>
>> >>On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote:
>> >>>
>> >>>
>> >>> >-----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. So assume if 10 times the same frame has been scanned and we
are in 11 iteration and at that time a new flip is issued (where we are doing these checks).
At that moment the delta of curr - prev will still have a huge value and will surely give results,
adding the lines scanned in earlier iterations of the same frame. Thus by doing the modulo operation
using vtotal we can really check for the scanline in the current iteration of the scan.
Thus getting a high value of scanline is expected.
Regards,
Uma Shankar
>> This causes calculated value to be different from PIPE SCANLINE value read
>from register.
>>
>> But if we keep scanline and take the modulo with vtotal after adding
>> the vblank_start (not taking min with vtotal -1), we are getting
>> scanline equal to (delta of 1 all the time ) what the PIPE SCANLINE
>> register returns for HDMI. We can use HDMI as a reference to validate if
>timestamp based calculation aligns to h/w values returned by Pipe scanline
>register. So if we do like below:
>>
>> scanline = div_u64(...);
>> return (scanline + vblank_start) % vtotal;
>>
>> It works perfectly fine. Tested it with "kms_plane_multiple" and
>> almost getting the same result all the time with no atomic update
>> failures. I am not sure on other platforms, but BXT/APL its working fine.
>Without these changes, issue can easily be reproduced using this IGT test.
>>
>> Will send the updated patch, please have a look once.
>>
>> Thanks & Regards,
>> Uma Shankar
>>
>> >>Oh and there's maybe another race lurking here. We might cross into
>> >>the next vblank just between the PIPE_FRMTMSTMP and TIMESTAMP_CTR
>> >>reads. If that happens we get an answer that's definitely too big for one
>frame.
>> >>I guess we could avoid that particular problem by making sure we
>> >>really read PIPE_FRMTMSTMP and TIMESTAMP_CTR during the same frame.
>Eg.
>> >>something like:
>> >>
>> >>do {
>> >> prev = PIPE_FRMTMSTMP;
>> >> curr = TIMESTAMP_CTR
>> >> post = PIPE_FRMTMSTMP
>> >>} while (prev != post);
>> >>
>> >
>> >Got it. Will add this condition to handle the race situation. Thanks
>> >for the explanation.
>> >
>> >Regards,
>> >Uma Shankar
>> >
>> >>
>> >>>
>> >>> >return (scanline + vblank_start) % vtotal;
>> >>> >
>> >>> >At least that's my thinking atm. Feel free to rip my reasoning to
>> >>> >shreds if you think I'm totally wrong here.
>> >>> >
>> >>>
>> >>> One more thing we missed is, that the current timestamp is just a
>> >>> 32 bit register
>> >>value.
>> >>> It can overflow and wrap around. So a situation can come, where
>> >>> current timestamp will be less than prev timestamp (read from
>> >>> frame time stamp reg). We need to handle that situation as well.
>> >>> Will fix that in the
>> >>next version and resend.
>> >>
>> >>Modulo 2^32 math will handle that just fine.
>> >>
>> >>>
>> >>> Thanks Ville for your valuable review comments.
>> >>>
>> >>> Regards,
>> >>> Uma Shankar
>> >>>
>> >>> >
>> >>> >> +
>> >>> >> + return ulScanlineNo2;
>> >>> >> +}
>> >>> >> +
>> >>> >> static void intel_dsi_connector_destroy(struct drm_connector
>> >>> >> *connector) {
>> >>> >> struct intel_connector *intel_connector =
>> >>> >> to_intel_connector(connector);
>> >>> >> --
>> >>> >> 1.9.1
>> >>> >
>> >>> >--
>> >>> >Ville Syrjälä
>> >>> >Intel OTC
>> >>
>> >>--
>> >>Ville Syrjälä
>> >>Intel OTC
>> >_______________________________________________
>> >Intel-gfx mailing list
>> >Intel-gfx at lists.freedesktop.org
>> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel OTC
More information about the Intel-gfx
mailing list