Re: [PATCH] tests/gem_userptr_blits: Expanded userptr test cases

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

Hi,

On 01/29/2014 08:11 PM, Daniel Vetter wrote:
> On Wed, Jan 29, 2014 at 01:30:54PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> A set of userptr test cases to support the new feature.

[snip]

> Minor thing on patch style: I'd split this up into 3 parts:
> - Extraction of the helpers - always useful to shine a bit light onto new
>    helper stuff so other people also notice them.
> - The new testcase.
> - Removal of the old vmap testcase.

I was afraid someone will say that, but was hoping for a lower bar 
since, to quote what yourself say later in this mail, "is a bit evil, 
but this is a testuite ;-)". :)

Okay, I'll split it up.

> The other patch style thing are the helpers - the forking_eviction stuff
> doesn't really sound like a bit of helper code. igt_exchange_uint32_t
> certainly is, but the other stuff I'd just put into a common source file
> which is included by both tests. Yeah #include "common.c" is a bit evil,
> but this is a testsuite ;-) Or just copy&paste the code into the userptr
> test, which is the approach I'd have done.

It would be too  much c&p for my liking so I chose this route.

> Now for test coverage it sounds like this testcases here has been more
> than good enough to shake down the userptr implementation, so I think
> we're covered.
>
> But there is also basic interface coverage for sanity-checking and
> defending against evil userspace. For this case here I think we need:
>
> - Tests with un-aligned ptr/size.
> - Tests with invalid flags.

Above are already there as test_usage_restrictions and test_input_checking.

> - Basic nastiness of handing in an invalid pointer.

You mean trying to map something which doesn't exist in user address 
space? Any idea how to obtain such a pointer? Or just use zero?

> Then there's all the interactions with other gem interfaces:
> - pwrite/pread/set_tiling: Should probably all fail with -EINVAL or
>    something like that.

Ok.

> - dma-buf export/gem flink: should succeed.
> - But: dma-buf mapping for a foreign device should fail. This will be a
>    pain to test on Android since we don't have anything else really. I can
>    take that and do a test like the pile of prime_nv tests we have.

Flink is there in forking_evictions.

Dmabuf is something I know nothing at the moment so I'll have to look 
into it.

> - gtt mmaps: Theoretically works, but dunno whether it makes sense.

According to Chris not on all architectures, I have to find relevant 
documentation for that.

> - Anything esle I've forgotten?

Don't know, but thanks for your comments!

Regards,

Tvrtko