Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Fernando Ramos <greenfoo@u92.eu>
To:     dri-devel@lists.freedesktop.org
Cc:     linux-kernel@vger.kernel.org, sean@poorly.run,
        linux-doc@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
        freedreno@lists.freedesktop.org, nouveau@lists.freedesktop.org,
        linux-renesas-soc@vger.kernel.org, linux-tegra@vger.kernel.org
Subject: [PATCH v4 18/20] drm/amd: cleanup: drm_modeset_lock_all() --> DRM_MODESET_LOCK_ALL_BEGIN() [part 3]
Date:   Wed, 13 Oct 2021 22:48:44 +0200
Message-Id: <20211013204846.90026-19-greenfoo@u92.eu>
In-Reply-To: <20211013204846.90026-1-greenfoo@u92.eu>
References: <20211013204846.90026-1-greenfoo@u92.eu>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

As requested in Documentation/gpu/todo.rst, replace driver calls to
drm_modeset_lock_all() with DRM_MODESET_LOCK_ALL_BEGIN() and
DRM_MODESET_LOCK_ALL_END()

NOTE:

While this change is similar to the one done two commits ago, it
contains an important extra nuances that I'm going to explain next.

The only difference between the old drm_modeset_{lock,unlock}_all()
functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that
the former use a global context stored in dev->mode_config.acquire_ctx
while the latter depend on a user provided one (typically in the stack).

This means that as long as no one accesses the global
dev->mode_config.acquire_ctx context in the block that runs between
lock/BEGIN and unlock/END, the code should be equivalent before and
after my changes.

Turns out that, while not obvious at first sight, the call to
dm_restore_drm_connector_state() done between drm_modset_lock_all() and
drm_modeset_unlock_all() ends up using that global context structure
stored in dev.

To fix this we need to update some function prototypes to accept the
new stack allocated variable as an argument.

Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 ++++++++++++-------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 ++-
 .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 13 ++++++---
 3 files changed, 29 insertions(+), 14 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 10ed1f8ad514..7a3c5def9fb9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -81,6 +81,7 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_vblank.h>
 #include <drm/drm_audio_component.h>
+#include <drm/drm_drv.h>
 
 #if defined(CONFIG_DRM_AMD_DC_DCN)
 #include "ivsrcid/dcn/irqsrcs_dcn_1_0.h"
@@ -2906,6 +2907,8 @@ static void handle_hpd_irq_helper(struct amdgpu_dm_connector *aconnector)
 	struct amdgpu_device *adev = drm_to_adev(dev);
 	struct dm_connector_state *dm_con_state = to_dm_connector_state(connector->state);
 	struct dm_crtc_state *dm_crtc_state = NULL;
+	struct drm_modeset_acquire_ctx ctx;
+	int ret;
 
 	if (adev->dm.disable_hpd_irq)
 		return;
@@ -2947,9 +2950,9 @@ static void handle_hpd_irq_helper(struct amdgpu_dm_connector *aconnector)
 		goto out;
 	}
 
-	drm_modeset_lock_all(dev);
-	dm_restore_drm_connector_state(dev, connector);
-	drm_modeset_unlock_all(dev);
+	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
+	dm_restore_drm_connector_state(dev, connector, &ctx);
+	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
 
 	if (aconnector->base.force == DRM_FORCE_UNSPECIFIED)
 		drm_kms_helper_hotplug_event(dev);
@@ -3070,6 +3073,7 @@ static void handle_hpd_rx_irq(void *param)
 	struct drm_connector *connector = &aconnector->base;
 	struct drm_device *dev = connector->dev;
 	struct dc_link *dc_link = aconnector->dc_link;
+	struct drm_modeset_acquire_ctx ctx;
 	bool is_mst_root_connector = aconnector->mst_mgr.mst_state;
 	bool result = false;
 	enum dc_connection_type new_connection_type = dc_connection_none;
@@ -3079,6 +3083,7 @@ static void handle_hpd_rx_irq(void *param)
 	bool has_left_work = false;
 	int idx = aconnector->base.index;
 	struct hpd_rx_irq_offload_work_queue *offload_wq = &adev->dm.hpd_rx_offload_wq[idx];
