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

[Bas Nieuwenhuizen]

wow,  I apologize for wording this in a way that looks like an attack :(

What I'm primarily concerned with is that this logic is effectively
second-guessing addrlib, which breaks when we second-guess the wrong
value for pre-existing addrlib (as was the case with the Raven issue),
or when addrlib changes its choice. So obviously this adds maintenance
work. Of course this can still be worth it but with neither a commit
message or review comments I can't tell. Hence I cc'd you on this
thread, so we can get to know the goal of this second-guessing and
then get consensus on whether it is worth it. I'm not wanting to
revert just for the fact it is unreviewed.

Independent of that, the reviewing issues for radeonsi are nasty,
especially when Nicolai is off-grid. I'm not sure I have a problem
with unreviewed changes in radeonsi itself honestly since it seems par
for the course in mesa for the drivers with ~1 maintainer to do that
anyway. Is it ideal? no, but this clearly needs effort from reviewers
to review code that is in another driver, and I'm not sure how to best
achieve that. I'd like it if some more effort was put into making sure
amd/common has reviews, but you are right that when it is part of a
radeonsi series I'm not the most responsive reviewer on those patches
either :(






On Tue, Nov 20, 2018 at 7:47 AM Marek Olšák <maraeo at gmail.com> wrote:
>
> On Tue, Nov 20, 2018 at 12:08 AM Dave Airlie <airlied at gmail.com> wrote:
>>
>> On Tue, 20 Nov 2018 at 14:42, Marek Olšák <maraeo at gmail.com> wrote:
>> >
>> > On Mon, Nov 19, 2018 at 7:15 PM Bas Nieuwenhuizen <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>
>> >> 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>
>> >>
>> >>
>> >> 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