[Intel-gfx] [PATCH 1/7] drm/i915: Specify bsd rings through exec flag

[Daniel Vetter]

On Wed, Dec 10, 2014 at 02:18:15AM +0000, Gong, Zhipeng wrote:
> 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 at ffwll.ch> wrote:
> > > > On Mon, Nov 24, 2014 at 08:29:40AM -0800, Rodrigo Vivi wrote:
> > > >> From: Zhipeng Gong <zhipeng.gong at intel.com>
> > > >>
> > > >> 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 at intel.com>
> > > >> Reviewed-by-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > >
> > > > 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.

Well then same arguments applies with ring2 since only ring1 is special?
It's just to minimize abi and reduce the amount of rope we hand to
userspace.

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

Well, I need a real user for this and it needs to be opensource. Given
that the real user on bdw currently is closed source this is a bit tricky
and one of these things to send Dave Airlie into ballistic modes. So I
don't want to try even.

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

Yeah, but userspace could submit e.g. EXEC_BLITTER | EXEC_BSD_RING1 which
doesn't make sense and must be rejected.

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

Two things essentially:
- all flag combinations that don't make sense _must_ be rejected with
  -EINVAL.
- you must have an igt testcase for each such case

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