+	int ret;
 
 	memset(&hpd_irq_data, 0, sizeof(hpd_irq_data));
 
@@ -3153,9 +3158,9 @@ static void handle_hpd_rx_irq(void *param)
 			goto finish;
 		}
 
-		drm_modeset_lock_all(dev);
-		dm_restore_drm_connector_state(dev, connector);
-		drm_modeset_unlock_all(dev);
+		DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
+		dm_restore_drm_connector_state(dev, connector, &ctx);
+		DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
 
 		drm_kms_helper_hotplug_event(dev);
 	}
@@ -9703,7 +9708,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 }
 
 
-static int dm_force_atomic_commit(struct drm_connector *connector)
+static int dm_force_atomic_commit(struct drm_connector *connector,
+				  struct drm_modeset_acquire_ctx *ctx)
 {
 	int ret = 0;
 	struct drm_device *ddev = connector->dev;
@@ -9717,7 +9723,7 @@ static int dm_force_atomic_commit(struct drm_connector *connector)
 	if (!state)
 		return -ENOMEM;
 
-	state->acquire_ctx = ddev->mode_config.acquire_ctx;
+	state->acquire_ctx = ctx;
 
 	/* Construct an atomic state to restore previous display setting */
 
@@ -9764,7 +9770,8 @@ static int dm_force_atomic_commit(struct drm_connector *connector)
  * same port and when running without usermode desktop manager supprot
  */
 void dm_restore_drm_connector_state(struct drm_device *dev,
-				    struct drm_connector *connector)
+				    struct drm_connector *connector,
+				    struct drm_modeset_acquire_ctx *ctx)
 {
 	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 	struct amdgpu_crtc *disconnected_acrtc;
@@ -9787,7 +9794,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
 	 * to turn on the display, so we do it here
 	 */
 	if (acrtc_state->stream->sink != aconnector->dc_sink)
-		dm_force_atomic_commit(&aconnector->base);
+		dm_force_atomic_commit(&aconnector->base, ctx);
 }
 
 /*
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 37e61a88d49e..1a7c8fbf149a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -706,7 +706,8 @@ enum drm_mode_status amdgpu_dm_connector_mode_valid(struct drm_connector *connec
 				   struct drm_display_mode *mode);
 
 void dm_restore_drm_connector_state(struct drm_device *dev,
-				    struct drm_connector *connector);
+				    struct drm_connector *connector,
+				    struct drm_modeset_acquire_ctx *ctx);
 
 void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 					struct edid *edid);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index 7751038d5788..f4542760b006 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -24,6 +24,7 @@
  */
 
 #include <linux/uaccess.h>
+#include <drm/drm_drv.h>
 
 #include "dc.h"
 #include "amdgpu.h"
@@ -1192,12 +1193,14 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf,
 	struct drm_connector *connector = &aconnector->base;
 	struct dc_link *link = NULL;
 	struct drm_device *dev = connector->dev;
+	struct drm_modeset_acquire_ctx ctx;
 	enum dc_connection_type new_connection_type = dc_connection_none;
 	char *wr_buf = NULL;
 	uint32_t wr_buf_size = 42;
 	int max_param_num = 1;
 	long param[1] = {0};
 	uint8_t param_nums = 0;
+	int ret;
 
 	if (!aconnector || !aconnector->dc_link)
 		return -EINVAL;
@@ -1258,9 +1261,13 @@ static ssize_t trigger_hotplug(struct file *f, const char __user *buf,
 		goto unlock;
 	}
 
-	drm_modeset_lock_all(dev);
-	dm_restore_drm_connector_state(dev, connector);
-	drm_modeset_unlock_all(dev);
+	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
+	dm_restore_drm_connector_state(dev, connector, &ctx);
+	DRM_MODESET_LOCK_ALL_END(dev, ctx, ret);
+
+	if (ret)
+		return ret;
+
 	drm_kms_helper_hotplug_event(dev);
 
 unlock:
-- 
2.33.0