| Message ID | 20170512135335.20099-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/05/2017 14:53, Chris Wilson wrote: > Storing both the mask and the id is redundant as we can trivially > compute one from the other. As the mask is more frequently used, remove > the id. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------ > drivers/gpu/drm/i915/intel_uncore.h | 6 ++---- > 3 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index bd9abef40c66..a2c9c3e792e1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data) > > for_each_fw_domain(fw_domain, i915, tmp) > seq_printf(m, "%s.wake_count = %u\n", > - intel_uncore_forcewake_domain_to_str(fw_domain->id), > + intel_uncore_forcewake_domain_to_str(fw_domain), > READ_ONCE(fw_domain->wake_count)); > > return 0; > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 08d7d08438c0..7eaa592aed26 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = { > }; > > const char * > -intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id) > +intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d) > { > + unsigned int id = ilog2(d->mask); > + > BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT); > > if (id >= 0 && id < FW_DOMAIN_ID_COUNT) > @@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915, > FORCEWAKE_KERNEL) == 0, > FORCEWAKE_ACK_TIMEOUT_MS)) > DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n", > - intel_uncore_forcewake_domain_to_str(d->id)); > + intel_uncore_forcewake_domain_to_str(d)); > } > > static inline void > @@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915, > FORCEWAKE_KERNEL), > FORCEWAKE_ACK_TIMEOUT_MS)) > DRM_ERROR("%s: timed out waiting for forcewake ack request.\n", > - intel_uncore_forcewake_domain_to_str(d->id)); > + intel_uncore_forcewake_domain_to_str(d)); > } > > static inline void > @@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) > struct intel_uncore_forcewake_domain *domain = > container_of(timer, struct intel_uncore_forcewake_domain, timer); > struct drm_i915_private *dev_priv = > - container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]); > + container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]); Not sure I see the benefit of removing one field and then needing this ilog2 in the timer callback. Regards, Tvrtko > unsigned long irqflags; > > assert_rpm_device_not_suspended(dev_priv); > @@ -1149,8 +1151,6 @@ static void fw_domain_init(struct drm_i915_private *dev_priv, > d->reg_set = reg_set; > d->reg_ack = reg_ack; > > - d->id = domain_id; > - > BUILD_BUG_ON(FORCEWAKE_RENDER != (1 << FW_DOMAIN_ID_RENDER)); > BUILD_BUG_ON(FORCEWAKE_BLITTER != (1 << FW_DOMAIN_ID_BLITTER)); > BUILD_BUG_ON(FORCEWAKE_MEDIA != (1 << FW_DOMAIN_ID_MEDIA)); > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h > index ff6fe2bb0ccf..5fec5fd4346c 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.h > +++ b/drivers/gpu/drm/i915/intel_uncore.h > @@ -93,8 +93,7 @@ struct intel_uncore { > u32 fw_reset; > > struct intel_uncore_forcewake_domain { > - enum forcewake_domain_id id; > - enum forcewake_domains mask; > + unsigned int mask; > unsigned int wake_count; > struct hrtimer timer; > i915_reg_t reg_set; > @@ -123,8 +122,7 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv); > > u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv); > void assert_forcewakes_inactive(struct drm_i915_private *dev_priv); > -const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id); > - > +const char *intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *domain); > enum forcewake_domains > intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv, > i915_reg_t reg, unsigned int op); >
On Mon, May 15, 2017 at 11:18:11AM +0100, Tvrtko Ursulin wrote: > > On 12/05/2017 14:53, Chris Wilson wrote: > >Storing both the mask and the id is redundant as we can trivially > >compute one from the other. As the mask is more frequently used, remove > >the id. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Mika Kuoppala <mika.kuoppala@intel.com> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >--- > > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > > drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------ > > drivers/gpu/drm/i915/intel_uncore.h | 6 ++---- > > 3 files changed, 9 insertions(+), 11 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >index bd9abef40c66..a2c9c3e792e1 100644 > >--- a/drivers/gpu/drm/i915/i915_debugfs.c > >+++ b/drivers/gpu/drm/i915/i915_debugfs.c > >@@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data) > > > > for_each_fw_domain(fw_domain, i915, tmp) > > seq_printf(m, "%s.wake_count = %u\n", > >- intel_uncore_forcewake_domain_to_str(fw_domain->id), > >+ intel_uncore_forcewake_domain_to_str(fw_domain), > > READ_ONCE(fw_domain->wake_count)); > > > > return 0; > >diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > >index 08d7d08438c0..7eaa592aed26 100644 > >--- a/drivers/gpu/drm/i915/intel_uncore.c > >+++ b/drivers/gpu/drm/i915/intel_uncore.c > >@@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = { > > }; > > > > const char * > >-intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id) > >+intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d) > > { > >+ unsigned int id = ilog2(d->mask); > >+ > > BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT); > > > > if (id >= 0 && id < FW_DOMAIN_ID_COUNT) > >@@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915, > > FORCEWAKE_KERNEL) == 0, > > FORCEWAKE_ACK_TIMEOUT_MS)) > > DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n", > >- intel_uncore_forcewake_domain_to_str(d->id)); > >+ intel_uncore_forcewake_domain_to_str(d)); > > } > > > > static inline void > >@@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915, > > FORCEWAKE_KERNEL), > > FORCEWAKE_ACK_TIMEOUT_MS)) > > DRM_ERROR("%s: timed out waiting for forcewake ack request.\n", > >- intel_uncore_forcewake_domain_to_str(d->id)); > >+ intel_uncore_forcewake_domain_to_str(d)); > > } > > > > static inline void > >@@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) > > struct intel_uncore_forcewake_domain *domain = > > container_of(timer, struct intel_uncore_forcewake_domain, timer); > > struct drm_i915_private *dev_priv = > >- container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]); > >+ container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]); > > Not sure I see the benefit of removing one field and then needing > this ilog2 in the timer callback. Timer was infrequent enough compared to the other paths that the extra instruction seemed a matter of little concern, and out of the way. -Chris
On 15/05/2017 11:45, Chris Wilson wrote: > On Mon, May 15, 2017 at 11:18:11AM +0100, Tvrtko Ursulin wrote: >> >> On 12/05/2017 14:53, Chris Wilson wrote: >>> Storing both the mask and the id is redundant as we can trivially >>> compute one from the other. As the mask is more frequently used, remove >>> the id. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_debugfs.c | 2 +- >>> drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------ >>> drivers/gpu/drm/i915/intel_uncore.h | 6 ++---- >>> 3 files changed, 9 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >>> index bd9abef40c66..a2c9c3e792e1 100644 >>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>> @@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data) >>> >>> for_each_fw_domain(fw_domain, i915, tmp) >>> seq_printf(m, "%s.wake_count = %u\n", >>> - intel_uncore_forcewake_domain_to_str(fw_domain->id), >>> + intel_uncore_forcewake_domain_to_str(fw_domain), >>> READ_ONCE(fw_domain->wake_count)); >>> >>> return 0; >>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c >>> index 08d7d08438c0..7eaa592aed26 100644 >>> --- a/drivers/gpu/drm/i915/intel_uncore.c >>> +++ b/drivers/gpu/drm/i915/intel_uncore.c >>> @@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = { >>> }; >>> >>> const char * >>> -intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id) >>> +intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d) >>> { >>> + unsigned int id = ilog2(d->mask); >>> + >>> BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT); >>> >>> if (id >= 0 && id < FW_DOMAIN_ID_COUNT) >>> @@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915, >>> FORCEWAKE_KERNEL) == 0, >>> FORCEWAKE_ACK_TIMEOUT_MS)) >>> DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n", >>> - intel_uncore_forcewake_domain_to_str(d->id)); >>> + intel_uncore_forcewake_domain_to_str(d)); >>> } >>> >>> static inline void >>> @@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915, >>> FORCEWAKE_KERNEL), >>> FORCEWAKE_ACK_TIMEOUT_MS)) >>> DRM_ERROR("%s: timed out waiting for forcewake ack request.\n", >>> - intel_uncore_forcewake_domain_to_str(d->id)); >>> + intel_uncore_forcewake_domain_to_str(d)); >>> } >>> >>> static inline void >>> @@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) >>> struct intel_uncore_forcewake_domain *domain = >>> container_of(timer, struct intel_uncore_forcewake_domain, timer); >>> struct drm_i915_private *dev_priv = >>> - container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]); >>> + container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]); >> >> Not sure I see the benefit of removing one field and then needing >> this ilog2 in the timer callback. > > Timer was infrequent enough compared to the other paths that the extra > instruction seemed a matter of little concern, and out of the way. On one hand that's true, one the other it just looks inelegant. Would storing a backpointer to intel_uncore from each domain be acceptable or it would defeat the purpose? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index bd9abef40c66..a2c9c3e792e1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data) for_each_fw_domain(fw_domain, i915, tmp) seq_printf(m, "%s.wake_count = %u\n", - intel_uncore_forcewake_domain_to_str(fw_domain->id), + intel_uncore_forcewake_domain_to_str(fw_domain), READ_ONCE(fw_domain->wake_count)); return 0; diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 08d7d08438c0..7eaa592aed26 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = { }; const char * -intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id) +intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d) { + unsigned int id = ilog2(d->mask); + BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT); if (id >= 0 && id < FW_DOMAIN_ID_COUNT) @@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915, FORCEWAKE_KERNEL) == 0, FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n", - intel_uncore_forcewake_domain_to_str(d->id)); + intel_uncore_forcewake_domain_to_str(d)); } static inline void @@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915, FORCEWAKE_KERNEL), FORCEWAKE_ACK_TIMEOUT_MS)) DRM_ERROR("%s: timed out waiting for forcewake ack request.\n", - intel_uncore_forcewake_domain_to_str(d->id)); + intel_uncore_forcewake_domain_to_str(d)); } static inline void @@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer) struct intel_uncore_forcewake_domain *domain = container_of(timer, struct intel_uncore_forcewake_domain, timer); struct drm_i915_private *dev_priv = - container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]); + container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]); unsigned long irqflags; assert_rpm_device_not_suspended(dev_priv); @@ -1149,8 +1151,6 @@ static void fw_domain_init(struct drm_i915_private *dev_priv, d->reg_set = reg_set; d->reg_ack = reg_ack; - d->id = domain_id; - BUILD_BUG_ON(FORCEWAKE_RENDER != (1 << FW_DOMAIN_ID_RENDER)); BUILD_BUG_ON(FORCEWAKE_BLITTER != (1 << FW_DOMAIN_ID_BLITTER)); BUILD_BUG_ON(FORCEWAKE_MEDIA != (1 << FW_DOMAIN_ID_MEDIA)); diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index ff6fe2bb0ccf..5fec5fd4346c 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -93,8 +93,7 @@ struct intel_uncore { u32 fw_reset; struct intel_uncore_forcewake_domain { - enum forcewake_domain_id id; - enum forcewake_domains mask; + unsigned int mask; unsigned int wake_count; struct hrtimer timer; i915_reg_t reg_set; @@ -123,8 +122,7 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv); u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv); void assert_forcewakes_inactive(struct drm_i915_private *dev_priv); -const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id); - +const char *intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *domain); enum forcewake_domains intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv, i915_reg_t reg, unsigned int op);
Storing both the mask and the id is redundant as we can trivially compute one from the other. As the mask is more frequently used, remove the id. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------ drivers/gpu/drm/i915/intel_uncore.h | 6 ++---- 3 files changed, 9 insertions(+), 11 deletions(-)