drm/msm: Various dpu locking and legacy cleanups

From: Sean Paul <[email protected]>

Instead of registering through dpu_power_handle just to get a call on runtime_resume, call the crtc function directly.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 23 ++++++-----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  | 10 ++++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  4 ++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |  8 ++++----
 4 files changed, 20 insertions(+), 25 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +5 -6

index e09209d6c469..c55cb751e2b4 100644
@@ -33,7 +33,6 @@
 #include "dpu_plane.h"
 #include "dpu_encoder.h"
 #include "dpu_vbif.h"
-#include "dpu_power_handle.h"
 #include "dpu_core_perf.h"
 #include "dpu_trace.h"
 

@@ -69,8 +68,6 @@ static void dpu_crtc_destroy(struct drm_crtc *crtc)
 	if (!crtc)
 		return;
 
-	dpu_crtc->phandle = NULL;
-
 	drm_crtc_cleanup(crtc);
 	mutex_destroy(&dpu_crtc->crtc_lock);
 	kfree(dpu_crtc);

@@ -844,15 +841,17 @@ static struct drm_crtc_state
 *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &cstate->base;
 }
 
-static void dpu_crtc_handle_power_event(u32 event_type, void *arg)
+void dpu_crtc_runtime_resume(struct drm_crtc *crtc)
 {
-	struct drm_crtc *crtc = arg;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct drm_encoder *encoder;
 
 	mutex_lock(&dpu_crtc->crtc_lock);
 
-	trace_dpu_crtc_handle_power_event(DRMID(crtc), event_type);
+	if (!dpu_crtc->enabled)
+		goto end;
+
+	trace_dpu_crtc_runtime_resume(DRMID(crtc));
 
 	/* restore encoder; crtc will be programmed during commit */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	drm_for_each_encoder(encoder, crtc->dev) {

@@ -862,6 +861,7 @@ static void dpu_crtc_handle_power_event(u32
 event_type, void *arg)
 		dpu_encoder_virt_restore(encoder);
 	}
 
+end:
 	mutex_unlock(&dpu_crtc->crtc_lock);
 }

@@ -917,10 +917,6 @@ static void dpu_crtc_disable(struct drm_crtc 
 *crtc)
 		dpu_encoder_register_frame_event_callback(encoder, NULL,
 NULL);
 	}
 
-	if (dpu_crtc->power_event)
-		dpu_power_handle_unregister_event(dpu_crtc->phandle,
-				dpu_crtc->power_event);
-
 	memset(cstate->mixers, 0, sizeof(cstate->mixers));

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -7

 	cstate->num_mixers = 0;

@@ -972,11 +968,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 
 	/* Enable/restore vblank irq handling */
 	drm_crtc_vblank_on(crtc);
-
-	dpu_crtc->power_event = dpu_power_handle_register_event(
-		dpu_crtc->phandle, DPU_POWER_EVENT_ENABLE,
-		dpu_crtc_handle_power_event, crtc, dpu_crtc->name);
-
 }
 
 struct plane_state {

@@ -1522,8 +1513,6 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
 *dev, struct drm_plane *plane,
 	/* initialize event handling */
 	spin_lock_init(&dpu_crtc->event_lock);
 
-	dpu_crtc->phandle = &kms->phandle;
-
 	DPU_DEBUG("%s: successfully initialized crtc\n", dpu_crtc->name);
 	return crtc;

}

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +0 -1

