On 20.11.18 07:46, Marek Olšák wrote:
On Tue, Nov 20, 2018 at 12:08 AM Dave Airlie <airl...@gmail.com
<mailto:airl...@gmail.com>> wrote:
On Tue, 20 Nov 2018 at 14:42, Marek Olšák <mar...@gmail.com
<mailto:mar...@gmail.com>> wrote:
>
> On Mon, Nov 19, 2018 at 7:15 PM Bas Nieuwenhuizen
<b...@basnieuwenhuizen.nl <mailto:b...@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.ol...@amd.com
<mailto:marek.ol...@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 <die...@nuetzel-hh.de
<mailto:die...@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.
Well, the difference is that it might make it easier to figure out what
the right fix (whatever you want to call it) is now given the addrlib
change.
The funny thing is, I actually ran into exactly this issue while working
on an internal branch, and my conclusion then was that it's probably the
right thing to just get rid of the preferred set. I do need to put that
through proper testing.
Cheers,
Nicolai
Marek
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
