<div dir="ltr"><div>Those are all good points. I would suggest putting the commit explanations into the code instead of commit messages, keeping the commit messages empty. When we have a good commit message, we can just copy it into the code as a comment. </div><div> </div><div>It's understandable that people don't understand some of my patches. I also don't understand some of other people's patches. It's normal. I don't think commit messages always help. If you don't work on a specific subtree on a regular basis, it's normal that over time you'll stop understanding it. </div><div> </div><div>Marek </div></div> <div class="gmail_quote"><div dir="ltr">On Tue, Nov 20, 2018 at 5:28 AM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 20/11/18 5:46 pm, Marek Olšák wrote: > On Tue, Nov 20, 2018 at 12:08 AM Dave Airlie <<a href="mailto:airlied@gmail.com" target="_blank">airlied@gmail.com</a> > <mailto:<a href="mailto:airlied@gmail.com" target="_blank">airlied@gmail.com</a>>> wrote: > > On Tue, 20 Nov 2018 at 14:42, Marek Olšák <<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a> > <mailto:<a href="mailto:maraeo@gmail.com" target="_blank">maraeo@gmail.com</a>>> wrote: > > > > On Mon, Nov 19, 2018 at 7:15 PM Bas Nieuwenhuizen > <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a> <mailto:<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>>> wrote: > >> > >> So I tried to test this with radv and got a bunch of crashes in CTS, > >> mostly around 3d image support: > >> > >> #3 0x00007ffff71a9396 in __assert_fail () from /usr/lib/libc.so.6 > >> #4 0x00007ffff69da3b4 in > >> Addr::V2::Gfx9Lib::HwlGetPreferredSurfaceSetting > (this=0x555557661b30, > >> pIn=0x7fffffffd5f0, pOut=0x7fffffffd5d0) > >> at ../mesa/src/amd/addrlib/src/gfx9/gfx9addrlib.cpp:3684 > >> #5 0x00007ffff69cf331 in > >> Addr::V2::Lib::Addr2GetPreferredSurfaceSetting (this=0x555557661b30, > >> pIn=0x7fffffffd5f0, pOut=0x7fffffffd5d0) > >> at ../mesa/src/amd/addrlib/src/core/addrlib2.cpp:1742 > >> #6 0x00007ffff69c4e87 in Addr2GetPreferredSurfaceSetting > >> (hLib=0x555557661b30, pIn=0x7fffffffd5f0, pOut=0x7fffffffd5d0) > >> at ../mesa/src/amd/addrlib/src/addrinterface.cpp:1697 > >> #7 0x00007ffff69bf8d4 in gfx9_get_preferred_swizzle_mode > >> (addrlib=0x555557661b30, in=0x7fffffffd690, is_fmask=false, > >> flags=33555202, swizzle_mode=0x7fffffffd698) > >> > >> It seems to be caused by the explicit swizzle mode override that > we do with > >> > >> commit b64b7125586ce48232658cd860f549a6139b6ddd > >> Author: Marek Olšák <<a href="mailto:marek.olsak@amd.com" target="_blank">marek.olsak@amd.com</a> > <mailto:<a href="mailto:marek.olsak@amd.com" target="_blank">marek.olsak@amd.com</a>>> > >> Date: Mon Apr 2 12:54:52 2018 -0400 > >> > >> ac/surface/gfx9: request desired micro tile mode explicitly > >> > >> Tested-by: Dieter Nützel <<a href="mailto:Dieter@nuetzel-hh.de" target="_blank">Dieter@nuetzel-hh.de</a> > <mailto:<a href="mailto:Dieter@nuetzel-hh.de" target="_blank">Dieter@nuetzel-hh.de</a>>> > >> > >> > >> Since we never got a reason to have it (the commit message above is > >> not descriptive and the patch not reviewed) and this is the second > >> time already that this breaks stuff (The other was allowing S tiling > >> for raven displayable surfaces, per 7eff8d7d3564), maybe revert > it and > >> let addrlib make the decision? > > > > > > Yes, my commits are mostly unreviewed. It's the norm now. Willing > reviewers don't exist anymore. I don't really mind that my patches > are not reviewed, but whoever complains that I push unreviewed > commits should ask himself why he didn't review them in their review > period. That applies to everybody. Either review regularly or accept > that unreviewed commits are normal. > > > > Secondly, past commits can't break future commits, so don't say > it breaks stuff again. It's illogical. > > > > There may be multiple reasons why the commit exists. As long as > reverting it doesn't break piglit / radeonsi, I'm OK with the reverting. > > Marek, > > There is no way anybody could review this commit, the commit log > contains 0 information on why or what the commit is doing or what it > fixes, there is nothing to say what the reviewer is looking out for. > > > It's something you could have mentioned on the review though. Instead > you decided to be silent. I'm not blaming you. I understand that you > probably had no time to review radv-related patches at that time. There > however has to be some reviewer if you care about the long-term outcome > whether you personally have time or not. 0 information is not an excuse > not to review. Had there been any explanation in the commit, it wouldn't > have made any difference on the current situation, other than perhaps > preventing this pointless discussion that doesn't help anybody. > > Marek As per the reply from Bas. Please take this as feedback, and not an attack. But the reason I don't even attempt to review most of your radeonsi patches is because the commit messages are empty. Reviews are are two way street, the time you save not writing a commit message just cost the reviewer time (probably more) in trying to comprehend what is going on. Double that time again (at least) for someone who doesn't know the driver intimately. As far as asking for a commit messages as part of review, I have done this on a number of occasions but it shouldn't really be on the reviewer to do this. In pretty much any other driver not having a proper commit message is pretty much an instant no go for anything other than truly self explanatory commits. Besides assisting review, I find commit messages an invaluable learning tool. As well as learning by just browsing/reviewing patch submissions the power of git unlocks these invaluable resources in an instant. Don't know what x code does or confused why it does something unexpected then a quick git log -S"string of code" will usually answer the question pretty quickly. Unfortunately I've done just this a number of times in radeonsi only to be greeted by an empty commit message. Again the same advantages of having a good commit message are seen when bisecting, etc. As far as staying silent on this feedback I have considered saying something about this for sometime. However I've avoided it so far as to avoid causing offense, or come across as lecturing a fellow open source dev on what seems like common knowledge. Now whether better commit messages would mean I would review more code is hard to say because I still don't know the driver that well. But it would make me feel more comfortable giving an ack to at least say I've read over the code and everything seems to make sense. I tend to look over most of you series currently but don't make any comments because I have no idea why a lot of the changes were made. Before I go I'll just say writing good commit messages is something we all need to work on, it can be easy to slip into habits of not doing so. I'm more than guilty of writing my fair share of less than ideal messages but I think in the long run the effort spent getting these things right is well worth it. </blockquote></div>