index 4822602402f9..1dca91d1210f 100644
@@ -151,7 +151,6 @@ struct dpu_crtc_frame_event {
  * @event_worker  : Event worker queue
  * @event_lock    : Spinlock around event handling code
  * @phandle: Pointer to power handler
- * @power_event   : registered power event handle
  * @cur_perf      : current performance committed to clock/bandwidth

driver
*/
struct dpu_crtc {

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +2 -3

@@ -187,9 +186,6 @@ struct dpu_crtc {
 	/* for handling internal event thread */
 	spinlock_t event_lock;
 
-	struct dpu_power_handle *phandle;
-	struct dpu_power_event *power_event;
-
 	struct dpu_core_perf_params cur_perf;
 
 	struct dpu_crtc_smmu_state_data smmu_state;

@@ -333,4 +329,10 @@ static inline bool dpu_crtc_is_enabled(struct
 drm_crtc *crtc)
 	return crtc ? crtc->enabled : false;
 }
 
+/**
+ * dpu_crtc_runtime_resume - called by the top-level on 

pm_runtime_resume
+ * @crtc: CRTC to resume
+ */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +2 -0

+void dpu_crtc_runtime_resume(struct drm_crtc *crtc);
+
 #endif /* _DPU_CRTC_H_ */

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

index ab8574ab8327..654ea5060e02 100644
@@ -1137,6 +1137,7 @@ static int __maybe_unused 

dpu_runtime_resume(struct
device *dev)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +1 -0

 	int rc = -1;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
+	struct drm_crtc *crtc;
 	struct drm_device *ddev;
 	struct dss_module_power *mp = &dpu_kms->mp;

@@ -1154,6 +1155,9 @@ static int __maybe_unused 

dpu_runtime_resume(struct
device *dev)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +3 -0

 	dpu_vbif_init_memtypes(dpu_kms);
 
+	drm_for_each_crtc(crtc, ddev)
+		dpu_crtc_runtime_resume(crtc);
+
 	rc = dpu_power_resource_enable(&dpu_kms->phandle, true);
 	if (rc)
 		DPU_ERROR("resource enable failed: %d\n", rc);

drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h +4 -0

index 7ab0ba8224f6..328df37d7580 100644
@@ -298,6 +298,10 @@ DEFINE_EVENT(dpu_drm_obj_template,
 dpu_kms_wait_for_commit_done,
 	TP_PROTO(uint32_t drm_id),
 	TP_ARGS(drm_id)
 );
+DEFINE_EVENT(dpu_drm_obj_template, dpu_crtc_runtime_resume,
+	TP_PROTO(uint32_t drm_id),
+	TP_ARGS(drm_id)
+);
 
 TRACE_EVENT(dpu_enc_enable,

drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h +0 -4

 	TP_PROTO(uint32_t drm_id, int hdisplay, int vdisplay),

@@ -518,10 +522,6 @@ DEFINE_EVENT(dpu_id_event_template,
 dpu_crtc_frame_event_cb,
 	TP_PROTO(uint32_t drm_id, u32 event),
 	TP_ARGS(drm_id, event)
 );
-DEFINE_EVENT(dpu_id_event_template, dpu_crtc_handle_power_event,
-	TP_PROTO(uint32_t drm_id, u32 event),
-	TP_ARGS(drm_id, event)
-);
 DEFINE_EVENT(dpu_id_event_template, dpu_crtc_frame_event_done,
 	TP_PROTO(uint32_t drm_id, u32 event),

TP_ARGS(drm_id, event)

From: Sean Paul <[email protected]>

The crtc runtime resume doesn't actually operate on the crtc, but rather its encoders. The problem with this is that we need to inspect the crtc state to get the currently connected encoders. Since runtime resume isn't guaranteed to be called while holding the modeset locks (although it sometimes is), this presents a race condition.

Now that we have ->enabled on the virtual encoders, and a lock to protect it, just call resume on each encoder and only restore the ones that are enabled.

Changes in v2:
- None

Cc Jeykumar Sankaran <[email protected]>

Signed-off-by Sean Paul <[email protected]>

---

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 24 ---------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  6 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 +++++++++------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  6 +++---
 5 files changed, 15 insertions(+), 49 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -24

index d8f58caf2772..9be24907f8c1 100644
@@ -841,30 +841,6 @@ static struct drm_crtc_state
 *dpu_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &cstate->base;
 }
 
-void dpu_crtc_runtime_resume(struct drm_crtc *crtc)
-{
-	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
-	struct drm_encoder *encoder;
-
-	mutex_lock(&dpu_crtc->crtc_lock);
-
-	if (!dpu_crtc->enabled)
-		goto end;
-
-	trace_dpu_crtc_runtime_resume(DRMID(crtc));
-
-	/* restore encoder; crtc will be programmed during commit */
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
-
-		dpu_encoder_virt_restore(encoder);
-	}
-
-end:
-	mutex_unlock(&dpu_crtc->crtc_lock);
-}
-
 static void dpu_crtc_disable(struct drm_crtc *crtc)
 {

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	struct dpu_crtc *dpu_crtc;

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +0 -2

index 1dca91d1210f..93d21a61a040 100644
@@ -329,10 +329,4 @@ static inline bool dpu_crtc_is_enabled(struct
 drm_crtc *crtc)
 	return crtc ? crtc->enabled : false;
 }
 
-/**
- * dpu_crtc_runtime_resume - called by the top-level on 

pm_runtime_resume
- * @crtc: CRTC to resume
- */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +0 -2

-void dpu_crtc_runtime_resume(struct drm_crtc *crtc);
-
 #endif /* _DPU_CRTC_H_ */

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0

index 3daa86220d47..d89ac520f7e6 100644
@@ -1089,28 +1089,24 @@ static void 

_dpu_encoder_virt_enable_helper(struct
drm_encoder *drm_enc)

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +3 -12

 	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
 }
 
-void dpu_encoder_virt_restore(struct drm_encoder *drm_enc)
+void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc)
 {
-	struct dpu_encoder_virt *dpu_enc = NULL;
-	int i;
-
-	if (!drm_enc) {
-		DPU_ERROR("invalid encoder\n");
-		return;
-	}
-	dpu_enc = to_dpu_encoder_virt(drm_enc);
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
 
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
+	mutex_lock(&dpu_enc->enc_lock);
 
-		if (phys && (phys != dpu_enc->cur_master) &&

phys->ops.restore)

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +7 -2

-			phys->ops.restore(phys);
-	}
+	if (!dpu_enc->enabled)
+		goto out;
 
+	if (dpu_enc->cur_slave && dpu_enc->cur_slave->ops.restore)
+		dpu_enc->cur_slave->ops.restore(dpu_enc->cur_slave);
 	if (dpu_enc->cur_master && dpu_enc->cur_master->ops.restore)
 		dpu_enc->cur_master->ops.restore(dpu_enc->cur_master);
 
 	_dpu_encoder_virt_enable_helper(drm_enc);
+
+out:
+	mutex_unlock(&dpu_enc->enc_lock);
 }
 
 static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +1 -1

index 9dbf38f446d9..aa4f135218fa 100644
@@ -126,10 +126,10 @@ int dpu_encoder_wait_for_event(struct drm_encoder
 *drm_encoder,
 enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder
 *encoder);
 
 /**
- * dpu_encoder_virt_restore - restore the encoder configs
+ * dpu_encoder_virt_runtime_resume - pm runtime resume the encoder

configs
* @encoder: encoder pointer
*/

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +1 -1

-void dpu_encoder_virt_restore(struct drm_encoder *encoder);
+void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
 
 /**
  * dpu_encoder_init - initialize virtual encoder object

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

index 9f7da56bb453..1bec4540f3e1 100644
@@ -1129,7 +1129,7 @@ static int __maybe_unused 

dpu_runtime_resume(struct
device *dev)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +1 -1

 	int rc = -1;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dpu_kms *dpu_kms = platform_get_drvdata(pdev);
-	struct drm_crtc *crtc;
+	struct drm_encoder *encoder;
 	struct drm_device *ddev;
 	struct dss_module_power *mp = &dpu_kms->mp;

@@ -1147,8 +1147,8 @@ static int __maybe_unused 

dpu_runtime_resume(struct
device *dev)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +2 -2

 	dpu_vbif_init_memtypes(dpu_kms);
 
-	drm_for_each_crtc(crtc, ddev)
-		dpu_crtc_runtime_resume(crtc);
+	drm_for_each_encoder(encoder, ddev)
+		dpu_encoder_virt_runtime_resume(encoder);
 
 	return rc;
 }

From: Sean Paul <[email protected]>

Now that runtime resume is handled in encoder, we don't need to worry about crtc_lock recursion when calling pm_runtime_(get|put). So drop the lock drops in _dpu_crtc_vblank_enable_no_lock().

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

index 9be24907f8c1..80de5289ada3 100644
@@ -777,10 +777,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
 	struct drm_encoder *enc;
 
 	if (enable) {
-		/* drop lock since power crtc cb may try to re-acquire

lock */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -2

-		mutex_unlock(&dpu_crtc->crtc_lock);
 		pm_runtime_get_sync(dev->dev);
-		mutex_lock(&dpu_crtc->crtc_lock);
 
 		list_for_each_entry(enc, &dev->mode_config.encoder_list,

head) {

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 			if (enc->crtc != crtc)

@@ -805,10 +802,7 @@ static void _dpu_crtc_vblank_enable_no_lock(
 			dpu_encoder_register_vblank_callback(enc, NULL,
 NULL);
 		}
 
-		/* drop lock since power crtc cb may try to re-acquire

lock */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -2

-		mutex_unlock(&dpu_crtc->crtc_lock);
 		pm_runtime_put_sync(dev->dev);
-		mutex_lock(&dpu_crtc->crtc_lock);
 	}
 }

From: Sean Paul <[email protected]>

This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks since it digs into the state objects.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

index 80de5289ada3..156f4c77ca44 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 		} else {
 

trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),

fevent->event);

Quoted 10 lines · Sean Paul

From: Sean Paul <[email protected]>

This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks since it digs into the state objects.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---

Reviewed-by Jeykumar Sankaran <[email protected]>

Quoted 3 lines

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_frame_event_work(struct · line 341

Quoted 13 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -0

index 80de5289ada3..156f4c77ca44 100644
@@ -335,7 +335,9 @@ static void dpu_crtc_frame_event_work(struct
 kthread_work *work)
 			/* release bandwidth and other resources */
 			trace_dpu_crtc_frame_event_done(DRMID(crtc),
 							fevent->event);
+			drm_modeset_lock_all(crtc->dev);
 			dpu_core_perf_crtc_release_bw(crtc);
+			drm_modeset_unlock_all(crtc->dev);

We might need to revisit this locking when we measure for performance as it could block the incoming frame locking.

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 		} else {
 

trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),

fevent->event);

Signature · 1 line

Jeykumar S

From: Sean Paul <[email protected]>

This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks since it digs into the state objects.

Changes in v2:
- None
Changes in v3:
- Use those nifty new DRM_MODESET_LOCK_ALL_* helpers (Daniel)

Cc Daniel Vetter <[email protected]>

Cc Jeykumar Sankaran <[email protected]>

Signed-off-by Sean Paul <[email protected]>

---

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +13 -0

index 74ef384d9cd6a..03ddd281a354f 100644
@@ -306,6 +306,19 @@ static void dpu_crtc_vblank_cb(void *data)
 	trace_dpu_crtc_vblank_cb(DRMID(crtc));
 }
 
+static void dpu_crtc_release_bw_unlocked(struct drm_crtc *crtc)
+{
+	int ret = 0;
+	struct drm_modeset_acquire_ctx ctx;
+
+	DRM_MODESET_LOCK_ALL_BEGIN(crtc->dev, ctx, 0, ret);
+	dpu_core_perf_crtc_release_bw(crtc);
+	DRM_MODESET_LOCK_ALL_END(ctx, ret);
+	if (ret)
+		DRM_ERROR("Failed to acquire modeset locks to release bw,
 %d\n",
+			  ret);
+}
+
 static void dpu_crtc_frame_event_work(struct kthread_work *work)
 {

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -1

 	struct dpu_crtc_frame_event *fevent = container_of(work,

@@ -335,7 +348,7 @@ static void dpu_crtc_frame_event_work(struct
 kthread_work *work)
 			/* release bandwidth and other resources */
 			trace_dpu_crtc_frame_event_done(DRMID(crtc),
 							fevent->event);
-			dpu_core_perf_crtc_release_bw(crtc);
+			dpu_crtc_release_bw_unlocked(crtc);
 		} else {
 

trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),

fevent->event);

From: Sean Paul <[email protected]>

This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks since it digs into the state objects.

Changes in v2:
- None
Changes in v3:
- Use those nifty new DRM_MODESET_LOCK_ALL_* helpers (Daniel)

Cc Daniel Vetter <[email protected]>

Cc Jeykumar Sankaran <[email protected]>

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

index 74ef384d9cd6a..03ddd281a354f 100644
@@ -306,6 +306,19 @@ static void dpu_crtc_vblank_cb(void *data)
 	trace_dpu_crtc_vblank_cb(DRMID(crtc));

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +6 -1

+	if (ret)
+		DRM_ERROR("Failed to acquire modeset locks to release bw, %d\n",
+			  ret);
+}
+
 static void dpu_crtc_frame_event_work(struct kthread_work *work)
 {
 	struct dpu_crtc_frame_event *fevent = container_of(work,

@@ -335,7 +348,7 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work)
 			/* release bandwidth and other resources */
 			trace_dpu_crtc_frame_event_done(DRMID(crtc),
 							fevent->event);
-			dpu_core_perf_crtc_release_bw(crtc);
+			dpu_crtc_release_bw_unlocked(crtc);
 		} else {
 			trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),
 								fevent->event);

Signature · 1 line

Sean Paul, Software Engineer, Google / Chromium OS

From: Sean Paul <[email protected]>

It's for legacy drivers, for atomic drivers crtc->state->encoder_mask should be used to map encoder to crtc.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46 ++++++++----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
 2 files changed, 29 insertions(+), 36 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -3

index 156f4c77ca44..a008a87a8113 100644
@@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
 drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
-	drm_for_each_encoder(encoder, crtc->dev)
-		if (encoder->crtc == crtc)
-			return dpu_encoder_get_intf_mode(encoder);
+	/* TODO: Returns the first INTF_MODE, could there be multiple

values? */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0

+	drm_for_each_encoder_mask(encoder, crtc->dev,

crtc->state->encoder_mask)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +3 -6

