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@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@xxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
