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

[Sagar Arun Kamble]

On 2/28/2018 11:29 PM, Oscar Mateo wrote:
>
>
> On 2/26/2018 9:49 PM, Sagar Arun Kamble wrote:
>>
>>
>> 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);
>> }
>
> This won't work either, because the index for engines and forcewake 
> domains is different. If you think it is important to make the prune 
> function more generic, I can translate between the two (but it will be 
> for naught if, as you mentioned, we are working to create separate 
> default ring_mask and runtime ring_mask in the future).
>
Yes. Translation will help (I thought of FW_D_ID_MEDIA to be reused for 
FW_D_ID_MEDIA_VDB0).
I think this patch can go in current shape and will pursue other changes 
as follow up based on inputs.

I feel making it generic will allow pruning to scale across engine types 
(if that is needed in future).
I am not sure if we want to pursue the default/runtime ring_mask change 
(another use case of this that i think is if user wants to know default 
config vs fused)
Tvrtko, Chris - what do you think?
>>>>>>> + 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