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

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Lu Baolu <baolu.lu@linux.intel.com>, Intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org,
	Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Subject: Re: [PATCH] drm/i915: Use per device iommu check
Date: Wed, 10 Nov 2021 09:30:48 +0000	[thread overview]
Message-ID: <4d1a0ab9-e0d8-2ed9-1fc4-9ffaf2f19bef@linux.intel.com> (raw)
In-Reply-To: <6e8c55a7-45b6-57ab-35f7-d522401efccb@linux.intel.com>

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.

Regards,

Tvrtko

> 
> A possible way could 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;
> 
>      ... ...
> }
> 
> Actually I don't like this either since it checks the domain->type out
> of the iommu subsystem. We could refactor this later by export an iommu
> interface for this check.
> 
> Best regards,
> baolu