| Message ID | 20200225171125.28885-7-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Proper dbuf global state | expand |
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; >
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.
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> >
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().
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. >
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;