| Message ID | 1504878535-28667-1-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + > /* 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. > + 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. > + 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; 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. 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); 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. > + > + return ulScanlineNo2; > +} > + > static void intel_dsi_connector_destroy(struct drm_connector *connector) > { > struct intel_connector *intel_connector = to_intel_connector(connector); > -- > 1.9.1
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. 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.
HI, > -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > Ville Syrjälä > Sent: perjantai 8. syyskuuta 2017 17.55 > To: Srinivas, Vidya <vidya.srinivas@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Enable scanline read for gen9 dsi > > > 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. > Also please use trybot also as dsi system not in pw runs due to known issue. > -- > Ville Syrjälä > Intel OTC Jani Saarinen Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
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. > 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. -Daniel
Quoting Daniel Vetter (2017-09-08 20:45:11) > On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote: > > 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. You are sort of getting to the point where you just use the ART cpu clock, using an ewma seeded with the vrefresh and fed with the vblank intervals as an estimator for how long you have left to the next vblank. -Chris
Op 08-09-17 om 21:55 schreef Chris Wilson: > Quoting Daniel Vetter (2017-09-08 20:45:11) >> On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote: >>> 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. > You are sort of getting to the point where you just use the ART cpu > clock, using an ewma seeded with the vrefresh and fed with the vblank > intervals as an estimator for how long you have left to the next vblank. Agreed, this seems to be the case.. In which case can't we use that for all of DDI to get a better than scanline resolution for last vblank time by replacing the get_vblank_timestamp hook?
On Mon, Sep 11, 2017 at 10:52:27AM +0200, Maarten Lankhorst wrote: > Op 08-09-17 om 21:55 schreef Chris Wilson: > > Quoting Daniel Vetter (2017-09-08 20:45:11) > >> On Fri, Sep 08, 2017 at 05:55:24PM +0300, Ville Syrjälä wrote: > >>> 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. > > You are sort of getting to the point where you just use the ART cpu > > clock, using an ewma seeded with the vrefresh and fed with the vblank > > intervals as an estimator for how long you have left to the next vblank. > Agreed, this seems to be the case.. In which case can't we use that for all of DDI to get a > better than scanline resolution for last vblank time by replacing the get_vblank_timestamp hook? Like I said that should be doable with just a simple timestamp check to fix up the bogus 0.
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Friday, September 8, 2017 8:18 PM >To: Srinivas, Vidya <vidya.srinivas@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika <mika.kahola@intel.com>; >Kamath, Sunil <sunil.kamath@intel.com>; Shankar, Uma ><uma.shankar@intel.com>; Konduru, Chandra <chandra.konduru@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@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. > 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. >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. 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
>-----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. >> 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@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Sep 11, 2017 at 01:04:18PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Friday, September 8, 2017 8:18 PM > >To: Srinivas, Vidya <vidya.srinivas@intel.com> > >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika <mika.kahola@intel.com>; > >Kamath, Sunil <sunil.kamath@intel.com>; Shankar, Uma > ><uma.shankar@intel.com>; Konduru, Chandra <chandra.konduru@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@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. > > > > 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); > > >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
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.
>-----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
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Monday, September 11, 2017 11:20 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; intel-gfx@lists.freedesktop.org; >Kahola, Mika <mika.kahola@intel.com>; Kamath, Sunil ><sunil.kamath@intel.com>; Konduru, Chandra <chandra.konduru@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@linux.intel.com] >> >Sent: Friday, September 8, 2017 8:18 PM >> >To: Srinivas, Vidya <vidya.srinivas@intel.com> >> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika >> ><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>; >> >Shankar, Uma <uma.shankar@intel.com>; Konduru, Chandra >> ><chandra.konduru@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@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. >> > >> >> 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
>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Shankar, Uma >Sent: Tuesday, September 12, 2017 3:20 PM >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 > > > >>-----Original Message----- >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >>Sent: Monday, September 11, 2017 11:20 PM >>To: Shankar, Uma <uma.shankar@intel.com> >>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; >>intel-gfx@lists.freedesktop.org; Kahola, Mika <mika.kahola@intel.com>; >>Kamath, Sunil <sunil.kamath@intel.com>; Konduru, Chandra >><chandra.konduru@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@linux.intel.com] >>> >Sent: Friday, September 8, 2017 8:18 PM >>> >To: Srinivas, Vidya <vidya.srinivas@intel.com> >>> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika >>> ><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>; >>> >Shankar, Uma <uma.shankar@intel.com>; Konduru, Chandra >>> ><chandra.konduru@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@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. >>> > >>> >>> 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 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@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Sep 12, 2017 at 01:23:39PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > >Shankar, Uma > >Sent: Tuesday, September 12, 2017 3:20 PM > >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 > > > > > > > >>-----Original Message----- > >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >>Sent: Monday, September 11, 2017 11:20 PM > >>To: Shankar, Uma <uma.shankar@intel.com> > >>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; > >>intel-gfx@lists.freedesktop.org; Kahola, Mika <mika.kahola@intel.com>; > >>Kamath, Sunil <sunil.kamath@intel.com>; Konduru, Chandra > >><chandra.konduru@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@linux.intel.com] > >>> >Sent: Friday, September 8, 2017 8:18 PM > >>> >To: Srinivas, Vidya <vidya.srinivas@intel.com> > >>> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika > >>> ><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>; > >>> >Shankar, Uma <uma.shankar@intel.com>; Konduru, Chandra > >>> ><chandra.konduru@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@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. > >>> > > >>> > >>> 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? > 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@lists.freedesktop.org > >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Tuesday, September 12, 2017 7:04 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@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@lists.freedesktop.org] On >> >Behalf Of Shankar, Uma >> >Sent: Tuesday, September 12, 2017 3:20 PM >> >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 >> > >> > >> > >> >>-----Original Message----- >> >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> >>Sent: Monday, September 11, 2017 11:20 PM >> >>To: Shankar, Uma <uma.shankar@intel.com> >> >>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; >> >>intel-gfx@lists.freedesktop.org; Kahola, Mika >> >><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>; >> >>Konduru, Chandra <chandra.konduru@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@linux.intel.com] >> >>> >Sent: Friday, September 8, 2017 8:18 PM >> >>> >To: Srinivas, Vidya <vidya.srinivas@intel.com> >> >>> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika >> >>> ><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>; >> >>> >Shankar, Uma <uma.shankar@intel.com>; Konduru, Chandra >> >>> ><chandra.konduru@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@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. >> >>> > >> >>> >> >>> 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@lists.freedesktop.org >> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Ville Syrjälä >Intel OTC
On Tue, Sep 12, 2017 at 01:40:58PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Tuesday, September 12, 2017 7:04 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@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@lists.freedesktop.org] On > >> >Behalf Of Shankar, Uma > >> >Sent: Tuesday, September 12, 2017 3:20 PM > >> >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 > >> > > >> > > >> > > >> >>-----Original Message----- > >> >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >> >>Sent: Monday, September 11, 2017 11:20 PM > >> >>To: Shankar, Uma <uma.shankar@intel.com> > >> >>Cc: Srinivas, Vidya <vidya.srinivas@intel.com>; > >> >>intel-gfx@lists.freedesktop.org; Kahola, Mika > >> >><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>; > >> >>Konduru, Chandra <chandra.konduru@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@linux.intel.com] > >> >>> >Sent: Friday, September 8, 2017 8:18 PM > >> >>> >To: Srinivas, Vidya <vidya.srinivas@intel.com> > >> >>> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika > >> >>> ><mika.kahola@intel.com>; Kamath, Sunil <sunil.kamath@intel.com>; > >> >>> >Shankar, Uma <uma.shankar@intel.com>; Konduru, Chandra > >> >>> ><chandra.konduru@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@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. > >> >>> > > >> >>> > >> >>> 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?
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Tuesday, September 12, 2017 7:43 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@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@linux.intel.com] >> >Sent: Tuesday, September 12, 2017 7:04 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya >> ><vidya.srinivas@intel.com> >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi >> > >> >> >>> >> >> >>> >-----Original Message----- >> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> >> >>> >Sent: Friday, September 8, 2017 8:18 PM >> >> >>> >To: Srinivas, Vidya <vidya.srinivas@intel.com> >> >> >>> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika >> >> >>> ><mika.kahola@intel.com>; Kamath, Sunil >> >> >>> ><sunil.kamath@intel.com>; Shankar, Uma >> >> >>> ><uma.shankar@intel.com>; Konduru, Chandra >> >> >>> ><chandra.konduru@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@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. >> >> >>> > >> >> >>> >> >> >>> 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. >-- >Ville Syrjälä >Intel OTC
On Tue, Sep 12, 2017 at 02:21:42PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Tuesday, September 12, 2017 7:43 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@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@linux.intel.com] > >> >Sent: Tuesday, September 12, 2017 7:04 PM > >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya > >> ><vidya.srinivas@intel.com> > >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi > >> > > >> >> >>> > >> >> >>> >-----Original Message----- > >> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >> >> >>> >Sent: Friday, September 8, 2017 8:18 PM > >> >> >>> >To: Srinivas, Vidya <vidya.srinivas@intel.com> > >> >> >>> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika > >> >> >>> ><mika.kahola@intel.com>; Kamath, Sunil > >> >> >>> ><sunil.kamath@intel.com>; Shankar, Uma > >> >> >>> ><uma.shankar@intel.com>; Konduru, Chandra > >> >> >>> ><chandra.konduru@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@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. > >> >> >>> > > >> >> >>> > >> >> >>> 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. Hmm. It's not a command mode display is it?
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Tuesday, September 12, 2017 8:36 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@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@linux.intel.com] >> >Sent: Tuesday, September 12, 2017 7:43 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya >> ><vidya.srinivas@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@linux.intel.com] >> >> >Sent: Tuesday, September 12, 2017 7:04 PM >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya >> >> ><vidya.srinivas@intel.com> >> >> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi >> >> > >> >> >> >>> >> >> >> >>> >-----Original Message----- >> >> >> >>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> >> >> >>> >Sent: Friday, September 8, 2017 8:18 PM >> >> >> >>> >To: Srinivas, Vidya <vidya.srinivas@intel.com> >> >> >> >>> >Cc: intel-gfx@lists.freedesktop.org; Kahola, Mika >> >> >> >>> ><mika.kahola@intel.com>; Kamath, Sunil >> >> >> >>> ><sunil.kamath@intel.com>; Shankar, Uma >> >> >> >>> ><uma.shankar@intel.com>; Konduru, Chandra >> >> >> >>> ><chandra.konduru@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@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. >> >> >> >>> > >> >> >> >>> >> >> >> >>> 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
On Wed, Sep 13, 2017 at 08:24:38AM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >Sent: Tuesday, September 12, 2017 8:36 PM > >To: Shankar, Uma <uma.shankar@intel.com> > >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@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@linux.intel.com] > >> >Sent: Tuesday, September 12, 2017 7:43 PM > >> >To: Shankar, Uma <uma.shankar@intel.com> > >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya > >> ><vidya.srinivas@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: > >> >> 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. Well, that's rather unfortuante. I guess we'd have to keep the vblank interrupt unmasked all the time. Hopefully we could still disable it in IER. So we'd unmask the interrupt permanently, and just toggle the IER bit as needed. That should be doable but somewhat annoying because it's exactly the opposite of what we do normally.
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Wednesday, September 13, 2017 11:07 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya <vidya.srinivas@intel.com> >Subject: Re: [PATCH] drm/i915: Enable scanline read for gen9 dsi > >On Wed, Sep 13, 2017 at 08:24:38AM +0000, Shankar, Uma wrote: >> >> >> >-----Original Message----- >> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> >Sent: Tuesday, September 12, 2017 8:36 PM >> >To: Shankar, Uma <uma.shankar@intel.com> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya >> ><vidya.srinivas@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@linux.intel.com] >> >> >Sent: Tuesday, September 12, 2017 7:43 PM >> >> >To: Shankar, Uma <uma.shankar@intel.com> >> >> >Cc: intel-gfx@lists.freedesktop.org; Srinivas, Vidya >> >> ><vidya.srinivas@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: >> >> >> 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. > >Well, that's rather unfortuante. I guess we'd have to keep the vblank interrupt >unmasked all the time. Hopefully we could still disable it in IER. > >So we'd unmask the interrupt permanently, and just toggle the IER bit as needed. >That should be doable but somewhat annoying because it's exactly the opposite >of what we do normally. > We tried controlling Vblank through IER, keeping IMR always unmasked as you suggested. This is causing frame time stamp properly getting updated at every vblank. We will send the patch for the same for review. Thanks Ville for your suggestion. Regards, Uma Shankar >-- >Ville Syrjälä >Intel OTC
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; + 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) + /* 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; + 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)); + WARN_ON(!vtotal); + if (!vtotal) + return ulScanlineNo2; + + ulScanlineTime = div_fixed16(1000000, vtotal * vrefresh); + ulScanlineNo2 = div_round_up_u32_fixed16((ulCurrTime - ulPrevTime), + ulScanlineTime); + ulScanlineNo2 = (ulScanlineNo2 + vtotal) % vtotal; + + return ulScanlineNo2; +} + static void intel_dsi_connector_destroy(struct drm_connector *connector) { struct intel_connector *intel_connector = to_intel_connector(connector);