Re: [PATCH 0/5] drm/i915: Add the support of dual BSD rings on BDW GT3

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 10, 2014 at 11:28:46AM +0800, Zhao Yakui wrote:
> On Wed, 2014-04-09 at 08:45 -0600, Daniel Vetter wrote:
> > On Wed, Apr 09, 2014 at 09:59:51AM +0800, Zhao Yakui wrote:
> > > 
> > > This is the patch set that tries to add the support of dual BSD rings on BDW
> > > GT3. Based on hardware spec, the BDW GT3 has two independent BSD rings, which
> > > can be used to process the video commands. To be simpler, it is transparent 
> > > to user-space driver/middleware. In such case the kernel driver will decide
> > > which ring is to dispatch the BSD video command.
> > > 
> > > As every BSD ring is powerful, it is enough to dispatch the BSD video command
> > > based on the drm fd. In such case the different BSD ring is used for video playing
> > > back and encoding. At the same time the coarse dispatch mechanism can help to avoid
> > > the object synchronization between the BSD rings.
> > 
> > Ok, I've quickly read through it all and commented on a few things. Imo
> > the last patch should be massively simplified, at least for the first
> > round. Other things look small.
> > 
> Hi, Daniel
> 
> Thanks for your review.
> 
> > What's still missing are testcases, and I have two things in mind here:
> > - Exercise the 2nd ring dispatch and sync a bit. Since the 2nd bsd ring is
> >   hidden within the kernel I think the right approach would be to open a
> >   few drm fds (10 or so) and then randomly use them with a dummy reloc. We
> >   have two testcases which can be used as blueprints that need
> >   adjustement:
> > 
> >   - gem_ring_sync_loop: Probably easiest to copy it to a new file as
> >     gem_multi_bsd_sync_loop. This test exercises semaphores.
> >   - gem_dummy_reloc_loop, subtest mixed: Almost the same as the above, but
> >     the sync is done _inside_ the loop and hence this exercises gpu/cpu
> >     sync. We need both tests adjusted, for for this we need a new
> >     multi-bsd test.
> 
> Agree with your concerns. I will try to add the
> gem_multi_bsd_sync_loop/dummy_reloc_loop test case so that it can test
> the sync with multi-BSD.
> 
> BTW: How about if I directly add multiple fds in gem_ring_sync_loop test
> case and then test the sync among the different rings?  In such case the
> user-application doesn't need to know the existence of multi-BSD rings.

We don't need it for the other rings, so I think it's better to leave the
existing tests as-is to avoid introducing bugs. Testing testcase is always
fairly hard, since you have to break your kernel to make sure the test
still catches bugs ;-)

Also for testing VCS1 and VCS2 we need to have multiple fd using the
_same_ logical ring exposed to userspace, so the test logic will look a
bit different anyway.

> > - New testcase to fully test main execbuffer flags. This is simply
> >   something that's we don't yet have. The next guy to touch execbuf code
> >   needs to add it, and it looks like that's you ;-) I've done a JIRA task
> >   for the resource streamer work, but I think the resource streamer wont
> >   be merged anytime soon. So I'll reassign to you. Jira task is VIZ-3129.
> 
> For the new testcase of execbuffer flag:  Do you have any idea about
> which kind of exec flag needs to be checked? Do you have any idea about
> the expected failure/successful behavour for the flags?
>    For example: I915_EXEC_PINNED : If one object is not pinned and
> submitted, what behavour is expected? Fail or wrong?

I've clarified the JIRA, the test is just for the flags/values in the main
execbuf structure. And the idea is to do the basic api sanity checking as
outlined in my blog post

http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

i.e. go through all fields in struct drm_i915_gem_execbuffer2 and write a
test which checks that the kernel correctly rejects invalid input data. So
e.g. for pointer you can supply NULL or a pointer to invalid memory,
buffer count is already checked with the overflow tests, but also invalid
flags and also making sure that if reserved fields aren't 0 the kernel
rejects the batch.

Of course to be able to check this you first need to construct a valid
no-op batch (e.g. copy from gem_exec_nop.c) and submit it (to make sure no
one breaks the test later on). Then each subtest only changes the relevant
field to make sure the kernel really did check the field (and not just
returned -EINVAL due to something else).

Some execbuf fields are special and e.g. contexts are not valid when
there's no hw context support.

If you want to look for examples check out the basic api tests for
recently added ioctls like in gem_reset_stats.c. Execbuffer ioctl is
simply a bit more complex.

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