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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 12/11/2021 00:53, Lu Baolu wrote:
> On 11/11/21 11:06 PM, Tvrtko Ursulin wrote:
>>
>> 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@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.
>>
>> I tried your suggestion (checking for __IOMMU_DOMAIN_PAGING) and it 
>> works better, however I have observed one odd behaviour (for me at 
>> least).
>>
>> In short - why does the DMAR mode for the discrete device change 
>> depending on igfx_off parameter?
>>
>> Consider the laptop has these two graphics cards:
>>
>> # cat /sys/kernel/debug/dri/0/name
>> i915 dev=0000:00:02.0 unique=0000:00:02.0 # integrated
>>
>> # cat /sys/kernel/debug/dri/1/name
>> i915 dev=0000:03:00.0 unique=0000:03:00.0 # discrete
>>
>> Booting with different options:
>> ===============================
>>
>> default / intel_iommu=on
>> ------------------------
>>
>> # cat /sys/class/iommu/dmar0/devices/0000:00:02.0/iommu_group/type
>> DMA-FQ
>> # cat /sys/class/iommu/dmar2/devices/0000:03:00.0/iommu_group/type
>> DMA-FQ
>>
>> # 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
>>
>> All good.
>>
>> intel_iommu=igfx_off
>> --------------------
>>
>> ## no dmar0 in sysfs
>> # cat /sys/class/iommu/dmar2/devices/0000:03:00.0/iommu_group/type
>> identity
>>
>> Unexpected!?
>>
>> # 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: disabled # At least 
>> the i915 patch detects it correctly.
>>
>> intel_iommu=off
>> ---------------
>>
>> ## no dmar0 in sysfs
>> ## no dmar2 in sysfs
>>
>> # 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: disabled
>>
>> All good.
>>
>> The fact discrete graphics changes from translated to pass-through 
>> when igfx_off is set is surprising to me. Is this a bug?
> 
> The existing VT-d implementation doesn't distinguish igfx from dgfx. It
> only checks whether the device is of a display class:
> 
> #define IS_GFX_DEVICE(pdev) ((pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY)
> 
> When igfx_off is specified, all graphic devices will put into pass-
> through (the same meaning as identity mapping) mode. For igfx, since
> the iommu is always dedicated, hence it further turn off the iommu
> (hence there's no iommu domain) to save power.

Ah okay. Is this something we want to change/fix?

Regards,

Tvrtko