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

From: Oscar Mateo <oscar.mateo@intel.com>
To: Sagar Arun Kamble <sagar.a.kamble@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH v9] drm/i915/icl: Check for fused-off VDBOX and VEBOX instances
Date: Wed, 28 Feb 2018 09:59:54 -0800	[thread overview]
Message-ID: <760c236f-45bf-2cb7-9b78-476a1c2b0b03@intel.com> (raw)
In-Reply-To: <dc18b8b0-fb49-ee49-4b28-1d92faec59be@intel.com>

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).

>>>>>> + 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);
>>>>>
>>>>
>>>
>>
>

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