Re: [PATCH v2] drm/i915: Avoid selecting unavailable BSD2 ring

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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

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

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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx