[v2] lib: Skip aliased bsd ABI ring if bsd2 is available

Submitted by Chris Wilson on Feb. 21, 2018, 12:17 p.m.

Details

Message ID 20180221121719.4314-1-chris@chris-wilson.co.uk
State New
Series "lib: Skip aliased bsd ABI ring if bsd2 is available"
Headers show

Commit Message

Chris Wilson Feb. 21, 2018, 12:17 p.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.

v2: When in doubt, shout!

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..b9b86079 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

Tvrtko Ursulin Feb. 21, 2018, 12:53 p.m.
On 21/02/2018 12:17, 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.
> 
> v2: When in doubt, shout!
> 
> 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..b9b86079 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;
>   	}

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

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

Maybe I'm confused.. please simplify the statement? :)

Regards,

Tvrtko
Tvrtko Ursulin Feb. 21, 2018, 12:55 p.m.
On 21/02/2018 12:53, Tvrtko Ursulin wrote:
> 
> On 21/02/2018 12:17, 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.
>>
>> v2: When in doubt, shout!
>>
>> 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..b9b86079 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;
>>       }
> 
> If default BSD (1)
>      and no BSD2 -> 1 ^ 1 = 1 OK

Oops
1 ^ 1 = 0 so also bad

>      and BSD2 -> 1 ^ 0 = 1 BAD
> 
> If explicit BSD (0)
>      and no BSD2 -> 0 ^ 1 = 1 BAD
>      has BSD2 -> 0 ^ 0 = 0 = BAD
> 
> Maybe I'm confused.. please simplify the statement? :)
> 
> Regards,
> 
> Tvrtko
Chris Wilson Feb. 21, 2018, 1:07 p.m.
Quoting Tvrtko Ursulin (2018-02-21 12:55:17)
> 
> On 21/02/2018 12:53, Tvrtko Ursulin wrote:
> > 
> > On 21/02/2018 12:17, 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.
> >>
> >> v2: When in doubt, shout!
> >>
> >> 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..b9b86079 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;
> >>       }
> > 
> > If default BSD (1)
> >      and no BSD2 -> 1 ^ 1 = 1 OK
> 
> Oops
> 1 ^ 1 = 0 so also bad
> 
> >      and BSD2 -> 1 ^ 0 = 1 BAD
> > 
> > If explicit BSD (0)
> >      and no BSD2 -> 0 ^ 1 = 1 BAD
> >      has BSD2 -> 0 ^ 0 = 0 = BAD
> > 
> > Maybe I'm confused.. please simplify the statement? :)

return false?

default BSD ^ has-bsd2
0		0	-> 0 OK
0		1	-> 1 SKIP
1		0	-> 1 SKIP
1		1	-> 0 OK

So we only use bsd (implicit) on single BSD engine machines, and only
use bsd0,bsd1 (explicit) on dual engine machines.
-Chris
Tvrtko Ursulin Feb. 21, 2018, 1:35 p.m.
On 21/02/2018 13:07, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-21 12:55:17)
>>
>> On 21/02/2018 12:53, Tvrtko Ursulin wrote:
>>>
>>> On 21/02/2018 12:17, 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.
>>>>
>>>> v2: When in doubt, shout!
>>>>
>>>> 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..b9b86079 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;
>>>>        }
>>>
>>> If default BSD (1)
>>>       and no BSD2 -> 1 ^ 1 = 1 OK
>>
>> Oops
>> 1 ^ 1 = 0 so also bad
>>
>>>       and BSD2 -> 1 ^ 0 = 1 BAD
>>>
>>> If explicit BSD (0)
>>>       and no BSD2 -> 0 ^ 1 = 1 BAD
>>>       has BSD2 -> 0 ^ 0 = 0 = BAD
>>>
>>> Maybe I'm confused.. please simplify the statement? :)
> 
> return false?
> 
> default BSD ^ has-bsd2
> 0		0	-> 0 OK
> 0		1	-> 1 SKIP
> 1		0	-> 1 SKIP
> 1		1	-> 0 OK
> 
> So we only use bsd (implicit) on single BSD engine machines, and only
> use bsd0,bsd1 (explicit) on dual engine machines.

Don't get in your notation why is 0 OK and 1 SKIP. 0 is false = no ring 
= skip.

default BSD ^ 	has-bsd2
0		0		0 ^ !0 = 1 has engine - WRONG
0		1		0 ^ !1 = 0 no engine - WRONG
1		0		1 ^ !0 = 0 no engine - WRONG
1		1		1 ^ !1 = 1 has engine - WRONG


default BSD ^ 	has-bsd2
0		0		0 ^ 0 = 0 no engine - OK
0		1		0 ^ 1 = 1 has engine - OK
1		0		1 ^ 0 = 1 has engine - OK
1		1		1 ^ 1 = 0 no engine - OK

So previous version was OK!?

