>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Monday, September 11, 2017 11:20 PM >To: Shankar, Uma <uma.shankar@xxxxxxxxx> >Cc: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; >Kahola, Mika <mika.kahola@xxxxxxxxx>; Kamath, Sunil ><sunil.kamath@xxxxxxxxx>; Konduru, Chandra <chandra.konduru@xxxxxxxxx> >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@xxxxxxxxxxxxxxx] >> >Sent: Friday, September 8, 2017 8:18 PM >> >To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx> >> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Kahola, Mika >> ><mika.kahola@xxxxxxxxx>; Kamath, Sunil <sunil.kamath@xxxxxxxxx>; >> >Shankar, Uma <uma.shankar@xxxxxxxxx>; Konduru, Chandra >> ><chandra.konduru@xxxxxxxxx> >> >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@xxxxxxxxx> >> >> >> >> 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@xxxxxxxxx> >> >> Signed-off-by: Chandra Konduru <chandra.konduru@xxxxxxxxx> >> >> Signed-off-by: Vidya Srinivas <vidya.srinivas@xxxxxxxxx> >> >> --- >> >> 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. > >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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
