[PATCH] drm/i915: Use per device iommu check

[Tvrtko Ursulin]

On 10/11/2021 12:35, Lu Baolu wrote:
> 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 at 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 at intel.com>
>>>>>> Cc: Lu Baolu<baolu.lu at 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.

If I got it right, this would go back to your earlier recommendation to 
have the check look like this:

static bool intel_vtd_active(struct drm_i915_private *i915)
{
         struct iommu_domain *domain;

         domain = iommu_get_domain_for_dev(i915->drm.dev);
         if (domain && (domain->type & __IOMMU_DOMAIN_PAGING))
                 return true;
	...

This would be okay as a first step?

Elsewhere in the thread Robin suggested looking at the dec->dma_ops and 
comparing against iommu_dma_ops. These two solution would be effectively 
the same?

Regards,

Tvrtko