lib: Skip aliased bsd ABI ring if bsd2 is available

Submitted by Chris Wilson on Feb. 21, 2018, 11:21 a.m.

Details

Message ID 20180221112151.7244-1-chris@chris-wilson.co.uk
State New
Headers show
Series "lib: Skip aliased bsd ABI ring if bsd2 is available" ( rev: 1 ) in IGT (deprecated)

Not browsing as part of any series.

Commit Message

Chris Wilson Feb. 21, 2018, 11:21 a.m.
How much do I want this uABI to rot away? Say "Never again!" to implicit
aliasing.

In the meantime, we do not need to perform duplicate work on bsd2
machines, as especially we do not know which engine bsd relates to.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/ioctl_wrappers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 8748cfcf..868d68f7 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1460,7 +1460,7 @@  bool gem_has_ring(int fd, unsigned ring)
 
 	/* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */
 	if ((ring & ~(3<<13)) == I915_EXEC_BSD) {
-		if (ring & (3 << 13) && !gem_has_bsd2(fd))
+		if (!(ring & (3 << 13)) ^ gem_has_bsd2(fd))
 			return false;
 	}
 

Comments

On 21/02/2018 11:21, Chris Wilson wrote:
> How much do I want this uABI to rot away? Say "Never again!" to implicit
> aliasing.
> 
> In the meantime, we do not need to perform duplicate work on bsd2
> machines, as especially we do not know which engine bsd relates to.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   lib/ioctl_wrappers.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 8748cfcf..868d68f7 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1460,7 +1460,7 @@ bool gem_has_ring(int fd, unsigned ring)
>   
>   	/* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */
>   	if ((ring & ~(3<<13)) == I915_EXEC_BSD) {

What is this testing for? Why not just ring & i915_EXEC_RING_MASK == 
I915_EXEC_BSD? It there are some other bits set it will behave strangely.

> -		if (ring & (3 << 13) && !gem_has_bsd2(fd))

Disallow explicit selection if no vcs2, makes sense.

> +		if (!(ring & (3 << 13)) ^ gem_has_bsd2(fd))

Ugh..

If default BSD (1)
	and no BSD2 -> 1 ^ 0 = 1 OK
	and BSD2 -> 1 ^ 1 = 1 BAD

If explicit BSD (0)
	and no BSD2 -> 0 ^ 0 = 0 BAD
	has BSD2 -> 0 ^ 1 = 1 = OK

Or I made a mistake, its possible..

Regards,

Tvrtko

>   			return false;
>   	}
>   
>
Quoting Tvrtko Ursulin (2018-02-21 12:25:55)
> 
> On 21/02/2018 11:21, Chris Wilson wrote:
> > How much do I want this uABI to rot away? Say "Never again!" to implicit
> > aliasing.
> > 
> > In the meantime, we do not need to perform duplicate work on bsd2
> > machines, as especially we do not know which engine bsd relates to.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   lib/ioctl_wrappers.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 8748cfcf..868d68f7 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -1460,7 +1460,7 @@ bool gem_has_ring(int fd, unsigned ring)
> >   
> >       /* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */
> >       if ((ring & ~(3<<13)) == I915_EXEC_BSD) {
> 
> What is this testing for? Why not just ring & i915_EXEC_RING_MASK == 
> I915_EXEC_BSD? It there are some other bits set it will behave strangely.

What other bits? :) (Note EXEC_RING_MASK has the wrong value in the
headers.)
-Chris