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

[Emil Velikov]

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.

Thanks
Emil