[Mesa-dev] [Mesa-stable] [PATCH] nvc0: do not clear surfaces bins in the validate function

[Samuel Pitoiset]

On 06/06/2016 12:30 AM, Emil Velikov wrote:
> On 5 June 2016 at 23:22, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Sun, Jun 5, 2016 at 6:08 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> On 5 June 2016 at 23:00, Samuel Pitoiset <samuel.pitoiset at gmail.com> wrote:
>>>>
>>>>
>>>> On 06/05/2016 11:50 PM, Emil Velikov wrote:
>>>>>
>>>>> On 5 June 2016 at 22:36, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>
>>>>>> On Sun, Jun 5, 2016 at 5:34 PM, Emil Velikov <emil.l.velikov at gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On 5 June 2016 at 22:17, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>>>
>>>>>>>> On Sun, Jun 5, 2016 at 5:16 PM, Emil Velikov <emil.l.velikov at gmail.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> On 5 June 2016 at 22:13, Emil Velikov <emil.l.velikov at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On 5 June 2016 at 17:56, Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> We should not call nouveau_bufctx_reset() inside a validate
>>>>>>>>>>> function.
>>>>>>>>>>
>>>>>>>>>> This seems to contradict the changes introduced in nvc0_compute.c.
>>>>>>>>>> Worth explaining a bit better the dos and don'ts ?
>>>>>>>>>>
>>>>>>>>> As this is already in master, can you please provide a more
>>>>>>>>> elaborate/correct summary for -stable ?
>>>>>>>>
>>>>>>>>
>>>>>>>> I think it's fine as is.
>>>>>>>>
>>>>>>>> Do: reset bufctx when setting dirty bit
>>>>>>>> Don't: reset bufctx in validate logic, since it's "too late" by then.
>>>>>>>> (Not strictly wrong, but just should do it earlier.)
>>>>>>>
>>>>>>>
>>>>>>> So nvc0_compute_*validate*_surfaces is not validate logic ? Err...
>>>>>>> what a confusing name it has ;-)
>>>>>>
>>>>>>
>>>>>> It validates compute. And it invalidates (and clears) the 3d bin.
>>>>>>
>>>>> So one can reset_bufctx(3d) from the compute validate and vice-versa.
>>>>> While doing reset_bufctx(foo) from foo validate is a bad idea ?
>>>>> Shouldn't one just say so in the commit message ?
>>>>
>>>>
>>>> Because the common practice is to clear foo bins at the same place where the
>>>> dirty_3d |= foo is updated, this makes sense. :)
>>>>
>>> Yet the commit message does not say that, right ? It says "We should
>>> not call nouveau_bufctx_reset() inside a validate function.", while
>>> the patch does the complete opposite - it adds a call to
>>> nouveau_bufctx_reset() inside a validate function.
>>>
>>> All I'm asking is for the commit message to reflect the code change or
>>> vice-versa. I hope I'm not being unreasonable ?
>>
>> The commit message also doesn't explain what bufctx's are, what a
>> validation function is, and the overall structure of the nouveau code
>> and how it uses those bufctx's.
>>
>> You're arguing about clarity with the ~2 active developers/reviewers
>> who understand the code.
> What I'm saying is that with contradicting commit messages like this
> one there'll be little others who will understand it :-\
>
>> I understand that this might be unclear to
>> you, but I know I'm not about to explain everything in commit messages
>> all the time -- too much effort for zero benefit. Case in point - had
>> this commit said "nvc0: fix validation logic" and left it at that, we
>> wouldn't be having this discussion. But there was a bit of an
>> explanation, that was perhaps not infinitely precise, and now there's
>> a long discussion about how it's unclear.
>>
> Looking from another angle - is false information better than no
> information ? All I was asking is to correct the commit message. I
> wasn't asking that one should explain reasoning behind it or anything
> deeper into the stack, which I believe was understood.
>
> Seems like that is too much to ask so I won't bother you guys any more.

I'll make an effort the next time. At least, try to not introduce 
contradictions between the code and the commit message.

>
> Thanks
> Emil
>