[Mesa-dev] [PATCH 2/2] st/va: flush pipeline after post processing

[Christian König]

Am 29.11.2016 um 17:05 schrieb Nicolai Hähnle:
> On 29.11.2016 15:36, Christian König wrote:
>> Am 29.11.2016 um 15:28 schrieb Nicolai Hähnle:
>>> On 29.11.2016 15:12, Christian König wrote:
>>>> Am 29.11.2016 um 15:06 schrieb Nicolai Hähnle:
>>>>> On 29.11.2016 14:50, Christian König wrote:
>>>>>> Am 29.11.2016 um 14:46 schrieb Nicolai Hähnle:
>>>>>>> On 28.11.2016 15:51, Christian König wrote:
>>>>>>>> From: sguttula <suresh.guttula at amd.com>
>>>>>>>>
>>>>>>>> This will flush the pipeline,which will allow to share dma-buf 
>>>>>>>> based
>>>>>>>> buffers.
>>>>>>>>
>>>>>>>> Signed-off-by: Suresh Guttula <Suresh.Guttula at amd.com>
>>>>>>>> Reviewed-by: Christian König <christian.koenig at amd.com>
>>>>>>>
>>>>>>> Why is there no fence? Relying on the correctness of doing a flush
>>>>>>> without a fence seems very likely to be wrong... it might seemingly
>>>>>>> fix a sharing issue, but once the timing changes, the other side 
>>>>>>> of a
>>>>>>> buffer sharing might still see wrong results if it isn't properly
>>>>>>> synchronized.
>>>>>>
>>>>>> Well there is no facility to share a fence with another side, so the
>>>>>> kernel must make sure that the correct order is kept when the
>>>>>> DMA-buf is
>>>>>> used by multiple processes that everything executes in the right
>>>>>> order.
>>>>>
>>>>> Ah right, the kernel does most of the job. Still, because of
>>>>> multi-threaded submit, returning from pipe->flush doesn't actually
>>>>> guarantee that the work has even been submitted by the kernel.
>>>>>
>>>>> So unless the only guarantee you want here is that progress happens
>>>>> eventually, you'll still need a fence.
>>>>>
>>>>> I don't think we have an interface that guarantees that work has
>>>>> reached the kernel without also waiting for job completion.
>>>>
>>>> I'm pretty sure that this isn't correct, otherwise VDPAU interop won't
>>>> work either.
>>>
>>> Maybe we're just getting lucky?
>>>
>>>
>>>> When pipe->flush() is called it must be guaranteed that all work is
>>>> submitted to the kernel.
>>>
>>> It guarantees that the work is (eventually, in practice very soon)
>>> submitted to the kernel, but it does _not_ guarantee that the kernel
>>> has already returned from the CS ioctl.
>>>
>>> The whole point of multi-threaded dispatch is to avoid the wait that
>>> would be required to guarantee that.
>>
>> We neither need nor want multi-threaded dispatch for the whole MM parts.
>>
>> When I've implemented the whole multi ring dispatch logic this was
>> clearly the case and async flushes where only initialized by the winsys
>> if the RADEON_FLUSH_ASYNC flag was given.
>>
>> And as far as I understand it on the pipe layer the driver can only use
>> a async flush if the PIPE_FLUSH_DEFERRED flag is given.
>>
>> If that isn't the case here then that's clearly a bug which needs to be
>> fixed.
>
> Well, what you call to ensure things go to the kernel is 
> ws->cs_sync_flush.
>
> This function is never called from any of the video code, but that's 
> probably okay.
>
> It is called from r600_flush_from_st for non-deferred, but only when 
> nothing has been submitted to the gfx ring. When something is on the 
> gfx ring, we go to si_context_gfx_flush which doesn't call it.
>
> As for PIPE_FLUSH_DEFERRED, the idea is that this may not actually 
> trigger a flush at all. All it gives you is a fence which will 
> complete based on the next real flush.
>
> Without flags, pipe->flush is supposed to have the semantics of 
> glFinish: it simply guarantees that all previous commands will 
> eventually be submitted to the GPU, but nothing else.
>
> Because of that, I actually think the fact that r600_flush_from_st 
> calls cs_sync_flush at all right now is a bug. But clearly it seems 
> necessary to call cs_sync_flush *sometimes*.
>
> So yes, it looks like there are a bunch of bugs here, some of them 
> perhaps occasionally canceling each other out. And I kind of wonder 
> whether they might not actually affect all OpenGL apps... hooray :)

Well all of this isn't really a problem for the state tracker, but 
rather for the driver and winsys.

So I've pushed the patch for now to fix the problem at hand.

Regards,
Christian.

>
> Cheers,
> Nicolai