Regards,

Tvrtko
Chris Wilson Feb. 21, 2018, 1:42 p.m.
Quoting Tvrtko Ursulin (2018-02-21 13:35:38)
> 
> On 21/02/2018 13:07, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-02-21 12:55:17)
> >>
> >> On 21/02/2018 12:53, Tvrtko Ursulin wrote:
> >>>
> >>> On 21/02/2018 12:17, 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.
> >>>>
> >>>> v2: When in doubt, shout!
> >>>>
> >>>> 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..b9b86079 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;
> >>>>        }
> >>>
> >>> If default BSD (1)
> >>>       and no BSD2 -> 1 ^ 1 = 1 OK
> >>
> >> Oops
> >> 1 ^ 1 = 0 so also bad
> >>
> >>>       and BSD2 -> 1 ^ 0 = 1 BAD
> >>>
> >>> If explicit BSD (0)
> >>>       and no BSD2 -> 0 ^ 1 = 1 BAD
> >>>       has BSD2 -> 0 ^ 0 = 0 = BAD
> >>>
> >>> Maybe I'm confused.. please simplify the statement? :)
> > 
> > return false?
> > 
> > default BSD ^ has-bsd2
> > 0             0       -> 0 OK
> > 0             1       -> 1 SKIP
> > 1             0       -> 1 SKIP
> > 1             1       -> 0 OK
> > 
> > So we only use bsd (implicit) on single BSD engine machines, and only
> > use bsd0,bsd1 (explicit) on dual engine machines.
> 
> Don't get in your notation why is 0 OK and 1 SKIP. 0 is false = no ring 
> = skip.
> 
> default BSD ^   has-bsd2
> 0               0               0 ^ !0 = 1 has engine - WRONG
> 0               1               0 ^ !1 = 0 no engine - WRONG
> 1               0               1 ^ !0 = 0 no engine - WRONG
> 1               1               1 ^ !1 = 1 has engine - WRONG
> 
> 
> default BSD ^   has-bsd2
> 0               0               0 ^ 0 = 0 no engine - OK
> 0               1               0 ^ 1 = 1 has engine - OK
> 1               0               1 ^ 0 = 1 has engine - OK
> 1               1               1 ^ 1 = 0 no engine - OK
> 
> So previous version was OK!?

Now I am confused, v2 is the one that appears to work.

Anyway, I remember why it is like this. The intention is to iterate the
ABI not the physical engines, I keep meaning to do
s/for_each_engine/for_each_uabi_ring/ and then for_each_physical_engine
that skips the aliased default and bsd (and whatever the future may
bring).

Bring on class/instance.
-Chris
Tvrtko Ursulin Feb. 21, 2018, 1:42 p.m.
On 21/02/2018 13:35, Tvrtko Ursulin wrote:
> 
> On 21/02/2018 13:07, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2018-02-21 12:55:17)
>>>
>>> On 21/02/2018 12:53, Tvrtko Ursulin wrote:
>>>>
>>>> On 21/02/2018 12:17, 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.
>>>>>
>>>>> v2: When in doubt, shout!
>>>>>
>>>>> 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..b9b86079 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;
>>>>>        }

For preserving time and sanity:

bool is_bsd = (ring & 0xf) == I915_EXEC_BSD;
bool implicit_bsd = !(ring & (3 << 13));
bool supports_explicit_bsd = gem_has_bsd2(fd);

if (is_bsd) {
	if (implicit_bsd && supports_explicit_bsd)
		return false;
	else if (!implicit_bsd && !supports_explicit_bsd)
		return false;
}

?

Regards,

Tvrtko
Chris Wilson Feb. 21, 2018, 1:45 p.m.
Quoting Tvrtko Ursulin (2018-02-21 13:42:29)
> 
> On 21/02/2018 13:35, Tvrtko Ursulin wrote:
> > 
> > On 21/02/2018 13:07, Chris Wilson wrote:
> >> Quoting Tvrtko Ursulin (2018-02-21 12:55:17)
> >>>
> >>> On 21/02/2018 12:53, Tvrtko Ursulin wrote:
> >>>>
> >>>> On 21/02/2018 12:17, 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.
> >>>>>
> >>>>> v2: When in doubt, shout!
> >>>>>
> >>>>> 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..b9b86079 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;
> >>>>>        }
> 
> For preserving time and sanity:
> 
> bool is_bsd = (ring & 0xf) == I915_EXEC_BSD;

& 63

Just to remind you how broken it all is.

> bool implicit_bsd = !(ring & (3 << 13));
> bool supports_explicit_bsd = gem_has_bsd2(fd);
> 
> if (is_bsd) {
>         if (implicit_bsd && supports_explicit_bsd)
>                 return false;
>         else if (!implicit_bsd && !supports_explicit_bsd)
>                 return false;
> }
> 

class/instance to the rescue. :)
-Chris