Include request for reset-rework branch v4

[Jerome Glisse]

On Thu, May 3, 2012 at 1:20 PM, Alex Deucher <alexdeucher at gmail.com> wrote:
> 2012/5/3 Jerome Glisse <j.glisse at gmail.com>:
>> On Thu, May 3, 2012 at 12:39 PM, Christian König
>> <deathsimple at vodafone.de> wrote:
>>> On 03.05.2012 18:32, Jerome Glisse wrote:
>>>>
>>>> On Thu, May 3, 2012 at 4:19 AM, Christian König<deathsimple at vodafone.de>
>>>>  wrote:
>>>>>
>>>>> On 02.05.2012 18:01, Jerome Glisse wrote:
>>>>>>
>>>>>> On Wed, May 2, 2012 at 9:11 AM, Christian König<deathsimple at vodafone.de>
>>>>>>  wrote:
>>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> there still seems to be the need for some further discussion about the
>>>>>>> SA
>>>>>>> code,
>>>>>>> so I again split that out of the patchset and tested the result a bit.
>>>>>>>
>>>>>>> Most of the stuff still works fine without those offending changes, so
>>>>>>> to
>>>>>>> avoid
>>>>>>> mailing around unrelated and already reviewed patches, I request the
>>>>>>> include
>>>>>>> the following 17 patches into drm-next.
>>>>>>>
>>>>>>> If you prefer to merge they are also available from
>>>>>>> git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Christian.
>>>>>>>
>>>>>> I am ok with this 17 patchset, i just sent 3 patch on top of those 17
>>>>>> that
>>>>>> bring back some other of the previous cleanup.
>>>>>
>>>>> At least for now those three are NAK, cause I just realized we need to
>>>>> put
>>>>> those on top of a more sophisticated fence implementation.
>>>>>
>>>>> Your idea of not using a list, but 64 bit sequences instead actually
>>>>> sounds
>>>>> quite nifty to me. Going to hack something together in the next couple of
>>>>> hours.
>>>>>
>>>>> Christian.
>>>>
>>>> Btw you said that you are having issue when using multiple ring, it
>>>> comes to my attention that you never sync with the GFX ring (unless
>>>> asked by userspace) that's wrong, before scheduling on another ring
>>>> than GFX index you need to emit semaphore to make the ring wait for
>>>> the last emited fence on the GFX ring because of ttm. What might
>>>> happen is that ttm scheduled bo move on the GFX ring and that the ring
>>>> you work on start using those bo at there soon to be GPU offset while
>>>> the bo data is not there yet.
>>>
>>> Yeah, already stumbled over that but IIRC Alex already solved that issue.
>>>
>>> We set the sync_obj to the fence of the move operation, so when there is a
>>> move operation in progress we sync to it correctly (at least I hope so).
>>>
>>> Christian.
>>>
>>
>> Looking at code doesn't seems ok, yes you use the fence from the move
>> operation but you only emit wait if userspace ask for it, that's
>> wrong.
>>
>> if (!(p->relocs[i].flags & RADEON_RELOC_DONT_SYNC)) {
>>
>
> The default is to sync all the rings.  We only skip the sync if the
> _DONT_ sync flag is set.
>
> Alex
>

That's my point exactly, so if a bo move happen and userspace set dont
sync flag then the other ring might start doing work before bo data is
in place. This test should be removed or at least make the sync to the
bo move fence unconditional and use this flag only for sync with other
ring relative to current job you are scheduling.

Cheers,
Jerome