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

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

On 12/11/2021 13:40, Tvrtko Ursulin wrote:
> 
> 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?

Ping on this - don't we want to fix igfx_off option to not apply to 
discrete GPUs?

Regards,

Tvrtko