[Intel-gfx] [PATCH 05/51] drm/i915: Add return code check to i915_gem_execbuffer_retire_commands()

[Dave Gordon]

On 06/03/15 15:57, Daniel Vetter wrote:
> On Fri, Mar 06, 2015 at 11:38:44AM +0000, John Harrison wrote:
>> On 05/03/2015 16:14, Daniel Vetter wrote:
>>> On Thu, Mar 05, 2015 at 03:06:42PM +0000, John Harrison wrote:
>>>> On 05/03/2015 14:44, Daniel Vetter wrote:
>>>>> Imo reserving a bit of ring space for each add_request should be solid.
>>>>> Userspace uses the exact same reservation logic for adding end-of-batch
>>>>> workarounds. The only thing needed to make this solid is to WARN if
>>>>> add_request ends up using more ring space than what we've reserved (not
>>>>> just when it actually runs out, that obviously doesn't happen often
>>>>> enough for testing).
>>>> The problem is that there could be multiple requests being processed in
>>>> parallel. This is especially true with the scheduler. Userland could submit
>>>> a whole stream of batches that all get queued up in the scheduler. Only
>>>> later do they get submitted to the hardware. The request must be allocated
>>>> up front because there is no other means of tracking them. But reserving
>>>> space at that point won't work because you either end up reserving massive
>>>> amounts of space if the reserve is cumulative, or not enough if only one
>>>> slot is reserved.
>>> At least with execlist we don't have that problem really since writing the
>>> ringbuffer is done synchronously and directly.
>>>
>>> For the legacy scheduler I expect that we won't do any of the ringbuf
>>> writes directly and instead that's all done by the scheduler
>>> asynchronously.
>>>
>>> So this should just be an issue while we are converting to the scheduler
>>> or on platforms that will never have one. And imo the request ringbuf
>>> reservation is the solution with the simplest impact on the design.
>> I don't understand what you mean here. The scheduler decouples the
>> submission of a batch buffer to the driver with the submission of that batch
>> buffer to the hardware. The submission code path itself is identical. The
>> scheduler does not do any hardware writes - it merely adds an extra layer
>> between the user interface code and the hardware access code. The request
>> creation must be done at driver submission time. The hardware submission
>> could be an arbitrary amount of time later. In the intervening time, any
>> number of new batch buffers may be submitted to the driver and thus new
>> requests be created. This is true for both legacy and execlist mode.
>>
>> The space reserve would have to be done at the start of the hardware
>> submission process not at driver submission time. I.e. it would need to be a
>> separate and new i915_gem_request_reserve_space() call at the start of the
>> back half of the exec buffer code. It could not be done during request
>> creation.
> 
> Yeah with the scheduler we need to take out the reservation again since it
> doesn't make a lot of sense - the scheduler is the safety net that
> guarantees execution (barring gpu death ofc as always) when the execbuf
> ioctl returns 0. Maybe in the end we need a special alloc_request function
> for execbuf to only reserve when the scheduler isn't around.
> 
> That additional work is just part of the prize we have to pay to never
> break things while refactoring code. And since we'll get the execlist
> scheduler in first that reservation code will probably stick around for a
> while (or maybe forever even if we decide against upstreaming the gen7
> scheduler).
> 
> Does this make sense?
> 
> Thanks, Daniel

I already put a pre-reservation in the preemptive version of the
scheduler. It won't start to write anything in the ringbuffer unless
it's guaranteed there's enough space to write the whole set of commands
to run a batch, from preamble to finalisation. That means that none of
the ring_begin() commands can fail, and we won't leave a partial set of
commands in the ring to confuse any subsequent (successful) submissions.

And yes, I added warnings if the commands emitted to the ring overran
the amount of reserved space, so that developers would know if they'd
increased the total ringspace requirement.

If there are any other failure modes while writing to the ring, we
should simply back up to the start of the sequence (i.e. rewind the
software copy of TAIL to where it was after we checked for space -- easy
because we haven't updated h/w TAIL yet), and we can leave the request
queued for subsequent retry (or fail it as though it had been submitted
and then killed by TDR, if the failure mode suggests it's not retryable).

[aside] One version of this was posted as
http://lists.freedesktop.org/archives/intel-gfx/2014-December/057247.html but
that re-used intel_ring_begin() as a reserve-space call, whereas another
version has a separate nonblocking function intel_ring_test_space() for
prechecking available space [/aside]

.Dave.