[v2,06/20] drm/i915: Polish some dbuf debugs
diff mbox series

Message ID 20200225171125.28885-7-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: Proper dbuf global state
Related show

Commit Message

Ville Syrjälä Feb. 25, 2020, 5:11 p.m. UTC
From: Ville Syrjälä <ville.syrjala@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@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 40 +++++++++----------
 1 file changed, 19 insertions(+), 21 deletions(-)

Comments

Stanislav Lisovskiy March 4, 2020, 4:29 p.m. UTC | #1
On Tue, 2020-02-25 at 19:11 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@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.

Stan

>  
>  	dev_priv->enabled_dbuf_slices_mask = req_slices;
>
Ville Syrjälä March 4, 2020, 6:26 p.m. UTC | #2
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@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@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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.
Stanislav Lisovskiy March 5, 2020, 9:53 a.m. UTC | #3
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@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@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@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@intel.com>


>
Ville Syrjälä March 5, 2020, 1:46 p.m. UTC | #4
On Thu, Mar 05, 2020 at 09:53:34AM +0000, Lisovskiy, Stanislav wrote:
> 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@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@intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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

The old code assumed DBUF_S1==0 for the purposes of passing it to
DBUF_CTL(), and for the purposes of the BIT(int).

The new code assumes DBUF_S1 == 0 for the purposes of terminating
the iteration.

Suo siellä, vetelä täällä.

We may actually need to change the device info to contain a dbuf slice
mask instead of just the number of slices. That's in case some hw has
some slices fused off (not sure that's actually a thing but maybe,
need to check the spec at some point for this). At that point we
probably want to stash the whole thing into a for_each_dbuf_slice().
Stanislav Lisovskiy March 5, 2020, 2:56 p.m. UTC | #5
On Thu, 2020-03-05 at 15:46 +0200, Ville Syrjälä wrote:
> On Thu, Mar 05, 2020 at 09:53:34AM +0000, Lisovskiy, Stanislav wrote:
> > 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@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@intel.com>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@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
> 
> The old code assumed DBUF_S1==0 for the purposes of passing it to
> DBUF_CTL(), and for the purposes of the BIT(int).
> 
> The new code assumes DBUF_S1 == 0 for the purposes of terminating
> the iteration.
> 
> Suo siellä, vetelä täällä.
> 
> We may actually need to change the device info to contain a dbuf
> slice
> mask instead of just the number of slices. That's in case some hw has
> some slices fused off (not sure that's actually a thing but maybe,
> need to check the spec at some point for this). At that point we
> probably want to stash the whole thing into a for_each_dbuf_slice().

Yes, for_each_dbuf_slice sounds much better indeed, that way you
neither have to use indexes nor start from some explicitly hardcoded
slice.

>

Patch
diff mbox series

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));
 
 	dev_priv->enabled_dbuf_slices_mask = req_slices;