Dear Christian, dear Daniel, dear Alex,
Am 23.03.22 um 16:32 schrieb Christian König:
Am 23.03.22 um 16:24 schrieb Daniel Stone:
On Wed, 23 Mar 2022 at 15:14, Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone <daniel@xxxxxxxxxxxxx>
wrote:
That's not what anyone's saying here ...
No-one's demanding AMD publish RTL, or internal design docs, or
hardware specs, or URLs to JIRA tickets no-one can access.
This is a large and invasive commit with pretty big ramifications;
containing exactly two lines of commit message, one of which just
duplicates the subject.
It cannot be the case that it's completely impossible to provide any
justification, background, or details, about this commit being made.
Unless, of course, it's to fix a non-public security issue, that is
reasonable justification for eliding some of the details. But then
again, 'huge change which is very deliberately opaque' is a really
good way to draw a lot of attention to the commit, and it would be
better to provide more detail about the change to help it slip under
the radar.
If dri-devel@ isn't allowed to inquire about patches which are posted,
then CCing the list is just a façade; might as well just do it all
internally and periodically dump out pull requests.
I think we are in agreement. I think the withheld information
Christian was referring to was on another thread with Christian and
Paul discussing a workaround for a hardware bug:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.net%2Flists%2Famd-gfx%2Fmsg75908.html&data=04%7C01%7Cchristian.koenig%40amd.com%7C6a3f2815d83b4872577008da0ce1347a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637836458652370599%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=QtNB0XHMhTgH%2FNHMwF23Qn%2BgSdYyHJSenbpP%2FHG%2BkxE%3D&reserved=0
(Thank you Microsoft for keeping us safe.)
I guess it proves, how assuming what other people should know/have read,
especially when crossing message threads, is causing confusion and
misunderstandings.
Right, that definitely seems like some crossed wires. I don't see
anything wrong with that commit at all: the commit message and a
comment notes that there is a hardware issue preventing Raven from
being able to do TMZ+GTT, and the code does the very straightforward
and obvious thing to ensure that on VCN 1.0, any TMZ buffer must be
VRAM-placed.
My questions were:
Where is that documented, and how can this be reproduced?
Shouldn’t these be answered by the commit message? In five(?) years,
somebody, maybe even with access to the currently non-public documents,
finds a fault in the commit, and would be helped by having an
document/errata number where to look at. To verify the fix, the
developer would need a method to produce the error, so why not just
share it?
Also, I assume that workarounds often come with downsides, as otherwise
it would have been programmed like this from the beginning, or instead
of “workaround” it would be called “improvement”. Shouldn’t that also be
answered?
So totally made-up example:
Currently, there is a graphics corruption running X on system Y. This is
caused by a hardware bug in Raven ASIC (details internal document
#NNNN/AMD-Jira #N), and can be worked around by [what is in the commit
message].
The workaround does not affect the performance, and testing X shows the
error is fixed.
This one, on the other hand, is much less clear ...
Yes, completely agree. I mean a good bunch of comments on commit
messages are certainly valid and we could improve them.
That’d be great.
But this patch here was worked on by both AMD and Intel developers.
Where both sides and I think even people from other companies perfectly
understands why, what, how etc...
When now somebody comes along and asks for a whole explanation of the
context why we do it then that sounds really strange to me.
The motivation should be part of the commit message. I didn’t mean
anyone to rewrite buddy memory allocator Wikipedia article [1]. But the
commit message at hand for switching the allocator is definitely too terse.
Kind regards,
Paul
[1]: https://en.wikipedia.org/wiki/Buddy_memory_allocation
