Re: [PATCH v9] drm/i915/icl: Check for fused-off VDBOX and VEBOX instances

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 2/27/2018 4:34 AM, Oscar Mateo wrote:
>
>
> On 2/25/2018 9:22 PM, Sagar Arun Kamble wrote:
>>
>>
>> On 2/23/2018 4:35 AM, Oscar Mateo wrote:
>>>
>>>
>> <snip>
>>>>> + * We might have detected that some engines are fused off after 
>>>>> we initialized
>>>>> + * the forcewake domains. Prune them, to make sure they only 
>>>>> reference existing
>>>>> + * engines.
>>>>> + */
>>>>> +void intel_uncore_prune(struct drm_i915_private *dev_priv)
>>>>> +{
>>>>> +    if (INTEL_GEN(dev_priv) >= 11) {
>>>>> +        enum forcewake_domains fw_domains = 
>>>>> dev_priv->uncore.fw_domains;
>>>>> +        enum forcewake_domain_id domain_id;
>>>>> +        int i;
>>>>> +
>>>>> +        for (i = 0; i < I915_MAX_VCS; i++) {
>>>>> +            domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
>>>>> +
>>>>> +            if (HAS_ENGINE(dev_priv, _VCS(i)))
>>>>> +                continue;
>>>>> +
>>>>> +            if (fw_domains & BIT(domain_id))
>>>> fw_domains check seems redundant as it is initialized based on 
>>>> HAS_ENGINE.
>>>> we can just have
>>>> if (!HAS_ENGINE(dev_priv, _VCS(i)))
>>>>     fw_domain_fini(dev_priv, domain_id);
>>>
>>> I don't want to call fw_domain_fini on something we never 
>>> initialized in the first place (e.g. VCS1/3 and VECS1/2/3 on an 
>>> ICL-LP).
>>>
>> Right. Makes sense.
>> for loop iterating over engines based on ring_mask can help us rely 
>> on only HAS_ENGINE condition and then we can have complete pruning in 
>> single for loop.
>> what do you think?
>
> Hmmm.. I'm not sure I follow: intel_device_info_init_mmio modifies 
> ring_mask, so if I loop over engines based on ring_mask I am going to 
> miss those I want to prune, right?
>
Oops ... you are right ...
My suggestion about skipping fw_domains check will not work currently. 
In future may be if we create default ring_mask and runtime ring_mask it 
can be reworked.

Other suggestion to use single for loop (for_each_engine()) can be done 
I think.
It will make it generic for all engine types.  Below is what I am 
thinking of as part of intel_uncore_prune:

for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
     if (HAS_ENGINE(dev_priv, i))
         continue;
     if (fw_domains & BIT(i))
         fw_domain_fini(dev_priv, i);
}
>>>>> + fw_domain_fini(dev_priv, domain_id);
>>>>> +        }
>>>>> +
>>>>> +        for (i = 0; i < I915_MAX_VECS; i++) {
>>>>> +            domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
>>>>> +
>>>>> +            if (HAS_ENGINE(dev_priv, _VECS(i)))
>>>>> +                continue;
>>>>> +
>>>>> +            if (fw_domains & BIT(domain_id))
>>>>> +                fw_domain_fini(dev_priv, domain_id);
>>>>> +        }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>   void intel_uncore_fini(struct drm_i915_private *dev_priv)
>>>>>   {
>>>>>       /* Paranoia: make sure we have disabled everything before we 
>>>>> exit. */
>>>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h 
>>>>> b/drivers/gpu/drm/i915/intel_uncore.h
>>>>> index 53ef77d..28feabf 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_uncore.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_uncore.h
>>>>> @@ -129,6 +129,7 @@ struct intel_uncore {
>>>>>     void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
>>>>>   void intel_uncore_init(struct drm_i915_private *dev_priv);
>>>>> +void intel_uncore_prune(struct drm_i915_private *dev_priv);
>>>>>   bool intel_uncore_unclaimed_mmio(struct drm_i915_private 
>>>>> *dev_priv);
>>>>>   bool intel_uncore_arm_unclaimed_mmio_detection(struct 
>>>>> drm_i915_private *dev_priv);
>>>>>   void intel_uncore_fini(struct drm_i915_private *dev_priv);
>>>>
>>>
>>
>

-- 
Thanks,
Sagar

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