[Intel-gfx] [PATCH] tests/gem_userptr_blits: Expanded userptr test cases

[Daniel Vetter]

On Thu, Jan 30, 2014 at 10:20 AM, Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
> 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 at 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.

You have too high standards ;-) I'm quite a bit in favour of massive
c&p in testcases since change tests always has the risk to break them,
and hence lose a bit of coverage. But since this is for stress-tests
the risk for subtle breakage is fairly small I think.

>> 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.

Oh, missed that. In that case we're good already.

>> - 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?

NULL should be good enough. 2nd version could use a gtt mmap'ed area
obtained from i915.ko - those aren't struct page backed either and in
addition also provoke lock inversion issues. I think with those two
cases we should be well covered.

>> 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.

dma-buf has two use-case:
- Improved buffer sharing (better security) than flink, used by
wayland and dri3.
- Sharing backing storage across devices (similar to what ION does on
Android - actually ION is nowadays based on dma-buf internally).

We might want allow the 1. usecase with userptr I think, but we also
need to reject the 2. one. But since this is a bit tricky it might be
easier to outright reject all dma-buf exporting of userptrs for now.
See the prime_self_import testcase, the actual exporting (i.e. where
we'd need to insert a return -EINVAL) in the kernel happens in
i915_gem_prime_export

>> - 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.

I think we can just reject gtt mmaps for now then. Tbh I don't think
it makes too much sense as a use-case really anyway.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch