Re: [Intel-gfx] Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
On Wed, 23 Mar 2022 at 16:32, Christian König wrote: > > Am 23.03.22 um 16:24 schrieb Daniel Stone: > > On Wed, 23 Mar 2022 at 15:14, Alex Deucher wrote: > >> On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone 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.htmldata=04%7C01%7Cchristian.koenig%40amd.com%7C6a3f2815d83b4872577008da0ce1347a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637836458652370599%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=QtNB0XHMhTgH%2FNHMwF23Qn%2BgSdYyHJSenbpP%2FHG%2BkxE%3Dreserved=0 > > 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. > > > > 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. > > 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. Yeah gpus are using pages a lot more like the cpu (with bigger pages of benefit, but not required, hence the buddy allocator to coalesce them), and extremely funny contig allocations with bonkers requirements aren't needed anymore (which was the speciality of drm_mm.c). Hence why both i915 and amdgpu move over to this new buddy allocator for managing vram. I guess that could be added to the commit message, but also it's kinda well known - the i915 patches also didn't explain why we want to manage our vram with a buddy allocator (I think some of the earlier versions explained it a bit, but the version with ttm integration that landed didnt). But yeah the confusing comments about hiding stuff that somehow spilled over from other discussions into this didn't help :-/ -Daniel > Thanks for jumping in here, > Christian. > > > > > Cheers, > > Daniel > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
Am 23.03.22 um 16:24 schrieb Daniel Stone: On Wed, 23 Mar 2022 at 15:14, Alex Deucher wrote: On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone 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.htmldata=04%7C01%7Cchristian.koenig%40amd.com%7C6a3f2815d83b4872577008da0ce1347a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637836458652370599%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=QtNB0XHMhTgH%2FNHMwF23Qn%2BgSdYyHJSenbpP%2FHG%2BkxE%3Dreserved=0 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. 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. 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. Thanks for jumping in here, Christian. Cheers, Daniel
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
On Wed, 23 Mar 2022 at 15:14, Alex Deucher wrote: > On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone 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://www.spinics.net/lists/amd-gfx/msg75908.html 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. This one, on the other hand, is much less clear ... Cheers, Daniel
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
Am 23.03.22 um 15:00 schrieb Daniel Stone: On Wed, 23 Mar 2022 at 08:19, Christian König wrote: Am 23.03.22 um 09:10 schrieb Paul Menzel: Sorry, I disagree. The motivation needs to be part of the commit message. For example see recent discussion on the LWN article *Donenfeld: Random number generator enhancements for Linux 5.17 and 5.18* [1]. How much the commit message should be extended, I do not know, but the current state is insufficient (too terse). Well the key point is it's not about you to judge that. If you want to complain about the commit message then come to me with that and don't request information which isn't supposed to be publicly available. So to make it clear: The information is intentionally hold back and not made public. In that case, the code isn't suitable to be merged into upstream trees; it can be resubmitted when it can be explained. Well what Paul is requesting here are business information, not technical information. In other words we perfectly explained why it is technically necessary already which is sufficient. Regards, Christian. Cheers, Daniel
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
On Wed, Mar 23, 2022 at 11:04 AM Daniel Stone wrote: > > Hi Alex, > > On Wed, 23 Mar 2022 at 14:42, Alex Deucher wrote: > > On Wed, Mar 23, 2022 at 10:00 AM Daniel Stone wrote: > > > On Wed, 23 Mar 2022 at 08:19, Christian König > > > wrote: > > > > Well the key point is it's not about you to judge that. > > > > > > > > If you want to complain about the commit message then come to me with > > > > that and don't request information which isn't supposed to be publicly > > > > available. > > > > > > > > So to make it clear: The information is intentionally hold back and not > > > > made public. > > > > > > In that case, the code isn't suitable to be merged into upstream > > > trees; it can be resubmitted when it can be explained. > > > > So you are saying we need to publish the problematic RTL to be able to > > fix a HW bug in the kernel? That seems a little unreasonable. Also, > > links to internal documents or bug trackers don't provide much value > > to the community since they can't access them. In general, adding > > internal documents to commit messages is frowned on. > > 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://www.spinics.net/lists/amd-gfx/msg75908.html Alex Alex
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
Hi Alex, On Wed, 23 Mar 2022 at 14:42, Alex Deucher wrote: > On Wed, Mar 23, 2022 at 10:00 AM Daniel Stone wrote: > > On Wed, 23 Mar 2022 at 08:19, Christian König > > wrote: > > > Well the key point is it's not about you to judge that. > > > > > > If you want to complain about the commit message then come to me with > > > that and don't request information which isn't supposed to be publicly > > > available. > > > > > > So to make it clear: The information is intentionally hold back and not > > > made public. > > > > In that case, the code isn't suitable to be merged into upstream > > trees; it can be resubmitted when it can be explained. > > So you are saying we need to publish the problematic RTL to be able to > fix a HW bug in the kernel? That seems a little unreasonable. Also, > links to internal documents or bug trackers don't provide much value > to the community since they can't access them. In general, adding > internal documents to commit messages is frowned on. 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. Cheers, Daniel
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
On Wed, Mar 23, 2022 at 10:00 AM Daniel Stone wrote: > > On Wed, 23 Mar 2022 at 08:19, Christian König > wrote: > > Am 23.03.22 um 09:10 schrieb Paul Menzel: > > > Sorry, I disagree. The motivation needs to be part of the commit > > > message. For example see recent discussion on the LWN article > > > *Donenfeld: Random number generator enhancements for Linux 5.17 and > > > 5.18* [1]. > > > > > > How much the commit message should be extended, I do not know, but the > > > current state is insufficient (too terse). > > > > Well the key point is it's not about you to judge that. > > > > If you want to complain about the commit message then come to me with > > that and don't request information which isn't supposed to be publicly > > available. > > > > So to make it clear: The information is intentionally hold back and not > > made public. > > In that case, the code isn't suitable to be merged into upstream > trees; it can be resubmitted when it can be explained. So you are saying we need to publish the problematic RTL to be able to fix a HW bug in the kernel? That seems a little unreasonable. Also, links to internal documents or bug trackers don't provide much value to the community since they can't access them. In general, adding internal documents to commit messages is frowned on. Alex
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
On Wed, 23 Mar 2022 at 08:19, Christian König wrote: > Am 23.03.22 um 09:10 schrieb Paul Menzel: > > Sorry, I disagree. The motivation needs to be part of the commit > > message. For example see recent discussion on the LWN article > > *Donenfeld: Random number generator enhancements for Linux 5.17 and > > 5.18* [1]. > > > > How much the commit message should be extended, I do not know, but the > > current state is insufficient (too terse). > > Well the key point is it's not about you to judge that. > > If you want to complain about the commit message then come to me with > that and don't request information which isn't supposed to be publicly > available. > > So to make it clear: The information is intentionally hold back and not > made public. In that case, the code isn't suitable to be merged into upstream trees; it can be resubmitted when it can be explained. Cheers, Daniel
Re: Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
Hi Paul, Am 23.03.22 um 09:10 schrieb Paul Menzel: Dear Christian, Am 23.03.22 um 08:42 schrieb Christian König: Am 23.03.22 um 07:42 schrieb Paul Menzel: Am 23.03.22 um 07:25 schrieb Arunpravin Paneer Selvam: - Remove drm_mm references and replace with drm buddy functionalities The commit message summary to me suggested, you can somehow use both allocators now. Two suggestions below: 1. Switch to drm buddy allocator 2. Use drm buddy alllocator - Add res cursor support for drm buddy As an allocator switch sounds invasive, could you please extend the commit message, briefly describing the current situation, saying what the downsides are, and why the buddy allocator is “better”. Well, Paul please stop bothering developers with those requests. It's my job as maintainer to supervise the commit messages and it is certainly NOT require to explain all the details of the current situation in a commit message. That is just overkill. I did not request all the details, and I think my requests are totally reasonable. But let’s change the perspective. If there were not any AMD graphics drivers bug, I would have never needed to look at the code and deal with it. Unfortunately the AMD graphics driver situation – which improved a lot in recent years – with no public documentation, proprietary firmware and complex devices is still not optimal, and a lot of bugs get reported, and I am also hit by bugs, taking time to deal with them, and maybe reporting and helping to analyze them. So to keep your wording, if you would stop bothering users with bugs and requesting their help in fixing them – asking the user to bisect the issue is often the first thing. Actually it should not be unreasonable for customers buying an AMD device to expect get bug free drivers. It’s strange and a sad fact, that the software industry succeeded to sway that valid expectation and customers now except they need to regularly install software updates, and do not get, for example, a price reduction when there are bugs. Also, as stated everywhere, reviewer time is scarce, so commit authors should make it easy to attract new folks. A simple note that we are switching from the drm_mm backend to the buddy backend is sufficient, and that is exactly what the commit message is saying here. Sorry, I disagree. The motivation needs to be part of the commit message. For example see recent discussion on the LWN article *Donenfeld: Random number generator enhancements for Linux 5.17 and 5.18* [1]. How much the commit message should be extended, I do not know, but the current state is insufficient (too terse). Well the key point is it's not about you to judge that. If you want to complain about the commit message then come to me with that and don't request information which isn't supposed to be publicly available. So to make it clear: The information is intentionally hold back and not made public. Regards, Christian. Kind regards, Paul [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F888413%2Fdata=04%7C01%7Cchristian.koenig%40amd.com%7C466afc41893d4f43ab6a08da0ca48c0a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637836198129744073%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=Ar0P8Yc61MTXP0khtoH6WVRDAKhvxXNaOJY0LRnl8Qk%3Dreserved=0 "Donenfeld: Random number generator enhancements for Linux 5.17 and 5.18"
Commit messages (was: [PATCH v11] drm/amdgpu: add drm buddy support to amdgpu)
Dear Christian, Am 23.03.22 um 08:42 schrieb Christian König: Am 23.03.22 um 07:42 schrieb Paul Menzel: Am 23.03.22 um 07:25 schrieb Arunpravin Paneer Selvam: - Remove drm_mm references and replace with drm buddy functionalities The commit message summary to me suggested, you can somehow use both allocators now. Two suggestions below: 1. Switch to drm buddy allocator 2. Use drm buddy alllocator - Add res cursor support for drm buddy As an allocator switch sounds invasive, could you please extend the commit message, briefly describing the current situation, saying what the downsides are, and why the buddy allocator is “better”. Well, Paul please stop bothering developers with those requests. It's my job as maintainer to supervise the commit messages and it is certainly NOT require to explain all the details of the current situation in a commit message. That is just overkill. I did not request all the details, and I think my requests are totally reasonable. But let’s change the perspective. If there were not any AMD graphics drivers bug, I would have never needed to look at the code and deal with it. Unfortunately the AMD graphics driver situation – which improved a lot in recent years – with no public documentation, proprietary firmware and complex devices is still not optimal, and a lot of bugs get reported, and I am also hit by bugs, taking time to deal with them, and maybe reporting and helping to analyze them. So to keep your wording, if you would stop bothering users with bugs and requesting their help in fixing them – asking the user to bisect the issue is often the first thing. Actually it should not be unreasonable for customers buying an AMD device to expect get bug free drivers. It’s strange and a sad fact, that the software industry succeeded to sway that valid expectation and customers now except they need to regularly install software updates, and do not get, for example, a price reduction when there are bugs. Also, as stated everywhere, reviewer time is scarce, so commit authors should make it easy to attract new folks. A simple note that we are switching from the drm_mm backend to the buddy backend is sufficient, and that is exactly what the commit message is saying here. Sorry, I disagree. The motivation needs to be part of the commit message. For example see recent discussion on the LWN article *Donenfeld: Random number generator enhancements for Linux 5.17 and 5.18* [1]. How much the commit message should be extended, I do not know, but the current state is insufficient (too terse). Kind regards, Paul [1]: https://lwn.net/Articles/888413/ "Donenfeld: Random number generator enhancements for Linux 5.17 and 5.18"
