[PATCH 00/18] DC Patches for Dec 19, 2022

[PATCH 00/18] DC Patches for Dec 19, 2022

[Aurabindo Pillai]

This DC patchset brings improvements in multiple areas. In summary, we have:

* Fixes for various features like SubVP, ABM, HDCP, Secure display
* Fix a stability issue when running IGT test suite
* Improvements for eDP panels

-----

Alan Liu (1):
  drm/amd/display: Improvements in secure display

Alex Hung (1):
  drm/amd/display: Use mdelay to avoid crashes

Aric Cyr (2):
  drm/amd/display: Reorder dc_state fields to optimize clearing the
    struct
  drm/amd/display: 3.2.217

Aurabindo Pillai (1):
  drm/amd/display: set ignore msa parameter only if freesync is enabled

Dmytro Laktyushkin (1):
  drm/amd/display: fix dc_get_edp_link_panel_inst to only consider links
    with panels

Lee, Alvin (1):
  drm/amd/display: Turn on phantom OTG before disabling phantom pipe

Leo Chen (1):
  drm/amd/display: Adding braces to prepare for future changes to
    behavior of if block

Leon Huang (2):
  drm/amd/display: Refactor ABM code flow
  drm/amd/display: Fix crash when setting ABM pipe/backlight

Nicholas Kazlauskas (1):
  drm/amd/display: Defer DIG FIFO disable after VID stream enable

Samson Tam (1):
  drm/amd/display: Uninitialized variables causing 4k60 UCLK to stay at
    DPM1 and not DPM0

Swapnil Patel (1):
  drm/amd/display: patch cases with unknown plane state to prevent
    warning

Wenjing Liu (3):
  drm/amd/display: move dccg programming from link hwss hpo dp to hwss
  drm/amd/display: update pixel rate div in enable stream
  drm/amd/display: allow hpo and dio encoder switching during dp retrain
    test

hersen wu (2):
  drm/amd/display: save restore hdcp state when display is unplugged
    from mst hub
  drm/amd/display: phase3 mst hdcp for multiple displays

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 219 ++++++++++++---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c |   6 +
 .../amd/display/amdgpu_dm/amdgpu_dm_hdcp.h    |  14 +
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  26 ++
 drivers/gpu/drm/amd/display/dc/core/dc.c      |  61 ++---
 drivers/gpu/drm/amd/display/dc/core/dc_link.c |   3 -
 .../gpu/drm/amd/display/dc/core/dc_link_dp.c  |  64 ++---
 .../gpu/drm/amd/display/dc/core/dc_resource.c |  39 +++
 drivers/gpu/drm/amd/display/dc/dc.h           |   6 +-
 drivers/gpu/drm/amd/display/dc/dc_link.h      |  15 +-
 drivers/gpu/drm/amd/display/dc/dce/Makefile   |   3 +-
 drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 249 +++++++----------
 .../gpu/drm/amd/display/dc/dce/dmub_abm_lcd.c | 259 ++++++++++++++++++
 .../gpu/drm/amd/display/dc/dce/dmub_abm_lcd.h |  45 +++
 .../display/dc/dce110/dce110_hw_sequencer.c   |  15 +-
 .../drm/amd/display/dc/dcn20/dcn20_hwseq.c    |  86 ++++++
 .../drm/amd/display/dc/dcn21/dcn21_hwseq.c    |  69 +++--
 .../amd/display/dc/dcn301/dcn301_resource.c   |   3 +-
 .../dc/dcn314/dcn314_dio_stream_encoder.c     |   6 +-
 .../drm/amd/display/dc/dcn32/dcn32_hwseq.c    |  36 +++
 .../drm/amd/display/dc/dcn32/dcn32_hwseq.h    |   2 +
 .../gpu/drm/amd/display/dc/dcn32/dcn32_init.c |   1 +
 .../dc/dml/dcn32/display_mode_vba_util_32.c   |   6 +-
 .../gpu/drm/amd/display/dc/inc/core_types.h   |  18 +-
 drivers/gpu/drm/amd/display/dc/inc/hw/abm.h   |   6 +
 .../gpu/drm/amd/display/dc/inc/hw_sequencer.h |   1 +
 drivers/gpu/drm/amd/display/dc/inc/resource.h |   9 +
 .../amd/display/dc/link/link_hwss_hpo_dp.c    |  37 ---
 28 files changed, 946 insertions(+), 358 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.c
 create mode 100644 drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.h

-- 
2.39.0

[PATCH 01/18] drm/amd/display: save restore hdcp state when display is unplugged from mst hub

[Aurabindo Pillai]

From: hersen wu <hersenxs.wu@amd.com>

[Why]
connector hdcp properties are lost after display is
unplgged from mst hub. connector is destroyed with
dm_dp_mst_connector_destroy. when display is plugged
back, hdcp is not desired and it wouldnt be enabled.

[How]
save hdcp properties into hdcp_work within
amdgpu_dm_atomic_commit_tail. If the same display is
plugged back with same display index, its hdcp
properties will be retrieved from hdcp_work within
dm_dp_mst_get_modes.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: hersen wu <hersenxs.wu@amd.com>
Reviewed-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 ++++++++++++++++-
 .../amd/display/amdgpu_dm/amdgpu_dm_hdcp.h    | 14 ++++++++++
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 26 +++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a60b3729752a..acd78fde6121 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8317,11 +8317,33 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			continue;
 		}
 
-		if (is_content_protection_different(new_con_state, old_con_state, connector, adev->dm.hdcp_workqueue))
+		if (is_content_protection_different(new_con_state, old_con_state, connector, adev->dm.hdcp_workqueue)) {
+			/* when display is unplugged from mst hub, connctor will
+			 * be destroyed within dm_dp_mst_connector_destroy. connector
+			 * hdcp perperties, like type, undesired, desired, enabled,
+			 * will be lost. So, save hdcp properties into hdcp_work within
+			 * amdgpu_dm_atomic_commit_tail. if the same display is
+			 * plugged back with same display index, its hdcp properties
+			 * will be retrieved from hdcp_work within dm_dp_mst_get_modes
+			 */
+
+			if (aconnector->dc_link && aconnector->dc_sink &&
+				aconnector->dc_link->type == dc_connection_mst_branch) {
+				struct hdcp_workqueue *hdcp_work = adev->dm.hdcp_workqueue;
+				struct hdcp_workqueue *hdcp_w =
+					&hdcp_work[aconnector->dc_link->link_index];
+
+				hdcp_w->hdcp_content_type[connector->index] =
+					new_con_state->hdcp_content_type;
+				hdcp_w->content_protection[connector->index] =
+					new_con_state->content_protection;
+			}
+
 			hdcp_update_display(
 				adev->dm.hdcp_workqueue, aconnector->dc_link->link_index, aconnector,
 				new_con_state->hdcp_content_type,
 				new_con_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED);
+    }
 	}
 #endif
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h
index 09294ff122fe..bbbf7d0eff82 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_hdcp.h
@@ -52,6 +52,20 @@ struct hdcp_workqueue {
 	struct mod_hdcp_link link;
 
 	enum mod_hdcp_encryption_status encryption_status;
+
+	/* when display is unplugged from mst hub, connctor will be
+	 * destroyed within dm_dp_mst_connector_destroy. connector
+	 * hdcp perperties, like type, undesired, desired, enabled,
+	 * will be lost. So, save hdcp properties into hdcp_work within
+	 * amdgpu_dm_atomic_commit_tail. if the same display is
+	 * plugged back with same display index, its hdcp properties
+	 * will be retrieved from hdcp_work within dm_dp_mst_get_modes
+	 */
+	/* un-desired, desired, enabled */
+	unsigned int content_protection[AMDGPU_DM_MAX_DISPLAY_INDEX];
+	/* hdcp1.x, hdcp2.x */
+	unsigned int hdcp_content_type[AMDGPU_DM_MAX_DISPLAY_INDEX];
+
 	uint8_t max_link;
 
 	uint8_t *srm;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index eef71940e43d..ad02435a3bfd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -32,6 +32,10 @@
 #include "amdgpu_dm.h"
 #include "amdgpu_dm_mst_types.h"
 
+#ifdef CONFIG_DRM_AMD_DC_HDCP
+#include "amdgpu_dm_hdcp.h"
+#endif
+
 #include "dc.h"
 #include "dm_helpers.h"
 
@@ -344,6 +348,28 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 		/* dc_link_add_remote_sink returns a new reference */
 		aconnector->dc_sink = dc_sink;
 
+		/* when display is unplugged from mst hub, connctor will be
+		 * destroyed within dm_dp_mst_connector_destroy. connector
+		 * hdcp perperties, like type, undesired, desired, enabled,
+		 * will be lost. So, save hdcp properties into hdcp_work within
+		 * amdgpu_dm_atomic_commit_tail. if the same display is
+		 * plugged back with same display index, its hdcp properties
+		 * will be retrieved from hdcp_work within dm_dp_mst_get_modes
+		 */
+#ifdef CONFIG_DRM_AMD_DC_HDCP
+		if (aconnector->dc_sink && connector->state) {
+			struct drm_device *dev = connector->dev;
+			struct amdgpu_device *adev = drm_to_adev(dev);
+			struct hdcp_workqueue *hdcp_work = adev->dm.hdcp_workqueue;
+			struct hdcp_workqueue *hdcp_w = &hdcp_work[aconnector->dc_link->link_index];
+
+			connector->state->hdcp_content_type =
+			hdcp_w->hdcp_content_type[connector->index];
+			connector->state->content_protection =
+			hdcp_w->content_protection[connector->index];
+		}
+#endif
+
 		if (aconnector->dc_sink) {
 			amdgpu_dm_update_freesync_caps(
 					connector, aconnector->edid);
-- 
2.39.0

[PATCH 02/18] drm/amd/display: phase3 mst hdcp for multiple displays

[Aurabindo Pillai]

From: hersen wu <hersenxs.wu@amd.com>

[Why]
multiple display hdcp are enabled within event_property_validate,
event_property_update by looping all displays on mst hub. when
one of display on mst hub in unplugged or disabled, hdcp are
disabled for all displays on mst hub within hdcp_reset_display
by looping all displays of mst link. for displays still active,
their encryption status are off. kernel driver will not run hdcp
authentication again. therefore, hdcp are not enabled automatically.

[How]
within is_content_protection_different, check drm_crtc_state changes
of all displays on mst hub, if need, triger hdcp_update_display to
re-run hdcp authentication.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: hersen wu <hersenxs.wu@amd.com>
Reviewed-by: Bhawanpreet Lakha <Bhawanpreet.Lakha@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 183 ++++++++++++++----
 1 file changed, 142 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index acd78fde6121..24443547bd02 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7379,27 +7379,55 @@ is_scaling_state_different(const struct dm_connector_state *dm_state,
 }
 
 #ifdef CONFIG_DRM_AMD_DC_HDCP
-static bool is_content_protection_different(struct drm_connector_state *state,
-					    const struct drm_connector_state *old_state,
-					    const struct drm_connector *connector, struct hdcp_workqueue *hdcp_w)
+static bool is_content_protection_different(struct drm_crtc_state *new_crtc_state,
+					    struct drm_crtc_state *old_crtc_state,
+					    struct drm_connector_state *new_conn_state,
+					    struct drm_connector_state *old_conn_state,
+					    const struct drm_connector *connector,
+					    struct hdcp_workqueue *hdcp_w)
 {
 	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 	struct dm_connector_state *dm_con_state = to_dm_connector_state(connector->state);
 
-	/* Handle: Type0/1 change */
-	if (old_state->hdcp_content_type != state->hdcp_content_type &&
-	    state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
-		state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+	pr_debug("[HDCP_DM] connector->index: %x connect_status: %x dpms: %x\n",
+		connector->index, connector->status, connector->dpms);
+	pr_debug("[HDCP_DM] state protection old: %x new: %x\n",
+		old_conn_state->content_protection, new_conn_state->content_protection);
+
+	if (old_crtc_state)
+		pr_debug("[HDCP_DM] old crtc en: %x a: %x m: %x a-chg: %x c-chg: %x\n",
+		old_crtc_state->enable,
+		old_crtc_state->active,
+		old_crtc_state->mode_changed,
+		old_crtc_state->active_changed,
+		old_crtc_state->connectors_changed);
+
+	if (new_crtc_state)
+		pr_debug("[HDCP_DM] NEW crtc en: %x a: %x m: %x a-chg: %x c-chg: %x\n",
+		new_crtc_state->enable,
+		new_crtc_state->active,
+		new_crtc_state->mode_changed,
+		new_crtc_state->active_changed,
+		new_crtc_state->connectors_changed);
+
+	/* hdcp content type change */
+	if (old_conn_state->hdcp_content_type != new_conn_state->hdcp_content_type &&
+	    new_conn_state->content_protection != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
+		new_conn_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		pr_debug("[HDCP_DM] Type0/1 change %s :true\n", __func__);
 		return true;
 	}
 
-	/* CP is being re enabled, ignore this
-	 *
-	 * Handles:	ENABLED -> DESIRED
-	 */
-	if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
-	    state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
-		state->content_protection = DRM_MODE_CONTENT_PROTECTION_ENABLED;
+	/* CP is being re enabled, ignore this */
+	if (old_conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
+	    new_conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+		if (new_crtc_state && new_crtc_state->mode_changed) {
+			new_conn_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+			pr_debug("[HDCP_DM] ENABLED->DESIRED & mode_changed %s :true\n", __func__);
+			return true;
+		};
+		new_conn_state->content_protection = DRM_MODE_CONTENT_PROTECTION_ENABLED;
+		pr_debug("[HDCP_DM] ENABLED -> DESIRED %s :false\n", __func__);
 		return false;
 	}
 
@@ -7407,9 +7435,9 @@ static bool is_content_protection_different(struct drm_connector_state *state,
 	 *
 	 * Handles:	UNDESIRED -> ENABLED
 	 */
-	if (old_state->content_protection == DRM_MODE_CONTENT_PROTECTION_UNDESIRED &&
-	    state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED)
-		state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
+	if (old_conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_UNDESIRED &&
+	    new_conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_ENABLED)
+		new_conn_state->content_protection = DRM_MODE_CONTENT_PROTECTION_DESIRED;
 
 	/* Stream removed and re-enabled
 	 *
@@ -7419,10 +7447,12 @@ static bool is_content_protection_different(struct drm_connector_state *state,
 	 *
 	 * Handles:	DESIRED -> DESIRED (Special case)
 	 */
-	if (!(old_state->crtc && old_state->crtc->enabled) &&
-		state->crtc && state->crtc->enabled &&
+	if (!(old_conn_state->crtc && old_conn_state->crtc->enabled) &&
+		new_conn_state->crtc && new_conn_state->crtc->enabled &&
 		connector->state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
 		dm_con_state->update_hdcp = false;
+		pr_debug("[HDCP_DM] DESIRED->DESIRED (Stream removed and re-enabled) %s :true\n",
+			__func__);
 		return true;
 	}
 
@@ -7434,35 +7464,42 @@ static bool is_content_protection_different(struct drm_connector_state *state,
 	 *
 	 * Handles:	DESIRED -> DESIRED (Special case)
 	 */
-	if (dm_con_state->update_hdcp && state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
-	    connector->dpms == DRM_MODE_DPMS_ON && aconnector->dc_sink != NULL) {
+	if (dm_con_state->update_hdcp &&
+	new_conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
+	connector->dpms == DRM_MODE_DPMS_ON && aconnector->dc_sink != NULL) {
 		dm_con_state->update_hdcp = false;
+		pr_debug("[HDCP_DM] DESIRED->DESIRED (Hot-plug, headless s3, dpms) %s :true\n",
+			__func__);
 		return true;
 	}
 
-	/*
-	 * Handles:	UNDESIRED -> UNDESIRED
-	 *		DESIRED -> DESIRED
-	 *		ENABLED -> ENABLED
-	 */
-	if (old_state->content_protection == state->content_protection)
+	if (old_conn_state->content_protection == new_conn_state->content_protection) {
+		if (new_conn_state->content_protection >= DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+			if (new_crtc_state && new_crtc_state->mode_changed) {
+				pr_debug("[HDCP_DM] DESIRED->DESIRED or ENABLE->ENABLE mode_change %s :true\n",
+					__func__);
+				return true;
+			};
+			pr_debug("[HDCP_DM] DESIRED->DESIRED & ENABLE->ENABLE %s :false\n",
+				__func__);
+			return false;
+		};
+
+		pr_debug("[HDCP_DM] UNDESIRED->UNDESIRED %s :false\n", __func__);
 		return false;
+	}
 
-	/*
-	 * Handles:	UNDESIRED -> DESIRED
-	 *		DESIRED -> UNDESIRED
-	 *		ENABLED -> UNDESIRED
-	 */
-	if (state->content_protection != DRM_MODE_CONTENT_PROTECTION_ENABLED)
+	if (new_conn_state->content_protection != DRM_MODE_CONTENT_PROTECTION_ENABLED) {
+		pr_debug("[HDCP_DM] UNDESIRED->DESIRED or DESIRED->UNDESIRED or ENABLED->UNDESIRED %s :true\n",
+			__func__);
 		return true;
+	}
 
-	/*
-	 * Handles:	DESIRED -> ENABLED
-	 */
+	pr_debug("[HDCP_DM] DESIRED->ENABLED %s :false\n", __func__);
 	return false;
 }
-
 #endif
+
 static void remove_stream(struct amdgpu_device *adev,
 			  struct amdgpu_crtc *acrtc,
 			  struct dc_stream_state *stream)
@@ -8297,15 +8334,66 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		}
 	}
 #ifdef CONFIG_DRM_AMD_DC_HDCP
+	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
+		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
+		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
+		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
+
+		pr_debug("[HDCP_DM] -------------- i : %x ----------\n", i);
+
+		if (!connector)
+			continue;
+
+		pr_debug("[HDCP_DM] connector->index: %x connect_status: %x dpms: %x\n",
+			connector->index, connector->status, connector->dpms);
+		pr_debug("[HDCP_DM] state protection old: %x new: %x\n",
+			old_con_state->content_protection, new_con_state->content_protection);
+
+		if (aconnector->dc_sink) {
+			if (aconnector->dc_sink->sink_signal != SIGNAL_TYPE_VIRTUAL &&
+				aconnector->dc_sink->sink_signal != SIGNAL_TYPE_NONE) {
+				pr_debug("[HDCP_DM] pipe_ctx dispname=%s\n",
+				aconnector->dc_sink->edid_caps.display_name);
+			}
+		}
+
+		new_crtc_state = NULL;
+		old_crtc_state = NULL;
+
+		if (acrtc) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
+			old_crtc_state = drm_atomic_get_old_crtc_state(state, &acrtc->base);
+		}
+
+		if (old_crtc_state)
+			pr_debug("old crtc en: %x a: %x m: %x a-chg: %x c-chg: %x\n",
+			old_crtc_state->enable,
+			old_crtc_state->active,
+			old_crtc_state->mode_changed,
+			old_crtc_state->active_changed,
+			old_crtc_state->connectors_changed);
+
+		if (new_crtc_state)
+			pr_debug("NEW crtc en: %x a: %x m: %x a-chg: %x c-chg: %x\n",
+			new_crtc_state->enable,
+			new_crtc_state->active,
+			new_crtc_state->mode_changed,
+			new_crtc_state->active_changed,
+			new_crtc_state->connectors_changed);
+	}
+
 	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
 		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 
 		new_crtc_state = NULL;
+		old_crtc_state = NULL;
 
-		if (acrtc)
+		if (acrtc) {
 			new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
+			old_crtc_state = drm_atomic_get_old_crtc_state(state, &acrtc->base);
+		}
 
 		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
@@ -8317,7 +8405,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			continue;
 		}
 
-		if (is_content_protection_different(new_con_state, old_con_state, connector, adev->dm.hdcp_workqueue)) {
+		if (is_content_protection_different(new_crtc_state, old_crtc_state, new_con_state,
+          old_con_state, connector, adev->dm.hdcp_workqueue)) {
 			/* when display is unplugged from mst hub, connctor will
 			 * be destroyed within dm_dp_mst_connector_destroy. connector
 			 * hdcp perperties, like type, undesired, desired, enabled,
@@ -8327,6 +8416,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			 * will be retrieved from hdcp_work within dm_dp_mst_get_modes
 			 */
 
+			bool enable_encryption = false;
+
+			if (new_con_state->content_protection ==
+				DRM_MODE_CONTENT_PROTECTION_DESIRED)
+				enable_encryption = true;
+
 			if (aconnector->dc_link && aconnector->dc_sink &&
 				aconnector->dc_link->type == dc_connection_mst_branch) {
 				struct hdcp_workqueue *hdcp_work = adev->dm.hdcp_workqueue;
@@ -8339,10 +8434,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 					new_con_state->content_protection;
 			}
 
+			if (new_crtc_state && new_crtc_state->mode_changed && new_con_state->content_protection >=
+          DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+        enable_encryption = true;
+			}
+
+			DRM_INFO("[HDCP_DM] hdcp_update_display enable_encryption = %x\n", enable_encryption);
+
 			hdcp_update_display(
 				adev->dm.hdcp_workqueue, aconnector->dc_link->link_index, aconnector,
-				new_con_state->hdcp_content_type,
-				new_con_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED);
+				new_con_state->hdcp_content_type, enable_encryption);
     }
 	}
 #endif
-- 
2.39.0

[PATCH 03/18] drm/amd/display: Uninitialized variables causing 4k60 UCLK to stay at DPM1 and not DPM0

[Aurabindo Pillai]

From: Samson Tam <samson.tam@amd.com>

[Why]
SwathSizePerSurfaceY[] and SwathSizePerSurfaceC[] values are uninitialized
 because we are using += instead of = operator.

[How]
Assign values in loop with = operator.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Samson Tam <samson.tam@amd.com>
Reviewed-by: Aric Cyr <aric.cyr@amd.com>
---
 .../drm/amd/display/dc/dml/dcn32/display_mode_vba_util_32.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn32/display_mode_vba_util_32.c b/drivers/gpu/drm/amd/display/dc/dml/dcn32/display_mode_vba_util_32.c
index 5af601cff1a0..b53feeaf5cf1 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/dcn32/display_mode_vba_util_32.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/dcn32/display_mode_vba_util_32.c
@@ -6257,12 +6257,12 @@ bool dml32_CalculateDETSwathFillLatencyHiding(unsigned int NumberOfActiveSurface
 	double SwathSizePerSurfaceC[DC__NUM_DPP__MAX];
 	bool NotEnoughDETSwathFillLatencyHiding = false;
 
-	/* calculate sum of single swath size for all pipes in bytes*/
+	/* calculate sum of single swath size for all pipes in bytes */
 	for (k = 0; k < NumberOfActiveSurfaces; k++) {
-		SwathSizePerSurfaceY[k] += SwathHeightY[k] * SwathWidthY[k] * BytePerPixelInDETY[k] * NumOfDPP[k];
+		SwathSizePerSurfaceY[k] = SwathHeightY[k] * SwathWidthY[k] * BytePerPixelInDETY[k] * NumOfDPP[k];
 
 		if (SwathHeightC[k] != 0)
-			SwathSizePerSurfaceC[k] += SwathHeightC[k] * SwathWidthC[k] * BytePerPixelInDETC[k] * NumOfDPP[k];
+			SwathSizePerSurfaceC[k] = SwathHeightC[k] * SwathWidthC[k] * BytePerPixelInDETC[k] * NumOfDPP[k];
 		else
 			SwathSizePerSurfaceC[k] = 0;
 
-- 
2.39.0

[PATCH 04/18] drm/amd/display: Improvements in secure display

[Aurabindo Pillai]

From: Alan Liu <HaoPing.Liu@amd.com>

[Why]
- Need error message when failing to allocating secure_display_ctx.
- Need to check if secure display context in psp is initialized or not
before using it.

[How]
- Add error message when memory allocation fail.
- Add check before accessing psp secure display context.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Alan Liu <HaoPing.Liu@amd.com>
Reviewed-by: Wayne Lin <Wayne.Lin@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c     | 3 +++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 24443547bd02..6b7a0f521f1f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1644,6 +1644,9 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 #endif
 #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
 	adev->dm.secure_display_ctxs = amdgpu_dm_crtc_secure_display_create_contexts(adev);
+	if (!adev->dm.secure_display_ctxs) {
+		DRM_ERROR("amdgpu: failed to initialize secure_display_ctxs.\n");
+	}
 #endif
 	if (dc_is_dmub_outbox_supported(adev->dm.dc)) {
 		init_completion(&adev->dm.dmub_aux_transfer_done);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index 8bf33fa4abd9..ad73e5855580 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -117,6 +117,12 @@ static void amdgpu_dm_crtc_notify_ta_to_read(struct work_struct *work)
 	}
 
 	psp = &drm_to_adev(crtc->dev)->psp;
+
+	if (!psp->securedisplay_context.context.initialized) {
+		DRM_DEBUG_DRIVER("Secure Display fails to notify PSP TA\n");
+		return;
+	}
+
 	stream = to_amdgpu_crtc(crtc)->dm_irq_params.stream;
 	phy_inst = stream->link->link_enc_hw_inst;
 
-- 
2.39.0

[PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Aurabindo Pillai]

From: Alex Hung <alex.hung@amd.com>

[Why]
When running IGT kms_bw test with DP monitor, some systems crash from
msleep no matter how long or short the time is.

[How]
To replace msleep with mdelay.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Alex Hung <alex.hung@amd.com>
Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
index 913a1fe6b3da..e6251ccadb70 100644
--- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
@@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
 			 * After output is idle pattern some sinks need time to recognize the stream
 			 * has changed or they enter protection state and hang.
 			 */
-			msleep(60);
+			mdelay(60);
 		} else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
 			if (!link->dc->config.edp_no_power_sequencing) {
 				/*
-- 
2.39.0

[PATCH 06/18] drm/amd/display: Refactor ABM code flow

[Aurabindo Pillai]

From: Leon Huang <Leon.Huang1@amd.com>

[Why&How]
Recent ABM flow is highly coupled with backlight control on
LCD, refactor ABM control flow to be extensible to support different
panel types(e.g. LCD/OLED/miniLED)

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Leon Huang <Leon.Huang1@amd.com>
Reviewed-by: Chun-Liang Chang <chun-liang.chang@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dc.h           |   4 +-
 drivers/gpu/drm/amd/display/dc/dce/Makefile   |   3 +-
 drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c | 249 +++++++----------
 .../gpu/drm/amd/display/dc/dce/dmub_abm_lcd.c | 259 ++++++++++++++++++
 .../gpu/drm/amd/display/dc/dce/dmub_abm_lcd.h |  45 +++
 .../drm/amd/display/dc/dcn21/dcn21_hwseq.c    |  62 ++---
 drivers/gpu/drm/amd/display/dc/inc/hw/abm.h   |   6 +
 7 files changed, 427 insertions(+), 201 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.c
 create mode 100644 drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.h

diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index c14205e3183f..2896157b37da 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -1434,7 +1434,9 @@ union dpcd_sink_ext_caps {
 		uint8_t hdr_aux_backlight_control : 1;
 		uint8_t reserved_1 : 2;
 		uint8_t oled : 1;
-		uint8_t reserved : 3;
+		uint8_t reserved_2 : 1;
+		uint8_t miniled : 1;
+		uint8_t reserved : 1;
 	} bits;
 	uint8_t raw;
 };
diff --git a/drivers/gpu/drm/amd/display/dc/dce/Makefile b/drivers/gpu/drm/amd/display/dc/dce/Makefile
index 0d7db132a20f..31354fe57cd9 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dce/Makefile
@@ -29,9 +29,10 @@
 DCE = dce_audio.o dce_stream_encoder.o dce_link_encoder.o dce_hwseq.o \
 dce_mem_input.o dce_clock_source.o dce_scl_filters.o dce_transform.o \
 dce_opp.o dce_dmcu.o dce_abm.o dce_ipp.o dce_aux.o \
-dce_i2c.o dce_i2c_hw.o dce_i2c_sw.o dmub_psr.o dmub_abm.o dce_panel_cntl.o \
+dce_i2c.o dce_i2c_hw.o dce_i2c_sw.o dmub_psr.o dmub_abm.o dmub_abm_lcd.o dce_panel_cntl.o \
 dmub_hw_lock_mgr.o dmub_outbox.o
 
+
 AMD_DAL_DCE = $(addprefix $(AMDDALPATH)/dc/dce/,$(DCE))
 
 AMD_DISPLAY_FILES += $(AMD_DAL_DCE)
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
index fb0dec4ed3a6..e668d0315a4c 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm.c
@@ -24,212 +24,153 @@
  */
 
 #include "dmub_abm.h"
-#include "dce_abm.h"
+#include "dmub_abm_lcd.h"
 #include "dc.h"
-#include "dc_dmub_srv.h"
-#include "dmub/dmub_srv.h"
 #include "core_types.h"
-#include "dm_services.h"
-#include "reg_helper.h"
-#include "fixed31_32.h"
-
-#include "atom.h"
 
 #define TO_DMUB_ABM(abm)\
 	container_of(abm, struct dce_abm, base)
 
-#define REG(reg) \
-	(dce_abm->regs->reg)
+#define ABM_FEATURE_NO_SUPPORT	0
+#define ABM_LCD_SUPPORT			1
 
-#undef FN
-#define FN(reg_name, field_name) \
-	dce_abm->abm_shift->field_name, dce_abm->abm_mask->field_name
+static unsigned int abm_feature_support(struct abm *abm, unsigned int panel_inst)
+{
+	struct dc_context *dc = abm->ctx;
+	struct dc_link *edp_links[MAX_NUM_EDP];
+	int i;
+	int edp_num;
+	unsigned int ret = ABM_FEATURE_NO_SUPPORT;
 
-#define CTX \
-	dce_abm->base.ctx
+	get_edp_links(dc->dc, edp_links, &edp_num);
 
-#define DISABLE_ABM_IMMEDIATELY 255
+	for (i = 0; i < edp_num; i++) {
+		if (edp_links[i]->link_status.link_active
+			&& panel_inst == i)
+			break;
+	}
 
+	if (i < edp_num) {
+		ret = ABM_LCD_SUPPORT;
+	}
 
+	return ret;
+}
 
-static void dmub_abm_enable_fractional_pwm(struct dc_context *dc)
+static void dmub_abm_init_ex(struct abm *abm, uint32_t backlight)
 {
-	union dmub_rb_cmd cmd;
-	uint32_t fractional_pwm = (dc->dc->config.disable_fractional_pwm == false) ? 1 : 0;
-	uint32_t edp_id_count = dc->dc_edp_id_count;
-	int i;
-	uint8_t panel_mask = 0;
-
-	for (i = 0; i < edp_id_count; i++)
-		panel_mask |= 0x01 << i;
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.abm_set_pwm_frac.header.type = DMUB_CMD__ABM;
-	cmd.abm_set_pwm_frac.header.sub_type = DMUB_CMD__ABM_SET_PWM_FRAC;
-	cmd.abm_set_pwm_frac.abm_set_pwm_frac_data.fractional_pwm = fractional_pwm;
-	cmd.abm_set_pwm_frac.abm_set_pwm_frac_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
-	cmd.abm_set_pwm_frac.abm_set_pwm_frac_data.panel_mask = panel_mask;
-	cmd.abm_set_pwm_frac.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_pwm_frac_data);
-
-	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
-	dc_dmub_srv_cmd_execute(dc->dmub_srv);
-	dc_dmub_srv_wait_idle(dc->dmub_srv);
+	dmub_abm_init(abm, backlight);
 }
 
-static void dmub_abm_init(struct abm *abm, uint32_t backlight)
+static unsigned int dmub_abm_get_current_backlight_ex(struct abm *abm)
 {
-	struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
-
-	REG_WRITE(DC_ABM1_HG_SAMPLE_RATE, 0x3);
-	REG_WRITE(DC_ABM1_HG_SAMPLE_RATE, 0x1);
-	REG_WRITE(DC_ABM1_LS_SAMPLE_RATE, 0x3);
-	REG_WRITE(DC_ABM1_LS_SAMPLE_RATE, 0x1);
-	REG_WRITE(BL1_PWM_BL_UPDATE_SAMPLE_RATE, 0x1);
-
-	REG_SET_3(DC_ABM1_HG_MISC_CTRL, 0,
-			ABM1_HG_NUM_OF_BINS_SEL, 0,
-			ABM1_HG_VMAX_SEL, 1,
-			ABM1_HG_BIN_BITWIDTH_SIZE_SEL, 0);
-
-	REG_SET_3(DC_ABM1_IPCSC_COEFF_SEL, 0,
-			ABM1_IPCSC_COEFF_SEL_R, 2,
-			ABM1_IPCSC_COEFF_SEL_G, 4,
-			ABM1_IPCSC_COEFF_SEL_B, 2);
+	return dmub_abm_get_current_backlight(abm);
+}
 
-	REG_UPDATE(BL1_PWM_CURRENT_ABM_LEVEL,
-			BL1_PWM_CURRENT_ABM_LEVEL, backlight);
+static unsigned int dmub_abm_get_target_backlight_ex(struct abm *abm)
+{
+	return dmub_abm_get_target_backlight(abm);
+}
 
-	REG_UPDATE(BL1_PWM_TARGET_ABM_LEVEL,
-			BL1_PWM_TARGET_ABM_LEVEL, backlight);
+static bool dmub_abm_set_level_ex(struct abm *abm, uint32_t level)
+{
+	bool ret = false;
+	unsigned int feature_support, i;
+	uint8_t panel_mask0 = 0;
 
-	REG_UPDATE(BL1_PWM_USER_LEVEL,
-			BL1_PWM_USER_LEVEL, backlight);
+	for (i = 0; i < MAX_NUM_EDP; i++) {
+		feature_support = abm_feature_support(abm, i);
 
-	REG_UPDATE_2(DC_ABM1_LS_MIN_MAX_PIXEL_VALUE_THRES,
-			ABM1_LS_MIN_PIXEL_VALUE_THRES, 0,
-			ABM1_LS_MAX_PIXEL_VALUE_THRES, 1000);
+		if (feature_support == ABM_LCD_SUPPORT)
+			panel_mask0 |= (0x01 << i);
+	}
 
-	REG_SET_3(DC_ABM1_HGLS_REG_READ_PROGRESS, 0,
-			ABM1_HG_REG_READ_MISSED_FRAME_CLEAR, 1,
-			ABM1_LS_REG_READ_MISSED_FRAME_CLEAR, 1,
-			ABM1_BL_REG_READ_MISSED_FRAME_CLEAR, 1);
+	if (panel_mask0)
+		ret = dmub_abm_set_level(abm, level, panel_mask0);
 
-	dmub_abm_enable_fractional_pwm(abm->ctx);
+	return ret;
 }
 
-static unsigned int dmub_abm_get_current_backlight(struct abm *abm)
+static bool dmub_abm_init_config_ex(struct abm *abm,
+	const char *src,
+	unsigned int bytes,
+	unsigned int inst)
 {
-	struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
-	unsigned int backlight = REG_READ(BL1_PWM_CURRENT_ABM_LEVEL);
+	unsigned int feature_support;
 
-	/* return backlight in hardware format which is unsigned 17 bits, with
-	 * 1 bit integer and 16 bit fractional
-	 */
-	return backlight;
-}
+	feature_support = abm_feature_support(abm, inst);
 
-static unsigned int dmub_abm_get_target_backlight(struct abm *abm)
-{
-	struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
-	unsigned int backlight = REG_READ(BL1_PWM_TARGET_ABM_LEVEL);
+	if (feature_support == ABM_LCD_SUPPORT)
+		dmub_abm_init_config(abm, src, bytes, inst);
 
-	/* return backlight in hardware format which is unsigned 17 bits, with
-	 * 1 bit integer and 16 bit fractional
-	 */
-	return backlight;
+	return true;
 }
 
-static bool dmub_abm_set_level(struct abm *abm, uint32_t level)
+static bool dmub_abm_set_pause_ex(struct abm *abm, bool pause, unsigned int panel_inst, unsigned int stream_inst)
 {
-	union dmub_rb_cmd cmd;
-	struct dc_context *dc = abm->ctx;
-	struct dc_link *edp_links[MAX_NUM_EDP];
-	int i;
-	int edp_num;
-	uint8_t panel_mask = 0;
-
-	get_edp_links(dc->dc, edp_links, &edp_num);
-
-	for (i = 0; i < edp_num; i++) {
-		if (edp_links[i]->link_status.link_active)
-			panel_mask |= (0x01 << i);
-	}
+	bool ret = false;
+	unsigned int feature_support;
 
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.abm_set_level.header.type = DMUB_CMD__ABM;
-	cmd.abm_set_level.header.sub_type = DMUB_CMD__ABM_SET_LEVEL;
-	cmd.abm_set_level.abm_set_level_data.level = level;
-	cmd.abm_set_level.abm_set_level_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
-	cmd.abm_set_level.abm_set_level_data.panel_mask = panel_mask;
-	cmd.abm_set_level.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_level_data);
+	feature_support = abm_feature_support(abm, panel_inst);
 
-	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
-	dc_dmub_srv_cmd_execute(dc->dmub_srv);
-	dc_dmub_srv_wait_idle(dc->dmub_srv);
+	if (feature_support == ABM_LCD_SUPPORT)
+		ret = dmub_abm_set_pause(abm, pause, panel_inst, stream_inst);
 
-	return true;
+	return ret;
 }
 
-static bool dmub_abm_init_config(struct abm *abm,
-	const char *src,
-	unsigned int bytes,
-	unsigned int inst)
+static bool dmub_abm_set_pipe_ex(struct abm *abm, uint32_t otg_inst, uint32_t option, uint32_t panel_inst)
 {
-	union dmub_rb_cmd cmd;
-	struct dc_context *dc = abm->ctx;
-	uint8_t panel_mask = 0x01 << inst;
+	bool ret = false;
+	unsigned int feature_support;
 
-	// TODO: Optimize by only reading back final 4 bytes
-	dmub_flush_buffer_mem(&dc->dmub_srv->dmub->scratch_mem_fb);
+	feature_support = abm_feature_support(abm, panel_inst);
 
-	// Copy iramtable into cw7
-	memcpy(dc->dmub_srv->dmub->scratch_mem_fb.cpu_addr, (void *)src, bytes);
+	if (feature_support == ABM_LCD_SUPPORT)
+		ret = dmub_abm_set_pipe(abm, otg_inst, option, panel_inst);
 
-	memset(&cmd, 0, sizeof(cmd));
-	// Fw will copy from cw7 to fw_state
-	cmd.abm_init_config.header.type = DMUB_CMD__ABM;
-	cmd.abm_init_config.header.sub_type = DMUB_CMD__ABM_INIT_CONFIG;
-	cmd.abm_init_config.abm_init_config_data.src.quad_part = dc->dmub_srv->dmub->scratch_mem_fb.gpu_addr;
-	cmd.abm_init_config.abm_init_config_data.bytes = bytes;
-	cmd.abm_init_config.abm_init_config_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
-	cmd.abm_init_config.abm_init_config_data.panel_mask = panel_mask;
+	return ret;
+}
 
-	cmd.abm_init_config.header.payload_bytes = sizeof(struct dmub_cmd_abm_init_config_data);
+static bool dmub_abm_set_event_ex(struct abm *abm, unsigned int full_screen, unsigned int video_mode,
+		unsigned int hdr_mode, unsigned int panel_inst)
+{
+	bool ret = false;
+	unsigned int feature_support;
 
-	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
-	dc_dmub_srv_cmd_execute(dc->dmub_srv);
-	dc_dmub_srv_wait_idle(dc->dmub_srv);
+	feature_support = abm_feature_support(abm, panel_inst);
 
-	return true;
+	return ret;
 }
 
-static bool dmub_abm_set_pause(struct abm *abm, bool pause, unsigned int panel_inst, unsigned int stream_inst)
+static bool dmub_abm_set_backlight_level_pwm_ex(struct abm *abm,
+		unsigned int backlight_pwm_u16_16,
+		unsigned int frame_ramp,
+		unsigned int controller_id,
+		unsigned int panel_inst)
 {
-	union dmub_rb_cmd cmd;
-	struct dc_context *dc = abm->ctx;
-	uint8_t panel_mask = 0x01 << panel_inst;
+	bool ret = false;
+	unsigned int feature_support;
 
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.abm_pause.header.type = DMUB_CMD__ABM;
-	cmd.abm_pause.header.sub_type = DMUB_CMD__ABM_PAUSE;
-	cmd.abm_pause.abm_pause_data.enable = pause;
-	cmd.abm_pause.abm_pause_data.panel_mask = panel_mask;
-	cmd.abm_set_level.header.payload_bytes = sizeof(struct dmub_cmd_abm_pause_data);
+	feature_support = abm_feature_support(abm, panel_inst);
 
-	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
-	dc_dmub_srv_cmd_execute(dc->dmub_srv);
-	dc_dmub_srv_wait_idle(dc->dmub_srv);
+	if (feature_support == ABM_LCD_SUPPORT)
+		ret = dmub_abm_set_backlight_level(abm, backlight_pwm_u16_16, frame_ramp, panel_inst);
 
-	return true;
+	return ret;
 }
 
 static const struct abm_funcs abm_funcs = {
-	.abm_init = dmub_abm_init,
-	.set_abm_level = dmub_abm_set_level,
-	.get_current_backlight = dmub_abm_get_current_backlight,
-	.get_target_backlight = dmub_abm_get_target_backlight,
-	.init_abm_config = dmub_abm_init_config,
-	.set_abm_pause = dmub_abm_set_pause,
+	.abm_init = dmub_abm_init_ex,
+	.set_abm_level = dmub_abm_set_level_ex,
+	.get_current_backlight = dmub_abm_get_current_backlight_ex,
+	.get_target_backlight = dmub_abm_get_target_backlight_ex,
+	.init_abm_config = dmub_abm_init_config_ex,
+	.set_abm_pause = dmub_abm_set_pause_ex,
+	.set_pipe_ex = dmub_abm_set_pipe_ex,
+	.set_abm_event = dmub_abm_set_event_ex,
+	.set_backlight_level_pwm = dmub_abm_set_backlight_level_pwm_ex,
+
 };
 
 static void dmub_abm_construct(
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.c b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.c
new file mode 100644
index 000000000000..a235ab0fbd6f
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.c
@@ -0,0 +1,259 @@
+/*
+ * Copyright 2019 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 "dmub_abm.h"
+#include "dce_abm.h"
+#include "dc.h"
+#include "dc_dmub_srv.h"
+#include "dmub/dmub_srv.h"
+#include "core_types.h"
+#include "dm_services.h"
+#include "reg_helper.h"
+#include "fixed31_32.h"
+
+#include "atom.h"
+
+#define TO_DMUB_ABM(abm)\
+	container_of(abm, struct dce_abm, base)
+
+#define REG(reg) \
+	(dce_abm->regs->reg)
+
+#undef FN
+#define FN(reg_name, field_name) \
+	dce_abm->abm_shift->field_name, dce_abm->abm_mask->field_name
+
+#define CTX \
+	dce_abm->base.ctx
+
+#define DISABLE_ABM_IMMEDIATELY 255
+
+
+
+static void dmub_abm_enable_fractional_pwm(struct dc_context *dc)
+{
+	union dmub_rb_cmd cmd;
+	uint32_t fractional_pwm = (dc->dc->config.disable_fractional_pwm == false) ? 1 : 0;
+	uint32_t edp_id_count = dc->dc_edp_id_count;
+	int i;
+	uint8_t panel_mask = 0;
+
+	for (i = 0; i < edp_id_count; i++)
+		panel_mask |= 0x01 << i;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abm_set_pwm_frac.header.type = DMUB_CMD__ABM;
+	cmd.abm_set_pwm_frac.header.sub_type = DMUB_CMD__ABM_SET_PWM_FRAC;
+	cmd.abm_set_pwm_frac.abm_set_pwm_frac_data.fractional_pwm = fractional_pwm;
+	cmd.abm_set_pwm_frac.abm_set_pwm_frac_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
+	cmd.abm_set_pwm_frac.abm_set_pwm_frac_data.panel_mask = panel_mask;
+	cmd.abm_set_pwm_frac.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_pwm_frac_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+}
+
+void dmub_abm_init(struct abm *abm, uint32_t backlight)
+{
+	struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
+
+	REG_WRITE(DC_ABM1_HG_SAMPLE_RATE, 0x3);
+	REG_WRITE(DC_ABM1_HG_SAMPLE_RATE, 0x1);
+	REG_WRITE(DC_ABM1_LS_SAMPLE_RATE, 0x3);
+	REG_WRITE(DC_ABM1_LS_SAMPLE_RATE, 0x1);
+	REG_WRITE(BL1_PWM_BL_UPDATE_SAMPLE_RATE, 0x1);
+
+	REG_SET_3(DC_ABM1_HG_MISC_CTRL, 0,
+			ABM1_HG_NUM_OF_BINS_SEL, 0,
+			ABM1_HG_VMAX_SEL, 1,
+			ABM1_HG_BIN_BITWIDTH_SIZE_SEL, 0);
+
+	REG_SET_3(DC_ABM1_IPCSC_COEFF_SEL, 0,
+			ABM1_IPCSC_COEFF_SEL_R, 2,
+			ABM1_IPCSC_COEFF_SEL_G, 4,
+			ABM1_IPCSC_COEFF_SEL_B, 2);
+
+	REG_UPDATE(BL1_PWM_CURRENT_ABM_LEVEL,
+			BL1_PWM_CURRENT_ABM_LEVEL, backlight);
+
+	REG_UPDATE(BL1_PWM_TARGET_ABM_LEVEL,
+			BL1_PWM_TARGET_ABM_LEVEL, backlight);
+
+	REG_UPDATE(BL1_PWM_USER_LEVEL,
+			BL1_PWM_USER_LEVEL, backlight);
+
+	REG_UPDATE_2(DC_ABM1_LS_MIN_MAX_PIXEL_VALUE_THRES,
+			ABM1_LS_MIN_PIXEL_VALUE_THRES, 0,
+			ABM1_LS_MAX_PIXEL_VALUE_THRES, 1000);
+
+	REG_SET_3(DC_ABM1_HGLS_REG_READ_PROGRESS, 0,
+			ABM1_HG_REG_READ_MISSED_FRAME_CLEAR, 1,
+			ABM1_LS_REG_READ_MISSED_FRAME_CLEAR, 1,
+			ABM1_BL_REG_READ_MISSED_FRAME_CLEAR, 1);
+
+	dmub_abm_enable_fractional_pwm(abm->ctx);
+}
+
+unsigned int dmub_abm_get_current_backlight(struct abm *abm)
+{
+	struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
+	unsigned int backlight = REG_READ(BL1_PWM_CURRENT_ABM_LEVEL);
+
+	/* return backlight in hardware format which is unsigned 17 bits, with
+	 * 1 bit integer and 16 bit fractional
+	 */
+	return backlight;
+}
+
+unsigned int dmub_abm_get_target_backlight(struct abm *abm)
+{
+	struct dce_abm *dce_abm = TO_DMUB_ABM(abm);
+	unsigned int backlight = REG_READ(BL1_PWM_TARGET_ABM_LEVEL);
+
+	/* return backlight in hardware format which is unsigned 17 bits, with
+	 * 1 bit integer and 16 bit fractional
+	 */
+	return backlight;
+}
+
+bool dmub_abm_set_level(struct abm *abm, uint32_t level, uint8_t panel_mask)
+{
+	union dmub_rb_cmd cmd;
+	struct dc_context *dc = abm->ctx;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abm_set_level.header.type = DMUB_CMD__ABM;
+	cmd.abm_set_level.header.sub_type = DMUB_CMD__ABM_SET_LEVEL;
+	cmd.abm_set_level.abm_set_level_data.level = level;
+	cmd.abm_set_level.abm_set_level_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
+	cmd.abm_set_level.abm_set_level_data.panel_mask = panel_mask;
+	cmd.abm_set_level.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_level_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+
+	return true;
+}
+
+void dmub_abm_init_config(struct abm *abm,
+	const char *src,
+	unsigned int bytes,
+	unsigned int inst)
+{
+	union dmub_rb_cmd cmd;
+	struct dc_context *dc = abm->ctx;
+	uint8_t panel_mask = 0x01 << inst;
+
+	// TODO: Optimize by only reading back final 4 bytes
+	dmub_flush_buffer_mem(&dc->dmub_srv->dmub->scratch_mem_fb);
+
+	// Copy iramtable into cw7
+	memcpy(dc->dmub_srv->dmub->scratch_mem_fb.cpu_addr, (void *)src, bytes);
+
+	memset(&cmd, 0, sizeof(cmd));
+	// Fw will copy from cw7 to fw_state
+	cmd.abm_init_config.header.type = DMUB_CMD__ABM;
+	cmd.abm_init_config.header.sub_type = DMUB_CMD__ABM_INIT_CONFIG;
+	cmd.abm_init_config.abm_init_config_data.src.quad_part = dc->dmub_srv->dmub->scratch_mem_fb.gpu_addr;
+	cmd.abm_init_config.abm_init_config_data.bytes = bytes;
+	cmd.abm_init_config.abm_init_config_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
+	cmd.abm_init_config.abm_init_config_data.panel_mask = panel_mask;
+
+	cmd.abm_init_config.header.payload_bytes = sizeof(struct dmub_cmd_abm_init_config_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+
+}
+
+bool dmub_abm_set_pause(struct abm *abm, bool pause, unsigned int panel_inst, unsigned int stream_inst)
+{
+	union dmub_rb_cmd cmd;
+	struct dc_context *dc = abm->ctx;
+	uint8_t panel_mask = 0x01 << panel_inst;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abm_pause.header.type = DMUB_CMD__ABM;
+	cmd.abm_pause.header.sub_type = DMUB_CMD__ABM_PAUSE;
+	cmd.abm_pause.abm_pause_data.enable = pause;
+	cmd.abm_pause.abm_pause_data.panel_mask = panel_mask;
+	cmd.abm_set_level.header.payload_bytes = sizeof(struct dmub_cmd_abm_pause_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+
+	return true;
+}
+
+bool dmub_abm_set_pipe(struct abm *abm, uint32_t otg_inst, uint32_t option, uint32_t panel_inst)
+{
+	union dmub_rb_cmd cmd;
+	struct dc_context *dc = abm->ctx;
+	uint32_t ramping_boundary = 0xFFFF;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abm_set_pipe.header.type = DMUB_CMD__ABM;
+	cmd.abm_set_pipe.header.sub_type = DMUB_CMD__ABM_SET_PIPE;
+	cmd.abm_set_pipe.abm_set_pipe_data.otg_inst = otg_inst;
+	cmd.abm_set_pipe.abm_set_pipe_data.set_pipe_option = option;
+	cmd.abm_set_pipe.abm_set_pipe_data.panel_inst = panel_inst;
+	cmd.abm_set_pipe.abm_set_pipe_data.ramping_boundary = ramping_boundary;
+	cmd.abm_set_pipe.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_pipe_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+
+	return true;
+}
+
+bool dmub_abm_set_backlight_level(struct abm *abm,
+		unsigned int backlight_pwm_u16_16,
+		unsigned int frame_ramp,
+		unsigned int panel_inst)
+{
+	union dmub_rb_cmd cmd;
+	struct dc_context *dc = abm->ctx;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abm_set_backlight.header.type = DMUB_CMD__ABM;
+	cmd.abm_set_backlight.header.sub_type = DMUB_CMD__ABM_SET_BACKLIGHT;
+	cmd.abm_set_backlight.abm_set_backlight_data.frame_ramp = frame_ramp;
+	cmd.abm_set_backlight.abm_set_backlight_data.backlight_user_level = backlight_pwm_u16_16;
+	cmd.abm_set_backlight.abm_set_backlight_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
+	cmd.abm_set_backlight.abm_set_backlight_data.panel_mask = (0x01 << panel_inst);
+	cmd.abm_set_backlight.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_backlight_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+
+	return true;
+}
+
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.h b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.h
new file mode 100644
index 000000000000..55cf4ec98475
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/dc/dce/dmub_abm_lcd.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright 2019 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 __DMUB_ABM_LCD_H__
+#define __DMUB_ABM_LCD_H__
+
+#include "abm.h"
+
+void dmub_abm_init(struct abm *abm, uint32_t backlight);
+bool dmub_abm_set_level(struct abm *abm, uint32_t level, uint8_t panel_mask);
+unsigned int dmub_abm_get_current_backlight(struct abm *abm);
+unsigned int dmub_abm_get_target_backlight(struct abm *abm);
+void dmub_abm_init_config(struct abm *abm,
+	const char *src,
+	unsigned int bytes,
+	unsigned int inst);
+bool dmub_abm_set_pause(struct abm *abm, bool pause, unsigned int panel_inst, unsigned int stream_inst);
+bool dmub_abm_set_pipe(struct abm *abm, uint32_t otg_inst, uint32_t option, uint32_t panel_inst);
+bool dmub_abm_set_backlight_level(struct abm *abm,
+		unsigned int backlight_pwm_u16_16,
+		unsigned int frame_ramp,
+		unsigned int panel_inst);
+#endif
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c
index 69cc192a7e71..69b2f277d41e 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c
@@ -137,28 +137,6 @@ void dcn21_PLAT_58856_wa(struct dc_state *context, struct pipe_ctx *pipe_ctx)
 	pipe_ctx->stream->dpms_off = true;
 }
 
-static bool dmub_abm_set_pipe(struct abm *abm, uint32_t otg_inst, uint32_t option, uint32_t panel_inst)
-{
-	union dmub_rb_cmd cmd;
-	struct dc_context *dc = abm->ctx;
-	uint32_t ramping_boundary = 0xFFFF;
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.abm_set_pipe.header.type = DMUB_CMD__ABM;
-	cmd.abm_set_pipe.header.sub_type = DMUB_CMD__ABM_SET_PIPE;
-	cmd.abm_set_pipe.abm_set_pipe_data.otg_inst = otg_inst;
-	cmd.abm_set_pipe.abm_set_pipe_data.set_pipe_option = option;
-	cmd.abm_set_pipe.abm_set_pipe_data.panel_inst = panel_inst;
-	cmd.abm_set_pipe.abm_set_pipe_data.ramping_boundary = ramping_boundary;
-	cmd.abm_set_pipe.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_pipe_data);
-
-	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
-	dc_dmub_srv_cmd_execute(dc->dmub_srv);
-	dc_dmub_srv_wait_idle(dc->dmub_srv);
-
-	return true;
-}
-
 void dcn21_set_abm_immediate_disable(struct pipe_ctx *pipe_ctx)
 {
 	struct abm *abm = pipe_ctx->stream_res.abm;
@@ -172,9 +150,9 @@ void dcn21_set_abm_immediate_disable(struct pipe_ctx *pipe_ctx)
 		return;
 	}
 
-	if (abm && panel_cntl) {
-		dmub_abm_set_pipe(abm, otg_inst, SET_ABM_PIPE_IMMEDIATELY_DISABLE,
-				panel_cntl->inst);
+	if (abm && panel_cntl && abm->funcs->set_pipe_ex) {
+		abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_IMMEDIATELY_DISABLE,
+			panel_cntl->inst);
 		panel_cntl->funcs->store_backlight_level(panel_cntl);
 	}
 }
