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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
