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

[Gong, Zhipeng]

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