From: Paul Menzel <pmenzel@molgen.mpg.de> To: "Christian König" <christian.koenig@amd.com>, "Daniel Stone" <daniel@fooishbar.org>, "Alex Deucher" <alexdeucher@gmail.com> Cc: amd-gfx@lists.freedesktop.org, Arunpravin Paneer Selvam <arunpravin.paneerselvam@amd.com>, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Matthew Auld <matthew.auld@intel.com>, Alexander Deucher <alexander.deucher@amd.com> Subject: Re: Commit messages Date: Fri, 25 Mar 2022 16:56:05 +0100 [thread overview] Message-ID: <c62c2ef4-9223-b84d-b532-8f3acaaa80ff@molgen.mpg.de> (raw) In-Reply-To: <c41203c8-841b-889f-5c9b-5982ee961849@amd.com> 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@gmail.com> wrote: >>> On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone <daniel@fooishbar.org> >>> 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
WARNING: multiple messages have this Message-ID (diff)
From: Paul Menzel <pmenzel@molgen.mpg.de> To: "Christian König" <christian.koenig@amd.com>, "Daniel Stone" <daniel@fooishbar.org>, "Alex Deucher" <alexdeucher@gmail.com> Cc: amd-gfx@lists.freedesktop.org, Arunpravin Paneer Selvam <arunpravin.paneerselvam@amd.com>, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Matthew Auld <matthew.auld@intel.com>, Alexander Deucher <alexander.deucher@amd.com> Subject: Re: [Intel-gfx] Commit messages Date: Fri, 25 Mar 2022 16:56:05 +0100 [thread overview] Message-ID: <c62c2ef4-9223-b84d-b532-8f3acaaa80ff@molgen.mpg.de> (raw) In-Reply-To: <c41203c8-841b-889f-5c9b-5982ee961849@amd.com> 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@gmail.com> wrote: >>> On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone <daniel@fooishbar.org> >>> 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
next prev parent reply other threads:[~2022-03-25 15:56 UTC|newest] Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-23 6:25 [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu Arunpravin Paneer Selvam 2022-03-23 6:25 ` Arunpravin Paneer Selvam 2022-03-23 6:25 ` [Intel-gfx] " Arunpravin Paneer Selvam 2022-03-23 6:42 ` Paul Menzel 2022-03-23 6:42 ` Paul Menzel 2022-03-23 6:42 ` [Intel-gfx] " Paul Menzel 2022-03-23 7:42 ` Christian König 2022-03-23 7:42 ` Christian König 2022-03-23 7:42 ` [Intel-gfx] " Christian König 2022-03-23 8:10 ` Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu) Paul Menzel 2022-03-23 8:10 ` Paul Menzel 2022-03-23 8:10 ` [Intel-gfx] " Paul Menzel 2022-03-23 8:18 ` Christian König 2022-03-23 8:18 ` Christian König 2022-03-23 8:18 ` [Intel-gfx] " Christian König 2022-03-23 14:00 ` Daniel Stone 2022-03-23 14:00 ` [Intel-gfx] " Daniel Stone 2022-03-23 14:42 ` Alex Deucher 2022-03-23 14:42 ` [Intel-gfx] " Alex Deucher 2022-03-23 15:03 ` Daniel Stone 2022-03-23 15:03 ` [Intel-gfx] " Daniel Stone 2022-03-23 15:14 ` Alex Deucher 2022-03-23 15:14 ` [Intel-gfx] " Alex Deucher 2022-03-23 15:24 ` Daniel Stone 2022-03-23 15:24 ` [Intel-gfx] " Daniel Stone 2022-03-23 15:32 ` Christian König 2022-03-23 15:32 ` Christian König 2022-03-24 10:30 ` [Intel-gfx] " Daniel Vetter 2022-03-24 10:30 ` Daniel Vetter 2022-03-25 15:56 ` Paul Menzel [this message] 2022-03-25 15:56 ` [Intel-gfx] Commit messages Paul Menzel 2022-03-23 15:19 ` Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu) Christian König 2022-03-23 15:19 ` [Intel-gfx] " Christian König 2022-03-23 7:16 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/amdgpu: add drm buddy support to amdgpu (rev2) Patchwork 2022-03-23 7:37 ` [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu Christian König 2022-03-23 7:37 ` [Intel-gfx] " Christian König 2022-03-23 7:37 ` Christian König [not found] ` <MN2PR12MB4342B7FD0CC5C480DEEF8A77E41D9@MN2PR12MB4342.namprd12.prod.outlook.com> 2022-03-29 11:19 ` Arunpravin Paneer Selvam 2022-03-29 11:19 ` Arunpravin Paneer Selvam 2022-03-29 11:19 ` [Intel-gfx] " Arunpravin Paneer Selvam 2022-03-29 11:24 ` Christian König 2022-03-29 11:24 ` Christian König 2022-03-29 11:24 ` [Intel-gfx] " Christian König 2022-03-29 16:00 ` Arunpravin Paneer Selvam 2022-03-29 16:00 ` Arunpravin Paneer Selvam 2022-03-29 16:00 ` [Intel-gfx] " Arunpravin Paneer Selvam 2022-03-29 19:18 ` Arunpravin Paneer Selvam 2022-03-29 19:18 ` Arunpravin Paneer Selvam 2022-03-29 19:18 ` [Intel-gfx] " Arunpravin Paneer Selvam 2022-03-30 6:53 ` Christian König 2022-03-30 6:53 ` Christian König 2022-03-30 6:53 ` [Intel-gfx] " Christian König 2022-03-23 11:26 ` kernel test robot 2022-03-23 11:26 ` [Intel-gfx] " kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2016-10-10 0:35 Commit messages Paul Eggleton 2016-10-10 3:29 ` akuster808 2016-10-10 3:46 ` Paul Eggleton 2009-01-13 16:11 [PATCH 3/3] Adds a #!bash to the top of bash completions so that editors can recognize, it as a bash script. Also adds a few simple comments above commands that, take arguments. The comments are meant to remind editors of potential, problems that can occur when the script is sourced on systems with "set, -u." Ted Pavlic 2009-01-13 16:45 ` Shawn O. Pearce 2009-01-13 20:03 ` Boyd Stephen Smith Jr. 2009-01-13 20:10 ` Adeodato Simó 2009-01-13 20:24 ` Commit messages Teemu Likonen 2009-01-14 16:55 ` Ted Pavlic
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c62c2ef4-9223-b84d-b532-8f3acaaa80ff@molgen.mpg.de \ --to=pmenzel@molgen.mpg.de \ --cc=alexander.deucher@amd.com \ --cc=alexdeucher@gmail.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=arunpravin.paneerselvam@amd.com \ --cc=christian.koenig@amd.com \ --cc=daniel@fooishbar.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=matthew.auld@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.