AMD Display Core (DC) patches (was: [PATCH 13/16] drm/amd/display: Revert FEC check in validation)
[Cc: +dri-devel@lists.freedesktop.org, +Daniel Vetter, +Alexander Deucher, +Greg KH]
Dear Alex,
I am a little confused and upset about how Display Core patches are handled in the Linux kernel.
Am 25.03.22 um 23:53 schrieb Alex Hung:
From: Martin Leung Martin.Leung@amd.com
git puts a line “This reverts commit …” into the commit message, when something is reverted. Why isn’t this here? Right now, commit 7d56a154e22f, reverted here, is proposed for the stable series. I guess, because these indicators and meta data are missing.
why and how: causes failure on install on certain machines
Why are such kind of commit messages accepted? What does “failure on install” even mean? Why can’t the machine configuration be documented so it can be reproduced, when necessary.
No less confusing, the date you posted it on amd-gfx is from March 25th, 2022, but the author date of the commit in agd5f/amd-staging-drm-next is `Fri Mar 18 11:12:36 2022 -0400`. Why is the patch missing the Date field then?
Reviewed-by: George Shen George.Shen@amd.com Acked-by: Alex Hung alex.hung@amd.com Signed-off-by: Martin Leung Martin.Leung@amd.com
Shouldn’t the Signed-off-by line by the author go first?
You committed this on `Mon Mar 28 08:26:48 2022 -0600`, while you posted the patch on amd-gfx on Friday. How should *proper* review happen over the weekend?
drivers/gpu/drm/amd/display/dc/core/dc.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index f2ad8f58e69c..c436db416708 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -1496,10 +1496,6 @@ bool dc_validate_boot_timing(const struct dc *dc, if (!link->link_enc->funcs->is_dig_enabled(link->link_enc)) return false;
/* Check for FEC status*/
if (link->link_enc->funcs->fec_is_active(link->link_enc))
return false;enc_inst = link->link_enc->funcs->get_dig_frontend(link->link_enc);
if (enc_inst == ENGINE_ID_UNKNOWN)
The patch reverted here, also lacked proper review, had a to-be desired commit message, did not follow the Linux kernel coding style (missing space before the comment terminator), so should not have been committed in the first place.
Seeing how many people are in the Cc list, I would have hoped, that somebody noticed and commented. The current state also makes it really hard for non-AMD employees to get the necessary information to do proper reviews as the needed documentation and information is non-public. So good/excellent commit messages are a must. I think to remember, you replied to me once, that Display Core patches are shared also with the Microsoft Windows driver, restricting the workflow options. But I think the issues I mentioned are unrelated. I know graphics hardware is very complex, but if quality of the commits and review would be improved, hopefully it saves time for everyone in the end, as less bugs are introduced.
Could AMD team please address these issues as soon as possible?
Kind regards,
Paul
On Tue, Apr 12, 2022 at 08:52:11AM +0200, Paul Menzel wrote:
Reviewed-by: George Shen George.Shen@amd.com Acked-by: Alex Hung alex.hung@amd.com Signed-off-by: Martin Leung Martin.Leung@amd.com
Shouldn’t the Signed-off-by line by the author go first?
No, this is the correct order.
thanks,
greg k-h
On Tue, Apr 12, 2022 at 2:52 AM Paul Menzel pmenzel@molgen.mpg.de wrote:
[Cc: +dri-devel@lists.freedesktop.org, +Daniel Vetter, +Alexander Deucher, +Greg KH]
Dear Alex,
I am a little confused and upset about how Display Core patches are handled in the Linux kernel.
Am 25.03.22 um 23:53 schrieb Alex Hung:
From: Martin Leung Martin.Leung@amd.com
git puts a line “This reverts commit …” into the commit message, when something is reverted. Why isn’t this here? Right now, commit 7d56a154e22f, reverted here, is proposed for the stable series. I guess, because these indicators and meta data are missing.
Sasha's tools proposed to pick it up which I often struggle with. It's very useful, but at the same time, we don't have the bandwidth to test every combination of patches that Sacha proposes on every stable kernel. The additional metadata would be useful, but I'm not sure if it would solve this problem. This patch would likely be picked up by Sasha as well once it landed.
why and how: causes failure on install on certain machines
Why are such kind of commit messages accepted? What does “failure on install” even mean? Why can’t the machine configuration be documented so it can be reproduced, when necessary.
No less confusing, the date you posted it on amd-gfx is from March 25th, 2022, but the author date of the commit in agd5f/amd-staging-drm-next is `Fri Mar 18 11:12:36 2022 -0400`. Why is the patch missing the Date field then?
Reviewed-by: George Shen George.Shen@amd.com Acked-by: Alex Hung alex.hung@amd.com Signed-off-by: Martin Leung Martin.Leung@amd.com
Shouldn’t the Signed-off-by line by the author go first?
You committed this on `Mon Mar 28 08:26:48 2022 -0600`, while you posted the patch on amd-gfx on Friday. How should *proper* review happen over the weekend?
drivers/gpu/drm/amd/display/dc/core/dc.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index f2ad8f58e69c..c436db416708 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -1496,10 +1496,6 @@ bool dc_validate_boot_timing(const struct dc *dc, if (!link->link_enc->funcs->is_dig_enabled(link->link_enc)) return false;
/* Check for FEC status*/if (link->link_enc->funcs->fec_is_active(link->link_enc))return false;enc_inst = link->link_enc->funcs->get_dig_frontend(link->link_enc); if (enc_inst == ENGINE_ID_UNKNOWN)The patch reverted here, also lacked proper review, had a to-be desired commit message, did not follow the Linux kernel coding style (missing space before the comment terminator), so should not have been committed in the first place.
Seeing how many people are in the Cc list, I would have hoped, that somebody noticed and commented. The current state also makes it really hard for non-AMD employees to get the necessary information to do proper reviews as the needed documentation and information is non-public. So good/excellent commit messages are a must. I think to remember, you replied to me once, that Display Core patches are shared also with the Microsoft Windows driver, restricting the workflow options. But I think the issues I mentioned are unrelated. I know graphics hardware is very complex, but if quality of the commits and review would be improved, hopefully it saves time for everyone in the end, as less bugs are introduced.
I agree that good commit messages are ideal and we should strive for them, but this is certainly not limited to GPUs. Subsystems all require a certain amount of assumed knowledge when it comes to commit messages.
At the same time, I think it would be good to set expectations. Too many frivolous review comments and escalations tends to turn people off even if they are well intentioned. There are always new people becoming kernel developers that may not have had a lot of previous kernel experience. That said, to the AMD developers, please try and address every comment.
Alex
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Greg KH -
Paul Menzel