@@ -191,18 +169,16 @@ void dcn21_set_pipe(struct pipe_ctx *pipe_ctx)
 		return;
 	}
 
-	if (abm && panel_cntl)
-		dmub_abm_set_pipe(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+	if (abm && panel_cntl && abm->funcs->set_pipe_ex)
+		abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+
 }
 
 bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx,
 		uint32_t backlight_pwm_u16_16,
 		uint32_t frame_ramp)
 {
-	union dmub_rb_cmd cmd;
 	struct dc_context *dc = pipe_ctx->stream->ctx;
-	struct abm *abm = pipe_ctx->stream_res.abm;
-	uint32_t otg_inst = pipe_ctx->stream_res.tg->inst;
 	struct panel_cntl *panel_cntl = pipe_ctx->stream->link->panel_cntl;
 
 	if (dc->dc->res_pool->dmcu) {
@@ -210,21 +186,17 @@ bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx,
 		return true;
 	}
 
-	if (abm && panel_cntl)
-		dmub_abm_set_pipe(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
-
-	memset(&cmd, 0, sizeof(cmd));
-	cmd.abm_set_backlight.header.type = DMUB_CMD__ABM;
-	cmd.abm_set_backlight.header.sub_type = DMUB_CMD__ABM_SET_BACKLIGHT;
-	cmd.abm_set_backlight.abm_set_backlight_data.frame_ramp = frame_ramp;
-	cmd.abm_set_backlight.abm_set_backlight_data.backlight_user_level = backlight_pwm_u16_16;
-	cmd.abm_set_backlight.abm_set_backlight_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
-	cmd.abm_set_backlight.abm_set_backlight_data.panel_mask = (0x01 << panel_cntl->inst);
-	cmd.abm_set_backlight.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_backlight_data);
-
-	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
-	dc_dmub_srv_cmd_execute(dc->dmub_srv);
-	dc_dmub_srv_wait_idle(dc->dmub_srv);
+	if (pipe_ctx->stream_res.abm != NULL) {
+		struct abm *abm = pipe_ctx->stream_res.abm;
+		uint32_t otg_inst = pipe_ctx->stream_res.tg->inst;
+
+		if (abm && panel_cntl && abm->funcs->set_pipe_ex)
+			abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+
+		if (panel_cntl && abm->funcs->set_backlight_level_pwm)
+			abm->funcs->set_backlight_level_pwm(abm, backlight_pwm_u16_16,
+			frame_ramp, 0, panel_cntl->inst);
+	}
 
 	return true;
 }
diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw/abm.h b/drivers/gpu/drm/amd/display/dc/inc/hw/abm.h
index ecb4191b6e64..db5cf9acafe6 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/hw/abm.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/hw/abm.h
@@ -55,6 +55,12 @@ struct abm_funcs {
 			unsigned int bytes,
 			unsigned int inst);
 	bool (*set_abm_pause)(struct abm *abm, bool pause, unsigned int panel_inst, unsigned int otg_inst);
+	bool (*set_pipe_ex)(struct abm *abm,
+			unsigned int otg_inst,
+			unsigned int option,
+			unsigned int panel_inst);
+	bool (*set_abm_event)(struct abm *abm, unsigned int full_screen, unsigned int video_mode,
+			unsigned int hdr_mode, unsigned int panel_inst);
 };
 
 #endif
-- 
2.39.0

[PATCH 07/18] drm/amd/display: Turn on phantom OTG before disabling phantom pipe

[Aurabindo Pillai]

From: "Lee, Alvin" <Alvin.Lee2@amd.com>

[Description]
- Proper phantom pipe disable sequence was missing in
  commit_planes_for_stream
- If disabling phantom pipe, turn on phantom OTG first, and turn
  off the phantom OTG after the plane is disabled
- Also update sequence for enabling / disabling phantom streams
  (apply_ctx_to_hw). When enabling phantom pipes, enable before
  doing front end programming for phantom pipes. If disabling
  phantom pipes, disable after front end programming (i.e. after
  phantom plane disable)