+		return dpu_encoder_get_intf_mode(encoder);
 
 	return INTF_MODE_NONE;
 }

@@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
 *crtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
-		if (encoder->crtc != crtc)
-			continue;
-
-		/* encoder will trigger pending mask now */
+	/* encoder will trigger pending mask now */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_trigger_kickoff_pending(encoder);
-	}

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 	/*
 	 * If no mixers have been allocated in dpu_crtc_atomic_check(),

@@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
 drm_crtc *crtc)
 void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -1

 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);

@@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
 *crtc)
 
 	DPU_ATRACE_BEGIN("crtc_commit");
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
+	/*
+	 * Encoder will flush/start now, unless it has a tx pending. If

so, it

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +4 -6

+	 * may delay and flush at an irq event (e.g. ppdone)
+	 */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
+				  crtc->state->encoder_mask) {
 		struct dpu_encoder_kickoff_params params = { 0 };
-
-		if (encoder->crtc != crtc)
-			continue;
-
-		/*
-		 * Encoder will flush/start now, unless it has a tx

pending.

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 * If so, it may delay and flush at an irq event (e.g.

ppdone)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 */
 		dpu_encoder_prepare_for_kickoff(encoder, &params);
 	}

@@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
 *crtc)
 
 	dpu_vbif_clear_errors(dpu_kms);

end:

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	reinit_completion(&dpu_crtc->frame_done_comp);

@@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc 
 *crtc)
 
 	dpu_core_perf_crtc_update(crtc, 0, true);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder, NULL,
 NULL);
-	}

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	memset(cstate->mixers, 0, sizeof(cstate->mixers));
 	cstate->num_mixers = 0;

@@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder,
 				dpu_crtc_frame_event_cb, (void *)crtc);
-	}
 
 	mutex_lock(&dpu_crtc->crtc_lock);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +3 -0

index 1bec4540f3e1..64134d619748 100644
@@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	struct dpu_kms *dpu_kms;
 	struct msm_drm_private *priv;
 	struct drm_device *dev;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct drm_encoder *encoder;
+	int i;
 
 	if (!kms)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

 		return;

@@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	priv = dev->dev_private;
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc != NULL)
+	/* Call prepare_commit for all affected encoders */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_for_each_encoder_mask(encoder, crtc->dev,
+					  crtc_state->encoder_mask) {
 			dpu_encoder_prepare_commit(encoder);
+		}
+	}
 }
 

/*

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

@@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 {
 	const struct drm_encoder_helper_funcs *funcs =
 encoder->helper_private;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
 
 	/* Forward this enable call to the commit hook */
 	if (funcs && funcs->commit)
 		funcs->commit(encoder);
 
-	if (crtc && crtc->state->active) {
+	drm_for_each_crtc(crtc, dev) {
+		if (!(crtc->state->encoder_mask &
 drm_encoder_mask(encoder)))
+			continue;
+

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

 		trace_dpu_kms_enc_enable(DRMID(crtc));
 		dpu_crtc_commit_kickoff(crtc);
 	}

Quoted 95 lines · Sean Paul

From: Sean Paul <[email protected]>

It's for legacy drivers, for atomic drivers crtc->state->encoder_mask should be used to map encoder to crtc.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46 ++++++++----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
 2 files changed, 29 insertions(+), 36 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -3

index 156f4c77ca44..a008a87a8113 100644
@@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
 drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
-	drm_for_each_encoder(encoder, crtc->dev)
-		if (encoder->crtc == crtc)
-			return dpu_encoder_get_intf_mode(encoder);
+	/* TODO: Returns the first INTF_MODE, could there be multiple

values? */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0

+	drm_for_each_encoder_mask(encoder, crtc->dev,

crtc->state->encoder_mask)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +3 -6

+		return dpu_encoder_get_intf_mode(encoder);
 
 	return INTF_MODE_NONE;
 }

@@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
 *crtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
-		if (encoder->crtc != crtc)
-			continue;
-
-		/* encoder will trigger pending mask now */
+	/* encoder will trigger pending mask now */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_trigger_kickoff_pending(encoder);
-	}

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 	/*
 	 * If no mixers have been allocated in dpu_crtc_atomic_check(),

@@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
 drm_crtc *crtc)
 void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -1

 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);

@@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
 *crtc)
 
 	DPU_ATRACE_BEGIN("crtc_commit");
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
+	/*
+	 * Encoder will flush/start now, unless it has a tx pending. If

so, it

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +4 -6

+	 * may delay and flush at an irq event (e.g. ppdone)
+	 */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
+				  crtc->state->encoder_mask) {
 		struct dpu_encoder_kickoff_params params = { 0 };
-
-		if (encoder->crtc != crtc)
-			continue;
-
-		/*
-		 * Encoder will flush/start now, unless it has a tx

pending.

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 * If so, it may delay and flush at an irq event (e.g.

ppdone)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 */
 		dpu_encoder_prepare_for_kickoff(encoder, &params);
 	}

@@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 
 	dpu_vbif_clear_errors(dpu_kms);
 

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) · line 762

Quoted 9 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -5

-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
-		if (encoder->crtc != crtc)
-			continue;
-
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_kickoff(encoder);
-	}

We wont be holding the modeset locks here (and in crtc_atomic_begin) in the display thread. Is
it safe to iterate over encoder_mask?

Quoted 90 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 end:

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	reinit_completion(&dpu_crtc->frame_done_comp);

@@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
 
 	dpu_core_perf_crtc_update(crtc, 0, true);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder, NULL,
 NULL);
-	}
 

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	memset(cstate->mixers, 0, sizeof(cstate->mixers));
 	cstate->num_mixers = 0;

@@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder,
 				dpu_crtc_frame_event_cb, (void *)crtc);
-	}
 
 	mutex_lock(&dpu_crtc->crtc_lock);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +3 -0

index 1bec4540f3e1..64134d619748 100644
@@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	struct dpu_kms *dpu_kms;
 	struct msm_drm_private *priv;
 	struct drm_device *dev;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct drm_encoder *encoder;
+	int i;
 
 	if (!kms)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

 		return;

@@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	priv = dev->dev_private;
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc != NULL)
+	/* Call prepare_commit for all affected encoders */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_for_each_encoder_mask(encoder, crtc->dev,
+					  crtc_state->encoder_mask) {
 			dpu_encoder_prepare_commit(encoder);
+		}
+	}
 }
 

/*

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

@@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 {
 	const struct drm_encoder_helper_funcs *funcs =
 encoder->helper_private;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
 
 	/* Forward this enable call to the commit hook */
 	if (funcs && funcs->commit)
 		funcs->commit(encoder);
 
-	if (crtc && crtc->state->active) {
+	drm_for_each_crtc(crtc, dev) {
+		if (!(crtc->state->encoder_mask &
 drm_encoder_mask(encoder)))
+			continue;
+

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

 		trace_dpu_kms_enc_enable(DRMID(crtc));
 		dpu_crtc_commit_kickoff(crtc);
 	}

Signature · 1 line

Jeykumar S

Quoted 6 lines · Jeykumar Sankaran

Quoted 5 lines · Sean Paul

From: Sean Paul <[email protected]>

It's for legacy drivers, for atomic drivers crtc->state->encoder_mask should be used to map encoder to crtc.

Quoted 6 lines

Quoted 6 lines

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46

++++++++----------------

Quoted 80 lines

Quoted 80 lines

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
 2 files changed, 29 insertions(+), 36 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -3

index 156f4c77ca44..a008a87a8113 100644
@@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
 drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
-	drm_for_each_encoder(encoder, crtc->dev)
-		if (encoder->crtc == crtc)
-			return dpu_encoder_get_intf_mode(encoder);
+	/* TODO: Returns the first INTF_MODE, could there be multiple

values? */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0

+	drm_for_each_encoder_mask(encoder, crtc->dev,

crtc->state->encoder_mask)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +3 -6

+		return dpu_encoder_get_intf_mode(encoder);
 
 	return INTF_MODE_NONE;
 }

@@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
 *crtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
-		if (encoder->crtc != crtc)
-			continue;
-
-		/* encoder will trigger pending mask now */
+	/* encoder will trigger pending mask now */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_trigger_kickoff_pending(encoder);
-	}

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 	/*
 	 * If no mixers have been allocated in dpu_crtc_atomic_check(),

@@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
 drm_crtc *crtc)
 void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -1

 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);

@@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
 *crtc)
 
 	DPU_ATRACE_BEGIN("crtc_commit");
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
+	/*
+	 * Encoder will flush/start now, unless it has a tx pending. If

so, it

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +4 -6

+	 * may delay and flush at an irq event (e.g. ppdone)
+	 */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
+				  crtc->state->encoder_mask) {
 		struct dpu_encoder_kickoff_params params = { 0 };
-
-		if (encoder->crtc != crtc)
-			continue;
-
-		/*
-		 * Encoder will flush/start now, unless it has a tx

pending.

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 * If so, it may delay and flush at an irq event (e.g.

ppdone)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 */
 		dpu_encoder_prepare_for_kickoff(encoder, &params);
 	}

Quoted 1 line

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc

Quoted 1 line

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

@@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc

*crtc)

Quoted 13 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) · line 762

Quoted 12 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -5

 	dpu_vbif_clear_errors(dpu_kms);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
-		if (encoder->crtc != crtc)
-			continue;
-
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_kickoff(encoder);
-	}

We wont be holding the modeset locks here (and in crtc_atomic_begin) in

the

Quoted 2 lines

display thread. Is
it safe to iterate over encoder_mask?

Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for dpu_crtc_commit_kickoff():

1- dpu_kms_encoder_enable() which is called via the encoder->funcs->enable hook
2- dpu_kms_commit() which is called in the mode_config->funcs->atomic_commit_tail

Both of these callsites will hold the modeset locks.

Am I missing something?

Thanks for your review,

Sean

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_disable(struct drm_crtc

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 end:
 	reinit_completion(&dpu_crtc->frame_done_comp);

@@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc

*crtc)

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	dpu_core_perf_crtc_update(crtc, 0, true);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder, NULL,
 NULL);
-	}
 

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_enable(struct drm_crtc

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	memset(cstate->mixers, 0, sizeof(cstate->mixers));

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	cstate->num_mixers = 0;

@@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc

*crtc,

Quoted 47 lines

Quoted 47 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder,
 				dpu_crtc_frame_event_cb, (void *)crtc);
-	}
 
 	mutex_lock(&dpu_crtc->crtc_lock);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +3 -0

index 1bec4540f3e1..64134d619748 100644
@@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	struct dpu_kms *dpu_kms;
 	struct msm_drm_private *priv;
 	struct drm_device *dev;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct drm_encoder *encoder;
+	int i;
 
 	if (!kms)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

 		return;

@@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	priv = dev->dev_private;
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc != NULL)
+	/* Call prepare_commit for all affected encoders */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_for_each_encoder_mask(encoder, crtc->dev,
+					  crtc_state->encoder_mask) {
 			dpu_encoder_prepare_commit(encoder);
+		}
+	}
 }
 

Quoted 2 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct

Quoted 2 lines

/*

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

@@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct

msm_kms

Quoted 25 lines

Quoted 19 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

 *kms,
 void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 {
 	const struct drm_encoder_helper_funcs *funcs =
 encoder->helper_private;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
 
 	/* Forward this enable call to the commit hook */
 	if (funcs && funcs->commit)
 		funcs->commit(encoder);
 
-	if (crtc && crtc->state->active) {
+	drm_for_each_crtc(crtc, dev) {
+		if (!(crtc->state->encoder_mask &
 drm_encoder_mask(encoder)))
+			continue;
+

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct msm_kms *kms, · line 367

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

 		trace_dpu_kms_enc_enable(DRMID(crtc));
 		dpu_crtc_commit_kickoff(crtc);
 	}

--
Jeykumar S

Quoted 114 lines · Sean Paul

Quoted 6 lines · Jeykumar Sankaran

Quoted 5 lines · Sean Paul

From: Sean Paul <[email protected]>

It's for legacy drivers, for atomic drivers crtc->state->encoder_mask should be used to map encoder to crtc.

Quoted 6 lines

Quoted 6 lines

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46

++++++++----------------

Quoted 80 lines

Quoted 80 lines

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
 2 files changed, 29 insertions(+), 36 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -3

index 156f4c77ca44..a008a87a8113 100644
@@ -284,9 +284,9 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
 drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
-	drm_for_each_encoder(encoder, crtc->dev)
-		if (encoder->crtc == crtc)
-			return dpu_encoder_get_intf_mode(encoder);
+	/* TODO: Returns the first INTF_MODE, could there be multiple

values? */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0

+	drm_for_each_encoder_mask(encoder, crtc->dev,

crtc->state->encoder_mask)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +3 -6

+		return dpu_encoder_get_intf_mode(encoder);
 
 	return INTF_MODE_NONE;
 }

@@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
 *crtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
-		if (encoder->crtc != crtc)
-			continue;
-
-		/* encoder will trigger pending mask now */
+	/* encoder will trigger pending mask now */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_trigger_kickoff_pending(encoder);
-	}

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 	/*
 	 * If no mixers have been allocated in dpu_crtc_atomic_check(),

@@ -704,7 +700,6 @@ static int _dpu_crtc_wait_for_frame_done(struct
 drm_crtc *crtc)
 void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -1

 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);

@@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
 *crtc)
 
 	DPU_ATRACE_BEGIN("crtc_commit");
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
+	/*
+	 * Encoder will flush/start now, unless it has a tx pending. If

so, it

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +4 -6

+	 * may delay and flush at an irq event (e.g. ppdone)
+	 */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
+				  crtc->state->encoder_mask) {
 		struct dpu_encoder_kickoff_params params = { 0 };
-
-		if (encoder->crtc != crtc)
-			continue;
-
-		/*
-		 * Encoder will flush/start now, unless it has a tx

pending.

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 * If so, it may delay and flush at an irq event (e.g.

ppdone)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 */
 		dpu_encoder_prepare_for_kickoff(encoder, &params);
 	}

Quoted 1 line

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc

Quoted 1 line

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

@@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc

*crtc)

Quoted 14 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) · line 762

Quoted 12 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -5

 	dpu_vbif_clear_errors(dpu_kms);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
 {
-		if (encoder->crtc != crtc)
-			continue;
-
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_kickoff(encoder);
-	}

We wont be holding the modeset locks here (and in crtc_atomic_begin) in

the

Quoted 2 lines

display thread. Is
it safe to iterate over encoder_mask?

Quoted 8 lines

Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for dpu_crtc_commit_kickoff():

1- dpu_kms_encoder_enable() which is called via the encoder->funcs->enable
hook
2- dpu_kms_commit() which is called in the mode_config->funcs->atomic_commit_tail

atomic_commit_tail will be called from the WQ. Do we hold the modeset locks in the WQ? I believe userspace thread will drop the locks at the end of drm_mode_atomic_ioctl.

Thanks

Quoted 105 lines

Both of these callsites will hold the modeset locks.

Am I missing something?

Thanks for your review,

Sean

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_disable(struct drm_crtc

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 end:
 	reinit_completion(&dpu_crtc->frame_done_comp);

@@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc

*crtc)

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	dpu_core_perf_crtc_update(crtc, 0, true);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder, NULL,
 NULL);
-	}
 

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_enable(struct drm_crtc

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	memset(cstate->mixers, 0, sizeof(cstate->mixers));

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	cstate->num_mixers = 0;

@@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc

*crtc,

Quoted 47 lines

Quoted 47 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder,
 				dpu_crtc_frame_event_cb, (void *)crtc);
-	}
 
 	mutex_lock(&dpu_crtc->crtc_lock);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +3 -0

index 1bec4540f3e1..64134d619748 100644
@@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	struct dpu_kms *dpu_kms;
 	struct msm_drm_private *priv;
 	struct drm_device *dev;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct drm_encoder *encoder;
+	int i;
 
 	if (!kms)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

 		return;

@@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct msm_kms
 *kms,
 	priv = dev->dev_private;
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc != NULL)
+	/* Call prepare_commit for all affected encoders */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_for_each_encoder_mask(encoder, crtc->dev,
+					  crtc_state->encoder_mask) {
 			dpu_encoder_prepare_commit(encoder);
+		}
+	}
 }
 

Quoted 2 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct

Quoted 2 lines

/*

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

@@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct

msm_kms

Quoted 25 lines

Quoted 19 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

 *kms,
 void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 {
 	const struct drm_encoder_helper_funcs *funcs =
 encoder->helper_private;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
 
 	/* Forward this enable call to the commit hook */
 	if (funcs && funcs->commit)
 		funcs->commit(encoder);
 
