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

Tue Nov 20 22:00:34 UTC 2018

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.

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.

Marek

On Tue, Nov 20, 2018 at 5:28 AM Timothy Arceri <tarceri at itsqueeze.com>
wrote:

> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181120/f6242a17/attachment-0001.html>