Re: [PATCH 3/3] drm/i915/dc3co: Avoid full modeset when EXITLINE needs to be changedy

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2019-11-19 at 00:12:35 +0530, Souza, Jose wrote:
> On Sun, 2019-11-17 at 18:23 +0530, Anshuamn Gupta wrote:
> > On 2019-10-31 at 17:14:22 -0700, José Roberto de Souza wrote:
> > > A recent change in BSpec allow us to change EXTLINE while
> > > transcoder
> > > is enabled so this allow us to change it even when doing the first
> > > fastset after taking over previous hardware state set by BIOS.
> > > BIOS don't enable PSR, so if sink supports PSR it will be enabled
> > > on
> > > the first fastset, so moving the EXTLINE compute and set to PSR
> > > flows
> > > allow us to simplfy a bunch of code.
> > > 
> > > This will save a lot of time in all the IGT tests that uses CRC, as
> > > when PSR2 is enabled CRCs are not generated, so we switch to PSR1,
> > > so
> > > the previous code would compute dc3co_exitline=0 causing a full
> > > modeset that would shutdown pipe, enable and train link.
> > > 
> > > BSpec: 49196
> > > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > > Cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c     | 86 ----------------
> > > ----
> > >  drivers/gpu/drm/i915/display/intel_display.c |  1 -
> > >  drivers/gpu/drm/i915/display/intel_psr.c     | 47 +++++++++++
> > >  3 files changed, 47 insertions(+), 87 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index b51f244ad7a5..f52fb7619d46 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3339,86 +3339,6 @@ static void
> > > intel_ddi_disable_fec_state(struct intel_encoder *encoder,
> > >  	POSTING_READ(intel_dp->regs.dp_tp_ctl);
> > >  }
> > >  
> > > -static void
> > > -tgl_clear_psr2_transcoder_exitline(const struct intel_crtc_state
> > > *cstate)
> > > -{
> > > -	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc-
> > > >dev);
> > > -	u32 val;
> > > -
> > > -	if (!cstate->dc3co_exitline)
> > > -		return;
> > > -
> > > -	val = I915_READ(EXITLINE(cstate->cpu_transcoder));
> > > -	val &= ~(EXITLINE_MASK | EXITLINE_ENABLE);
> > > -	I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> > > -}
> > > -
> > > -static void
> > > -tgl_set_psr2_transcoder_exitline(const struct intel_crtc_state
> > > *cstate)
> > > -{
> > > -	u32 val, exit_scanlines;
> > > -	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc-
> > > >dev);
> > > -
> > > -	if (!cstate->dc3co_exitline)
> > > -		return;
> > > -
> > > -	exit_scanlines = cstate->dc3co_exitline;
> > > -	exit_scanlines <<= EXITLINE_SHIFT;
> > > -	val = I915_READ(EXITLINE(cstate->cpu_transcoder));
> > > -	val &= ~(EXITLINE_MASK | EXITLINE_ENABLE);
> > > -	val |= exit_scanlines;
> > > -	val |= EXITLINE_ENABLE;
> > > -	I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> > > -}
> > > -
> > > -static void tgl_dc3co_exitline_compute_config(struct intel_encoder
> > > *encoder,
> > > -					      struct intel_crtc_state
> > > *cstate)
> > > -{
> > > -	u32 exit_scanlines;
> > > -	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc-
> > > >dev);
> > > -	u32 crtc_vdisplay = cstate->base.adjusted_mode.crtc_vdisplay;
> > > -
> > > -	cstate->dc3co_exitline = 0;
> > > -
> > > -	if (!(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO))
> > > -		return;
> > > -
> > > -	/* B.Specs:49196 DC3CO only works with pipeA and DDIA.*/
> > > -	if (to_intel_crtc(cstate->base.crtc)->pipe != PIPE_A ||
> > > -	    encoder->port != PORT_A)
> > > -		return;
> > > -
> > > -	if (!cstate->has_psr2 || !cstate->base.active)
> > > -		return;
> > > -
> > > -	/*
> > > -	 * DC3CO Exit time 200us B.Spec 49196
> > > -	 * PSR2 transcoder Early Exit scanlines = ROUNDUP(200 / line
> > > time) + 1
> > > -	 */
> > > -	exit_scanlines =
> > > -		intel_usecs_to_scanlines(&cstate->base.adjusted_mode,
> > > 200) + 1;
> > > -
> > > -	if (WARN_ON(exit_scanlines > crtc_vdisplay))
> > > -		return;
> > > -
> > > -	cstate->dc3co_exitline = crtc_vdisplay - exit_scanlines;
> > > -	DRM_DEBUG_KMS("DC3CO exit scanlines %d\n", cstate-
> > > >dc3co_exitline);
> > > -}
> > > -
> > > -static void tgl_dc3co_exitline_get_config(struct intel_crtc_state
> > > *crtc_state)
> > > -{
> > > -	u32 val;
> > > -	struct drm_i915_private *dev_priv = to_i915(crtc_state-
> > > >base.crtc->dev);
> > > -
> > > -	if (INTEL_GEN(dev_priv) < 12)
> > > -		return;
> > > -
> > > -	val = I915_READ(EXITLINE(crtc_state->cpu_transcoder));
> > > -
> > > -	if (val & EXITLINE_ENABLE)
> > > -		crtc_state->dc3co_exitline = val & EXITLINE_MASK;
> > > -}
> > > -
> > >  static void tgl_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > >  				  const struct intel_crtc_state
> > > *crtc_state,
> > >  				  const struct drm_connector_state
> > > *conn_state)
> > > @@ -3431,7 +3351,6 @@ static void tgl_ddi_pre_enable_dp(struct
> > > intel_encoder *encoder,
> > >  	int level = intel_ddi_dp_level(intel_dp);
> > >  	enum transcoder transcoder = crtc_state->cpu_transcoder;
> > >  
> > > -	tgl_set_psr2_transcoder_exitline(crtc_state);
> > >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > >  				 crtc_state->lane_count, is_mst);
> > >  
> > > @@ -3760,7 +3679,6 @@ static void intel_ddi_post_disable_dp(struct
> > > intel_encoder *encoder,
> > >  						  dig_port-
> > > >ddi_io_power_domain);
> > >  
> > >  	intel_ddi_clk_disable(encoder);
> > > -	tgl_clear_psr2_transcoder_exitline(old_crtc_state);
> > >  }
> > >  
> > >  static void intel_ddi_post_disable_hdmi(struct intel_encoder
> > > *encoder,
> > > @@ -4308,9 +4226,6 @@ void intel_ddi_get_config(struct
> > > intel_encoder *encoder,
> > >  		break;
> > >  	}
> > >  
> > > -	if (encoder->type == INTEL_OUTPUT_EDP)
> > > -		tgl_dc3co_exitline_get_config(pipe_config);
> > > -
> > >  	pipe_config->has_audio =
> > >  		intel_ddi_is_audio_enabled(dev_priv, cpu_transcoder);
> > >  
> > > @@ -4392,7 +4307,6 @@ static int intel_ddi_compute_config(struct
> > > intel_encoder *encoder,
> > >  		ret = intel_hdmi_compute_config(encoder, pipe_config,
> > > conn_state);
> > >  	} else {
> > >  		ret = intel_dp_compute_config(encoder, pipe_config,
> > > conn_state);
> > > -		tgl_dc3co_exitline_compute_config(encoder,
> > > pipe_config);
> > >  	}
> > >  
> > >  	if (ret)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 348ce0456696..6c1c93b2a81f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13091,7 +13091,6 @@ intel_pipe_config_compare(const struct
> > > intel_crtc_state *current_config,
> > >  
> > >  	PIPE_CONF_CHECK_I(pixel_multiplier);
> > >  	PIPE_CONF_CHECK_I(output_format);
> > > -	PIPE_CONF_CHECK_I(dc3co_exitline);
> > >  	PIPE_CONF_CHECK_BOOL(has_hdmi_sink);
> > >  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
> > >  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index bb9b5349b72a..06a0a1c9621e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -596,6 +596,37 @@ static void
> > > tgl_disallow_dc3co_on_psr2_exit(struct drm_i915_private *dev_priv)
> > >  	tgl_psr2_disable_dc3co(dev_priv);
> > >  }
> > >  
> > > +static void
> > > +tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
> > > +				  struct intel_crtc_state *crtc_state)
> > > +{
> > > +	const u32 crtc_vdisplay = crtc_state-
> > > >base.adjusted_mode.crtc_vdisplay;
> > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	u32 exit_scanlines;
> > We wanted to explicitly clear dc3co_exitline in case it is not
> > supported.
> 
> 
> The whole intel_crtc_state is zeroed before the compute functions are
> called so no need to set dc3co_exitline to zero.
> 
> > > +
> > > +	if (!(dev_priv->csr.allowed_dc_mask & DC_STATE_EN_DC3CO))
> > > +		return;
> > > +
> > > +	/* B.Specs:49196 DC3CO only works with pipeA and DDIA.*/
> > > +	if (to_intel_crtc(crtc_state->base.crtc)->pipe != PIPE_A ||
> > > +	    dig_port->base.port != PORT_A)
> > > +		return;
> > > +
> > > +	/*
> > > +	 * DC3CO Exit time 200us B.Spec 49196
> > > +	 * PSR2 transcoder Early Exit scanlines = ROUNDUP(200 / line
> > > time) + 1
> > > +	 */
> > > +	exit_scanlines =
> > > +		intel_usecs_to_scanlines(&crtc_state-
> > > >base.adjusted_mode, 200) + 1;
> > > +
> > > +	if (WARN_ON(exit_scanlines > crtc_vdisplay))
> > > +		return;
> > > +
> > > +	crtc_state->dc3co_exitline = crtc_vdisplay - exit_scanlines;
> > > +	DRM_DEBUG_KMS("DC3CO exit scanlines %d\n", crtc_state-
> > > >dc3co_exitline);
> > > +}
> > > +
> > >  static bool intel_psr2_config_valid(struct intel_dp *intel_dp,
> > >  				    struct intel_crtc_state
> > > *crtc_state)
> > >  {
> > > @@ -658,6 +689,7 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  		return false;
> > >  	}
> > >  
> > > +	tgl_dc3co_exitline_compute_config(intel_dp, crtc_state);
> > >  	return true;
> > >  }
> > >  
> > > @@ -775,6 +807,21 @@ static void intel_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  	I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
> > >  
> > >  	psr_irq_control(dev_priv);
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 12 && dev_priv->psr.psr2_enabled) {
> > IMHO we can use dc3co_enabled flag here instead of psr2_enabled, and
> > it would be 
> > nice to have a seperate function to set the exitline here.
> > Thanks,
> > Anshuman Gupta.
> 
> Here it is checking for PSR2 because in case DC3CO is disabled it will
> set EXITLINE to 0 but we could keep it set too without any drawbacks.
IMHO EXITLINE is only used to exit the DC3CO until a programmed exit line event,
this optimally align the frame update command position within the
repeated video frame. so there is no use case of EXITLINE without DC3CO.
Thanks ,
Anshuman Gupta.
> Thoughts Imre?
> 
> I don't think we need a separated function it is just RMW into one
> registers.
> 
> > > +		u32 val;
> > > +
> > > +		/*
> > > +		 * TODO: if future platforms supports DC3CO in more
> > > than one
> > > +		 * transcoder, EXITLINE will need to be unset when
> > > disabling PSR
> > > +		 */
> > > +		val = I915_READ(EXITLINE(cpu_transcoder));
> > > +		val &= ~(EXITLINE_MASK | EXITLINE_ENABLE);
> > > +		val |= crtc_state->dc3co_exitline << EXITLINE_SHIFT;
> > > +		if (crtc_state->dc3co_exitline)
> > > +			val |= EXITLINE_ENABLE;
> > > +		I915_WRITE(EXITLINE(cpu_transcoder), val);
> > > +	}
> > >  }
> > >  
> > >  static void intel_psr_enable_locked(struct drm_i915_private
> > > *dev_priv,
> > > -- 
> > > 2.23.0
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux