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@xxxxxxx>
> Reported-by: Scott Bruce <smbruce@xxxxxxxxx>
> Reported-by: Chris Hixon <linux-kernel-bugs@xxxxxxxxxxxxx>
> Reported-by: spasswolf@xxxxxx
> 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@xxxxxxxxxxxxx/
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@xxxxxxx>
> ---
> 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
