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

Robin Murphy <robin.murphy@xxxxxxx> · Thu, 25 Nov 2021 11:47:37 +0000

On 2021-11-25 10:42, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

With both integrated and discrete Intel GPUs in a system, the current
global check of intel_iommu_gfx_mapped, as done from intel_vtd_active()
may not be completely accurate.

In this patch we add i915 parameter to intel_vtd_active() in order to
prepare it for multiple GPUs and we also change the check away from Intel
specific intel_iommu_gfx_mapped (global exported by the Intel IOMMU
driver) to probing the presence of IOMMU domain on a specific device
using iommu_get_domain_for_dev().

FWIW the way you have it now is functionally equivalent to using 
device_iommu_mapped(), which I think might be slightly clearer for the 
current intent, but I don't have a significantly strong preference 
(after all, this *was* the de-facto way of checking before 
device_iommu_mapped() was introduced, and there are still other examples 
of it around). So from the IOMMU perspective,

Acked-by: Robin Murphy <robin.murphy@xxxxxxx>

Perhaps the AGP driver could also be tweaked and intel_iommu_gfx_mapped 
cleaned away entirely, but I'll leave that for Baolu to think about :)

Cheers,
Robin.

It was suggested to additionally check for __IOMMU_DOMAIN_PAGING bit
present in the returned iommu domain, however I opted not to do that at
this point.

Checking for this flag would detect whether IOMMU is in address
translation mode, with the assumption that is the only relevant question.
Downside to that is that in identity mapping (pass-through) mode IOMMU
hardware is still active, sitting on the communication path, just not
doing address translation.

My rationale was, that for the many intel_vtd_active() checks in our code
base, while some clearly are about performance impact of address
translation, some may be about working around functional issues when the
IOMMU hardware is simply being active. There also may be some performance
impact in pass-through mode, but I have not specifically attempted to
measure it.

Therefore the safest option feels to be to keep intel_vtd_active()
answering the question of "is the IOMMU hardware active" for this device.
If in the future we want to expand the set of questions to "is IOMMU
active and doing address translation" we can easily do that by adding a
new helper to be called from appropriate sites.

v2:
   * Check for dmar translation specifically, not just iommu domain. (Baolu)

v3:
  * Go back to plain "any domain" check for now, rewrite commit message.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Cc: Robin Murphy <robin.murphy@xxxxxxx>
