[Intel-gfx] [PATCH v2] drm/i915: Avoid selecting unavailable BSD2 ring

[Chris Wilson]

On Tue, Feb 23, 2016 at 01:31:17PM +0000, Tvrtko Ursulin wrote:
> 
> On 23/02/16 13:06, Gabriel Feceoru wrote:
> > 
> > 
> > On 23.02.2016 13:05, Tvrtko Ursulin wrote:
> >>
> >> Hi,
> >>
> >> On 23/02/16 10:52, Gabriel Feceoru wrote:
> >>> Return error when I915_EXEC_BSD_RING2 flag is set but BSD2 ring
> >>> is not available in the HW.
> >>
> >> What is the reasoning behind this? So far kernel was allowing userspace
> >> to select these bits and execute on the first engine. With this patch it
> >> would start failing potentially breaking userspace. Is it not too late
> >> to make such change?
> > 
> > I noticed some inconsistencies in igt with regards to bsd and bsd1.
> > For instance, if bsd2 is not available, gem_sync at basic-bsd1 is skipped, 
> > but it's skipped because of the 2nd check gem_has_bsd2 (see 
> > gem_require_ring). Surprisingly gem_has_ring() didn't complain about bsd1.
> > 
> > This fix will make gem_has_ring() return false.
> > 
> > I'm not aware about legacy/compatibility issue - if that's the case, 
> > please disregard this.
> 
> Hmmm.. Chris, what is the reasoning behind:
> 
> commit eaa03678b00179da89f194113c0740c033857c1c
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Thu Jan 28 13:44:19 2016 +0000
> 
>     lib: Hide BSD1/BSD2 rings on hardware without BSD2
>     
>     The kernel happily lets us run on I915_EXEC_BSD2 even with such hardware
>     existing. Sigh.
>     
>     Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9dfa9b2603ce..fa44080e5902 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1341,6 +1341,12 @@ static int gem_has_ring(int fd, int ring)
>  void gem_require_ring(int fd, int ring_id)
>  {
>         igt_require(gem_has_ring(fd, ring_id));
> +
> +       /* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */
> +       if ((ring_id & ~(3<<13)) == I915_EXEC_BSD) {
> +               if (ring_id & (3 << 13))
> +                       igt_require(gem_has_bsd2(fd));
> +       }
>  }
>  
>  /* prime */
> 
> ABI was (and still is) that specifying BSD1 or BSD2 explicitly is
> silently ignored by the kernel when BSD2 is not preset, defaulting
> to BSD1.

Thereby pretending that BSD, BSD1, BSD2 exist.

> This patch makes tests requesting BSD1 skip when there is no BSD2
> which I think is wrong in any case.

BSD 1/2 selection only makes sense when we have multiple BSD rings.
Running tests on BSD default, BSD1 and BSD2 is pointless if they all
are equivalent. Using the BSD ping-pong when we have BSD1 and BSD2 is
questionable as the ping-pong nature is uncontrolled, but nevertheless
the code path needs to be tested.

> If we want to (and can) change the ABI it should only reject the
> non-existent ring and not limit the selection mechanism to
> hardware which has BSD2.

I disagree, we have a ring selection mechanism. If the extension doesn't
exist, trying to use it should be an error. The extension was not only
an ABI mistake but undesired (it is now defunct).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre