Re: [RFC,libdrm 1/3] tegra: Add stream library

[Mark Zhang <nvmarkzhang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 12/28/2012 04:50 PM, Arto Merilainen wrote:
> On 12/28/2012 09:57 AM, Mark Zhang wrote:
>> On 12/28/2012 03:45 PM, Arto Merilainen wrote:
>>> On 12/28/2012 08:47 AM, Mark Zhang wrote:
>>>>> +
>>>>> +    /* Add fences */
>>>>> +    if (num_fences) {
>>>>> +
>>>>> +        tegra_stream_push(stream,
>>>>> +            nvhost_opcode_setclass(NV_HOST1X_CLASS_ID,
>>>>> +            host1x_uclass_wait_syncpt_r(), num_fences));
>>>>> +
>>>>> +        for (; num_fences; num_fences--, fence++) {
>>>>> +            assert(tegra_fence_is_valid(fence));
>>>>
>>>> This is useless. We already add "1 + num_fences" to num_words above. So
>>>> move this "assert" before adding "1 + num_fences" to num_words makes
>>>> sense.
>>>>
>>>
>>> The assertion checks the validity of a single fence - not if there is
>>> room in the command buffer.
>>>
>>> The goal is to prevent having invalid fences in the command stream. If
>>> this check were not here it would be possible to initialise a fence with
>>> tegra_fence_clear() and put that fence into the stream.
>>
>> My idea is, if one fence is invalid, then we should not count this in
>> "num_words". In current code, if one fence is invalid, then this fence
>> will not be pushed into the command stream, and the "num_words" shows a
>> wrong command buffer size.
>>
>> So I think we should:
>> - validate the fences, remove the invalid fence
>> - update num_words
>> - then you don't need to check fence here - I mean, before push a host1x
>> syncpt wait command into the active buffer of stream.
>>
> 
> In my opinion asking tegra_stream_begin() to put a bad fence into the
> stream is a case we should never be. assert() kills the application
> immediately (in debug builds) and usually this helps the programmer for
> 1) finding bugs 2) not doing bad code.
> 

Yep, I agree. But in release builds, assert does nothing. So this
checking doesn't make sense and also a wrong fence will be pushed into
command buffer silently. And we always use release version in real
products, so we can't count on this "assert".

> "Silencing" is not a good solution especially in this case:
> tegra_stream_flush() returns an invalid fence when flushing fails. If
> the application chains submits (i.e. do a blit and then do another using
> the output of the first blit) it is crucial to be sure the first submit
> has been performed before starting the second one.
>

Yes. So I suggest doing fence checking at the beginning of the
"tegra_stream_begin", if invalid fence found, return an error.


> - Arto