---
  drivers/gpu/drm/i915/display/intel_bw.c      |  2 +-
  drivers/gpu/drm/i915/display/intel_display.c |  2 +-
  drivers/gpu/drm/i915/display/intel_fbc.c     |  2 +-
  drivers/gpu/drm/i915/gem/i915_gem_stolen.c   |  2 +-
  drivers/gpu/drm/i915/gem/i915_gemfs.c        |  2 +-
  drivers/gpu/drm/i915/gt/intel_ggtt.c         |  4 ++--
  drivers/gpu/drm/i915/i915_debugfs.c          |  1 +
  drivers/gpu/drm/i915/i915_driver.c           |  7 +++++++
  drivers/gpu/drm/i915/i915_drv.h              | 13 +++++++------
  drivers/gpu/drm/i915/i915_gpu_error.c        |  5 +----
  drivers/gpu/drm/i915/intel_device_info.c     | 14 +-------------
  drivers/gpu/drm/i915/intel_pm.c              |  2 +-
  12 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index abec394f6869..2da4aacc956b 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -634,7 +634,7 @@ static unsigned int intel_bw_data_rate(struct drm_i915_private *dev_priv,
  	for_each_pipe(dev_priv, pipe)
  		data_rate += bw_state->data_rate[pipe];
  
-	if (DISPLAY_VER(dev_priv) >= 13 && intel_vtd_active())
+	if (DISPLAY_VER(dev_priv) >= 13 && intel_vtd_active(dev_priv))
  		data_rate = data_rate * 105 / 100;
  
  	return data_rate;
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b2d51cd79d6c..1ef77ba7f645 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1293,7 +1293,7 @@ static bool needs_async_flip_vtd_wa(const struct intel_crtc_state *crtc_state)
  {
  	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
  
-	return crtc_state->uapi.async_flip && intel_vtd_active() &&
+	return crtc_state->uapi.async_flip && intel_vtd_active(i915) &&
  		(DISPLAY_VER(i915) == 9 || IS_BROADWELL(i915) || IS_HASWELL(i915));
  }
  
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index d0c34bc3af6c..614e8697c068 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -1677,7 +1677,7 @@ static int intel_sanitize_fbc_option(struct drm_i915_private *i915)
  static bool need_fbc_vtd_wa(struct drm_i915_private *i915)
  {
  	/* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt */
-	if (intel_vtd_active() &&
+	if (intel_vtd_active(i915) &&
  	    (IS_SKYLAKE(i915) || IS_BROXTON(i915))) {
  		drm_info(&i915->drm,
  			 "Disabling framebuffer compression (FBC) to prevent screen flicker with VT-d enabled\n");
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index ddd37ccb1362..cf100c0ea3b7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -399,7 +399,7 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
  		return 0;
  	}
  
-	if (intel_vtd_active() && GRAPHICS_VER(i915) < 8) {
+	if (intel_vtd_active(i915) && GRAPHICS_VER(i915) < 8) {
  		drm_notice(&i915->drm,
  			   "%s, disabling use of stolen memory\n",
  			   "DMAR active");
diff --git a/drivers/gpu/drm/i915/gem/i915_gemfs.c b/drivers/gpu/drm/i915/gem/i915_gemfs.c
index dbdbdc344d87..11cd66d183e6 100644
--- a/drivers/gpu/drm/i915/gem/i915_gemfs.c
+++ b/drivers/gpu/drm/i915/gem/i915_gemfs.c
@@ -31,7 +31,7 @@ int i915_gemfs_init(struct drm_i915_private *i915)
  	 */
  
  	opts = NULL;
-	if (intel_vtd_active()) {
+	if (intel_vtd_active(i915)) {
  		if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
  			static char huge_opt[] = "huge=within_size"; /* r/w */
  
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 555111c3bee5..110d3944f9a2 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -106,7 +106,7 @@ static bool needs_idle_maps(struct drm_i915_private *i915)
  	 * Query intel_iommu to see if we need the workaround. Presumably that
  	 * was loaded first.
  	 */
-	if (!intel_vtd_active())
+	if (!intel_vtd_active(i915))
  		return false;
  
  	if (GRAPHICS_VER(i915) == 5 && IS_MOBILE(i915))
@@ -1233,7 +1233,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *i915)
  	if (ret)
  		return ret;
  
-	if (intel_vtd_active())
+	if (intel_vtd_active(i915))
  		drm_info(&i915->drm, "VT-d active for gfx access\n");
  
  	return 0;
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fe638b5da7c0..390d541f64ea 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -65,6 +65,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
  
  	intel_device_info_print_static(INTEL_INFO(i915), &p);
  	intel_device_info_print_runtime(RUNTIME_INFO(i915), &p);
+	i915_print_iommu_status(i915, &p);
  	intel_gt_info_print(&i915->gt.info, &p);
  	intel_driver_caps_print(&i915->caps, &p);
  
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index a13666627dad..bbc99fc5888f 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -741,6 +741,12 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
  	i915_gem_driver_unregister(dev_priv);
  }
  
+void
+i915_print_iommu_status(struct drm_i915_private *i915, struct drm_printer *p)
+{
+	drm_printf(p, "iommu: %s\n", enableddisabled(intel_vtd_active(i915)));
+}
+
  static void i915_welcome_messages(struct drm_i915_private *dev_priv)
  {
  	if (drm_debug_enabled(DRM_UT_DRIVER)) {
@@ -756,6 +762,7 @@ static void i915_welcome_messages(struct drm_i915_private *dev_priv)
  
  		intel_device_info_print_static(INTEL_INFO(dev_priv), &p);
  		intel_device_info_print_runtime(RUNTIME_INFO(dev_priv), &p);
+		i915_print_iommu_status(dev_priv, &p);
  		intel_gt_info_print(&dev_priv->gt.info, &p);
  	}
  
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1bfadd9127fc..da3b32ae65e0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1760,26 +1760,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();
  }
  
+void
+i915_print_iommu_status(struct drm_i915_private *i915, struct drm_printer *p);
+
  static inline bool intel_scanout_needs_vtd_wa(struct drm_i915_private *dev_priv)
  {
-	return GRAPHICS_VER(dev_priv) >= 6 && intel_vtd_active();
+	return GRAPHICS_VER(dev_priv) >= 6 && intel_vtd_active(dev_priv);
  }
  
  static inline bool
  intel_ggtt_update_needs_vtd_wa(struct drm_i915_private *i915)
  {
-	return IS_BROXTON(i915) && intel_vtd_active();
+	return IS_BROXTON(i915) && intel_vtd_active(i915);
  }
  
  static inline bool
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 2a2d7643b551..3f39d6641894 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1750,10 +1750,7 @@ static void capture_gen(struct i915_gpu_coredump *error)
  	error->wakelock = atomic_read(&i915->runtime_pm.wakeref_count);
  	error->suspended = i915->runtime_pm.suspended;
  
-	error->iommu = -1;
-#ifdef CONFIG_INTEL_IOMMU
-	error->iommu = intel_iommu_gfx_mapped;
-#endif
+	error->iommu = intel_vtd_active(i915);
  	error->reset_count = i915_reset_count(&i915->gpu_error);
  	error->suspend_count = i915->suspend_count;
  
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 6e6b317bc33c..e6605b5181a5 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -83,17 +83,6 @@ const char *intel_platform_name(enum intel_platform platform)
  	return platform_names[platform];
  }
  
-static const char *iommu_name(void)
-{
-	const char *msg = "n/a";
-
-#ifdef CONFIG_INTEL_IOMMU
-	msg = enableddisabled(intel_iommu_gfx_mapped);
-#endif
-
-	return msg;
-}
-
  void intel_device_info_print_static(const struct intel_device_info *info,
  				    struct drm_printer *p)
  {
@@ -114,7 +103,6 @@ void intel_device_info_print_static(const struct intel_device_info *info,
  		drm_printf(p, "display version: %u\n", info->display.ver);
  
  	drm_printf(p, "gt: %d\n", info->gt);
-	drm_printf(p, "iommu: %s\n", iommu_name());
  	drm_printf(p, "memory-regions: %x\n", info->memory_regions);
  	drm_printf(p, "page-sizes: %x\n", info->page_sizes);
  	drm_printf(p, "platform: %s\n", intel_platform_name(info->platform));
@@ -374,7 +362,7 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
  			info->display.has_dsc = 0;
  	}
  
-	if (GRAPHICS_VER(dev_priv) == 6 && intel_vtd_active()) {
+	if (GRAPHICS_VER(dev_priv) == 6 && intel_vtd_active(dev_priv)) {
  		drm_info(&dev_priv->drm,
  			 "Disabling ppGTT for VT-d support\n");
  		info->ppgtt_type = INTEL_PPGTT_NONE;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 01fa3fac1b57..cff0f32bedc9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -98,7 +98,7 @@ static void gen9_init_clock_gating(struct drm_i915_private *dev_priv)
  		 * "Plane N strech max must be programmed to 11b (x1)
  		 *  when Async flips are enabled on that plane."
  		 */
-		if (!IS_GEMINILAKE(dev_priv) && intel_vtd_active())
+		if (!IS_GEMINILAKE(dev_priv) && intel_vtd_active(dev_priv))
  			intel_uncore_rmw(&dev_priv->uncore, CHICKEN_PIPESL_1(pipe),
  					 SKL_PLANE1_STRETCH_MAX_MASK, SKL_PLANE1_STRETCH_MAX_X1);
  	}