- TODO: Still need to properly handle transition case when a phantom
  pipe is transitioned directly into a real pipe (need to fully disable
  the phantom pipe first)

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Alvin Lee <Alvin.Lee2@amd.com>
Reviewed-by: Jun Lei <Jun.Lei@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc.c      | 58 ++++++++-----------
 .../drm/amd/display/dc/dcn20/dcn20_hwseq.c    | 24 ++++++++
 .../drm/amd/display/dc/dcn32/dcn32_hwseq.c    | 36 ++++++++++++
 .../drm/amd/display/dc/dcn32/dcn32_hwseq.h    |  2 +
 .../gpu/drm/amd/display/dc/dcn32/dcn32_init.c |  1 +
 .../gpu/drm/amd/display/dc/inc/hw_sequencer.h |  1 +
 6 files changed, 88 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index e265faff4c3d..f41933845d2a 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -3326,6 +3326,7 @@ static void commit_planes_for_stream(struct dc *dc,
 	struct pipe_ctx *top_pipe_to_program = NULL;
 	bool should_lock_all_pipes = (update_type != UPDATE_TYPE_FAST);
 	bool subvp_prev_use = false;
+	bool subvp_curr_use = false;
 
 	// Once we apply the new subvp context to hardware it won't be in the
 	// dc->current_state anymore, so we have to cache it before we apply
@@ -3382,6 +3383,15 @@ static void commit_planes_for_stream(struct dc *dc,
 			break;
 	}
 
+	for (i = 0; i < dc->res_pool->pipe_count; i++) {
+		struct pipe_ctx *pipe = &context->res_ctx.pipe_ctx[i];
+
+		if (pipe->stream && pipe->stream->mall_stream_config.type == SUBVP_PHANTOM) {
+			subvp_curr_use = true;
+			break;
+		}
+	}
+
 	if (stream->test_pattern.type != DP_TEST_PATTERN_VIDEO_MODE) {
 		struct pipe_ctx *mpcc_pipe;
 		struct pipe_ctx *odm_pipe;
@@ -3653,42 +3663,22 @@ static void commit_planes_for_stream(struct dc *dc,
 					top_pipe_to_program->stream_res.tg);
 		}
 
-	/* For phantom pipe OTG enable, it has to be done after any previous pipe
-	 * that was in use has already been programmed at gotten its double buffer
-	 * update for "disable".
-	 */
-	if (update_type != UPDATE_TYPE_FAST) {
-		for (i = 0; i < dc->res_pool->pipe_count; i++) {
-			struct pipe_ctx *pipe = &context->res_ctx.pipe_ctx[i];
-			struct pipe_ctx *old_pipe = &dc->current_state->res_ctx.pipe_ctx[i];
-
-			/* If an active, non-phantom pipe is being transitioned into a phantom
-			 * pipe, wait for the double buffer update to complete first before we do
-			 * ANY phantom pipe programming.
-			 */
-			if (pipe->stream && pipe->stream->mall_stream_config.type == SUBVP_PHANTOM &&
-					old_pipe->stream && old_pipe->stream->mall_stream_config.type != SUBVP_PHANTOM) {
-				old_pipe->stream_res.tg->funcs->wait_for_state(
-						old_pipe->stream_res.tg,
-						CRTC_STATE_VBLANK);
-				old_pipe->stream_res.tg->funcs->wait_for_state(
-						old_pipe->stream_res.tg,
-						CRTC_STATE_VACTIVE);
-			}
+	if (subvp_curr_use) {
+		/* If enabling subvp or transitioning from subvp->subvp, enable the
+		 * phantom streams before we program front end for the phantom pipes.
+		 */
+		if (update_type != UPDATE_TYPE_FAST) {
+			if (dc->hwss.enable_phantom_streams)
+				dc->hwss.enable_phantom_streams(dc, context);
 		}
-		for (i = 0; i < dc->res_pool->pipe_count; i++) {
-			struct pipe_ctx *new_pipe = &context->res_ctx.pipe_ctx[i];
+	}
 
-			if ((new_pipe->stream && new_pipe->stream->mall_stream_config.type == SUBVP_PHANTOM) ||
-					subvp_prev_use) {
-				// If old context or new context has phantom pipes, apply
-				// the phantom timings now. We can't change the phantom
-				// pipe configuration safely without driver acquiring
-				// the DMCUB lock first.
-				dc->hwss.apply_ctx_to_hw(dc, context);
-				break;
-			}
-		}
+	if (subvp_prev_use && !subvp_curr_use) {
+		/* If disabling subvp, disable phantom streams after front end
+		 * programming has completed (we turn on phantom OTG in order
+		 * to complete the plane disable for phantom pipes).
+		 */
+		dc->hwss.apply_ctx_to_hw(dc, context);
 	}
 
 	if (update_type != UPDATE_TYPE_FAST)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
index 6291a241158a..366ba05cf8ef 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
@@ -582,6 +582,9 @@ void dcn20_plane_atomic_disable(struct dc *dc, struct pipe_ctx *pipe_ctx)
 	if (pipe_ctx->stream_res.gsl_group != 0)
 		dcn20_setup_gsl_group_as_lock(dc, pipe_ctx, false);
 
+	if (hubp->funcs->hubp_update_mall_sel)
+		hubp->funcs->hubp_update_mall_sel(hubp, 0, false);
+
 	dc->hwss.set_flip_control_gsl(pipe_ctx, false);
 
 	hubp->funcs->hubp_clk_cntl(hubp, false);
@@ -605,6 +608,9 @@ void dcn20_plane_atomic_disable(struct dc *dc, struct pipe_ctx *pipe_ctx)
 
 void dcn20_disable_plane(struct dc *dc, struct pipe_ctx *pipe_ctx)
 {
+	bool is_phantom = pipe_ctx->plane_state && pipe_ctx->plane_state->is_phantom;
+	struct timing_generator *tg = is_phantom ? pipe_ctx->stream_res.tg : NULL;
+
 	DC_LOGGER_INIT(dc->ctx->logger);
 
 	if (!pipe_ctx->plane_res.hubp || pipe_ctx->plane_res.hubp->power_gated)
@@ -612,6 +618,12 @@ void dcn20_disable_plane(struct dc *dc, struct pipe_ctx *pipe_ctx)
 
 	dcn20_plane_atomic_disable(dc, pipe_ctx);
 
+	/* Turn back off the phantom OTG after the phantom plane is fully disabled
+	 */
+	if (is_phantom)
+		if (tg && tg->funcs->disable_phantom_crtc)
+			tg->funcs->disable_phantom_crtc(tg);
+
 	DC_LOG_DC("Power down front end %d\n",
 					pipe_ctx->pipe_idx);
 }
@@ -1803,6 +1815,18 @@ void dcn20_program_front_end_for_ctx(
 		dcn20_detect_pipe_changes(&dc->current_state->res_ctx.pipe_ctx[i],
 				&context->res_ctx.pipe_ctx[i]);
 
+	/* When disabling phantom pipes, turn on phantom OTG first (so we can get double
+	 * buffer updates properly)
+	 */
+	for (i = 0; i < dc->res_pool->pipe_count; i++)
+		if (context->res_ctx.pipe_ctx[i].update_flags.bits.disable
+				&& dc->current_state->res_ctx.pipe_ctx[i].stream->mall_stream_config.type == SUBVP_PHANTOM) {
+			struct timing_generator *tg = dc->current_state->res_ctx.pipe_ctx[i].stream_res.tg;
+
+			if (tg->funcs->enable_crtc)
+				tg->funcs->enable_crtc(tg);
+		}
+
 	/* OTG blank before disabling all front ends */
 	for (i = 0; i < dc->res_pool->pipe_count; i++)
 		if (context->res_ctx.pipe_ctx[i].update_flags.bits.disable
diff --git a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.c
index 2f0ebe1f6c45..c10d8a60380a 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.c
@@ -1451,3 +1451,39 @@ void dcn32_update_dsc_pg(struct dc *dc,
 		}
 	}
 }
+
+void dcn32_enable_phantom_streams(struct dc *dc, struct dc_state *context)
+{
+	unsigned int i;
+
+	for (i = 0; i < dc->res_pool->pipe_count; i++) {
+		struct pipe_ctx *pipe = &context->res_ctx.pipe_ctx[i];
+		struct pipe_ctx *old_pipe = &dc->current_state->res_ctx.pipe_ctx[i];
+
+		/* If an active, non-phantom pipe is being transitioned into a phantom
+		 * pipe, wait for the double buffer update to complete first before we do
+		 * ANY phantom pipe programming.
+		 */
+		if (pipe->stream && pipe->stream->mall_stream_config.type == SUBVP_PHANTOM &&
+				old_pipe->stream && old_pipe->stream->mall_stream_config.type != SUBVP_PHANTOM) {
+			old_pipe->stream_res.tg->funcs->wait_for_state(
+					old_pipe->stream_res.tg,
+					CRTC_STATE_VBLANK);
+			old_pipe->stream_res.tg->funcs->wait_for_state(
+					old_pipe->stream_res.tg,
+					CRTC_STATE_VACTIVE);
+		}
+	}
+	for (i = 0; i < dc->res_pool->pipe_count; i++) {
+		struct pipe_ctx *new_pipe = &context->res_ctx.pipe_ctx[i];
+
+		if (new_pipe->stream && new_pipe->stream->mall_stream_config.type == SUBVP_PHANTOM) {
+			// If old context or new context has phantom pipes, apply
+			// the phantom timings now. We can't change the phantom
+			// pipe configuration safely without driver acquiring
+			// the DMCUB lock first.
+			dc->hwss.apply_ctx_to_hw(dc, context);
+			break;
+		}
+	}
+}
diff --git a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.h b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.h
index 7de36529cf99..e9e9534f3668 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_hwseq.h
@@ -102,4 +102,6 @@ void dcn32_update_dsc_pg(struct dc *dc,
 		struct dc_state *context,
 		bool safe_to_disable);
 
+void dcn32_enable_phantom_streams(struct dc *dc, struct dc_state *context);
+
 #endif /* __DC_HWSS_DCN32_H__ */
diff --git a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_init.c b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_init.c
index dc4649458567..330d7cbc7398 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_init.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn32/dcn32_init.c
@@ -106,6 +106,7 @@ static const struct hw_sequencer_funcs dcn32_funcs = {
 	.set_disp_pattern_generator = dcn30_set_disp_pattern_generator,
 	.get_dcc_en_bits = dcn10_get_dcc_en_bits,
 	.commit_subvp_config = dcn32_commit_subvp_config,
+	.enable_phantom_streams = dcn32_enable_phantom_streams,
 	.subvp_pipe_control_lock = dcn32_subvp_pipe_control_lock,
 	.update_visual_confirm_color = dcn20_update_visual_confirm_color,
 	.update_phantom_vp_position = dcn32_update_phantom_vp_position,
diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h b/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h
index c43523f9ff6d..88ac723d10aa 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/hw_sequencer.h
@@ -266,6 +266,7 @@ struct hw_sequencer_funcs {
 	void (*apply_update_flags_for_phantom)(struct pipe_ctx *phantom_pipe);
 
 	void (*commit_subvp_config)(struct dc *dc, struct dc_state *context);
+	void (*enable_phantom_streams)(struct dc *dc, struct dc_state *context);
 	void (*subvp_pipe_control_lock)(struct dc *dc,
 			struct dc_state *context,
 			bool lock,
-- 
2.39.0

[PATCH 08/18] drm/amd/display: patch cases with unknown plane state to prevent warning

[Aurabindo Pillai]

From: Swapnil Patel <Swapnil.Patel@amd.com>

[Why]
DCN301 resource function is missing function pointer to
handle cases with unknown plane state.
This causes assertion when global state is validated while
using swizzle parameter as “DC_UNKNOWN”

[How]
Add function pointer to handle and patch cases when plane
state is unknown.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Swapnil Patel <Swapnil.Patel@amd.com>
Reviewed-by: Sung joon Kim <Sungjoon.Kim@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 8cf10351f271..ee62ae3eb98f 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
@@ -1414,7 +1414,8 @@ static struct resource_funcs dcn301_res_pool_funcs = {
 	.find_first_free_match_stream_enc_for_link = dcn10_find_first_free_match_stream_enc_for_link,
 	.acquire_post_bldn_3dlut = dcn30_acquire_post_bldn_3dlut,
 	.release_post_bldn_3dlut = dcn30_release_post_bldn_3dlut,
-	.update_bw_bounding_box = dcn301_update_bw_bounding_box
+	.update_bw_bounding_box = dcn301_update_bw_bounding_box,
+	.patch_unknown_plane_state = dcn20_patch_unknown_plane_state
 };
 
 static bool dcn301_resource_construct(
-- 
2.39.0

[PATCH 09/18] drm/amd/display: Defer DIG FIFO disable after VID stream enable

[Aurabindo Pillai]

From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

[Why]
On some monitors we see a brief flash of corruption during the
monitor disable sequence caused by FIFO being disabled in the middle
of an active DP stream.

[How]
Wait until DP vid stream is disabled before turning off the FIFO.

The FIFO reset on DP unblank should take care of clearing any FIFO
error, if any.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Reviewed-by: Syed Hassan <Syed.Hassan@amd.com>
---
 .../drm/amd/display/dc/dcn314/dcn314_dio_stream_encoder.c   | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_dio_stream_encoder.c b/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_dio_stream_encoder.c
index 38842f938bed..0926db018338 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_dio_stream_encoder.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn314/dcn314_dio_stream_encoder.c
@@ -278,10 +278,10 @@ static void enc314_stream_encoder_dp_blank(
 	struct dc_link *link,
 	struct stream_encoder *enc)
 {
-	/* New to DCN314 - disable the FIFO before VID stream disable. */
-	enc314_disable_fifo(enc);
-
 	enc1_stream_encoder_dp_blank(link, enc);
+
+	/* Disable FIFO after the DP vid stream is disabled to avoid corruption. */
+	enc314_disable_fifo(enc);
 }
 
 static void enc314_stream_encoder_dp_unblank(
-- 
2.39.0

[PATCH 10/18] drm/amd/display: fix dc_get_edp_link_panel_inst to only consider links with panels

[Aurabindo Pillai]

From: Dmytro Laktyushkin <Dmytro.Laktyushkin@amd.com>

This function is meant to be used on multi-edp systems and only makes sense
if only links with connected panels are considered.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Dmytro Laktyushkin <Dmytro.Laktyushkin@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dc_link.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dc_link.h b/drivers/gpu/drm/amd/display/dc/dc_link.h
index 2e18bcf6b11a..8565bbb75177 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_link.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_link.h
@@ -335,15 +335,18 @@ static inline bool dc_get_edp_link_panel_inst(const struct dc *dc,
 		unsigned int *inst_out)
 {
 	struct dc_link *edp_links[MAX_NUM_EDP];
-	int edp_num;
+	int edp_num, i;
 
-	if (link->connector_signal != SIGNAL_TYPE_EDP)
+	*inst_out = 0;
+	if (link->connector_signal != SIGNAL_TYPE_EDP || !link->local_sink)
 		return false;
 	get_edp_links(dc, edp_links, &edp_num);
-	if ((edp_num > 1) && (link->link_index > edp_links[0]->link_index))
-		*inst_out = 1;
-	else
-		*inst_out = 0;
+	for (i = 0; i < edp_num; i++) {
+		if (link == edp_links[i])
+			break;
+		if (edp_links[i]->local_sink)
+			(*inst_out)++;
+	}
 	return true;
 }
 
-- 
2.39.0

[PATCH 11/18] drm/amd/display: move dccg programming from link hwss hpo dp to hwss

[Aurabindo Pillai]

From: Wenjing Liu <wenjing.liu@amd.com>

[why] dccg clock programming shouldn't be part of link hwss programming
sequence. The scope of link hwss is limited to encoder and phy
programming.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Wenjing Liu <wenjing.liu@amd.com>
Reviewed-by: Jun Lei <Jun.Lei@amd.com>
---
 .../display/dc/dce110/dce110_hw_sequencer.c   | 13 +++++
 .../drm/amd/display/dc/dcn20/dcn20_hwseq.c    | 50 +++++++++++++++++++
 .../amd/display/dc/link/link_hwss_hpo_dp.c    | 37 --------------
 3 files changed, 63 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
index e6251ccadb70..2d9302959f25 100644
--- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
@@ -1142,6 +1142,10 @@ void dce110_disable_stream(struct pipe_ctx *pipe_ctx)
 	struct dc_link *link = stream->link;
 	struct dc *dc = pipe_ctx->stream->ctx->dc;
 	const struct link_hwss *link_hwss = get_link_hwss(link, &pipe_ctx->link_res);
+	struct dccg *dccg = dc->res_pool->dccg;
+	struct timing_generator *tg = pipe_ctx->stream_res.tg;
+	struct dtbclk_dto_params dto_params = {0};
+	int dp_hpo_inst;
 
 	if (dc_is_hdmi_tmds_signal(pipe_ctx->stream->signal)) {
 		pipe_ctx->stream_res.stream_enc->funcs->stop_hdmi_info_packets(
@@ -1161,6 +1165,15 @@ void dce110_disable_stream(struct pipe_ctx *pipe_ctx)
 
 	link_hwss->reset_stream_encoder(pipe_ctx);
 
+	if (is_dp_128b_132b_signal(pipe_ctx)) {
+		dto_params.otg_inst = tg->inst;
+		dto_params.timing = &pipe_ctx->stream->timing;
+		dp_hpo_inst = pipe_ctx->stream_res.hpo_dp_stream_enc->inst;
+		dccg->funcs->set_dtbclk_dto(dccg, &dto_params);
+		dccg->funcs->disable_symclk32_se(dccg, dp_hpo_inst);
+		dccg->funcs->set_dpstreamclk(dccg, REFCLK, tg->inst, dp_hpo_inst);
+	}
+
 	if (is_dp_128b_132b_signal(pipe_ctx)) {
 		/* TODO: This looks like a bug to me as we are disabling HPO IO when
 		 * we are just disabling a single HPO stream. Shouldn't we disable HPO
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
index 366ba05cf8ef..c81b70c7f3f9 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
@@ -2639,6 +2639,37 @@ void dcn20_update_mpcc(struct dc *dc, struct pipe_ctx *pipe_ctx)
 	hubp->mpcc_id = mpcc_id;
 }
 
+static enum phyd32clk_clock_source get_phyd32clk_src(struct dc_link *link)
+{
+	switch (link->link_enc->transmitter) {
+	case TRANSMITTER_UNIPHY_A:
+		return PHYD32CLKA;
+	case TRANSMITTER_UNIPHY_B:
+		return PHYD32CLKB;
+	case TRANSMITTER_UNIPHY_C:
+		return PHYD32CLKC;
+	case TRANSMITTER_UNIPHY_D:
+		return PHYD32CLKD;
+	case TRANSMITTER_UNIPHY_E:
+		return PHYD32CLKE;
+	default:
+		return PHYD32CLKA;
+	}
+}
+
+static int get_odm_segment_count(struct pipe_ctx *pipe_ctx)
+{
+	struct pipe_ctx *odm_pipe = pipe_ctx->next_odm_pipe;
+	int count = 1;
+
+	while (odm_pipe != NULL) {
+		count++;
+		odm_pipe = odm_pipe->next_odm_pipe;
+	}
+
+	return count;
+}
+
 void dcn20_enable_stream(struct pipe_ctx *pipe_ctx)
 {
 	enum dc_lane_count lane_count =
@@ -2652,12 +2683,31 @@ void dcn20_enable_stream(struct pipe_ctx *pipe_ctx)
 	struct timing_generator *tg = pipe_ctx->stream_res.tg;
 	const struct link_hwss *link_hwss = get_link_hwss(link, &pipe_ctx->link_res);
 	struct dc *dc = pipe_ctx->stream->ctx->dc;
+	struct dtbclk_dto_params dto_params = {0};
+	struct dccg *dccg = dc->res_pool->dccg;
+	enum phyd32clk_clock_source phyd32clk;
+	int dp_hpo_inst;
 
 	if (is_dp_128b_132b_signal(pipe_ctx)) {
 		if (dc->hwseq->funcs.setup_hpo_hw_control)
 			dc->hwseq->funcs.setup_hpo_hw_control(dc->hwseq, true);
 	}
 
+	if (is_dp_128b_132b_signal(pipe_ctx)) {
+		dp_hpo_inst = pipe_ctx->stream_res.hpo_dp_stream_enc->inst;
+		dccg->funcs->set_dpstreamclk(dccg, DTBCLK0, tg->inst, dp_hpo_inst);
+
+		phyd32clk = get_phyd32clk_src(link);
+		dccg->funcs->enable_symclk32_se(dccg, dp_hpo_inst, phyd32clk);
+
+		dto_params.otg_inst = tg->inst;
+		dto_params.pixclk_khz = pipe_ctx->stream->timing.pix_clk_100hz / 10;
+		dto_params.num_odm_segments = get_odm_segment_count(pipe_ctx);
+		dto_params.timing = &pipe_ctx->stream->timing;
+		dto_params.ref_dtbclk_khz = dc->clk_mgr->funcs->get_dtb_ref_clk_frequency(dc->clk_mgr);
+		dccg->funcs->set_dtbclk_dto(dccg, &dto_params);
+	}
+
 	link_hwss->setup_stream_encoder(pipe_ctx);
 
 	if (pipe_ctx->plane_state && pipe_ctx->plane_state->flip_immediate != 1) {
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_hwss_hpo_dp.c b/drivers/gpu/drm/amd/display/dc/link/link_hwss_hpo_dp.c
index 2f46e1ac4ce0..164d631e8809 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_hwss_hpo_dp.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_hwss_hpo_dp.c
@@ -87,57 +87,20 @@ static void set_hpo_dp_hblank_min_symbol_width(struct pipe_ctx *pipe_ctx,
 			hblank_min_symbol_width);
 }
 
-static int get_odm_segment_count(struct pipe_ctx *pipe_ctx)
-{
-	struct pipe_ctx *odm_pipe = pipe_ctx->next_odm_pipe;
-	int count = 1;
-
-	while (odm_pipe != NULL) {
-		count++;
-		odm_pipe = odm_pipe->next_odm_pipe;
-	}
-
-	return count;
-}
-
 static void setup_hpo_dp_stream_encoder(struct pipe_ctx *pipe_ctx)
 {
-	struct dc *dc = pipe_ctx->stream->ctx->dc;
 	struct hpo_dp_stream_encoder *stream_enc = pipe_ctx->stream_res.hpo_dp_stream_enc;
 	struct hpo_dp_link_encoder *link_enc = pipe_ctx->link_res.hpo_dp_link_enc;
-	struct dccg *dccg = dc->res_pool->dccg;
-	struct timing_generator *tg = pipe_ctx->stream_res.tg;
-	struct dtbclk_dto_params dto_params = {0};
-	enum phyd32clk_clock_source phyd32clk = get_phyd32clk_src(pipe_ctx->stream->link);
 
-	dto_params.otg_inst = tg->inst;
-	dto_params.pixclk_khz = pipe_ctx->stream->timing.pix_clk_100hz / 10;
-	dto_params.num_odm_segments = get_odm_segment_count(pipe_ctx);
-	dto_params.timing = &pipe_ctx->stream->timing;
-	dto_params.ref_dtbclk_khz = dc->clk_mgr->funcs->get_dtb_ref_clk_frequency(dc->clk_mgr);
-
-	dccg->funcs->set_dpstreamclk(dccg, DTBCLK0, tg->inst, stream_enc->inst);
-	dccg->funcs->enable_symclk32_se(dccg, stream_enc->inst, phyd32clk);
-	dccg->funcs->set_dtbclk_dto(dccg, &dto_params);
 	stream_enc->funcs->enable_stream(stream_enc);
 	stream_enc->funcs->map_stream_to_link(stream_enc, stream_enc->inst, link_enc->inst);
 }
 
 static void reset_hpo_dp_stream_encoder(struct pipe_ctx *pipe_ctx)
 {
-	struct dc *dc = pipe_ctx->stream->ctx->dc;
 	struct hpo_dp_stream_encoder *stream_enc = pipe_ctx->stream_res.hpo_dp_stream_enc;
-	struct dccg *dccg = dc->res_pool->dccg;
-	struct timing_generator *tg = pipe_ctx->stream_res.tg;
-	struct dtbclk_dto_params dto_params = {0};
-
-	dto_params.otg_inst = tg->inst;
-	dto_params.timing = &pipe_ctx->stream->timing;
 
 	stream_enc->funcs->disable(stream_enc);
-	dccg->funcs->set_dtbclk_dto(dccg, &dto_params);
-	dccg->funcs->disable_symclk32_se(dccg, stream_enc->inst);
-	dccg->funcs->set_dpstreamclk(dccg, REFCLK, tg->inst, stream_enc->inst);
 }
 
 static void setup_hpo_dp_stream_attribute(struct pipe_ctx *pipe_ctx)
-- 
2.39.0

[PATCH 12/18] drm/amd/display: update pixel rate div in enable stream

[Aurabindo Pillai]

From: Wenjing Liu <wenjing.liu@amd.com>

[why]
Pixel rate div depends on the type of encoder
that we are enabling stream with. If we swap between
HPO and DIO encoder at the time we call enable stream
for the new encoder, we must reprogram pixel rate div
based on the new encoder type.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Wenjing Liu <wenjing.liu@amd.com>
Reviewed-by: Jun Lei <Jun.Lei@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
index c81b70c7f3f9..20c85ef2a957 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
@@ -2687,6 +2687,9 @@ void dcn20_enable_stream(struct pipe_ctx *pipe_ctx)
 	struct dccg *dccg = dc->res_pool->dccg;
 	enum phyd32clk_clock_source phyd32clk;
 	int dp_hpo_inst;
+	struct dce_hwseq *hws = dc->hwseq;
+	unsigned int k1_div = PIXEL_RATE_DIV_NA;
+	unsigned int k2_div = PIXEL_RATE_DIV_NA;
 
 	if (is_dp_128b_132b_signal(pipe_ctx)) {
 		if (dc->hwseq->funcs.setup_hpo_hw_control)
@@ -2708,6 +2711,15 @@ void dcn20_enable_stream(struct pipe_ctx *pipe_ctx)
 		dccg->funcs->set_dtbclk_dto(dccg, &dto_params);
 	}
 
+	if (hws->funcs.calculate_dccg_k1_k2_values && dc->res_pool->dccg->funcs->set_pixel_rate_div) {
+		hws->funcs.calculate_dccg_k1_k2_values(pipe_ctx, &k1_div, &k2_div);
+
+		dc->res_pool->dccg->funcs->set_pixel_rate_div(
+			dc->res_pool->dccg,
+			pipe_ctx->stream_res.tg->inst,
+			k1_div, k2_div);
+	}
+
 	link_hwss->setup_stream_encoder(pipe_ctx);
 
 	if (pipe_ctx->plane_state && pipe_ctx->plane_state->flip_immediate != 1) {
-- 
2.39.0

[PATCH 13/18] drm/amd/display: allow hpo and dio encoder switching during dp retrain test

[Aurabindo Pillai]

From: Wenjing Liu <wenjing.liu@amd.com>

[why]
During DP2.1 LL CTS if test equipment requests to change between
DP2.1 and DP1.4 link rates, we need to swap between HPO and DIO
encoders by remapping encoder resource.

[how]
Add a function dc resource to update encoder resources and toggle
dpms state for all enabled stream associated witht the link under test.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Wenjing Liu <wenjing.liu@amd.com>
Reviewed-by: Jun Lei <Jun.Lei@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc_link.c |  3 -
 .../gpu/drm/amd/display/dc/core/dc_link_dp.c  | 64 +++++++------------
 .../gpu/drm/amd/display/dc/core/dc_resource.c | 39 +++++++++++
 drivers/gpu/drm/amd/display/dc/inc/resource.h |  9 +++
 4 files changed, 72 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index 1ca3328b492c..ee20b4d3afd4 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -4649,9 +4649,6 @@ void dc_link_set_preferred_training_settings(struct dc *dc,
 
 	if (link_setting != NULL) {
 		link->preferred_link_setting = *link_setting;
-		if (dp_get_link_encoding_format(link_setting) == DP_128b_132b_ENCODING)
-			/* TODO: add dc update for acquiring link res  */
-			skip_immediate_retrain = true;
 	} else {
 		link->preferred_link_setting.lane_count = LANE_COUNT_UNKNOWN;
 		link->preferred_link_setting.link_rate = LINK_RATE_UNKNOWN;
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
index af9411ee3c74..e8a3d72a4c74 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
@@ -7276,51 +7276,35 @@ void dp_retrain_link_dp_test(struct dc_link *link,
 			struct dc_link_settings *link_setting,
 			bool skip_video_pattern)
 {
-	struct pipe_ctx *pipes =
-			&link->dc->current_state->res_ctx.pipe_ctx[0];
+	struct pipe_ctx *pipe;
 	unsigned int i;
-	bool do_fallback = false;
 
+	udelay(100);
 
 	for (i = 0; i < MAX_PIPES; i++) {
-		if (pipes[i].stream != NULL &&
-			!pipes[i].top_pipe && !pipes[i].prev_odm_pipe &&
-			pipes[i].stream->link != NULL &&
-			pipes[i].stream_res.stream_enc != NULL &&
-			pipes[i].stream->link == link) {
-			udelay(100);
-
-			link->dc->hwss.blank_stream(&pipes[i]);
-
-			/* disable any test pattern that might be active */
-			dp_set_hw_test_pattern(link, &pipes[i].link_res,
-					DP_TEST_PATTERN_VIDEO_MODE, NULL, 0);
-
-			dp_receiver_power_ctrl(link, false);
-
-			link->dc->hwss.disable_stream(&pipes[i]);
-			if (pipes[i].stream_res.audio && !link->dc->debug.az_endpoint_mute_only)
-				pipes[i].stream_res.audio->funcs->az_disable(pipes[i].stream_res.audio);
-
-			link->dc->hwss.disable_link_output(link, &pipes[i].link_res, SIGNAL_TYPE_DISPLAY_PORT);
-
-			if (link->ep_type == DISPLAY_ENDPOINT_USB4_DPIA)
-				do_fallback = true;
-
-			perform_link_training_with_retries(
-					link_setting,
-					skip_video_pattern,
-					LINK_TRAINING_ATTEMPTS,
-					&pipes[i],
-					SIGNAL_TYPE_DISPLAY_PORT,
-					do_fallback);
-
-			link->dc->hwss.enable_stream(&pipes[i]);
-
-			link->dc->hwss.unblank_stream(&pipes[i],
-					link_setting);
+		pipe = &link->dc->current_state->res_ctx.pipe_ctx[i];
+		if (pipe->stream != NULL &&
+				pipe->stream->link == link &&
+				!pipe->stream->dpms_off &&
+				!pipe->top_pipe && !pipe->prev_odm_pipe) {
+			core_link_disable_stream(pipe);
+			pipe->link_config.dp_link_settings = *link_setting;
+			update_dp_encoder_resources_for_test_harness(
+					link->dc,
+					pipe->stream->ctx->dc->current_state,
+					pipe);
+		}
+	}
 
-			link->dc->hwss.enable_audio_stream(&pipes[i]);
+	for (i = 0; i < MAX_PIPES; i++) {
+		pipe = &link->dc->current_state->res_ctx.pipe_ctx[i];
+		if (pipe->stream != NULL &&
+				pipe->stream->link == link &&
+				!pipe->stream->dpms_off &&
+				!pipe->top_pipe && !pipe->prev_odm_pipe) {
+			core_link_enable_stream(
+					pipe->stream->ctx->dc->current_state,
+					pipe);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
index 002b7b512b09..06b5f49e0954 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
@@ -3990,3 +3990,42 @@ bool dc_resource_acquire_secondary_pipe_for_mpc_odm(
 
 	return true;
 }
+
+enum dc_status update_dp_encoder_resources_for_test_harness(const struct dc *dc,
+		struct dc_state *context,
+		struct pipe_ctx *pipe_ctx)
+{
+	if (dp_get_link_encoding_format(&pipe_ctx->link_config.dp_link_settings) == DP_128b_132b_ENCODING) {
+		if (pipe_ctx->stream_res.hpo_dp_stream_enc == NULL) {
+			pipe_ctx->stream_res.hpo_dp_stream_enc =
+					find_first_free_match_hpo_dp_stream_enc_for_link(
+							&context->res_ctx, dc->res_pool, pipe_ctx->stream);
+
+			if (!pipe_ctx->stream_res.hpo_dp_stream_enc)
+				return DC_NO_STREAM_ENC_RESOURCE;
+
+			update_hpo_dp_stream_engine_usage(
+					&context->res_ctx, dc->res_pool,
+					pipe_ctx->stream_res.hpo_dp_stream_enc,
+					true);
+		}
+
+		if (pipe_ctx->link_res.hpo_dp_link_enc == NULL) {
+			if (!add_hpo_dp_link_enc_to_ctx(&context->res_ctx, dc->res_pool, pipe_ctx, pipe_ctx->stream))
+				return DC_NO_LINK_ENC_RESOURCE;
+		}
+	} else {
+		if (pipe_ctx->stream_res.hpo_dp_stream_enc) {
+			update_hpo_dp_stream_engine_usage(
+					&context->res_ctx, dc->res_pool,
+					pipe_ctx->stream_res.hpo_dp_stream_enc,
+					false);
+			pipe_ctx->stream_res.hpo_dp_stream_enc = NULL;
+		}
+		if (pipe_ctx->link_res.hpo_dp_link_enc)
+			remove_hpo_dp_link_enc_from_ctx(&context->res_ctx, pipe_ctx, pipe_ctx->stream);
+	}
+
+	return DC_OK;
+}
+
diff --git a/drivers/gpu/drm/amd/display/dc/inc/resource.h b/drivers/gpu/drm/amd/display/dc/inc/resource.h
index 5040836f404d..4ab029e3326d 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/resource.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/resource.h
@@ -236,4 +236,13 @@ bool dc_resource_acquire_secondary_pipe_for_mpc_odm(
 		struct pipe_ctx *pri_pipe,
 		struct pipe_ctx *sec_pipe,
 		bool odm);
+
+/* A test harness interface that modifies dp encoder resources in the given dc
+ * state and bypasses the need to revalidate. The interface assumes that the
+ * test harness interface is called with pre-validated link config stored in the
+ * pipe_ctx and updates dp encoder resources according to the link config.
+ */
+enum dc_status update_dp_encoder_resources_for_test_harness(const struct dc *dc,
+		struct dc_state *context,
+		struct pipe_ctx *pipe_ctx);
 #endif /* DRIVERS_GPU_DRM_AMD_DC_DEV_DC_INC_RESOURCE_H_ */
-- 
2.39.0

[PATCH 14/18] drm/amd/display: Fix crash when setting ABM pipe/backlight

[Aurabindo Pillai]

From: Leon Huang <leon.huang1@amd.com>

[Why] Setting ABM pipe/backlight crashes the system when abm callback
func pointers are NULL For some usecase, driver would like to control
PWM level before ABM resource is ready. But recent flow refactor of ABM
didn't consider that use case.

[How] Rollback flow that sending inbox command to dmub directly when ABM
function pointers aren't ready.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Leon Huang <Leon.Huang1@amd.com>
Reviewed-by: Chun-Liang Chang <chun-liang.chang@amd.com>
---
 .../drm/amd/display/dc/dcn21/dcn21_hwseq.c    | 81 ++++++++++++++++---
 1 file changed, 69 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c
index 69b2f277d41e..5d77f7586816 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_hwseq.c
@@ -137,6 +137,47 @@ void dcn21_PLAT_58856_wa(struct dc_state *context, struct pipe_ctx *pipe_ctx)
 	pipe_ctx->stream->dpms_off = true;
 }
 
+static bool dmub_abm_set_pipe(struct abm *abm, uint32_t otg_inst, uint32_t option, uint32_t panel_inst)
+{
+	union dmub_rb_cmd cmd;
+	struct dc_context *dc = abm->ctx;
+	uint32_t ramping_boundary = 0xFFFF;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abm_set_pipe.header.type = DMUB_CMD__ABM;
+	cmd.abm_set_pipe.header.sub_type = DMUB_CMD__ABM_SET_PIPE;
+	cmd.abm_set_pipe.abm_set_pipe_data.otg_inst = otg_inst;
+	cmd.abm_set_pipe.abm_set_pipe_data.set_pipe_option = option;
+	cmd.abm_set_pipe.abm_set_pipe_data.panel_inst = panel_inst;
+	cmd.abm_set_pipe.abm_set_pipe_data.ramping_boundary = ramping_boundary;
+	cmd.abm_set_pipe.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_pipe_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+
+	return true;
+}
+
+static void dmub_abm_set_backlight(struct dc_context *dc, uint32_t backlight_pwm_u16_16,
+									uint32_t frame_ramp, uint32_t panel_inst)
+{
+	union dmub_rb_cmd cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.abm_set_backlight.header.type = DMUB_CMD__ABM;
+	cmd.abm_set_backlight.header.sub_type = DMUB_CMD__ABM_SET_BACKLIGHT;
+	cmd.abm_set_backlight.abm_set_backlight_data.frame_ramp = frame_ramp;
+	cmd.abm_set_backlight.abm_set_backlight_data.backlight_user_level = backlight_pwm_u16_16;
+	cmd.abm_set_backlight.abm_set_backlight_data.version = DMUB_CMD_ABM_CONTROL_VERSION_1;
+	cmd.abm_set_backlight.abm_set_backlight_data.panel_mask = (0x01 << panel_inst);
+	cmd.abm_set_backlight.header.payload_bytes = sizeof(struct dmub_cmd_abm_set_backlight_data);
+
+	dc_dmub_srv_cmd_queue(dc->dmub_srv, &cmd);
+	dc_dmub_srv_cmd_execute(dc->dmub_srv);
+	dc_dmub_srv_wait_idle(dc->dmub_srv);
+}
+
 void dcn21_set_abm_immediate_disable(struct pipe_ctx *pipe_ctx)
 {
 	struct abm *abm = pipe_ctx->stream_res.abm;
@@ -150,9 +191,13 @@ void dcn21_set_abm_immediate_disable(struct pipe_ctx *pipe_ctx)
 		return;
 	}
 
-	if (abm && panel_cntl && abm->funcs->set_pipe_ex) {
-		abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_IMMEDIATELY_DISABLE,
+	if (abm && panel_cntl) {
+		if (abm->funcs && abm->funcs->set_pipe_ex)
+			abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_IMMEDIATELY_DISABLE,
 			panel_cntl->inst);
+		else {
+				dmub_abm_set_pipe(abm, otg_inst, SET_ABM_PIPE_IMMEDIATELY_DISABLE, panel_cntl->inst);
+		}
 		panel_cntl->funcs->store_backlight_level(panel_cntl);
 	}
 }
@@ -169,9 +214,13 @@ void dcn21_set_pipe(struct pipe_ctx *pipe_ctx)
 		return;
 	}
 
-	if (abm && panel_cntl && abm->funcs->set_pipe_ex)
-		abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
-
+	if (abm && panel_cntl) {
+		if (abm->funcs && abm->funcs->set_pipe_ex)
+			abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+		else {
+				dmub_abm_set_pipe(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+		}
+	}
 }
 
 bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx,
@@ -180,23 +229,31 @@ bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx,
 {
 	struct dc_context *dc = pipe_ctx->stream->ctx;
 	struct panel_cntl *panel_cntl = pipe_ctx->stream->link->panel_cntl;
+	struct abm *abm = pipe_ctx->stream_res.abm;
 
 	if (dc->dc->res_pool->dmcu) {
 		dce110_set_backlight_level(pipe_ctx, backlight_pwm_u16_16, frame_ramp);
 		return true;
 	}
 
-	if (pipe_ctx->stream_res.abm != NULL) {
-		struct abm *abm = pipe_ctx->stream_res.abm;
+	if (abm != NULL) {
+
 		uint32_t otg_inst = pipe_ctx->stream_res.tg->inst;
 
-		if (abm && panel_cntl && abm->funcs->set_pipe_ex)
-			abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+		if (abm && panel_cntl) {
+			if (abm->funcs && abm->funcs->set_pipe_ex)
+				abm->funcs->set_pipe_ex(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+			else {
+					dmub_abm_set_pipe(abm, otg_inst, SET_ABM_PIPE_NORMAL, panel_cntl->inst);
+			}
+		}
+	}
 
-		if (panel_cntl && abm->funcs->set_backlight_level_pwm)
-			abm->funcs->set_backlight_level_pwm(abm, backlight_pwm_u16_16,
+	if (abm && abm->funcs && abm->funcs->set_backlight_level_pwm)
+		abm->funcs->set_backlight_level_pwm(abm, backlight_pwm_u16_16,
 			frame_ramp, 0, panel_cntl->inst);
-	}
+	else
+		dmub_abm_set_backlight(dc, backlight_pwm_u16_16, frame_ramp, panel_cntl->inst);
 
 	return true;
 }
-- 
2.39.0

[PATCH 15/18] drm/amd/display: set ignore msa parameter only if freesync is enabled

[Aurabindo Pillai]

[Why&How]

ignore_msa_timing_param is used by SubVP logic to determine if SubVP
+ DRR is possible. Linux does not support freesync on multi display
config, which results in incorrect assumption of VRR support if we
set this parameter when VRR is supported, but not enabled.

Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Reviewed-by: Rodrigo Siqueira <rodrigo.siqueira@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 6b7a0f521f1f..f9461857ac6a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8813,15 +8813,22 @@ static void get_freesync_config_for_crtc(
 	struct drm_display_mode *mode = &new_crtc_state->base.mode;
 	int vrefresh = drm_mode_vrefresh(mode);
 	bool fs_vid_mode = false;
+	bool drr_active = false;
 
 	new_crtc_state->vrr_supported = new_con_state->freesync_capable &&
 					vrefresh >= aconnector->min_vfreq &&
 					vrefresh <= aconnector->max_vfreq;
 
-	if (new_crtc_state->vrr_supported) {
+	drr_active = new_crtc_state->vrr_supported &&
+		new_crtc_state->freesync_config.state != VRR_STATE_DISABLED &&
+		new_crtc_state->freesync_config.state != VRR_STATE_INACTIVE &&
+		new_crtc_state->freesync_config.state != VRR_STATE_UNSUPPORTED;
+
+	if (drr_active)
 		new_crtc_state->stream->ignore_msa_timing_param = true;
-		fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED;
 
+	if (new_crtc_state->vrr_supported) {
+		fs_vid_mode = new_crtc_state->freesync_config.state == VRR_STATE_ACTIVE_FIXED;
 		config.min_refresh_in_uhz = aconnector->min_vfreq * 1000000;
 		config.max_refresh_in_uhz = aconnector->max_vfreq * 1000000;
 		config.vsif_supported = true;
-- 
2.39.0

[PATCH 16/18] drm/amd/display: Adding braces to prepare for future changes to behavior of if block

[Aurabindo Pillai]

From: Leo Chen <sancchen@amd.com>

[Why & How]
For certain features, there will be more implementations needed in the if-block.
Braces are added as part of the preparation.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Leo Chen <sancchen@amd.com>
Reviewed-by: Charlene Liu <Charlene.Liu@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index f41933845d2a..dbf90ca0cf98 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2008,8 +2008,9 @@ bool dc_commit_state(struct dc *dc, struct dc_state *context)
 		return result == DC_OK;
 	}
 
-	if (!streams_changed(dc, context->streams, context->stream_count))
+	if (!streams_changed(dc, context->streams, context->stream_count)) {
 		return DC_OK;
+	}
 
 	DC_LOG_DC("%s: %d streams\n",
 				__func__, context->stream_count);
-- 
2.39.0

[PATCH 17/18] drm/amd/display: Reorder dc_state fields to optimize clearing the struct

[Aurabindo Pillai]

From: Aric Cyr <aric.cyr@amd.com>

[why & how]
By moving bw_ctx field to the end of the dc_state the state can be
cleared more efficiently without resulting in large DML memcpy
operations, resulting in better mode enumeration performance on some
platforms.

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Aric Cyr <aric.cyr@amd.com>
Reviewed-by: Nevenko Stupar <Nevenko.Stupar@amd.com>
---
 .../gpu/drm/amd/display/dc/inc/core_types.h    | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/inc/core_types.h b/drivers/gpu/drm/amd/display/dc/inc/core_types.h
index 525f8f0b8732..b093ea495468 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/core_types.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/core_types.h
@@ -546,15 +546,6 @@ struct dc_state {
 	 */
 	struct resource_context res_ctx;
 
-	/**
-	 * @bw_ctx: The output from bandwidth and watermark calculations and the DML
-	 *
-	 * Each context must have its own instance of VBA, and in order to
-	 * initialize and obtain IP and SOC, the base DML instance from DC is
-	 * initially copied into every context.
-	 */
-	struct bw_context bw_ctx;
-
 	/**
 	 * @pp_display_cfg: PowerPlay clocks and settings
 	 * Note: this is a big struct, do *not* put on stack!
@@ -569,6 +560,15 @@ struct dc_state {
 
 	struct clk_mgr *clk_mgr;
 
+	/**
+	 * @bw_ctx: The output from bandwidth and watermark calculations and the DML
+	 *
+	 * Each context must have its own instance of VBA, and in order to
+	 * initialize and obtain IP and SOC, the base DML instance from DC is
+	 * initially copied into every context.
+	 */
+	struct bw_context bw_ctx;
+
 	/**
 	 * @refcount: refcount reference
 	 *
-- 
2.39.0

[PATCH 18/18] drm/amd/display: 3.2.217

[Aurabindo Pillai]

From: Aric Cyr <aric.cyr@amd.com>

Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Aric Cyr <aric.cyr@amd.com>
---
 drivers/gpu/drm/amd/display/dc/dc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index 2896157b37da..0f7b7ccfcb40 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -47,7 +47,7 @@ struct aux_payload;
 struct set_config_cmd_payload;
 struct dmub_notification;
 
-#define DC_VER "3.2.216"
+#define DC_VER "3.2.217"
 
 #define MAX_SURFACES 3
 #define MAX_PLANES 6
-- 
2.39.0

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Deucher]

On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
<aurabindo.pillai@amd.com> wrote:
>
> From: Alex Hung <alex.hung@amd.com>
>
> [Why]
> When running IGT kms_bw test with DP monitor, some systems crash from
> msleep no matter how long or short the time is.
>
> [How]
> To replace msleep with mdelay.

Can you provide a bit more info about the crash?  A lot of platforms
don't support delay larger than 2-4ms so this change will generate
errors on ARM and possibly other platforms.

Alex

>
> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Signed-off-by: Alex Hung <alex.hung@amd.com>
> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> ---
>  drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> index 913a1fe6b3da..e6251ccadb70 100644
> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
>                          * After output is idle pattern some sinks need time to recognize the stream
>                          * has changed or they enter protection state and hang.
>                          */
> -                       msleep(60);
> +                       mdelay(60);
>                 } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
>                         if (!link->dc->config.edp_no_power_sequencing) {
>                                 /*
> --
> 2.39.0
>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Hung]

On 2022-12-14 13:48, Alex Deucher wrote:
> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
> <aurabindo.pillai@amd.com> wrote:
>>
>> From: Alex Hung <alex.hung@amd.com>
>>
>> [Why]
>> When running IGT kms_bw test with DP monitor, some systems crash from
>> msleep no matter how long or short the time is.
>>
>> [How]
>> To replace msleep with mdelay.
> 
> Can you provide a bit more info about the crash?  A lot of platforms
> don't support delay larger than 2-4ms so this change will generate
> errors on ARM and possibly other platforms.
> 
> Alex

The msleep was introduced in eec3303de3378 for non-compliant display 
port monitors but IGT's kms_bw test can cause a recent h/w to hang at 
msleep(60) when calling "igt_remove_fb" in IGT 
(https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_bw.c#L197)

It is possible to workaround this by reversing order of 
igt_remove_fb(&buffer[i]), as the following example:

   igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]

Hangs:
   igt_remove_fb with the order buffer[0], buffer[1], buffer[2]

No hangs:
   igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]

However, IGT simply exposes the problem and it makes more sense to stop 
the hang from occurring.

I also tried to remove the msleep completely and it also work, but I 
didn't want to break the fix for the original problematic hardware 
configuration.

> 
>>
>> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>> index 913a1fe6b3da..e6251ccadb70 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
>>                           * After output is idle pattern some sinks need time to recognize the stream
>>                           * has changed or they enter protection state and hang.
>>                           */
>> -                       msleep(60);
>> +                       mdelay(60);
>>                  } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
>>                          if (!link->dc->config.edp_no_power_sequencing) {
>>                                  /*
>> --
>> 2.39.0
>>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Deucher]

On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>
>
>
> On 2022-12-14 13:48, Alex Deucher wrote:
> > On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
> > <aurabindo.pillai@amd.com> wrote:
> >>
> >> From: Alex Hung <alex.hung@amd.com>
> >>
> >> [Why]
> >> When running IGT kms_bw test with DP monitor, some systems crash from
> >> msleep no matter how long or short the time is.
> >>
> >> [How]
> >> To replace msleep with mdelay.
> >
> > Can you provide a bit more info about the crash?  A lot of platforms
> > don't support delay larger than 2-4ms so this change will generate
> > errors on ARM and possibly other platforms.
> >
> > Alex
>
> The msleep was introduced in eec3303de3378 for non-compliant display
> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
> msleep(60) when calling "igt_remove_fb" in IGT
> (https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_bw.c#L197)
>
> It is possible to workaround this by reversing order of
> igt_remove_fb(&buffer[i]), as the following example:
>
>    igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>
> Hangs:
>    igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>
> No hangs:
>    igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>
> However, IGT simply exposes the problem and it makes more sense to stop
> the hang from occurring.
>
> I also tried to remove the msleep completely and it also work, but I
> didn't want to break the fix for the original problematic hardware
> configuration.

Why does sleep vs delay make a difference?  Is there some race that we
are not locking against?

Alex

>
> >
> >>
> >> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> >> Signed-off-by: Alex Hung <alex.hung@amd.com>
> >> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >> index 913a1fe6b3da..e6251ccadb70 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
> >>                           * After output is idle pattern some sinks need time to recognize the stream
> >>                           * has changed or they enter protection state and hang.
> >>                           */
> >> -                       msleep(60);
> >> +                       mdelay(60);
> >>                  } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
> >>                          if (!link->dc->config.edp_no_power_sequencing) {
> >>                                  /*
> >> --
> >> 2.39.0
> >>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Hung]

On 2022-12-14 14:54, Alex Deucher wrote:
> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>
>>
>>
>> On 2022-12-14 13:48, Alex Deucher wrote:
>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>> <aurabindo.pillai@amd.com> wrote:
>>>>
>>>> From: Alex Hung <alex.hung@amd.com>
>>>>
>>>> [Why]
>>>> When running IGT kms_bw test with DP monitor, some systems crash from
>>>> msleep no matter how long or short the time is.
>>>>
>>>> [How]
>>>> To replace msleep with mdelay.
>>>
>>> Can you provide a bit more info about the crash?  A lot of platforms
>>> don't support delay larger than 2-4ms so this change will generate
>>> errors on ARM and possibly other platforms.
>>>
>>> Alex
>>
>> The msleep was introduced in eec3303de3378 for non-compliant display
>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
>> msleep(60) when calling "igt_remove_fb" in IGT
>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%2F-%2Fblob%2Fmaster%2Ftests%2Fkms_bw.c%23L197&amp;data=05%7C01%7Calex.hung%40amd.com%7Cee0c28620f2447f13a8f08dade1dc0bc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638066516634100466%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mx33srt3RgcA4ZklYVXom9ZQklJjWmcywEtb4qJQVBQ%3D&amp;reserved=0)
>>
>> It is possible to workaround this by reversing order of
>> igt_remove_fb(&buffer[i]), as the following example:
>>
>>     igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>>
>> Hangs:
>>     igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>
>> No hangs:
>>     igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>>
>> However, IGT simply exposes the problem and it makes more sense to stop
>> the hang from occurring.
>>
>> I also tried to remove the msleep completely and it also work, but I
>> didn't want to break the fix for the original problematic hardware
>> configuration.
> 
> Why does sleep vs delay make a difference?  Is there some race that we
> are not locking against?
> 
> Alex
> 

That was my original thought but I did not find any previously. I will 
investigate it again.

If mdelay(>4) isn't usable on other platforms, is it an option to use 
mdelay on x86_64 only and keep msleep on other platforms or just remove 
the msleep for other platforms, something like

-                       msleep(60);
+#ifdef CONFIG_X86_64
+                       mdelay(60);
+#endif


>>
>>>
>>>>
>>>> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>>> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>> index 913a1fe6b3da..e6251ccadb70 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
>>>>                            * After output is idle pattern some sinks need time to recognize the stream
>>>>                            * has changed or they enter protection state and hang.
>>>>                            */
>>>> -                       msleep(60);
>>>> +                       mdelay(60);
>>>>                   } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
>>>>                           if (!link->dc->config.edp_no_power_sequencing) {
>>>>                                   /*
>>>> --
>>>> 2.39.0
>>>>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Deucher]

On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>
>
>
> On 2022-12-14 14:54, Alex Deucher wrote:
> > On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
> >>
> >>
> >>
> >> On 2022-12-14 13:48, Alex Deucher wrote:
> >>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
> >>> <aurabindo.pillai@amd.com> wrote:
> >>>>
> >>>> From: Alex Hung <alex.hung@amd.com>
> >>>>
> >>>> [Why]
> >>>> When running IGT kms_bw test with DP monitor, some systems crash from
> >>>> msleep no matter how long or short the time is.
> >>>>
> >>>> [How]
> >>>> To replace msleep with mdelay.
> >>>
> >>> Can you provide a bit more info about the crash?  A lot of platforms
> >>> don't support delay larger than 2-4ms so this change will generate
> >>> errors on ARM and possibly other platforms.
> >>>
> >>> Alex
> >>
> >> The msleep was introduced in eec3303de3378 for non-compliant display
> >> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
> >> msleep(60) when calling "igt_remove_fb" in IGT
> >> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%2F-%2Fblob%2Fmaster%2Ftests%2Fkms_bw.c%23L197&amp;data=05%7C01%7Calex.hung%40amd.com%7Cee0c28620f2447f13a8f08dade1dc0bc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638066516634100466%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mx33srt3RgcA4ZklYVXom9ZQklJjWmcywEtb4qJQVBQ%3D&amp;reserved=0)
> >>
> >> It is possible to workaround this by reversing order of
> >> igt_remove_fb(&buffer[i]), as the following example:
> >>
> >>     igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
> >>
> >> Hangs:
> >>     igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
> >>
> >> No hangs:
> >>     igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
> >>
> >> However, IGT simply exposes the problem and it makes more sense to stop
> >> the hang from occurring.
> >>
> >> I also tried to remove the msleep completely and it also work, but I
> >> didn't want to break the fix for the original problematic hardware
> >> configuration.
> >
> > Why does sleep vs delay make a difference?  Is there some race that we
> > are not locking against?
> >
> > Alex
> >
>
> That was my original thought but I did not find any previously. I will
> investigate it again.
>
> If mdelay(>4) isn't usable on other platforms, is it an option to use
> mdelay on x86_64 only and keep msleep on other platforms or just remove
> the msleep for other platforms, something like
>
> -                       msleep(60);
> +#ifdef CONFIG_X86_64
> +                       mdelay(60);
> +#endif

That's pretty ugly.  I'd rather try and resolve the root cause.  How
important is the IGT test?  What does it do?  Is the test itself
correct?

Alex


>
>
> >>
> >>>
> >>>>
> >>>> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> >>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
> >>>> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> >>>> ---
> >>>>    drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>>> index 913a1fe6b3da..e6251ccadb70 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>>> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
> >>>>                            * After output is idle pattern some sinks need time to recognize the stream
> >>>>                            * has changed or they enter protection state and hang.
> >>>>                            */
> >>>> -                       msleep(60);
> >>>> +                       mdelay(60);
> >>>>                   } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
> >>>>                           if (!link->dc->config.edp_no_power_sequencing) {
> >>>>                                   /*
> >>>> --
> >>>> 2.39.0
> >>>>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Hung]

On 2022-12-14 15:35, Alex Deucher wrote:
> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>>
>>
>>
>> On 2022-12-14 14:54, Alex Deucher wrote:
>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>> <aurabindo.pillai@amd.com> wrote:
>>>>>>
>>>>>> From: Alex Hung <alex.hung@amd.com>
>>>>>>
>>>>>> [Why]
>>>>>> When running IGT kms_bw test with DP monitor, some systems crash from
>>>>>> msleep no matter how long or short the time is.
>>>>>>
>>>>>> [How]
>>>>>> To replace msleep with mdelay.
>>>>>
>>>>> Can you provide a bit more info about the crash?  A lot of platforms
>>>>> don't support delay larger than 2-4ms so this change will generate
>>>>> errors on ARM and possibly other platforms.
>>>>>
>>>>> Alex
>>>>
>>>> The msleep was introduced in eec3303de3378 for non-compliant display
>>>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
>>>> msleep(60) when calling "igt_remove_fb" in IGT
>>>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%2F-%2Fblob%2Fmaster%2Ftests%2Fkms_bw.c%23L197&amp;data=05%7C01%7Calex.hung%40amd.com%7Cef40490c3d0f4a0448a808dade239493%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638066541657914573%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=kx7mnbuN%2BhmIPEgj4l1cuek0nvqK16Ig3fWAHopA8CI%3D&amp;reserved=0)
>>>>
>>>> It is possible to workaround this by reversing order of
>>>> igt_remove_fb(&buffer[i]), as the following example:
>>>>
>>>>      igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>>>>
>>>> Hangs:
>>>>      igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>>>
>>>> No hangs:
>>>>      igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>>>>
>>>> However, IGT simply exposes the problem and it makes more sense to stop
>>>> the hang from occurring.
>>>>
>>>> I also tried to remove the msleep completely and it also work, but I
>>>> didn't want to break the fix for the original problematic hardware
>>>> configuration.
>>>
>>> Why does sleep vs delay make a difference?  Is there some race that we
>>> are not locking against?
>>>
>>> Alex
>>>
>>
>> That was my original thought but I did not find any previously. I will
>> investigate it again.
>>
>> If mdelay(>4) isn't usable on other platforms, is it an option to use
>> mdelay on x86_64 only and keep msleep on other platforms or just remove
>> the msleep for other platforms, something like
>>
>> -                       msleep(60);
>> +#ifdef CONFIG_X86_64
>> +                       mdelay(60);
>> +#endif
> 
> That's pretty ugly.  I'd rather try and resolve the root cause.  How
> important is the IGT test?  What does it do?  Is the test itself
> correct?
> 

Agreed, and I didn't want to add conditions around the mdelay for the 
same reason. I will assume this is not an option now.

As in the previous comment, IGT can be modified to avoid the crash by 
reversing the order fb is removed - though I suspect I will receive 
questions why this is not fixed in kernel.

I wanted to fix this in kernel because nothing stops other user-space 
applications to use the same way to crash kernel, so fixing IGT is the 
second option.

Apparently causing problems on other platforms isn't an option at all so 
I will try to figure out an non-mdelay solution, and then maybe an IGT 
solution instead.

> Alex
> 
> 
>>
>>
>>>>
>>>>>
>>>>>>
>>>>>> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>>>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>>>>> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>> index 913a1fe6b3da..e6251ccadb70 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
>>>>>>                             * After output is idle pattern some sinks need time to recognize the stream
>>>>>>                             * has changed or they enter protection state and hang.
>>>>>>                             */
>>>>>> -                       msleep(60);
>>>>>> +                       mdelay(60);
>>>>>>                    } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
>>>>>>                            if (!link->dc->config.edp_no_power_sequencing) {
>>>>>>                                    /*
>>>>>> --
>>>>>> 2.39.0
>>>>>>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Deucher]

On Wed, Dec 14, 2022 at 5:56 PM Alex Hung <alex.hung@amd.com> wrote:
>
>
>
> On 2022-12-14 15:35, Alex Deucher wrote:
> > On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
> >>
> >>
> >>
> >> On 2022-12-14 14:54, Alex Deucher wrote:
> >>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2022-12-14 13:48, Alex Deucher wrote:
> >>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
> >>>>> <aurabindo.pillai@amd.com> wrote:
> >>>>>>
> >>>>>> From: Alex Hung <alex.hung@amd.com>
> >>>>>>
> >>>>>> [Why]
> >>>>>> When running IGT kms_bw test with DP monitor, some systems crash from
> >>>>>> msleep no matter how long or short the time is.
> >>>>>>
> >>>>>> [How]
> >>>>>> To replace msleep with mdelay.
> >>>>>
> >>>>> Can you provide a bit more info about the crash?  A lot of platforms
> >>>>> don't support delay larger than 2-4ms so this change will generate
> >>>>> errors on ARM and possibly other platforms.
> >>>>>
> >>>>> Alex
> >>>>
> >>>> The msleep was introduced in eec3303de3378 for non-compliant display
> >>>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
> >>>> msleep(60) when calling "igt_remove_fb" in IGT
> >>>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%2F-%2Fblob%2Fmaster%2Ftests%2Fkms_bw.c%23L197&amp;data=05%7C01%7Calex.hung%40amd.com%7Cef40490c3d0f4a0448a808dade239493%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638066541657914573%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=kx7mnbuN%2BhmIPEgj4l1cuek0nvqK16Ig3fWAHopA8CI%3D&amp;reserved=0)
> >>>>
> >>>> It is possible to workaround this by reversing order of
> >>>> igt_remove_fb(&buffer[i]), as the following example:
> >>>>
> >>>>      igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
> >>>>
> >>>> Hangs:
> >>>>      igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
> >>>>
> >>>> No hangs:
> >>>>      igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
> >>>>
> >>>> However, IGT simply exposes the problem and it makes more sense to stop
> >>>> the hang from occurring.
> >>>>
> >>>> I also tried to remove the msleep completely and it also work, but I
> >>>> didn't want to break the fix for the original problematic hardware
> >>>> configuration.
> >>>
> >>> Why does sleep vs delay make a difference?  Is there some race that we
> >>> are not locking against?
> >>>
> >>> Alex
> >>>
> >>
> >> That was my original thought but I did not find any previously. I will
> >> investigate it again.
> >>
> >> If mdelay(>4) isn't usable on other platforms, is it an option to use
> >> mdelay on x86_64 only and keep msleep on other platforms or just remove
> >> the msleep for other platforms, something like
> >>
> >> -                       msleep(60);
> >> +#ifdef CONFIG_X86_64
> >> +                       mdelay(60);
> >> +#endif
> >
> > That's pretty ugly.  I'd rather try and resolve the root cause.  How
> > important is the IGT test?  What does it do?  Is the test itself
> > correct?
> >
>
> Agreed, and I didn't want to add conditions around the mdelay for the
> same reason. I will assume this is not an option now.
>
> As in the previous comment, IGT can be modified to avoid the crash by
> reversing the order fb is removed - though I suspect I will receive
> questions why this is not fixed in kernel.
>
> I wanted to fix this in kernel because nothing stops other user-space
> applications to use the same way to crash kernel, so fixing IGT is the
> second option.
>
> Apparently causing problems on other platforms isn't an option at all so
> I will try to figure out an non-mdelay solution, and then maybe an IGT
> solution instead.
>

What hangs?  The test or the kernel or the hardware?

Alex


> > Alex
> >
> >
> >>
> >>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> >>>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
> >>>>>> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
> >>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>>>>> index 913a1fe6b3da..e6251ccadb70 100644
> >>>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >>>>>> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
> >>>>>>                             * After output is idle pattern some sinks need time to recognize the stream
> >>>>>>                             * has changed or they enter protection state and hang.
> >>>>>>                             */
> >>>>>> -                       msleep(60);
> >>>>>> +                       mdelay(60);
> >>>>>>                    } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
> >>>>>>                            if (!link->dc->config.edp_no_power_sequencing) {
> >>>>>>                                    /*
> >>>>>> --
> >>>>>> 2.39.0
> >>>>>>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Hung]

On 2022-12-14 16:06, Alex Deucher wrote:
> On Wed, Dec 14, 2022 at 5:56 PM Alex Hung <alex.hung@amd.com> wrote:
>>
>>
>>
>> On 2022-12-14 15:35, Alex Deucher wrote:
>>> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 2022-12-14 14:54, Alex Deucher wrote:
>>>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>>>> <aurabindo.pillai@amd.com> wrote:
>>>>>>>>
>>>>>>>> From: Alex Hung <alex.hung@amd.com>
>>>>>>>>
>>>>>>>> [Why]
>>>>>>>> When running IGT kms_bw test with DP monitor, some systems crash from
>>>>>>>> msleep no matter how long or short the time is.
>>>>>>>>
>>>>>>>> [How]
>>>>>>>> To replace msleep with mdelay.
>>>>>>>
>>>>>>> Can you provide a bit more info about the crash?  A lot of platforms
>>>>>>> don't support delay larger than 2-4ms so this change will generate
>>>>>>> errors on ARM and possibly other platforms.
>>>>>>>
>>>>>>> Alex
>>>>>>
>>>>>> The msleep was introduced in eec3303de3378 for non-compliant display
>>>>>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
>>>>>> msleep(60) when calling "igt_remove_fb" in IGT
>>>>>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%2F-%2Fblob%2Fmaster%2Ftests%2Fkms_bw.c%23L197&amp;data=05%7C01%7Calex.hung%40amd.com%7C81664450189542a7bbc408dade27d0e9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638066559844526853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=M%2BF4H2qddXItPoUZRVyhlc5N8UF1%2FHIh8PpfT%2BCmdZ4%3D&amp;reserved=0)
>>>>>>
>>>>>> It is possible to workaround this by reversing order of
>>>>>> igt_remove_fb(&buffer[i]), as the following example:
>>>>>>
>>>>>>       igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>
>>>>>> Hangs:
>>>>>>       igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>
>>>>>> No hangs:
>>>>>>       igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>>>>>>
>>>>>> However, IGT simply exposes the problem and it makes more sense to stop
>>>>>> the hang from occurring.
>>>>>>
>>>>>> I also tried to remove the msleep completely and it also work, but I
>>>>>> didn't want to break the fix for the original problematic hardware
>>>>>> configuration.
>>>>>
>>>>> Why does sleep vs delay make a difference?  Is there some race that we
>>>>> are not locking against?
>>>>>
>>>>> Alex
>>>>>
>>>>
>>>> That was my original thought but I did not find any previously. I will
>>>> investigate it again.
>>>>
>>>> If mdelay(>4) isn't usable on other platforms, is it an option to use
>>>> mdelay on x86_64 only and keep msleep on other platforms or just remove
>>>> the msleep for other platforms, something like
>>>>
>>>> -                       msleep(60);
>>>> +#ifdef CONFIG_X86_64
>>>> +                       mdelay(60);
>>>> +#endif
>>>
>>> That's pretty ugly.  I'd rather try and resolve the root cause.  How
>>> important is the IGT test?  What does it do?  Is the test itself
>>> correct?
>>>
>>
>> Agreed, and I didn't want to add conditions around the mdelay for the
>> same reason. I will assume this is not an option now.
>>
>> As in the previous comment, IGT can be modified to avoid the crash by
>> reversing the order fb is removed - though I suspect I will receive
>> questions why this is not fixed in kernel.
>>
>> I wanted to fix this in kernel because nothing stops other user-space
>> applications to use the same way to crash kernel, so fixing IGT is the
>> second option.
>>
>> Apparently causing problems on other platforms isn't an option at all so
>> I will try to figure out an non-mdelay solution, and then maybe an IGT
>> solution instead.
>>
> 
> What hangs?  The test or the kernel or the hardware?

The system becomes completely unresponsive - no keyboard, mouse nor 
remote accesses.

> 
> Alex
> 
> 
>>> Alex
>>>
>>>
>>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>>>>>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>>>>>>> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>>>> index 913a1fe6b3da..e6251ccadb70 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>>>> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
>>>>>>>>                              * After output is idle pattern some sinks need time to recognize the stream
>>>>>>>>                              * has changed or they enter protection state and hang.
>>>>>>>>                              */
>>>>>>>> -                       msleep(60);
>>>>>>>> +                       mdelay(60);
>>>>>>>>                     } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
>>>>>>>>                             if (!link->dc->config.edp_no_power_sequencing) {
>>>>>>>>                                     /*
>>>>>>>> --
>>>>>>>> 2.39.0
>>>>>>>>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Christian König]

Am 15.12.22 um 00:33 schrieb Alex Hung:
>
>
> On 2022-12-14 16:06, Alex Deucher wrote:
>> On Wed, Dec 14, 2022 at 5:56 PM Alex Hung <alex.hung@amd.com> wrote:
>>>
>>>
>>>
>>> On 2022-12-14 15:35, Alex Deucher wrote:
>>>> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2022-12-14 14:54, Alex Deucher wrote:
>>>>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>>>>> <aurabindo.pillai@amd.com> wrote:
>>>>>>>>>
>>>>>>>>> From: Alex Hung <alex.hung@amd.com>
>>>>>>>>>
>>>>>>>>> [Why]
>>>>>>>>> When running IGT kms_bw test with DP monitor, some systems 
>>>>>>>>> crash from
>>>>>>>>> msleep no matter how long or short the time is.
>>>>>>>>>
>>>>>>>>> [How]
>>>>>>>>> To replace msleep with mdelay.
>>>>>>>>
>>>>>>>> Can you provide a bit more info about the crash?  A lot of 
>>>>>>>> platforms
>>>>>>>> don't support delay larger than 2-4ms so this change will generate
>>>>>>>> errors on ARM and possibly other platforms.
>>>>>>>>
>>>>>>>> Alex
>>>>>>>
>>>>>>> The msleep was introduced in eec3303de3378 for non-compliant 
>>>>>>> display
>>>>>>> port monitors but IGT's kms_bw test can cause a recent h/w to 
>>>>>>> hang at
>>>>>>> msleep(60) when calling "igt_remove_fb" in IGT
>>>>>>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%2F-%2Fblob%2Fmaster%2Ftests%2Fkms_bw.c%23L197&amp;data=05%7C01%7Calex.hung%40amd.com%7C81664450189542a7bbc408dade27d0e9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638066559844526853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=M%2BF4H2qddXItPoUZRVyhlc5N8UF1%2FHIh8PpfT%2BCmdZ4%3D&amp;reserved=0) 
>>>>>>>
>>>>>>>
>>>>>>> It is possible to workaround this by reversing order of
>>>>>>> igt_remove_fb(&buffer[i]), as the following example:
>>>>>>>
>>>>>>>       igt_create_color_fb with the order buffer[0], buffer[1], 
>>>>>>> buffer[2]
>>>>>>>
>>>>>>> Hangs:
>>>>>>>       igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>
>>>>>>> No hangs:
>>>>>>>       igt_remove_fb with the reversed order buffer[2], 
>>>>>>> buffer[1], buffer[0]
>>>>>>>
>>>>>>> However, IGT simply exposes the problem and it makes more sense 
>>>>>>> to stop
>>>>>>> the hang from occurring.
>>>>>>>
>>>>>>> I also tried to remove the msleep completely and it also work, 
>>>>>>> but I
>>>>>>> didn't want to break the fix for the original problematic hardware
>>>>>>> configuration.
>>>>>>
>>>>>> Why does sleep vs delay make a difference?  Is there some race 
>>>>>> that we
>>>>>> are not locking against?
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>
>>>>> That was my original thought but I did not find any previously. I 
>>>>> will
>>>>> investigate it again.
>>>>>
>>>>> If mdelay(>4) isn't usable on other platforms, is it an option to use
>>>>> mdelay on x86_64 only and keep msleep on other platforms or just 
>>>>> remove
>>>>> the msleep for other platforms, something like
>>>>>
>>>>> -                       msleep(60);
>>>>> +#ifdef CONFIG_X86_64
>>>>> +                       mdelay(60);
>>>>> +#endif
>>>>
>>>> That's pretty ugly.  I'd rather try and resolve the root cause.  How
>>>> important is the IGT test?  What does it do?  Is the test itself
>>>> correct?
>>>>
>>>
>>> Agreed, and I didn't want to add conditions around the mdelay for the
>>> same reason. I will assume this is not an option now.
>>>
>>> As in the previous comment, IGT can be modified to avoid the crash by
>>> reversing the order fb is removed - though I suspect I will receive
>>> questions why this is not fixed in kernel.
>>>
>>> I wanted to fix this in kernel because nothing stops other user-space
>>> applications to use the same way to crash kernel, so fixing IGT is the
>>> second option.
>>>
>>> Apparently causing problems on other platforms isn't an option at 
>>> all so
>>> I will try to figure out an non-mdelay solution, and then maybe an IGT
>>> solution instead.
>>>
>>
>> What hangs?  The test or the kernel or the hardware?
>
> The system becomes completely unresponsive - no keyboard, mouse nor 
> remote accesses.

I agree with Alex that changing this is extremely questionable and not 
justified at all.

My educated guess is that by using mdelay() instead of msleep() we keep 
the CPU core busy and preventing something from happening at the same 
time as something else.

This clearly points to missing locking or similar to protect concurrent 
execution of things.

Christian.

>
>>
>> Alex
>>
>>
>>>> Alex
>>>>
>>>>
>>>>>
>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
>>>>>>>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>>>>>>>>> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 
>>>>>>>>> 2 +-
>>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git 
>>>>>>>>> a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c 
>>>>>>>>> b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>>>>> index 913a1fe6b3da..e6251ccadb70 100644
>>>>>>>>> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
>>>>>>>>> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx 
>>>>>>>>> *pipe_ctx)
>>>>>>>>>                              * After output is idle pattern 
>>>>>>>>> some sinks need time to recognize the stream
>>>>>>>>>                              * has changed or they enter 
>>>>>>>>> protection state and hang.
>>>>>>>>>                              */
>>>>>>>>> -                       msleep(60);
>>>>>>>>> +                       mdelay(60);
>>>>>>>>>                     } else if (pipe_ctx->stream->signal == 
>>>>>>>>> SIGNAL_TYPE_EDP) {
>>>>>>>>>                             if 
>>>>>>>>> (!link->dc->config.edp_no_power_sequencing) {
>>>>>>>>>                                     /*
>>>>>>>>> -- 
>>>>>>>>> 2.39.0
>>>>>>>>>

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Michel Dänzer]

On 12/15/22 09:09, Christian König wrote:
> Am 15.12.22 um 00:33 schrieb Alex Hung:
>> On 2022-12-14 16:06, Alex Deucher wrote:
>>> On Wed, Dec 14, 2022 at 5:56 PM Alex Hung <alex.hung@amd.com> wrote:
>>>> On 2022-12-14 15:35, Alex Deucher wrote:
>>>>> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>> On 2022-12-14 14:54, Alex Deucher wrote:
>>>>>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>>>>>> <aurabindo.pillai@amd.com> wrote:
>>>>>>>>>>
>>>>>>>>>> From: Alex Hung <alex.hung@amd.com>
>>>>>>>>>>
>>>>>>>>>> [Why]
>>>>>>>>>> When running IGT kms_bw test with DP monitor, some systems crash from
>>>>>>>>>> msleep no matter how long or short the time is.
>>>>>>>>>>
>>>>>>>>>> [How]
>>>>>>>>>> To replace msleep with mdelay.
>>>>>>>>>
>>>>>>>>> Can you provide a bit more info about the crash?  A lot of platforms
>>>>>>>>> don't support delay larger than 2-4ms so this change will generate
>>>>>>>>> errors on ARM and possibly other platforms.
>>>>>>>>>
>>>>>>>>> Alex
>>>>>>>>
>>>>>>>> The msleep was introduced in eec3303de3378 for non-compliant display
>>>>>>>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
>>>>>>>> msleep(60) when calling "igt_remove_fb" in IGT
>>>>>>>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%2F-%2Fblob%2Fmaster%2Ftests%2Fkms_bw.c%23L197&amp;data=05%7C01%7Calex.hung%40amd.com%7C81664450189542a7bbc408dade27d0e9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C638066559844526853%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=M%2BF4H2qddXItPoUZRVyhlc5N8UF1%2FHIh8PpfT%2BCmdZ4%3D&amp;reserved=0)
>>>>>>>>
>>>>>>>> It is possible to workaround this by reversing order of
>>>>>>>> igt_remove_fb(&buffer[i]), as the following example:
>>>>>>>>
>>>>>>>>       igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>
>>>>>>>> Hangs:
>>>>>>>>       igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>
>>>>>>>> No hangs:
>>>>>>>>       igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>>>>>>>>
>>>>>>>> However, IGT simply exposes the problem and it makes more sense to stop
>>>>>>>> the hang from occurring.
>>>>>>>>
>>>>>>>> I also tried to remove the msleep completely and it also work, but I
>>>>>>>> didn't want to break the fix for the original problematic hardware
>>>>>>>> configuration.
>>>>>>>
>>>>>>> Why does sleep vs delay make a difference?  Is there some race that we
>>>>>>> are not locking against?
>>>>>>>
>>>>>>> Alex
>>>>>>>
>>>>>>
>>>>>> That was my original thought but I did not find any previously. I will
>>>>>> investigate it again.
>>>>>>
>>>>>> If mdelay(>4) isn't usable on other platforms, is it an option to use
>>>>>> mdelay on x86_64 only and keep msleep on other platforms or just remove
>>>>>> the msleep for other platforms, something like
>>>>>>
>>>>>> -                       msleep(60);
>>>>>> +#ifdef CONFIG_X86_64
>>>>>> +                       mdelay(60);
>>>>>> +#endif
>>>>>
>>>>> That's pretty ugly.  I'd rather try and resolve the root cause.  How
>>>>> important is the IGT test?  What does it do?  Is the test itself
>>>>> correct?
>>>>>
>>>>
>>>> Agreed, and I didn't want to add conditions around the mdelay for the
>>>> same reason. I will assume this is not an option now.
>>>>
>>>> As in the previous comment, IGT can be modified to avoid the crash by
>>>> reversing the order fb is removed - though I suspect I will receive
>>>> questions why this is not fixed in kernel.
>>>>
>>>> I wanted to fix this in kernel because nothing stops other user-space
>>>> applications to use the same way to crash kernel, so fixing IGT is the
>>>> second option.
>>>>
>>>> Apparently causing problems on other platforms isn't an option at all so
>>>> I will try to figure out an non-mdelay solution, and then maybe an IGT
>>>> solution instead.
>>>
>>> What hangs?  The test or the kernel or the hardware?
>>
>> The system becomes completely unresponsive - no keyboard, mouse nor remote accesses.
> 
> I agree with Alex that changing this is extremely questionable and not justified at all.
> 
> My educated guess is that by using mdelay() instead of msleep() we keep the CPU core busy and preventing something from happening at the same time as something else.
> 
> This clearly points to missing locking or similar to protect concurrent execution of things.
Might another possibility be that this code gets called from an atomic context which can't sleep?


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Harry Wentland]

On 12/15/22 05:29, Michel Dänzer wrote:
> On 12/15/22 09:09, Christian König wrote:
>> Am 15.12.22 um 00:33 schrieb Alex Hung:
>>> On 2022-12-14 16:06, Alex Deucher wrote:
>>>> On Wed, Dec 14, 2022 at 5:56 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>> On 2022-12-14 15:35, Alex Deucher wrote:
>>>>>> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>> On 2022-12-14 14:54, Alex Deucher wrote:
>>>>>>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>>>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>>>>>>> <aurabindo.pillai@amd.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> From: Alex Hung <alex.hung@amd.com>
>>>>>>>>>>>
>>>>>>>>>>> [Why]
>>>>>>>>>>> When running IGT kms_bw test with DP monitor, some systems crash from
>>>>>>>>>>> msleep no matter how long or short the time is.
>>>>>>>>>>>
>>>>>>>>>>> [How]
>>>>>>>>>>> To replace msleep with mdelay.
>>>>>>>>>>
>>>>>>>>>> Can you provide a bit more info about the crash?  A lot of platforms
>>>>>>>>>> don't support delay larger than 2-4ms so this change will generate
>>>>>>>>>> errors on ARM and possibly other platforms.
>>>>>>>>>>
>>>>>>>>>> Alex
>>>>>>>>>
>>>>>>>>> The msleep was introduced in eec3303de3378 for non-compliant display
>>>>>>>>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
>>>>>>>>> msleep(60) when calling "igt_remove_fb" in IGT
>>>>>>>>> (https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_bw.c#L197>>>>>>>>>>
>>>>>>>>> It is possible to workaround this by reversing order of
>>>>>>>>> igt_remove_fb(&buffer[i]), as the following example:
>>>>>>>>>
>>>>>>>>>       igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>>
>>>>>>>>> Hangs:
>>>>>>>>>       igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>>
>>>>>>>>> No hangs:
>>>>>>>>>       igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>>>>>>>>>
>>>>>>>>> However, IGT simply exposes the problem and it makes more sense to stop
>>>>>>>>> the hang from occurring.
>>>>>>>>>
>>>>>>>>> I also tried to remove the msleep completely and it also work, but I
>>>>>>>>> didn't want to break the fix for the original problematic hardware
>>>>>>>>> configuration.
>>>>>>>>
>>>>>>>> Why does sleep vs delay make a difference?  Is there some race that we
>>>>>>>> are not locking against?
>>>>>>>>
>>>>>>>> Alex
>>>>>>>>
>>>>>>>
>>>>>>> That was my original thought but I did not find any previously. I will
>>>>>>> investigate it again.
>>>>>>>
>>>>>>> If mdelay(>4) isn't usable on other platforms, is it an option to use
>>>>>>> mdelay on x86_64 only and keep msleep on other platforms or just remove
>>>>>>> the msleep for other platforms, something like
>>>>>>>
>>>>>>> -                       msleep(60);
>>>>>>> +#ifdef CONFIG_X86_64
>>>>>>> +                       mdelay(60);
>>>>>>> +#endif
>>>>>>
>>>>>> That's pretty ugly.  I'd rather try and resolve the root cause.  How
>>>>>> important is the IGT test?  What does it do?  Is the test itself
>>>>>> correct?
>>>>>>
>>>>>
>>>>> Agreed, and I didn't want to add conditions around the mdelay for the
>>>>> same reason. I will assume this is not an option now.
>>>>>
>>>>> As in the previous comment, IGT can be modified to avoid the crash by
>>>>> reversing the order fb is removed - though I suspect I will receive
>>>>> questions why this is not fixed in kernel.
>>>>>
>>>>> I wanted to fix this in kernel because nothing stops other user-space
>>>>> applications to use the same way to crash kernel, so fixing IGT is the
>>>>> second option.
>>>>>
>>>>> Apparently causing problems on other platforms isn't an option at all so
>>>>> I will try to figure out an non-mdelay solution, and then maybe an IGT
>>>>> solution instead.
>>>>
>>>> What hangs?  The test or the kernel or the hardware?
>>>
>>> The system becomes completely unresponsive - no keyboard, mouse nor remote accesses.
>>
>> I agree with Alex that changing this is extremely questionable and not justified at all.
>>
>> My educated guess is that by using mdelay() instead of msleep() we keep the CPU core busy and preventing something from happening at the same time as something else.
>>
>> This clearly points to missing locking or similar to protect concurrent execution of things.
> Might another possibility be that this code gets called from an atomic context which can't sleep?
> 
> 

It can come through handle_hpd_rx_irq but we're using a workqueue
to queue interrupt handling so this shouldn't come from an atomic
context. I currently don't see where else it might be used in an
atomic context. Alex Hung, can you do a dump_stack() in this function
to see where the problematic call is coming from?

Fixing IGT will only mask the issue. Userspace should never be able
to put the system in a state where it stops responding entirely. This
will need some sort of fix in the kernel.

Harry

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Aurabindo Pillai]

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


On 12/15/22 10:17, Harry Wentland wrote:
>
> On 12/15/22 05:29, Michel Dänzer wrote:
>> On 12/15/22 09:09, Christian König wrote:
>>> Am 15.12.22 um 00:33 schrieb Alex Hung:
>>>> On 2022-12-14 16:06, Alex Deucher wrote:
>>>>> On Wed, Dec 14, 2022 at 5:56 PM Alex Hung<alex.hung@amd.com>  wrote:
>>>>>> On 2022-12-14 15:35, Alex Deucher wrote:
>>>>>>> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung<alex.hung@amd.com>  wrote:
>>>>>>>> On 2022-12-14 14:54, Alex Deucher wrote:
>>>>>>>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung<alex.hung@amd.com>  wrote:
>>>>>>>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>>>>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>>>>>>>> <aurabindo.pillai@amd.com>  wrote:
>>>>>>>>>>>> From: Alex Hung<alex.hung@amd.com>
>>>>>>>>>>>>
>>>>>>>>>>>> [Why]
>>>>>>>>>>>> When running IGT kms_bw test with DP monitor, some systems crash from
>>>>>>>>>>>> msleep no matter how long or short the time is.
>>>>>>>>>>>>
>>>>>>>>>>>> [How]
>>>>>>>>>>>> To replace msleep with mdelay.
>>>>>>>>>>> Can you provide a bit more info about the crash?  A lot of platforms
>>>>>>>>>>> don't support delay larger than 2-4ms so this change will generate
>>>>>>>>>>> errors on ARM and possibly other platforms.
>>>>>>>>>>>
>>>>>>>>>>> Alex
>>>>>>>>>> The msleep was introduced in eec3303de3378 for non-compliant display
>>>>>>>>>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
>>>>>>>>>> msleep(60) when calling "igt_remove_fb" in IGT
>>>>>>>>>> (https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_bw.c#L197>>>>>>>>>>
>>>>>>>>>> It is possible to workaround this by reversing order of
>>>>>>>>>> igt_remove_fb(&buffer[i]), as the following example:
>>>>>>>>>>
>>>>>>>>>>        igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>>>
>>>>>>>>>> Hangs:
>>>>>>>>>>        igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>>>
>>>>>>>>>> No hangs:
>>>>>>>>>>        igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>>>>>>>>>>
>>>>>>>>>> However, IGT simply exposes the problem and it makes more sense to stop
>>>>>>>>>> the hang from occurring.
>>>>>>>>>>
>>>>>>>>>> I also tried to remove the msleep completely and it also work, but I
>>>>>>>>>> didn't want to break the fix for the original problematic hardware
>>>>>>>>>> configuration.
>>>>>>>>> Why does sleep vs delay make a difference?  Is there some race that we
>>>>>>>>> are not locking against?
>>>>>>>>>
>>>>>>>>> Alex
>>>>>>>>>
>>>>>>>> That was my original thought but I did not find any previously. I will
>>>>>>>> investigate it again.
>>>>>>>>
>>>>>>>> If mdelay(>4) isn't usable on other platforms, is it an option to use
>>>>>>>> mdelay on x86_64 only and keep msleep on other platforms or just remove
>>>>>>>> the msleep for other platforms, something like
>>>>>>>>
>>>>>>>> -                       msleep(60);
>>>>>>>> +#ifdef CONFIG_X86_64
>>>>>>>> +                       mdelay(60);
>>>>>>>> +#endif
>>>>>>> That's pretty ugly.  I'd rather try and resolve the root cause.  How
>>>>>>> important is the IGT test?  What does it do?  Is the test itself
>>>>>>> correct?
>>>>>>>
>>>>>> Agreed, and I didn't want to add conditions around the mdelay for the
>>>>>> same reason. I will assume this is not an option now.
>>>>>>
>>>>>> As in the previous comment, IGT can be modified to avoid the crash by
>>>>>> reversing the order fb is removed - though I suspect I will receive
>>>>>> questions why this is not fixed in kernel.
>>>>>>
>>>>>> I wanted to fix this in kernel because nothing stops other user-space
>>>>>> applications to use the same way to crash kernel, so fixing IGT is the
>>>>>> second option.
>>>>>>
>>>>>> Apparently causing problems on other platforms isn't an option at all so
>>>>>> I will try to figure out an non-mdelay solution, and then maybe an IGT
>>>>>> solution instead.
>>>>> What hangs?  The test or the kernel or the hardware?
>>>> The system becomes completely unresponsive - no keyboard, mouse nor remote accesses.
>>> I agree with Alex that changing this is extremely questionable and not justified at all.
>>>
>>> My educated guess is that by using mdelay() instead of msleep() we keep the CPU core busy and preventing something from happening at the same time as something else.
>>>
>>> This clearly points to missing locking or similar to protect concurrent execution of things.
>> Might another possibility be that this code gets called from an atomic context which can't sleep?
>>
>>
> It can come through handle_hpd_rx_irq but we're using a workqueue
> to queue interrupt handling so this shouldn't come from an atomic
> context. I currently don't see where else it might be used in an
> atomic context. Alex Hung, can you do a dump_stack() in this function
> to see where the problematic call is coming from?
>
> Fixing IGT will only mask the issue. Userspace should never be able
> to put the system in a state where it stops responding entirely. This
> will need some sort of fix in the kernel.
>
> Harry
>
>
I will drop this patch from the series.

[-- Attachment #2: Type: text/html, Size: 7790 bytes --]

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Hung]

On 2022-12-15 08:17, Harry Wentland wrote:
> 
> 
> On 12/15/22 05:29, Michel Dänzer wrote:
>> On 12/15/22 09:09, Christian König wrote:
>>> Am 15.12.22 um 00:33 schrieb Alex Hung:
>>>> On 2022-12-14 16:06, Alex Deucher wrote:
>>>>> On Wed, Dec 14, 2022 at 5:56 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>> On 2022-12-14 15:35, Alex Deucher wrote:
>>>>>>> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>>> On 2022-12-14 14:54, Alex Deucher wrote:
>>>>>>>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>>>>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>>>>>>>> <aurabindo.pillai@amd.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> From: Alex Hung <alex.hung@amd.com>
>>>>>>>>>>>>
>>>>>>>>>>>> [Why]
>>>>>>>>>>>> When running IGT kms_bw test with DP monitor, some systems crash from
>>>>>>>>>>>> msleep no matter how long or short the time is.
>>>>>>>>>>>>
>>>>>>>>>>>> [How]
>>>>>>>>>>>> To replace msleep with mdelay.
>>>>>>>>>>>
>>>>>>>>>>> Can you provide a bit more info about the crash?  A lot of platforms
>>>>>>>>>>> don't support delay larger than 2-4ms so this change will generate
>>>>>>>>>>> errors on ARM and possibly other platforms.
>>>>>>>>>>>
>>>>>>>>>>> Alex
>>>>>>>>>>
>>>>>>>>>> The msleep was introduced in eec3303de3378 for non-compliant display
>>>>>>>>>> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
>>>>>>>>>> msleep(60) when calling "igt_remove_fb" in IGT
>>>>>>>>>> (https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_bw.c#L197>>>>>>>>>>
>>>>>>>>>> It is possible to workaround this by reversing order of
>>>>>>>>>> igt_remove_fb(&buffer[i]), as the following example:
>>>>>>>>>>
>>>>>>>>>>        igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>>>
>>>>>>>>>> Hangs:
>>>>>>>>>>        igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>>>>>>>>>>
>>>>>>>>>> No hangs:
>>>>>>>>>>        igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>>>>>>>>>>
>>>>>>>>>> However, IGT simply exposes the problem and it makes more sense to stop
>>>>>>>>>> the hang from occurring.
>>>>>>>>>>
>>>>>>>>>> I also tried to remove the msleep completely and it also work, but I
>>>>>>>>>> didn't want to break the fix for the original problematic hardware
>>>>>>>>>> configuration.
>>>>>>>>>
>>>>>>>>> Why does sleep vs delay make a difference?  Is there some race that we
>>>>>>>>> are not locking against?
>>>>>>>>>
>>>>>>>>> Alex
>>>>>>>>>
>>>>>>>>
>>>>>>>> That was my original thought but I did not find any previously. I will
>>>>>>>> investigate it again.
>>>>>>>>
>>>>>>>> If mdelay(>4) isn't usable on other platforms, is it an option to use
>>>>>>>> mdelay on x86_64 only and keep msleep on other platforms or just remove
>>>>>>>> the msleep for other platforms, something like
>>>>>>>>
>>>>>>>> -                       msleep(60);
>>>>>>>> +#ifdef CONFIG_X86_64
>>>>>>>> +                       mdelay(60);
>>>>>>>> +#endif
>>>>>>>
>>>>>>> That's pretty ugly.  I'd rather try and resolve the root cause.  How
>>>>>>> important is the IGT test?  What does it do?  Is the test itself
>>>>>>> correct?
>>>>>>>
>>>>>>
>>>>>> Agreed, and I didn't want to add conditions around the mdelay for the
>>>>>> same reason. I will assume this is not an option now.
>>>>>>
>>>>>> As in the previous comment, IGT can be modified to avoid the crash by
>>>>>> reversing the order fb is removed - though I suspect I will receive
>>>>>> questions why this is not fixed in kernel.
>>>>>>
>>>>>> I wanted to fix this in kernel because nothing stops other user-space
>>>>>> applications to use the same way to crash kernel, so fixing IGT is the
>>>>>> second option.
>>>>>>
>>>>>> Apparently causing problems on other platforms isn't an option at all so
>>>>>> I will try to figure out an non-mdelay solution, and then maybe an IGT
>>>>>> solution instead.
>>>>>
>>>>> What hangs?  The test or the kernel or the hardware?
>>>>
>>>> The system becomes completely unresponsive - no keyboard, mouse nor remote accesses.
>>>
>>> I agree with Alex that changing this is extremely questionable and not justified at all.
>>>
>>> My educated guess is that by using mdelay() instead of msleep() we keep the CPU core busy and preventing something from happening at the same time as something else.
>>>
>>> This clearly points to missing locking or similar to protect concurrent execution of things.
>> Might another possibility be that this code gets called from an atomic context which can't sleep?
>>
>>
> 
> It can come through handle_hpd_rx_irq but we're using a workqueue
> to queue interrupt handling so this shouldn't come from an atomic
> context. I currently don't see where else it might be used in an
> atomic context. Alex Hung, can you do a dump_stack() in this function
> to see where the problematic call is coming from?


IGT's kms_bw executes as below (when passing)

IGT-Version: 1.26-gf4067678 (x86_64) (Linux: 5.19.0-99-custom x86_64)
Starting subtest: linear-tiling-1-displays-1920x1080p
Subtest linear-tiling-1-displays-1920x1080p: SUCCESS (0.225s)
Starting subtest: linear-tiling-1-displays-2560x1440p
Subtest linear-tiling-1-displays-2560x1440p: SUCCESS (0.111s)
Starting subtest: linear-tiling-1-displays-3840x2160p
Subtest linear-tiling-1-displays-3840x2160p: SUCCESS (0.118s)
Starting subtest: linear-tiling-2-displays-1920x1080p
Subtest linear-tiling-2-displays-1920x1080p: SUCCESS (0.409s)
Starting subtest: linear-tiling-2-displays-2560x1440p
Subtest linear-tiling-2-displays-2560x1440p: SUCCESS (0.417s)
Starting subtest: linear-tiling-2-displays-3840x2160p
Subtest linear-tiling-2-displays-3840x2160p: SUCCESS (0.444s)
Starting subtest: linear-tiling-3-displays-1920x1080p
Subtest linear-tiling-3-displays-1920x1080p: SUCCESS (0.547s)
Starting subtest: linear-tiling-3-displays-2560x1440p
Subtest linear-tiling-3-displays-2560x1440p: SUCCESS (0.555s)
Starting subtest: linear-tiling-3-displays-3840x2160p
Subtest linear-tiling-3-displays-3840x2160p: SUCCESS (0.586s)
Starting subtest: linear-tiling-4-displays-1920x1080p
Subtest linear-tiling-4-displays-1920x1080p: SUCCESS (0.734s)
Starting subtest: linear-tiling-4-displays-2560x1440p
Subtest linear-tiling-4-displays-2560x1440p: SUCCESS (0.742s)
Starting subtest: linear-tiling-4-displays-3840x2160p
Subtest linear-tiling-4-displays-3840x2160p: SUCCESS (0.778s)
Starting subtest: linear-tiling-5-displays-1920x1080p
Subtest linear-tiling-5-displays-1920x1080p: SUCCESS (0.734s)
Starting subtest: linear-tiling-5-displays-2560x1440p
Subtest linear-tiling-5-displays-2560x1440p: SUCCESS (0.743s)
Starting subtest: linear-tiling-5-displays-3840x2160p
Subtest linear-tiling-5-displays-3840x2160p: SUCCESS (0.781s)
Starting subtest: linear-tiling-6-displays-1920x1080p
Test requirement not met in function run_test_linear_tiling, file 
../tests/kms_bw.c:156:
Test requirement: !(pipe > num_pipes)
ASIC does not have 5 pipes
Subtest linear-tiling-6-displays-1920x1080p: SKIP (0.000s)
Starting subtest: linear-tiling-6-displays-2560x1440p
Test requirement not met in function run_test_linear_tiling, file 
../tests/kms_bw.c:156:
Test requirement: !(pipe > num_pipes)
ASIC does not have 5 pipes
Subtest linear-tiling-6-displays-2560x1440p: SKIP (0.000s)
Starting subtest: linear-tiling-6-displays-3840x2160p
Test requirement not met in function run_test_linear_tiling, file 
../tests/kms_bw.c:156:
Test requirement: !(pipe > num_pipes)
ASIC does not have 5 pipes
Subtest linear-tiling-6-displays-3840x2160p: SKIP (0.000s)

The crash usually occurs when executing 
"linear-tiling-3-displays-1920x1080p" most of time, but the crash can 
also occurs at "linear-tiling-3-displays-2560x1440p"

============
This is dump_stack right before the failing msleep.

[IGT] kms_bw: starting subtest linear-tiling-3-displays-1920x1080p
CPU: 1 PID: 76 Comm: kworker/1:1 Not tainted 5.19.0-99-custom #126
Workqueue: events drm_mode_rmfb_work_fn [drm]
Call Trace:
  <TASK>
  dump_stack_lvl+0x49/0x63
  dump_stack+0x10/0x16
  dce110_blank_stream.cold+0x5/0x14 [amdgpu]
  core_link_disable_stream+0xe0/0x6b0 [amdgpu]
  ? optc1_set_vtotal_min_max+0x6b/0x80 [amdgpu]
  dcn31_reset_hw_ctx_wrap+0x229/0x410 [amdgpu]
  dce110_apply_ctx_to_hw+0x6e/0x6c0 [amdgpu]
  ? dcn20_plane_atomic_disable+0xb2/0x160 [amdgpu]
  ? dcn20_disable_plane+0x2c/0x60 [amdgpu]
  ? dcn20_post_unlock_program_front_end+0x77/0x2c0 [amdgpu]
  dc_commit_state_no_check+0x39a/0xcd0 [amdgpu]
  ? dc_validate_global_state+0x2ba/0x330 [amdgpu]
  dc_commit_state+0x10f/0x150 [amdgpu]
  amdgpu_dm_atomic_commit_tail+0x631/0x2d30 [amdgpu]
  ? dcn30_internal_validate_bw+0x349/0xa00 [amdgpu]
  ? slab_post_alloc_hook+0x53/0x270
  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
  ? dc_validate_global_state+0x27a/0x330 [amdgpu]
  ? slab_post_alloc_hook+0x53/0x270
  ? __kmalloc+0x18c/0x300
  ? drm_dp_mst_atomic_setup_commit+0x8a/0x1a0 [drm_display_helper]
  ? preempt_count_add+0x7c/0xc0
  ? _raw_spin_unlock_irq+0x1f/0x40
  ? wait_for_completion_timeout+0x114/0x140
  ? preempt_count_add+0x7c/0xc0
  ? _raw_spin_unlock_irq+0x1f/0x40
  commit_tail+0x99/0x140 [drm_kms_helper]
  drm_atomic_helper_commit+0x12b/0x150 [drm_kms_helper]
  drm_atomic_commit+0x79/0x100 [drm]
  ? drm_plane_get_damage_clips.cold+0x1d/0x1d [drm]
  drm_framebuffer_remove+0x499/0x510 [drm]
  drm_mode_rmfb_work_fn+0x4b/0x90 [drm]
  process_one_work+0x21d/0x3f0
  worker_thread+0x1fa/0x3c0
  ? process_one_work+0x3f0/0x3f0
  kthread+0xff/0x130
  ? kthread_complete_and_exit+0x20/0x20
  ret_from_fork+0x22/0x30
  </TASK>


============
If msleep is replaced by mdelay, the dump_stack is almost the same:

$ diff mdelay.log msleep.log
<  dce110_blank_stream.cold+0x5/0x1f [amdgpu]
---
 >  dce110_blank_stream.cold+0x5/0x14 [amdgpu]


============
If the IGT fix is applied (i.e. no crashes when removing buffers[i] 
reversely by "igt_remove_fb", the dump_stack outputs are the same:

$ diff msleep_igt.log msleep.log
2c2
< CPU: 6 PID: 78 Comm: kworker/6:1 Not tainted 5.19.0-99-custom #126
---
 > CPU: 1 PID: 76 Comm: kworker/1:1 Not tainted 5.19.0-99-custom #126

============
Note the msleep doesn't always trigger crashes. One of the passing 
dump_stack is as below:

[IGT] kms_bw: starting subtest linear-tiling-2-displays-1920x1080p
CPU: 6 PID: 78 Comm: kworker/6:1 Not tainted 5.19.0-99-custom #126
Workqueue: events drm_mode_rmfb_work_fn [drm]
Call Trace:
  <TASK>
  dump_stack_lvl+0x49/0x63
  dump_stack+0x10/0x16
  dce110_blank_stream.cold+0x5/0x14 [amdgpu]
  core_link_disable_stream+0xe0/0x6b0 [amdgpu]
  ? optc1_set_vtotal_min_max+0x6b/0x80 [amdgpu]
  dcn31_reset_hw_ctx_wrap+0x229/0x410 [amdgpu]
  dce110_apply_ctx_to_hw+0x6e/0x6c0 [amdgpu]
  ? dcn20_plane_atomic_disable+0xb2/0x160 [amdgpu]
  ? dcn20_disable_plane+0x2c/0x60 [amdgpu]
  ? dcn20_post_unlock_program_front_end+0x77/0x2c0 [amdgpu]
  dc_commit_state_no_check+0x39a/0xcd0 [amdgpu]
  ? dc_validate_global_state+0x2ba/0x330 [amdgpu]
  dc_commit_state+0x10f/0x150 [amdgpu]
  amdgpu_dm_atomic_commit_tail+0x631/0x2d30 [amdgpu]
  ? debug_smp_processor_id+0x17/0x20
  ? dc_fpu_end+0x4e/0xd0 [amdgpu]
  ? dcn316_populate_dml_pipes_from_context+0x69/0x2a0 [amdgpu]
  ? dcn31_calculate_wm_and_dlg_fp+0x50/0x530 [amdgpu]
  ? kernel_fpu_end+0x26/0x50
  ? dc_fpu_end+0x4e/0xd0 [amdgpu]
  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
  ? dc_validate_global_state+0x27a/0x330 [amdgpu]
  ? slab_post_alloc_hook+0x53/0x270
  ? __kmalloc+0x18c/0x300
  ? drm_dp_mst_atomic_setup_commit+0x8a/0x1a0 [drm_display_helper]
  ? preempt_count_add+0x7c/0xc0
  ? _raw_spin_unlock_irq+0x1f/0x40
  ? wait_for_completion_timeout+0x114/0x140
  ? preempt_count_add+0x7c/0xc0
  ? _raw_spin_unlock_irq+0x1f/0x40
  commit_tail+0x99/0x140 [drm_kms_helper]
  drm_atomic_helper_commit+0x12b/0x150 [drm_kms_helper]
  drm_atomic_commit+0x79/0x100 [drm]
  ? drm_plane_get_damage_clips.cold+0x1d/0x1d [drm]
  drm_framebuffer_remove+0x499/0x510 [drm]
  drm_mode_rmfb_work_fn+0x4b/0x90 [drm]
  process_one_work+0x21d/0x3f0
  worker_thread+0x1fa/0x3c0
  ? process_one_work+0x3f0/0x3f0
  kthread+0xff/0x130
  ? kthread_complete_and_exit+0x20/0x20
  ret_from_fork+0x22/0x30
  </TASK>



> 
> Fixing IGT will only mask the issue. Userspace should never be able
> to put the system in a state where it stops responding entirely. This
> will need some sort of fix in the kernel.
> 
> Harry
> 
> 

Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Harry Wentland]

On 12/15/22 16:02, Alex Hung wrote:
> 
> 
> On 2022-12-15 08:17, Harry Wentland wrote:
>>
>>
>> On 12/15/22 05:29, Michel Dänzer wrote:
>>> On 12/15/22 09:09, Christian König wrote:
>>>> Am 15.12.22 um 00:33 schrieb Alex Hung:
>>>>> On 2022-12-14 16:06, Alex Deucher wrote:
>>>>>> On Wed, Dec 14, 2022 at 5:56 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>> On 2022-12-14 15:35, Alex Deucher wrote:
>>>>>>>> On Wed, Dec 14, 2022 at 5:25 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>>>> On 2022-12-14 14:54, Alex Deucher wrote:
>>>>>>>>>> On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>>>>>>>>>>> On 2022-12-14 13:48, Alex Deucher wrote:
>>>>>>>>>>>> On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
>>>>>>>>>>>> <aurabindo.pillai@amd.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> From: Alex Hung <alex.hung@amd.com>
>>>>>>>>>>>>>

<snip>

>>
>> It can come through handle_hpd_rx_irq but we're using a workqueue
>> to queue interrupt handling so this shouldn't come from an atomic
>> context. I currently don't see where else it might be used in an
>> atomic context. Alex Hung, can you do a dump_stack() in this function
>> to see where the problematic call is coming from?
> 
> 
> IGT's kms_bw executes as below (when passing)
> 
> IGT-Version: 1.26-gf4067678 (x86_64) (Linux: 5.19.0-99-custom x86_64)
> Starting subtest: linear-tiling-1-displays-1920x1080p
> Subtest linear-tiling-1-displays-1920x1080p: SUCCESS (0.225s)
> Starting subtest: linear-tiling-1-displays-2560x1440p
> Subtest linear-tiling-1-displays-2560x1440p: SUCCESS (0.111s)
> Starting subtest: linear-tiling-1-displays-3840x2160p
> Subtest linear-tiling-1-displays-3840x2160p: SUCCESS (0.118s)
> Starting subtest: linear-tiling-2-displays-1920x1080p
> Subtest linear-tiling-2-displays-1920x1080p: SUCCESS (0.409s)
> Starting subtest: linear-tiling-2-displays-2560x1440p
> Subtest linear-tiling-2-displays-2560x1440p: SUCCESS (0.417s)
> Starting subtest: linear-tiling-2-displays-3840x2160p
> Subtest linear-tiling-2-displays-3840x2160p: SUCCESS (0.444s)
> Starting subtest: linear-tiling-3-displays-1920x1080p
> Subtest linear-tiling-3-displays-1920x1080p: SUCCESS (0.547s)
> Starting subtest: linear-tiling-3-displays-2560x1440p
> Subtest linear-tiling-3-displays-2560x1440p: SUCCESS (0.555s)
> Starting subtest: linear-tiling-3-displays-3840x2160p
> Subtest linear-tiling-3-displays-3840x2160p: SUCCESS (0.586s)
> Starting subtest: linear-tiling-4-displays-1920x1080p
> Subtest linear-tiling-4-displays-1920x1080p: SUCCESS (0.734s)
> Starting subtest: linear-tiling-4-displays-2560x1440p
> Subtest linear-tiling-4-displays-2560x1440p: SUCCESS (0.742s)
> Starting subtest: linear-tiling-4-displays-3840x2160p
> Subtest linear-tiling-4-displays-3840x2160p: SUCCESS (0.778s)
> Starting subtest: linear-tiling-5-displays-1920x1080p
> Subtest linear-tiling-5-displays-1920x1080p: SUCCESS (0.734s)
> Starting subtest: linear-tiling-5-displays-2560x1440p
> Subtest linear-tiling-5-displays-2560x1440p: SUCCESS (0.743s)
> Starting subtest: linear-tiling-5-displays-3840x2160p
> Subtest linear-tiling-5-displays-3840x2160p: SUCCESS (0.781s)
> Starting subtest: linear-tiling-6-displays-1920x1080p
> Test requirement not met in function run_test_linear_tiling, file ../tests/kms_bw.c:156:
> Test requirement: !(pipe > num_pipes)
> ASIC does not have 5 pipes

Does this IGT patch fix the !(pipe > num_pipes) issue?

https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/commit/3bb9ee157642ec2433f01b51e09866da2b0f3dd8

> Subtest linear-tiling-6-displays-1920x1080p: SKIP (0.000s)
> Starting subtest: linear-tiling-6-displays-2560x1440p
> Test requirement not met in function run_test_linear_tiling, file ../tests/kms_bw.c:156:
> Test requirement: !(pipe > num_pipes)
> ASIC does not have 5 pipes
> Subtest linear-tiling-6-displays-2560x1440p: SKIP (0.000s)
> Starting subtest: linear-tiling-6-displays-3840x2160p
> Test requirement not met in function run_test_linear_tiling, file ../tests/kms_bw.c:156:
> Test requirement: !(pipe > num_pipes)
> ASIC does not have 5 pipes
> Subtest linear-tiling-6-displays-3840x2160p: SKIP (0.000s)
> 
> The crash usually occurs when executing "linear-tiling-3-displays-1920x1080p" most of time, but the crash can also occurs at "linear-tiling-3-displays-2560x1440p"
> 
> ============
> This is dump_stack right before the failing msleep.
> 
> [IGT] kms_bw: starting subtest linear-tiling-3-displays-1920x1080p
> CPU: 1 PID: 76 Comm: kworker/1:1 Not tainted 5.19.0-99-custom #126
> Workqueue: events drm_mode_rmfb_work_fn [drm]
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x49/0x63
>  dump_stack+0x10/0x16
>  dce110_blank_stream.cold+0x5/0x14 [amdgpu]
>  core_link_disable_stream+0xe0/0x6b0 [amdgpu]
>  ? optc1_set_vtotal_min_max+0x6b/0x80 [amdgpu]
>  dcn31_reset_hw_ctx_wrap+0x229/0x410 [amdgpu]
>  dce110_apply_ctx_to_hw+0x6e/0x6c0 [amdgpu]
>  ? dcn20_plane_atomic_disable+0xb2/0x160 [amdgpu]
>  ? dcn20_disable_plane+0x2c/0x60 [amdgpu]
>  ? dcn20_post_unlock_program_front_end+0x77/0x2c0 [amdgpu]
>  dc_commit_state_no_check+0x39a/0xcd0 [amdgpu]
>  ? dc_validate_global_state+0x2ba/0x330 [amdgpu]
>  dc_commit_state+0x10f/0x150 [amdgpu]
>  amdgpu_dm_atomic_commit_tail+0x631/0x2d30 [amdgpu]

This doesn't look like an atomic (i.e., high IRQ) context to me.
So it's more likely there is some race going on, like Alex
Deucher suggested.

Harry

>  ? dcn30_internal_validate_bw+0x349/0xa00 [amdgpu]
>  ? slab_post_alloc_hook+0x53/0x270
>  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
>  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
>  ? dc_validate_global_state+0x27a/0x330 [amdgpu]
>  ? slab_post_alloc_hook+0x53/0x270
>  ? __kmalloc+0x18c/0x300
>  ? drm_dp_mst_atomic_setup_commit+0x8a/0x1a0 [drm_display_helper]
>  ? preempt_count_add+0x7c/0xc0
>  ? _raw_spin_unlock_irq+0x1f/0x40
>  ? wait_for_completion_timeout+0x114/0x140
>  ? preempt_count_add+0x7c/0xc0
>  ? _raw_spin_unlock_irq+0x1f/0x40
>  commit_tail+0x99/0x140 [drm_kms_helper]
>  drm_atomic_helper_commit+0x12b/0x150 [drm_kms_helper]
>  drm_atomic_commit+0x79/0x100 [drm]
>  ? drm_plane_get_damage_clips.cold+0x1d/0x1d [drm]
>  drm_framebuffer_remove+0x499/0x510 [drm]
>  drm_mode_rmfb_work_fn+0x4b/0x90 [drm]
>  process_one_work+0x21d/0x3f0
>  worker_thread+0x1fa/0x3c0
>  ? process_one_work+0x3f0/0x3f0
>  kthread+0xff/0x130
>  ? kthread_complete_and_exit+0x20/0x20
>  ret_from_fork+0x22/0x30
>  </TASK>
> 
> 
> ============
> If msleep is replaced by mdelay, the dump_stack is almost the same:
> 
> $ diff mdelay.log msleep.log
> <  dce110_blank_stream.cold+0x5/0x1f [amdgpu]
> ---
>>  dce110_blank_stream.cold+0x5/0x14 [amdgpu]
> 
> 
> ============
> If the IGT fix is applied (i.e. no crashes when removing buffers[i] reversely by "igt_remove_fb", the dump_stack outputs are the same:
> 
> $ diff msleep_igt.log msleep.log
> 2c2
> < CPU: 6 PID: 78 Comm: kworker/6:1 Not tainted 5.19.0-99-custom #126
> ---
>> CPU: 1 PID: 76 Comm: kworker/1:1 Not tainted 5.19.0-99-custom #126
> 
> ============
> Note the msleep doesn't always trigger crashes. One of the passing dump_stack is as below:
> 
> [IGT] kms_bw: starting subtest linear-tiling-2-displays-1920x1080p
> CPU: 6 PID: 78 Comm: kworker/6:1 Not tainted 5.19.0-99-custom #126
> Workqueue: events drm_mode_rmfb_work_fn [drm]
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x49/0x63
>  dump_stack+0x10/0x16
>  dce110_blank_stream.cold+0x5/0x14 [amdgpu]
>  core_link_disable_stream+0xe0/0x6b0 [amdgpu]
>  ? optc1_set_vtotal_min_max+0x6b/0x80 [amdgpu]
>  dcn31_reset_hw_ctx_wrap+0x229/0x410 [amdgpu]
>  dce110_apply_ctx_to_hw+0x6e/0x6c0 [amdgpu]
>  ? dcn20_plane_atomic_disable+0xb2/0x160 [amdgpu]
>  ? dcn20_disable_plane+0x2c/0x60 [amdgpu]
>  ? dcn20_post_unlock_program_front_end+0x77/0x2c0 [amdgpu]
>  dc_commit_state_no_check+0x39a/0xcd0 [amdgpu]
>  ? dc_validate_global_state+0x2ba/0x330 [amdgpu]
>  dc_commit_state+0x10f/0x150 [amdgpu]
>  amdgpu_dm_atomic_commit_tail+0x631/0x2d30 [amdgpu]
>  ? debug_smp_processor_id+0x17/0x20
>  ? dc_fpu_end+0x4e/0xd0 [amdgpu]
>  ? dcn316_populate_dml_pipes_from_context+0x69/0x2a0 [amdgpu]
>  ? dcn31_calculate_wm_and_dlg_fp+0x50/0x530 [amdgpu]
>  ? kernel_fpu_end+0x26/0x50
>  ? dc_fpu_end+0x4e/0xd0 [amdgpu]
>  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
>  ? dcn31_validate_bandwidth+0x12c/0x2b0 [amdgpu]
>  ? dc_validate_global_state+0x27a/0x330 [amdgpu]
>  ? slab_post_alloc_hook+0x53/0x270
>  ? __kmalloc+0x18c/0x300
>  ? drm_dp_mst_atomic_setup_commit+0x8a/0x1a0 [drm_display_helper]
>  ? preempt_count_add+0x7c/0xc0
>  ? _raw_spin_unlock_irq+0x1f/0x40
>  ? wait_for_completion_timeout+0x114/0x140
>  ? preempt_count_add+0x7c/0xc0
>  ? _raw_spin_unlock_irq+0x1f/0x40
>  commit_tail+0x99/0x140 [drm_kms_helper]
>  drm_atomic_helper_commit+0x12b/0x150 [drm_kms_helper]
>  drm_atomic_commit+0x79/0x100 [drm]
>  ? drm_plane_get_damage_clips.cold+0x1d/0x1d [drm]
>  drm_framebuffer_remove+0x499/0x510 [drm]
>  drm_mode_rmfb_work_fn+0x4b/0x90 [drm]
>  process_one_work+0x21d/0x3f0
>  worker_thread+0x1fa/0x3c0
>  ? process_one_work+0x3f0/0x3f0
>  kthread+0xff/0x130
>  ? kthread_complete_and_exit+0x20/0x20
>  ret_from_fork+0x22/0x30
>  </TASK>
> 
> 
> 
>>
>> Fixing IGT will only mask the issue. Userspace should never be able
>> to put the system in a state where it stops responding entirely. This
>> will need some sort of fix in the kernel.
>>
>> Harry
>>
>>