[PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Guilherme G. Piccoli]

This reverts commit 06c3a652a787efc960af7c8816036d25c4227c6c.

After this commit, the Steam Deck cannot boot with graphics anymore;
the following message is observed on dmesg:

"[drm] ERROR [CRTC:67:crtc-0] flip_done timed out"

No other error is observed, it just stays like that. After bisecting
amd-staging-drm-next, we narrowed it down to this commit. Seems it
makes sense to revert it to have the tree bootable until a proper
solution is worked.

Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
Cc: André Almeida <andrealmeid@igalia.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

---

Hi Alex / Aurabindo, we couldn't boot the Deck with in HEAD
(amd-staging-drm-next), git bisect led to this commit. Since its
description already mentions a potential proper solution, related
to the DMCUB (and some complex state tracking), I thought it was
more effective to revert it to allow booting the tree in Deck (and
maybe other HW - I just tested the Deck BTW).
Lemme know your thoughts.

Special thanks to André and Melissa for helping the debug / bisect!
We're open to test alternative patches, feel free to ping.
Cheers,

Guilherme


 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c | 15 ++++++++++++---
 drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c | 10 ----------
 2 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
index 0e8f4f36c87c..27419cd98264 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
@@ -945,10 +945,19 @@ void optc1_set_drr(
 				OTG_FORCE_LOCK_ON_EVENT, 0,
 				OTG_SET_V_TOTAL_MIN_MASK_EN, 0,
 				OTG_SET_V_TOTAL_MIN_MASK, 0);
-	}
 
-	// Setup manual flow control for EOF via TRIG_A
-	optc->funcs->setup_manual_trigger(optc);
+		// Setup manual flow control for EOF via TRIG_A
+		optc->funcs->setup_manual_trigger(optc);
+
+	} else {
+		REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
+				OTG_SET_V_TOTAL_MIN_MASK, 0,
+				OTG_V_TOTAL_MIN_SEL, 0,
+				OTG_V_TOTAL_MAX_SEL, 0,
+				OTG_FORCE_LOCK_ON_EVENT, 0);
+
+		optc->funcs->set_vtotal_min_max(optc, 0, 0);
+	}
 }
 
 void optc1_set_vtotal_min_max(struct timing_generator *optc, int vtotal_min, int vtotal_max)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
index 58bdbd859bf9..d6f095b4555d 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
@@ -462,16 +462,6 @@ void optc2_setup_manual_trigger(struct timing_generator *optc)
 {
 	struct optc *optc1 = DCN10TG_FROM_TG(optc);
 
-	/* Set the min/max selectors unconditionally so that
-	 * DMCUB fw may change OTG timings when necessary
-	 * TODO: Remove the w/a after fixing the issue in DMCUB firmware
-	 */
-	REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
-				 OTG_V_TOTAL_MIN_SEL, 1,
-				 OTG_V_TOTAL_MAX_SEL, 1,
-				 OTG_FORCE_LOCK_ON_EVENT, 0,
-				 OTG_SET_V_TOTAL_MIN_MASK, (1 << 1)); /* TRIGA */
-
 	REG_SET_8(OTG_TRIGA_CNTL, 0,
 			OTG_TRIGA_SOURCE_SELECT, 21,
 			OTG_TRIGA_SOURCE_PIPE_SELECT, optc->inst,
-- 
2.40.1

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[André Almeida]

Hi Guilherme,

Em 02/07/2023 13:44, Guilherme G. Piccoli escreveu:
> This reverts commit 06c3a652a787efc960af7c8816036d25c4227c6c.
> 
> After this commit, the Steam Deck cannot boot with graphics anymore;
> the following message is observed on dmesg:
> 
> "[drm] ERROR [CRTC:67:crtc-0] flip_done timed out"
> 
> No other error is observed, it just stays like that. After bisecting
> amd-staging-drm-next, we narrowed it down to this commit. Seems it
> makes sense to revert it to have the tree bootable until a proper
> solution is worked.

Thank you for your patch, I confirm the exactly same behavior on my 
Steam Deck.

> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 

Reviewed-by: André Almeida <andrealmeid@igalia.com>

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Aurabindo Pillai]

