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

[Dave Gordon <david.s.gordon@intel.com>]

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

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