[Intel-gfx] [PATCH v2] drm/i915: Avoid selecting unavailable BSD2 ring

[Gabriel Feceoru]

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