[Intel-gfx] [PATCH 5/6] drm/i915: dynamically allocate forcewake domains

[Daniele Ceraolo Spurio]

On 6/18/19 4:23 PM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2019-06-19 00:06:39)
>>
>>
>> On 6/18/19 2:23 AM, Tvrtko Ursulin wrote:
>>>
>>> On 17/06/2019 19:09, Daniele Ceraolo Spurio wrote:
>>>> -static void intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>>> +static int intel_uncore_fw_domains_init(struct intel_uncore *uncore)
>>>>    {
>>>>        struct drm_i915_private *i915 = uncore->i915;
>>>> +    int ret;
>>>>        GEM_BUG_ON(!intel_uncore_has_forcewake(uncore));
>>>> +#define __fw_domain_init(id__, set__, ack__) \
>>>> +    ret = fw_domain_init(uncore, (id__), (set__), (ack__)); \
>>>> +    if (ret) \
>>>> +        goto out_clean;
>>>
>>> Hidden control flow is slightly objectionable but I don't offer any nice
>>> alternatives so I guess will have to pass. Or maybe accumulate the error
>>> (err |= fw_domain_init(..)) as you go and then cleanup at the end if any
>>> failed?
>>
>> I'd prefer to avoid accumulating the error since it'd just cause us to
>> having to unroll more domains when we could've stopped early.
>>
>>>
>>> On the other hand the idea slightly conflicts with my other suggestion
>>> to rename existing fw_domain_init to __fw_domain_init and call the macro
>>> fw_domain_init and avoid all the churn below.
>>>
>>
>> I'll pick this suggestion among the 2, unless there is another
>> suggestion on how to avoid the hidden goto.
> 
> Be careful, or you'll give Tvrtko the chance to suggest table driven
> setup. Maybe?
> -Chris
> 

I did consider using a table myself, but the differences between the 
domains are not easy to put in a table since some of them are per-gen 
and other per-platform. We could combine a table with information in the 
device_info struct like we do for the engines, but that felt a bit like 
overkill to me.

Daniele