-	if (crtc && crtc->state->active) {
+	drm_for_each_crtc(crtc, dev) {
+		if (!(crtc->state->encoder_mask &
 drm_encoder_mask(encoder)))
+			continue;
+

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct msm_kms *kms, · line 367

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

 		trace_dpu_kms_enc_enable(DRMID(crtc));
 		dpu_crtc_commit_kickoff(crtc);
 	}

--
Jeykumar S

Signature · 1 line

Jeykumar S

Quoted 6 lines · Jeykumar Sankaran

Quoted 5 lines · Sean Paul

Quoted 4 lines · Jeykumar Sankaran

Quoted 3 lines · Sean Paul

From: Sean Paul <[email protected]>

It's for legacy drivers, for atomic drivers

crtc->state->encoder_mask

Quoted 12 lines

Quoted 12 lines

Quoted 8 lines

Quoted 8 lines

should be used to map encoder to crtc.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 46

++++++++----------------

Quoted 3 lines

Quoted 3 lines

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 19 +++++++---
 2 files changed, 29 insertions(+), 36 deletions(-)

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c enum dpu_intf_mode

Quoted 6 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

index 156f4c77ca44..a008a87a8113 100644
@@ -284,9 +284,9 @@ enum dpu_intf_mode

dpu_crtc_get_intf_mode(struct

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c enum dpu_intf_mode · line 288

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -3

 drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
-	drm_for_each_encoder(encoder, crtc->dev)
-		if (encoder->crtc == crtc)
-			return dpu_encoder_get_intf_mode(encoder);
+	/* TODO: Returns the first INTF_MODE, could there be

multiple

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_atomic_begin(struct

Quoted 8 lines

values? */

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0

+	drm_for_each_encoder_mask(encoder, crtc->dev,

crtc->state->encoder_mask)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0

+		return dpu_encoder_get_intf_mode(encoder);
 
 	return INTF_MODE_NONE;
 }

@@ -551,13 +551,9 @@ static void dpu_crtc_atomic_begin(struct

drm_crtc

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_atomic_begin(struct · line 555

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 *crtc,
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
-	list_for_each_entry(encoder,

&dev->mode_config.encoder_list, head)

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -5

 {
-		if (encoder->crtc != crtc)
-			continue;
-
-		/* encoder will trigger pending mask now */
+	/* encoder will trigger pending mask now */
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_trigger_kickoff_pending(encoder);
-	}

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c

Quoted 2 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	/*
 	 * If no mixers have been allocated in

dpu_crtc_atomic_check(),

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static int

Quoted 1 line

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

@@ -704,7 +700,6 @@ static int

_dpu_crtc_wait_for_frame_done(struct

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 drm_crtc *crtc)
 void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);

struct dpu_crtc_state *cstate =

to_dpu_crtc_state(crtc->state);

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc · line 724

Quoted 6 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

@@ -720,16 +715,13 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
 *crtc)
 
 	DPU_ATRACE_BEGIN("crtc_commit");
 
-	list_for_each_entry(encoder,

&dev->mode_config.encoder_list, head)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc · line 721

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -0

 {
+	/*
+	 * Encoder will flush/start now, unless it has a tx

pending. If

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

so, it

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +3 -0

+	 * may delay and flush at an irq event (e.g. ppdone)
+	 */
+	drm_for_each_encoder_mask(encoder, crtc->dev,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -6

+				  crtc->state->encoder_mask) {
 		struct dpu_encoder_kickoff_params params = { 0 };
-
-		if (encoder->crtc != crtc)
-			continue;
-
-		/*
-		 * Encoder will flush/start now, unless it has a

tx

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c

Quoted 2 lines

pending.

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 * If so, it may delay and flush at an irq event

(e.g.

Quoted 11 lines

Quoted 11 lines

Quoted 6 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc

Quoted 6 lines

ppdone)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-		 */
 		dpu_encoder_prepare_for_kickoff(encoder, &params);
 	}

@@ -754,12 +746,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc

*crtc)

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc · line 756

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

 	dpu_vbif_clear_errors(dpu_kms);
 
-	list_for_each_entry(encoder,

&dev->mode_config.encoder_list, head)

Quoted 23 lines

Quoted 14 lines

Quoted 10 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c void dpu_crtc_commit_kickoff(struct drm_crtc · line 763

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -4

 {
-		if (encoder->crtc != crtc)
-			continue;
-
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_kickoff(encoder);
-	}

We wont be holding the modeset locks here (and in crtc_atomic_begin) in

the

Quoted 2 lines

display thread. Is
it safe to iterate over encoder_mask?

Quoted 8 lines

Hmm, I'm not sure I follow. AFAICT, there are 2 callsites for dpu_crtc_commit_kickoff():

1- dpu_kms_encoder_enable() which is called via the encoder->funcs->enable
hook
2- dpu_kms_commit() which is called in the mode_config->funcs->atomic_commit_tail

Quoted 3 lines

atomic_commit_tail will be called from the WQ. Do we hold the modeset locks in the WQ? I believe userspace thread will drop the locks at the end of drm_mode_atomic_ioctl.

Accessing state is safe in non-blocking commits due to stalling any subsequent
commits until the previous one has completed. Check out the comment at:

https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/drm_atomic_helpe
r.c#n1754

Sean

Quoted 17 lines

Thanks

Quoted 15 lines

Both of these callsites will hold the modeset locks.

Am I missing something?

Thanks for your review,

Sean

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_disable(struct drm_crtc

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 end:
 	reinit_completion(&dpu_crtc->frame_done_comp);

@@ -883,11 +871,8 @@ static void dpu_crtc_disable(struct drm_crtc

*crtc)

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -3

 	dpu_core_perf_crtc_update(crtc, 0, true);
 
-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,

crtc->state->encoder_mask)
dpu_encoder_register_frame_event_callback(encoder,

NULL,

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_enable(struct drm_crtc

Quoted 6 lines

NULL);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-	}
 
 	memset(cstate->mixers, 0, sizeof(cstate->mixers));
 	cstate->num_mixers = 0;

@@ -922,12 +907,9 @@ static void dpu_crtc_enable(struct drm_crtc

*crtc,

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);
 

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c static void dpu_crtc_enable(struct drm_crtc · line 913

Quoted 7 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -3

-	drm_for_each_encoder(encoder, crtc->dev) {
-		if (encoder->crtc != crtc)
-			continue;
+	drm_for_each_encoder_mask(encoder, crtc->dev,
 crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder,
 				dpu_crtc_frame_event_cb, (void

*)crtc);

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct

Quoted 10 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -1

-	}
 
 	mutex_lock(&dpu_crtc->crtc_lock);

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

index 1bec4540f3e1..64134d619748 100644
@@ -320,7 +320,10 @@ static void dpu_kms_prepare_commit(struct

msm_kms

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +3 -0

 *kms,
 	struct dpu_kms *dpu_kms;
 	struct msm_drm_private *priv;
 	struct drm_device *dev;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
 	struct drm_encoder *encoder;
+	int i;
 

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

 	if (!kms)

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

 		return;

@@ -332,9 +335,13 @@ static void dpu_kms_prepare_commit(struct

msm_kms

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct · line 336

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -1

 *kms,
 	priv = dev->dev_private;
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
-	list_for_each_entry(encoder,

&dev->mode_config.encoder_list, head)

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct · line 342

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +4 -1

-		if (encoder->crtc != NULL)
+	/* Call prepare_commit for all affected encoders */
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		drm_for_each_encoder_mask(encoder, crtc->dev,
+

crtc_state->encoder_mask) {

Quoted 36 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +2 -0

 			dpu_encoder_prepare_commit(encoder);
+		}
+	}
 }
 

Quoted 28 lines

Quoted 2 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void dpu_kms_prepare_commit(struct

Quoted 2 lines

/*

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

@@ -344,13 +351,17 @@ static void dpu_kms_prepare_commit(struct

msm_kms

Quoted 25 lines

Quoted 19 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +6 -2

 *kms,
 void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 {
 	const struct drm_encoder_helper_funcs *funcs =
 encoder->helper_private;
-	struct drm_crtc *crtc = encoder->crtc;
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
 
 	/* Forward this enable call to the commit hook */
 	if (funcs && funcs->commit)
 		funcs->commit(encoder);
 
-	if (crtc && crtc->state->active) {
+	drm_for_each_crtc(crtc, dev) {
+		if (!(crtc->state->encoder_mask &
 drm_encoder_mask(encoder)))
+			continue;
+

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c

Quoted 3 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

 		trace_dpu_kms_enc_enable(DRMID(crtc));
 		dpu_crtc_commit_kickoff(crtc);
 	}

--
Jeykumar S

--
Jeykumar S

From: Sean Paul <[email protected]>

Add modeset lock checks to functions that could be called outside the core atomic stack.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 1 +
 2 files changed, 3 insertions(+)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -0

index a008a87a8113..cd0a0bea4335 100644
@@ -284,6 +284,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct
 drm_crtc *crtc)
 		return INTF_MODE_NONE;
 	}
 
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 	/* TODO: Returns the first INTF_MODE, could there be multiple

values? */
drm_for_each_encoder_mask(encoder, crtc->dev,
crtc->state->encoder_mask)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -0

 		return dpu_encoder_get_intf_mode(encoder);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +1 -0

index 64134d619748..5104fc01147e 100644
@@ -358,6 +358,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
 *encoder)
 	if (funcs && funcs->commit)
 		funcs->commit(encoder);
 
+
 WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 	drm_for_each_crtc(crtc, dev) {

if (!(crtc->state->encoder_mask &
drm_encoder_mask(encoder)))
continue;

From: Sean Paul <[email protected]>

Add modeset lock checks to functions that could be called outside the core atomic stack.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 1 +
 2 files changed, 3 insertions(+)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -0

index a008a87a8113..cd0a0bea4335 100644
@@ -284,6 +284,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
                return INTF_MODE_NONE;
        }
 
+       WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
        /* TODO: Returns the first INTF_MODE, could there be multiple values? */
        drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
                return dpu_encoder_get_intf_mode(encoder);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

index 64134d619748..5104fc01147e 100644

Quoted 36 lines · Sean Paul

From: Sean Paul <[email protected]>

Add modeset lock checks to functions that could be called outside the core atomic stack.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 1 +
 2 files changed, 3 insertions(+)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -0

index a008a87a8113..cd0a0bea4335 100644
@@ -284,6 +284,8 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
                return INTF_MODE_NONE;
        }
 
+       WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
        /* TODO: Returns the first INTF_MODE, could there be multiple values? */
        drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
                return dpu_encoder_get_intf_mode(encoder);

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0

index 64134d619748..5104fc01147e 100644
@@ -358,6 +358,7 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
        if (funcs && funcs->commit)
                funcs->commit(encoder);
 

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c void dpu_kms_encoder_enable(struct drm_encoder *encoder) · line 364

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +1 -0

+       WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
        drm_for_each_crtc(crtc, dev) {
                if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
                        continue;

I'm fairly sure this is called in the atomic_commit path, and in there you might not actually hold these locks (if you do a nonblocking modeset).

The locking rules for ->state are pretty fun: Either hold the lock, or be in atomic commit. In the later case atomic helpers' commit ordering guarantees that you can safely access ->state (but read-only only) without hodling any locks. You might want to revert.

Don't ask why I stumbled over this.

-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

From: Sean Paul <[email protected]>

There are 4 times that _dpu_crtc_vblank_enable_no_lock() is called:

1- crtc enable
2- crtc disable
3- crtc vblank enable
4- crtc vblank disable

When we enable or disable the crtc, we call drm_crtc_vblank_on and drm_crtc_vblank_off respectively. That will gate vblank enables and disables to only being called when the crtc is active. That means that we can just enable/disable pm runtime in crtc enable/disable. This will be beneficial in trying to eliminate blocking calls from the vblank call chain.

Changes in v2:
- None

Signed-off-by Sean Paul <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +0 -2

index cd0a0bea4335..c49aaf412b6e 100644
@@ -769,8 +769,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
 	struct drm_encoder *enc;
 
 	if (enable) {
-		pm_runtime_get_sync(dev->dev);
-
 		list_for_each_entry(enc, &dev->mode_config.encoder_list,
 head) {
 			if (enc->crtc != crtc)

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +4 -2

 				continue;

@@ -793,8 +791,6 @@ static void _dpu_crtc_vblank_enable_no_lock(
 
 			dpu_encoder_register_vblank_callback(enc, NULL,
 NULL);
 		}
-
-		pm_runtime_put_sync(dev->dev);
 	}
 }

@@ -891,6 +887,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
 		crtc->state->event = NULL;
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 	}
+
+	pm_runtime_put_sync(crtc->dev->dev);
 }
 
 static void dpu_crtc_enable(struct drm_crtc *crtc,

@@ -906,6 +904,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 	}
 	priv = crtc->dev->dev_private;
 
+	pm_runtime_get_sync(crtc->dev->dev);
+
 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 	dpu_crtc = to_dpu_crtc(crtc);