Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/dp: Helper for checking DDI_BUF_CTL Idle status

[Manasi Navare <manasi.d.navare@intel.com>]

On Tue, Jun 23, 2020 at 10:57:10PM +0300, Imre Deak wrote:
> On Tue, Jun 23, 2020 at 12:42:00PM -0700, Manasi Navare wrote:
> > On Mon, Jun 22, 2020 at 06:49:26PM +0300, Imre Deak wrote:
> > > On Wed, Jun 17, 2020 at 05:01:23PM -0700, Manasi Navare wrote:
> > > > Modify the helper to add a fixed delay or poll with timeout
> > > > based on platform specification in bothe enable and disable
> > > > cases so check for either Idle bit set (DDI_BUF_CTL is idle
> > > > for disable case) or check for Idle bit = 0 (non idle for
> > > > DDI BUF enable case)
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_ddi.c | 34 +++++++++++++++---------
> > > >  1 file changed, 21 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > index ca7bb2294d2b..e4738c3b6d44 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > @@ -1182,18 +1182,26 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> > > >  }
> > > >  
> > > >  static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> > > > -				    enum port port)
> > > 
> > > maybe intel_ddi_wait_for_ddi_buf(i915, port, active) ?
> > 
> > So here you mean active which is true if we are checking during enable for non_idle
> > and vice versa for disable, active will be false or checking for idel state?
> 
> Maybe just use Ville's idea with two functions instead.
> 
> > 
> > > 
> > > > +				    enum port port, bool idle)
> > > >  {
> > > > -	i915_reg_t reg = DDI_BUF_CTL(port);
> > > > -	int i;
> > > > -
> > > > -	for (i = 0; i < 16; i++) {
> > > > -		udelay(1);
> > > > -		if (intel_de_read(dev_priv, reg) & DDI_BUF_IS_IDLE)
> > > > -			return;
> > > > +	if (idle) {
> > > > +		if (IS_BROXTON(dev_priv))
> > > > +			udelay(16);
> > > > +		else
> > > > +			if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > > > +					 DDI_BUF_IS_IDLE), 16))
> > > > +				drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> > > > +					port_name(port));
> > > > +	} else {
> > > > +		if (INTEL_GEN(dev_priv) < 10)
> > > > +			udelay(600);
> > > > +		else
> > > > +			if (wait_for_us(!(intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > > > +					  DDI_BUF_IS_IDLE), 600))
> > > > +				drm_err(&dev_priv->drm, "DDI port:%c buffer idle\n",
> > > > +					port_name(port));
> > > >  	}
> > > > -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> > > > -		port_name(port));
> > > > +
> > > 
> > > since we can only guarantee a minimum delay or timeout, imo it could be just:
> > > 
> > > 	if (BXT && !active || GEN <= 9 && active) {
> > > 		usleep_range(600, 1000);
> > > 		return;
> > 
> 
> > Didnt quite understand this logic, for BXT & !active which is BXT and
> > idle, it shd be fixed delay of just 16usecs
> > or if it is !BXT and !active then we wait with a timeout
> > also for gen <=9 and active, it shd be fixed delay of 600
> > and greater than or = 10 and active should be a timeout
> 
> yes, the above would match what I provided. The fixed delay for all
> platforms would be a minimum 600usec delay. You can't guarantee that the
> delay would be only 16usec in any case, so using 600 usec on BXT too
> would be ok.

still dont quite get it, how is usleep_range (600, 1000) providing a fixed delay?

Now if we split ino 2 functs, one for disable, for that:

if (BXT)
	usleep_range(600, 1000)
else
	wait_for_us(check if Idle bit set)

so in both functions, for the timeout part we still use the wait_for_us helper right?

Manasi

> 
> > Manasi
> > 
> > > 	}
> > > 
> > > 	if (wait_for_us(!(read(BUF_CTL) & IS_IDLE) == active, 600))
> > > 		drm_err("Port %c: Timeout waiting for DDI BUF to get %s\n",
> > > 			port, active ? "active" : "idle"));
> > > 		
> > > 
> > > >  }
> > > >  
> > > >  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> > > > @@ -1373,7 +1381,7 @@ void hsw_fdi_link_train(struct intel_encoder *encoder,
> > > >  		intel_de_write(dev_priv, DP_TP_CTL(PORT_E), temp);
> > > >  		intel_de_posting_read(dev_priv, DP_TP_CTL(PORT_E));
> > > >  
> > > > -		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
> > > > +		intel_wait_ddi_buf_idle(dev_priv, PORT_E, true);
> > > >  
> > > >  		/* Reset FDI_RX_MISC pwrdn lanes */
> > > >  		temp = intel_de_read(dev_priv, FDI_RX_MISC(PIPE_A));
> > > > @@ -3495,7 +3503,7 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder,
> > > >  	intel_ddi_disable_fec_state(encoder, crtc_state);
> > > >  
> > > >  	if (wait)
> > > > -		intel_wait_ddi_buf_idle(dev_priv, port);
> > > > +		intel_wait_ddi_buf_idle(dev_priv, port, true);
> > > >  }
> > > >  
> > > >  static void intel_ddi_post_disable_dp(struct intel_atomic_state *state,
> > > > @@ -4004,7 +4012,7 @@ static void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
> > > >  		intel_de_posting_read(dev_priv, intel_dp->regs.dp_tp_ctl);
> > > >  
> > > >  		if (wait)
> > > > -			intel_wait_ddi_buf_idle(dev_priv, port);
> > > > +			intel_wait_ddi_buf_idle(dev_priv, port, true);
> > > >  	}
> > > >  
> > > >  	dp_tp_ctl = DP_TP_CTL_ENABLE |
> > > 
> > > The DSI code could also use the new helper.
> > > 
> > > > -- 
> > > > 2.19.1
> > > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx