[Intel-gfx] [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

Tue Oct 23 23:59:23 UTC 2018

On Mon, Oct 22, 2018 at 09:26:06PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 22, 2018 at 06:04:28PM +0000, Srivatsa, Anusha wrote:
> > 
> > 
> > >-----Original Message-----
> > >From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > >Sent: Friday, October 19, 2018 4:12 PM
> > >To: Navare, Manasi D <manasi.d.navare at intel.com>
> > >Cc: Srivatsa, Anusha <anusha.srivatsa at intel.com>; intel-
> > >gfx at lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh at intel.com>; Jani
> > >Nikula <jani.nikula at linux.intel.com>
> > >Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
> > >
> > >On Fri, Oct 19, 2018 at 01:29:05PM -0700, Manasi Navare wrote:
> > >> On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
> > >> > On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
> > >> > > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> > >> > > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> > >> > > > > If FEC is supported, the corresponding DP_TP_CTL register bits
> > >> > > > > have to be configured.
> > >> > > > >
> > >> > > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30]
> > >> > > > > register and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> > >> > > > > Also add the warn message to make sure that the control
> > >> > > > > register is already active while enabling FEC.
> > >> > > > >
> > >> > > > > v2:
> > >> > > > > - Change commit message. Configure fec state after
> > >> > > > >   link training (Manasi, Gaurav)
> > >> > > > > - Remove redundent checks (Manasi)
> > >> > > > > - Remove the registers that get added automagically (Anusha)
> > >> > > > >
> > >> > > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state()
> > >> > > > > (Gaurav)
> > >> > > > >
> > >> > > > > v4: rebased.
> > >> > > > >
> > >> > > > > Cc: Gaurav K Singh <gaurav.k.singh at intel.com>
> > >> > > > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > >> > > > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > >> > > > > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > >> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > >> > > > > ---
> > >> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> > >> > > > > drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> > >> > > > > drivers/gpu/drm/i915/intel_dp.c  | 27
> > >> > > > > +++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_drv.h
> > >> > > > > |  3 ++-
> > >> > > > >  4 files changed, 33 insertions(+), 1 deletion(-)
> > >> > > > >
> > >> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > >> > > > > b/drivers/gpu/drm/i915/i915_reg.h index
> > >> > > > > bcde78bc0027..c8d7fdcd7823 100644
> > >> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > >> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > >> > > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> > >> > > > >  #define _DP_TP_CTL_B			0x64140
> > >> > > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
> > >_DP_TP_CTL_B)
> > >> > > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> > >> > > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> > >> > > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> > >> > > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> > >> > > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> > >> > > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> > >> > > > >  #define _DP_TP_STATUS_A			0x64044
> > >> > > > >  #define _DP_TP_STATUS_B			0x64144
> > >> > > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
> > >> > > > > _DP_TP_STATUS_B)
> > >> > > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> > >> > > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> > >> > > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> > >> > > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> > >> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > index f531900165bf..67c013ea4d39 100644
> > >> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct
> > >intel_encoder *encoder,
> > >> > > > >  					      DP_DECOMPRESSION_EN);
> > >> > > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state,
> > >DP_FEC_READY);
> > >> > > > >  	intel_dp_start_link_train(intel_dp);
> > >> > > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> > >> > > >
> > >> > > > This doesn't look like the correct spot. According to the spec
> > >> > > > it should be after stop_link_train() (where we set DP_TP_CTL to
> > >> > > > to normal).
> > >> > > >
> > >> > >
> > >> > > Yes I see in the display port enabling sequence page of the spec,
> > >> > > that it says enable FEC after DP_TP_CTL is set to Normal. Also the
> > >> > > FEC page says that this should be enabled only after ensuring that
> > >> > > link training completed. So according to that this call should happen after
> > >intel_dp_stop_link_train().
> > >> > > So yes move this to after intel_dp_stop_link_training().
> > >> > >
> > >> > > > > +
> > >> > > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > >> > > > >  		intel_dp_stop_link_train(intel_dp);
> > >> > > > >
> > >> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > >> > > > > b/drivers/gpu/drm/i915/intel_dp.c index
> > >> > > > > b4e8af3142a2..b9f85502d9ff 100644
> > >> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > >> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > >> > > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct
> > >intel_dp *intel_dp,
> > >> > > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in
> > >sink\n");  }
> > >> > > > >
> > >> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > >> > > > > +			       const struct intel_crtc_state *crtc_state) {
> > >> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > >> > > > > +	struct intel_digital_port *intel_dig_port =
> > >dp_to_dig_port(intel_dp);
> > >> > > > > +	enum port port = intel_dig_port->base.port;
> > >> > > > > +	u32 val;
> > >> > > > > +
> > >> > > > > +	/* FEC support exists for DP 1.4 only */
> > >> > > > > +	if (intel_dp_is_edp(intel_dp))
> > >> > > > > +		return;
> > >> > > > > +
> > >> > > > > +	/* If Display Compression is not enabled, FEC need not be
> > >configured */
> > >> > > > > +	if (!crtc_state->dsc_params.compression_enable)
> > >> > > > > +		return;
> > >> > > > > +
> > >> > > > > +	val = I915_READ(DP_TP_CTL(port));
> > >> > > > > +	val |= DP_TP_CTL_FEC_ENABLE;
> > >> > > > > +	I915_WRITE(DP_TP_CTL(port), val);
> > >> > > > > +
> > >> > > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> > >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > >> > > > > +				    1))
> > >> > > > > +		DRM_ERROR("Timed out waiting for FEC Enable
> > >Status\n"); }
> > >> > > >
> > >> > > > There is no reason to stick these into intel_dp.c when the only
> > >> > > > caller is in intel_ddi.c.
> > >> > > >
> > >> > >
> > >> > > So may be move the set_dsc_state function also from intel_dp.c to
> > >> > > intel_ddi.c Or does it make sense to move all these DSC related functions
> > >including fec to intel_dsc.c?
> > >> > >
> > >> > > > We also seem to be missing platform checks. FEC doesn't appear
> > >> > > > to be a thing prior to ICL. I guess the edp+compression_enable
> > >> > > > takes care of this, but I think I'd rather stick a new
> > >> > > > fec_enable bool into the crtc state. That way we get can add it to the
> > >state checker as well.
> > >> > > >
> > >> > >
> > >> > > fec_enable can be added to crtc_state->dsc_params may be?
> > >> >
> > >> > Isn't FEC something that can be used w/o DSC too?
> > >> >
> > >>
> > >> Nope, FEC always goes hand in hand with DSC on external DP and that
> > >> DSC on external DP cannot be enabled without FEC.
> > >
> > >Sure, DSC needs FEC. But FEC doesn't need DSC. At least if I'm reading the spec
> > >correctly.
> > >
> > >FEC is enabled for the entire main link, so everything getting transported over it
> > >get FECced. DSC on the other hand operates on on a stream level. So assuming a
> > >MST branch device that can do FEC but can't do DSC you should be able transport
> > >both compressed and uncompressed streams through it simultaneously. Kinda
> > >makes me think that we want to enable FEC for MST whenever the downstream
> > >device supports it.
> > 
> > FEC for MST modes is not supported for ICL platform. We could extend this for future platforms  for branch devices that support FEC.
> 
> Hmm. Well, looks like we'll have to think about that soon enough anyway,
> so IMO better do it right from the start. Avoids having to rewrite the
> SST path yet again when we add the MST support. And we'll also get the
> state checker as a bonus.

I think it makes sense to add crtc_state->fec_enable variable to add it non compressed and MST cases later.
But for ICL, to add the basic FEC support, this patch series can only enable it for DSC SST cases.

Manasi

> 
> > 
> > >And I suppose we could also consider using FEC opportunistically for
> > >uncompressed SST when we have the extra link bandwidth to handle the extra
> > >overhead.
> > 
> > Yes. The requirement though is for compressed streams. I am not sure if we will have to pay any power penalty for SST. Also my understanding is that if a few symbols in a non-compressed stream are in error, then only those symbols will be in error. These errors do not cause other pixels to get corrupted. In compressed streams, if a few symbols are in error, it can cause the entire slice to get corrupted. Will we be getting any much return on non-compressed streams?
> 
> No idea.
> 
> -- 
> Ville Syrjälä
> Intel