On Thu, Jan 30, 2014 at 10:20 AM, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> >>> >>> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
