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

[Zhao Yakui]

On Thu, 2014-04-10 at 00:58 -0600, Daniel Vetter wrote:
> 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.

OK. I will add the separated two test cases for it.

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

OK. I will take a look at your blog and understand what you mentioned.

BTW: Does it need to check all the flags defined in i915_drm.h or the
exported flag returned by i915_get_parameter?

Thanks.
    Yakui

> 
> Cheers, Daniel