[Mesa-dev] [PATCH 2/2] amd/addrlib: update Mesa's copy of addrlib

[Timothy Arceri]

On 20/11/18 5:46 pm, Marek Olšák wrote:
> On Tue, Nov 20, 2018 at 12:08 AM Dave Airlie <airlied at gmail.com 
> <mailto:airlied at gmail.com>> wrote:
> 
>     On Tue, 20 Nov 2018 at 14:42, Marek Olšák <maraeo at gmail.com
>     <mailto:maraeo at gmail.com>> wrote:
>      >
>      > On Mon, Nov 19, 2018 at 7:15 PM Bas Nieuwenhuizen
>     <bas at basnieuwenhuizen.nl <mailto:bas at basnieuwenhuizen.nl>> 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 <marek.olsak at amd.com
>     <mailto:marek.olsak at amd.com>>
>      >> Date:   Mon Apr 2 12:54:52 2018 -0400
>      >>
>      >>     ac/surface/gfx9: request desired micro tile mode explicitly
>      >>
>      >>     Tested-by: Dieter Nützel <Dieter at nuetzel-hh.de
>     <mailto:Dieter at nuetzel-hh.de>>
>      >>
>      >>
>      >> 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.