AMD Display Core (DC) patches (was: [PATCH 13/16] drm/amd/display: Revert FEC check in validation)

[Paul Menzel]

[Cc: +dri-devel at 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 at 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 at amd.com>
> Acked-by: Alex Hung <alex.hung at amd.com>
> Signed-off-by: Martin Leung <Martin.Leung at 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