Re: [PATCH 33/37] drm/i915: Verify page layout for rotated VMA

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

On 11/01/2017 21:09, Chris Wilson wrote:
> Exercise creating rotated VMA and checking the page order within.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/selftests/i915_vma.c | 130 ++++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
> index d229adabc5f8..95c5db2b0881 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
> @@ -287,11 +287,141 @@ static int igt_vma_pin1(void *arg)
>  	return err;
>  }
>
> +static unsigned long rotated_index(const struct intel_rotation_info *r,
> +				   unsigned int n,
> +				   unsigned int x,
> +				   unsigned int y)
> +{
> +	return r->plane[n].stride * (r->plane[n].height - y - 1) + x;
> +}
> +
> +static struct scatterlist *
> +assert_rotated(struct drm_i915_gem_object *obj,
> +	       const struct intel_rotation_info *r, unsigned int n,
> +	       struct scatterlist *sg)
> +{
> +	unsigned int x, y;
> +
> +	for (x = 0; x < r->plane[n].width; x++) {
> +		for (y = 0; y < r->plane[n].height; y++) {
> +			unsigned long src_idx;
> +			dma_addr_t src;
> +
> +			src_idx = rotated_index(r, n, x, y);
> +			src = i915_gem_object_get_dma_address(obj, src_idx);

Right, this is actually more like unrotate, from rotated coords to 
unrotated index. I'll assume the formula is correct if it passes. :)

> +
> +			if (sg_dma_len(sg) != PAGE_SIZE) {
> +				pr_err("Invalid sg.length, found %d, expected %lu for rotated page (%d, %d) [src index %lu]\n",
> +				       sg_dma_len(sg), PAGE_SIZE,
> +				       x, y, src_idx);
> +				return NULL;
> +			}
> +
> +			if (sg_dma_address(sg) != src) {
> +				pr_err("Invalid address for rotated page (%d, %d) [src index %lu]\n",
> +				       x, y, src_idx);
> +				return NULL;
> +			}
> +
> +			sg = ____sg_next(sg);
> +		}
> +	}
> +
> +	return sg;
> +}
> +
> +static int igt_vma_rotate(void *arg)
> +{
> +	struct drm_i915_private *i915 = arg;
> +	struct drm_i915_gem_object *obj;
> +	const unsigned int width = 6;
> +	const unsigned int height = 4;
> +	const unsigned int npages = 24;
> +	struct i915_vma *vma;
> +	struct i915_ggtt_view view;
> +	struct scatterlist *sg;
> +	unsigned int n;
> +	int err = -ENOMEM;
> +
> +	obj = i915_gem_object_create_internal(i915, npages*PAGE_SIZE);
> +	if (IS_ERR(obj))
> +		goto err;
> +
> +	view.type = I915_GGTT_VIEW_ROTATED;
> +	view.rotated.plane[0].offset = 0;
> +	view.rotated.plane[0].width = width;
> +	view.rotated.plane[0].height = height;
> +	view.rotated.plane[0].stride = width;
> +
> +	view.rotated.plane[1].offset = 0;
> +	view.rotated.plane[1].width = height;
> +	view.rotated.plane[1].height = width;
> +	view.rotated.plane[1].stride = height;

Ahem, why are the width & height assignments different between the 
planes and how can a single assert possibly work with this? Perhaps it 
all works totally differently after Ville's rewrite, I don't know any 
more obviously.

> +
> +	vma = i915_gem_obj_lookup_or_create_vma(obj, &i915->ggtt.base, &view);
> +	if (IS_ERR(vma)) {
> +		err = PTR_ERR(vma);
> +		goto err_object;
> +	}
> +
> +	if (!i915_vma_is_ggtt(vma)) {
> +		pr_err("VMA is not in the GGTT!\n");
> +		err = -EINVAL;
> +		goto err_object;
> +	}
> +
> +	err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
> +	if (err)
> +		goto err_object;

Log message would be good.

> +
> +	if (memcmp(&vma->ggtt_view, &view, sizeof(view))) {
> +		pr_err("VMA mismatch upon creation!\n");
> +		err = -EINVAL;
> +		goto err_object;
> +	}

i915_vma_compare to cover any future tweaking in that area?

> +
> +	if (vma->size != 2*npages*PAGE_SIZE) {
> +		pr_err("VMA is wrong size, expected %lu, found %llu\n",
> +		       2*npages*PAGE_SIZE, vma->size);
> +		err = -EINVAL;
> +		goto err_object;
> +	}

Wait a minute, how can rotated view be bigger than the object?!

[after some head scratching and bringing up the code]

Right, you are playing tricks with pretend two planes. It would be good 
to split the test into one plane only first, and then the two-plane 
configuration. Probably with a "real" backing store, I mean object with 
enough pages for both planes and a proper offset.

> +
> +	if (vma->node.size < vma->size) {
> +		pr_err("VMA binding too small, expected %llu, found %llu\n",
> +		       vma->size, vma->node.size);
> +		err = -EINVAL;
> +		goto err_object;
> +	}
> +
> +	if (vma->pages == obj->mm.pages) {
> +		pr_err("VMA using unrotated object pages!\n");
> +		err = -EINVAL;
> +		goto err_object;
> +	}
> +
> +	sg = vma->pages->sgl;
> +	for (n = 0; n < ARRAY_SIZE(view.rotated.plane); n++) {
> +		sg = assert_rotated(obj, &view.rotated, n, sg);
> +		if (!sg) {
> +			pr_err("Inconsistent VMA pages for plane %d\n", n);
> +			err = -EINVAL;
> +			goto err_object;
> +		}
> +	}
> +
> +err_object:
> +	i915_gem_object_put(obj);
> +err:
> +	return err;
> +}
> +
>  int i915_vma_mock_selftests(void)
>  {
>  	static const struct i915_subtest tests[] = {
>  		SUBTEST(igt_vma_create),
>  		SUBTEST(igt_vma_pin1),
> +		SUBTEST(igt_vma_rotate),
>  	};
>  	struct drm_i915_private *i915;
>  	int err;
>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx