[Intel-gfx] [PATCH v2 06/20] drm/i915: Polish some dbuf debugs

[Lisovskiy, Stanislav]

On Wed, 2020-03-04 at 20:26 +0200, Ville Syrjälä wrote:
> On Wed, Mar 04, 2020 at 04:29:47PM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Polish some of the dbuf code to give more meaningful debug
> > > messages and whatnot. Also we can switch over to the per-device
> > > debugs/warns at the same time.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_power.c    | 40 +++++++++--
> > > ----
> > > ----
> > >  1 file changed, 19 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 6e25a1317161..e81e561e8ac0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -4433,11 +4433,12 @@ static void
> > > intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
> > >  	mutex_unlock(&power_domains->lock);
> > >  }
> > >  
> > > -static inline
> > > -bool intel_dbuf_slice_set(struct drm_i915_private *dev_priv,
> > > -			  i915_reg_t reg, bool enable)
> > > +static void intel_dbuf_slice_set(struct drm_i915_private
> > > *dev_priv,
> > > +				 enum dbuf_slice slice, bool enable)
> > >  {
> > > -	u32 val, status;
> > > +	i915_reg_t reg = DBUF_CTL_S(slice);
> > > +	bool state;
> > > +	u32 val;
> > >  
> > >  	val = intel_de_read(dev_priv, reg);
> > >  	val = enable ? (val | DBUF_POWER_REQUEST) : (val &
> > > ~DBUF_POWER_REQUEST);
> > > @@ -4445,13 +4446,10 @@ bool intel_dbuf_slice_set(struct
> > > drm_i915_private *dev_priv,
> > >  	intel_de_posting_read(dev_priv, reg);
> > >  	udelay(10);
> > >  
> > > -	status = intel_de_read(dev_priv, reg) & DBUF_POWER_STATE;
> > > -	if ((enable && !status) || (!enable && status)) {
> > > -		drm_err(&dev_priv->drm, "DBus power %s timeout!\n",
> > > -			enable ? "enable" : "disable");
> > > -		return false;
> > > -	}
> > > -	return true;
> > > +	state = intel_de_read(dev_priv, reg) & DBUF_POWER_STATE;
> > > +	drm_WARN(&dev_priv->drm, enable != state,
> > > +		 "DBuf slice %d power %s timeout!\n",
> > > +		 slice, enable ? "enable" : "disable");
> > >  }
> > >  
> > >  static void gen9_dbuf_enable(struct drm_i915_private *dev_priv)
> > > @@ -4467,14 +4465,16 @@ static void gen9_dbuf_disable(struct
> > > drm_i915_private *dev_priv)
> > >  void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> > >  			    u8 req_slices)
> > >  {
> > > -	int i;
> > > -	int max_slices = INTEL_INFO(dev_priv)-
> > > > num_supported_dbuf_slices;
> > > 
> > > +	int num_slices = INTEL_INFO(dev_priv)-
> > > > num_supported_dbuf_slices;
> > > 
> > >  	struct i915_power_domains *power_domains = &dev_priv-
> > > > power_domains;
> > > 
> > > +	enum dbuf_slice slice;
> > >  
> > > -	drm_WARN(&dev_priv->drm, hweight8(req_slices) > max_slices,
> > > -		 "Invalid number of dbuf slices requested\n");
> > > +	drm_WARN(&dev_priv->drm, req_slices & ~(BIT(num_slices) - 1),
> > > +		 "Invalid set of dbuf slices (0x%x) requested (num dbuf
> > > slices %d)\n",
> > > +		 req_slices, num_slices);
> > >  
> > > -	DRM_DEBUG_KMS("Updating dbuf slices to 0x%x\n", req_slices);
> > > +	drm_dbg_kms(&dev_priv->drm,
> > > +		    "Updating dbuf slices to 0x%x\n", req_slices);
> > >  
> > >  	/*
> > >  	 * Might be running this in parallel to
> > > gen9_dc_off_power_well_enable
> > > @@ -4485,11 +4485,9 @@ void icl_dbuf_slices_update(struct
> > > drm_i915_private *dev_priv,
> > >  	 */
> > >  	mutex_lock(&power_domains->lock);
> > >  
> > > -	for (i = 0; i < max_slices; i++) {
> > > -		intel_dbuf_slice_set(dev_priv,
> > > -				     DBUF_CTL_S(i),
> > > -				     (req_slices & BIT(i)) != 0);
> > > -	}
> > > +	for (slice = DBUF_S1; slice < num_slices; slice++)
> > > +		intel_dbuf_slice_set(dev_priv, slice,
> > > +				     req_slices & BIT(slice));
> > 
> > Would be cool to completely get rid of any magic numbers or
> > definitions, 0 in a sense is more universal here than DBUF_S1.
> > 
> > If we are counting slices as numbers it seems logical that we 
> > iterate [0..num_slices) range. If you want to name the first slice
> > explicitly then it probably has to be something like iterator
> > logic, i.e for (slice = FIRST_SLICE; slice != LAST_SLICE; slice++).
> > 
> > But trying to name it at the same time with comparing to total
> > _amount_
> > looks a bit confusing.
> 
> This is the standard pattern used all over the driver.

Well, you can enumerate objects using their qualitative or quantative
characteristics, for instance if you take alphabet you would be
either enumerating letters like first is A and count until it becomes
Z, or
you take indexes and say start from index 0 and count until it becomes
26.

What happens here is mixing those: i.e take letter A and count until it
becomes 26, i.e mixing a name of an object with it's index, so
hopefully DBUF_S1 will always be defined as 0 :D

Anyways, 

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>


>