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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jun 30, 2020 at 02:10:45PM -0700, Manasi Navare wrote:
> On Wed, Jul 01, 2020 at 12:03:30AM +0300, Ville Syrjälä wrote:
> > On Fri, Jun 26, 2020 at 04:26:40PM -0700, Manasi Navare wrote:
> > > Modify the helper to add a fixed delay or poll with timeout
> > > based on platform specification to check for either Idle bit
> > > set (DDI_BUF_CTL is idle for disable case)
> > > 
> > > v3:
> > > * Change the timeout to 16usecs (Ville)
> > > v2:
> > > * Use 2 separate functions or idle and active (Ville)
> > > 
> > > 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 | 17 ++++++++---------
> > >  1 file changed, 8 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index 884b507c5f55..052a74625a61 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -1184,16 +1184,15 @@ 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)
> > >  {
> > > -	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 (IS_BROXTON(dev_priv)) {
> > > +		udelay(16);
> > > +		return;
> > >  	}
> > > -	drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c idle bit\n",
> > > -		port_name(port));
> > > +
> > > +	if (wait_for_us((intel_de_read(dev_priv, DDI_BUF_CTL(port)) &
> > > +			 DDI_BUF_IS_IDLE), 16))
> > 
> > 16 is the BXT number. IIRC the spec said 8 usec for the other platforms.
> >
> 
> Yes I see for HSW atleast yes it says 8usecs but i left it at 16 since thats
> what we always had and the only change was that BXT add a fixed delay
> But if you prefer i will change it to 8us timeout?

My usual approach is to a) just use the spec value, b) if there's
a sane reason for not using it then include a comment documenting
the spec value.

Often b) is just for "spec say a few microseconds, let's just wait
a full millisecond to make it simple", or for "old platforms want
timeout N, new ones want M, just go with the larger of the two for
simplicity". Arguably the current code was trying to follow the
latter approach, except if was supposed to since bxt wasn't supposed
to poll at all.

Anyways, since the current code already used 16 usec without any
clarification I guess this is no worse than what we had.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Manasi
>  
> > > +		drm_err(&dev_priv->drm, "Timeout waiting for DDI BUF %c to get idle\n",
> > > +			port_name(port));
> > >  }
> > >  
> > >  static u32 hsw_pll_to_ddi_pll_sel(const struct intel_shared_dpll *pll)
> > > -- 
> > > 2.19.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx