On 23.02.2016 21:36, Dave Gordon wrote:
On 23/02/16 14:39, Tvrtko Ursulin wrote:
On 23/02/16 14:03, Chris Wilson wrote:
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).
Not sure that I understand what you meant here. Nothing as far as I can
tell is now defunct. Neither the selection mechanism, or the existence
of BSD2.
To be absolutely clear, you are, or you are not, in favour of Gabriel's
patch to start failing execbuf with fine grained BSD selection flags
when BSD2 is not present?
Regards,
Tvrtko
Currently:
#define I915_EXEC_BSD (2<<0)
/** Used for switching BSD rings on the platforms with two BSD rings */
#define I915_EXEC_BSD_SHIFT (13)
#define I915_EXEC_BSD_MASK (3 << I915_EXEC_BSD_SHIFT) /* default
ping-pong mode */
#define I915_EXEC_BSD_DEFAULT (0 << I915_EXEC_BSD_SHIFT)
#define I915_EXEC_BSD_RING1 (1 << I915_EXEC_BSD_SHIFT)
#define I915_EXEC_BSD_RING2 (2 << I915_EXEC_BSD_SHIFT)
It makes sense to have the original "BSD" flag mean "the default BSD",
and use different flags to mean specifically "BSD1" or "BSD2". Which
appears to be what we've done; naive clients that don't set any of the
new BSD bits will get default behaviour (execute on *any* BSD ring) with
no control over the ping-pong mechanism (if any). Clients that specify a
specific ring should get that one, and only that one; if it doesn't
exist then it's an error.
Any program that's going to set these bits should first ask whether (or
which) engines exist and select appropriately. So I think I'm with Chris
here.
On the other hand, I think what Tvrtko said was "it should not be an
error to select a specific ring [that exists] just because there are no
other rings". Which I also agree with.
So the ring-select-checking code should:
1. reject the I915_EXEC_BSD_RING2 case if BSD2 does not exist
This is one of the things the patch does, I guess everybody agrees on
rejecting BSD2.
2. reject the I915_EXEC_BSD_RING1 case if BSD1 does not exist
(for some future bizarre numbering scheme? or a
partitioning system that reserves BSD1 for someone else?)
3. never reject the I915_EXEC_BSD_DEFAULT case
(although it will have rejected the I915_EXEC_BSD flag
before looking at these if there is no BSD ring at all)
4. for now (until we assign it a meaning) reject the case where
BOTH BSD ring-select bits are set.
Therefore I disagree with Gabriel's patch which would reject trying to
select BSD1 on a platform that only has the one BSD engine.
Depending on the BSD1 debate resolution I see two possible solutions:
1. Change the patch to reject I915_EXEC_BSD_RING2 only and change
gem_require_ring to allow BSD1.
2. Keep the patch, eventually remove that check in gem_require_ring
(which will become redundant) in igt
Thank you all for your input on this.
Gabriel.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
