Re: [PATCH 08/10] drm/i915/selftests: use intel_gtt_view_is_*() helpers more

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/i915/selftests: incorrect view type check breaks igt_vma_remapped_gtt
--

commit b3029da419b0bcdce00e05ce10b2c3fff729069e
Author: Jani Nikula <jani.nikula@intel.com>

drm/i915/selftests: use intel_gtt_view_is_*() helpers more

This commit replaces direct view type comparisons with intel_gtt_view_is_*()
helpers.

> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
> index b16297a6a4e8d..ca340a94f7728 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
[ ... ]
> @@ -1057,7 +1057,7 @@ static int igt_vma_remapped_gtt(void *arg)
>  				goto out;
>  			}
>  
> -			GEM_BUG_ON(vma->gtt_view.type != I915_GTT_VIEW_NORMAL);
> +			GEM_BUG_ON(!intel_gtt_view_is_normal(&vma->gtt_view));
>  
>  			map = i915_vma_pin_iomap(vma);
>  			i915_vma_unpin(vma);
[ ... ]
> @@ -1072,7 +1072,7 @@ static int igt_vma_remapped_gtt(void *arg)
>  					u32 exp = y << 16 | x;
>  					u32 val;
>  
> -					if (*t == I915_GTT_VIEW_ROTATED)
> +					if (intel_gtt_view_is_rotated(&vma->gtt_view))
>  						src_idx = rotated_index(&view.rotated, 0, x, y);
>  					else
>  						src_idx = remapped_index(&view.remapped, 0, x, y);

[Severity: Medium]
Will this check always evaluate to false? Earlier in igt_vma_remapped_gtt(),
the vma is pinned with a normal view, and verified with the GEM_BUG_ON
shown above. Because vma->gtt_view is normal, this condition will fail even
when testing a rotated view, incorrectly taking the remapped_index() path
and causing spurious test failures.

Should this check continue to evaluate the original test context instead,
such as by using intel_gtt_view_is_rotated(&view)?

> @@ -1081,7 +1081,7 @@ static int igt_vma_remapped_gtt(void *arg)
>  					val = ioread32(&map[offset / sizeof(*map)]);
>  					if (val != exp) {
>  						pr_err("%s VMA write test failed, expected 0x%x, found 0x%x\n",
> -						       *t == I915_GTT_VIEW_ROTATED ? "Rotated" : "Remapped",
> +						       intel_gtt_view_is_rotated(&vma->gtt_view) ? "Rotated" : "Remapped",
>  						       exp, val);
>  						i915_vma_unpin_iomap(vma);
>  						err = -EINVAL;

[Severity: Medium]
Does this error message also need to check the original view context? For
the same reason as above, if the test fails for a rotated view, it will
incorrectly print Remapped because vma->gtt_view is normal here.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789502902.git.jani.nikula@intel.com?part=8