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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Volkin, Bradley D" <bradley.d.volkin@intel.com>
Cc: "Intel-gfx@lists.freedesktop.org" <Intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 1/3] tests/gem_userptr_blits: Expanded userptr test cases
Date: Wed, 23 Apr 2014 19:53:38 +0200	[thread overview]
Message-ID: <20140423175338.GD10722@phenom.ffwll.local> (raw)
In-Reply-To: <20140423153227.GB8947@bdvolkin-ubuntu-desktop>

On Wed, Apr 23, 2014 at 08:32:27AM -0700, Volkin, Bradley D wrote:
> On Wed, Apr 23, 2014 at 06:33:40AM -0700, Tvrtko Ursulin wrote:
> > 
> > On 04/18/2014 06:10 PM, Volkin, Bradley D wrote:
> > > On Wed, Mar 19, 2014 at 04:13:04AM -0700, Tvrtko Ursulin wrote:
> > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > >>
> > >> A set of userptr test cases to support the new feature.
> > >>
> > >> For the eviction and swapping stress testing I have extracted
> > >> some common behaviour from gem_evict_everything and made both
> > >> test cases use it to avoid duplicating the code.
> > >>
> > >> Both unsynchronized and synchronized userptr objects are
> > >> tested but the latter set of tests will be skipped if kernel
> > >> is compiled without MMU_NOTIFIERS.
> > >>
> > >> Also, with 32-bit userspace swapping tests are skipped if
> > >> the system has a lot more RAM than process address space.
> > >> Forking swapping tests are not skipped since they can still
> > >> trigger swapping by cumulative effect.
> > >>
> > >> v2:
> > >>     * Fixed dmabuf test.
> > >>     * Added test for rejecting read-only.
> > >>     * Fixed ioctl detection for latest kernel patch.
> > >>
> > >> v3:
> > >>     * Updated copy() for Gen8+.
> > >>     * Fixed ioctl detection on kernels without MMU_NOTIFIERs.
> > >>
> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > >
> > > A number of the comments I made on patch 3 apply here as well.
> > > The sizeof(linear) thing is more prevalent in this test, though
> > > it looks like linear is at least used. Other than those comments
> > > this looks good to me.
> > 
> > Believe it or not that sizeof(linear) "idiom" I inherited from other 
> > blitter tests. Personally I don't care one way or another. But since it 
> > makes sense to get rid of it for the benchmark part, perhaps I should 
> > change it here as well to be consistent. How strongly do you feel 
> > strongly about this?
> 
> I think changing it would be slightly more readable, but if it's
> consistent with other blit tests then I don't feel too strongly
> about it. In fact, consistency with the other tests might be the
> better approach. I'm fine with whichever approach you prefer.

Some of the igt tests are so Gross Hacks that justifying ugliness in new
tests with consistency is ill-advised ;-)

If you find some spare cycles to clean up existing tests that would be
awesome, but I don't mind if they keep being ugly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch