Currently, we don't call dma_set_max_seg_size() for i915 because we intentionally do not limit the segment length that the device supports. However, this results in a warning being emitted if we try to map anything larger than SZ_64K on a kernel with CONFIG_DMA_API_DEBUG_SG enabled:
[ 7.751926] DMA-API: i915 0000:00:02.0: mapping sg segment longer than device claims to support [len=98304] [max=65536] [ 7.751934] WARNING: CPU: 5 PID: 474 at kernel/dma/debug.c:1220 debug_dma_map_sg+0x20f/0x340
This was originally brought up on https://bugs.freedesktop.org/show_bug.cgi?id=108517 , and the consensus there was it wasn't really useful to set a limit (and that dma-debug isn't really all that useful for i915 in the first place). Unfortunately though, CONFIG_DMA_API_DEBUG_SG is enabled in the debug configs for various distro kernels. Since a WARN_ON() will disable automatic problem reporting (and cause any CI with said option enabled to start complaining), we really should just fix the problem.
Note that as me and Chris Wilson discussed, the other solution for this would be to make DMA-API not make such assumptions when a driver hasn't explicitly set a maximum segment size. But, taking a look at the commit which originally introduced this behavior, commit 78c47830a5cb ("dma-debug: check scatterlist segments"), there is an explicit mention of this assumption and how it applies to devices with no segment size:
Conversely, devices which are less limited than the rather conservative defaults, or indeed have no limitations at all (e.g. GPUs with their own internal MMU), should be encouraged to set appropriate dma_parms, as they may get more efficient DMA mapping performance out of it.
So unless there's any concerns (I'm open to discussion!), let's just follow suite and call dma_set_max_seg_size() with UINT_MAX as our limit to silence any warnings.
Signed-off-by: Lyude Paul lyude@redhat.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: stable@vger.kernel.org # v4.18+ --- drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 0b81e0b64393..a1475039d182 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3152,6 +3152,11 @@ static int ggtt_probe_hw(struct i915_ggtt *ggtt, struct intel_gt *gt) if (ret) return ret;
+ /* We don't have a max segment size, so set it to the max so sg's + * debugging layer doesn't complain + */ + dma_set_max_seg_size(ggtt->vm.dma, UINT_MAX); + if ((ggtt->vm.total - 1) >> 32) { DRM_ERROR("We never expected a Global GTT with more than 32bits" " of address space! Found %lldM!\n",
Now that we've fixed i915 so that it sets a max SG segment length and gotten rid of the relevant warnings, let's enable CONFIG_DMA_API_DEBUG_SG for intel-ci so that we can catch issues like this in the future as well.
Signed-off-by: Lyude Paul lyude@redhat.com Cc: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/Kconfig.debug | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 00786a142ff0..ad8d3cd63c9f 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -32,6 +32,7 @@ config DRM_I915_DEBUG select DRM_DEBUG_SELFTEST select DMABUF_SELFTESTS select SW_SYNC # signaling validation framework (igt/syncobj*) + select DMA_API_DEBUG_SG select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST select DRM_I915_DEBUG_RUNTIME_PM
Quoting Lyude Paul (2019-08-22 21:31:27)
Now that we've fixed i915 so that it sets a max SG segment length and gotten rid of the relevant warnings, let's enable CONFIG_DMA_API_DEBUG_SG for intel-ci so that we can catch issues like this in the future as well.
Signed-off-by: Lyude Paul lyude@redhat.com Cc: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/i915/Kconfig.debug | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 00786a142ff0..ad8d3cd63c9f 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -32,6 +32,7 @@ config DRM_I915_DEBUG select DRM_DEBUG_SELFTEST select DMABUF_SELFTESTS select SW_SYNC # signaling validation framework (igt/syncobj*)
select DMA_API_DEBUG as well for it to be enabled, no recursive dependency solver in Kconfig.
select DMA_API_DEBUG_SG select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST select DRM_I915_DEBUG_RUNTIME_PM-- 2.21.0
Now that we've fixed i915 so that it sets a max SG segment length and gotten rid of the relevant warnings, let's enable CONFIG_DMA_API_DEBUG_SG for intel-ci so that we can catch issues like this in the future as well.
Changes since v2: * Also select DMA_API_DEBUG - Chris Wilson
Signed-off-by: Lyude Paul lyude@redhat.com Cc: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/Kconfig.debug | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug index 00786a142ff0..cb929d0218e8 100644 --- a/drivers/gpu/drm/i915/Kconfig.debug +++ b/drivers/gpu/drm/i915/Kconfig.debug @@ -32,6 +32,8 @@ config DRM_I915_DEBUG select DRM_DEBUG_SELFTEST select DMABUF_SELFTESTS select SW_SYNC # signaling validation framework (igt/syncobj*) + select DMA_API_DEBUG + select DMA_API_DEBUG_SG select DRM_I915_SW_FENCE_DEBUG_OBJECTS select DRM_I915_SELFTEST select DRM_I915_DEBUG_RUNTIME_PM
Quoting Lyude Paul (2019-08-22 21:31:26)
Currently, we don't call dma_set_max_seg_size() for i915 because we intentionally do not limit the segment length that the device supports. However, this results in a warning being emitted if we try to map anything larger than SZ_64K on a kernel with CONFIG_DMA_API_DEBUG_SG enabled:
[ 7.751926] DMA-API: i915 0000:00:02.0: mapping sg segment longer than device claims to support [len=98304] [max=65536] [ 7.751934] WARNING: CPU: 5 PID: 474 at kernel/dma/debug.c:1220 debug_dma_map_sg+0x20f/0x340
This was originally brought up on https://bugs.freedesktop.org/show_bug.cgi?id=108517 , and the consensus there was it wasn't really useful to set a limit (and that dma-debug isn't really all that useful for i915 in the first place). Unfortunately though, CONFIG_DMA_API_DEBUG_SG is enabled in the debug configs for various distro kernels. Since a WARN_ON() will disable automatic problem reporting (and cause any CI with said option enabled to start complaining), we really should just fix the problem.
Note that as me and Chris Wilson discussed, the other solution for this would be to make DMA-API not make such assumptions when a driver hasn't explicitly set a maximum segment size. But, taking a look at the commit which originally introduced this behavior, commit 78c47830a5cb ("dma-debug: check scatterlist segments"), there is an explicit mention of this assumption and how it applies to devices with no segment size:
Conversely, devices which are less limited than the rather conservative defaults, or indeed have no limitations at all (e.g. GPUs with their own internal MMU), should be encouraged to set appropriate dma_parms, as they may get more efficient DMA mapping performance out of it.So unless there's any concerns (I'm open to discussion!), let's just follow suite and call dma_set_max_seg_size() with UINT_MAX as our limit to silence any warnings.
Signed-off-by: Lyude Paul lyude@redhat.com Cc: Chris Wilson chris@chris-wilson.co.uk Cc: stable@vger.kernel.org # v4.18+
drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 0b81e0b64393..a1475039d182 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3152,6 +3152,11 @@ static int ggtt_probe_hw(struct i915_ggtt *ggtt, struct intel_gt *gt) if (ret) return ret;
/* We don't have a max segment size, so set it to the max so sg's* debugging layer doesn't complain*/dma_set_max_seg_size(ggtt->vm.dma, UINT_MAX);
The rest of the dma setup is in i915_driver_hw_probe, so I would put it there just after dma_set_coherent_mask() (maybe one day even being brave enough to pull out those to their own function).
I think I've made my point about the futility of dma-debug and you've made yours about the simplicity of the patch to shut it up, so move it across and Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Lyude Paul