On Tue, 2014-12-09 at 10:46 +0100, Daniel Vetter wrote: > On Mon, Dec 08, 2014 at 01:55:56PM -0800, Rodrigo Vivi wrote: > > On Tue, Nov 25, 2014 at 5:04 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > > > On Mon, Nov 24, 2014 at 08:29:40AM -0800, Rodrigo Vivi wrote: > > >> From: Zhipeng Gong <zhipeng.gong@xxxxxxxxx> > > >> > > >> On Broadwell GT3 we have 2 Video Command Streamers (VCS), but userspace > > >> has no control when using VCS1 or VCS2. This patch introduces a mechanism > > >> to avoid the default ping-pong mode and use one specific ring through > > >> execution flag. > > >> > > >> v2: fix whitespace (Rodrigo) > > >> v3: remove incorrect chunk that came on -collector rebase. (Rodrigo) > > >> > > >> Signed-off-by: Zhipeng Gong <zhipeng.gong@xxxxxxxxx> > > >> Reviewed-by-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > > > > > There's review from me pending on testcases and stuff, but I get the > > > impression that's lost now. Is it? > > > > tests are ready as well, I've tested and reviewed them. > > imho this is ready for merge. Anyway I'm going to submit again on next > > -collector round > > Last time I've looked it doesn't really address my review. Quoting > relevant parts: > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index e1ed85a..d9081ec 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -1273,8 +1273,23 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > > else if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) { > > if (HAS_BSD2(dev)) { > > int ring_id; > > - ring_id = gen8_dispatch_bsd_ring(dev, file); > > - ring = &dev_priv->ring[ring_id]; > > + > > + switch (args->flags & I915_EXEC_BSD_MASK) { > > + case I915_EXEC_BSD_DEFAULT: > > + ring_id = gen8_dispatch_bsd_ring(dev, file); > > + ring = &dev_priv->ring[ring_id]; > > + break; > > + case I915_EXEC_BSD_RING1: > > + ring = &dev_priv->ring[VCS]; > > Do we have any use-case for selecting ring1 specifically? I've thought > it's only ring2 that is special? The HEVC GPU commands should be dispatched to BSD RING 1 instead of BSD RING2 as the two rings are asymmetrical. For the H264 decoding/encoding either ring is OK. > > > + break; > > + case I915_EXEC_BSD_RING2: > > This needs a if (!IS_SKL(dev) return -EINVAL; check since the flag isnt > valid anywhere else. The override flag is required for the asymmetrical BSD rings in SKL. But it will be reasonable that user-space driver should have an opportunity to determine which ring is to dispatch the BSD GPU command even though the two rings are symmetrical, like BDW. I will send out a new version of the patch to add more comments. > Also you must add code to reject these flags if > userspace selects a ring other than bsd. "if ((args->flags & I915_EXEC_RING_MASK) == I915_EXEC_BSD) " condition checks already make sure that userspace would not select a ring other than bsd. > > And all these new -EINVAL cases need new subtests to validate them in > gem_exec_params.c. > > And I might have missed some case, ioctl validation is hard ;-) So please > double-check that really no insane combination that userspace might try to > abuse is caught and has a testcase in gem_exec_params. > > /endquote > > The testcase also doesn't check for these nasty condiditions (it would > fail with the current patch). Could you please give examples for "nasty conditions", I will add them. > -Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx