Re: [Intel-gfx] [PATCH] drm/i915: Use per device iommu check

[Lu Baolu <baolu.lu@linux.intel.com>]

On 2021/11/10 20:08, Tvrtko Ursulin wrote:
> 
> On 10/11/2021 12:04, Lu Baolu wrote:
>> On 2021/11/10 17:30, Tvrtko Ursulin wrote:
>>>
>>> On 10/11/2021 07:12, Lu Baolu wrote:
>>>> Hi Tvrtko,
>>>>
>>>> On 2021/11/9 20:17, Tvrtko Ursulin wrote:
>>>>> From: Tvrtko Ursulin<tvrtko.ursulin@intel.com>
>>>>>
>>>>> On igfx + dgfx setups, it appears that intel_iommu=igfx_off option 
>>>>> only
>>>>> disables the igfx iommu. Stop relying on global intel_iommu_gfx_mapped
>>>>> and probe presence of iommu domain per device to accurately reflect 
>>>>> its
>>>>> status.
>>>>>
>>>>> Signed-off-by: Tvrtko Ursulin<tvrtko.ursulin@intel.com>
>>>>> Cc: Lu Baolu<baolu.lu@linux.intel.com>
>>>>> ---
>>>>> Baolu, is my understanding here correct? Maybe I am confused by both
>>>>> intel_iommu_gfx_mapped and dmar_map_gfx being globals in the 
>>>>> intel_iommu
>>>>> driver. But it certainly appears the setup can assign some iommu 
>>>>> ops (and
>>>>> assign the discrete i915 to iommu group) when those two are set to 
>>>>> off.
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index e967cd08f23e..9fb38a54f1fe 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1763,26 +1763,27 @@ static inline bool run_as_guest(void)
>>>>   #define HAS_D12_PLANE_MINIMIZATION(dev_priv) 
>>>> (IS_ROCKETLAKE(dev_priv) || \
>>>>                             IS_ALDERLAKE_S(dev_priv))
>>>>
>>>> -static inline bool intel_vtd_active(void)
>>>> +static inline bool intel_vtd_active(struct drm_i915_private *i915)
>>>>   {
>>>> -#ifdef CONFIG_INTEL_IOMMU
>>>> -    if (intel_iommu_gfx_mapped)
>>>> +    if (iommu_get_domain_for_dev(i915->drm.dev))
>>>>           return true;
>>>> -#endif
>>>>
>>>>       /* Running as a guest, we assume the host is enforcing VT'd */
>>>>       return run_as_guest();
>>>>   }
>>>>
>>>> Have you verified this change? I am afraid that
>>>> iommu_get_domain_for_dev() always gets a valid iommu domain even
>>>> intel_iommu_gfx_mapped == 0.
>>>
>>> Yes it seems to work as is:
>>>
>>> default:
>>>
>>> # grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
>>> /sys/kernel/debug/dri/0/i915_capabilities:iommu: enabled
>>> /sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
>>>
>>> intel_iommu=igfx_off:
>>>
>>> # grep -i iommu /sys/kernel/debug/dri/*/i915_capabilities
>>> /sys/kernel/debug/dri/0/i915_capabilities:iommu: disabled
>>> /sys/kernel/debug/dri/1/i915_capabilities:iommu: enabled
>>>
>>> On my system dri device 0 is integrated graphics and 1 is discrete.
>>
>> The drm device 0 has a dedicated iommu. When the user request igfx not
>> mapped, the VT-d implementation will turn it off to save power. But for
>> shared iommu, you definitely will get it enabled.
> 
> Sorry I am not following, what exactly do you mean? Is there a platform 
> with integrated graphics without a dedicated iommu, in which case 
> intel_iommu=igfx_off results in intel_iommu_gfx_mapped == 0 and 
> iommu_get_domain_for_dev returning non-NULL?

Your code always work for an igfx with a dedicated iommu. This might be
always true on today's platforms. But from driver's point of view, we
should not make such assumption.

For example, if the iommu implementation decides not to turn off the
graphic iommu (perhaps due to some hw quirk or for graphic
virtualization), your code will be broken.

Best regards,
baolu