Re: [PATCH 1/7] drm/i915: Specify bsd rings through exec flag

[Daniel Vetter <daniel@ffwll.ch>]

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

> +                             break;
> +                     case I915_EXEC_BSD_RING2:

This needs a if (!IS_SKL(dev) return -EINVAL; check since the flag isnt
valid anywhere else. Also you must add code to reject these flags if
userspace selects 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).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx