Re: [PATCH 1/2] drm/i915: Compute the fw_domain id from the mask

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx