[Mesa-dev] [PATCH 2/2] r600g: simplify and fix flushing and synchronization v2

[Jerome Glisse]

On Mon, Jul 23, 2012 at 5:28 PM, Marek Olšák <maraeo at gmail.com> wrote:
> On Mon, Jul 23, 2012 at 4:25 PM, Jerome Glisse <j.glisse at gmail.com> wrote:
>> On Sun, Jul 22, 2012 at 8:58 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>> On Fri, Jul 20, 2012 at 4:54 AM, Jerome Glisse <j.glisse at gmail.com> wrote:
>>>> On Thu, Jul 19, 2012 at 10:25 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>>>> On Fri, Jul 20, 2012 at 4:17 AM, Jerome Glisse <j.glisse at gmail.com> wrote:
>>>>>> On Thu, Jul 19, 2012 at 10:06 PM, Marek Olšák <maraeo at gmail.com> wrote:
>>>>>>> I actually care a lot about lockups. Well, you are complaing about
>>>>>>> lockups, yet you have quite obvious bugs in your hyperz code, so let's
>>>>>>> fix them first. (I wouldn't even try and run the hyperz code in its
>>>>>>> current state. Please don't take that personally.) Then, if the
>>>>>>> lockups persist, we can start looking into *what* fixes them. You seem
>>>>>>> to think that this patch helps a lot, but you don't say why. Aren't
>>>>>>> you interested in what sequence of GPU commands helps? If I am
>>>>>>> counting correctly, there are 7 changes in behavior in this patch. It
>>>>>>> should be pretty easy to nail down the few that help, document them
>>>>>>> (like /* these two lines fix a lockup with hyperz */), and discard the
>>>>>>> rest. The documenting part is very important, so that the other
>>>>>>> developers won't break your code accidentally.
>>>>>>>
>>>>>>> Marek
>>>>>>>
>>>>>>
>>>>>> You haven't even try hyperz and you say i have an obvious bug, that's
>>>>>> kind of funny, but you would not know why. I try pretty much all of
>>>>>
>>>>> Oh come on, I already told you about all the bugs I found in the
>>>>> hyperz patch. You now know them too, and so does everybody else
>>>>> reading mesa-dev.
>>>>>
>>>>> Marek
>>>>>
>>>>
>>>> None of the issue you pointed out showed in piglit, none of them did
>>>> have impact on things like openarena, nexuiz, doomIII, lightmark, ...
>>>> so no issue you pointed does not cripple the hyperz patch, it's
>>>> working quite well for many things. Before you extrapolate, yes issue
>>>> you pointed out have impact in backward use of GL but none the less i
>>>> addressed them and i can tell you it does help a bit with lockup.
>>>
>>> I have no doubt that it helps with your lockups and I also have no
>>> doubt that the piece of code that helps can be bisected. I have
>>> mentioned 7 changes in the patch which are questionable, so the
>>> bisection should ideally take 3 steps. After we find the change which
>>> helps (and document it), we can discard the rest. That should give us
>>> the same stability as this patch does, but without unnecessary code
>>> which does cost GPU cycles (regardless of whether it is measurable on
>>> a particular machine or not).
>>>
>>> By the way, in draw_vbo, the emit functions should be called after
>>> r600_need_cs_space. Otherwise the command stream may overflow.
>>>
>>> Marek
>>
>> Again i haven't found a combination other than the outcome of the full
>> patch that helps more. So be my guest bisect on rv610, rv635, rv670,
>> rv710, rv740, rv770.
>
> So your patch doesn't fix any issue with evergreen? That's great.
> Thanks for keeping that to yourself. It's always a pleasure working
> with you. :) Now that we know the truth, the questionable changes to
> the evergreen code can be discarded freely.

No, it helps on evergreen too, redwood,juniper,turks and bart are the
only one i tested with. Evergreen is in a slightly better position but
when it comes to lockup there is no good metrics.

> Concerning older chipsets, I can do the bisection only on rs880, rv670
> and rv730. That will have to suffice. One way or another, every single
> change must be done for a *reason* and that reason should be
> documented if it's not obvious. Please give me all the necessary
> information, so that I can start bisecting. That is what lockups your
> patch fixes and where (name apps or tests, a specific place in a game,
> etc.) on what chipsets and whether hyperz is enabled.

Sorry no such things. It just helps, pick something test with and
without and you will see that with it lockup less often. I did not did
any of the change in isolation to fix a single case, it's just that
with all the change it helps. But of course you assume that i dumb and
i did spend no time testing, and just put together some random thing.

> It is very likely that all the changes I questioned in my first email
> do not make any difference with regard to lockups, because there are
> also other changes in your patch which may help too and which I fully
> agree with.
>
> Marek

As i said it's a package deal, i did not find a solution but i did
find something that improved the overall.

Cheers,
Jerome