[-- Attachment #1: Type: text/plain, Size: 3622 bytes --]



On 7/2/23 12:44, Guilherme G. Piccoli wrote:
> This reverts commit 06c3a652a787efc960af7c8816036d25c4227c6c.
> 
> After this commit, the Steam Deck cannot boot with graphics anymore;
> the following message is observed on dmesg:
> 
> "[drm] ERROR [CRTC:67:crtc-0] flip_done timed out"
> 
> No other error is observed, it just stays like that. After bisecting
> amd-staging-drm-next, we narrowed it down to this commit. Seems it
> makes sense to revert it to have the tree bootable until a proper
> solution is worked.
> 
> Cc: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Cc: André Almeida <andrealmeid@igalia.com>
> Cc: Melissa Wen <mwen@igalia.com>
> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> 
> ---
> 
> Hi Alex / Aurabindo, we couldn't boot the Deck with in HEAD
> (amd-staging-drm-next), git bisect led to this commit. Since its
> description already mentions a potential proper solution, related
> to the DMCUB (and some complex state tracking), I thought it was
> more effective to revert it to allow booting the tree in Deck (and
> maybe other HW - I just tested the Deck BTW).
> Lemme know your thoughts.
> 
> Special thanks to André and Melissa for helping the debug / bisect!
> We're open to test alternative patches, feel free to ping.
> Cheers,
> 
> Guilherme
> 
> 
>  drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c | 15 ++++++++++++---
>  drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c | 10 ----------
>  2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
> index 0e8f4f36c87c..27419cd98264 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_optc.c
> @@ -945,10 +945,19 @@ void optc1_set_drr(
>  				OTG_FORCE_LOCK_ON_EVENT, 0,
>  				OTG_SET_V_TOTAL_MIN_MASK_EN, 0,
>  				OTG_SET_V_TOTAL_MIN_MASK, 0);
> -	}
>  
> -	// Setup manual flow control for EOF via TRIG_A
> -	optc->funcs->setup_manual_trigger(optc);
> +		// Setup manual flow control for EOF via TRIG_A
> +		optc->funcs->setup_manual_trigger(optc);
> +
> +	} else {
> +		REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
> +				OTG_SET_V_TOTAL_MIN_MASK, 0,
> +				OTG_V_TOTAL_MIN_SEL, 0,
> +				OTG_V_TOTAL_MAX_SEL, 0,
> +				OTG_FORCE_LOCK_ON_EVENT, 0);
> +
> +		optc->funcs->set_vtotal_min_max(optc, 0, 0);
> +	}
>  }
>  
>  void optc1_set_vtotal_min_max(struct timing_generator *optc, int vtotal_min, int vtotal_max)
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
> index 58bdbd859bf9..d6f095b4555d 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_optc.c
> @@ -462,16 +462,6 @@ void optc2_setup_manual_trigger(struct timing_generator *optc)
>  {
>  	struct optc *optc1 = DCN10TG_FROM_TG(optc);
>  
> -	/* Set the min/max selectors unconditionally so that
> -	 * DMCUB fw may change OTG timings when necessary
> -	 * TODO: Remove the w/a after fixing the issue in DMCUB firmware
> -	 */
> -	REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
> -				 OTG_V_TOTAL_MIN_SEL, 1,
> -				 OTG_V_TOTAL_MAX_SEL, 1,
> -				 OTG_FORCE_LOCK_ON_EVENT, 0,
> -				 OTG_SET_V_TOTAL_MIN_MASK, (1 << 1)); /* TRIGA */
> -
>  	REG_SET_8(OTG_TRIGA_CNTL, 0,
>  			OTG_TRIGA_SOURCE_SELECT, 21,
>  			OTG_TRIGA_SOURCE_PIPE_SELECT, optc->inst,


Hi,

Sorry for the delayed response, this patch went unnoticed. This revert would break asics. Could you try the attached patch without reverting this one ?

[-- Attachment #2: 0002-drm-amd-display-add-DCN301-specific-logic-for-OTG-pr.patch --]
[-- Type: text/x-patch, Size: 10267 bytes --]

From 9ac8b837900ac242fcc9e948c8de7aca51a5be7e Mon Sep 17 00:00:00 2001
From: Aurabindo Pillai <aurabindo.pillai@amd.com>
Date: Tue, 11 Jul 2023 14:16:27 -0400
Subject: [PATCH 2/2] drm/amd/display: add DCN301 specific logic for OTG
 programming

[Why&How]
DCN301 does not have FAMS hence the workaround needed on other DCN3x
variants related to OTG min/max selector programming is not applicable for it.
Hence isolate it and have it use the old sequence without workaround.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 .../gpu/drm/amd/display/dc/dcn301/Makefile    |   3 +-
 .../drm/amd/display/dc/dcn301/dcn301_optc.c   | 185 ++++++++++++++++++
 .../drm/amd/display/dc/dcn301/dcn301_optc.h   |  36 ++++
 3 files changed, 223 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.c
 create mode 100644 drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.h

diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/Makefile b/drivers/gpu/drm/amd/display/dc/dcn301/Makefile
index 7aa628c21973..9002cb10a6ae 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn301/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/Makefile
@@ -11,7 +11,8 @@
 # Makefile for dcn30.
 
 DCN301 = dcn301_init.o dcn301_resource.o dcn301_dccg.o \
-		dcn301_dio_link_encoder.o dcn301_hwseq.o dcn301_panel_cntl.o dcn301_hubbub.o
+		dcn301_dio_link_encoder.o dcn301_hwseq.o dcn301_panel_cntl.o dcn301_hubbub.o \
+		dcn301_optc.o
 
 AMD_DAL_DCN301 = $(addprefix $(AMDDALPATH)/dc/dcn301/,$(DCN301))
 
diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.c b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.c
new file mode 100644
index 000000000000..b3cfcb887905
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.c
@@ -0,0 +1,185 @@
+/*
+ * Copyright 2020 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#include "reg_helper.h"
+#include "dcn301_optc.h"
+#include "dc.h"
+#include "dcn_calc_math.h"
+#include "dc_dmub_srv.h"
+
+#include "dml/dcn30/dcn30_fpu.h"
+#include "dc_trace.h"
+
+#define REG(reg)\
+	optc1->tg_regs->reg
+
+#define CTX \
+	optc1->base.ctx
+
+#undef FN
+#define FN(reg_name, field_name) \
+	optc1->tg_shift->field_name, optc1->tg_mask->field_name
+
+
+/**
+ * optc301_set_drr() - Program dynamic refresh rate registers m_OTGx_OTG_V_TOTAL_*.
+ *
+ * @optc: timing_generator instance.
+ * @params: parameters used for Dynamic Refresh Rate.
+ */
+void optc301_set_drr(
+	struct timing_generator *optc,
+	const struct drr_params *params)
+{
+	struct optc *optc1 = DCN10TG_FROM_TG(optc);
+
+	if (params != NULL &&
+		params->vertical_total_max > 0 &&
+		params->vertical_total_min > 0) {
+
+		if (params->vertical_total_mid != 0) {
+
+			REG_SET(OTG_V_TOTAL_MID, 0,
+				OTG_V_TOTAL_MID, params->vertical_total_mid - 1);
+
+			REG_UPDATE_2(OTG_V_TOTAL_CONTROL,
+					OTG_VTOTAL_MID_REPLACING_MAX_EN, 1,
+					OTG_VTOTAL_MID_FRAME_NUM,
+					(uint8_t)params->vertical_total_mid_frame_num);
+
+		}
+
+		optc->funcs->set_vtotal_min_max(optc, params->vertical_total_min - 1, params->vertical_total_max - 1);
+
+		REG_UPDATE_5(OTG_V_TOTAL_CONTROL,
+				OTG_V_TOTAL_MIN_SEL, 1,
+				OTG_V_TOTAL_MAX_SEL, 1,
+				OTG_FORCE_LOCK_ON_EVENT, 0,
+				OTG_SET_V_TOTAL_MIN_MASK_EN, 0,
+				OTG_SET_V_TOTAL_MIN_MASK, 0);
+		// Setup manual flow control for EOF via TRIG_A
+		optc->funcs->setup_manual_trigger(optc);
+
+	} else {
+		REG_UPDATE_4(OTG_V_TOTAL_CONTROL,
+				OTG_SET_V_TOTAL_MIN_MASK, 0,
+				OTG_V_TOTAL_MIN_SEL, 0,
+				OTG_V_TOTAL_MAX_SEL, 0,
+				OTG_FORCE_LOCK_ON_EVENT, 0);
+
+		optc->funcs->set_vtotal_min_max(optc, 0, 0);
+	}
+}
+
+
+void optc301_setup_manual_trigger(struct timing_generator *optc)
+{
+	struct optc *optc1 = DCN10TG_FROM_TG(optc);
+
+	REG_SET_8(OTG_TRIGA_CNTL, 0,
+			OTG_TRIGA_SOURCE_SELECT, 21,
+			OTG_TRIGA_SOURCE_PIPE_SELECT, optc->inst,
+			OTG_TRIGA_RISING_EDGE_DETECT_CNTL, 1,
+			OTG_TRIGA_FALLING_EDGE_DETECT_CNTL, 0,
+			OTG_TRIGA_POLARITY_SELECT, 0,
+			OTG_TRIGA_FREQUENCY_SELECT, 0,
+			OTG_TRIGA_DELAY, 0,
+			OTG_TRIGA_CLEAR, 1);
+}
+
+static struct timing_generator_funcs dcn30_tg_funcs = {
+		.validate_timing = optc1_validate_timing,
+		.program_timing = optc1_program_timing,
+		.setup_vertical_interrupt0 = optc1_setup_vertical_interrupt0,
+		.setup_vertical_interrupt1 = optc1_setup_vertical_interrupt1,
+		.setup_vertical_interrupt2 = optc1_setup_vertical_interrupt2,
+		.program_global_sync = optc1_program_global_sync,
+		.enable_crtc = optc2_enable_crtc,
+		.disable_crtc = optc1_disable_crtc,
+		/* used by enable_timing_synchronization. Not need for FPGA */
+		.is_counter_moving = optc1_is_counter_moving,
+		.get_position = optc1_get_position,
+		.get_frame_count = optc1_get_vblank_counter,
+		.get_scanoutpos = optc1_get_crtc_scanoutpos,
+		.get_otg_active_size = optc1_get_otg_active_size,
+		.set_early_control = optc1_set_early_control,
+		/* used by enable_timing_synchronization. Not need for FPGA */
+		.wait_for_state = optc1_wait_for_state,
+		.set_blank_color = optc3_program_blank_color,
+		.did_triggered_reset_occur = optc1_did_triggered_reset_occur,
+		.triplebuffer_lock = optc3_triplebuffer_lock,
+		.triplebuffer_unlock = optc2_triplebuffer_unlock,
+		.enable_reset_trigger = optc1_enable_reset_trigger,
+		.enable_crtc_reset = optc1_enable_crtc_reset,
+		.disable_reset_trigger = optc1_disable_reset_trigger,
+		.lock = optc3_lock,
+		.unlock = optc1_unlock,
+		.lock_doublebuffer_enable = optc3_lock_doublebuffer_enable,
+		.lock_doublebuffer_disable = optc3_lock_doublebuffer_disable,
+		.enable_optc_clock = optc1_enable_optc_clock,
+		.set_drr = optc301_set_drr,
+		.get_last_used_drr_vtotal = optc2_get_last_used_drr_vtotal,
+		.set_vtotal_min_max = optc3_set_vtotal_min_max,
+		.set_static_screen_control = optc1_set_static_screen_control,
+		.program_stereo = optc1_program_stereo,
+		.is_stereo_left_eye = optc1_is_stereo_left_eye,
+		.tg_init = optc3_tg_init,
+		.is_tg_enabled = optc1_is_tg_enabled,
+		.is_optc_underflow_occurred = optc1_is_optc_underflow_occurred,
+		.clear_optc_underflow = optc1_clear_optc_underflow,
+		.setup_global_swap_lock = NULL,
+		.get_crc = optc1_get_crc,
+		.configure_crc = optc2_configure_crc,
+		.set_dsc_config = optc3_set_dsc_config,
+		.get_dsc_status = optc2_get_dsc_status,
+		.set_dwb_source = NULL,
+		.set_odm_bypass = optc3_set_odm_bypass,
+		.set_odm_combine = optc3_set_odm_combine,
+		.get_optc_source = optc2_get_optc_source,
+		.set_out_mux = optc3_set_out_mux,
+		.set_drr_trigger_window = optc3_set_drr_trigger_window,
+		.set_vtotal_change_limit = optc3_set_vtotal_change_limit,
+		.set_gsl = optc2_set_gsl,
+		.set_gsl_source_select = optc2_set_gsl_source_select,
+		.set_vtg_params = optc1_set_vtg_params,
+		.program_manual_trigger = optc2_program_manual_trigger,
+		.setup_manual_trigger = optc301_setup_manual_trigger,
+		.get_hw_timing = optc1_get_hw_timing,
+		.wait_drr_doublebuffer_pending_clear = optc3_wait_drr_doublebuffer_pending_clear,
+};
+
+void dcn301_timing_generator_init(struct optc *optc1)
+{
+	optc1->base.funcs = &dcn30_tg_funcs;
+
+	optc1->max_h_total = optc1->tg_mask->OTG_H_TOTAL + 1;
+	optc1->max_v_total = optc1->tg_mask->OTG_V_TOTAL + 1;
+
+	optc1->min_h_blank = 32;
+	optc1->min_v_blank = 3;
+	optc1->min_v_blank_interlace = 5;
+	optc1->min_h_sync_width = 4;
+	optc1->min_v_sync_width = 1;
+}
diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.h b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.h
new file mode 100644
index 000000000000..b49585682a15
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_optc.h
@@ -0,0 +1,36 @@
+/*
+ * Copyright 2020 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#ifndef __DC_OPTC_DCN301_H__
+#define __DC_OPTC_DCN301_H__
+
+#include "dcn20/dcn20_optc.h"
+#include "dcn30/dcn30_optc.h"
+
+void dcn301_timing_generator_init(struct optc *optc1);
+void optc301_setup_manual_trigger(struct timing_generator *optc);
+void optc301_set_drr(struct timing_generator *optc, const struct drr_params *params);
+
+#endif /* __DC_OPTC_DCN301_H__ */
-- 
2.41.0


[-- Attachment #3: 0001-drm-amd-display-export-some-optc-function-for-reuse.patch --]
[-- Type: text/x-patch, Size: 2496 bytes --]

From ffd30f5fda1fc61b98b7e416d8c74daf59e0eb62 Mon Sep 17 00:00:00 2001
From: Aurabindo Pillai <aurabindo.pillai@amd.com>
Date: Tue, 11 Jul 2023 14:14:43 -0400
Subject: [PATCH 1/2] drm/amd/display: export some optc function for reuse

[Why&How]
Make a few functions non static so that they can be reused for other
asic. This is in preparation for separating out OTG programming sequence
for DCN301

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.c | 4 ++--
 drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.c b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.c
index dfb8f62765f2..5bf4d0aa6230 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.c
@@ -215,7 +215,7 @@ void optc3_set_odm_bypass(struct timing_generator *optc,
 	optc1->opp_count = 1;
 }
 
-static void optc3_set_odm_combine(struct timing_generator *optc, int *opp_id, int opp_cnt,
+void optc3_set_odm_combine(struct timing_generator *optc, int *opp_id, int opp_cnt,
 		struct dc_crtc_timing *timing)
 {
 	struct optc *optc1 = DCN10TG_FROM_TG(optc);
@@ -293,7 +293,7 @@ static void optc3_set_timing_double_buffer(struct timing_generator *optc, bool e
 		   OTG_DRR_TIMING_DBUF_UPDATE_MODE, mode);
 }
 
-static void optc3_wait_drr_doublebuffer_pending_clear(struct timing_generator *optc)
+void optc3_wait_drr_doublebuffer_pending_clear(struct timing_generator *optc)
 {
 	struct optc *optc1 = DCN10TG_FROM_TG(optc);
 
diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
index fb06dc9a4893..d3a056c12b0d 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_optc.h
@@ -351,6 +351,9 @@ void optc3_set_timing_db_mode(struct timing_generator *optc, bool enable);
 
 void optc3_set_odm_bypass(struct timing_generator *optc,
 		const struct dc_crtc_timing *dc_crtc_timing);
+void optc3_set_odm_combine(struct timing_generator *optc, int *opp_id, int opp_cnt,
+		struct dc_crtc_timing *timing);
+void optc3_wait_drr_doublebuffer_pending_clear(struct timing_generator *optc);
 void optc3_tg_init(struct timing_generator *optc);
 void optc3_set_vtotal_min_max(struct timing_generator *optc, int vtotal_min, int vtotal_max);
 #endif /* __DC_OPTC_DCN30_H__ */
-- 
2.41.0

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Guilherme G. Piccoli]

On 11/07/2023 15:22, Aurabindo Pillai wrote:
> [...]
> Hi,
> 
> Sorry for the delayed response, this patch went unnoticed. This revert would break asics. Could you try the attached patch without reverting this one ?

Hi Aurabindo, thanks for your response!

I've tried kernel 6.5-rc1, and it seems the issue is present, due to the
patch being merged on Linus tree [as 1598fc576420 ("drm/amd/display:
Program OTG vtotal min/max selectors unconditionally for DCN1+")].

Then, I tried both your attached patches on top of that, and
unfortunately, the behavior is the same: Steam Deck doesn't boot with
graphics, and we can see the single error "amdgpu 0000:04:00.0: [drm]
*ERROR* [CRTC:67:crtc-0] flip_done timed out" on dmesg.

Do you / Alex think we could get this revert for 6.5-rc2, so at least we
could boot mainline there while the issue is handled? It would be an
intermediate fix. You mentioned it breaks some asics, but did they work
until now, without your patch?

Thanks,


Guilherme

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Pillai, Aurabindo]

[-- Attachment #1.1: Type: text/plain, Size: 2244 bytes --]

[Public]

Hi Guilherme,

Sorry there was one more patch which I missed to attach. Please add this 3rd patch and retry.

Reverting that patch would cause high power consumption on Navi2x GPU also cause hangs on certain multi monitor configurations. With these 3 patches, you're getting the same effect as reverting the aforementioned patches, but it makes the reverted sequence available only for Steam deck hardware.

--

Regards,
Jay
________________________________
From: Guilherme G. Piccoli <gpiccoli@igalia.com>
Sent: Tuesday, July 11, 2023 7:15 PM
To: Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; kernel-dev@igalia.com <kernel-dev@igalia.com>; cristian.ciocaltea@collabora.com <cristian.ciocaltea@collabora.com>; André Almeida <andrealmeid@igalia.com>; Melissa Wen <mwen@igalia.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>
Subject: Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

On 11/07/2023 15:22, Aurabindo Pillai wrote:
> [...]
> Hi,
>
> Sorry for the delayed response, this patch went unnoticed. This revert would break asics. Could you try the attached patch without reverting this one ?

Hi Aurabindo, thanks for your response!

I've tried kernel 6.5-rc1, and it seems the issue is present, due to the
patch being merged on Linus tree [as 1598fc576420 ("drm/amd/display:
Program OTG vtotal min/max selectors unconditionally for DCN1+")].

Then, I tried both your attached patches on top of that, and
unfortunately, the behavior is the same: Steam Deck doesn't boot with
graphics, and we can see the single error "amdgpu 0000:04:00.0: [drm]
*ERROR* [CRTC:67:crtc-0] flip_done timed out" on dmesg.

Do you / Alex think we could get this revert for 6.5-rc2, so at least we
could boot mainline there while the issue is handled? It would be an
intermediate fix. You mentioned it breaks some asics, but did they work
until now, without your patch?

Thanks,


Guilherme

[-- Attachment #1.2: Type: text/html, Size: 4710 bytes --]

[-- Attachment #2: 0001-drm-amd-display-switch-to-DCN301-specific-TG-init.patch --]
[-- Type: application/octet-stream, Size: 1255 bytes --]

From e99069f1b3dc949b5381182fc8b4e0edb551adcb Mon Sep 17 00:00:00 2001
From: Aurabindo Pillai <aurabindo.pillai@amd.com>
Date: Wed, 12 Jul 2023 10:43:11 -0400
Subject: [PATCH] drm/amd/display: switch to DCN301 specific TG init

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
index 3485fbb1093e..1bee9a4636e6 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
@@ -42,7 +42,7 @@
 #include "dcn30/dcn30_hubp.h"
 #include "irq/dcn30/irq_service_dcn30.h"
 #include "dcn30/dcn30_dpp.h"
-#include "dcn30/dcn30_optc.h"
+#include "dcn301/dcn301_optc.h"
 #include "dcn20/dcn20_hwseq.h"
 #include "dcn30/dcn30_hwseq.h"
 #include "dce110/dce110_hw_sequencer.h"
@@ -855,7 +855,7 @@ static struct timing_generator *dcn301_timing_generator_create(
 	tgn10->tg_shift = &optc_shift;
 	tgn10->tg_mask = &optc_mask;
 
-	dcn30_timing_generator_init(tgn10);
+	dcn301_timing_generator_init(tgn10);
 
 	return &tgn10->base;
 }
-- 
2.41.0

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Guilherme G. Piccoli]

On 12/07/2023 11:47, Pillai, Aurabindo wrote:
> Hi Guilherme,
> 
> Sorry there was one more patch which I missed to attach. Please add this
> 3^rd  patch and retry.
> 
> Reverting that patch would cause high power consumption on Navi2x GPU
> also cause hangs on certain multi monitor configurations. With these 3
> patches, you're getting the same effect as reverting the aforementioned
> patches, but it makes the reverted sequence available only for Steam
> deck hardware.
> 

Thanks a lot for your detailed explanation, and the 3rd patch! Indeed,
amdgpu works fine on Deck with that - great =)

Feel free to add my:
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com> #Steam Deck

Oh, a fixes tag would also makes sense, I guess.
BTW, if possible to submit the 3 patches in a proper series to get it
merged on 6.5-rc cycle (the sooner the better), I'd really appreciate!

Cheers,


Guilherme

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Aurabindo Pillai]

On 7/12/2023 11:50 AM, Guilherme G. Piccoli wrote:
> On 12/07/2023 11:47, Pillai, Aurabindo wrote:
>> Hi Guilherme,
>>
>> Sorry there was one more patch which I missed to attach. Please add this
>> 3^rd  patch and retry.
>>
>> Reverting that patch would cause high power consumption on Navi2x GPU
>> also cause hangs on certain multi monitor configurations. With these 3
>> patches, you're getting the same effect as reverting the aforementioned
>> patches, but it makes the reverted sequence available only for Steam
>> deck hardware.
>>
> 
> Thanks a lot for your detailed explanation, and the 3rd patch! Indeed,
> amdgpu works fine on Deck with that - great =)
> 
> Feel free to add my:
> Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com> #Steam Deck
> 
> Oh, a fixes tag would also makes sense, I guess.
> BTW, if possible to submit the 3 patches in a proper series to get it
> merged on 6.5-rc cycle (the sooner the better), I'd really appreciate!
> 

Thanks for confirmation! I'll add the fixes tag so that it gets picked up.

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 2809 bytes --]

On 07/12, Pillai, Aurabindo wrote:
> [Public]
> 
> Hi Guilherme,
> 
> Sorry there was one more patch which I missed to attach. Please add this 3rd patch and retry.
> 
> Reverting that patch would cause high power consumption on Navi2x GPU also cause hangs on certain multi monitor configurations. With these 3 patches, you're getting the same effect as reverting the aforementioned patches, but it makes the reverted sequence available only for Steam deck hardware.
> 

Hi Jay,

Thanks for looking at this issue.

You mention power consumption and multi-monitor configuration issues
that can affect a driver if we revert this OTG change, and both sounds
quite relevant to me. Can they not affect DCN301 too? Is there something
that needs further work so the DCN301 can benefit from this improvement
as well?

Also, let us know if we can contribute in any way.

Best Regards,

Melissa


> --
> 
> Regards,
> Jay
> ________________________________
> From: Guilherme G. Piccoli <gpiccoli@igalia.com>
> Sent: Tuesday, July 11, 2023 7:15 PM
> To: Pillai, Aurabindo <Aurabindo.Pillai@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; kernel-dev@igalia.com <kernel-dev@igalia.com>; cristian.ciocaltea@collabora.com <cristian.ciocaltea@collabora.com>; André Almeida <andrealmeid@igalia.com>; Melissa Wen <mwen@igalia.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>
> Subject: Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"
> 
> On 11/07/2023 15:22, Aurabindo Pillai wrote:
> > [...]
> > Hi,
> >
> > Sorry for the delayed response, this patch went unnoticed. This revert would break asics. Could you try the attached patch without reverting this one ?
> 
> Hi Aurabindo, thanks for your response!
> 
> I've tried kernel 6.5-rc1, and it seems the issue is present, due to the
> patch being merged on Linus tree [as 1598fc576420 ("drm/amd/display:
> Program OTG vtotal min/max selectors unconditionally for DCN1+")].
> 
> Then, I tried both your attached patches on top of that, and
> unfortunately, the behavior is the same: Steam Deck doesn't boot with
> graphics, and we can see the single error "amdgpu 0000:04:00.0: [drm]
> *ERROR* [CRTC:67:crtc-0] flip_done timed out" on dmesg.
> 
> Do you / Alex think we could get this revert for 6.5-rc2, so at least we
> could boot mainline there while the issue is handled? It would be an
> intermediate fix. You mentioned it breaks some asics, but did they work
> until now, without your patch?
> 
> Thanks,
> 
> 
> Guilherme



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Aurabindo Pillai]

On 7/12/2023 12:24 PM, Melissa Wen wrote:
> On 07/12, Pillai, Aurabindo wrote:
>> [Public]
>>
>> Hi Guilherme,
>>
>> Sorry there was one more patch which I missed to attach. Please add this 3rd patch and retry.
>>
>> Reverting that patch would cause high power consumption on Navi2x GPU also cause hangs on certain multi monitor configurations. With these 3 patches, you're getting the same effect as reverting the aforementioned patches, but it makes the reverted sequence available only for Steam deck hardware.
>>
> 
> Hi Jay,
> 
> Thanks for looking at this issue.
> 
> You mention power consumption and multi-monitor configuration issues
> that can affect a driver if we revert this OTG change, and both sounds
> quite relevant to me. Can they not affect DCN301 too? Is there something
> that needs further work so the DCN301 can benefit from this improvement
> as well?
> 
> Also, let us know if we can contribute in any way.
> 

Hi Melissa,

Unfortunately, DCN301 does not support Firmware Assisted Memory Clock 
Switching, which is the feature that gets blocked on Navi2x if we revert 
the patch in question.  This is the feature that enables lower power 
consumption on some multi monitor configurations and high refresh rate 
single monitor configurations.

Navi2x is configured to use FAMS in the driver, but without this change, 
firmware wont be able to actually enable the feature in DMCUB, which 
puts the driver in a unexpected state. On DCN301, this unexpected state 
will not occur, because there is no FAMS support in driver nor firmware.

--
Jay

Re: [PATCH] Revert "drm/amd/display: Program OTG vtotal min/max selectors unconditionally for DCN1+"

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 1856 bytes --]

On 07/12, Aurabindo Pillai wrote:
> 
> 
> On 7/12/2023 12:24 PM, Melissa Wen wrote:
> > On 07/12, Pillai, Aurabindo wrote:
> > > [Public]
> > > 
> > > Hi Guilherme,
> > > 
> > > Sorry there was one more patch which I missed to attach. Please add this 3rd patch and retry.
> > > 
> > > Reverting that patch would cause high power consumption on Navi2x GPU also cause hangs on certain multi monitor configurations. With these 3 patches, you're getting the same effect as reverting the aforementioned patches, but it makes the reverted sequence available only for Steam deck hardware.
> > > 
> > 
> > Hi Jay,
> > 
> > Thanks for looking at this issue.
> > 
> > You mention power consumption and multi-monitor configuration issues
> > that can affect a driver if we revert this OTG change, and both sounds
> > quite relevant to me. Can they not affect DCN301 too? Is there something
> > that needs further work so the DCN301 can benefit from this improvement
> > as well?
> > 
> > Also, let us know if we can contribute in any way.
> > 
> 
> Hi Melissa,
> 
> Unfortunately, DCN301 does not support Firmware Assisted Memory Clock
> Switching, which is the feature that gets blocked on Navi2x if we revert the
> patch in question.  This is the feature that enables lower power consumption
> on some multi monitor configurations and high refresh rate single monitor
> configurations.
> 
> Navi2x is configured to use FAMS in the driver, but without this change,
> firmware wont be able to actually enable the feature in DMCUB, which puts
> the driver in a unexpected state. On DCN301, this unexpected state will not
> occur, because there is no FAMS support in driver nor firmware.

Oh, got it. Sounds fine.
Many thanks for explaning the context of this change.

Best Regards,

Melissa

> 
> --
> Jay
> 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]