Re: [PATCH 2/3] drm/i915: Simplify for_each_fw_domain iterators

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

On 04/04/16 20:14, Dave Gordon wrote:
> On 04/04/16 17:51, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> As the vast majority of users does not use the domain id variable,
>
> "do not use"

Yep.

>
>> we can eliminate it from the iterator and also change the latter
>> using the same principle as was recently done for for_each_engine.
>>
>> For a couple of callers which do need the domain id, or the domain
>> mask, store the later in the domain itself at init time and use
>> both through it.
>
> "For a couple of callers which do need the domain mask, store it
> in the domain array (which already has the domain id), then both
> can be retrieved thence." ?

Better yes.

>> Result is clearer code and smaller generated binary, especially
>> in the tight fw get/put loops. Also, relationship between domain
>> id and mask is no longer assumed in the macro.
>
> "in the macro or elsewhere" ?

Just in the macro, it is still hardcoded in fw_domain_init.

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> A few typos & clarifications above, plus a minor quibble about a macro
> name (which you haven't actually changed, but might as well fix).
> Otherwise LGTM, so
>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>
> (even if you don't change the macro name)

Thanks, consistency sounds good so I will change it.

>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c |  5 ++---
>>   drivers/gpu/drm/i915/i915_drv.h     | 17 +++++++-------
>>   drivers/gpu/drm/i915/intel_uncore.c | 45
>> +++++++++++++++++--------------------
>>   3 files changed, 32 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
>> b/drivers/gpu/drm/i915/i915_debugfs.c
>> index a2e3af02c292..06fce014d0b4 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1464,12 +1464,11 @@ static int i915_forcewake_domains(struct
>> seq_file *m, void *data)
>>       struct drm_device *dev = node->minor->dev;
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct intel_uncore_forcewake_domain *fw_domain;
>> -    int i;
>>
>>       spin_lock_irq(&dev_priv->uncore.lock);
>> -    for_each_fw_domain(fw_domain, dev_priv, i) {
>> +    for_each_fw_domain(fw_domain, dev_priv) {
>>           seq_printf(m, "%s.wake_count = %u\n",
>> -               intel_uncore_forcewake_domain_to_str(i),
>> +               intel_uncore_forcewake_domain_to_str(fw_domain->id),
>>                  fw_domain->wake_count);
>>       }
>>       spin_unlock_irq(&dev_priv->uncore.lock);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 7d4c704d7d75..160f980f0368 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -665,6 +665,7 @@ struct intel_uncore {
>>       struct intel_uncore_forcewake_domain {
>>           struct drm_i915_private *i915;
>>           enum forcewake_domain_id id;
>> +        enum forcewake_domains mask;
>
> At present this mask will always have only one bit set, but I suppose
> there might be some utility in allowing multiple bits (virtual domains?)

I did not like the name mask myself but couldn't think of anything 
better. Do you have a suggestion?

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx