Re: [PATCH v2] drm/amd/display: explicitly update clocks when DC is set to D3 in s0i3

[Thorsten Leemhuis <regressions@leemhuis.info>]

Hi, this is your Linux kernel regression tracker speaking.

On 05.01.22 18:06, Mario Limonciello wrote:
> The WA from commit 5965280abd30 ("drm/amd/display: Apply w/a for
> hard hang on HPD") causes a regression in s0ix where the system will
> fail to resume properly.  This may be because an HPD was active the last
> time clocks were updated but clocks didn't get updated again during s0ix.
> 
> So add an extra call to update clocks as part of the suspend routine:
> dm_suspend->dc_set_power_state->clk_mgr_optimize_pwr_state
> 
> In case HPD is set during this time, also check if the call happened during
> suspend to allow overriding the WA.

Thx for this. Our of interest: is that something that should still go
into v5.16?

> Cc: Qingqing Zhuo <qingqing.zhuo@amd.com>
> Reported-by: Scott Bruce <smbruce@gmail.com>
> Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com>
> Reported-by: spasswolf@web.de
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215436
> BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1821
> BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1852

What is "BugLink"? It's not mention in the kernel's documentation, which
tells people to use "Link:" for linking to bug reports:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html

That is not new, bug recently was made more explicit.

Hence, please consider changing them to "Link:", there are tools out
there that repy on them (I'm running such a tool, but there might be
others out there we are not aware of).

FWIW, in principe I agree that we need a seperate tag for this. But then
we should get this discussed and blessed through the usual channels.
That's why I recently proposed "Reported:", without much success so far:
https://lore.kernel.org/lkml/c6724c7fb534a46e095e6aef13d996ed9589e578.1639042966.git.linux@leemhuis.info/

Ciao, Thorsten

> Fixes: 5965280abd30 ("drm/amd/display: Apply w/a for hard hang on HPD")
> Fixes: 1bd3bc745e7f ("drm/amd/display: Extend w/a for hard hang on HPD to dcn20")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> changes from v1->v2:
>  * Add fallthrough statement
>  * Extend case to check if call was explicitly in s0ix since #1852 showed hpd_state
>    can be set at this time too
>  * Adjust commit message and title
>  * Add extra commit and bug fixed to metadata
>  drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c | 5 ++++-
>  drivers/gpu/drm/amd/display/dc/core/dc.c                  | 3 +++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c
> index fbda42313bfe..fa5efe10b779 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr.c
> @@ -23,6 +23,8 @@
>   *
>   */
>  
> +#include "amdgpu.h"
> +
>  #include "dccg.h"
>  #include "clk_mgr_internal.h"
>  
> @@ -126,6 +128,7 @@ static void rn_update_clocks(struct clk_mgr *clk_mgr_base,
>  			bool safe_to_lower)
>  {
>  	struct clk_mgr_internal *clk_mgr = TO_CLK_MGR_INTERNAL(clk_mgr_base);
> +	struct amdgpu_device *adev = clk_mgr_base->ctx->driver_context;
>  	struct dc_clocks *new_clocks = &context->bw_ctx.bw.dcn.clk;
>  	struct dc *dc = clk_mgr_base->ctx->dc;
>  	int display_count;
> @@ -157,7 +160,7 @@ static void rn_update_clocks(struct clk_mgr *clk_mgr_base,
>  			}
>  
>  			/* if we can go lower, go lower */
> -			if (display_count == 0 && !hpd_state) {
> +			if (display_count == 0 && (adev->in_s0ix || !hpd_state)) {
>  				rn_vbios_smu_set_dcn_low_power_state(clk_mgr, DCN_PWR_STATE_LOW_POWER);
>  				/* update power state */
>  				clk_mgr_base->clks.pwr_state = DCN_PWR_STATE_LOW_POWER;
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 91c4874473d6..8e0c820f6892 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -3299,6 +3299,9 @@ void dc_set_power_state(
>  		}
>  
>  		break;
> +	case DC_ACPI_CM_POWER_STATE_D3:
> +		clk_mgr_optimize_pwr_state(dc, dc->clk_mgr);
> +		fallthrough;
>  	default:
>  		ASSERT(dc->current_state->stream_count == 0);
>  		/* Zero out the current context so that on resume we start with