On Sun, Jun 5, 2016 at 6:08 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 5 June 2016 at 23:00, Samuel Pitoiset <samuel.pitoi...@gmail.com> wrote: >> >> >> On 06/05/2016 11:50 PM, Emil Velikov wrote: >>> >>> On 5 June 2016 at 22:36, Ilia Mirkin <imir...@alum.mit.edu> wrote: >>>> >>>> On Sun, Jun 5, 2016 at 5:34 PM, Emil Velikov <emil.l.veli...@gmail.com> >>>> wrote: >>>>> >>>>> On 5 June 2016 at 22:17, Ilia Mirkin <imir...@alum.mit.edu> wrote: >>>>>> >>>>>> On Sun, Jun 5, 2016 at 5:16 PM, Emil Velikov <emil.l.veli...@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> On 5 June 2016 at 22:13, Emil Velikov <emil.l.veli...@gmail.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> On 5 June 2016 at 17:56, Samuel Pitoiset <samuel.pitoi...@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. 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. -ilia _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
