[Intel-gfx] [PATCH v2 32/38] drm/i915: Verify page layout for rotated VMA

[Tvrtko Ursulin]

On 01/02/2017 14:55, Chris Wilson wrote:
> On Wed, Feb 01, 2017 at 02:33:22PM +0000, Tvrtko Ursulin wrote:

[snip]

>>> +		{ }
>>> +	}, *a, *b;
>>> +	const unsigned int max_pages = 64;
>>> +	int err = -ENOMEM;
>>> +
>>> +	/* Create VMA for many different combinations of planes and check
>>> +	 * that the page layout within the rotated VMA match our expectations.
>>> +	 */
>>> +
>>> +	obj = i915_gem_object_create_internal(i915, max_pages * PAGE_SIZE);
>>> +	if (IS_ERR(obj))
>>> +		goto err;
>>> +
>>> +	for (a = planes; a->width; a++) {
>>> +		for (b = planes + ARRAY_SIZE(planes); b-- != planes; ) {
>>> +			struct i915_ggtt_view view;
>>> +			struct scatterlist *sg;
>>> +			unsigned int n, max_offset;
>>> +
>>> +			max_offset = max(a->stride * a->height,
>>> +					 b->stride * b->height);
>>
>> It shouldn't be min?
>>
>>> +			GEM_BUG_ON(max_offset >= max_pages);
>>> +			max_offset = max_pages - max_offset;
>
> No, because it is inverted ^

I see.

>>> +			view.type = I915_GGTT_VIEW_ROTATED;
>>> +			view.rotated.plane[0] = *a;
>>> +			view.rotated.plane[1] = *b;
>>
>> Single plane tests could be added as well.
>
> There are. Second plane is set to {0}. That's the only way to do single
> plane tests, as I was thinking second plane with a first plane would be
> illegal?

Missed that.


>>> +
>>> +			for_each_prime_number_from(view.rotated.plane[0].offset, 0, max_offset) {
>>> +				for_each_prime_number_from(view.rotated.plane[1].offset, 0, max_offset) {
>>
>> I would try all offsets here and not only primes since it is super
>> fast and more importantly more realistic.
>
> I was worried about the combinatorial explosion. We could have upto
> 65536 checks for each pair of planes (currently x20).

There is at least one even offset so OK. :)

>>> +					struct i915_address_space *vm =
>>> +						&i915->ggtt.base;
>>> +					struct i915_vma *vma;
>>> +
>>> +					vma = i915_vma_instance(obj, vm, &view);
>>> +					if (IS_ERR(vma)) {
>>> +						err = PTR_ERR(vma);
>>> +						goto err_object;
>>> +					}
>>> +
>>> +					if (!i915_vma_is_ggtt(vma) ||
>>> +					    vma->vm != vm) {
>>> +						pr_err("VMA is not in the GGTT!\n");
>>> +						err = -EINVAL;
>>> +						goto err_object;
>>> +					}
>>> +
>>> +					if (memcmp(&vma->ggtt_view, &view, sizeof(view))) {
>>
>> Just because rotation is the largest view! :) Need to use the "type" here.
>
> I wasn't really sure the value in doing both memcmp() and
> i915_vma_compare(). I think I'm just going to stick with
> i915_vma_compare() only.

I'm OK with that. Wanted even to suggest dropping the is_ggtt test since 
that feels should happen in a more basic VMA creation test. But if such 
doesn't exist then it's fine.

Regards,

Tvrtko