Skip to contentSkip to conversation

drm/msm: dpu: Grab the modeset locks in frame_event

PATCH In review Sean Paul 3 participants 5 messages Nov 12, 2018 – Nov 26, 2018 2 messages not in the archive

Subsystems (3)
lore
Share & tools
lore

Sean Paul

author reviewer
#0 lore raw

PATCH 1/3 drm/msm: dpu: Grab the modeset locks in frame_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.

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +2 -0
index ed84cf44a222..6e636f373950 100644
@@ -338,7 +338,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);
 		} else {
 			trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),
 								fevent->event);
Signature · 6 lines
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 20181112214713.69826-1-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org

#2 lore raw

PATCH 2/3 drm/msm: dpu: Stop using encoder->crtc pointer

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.

---
 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 +14 -32
index 6e636f373950..bf541a942da8 100644
@@ -287,9 +287,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? */
+	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
+		return dpu_encoder_get_intf_mode(encoder);
 
 	return INTF_MODE_NONE;
 }
@@ -554,13 +554,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);
-	}
 
 	/*
 	 * If no mixers have been allocated in dpu_crtc_atomic_check(),
@@ -707,7 +703,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);
 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
@@ -723,16 +718,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
+	 * 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.
-		 * If so, it may delay and flush at an irq event (e.g. ppdone)
-		 */
 		dpu_encoder_prepare_for_kickoff(encoder, &params);
 	}
 
@@ -757,12 +749,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 
 	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);
-	}
 
 end:
 	reinit_completion(&dpu_crtc->frame_done_comp);
@@ -913,11 +901,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);
-	}
 
 	if (dpu_crtc->power_event)
 		dpu_power_handle_unregister_event(dpu_crtc->phandle,
@@ -956,12 +941,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);
 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +15 -4
index 985c855796ae..78709699fe1e 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)
 		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);
+		}
+	}
 }
 
 /*
@@ -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;
+
 		trace_dpu_kms_enc_enable(DRMID(crtc));
 		dpu_crtc_commit_kickoff(crtc);
 	}
Signature · 6 lines
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
#3 lore raw

PATCH 3/3 drm/msm: dpu: Add modeset lock checks where applicable

In reply to a message not in the archive

From: Sean Paul <[email protected]>

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

---
 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 bf541a942da8..8c34fb3b9286 100644
@@ -287,6 +287,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 +1 -0
index 78709699fe1e..aa32c353607b 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;
Signature · 6 lines
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
#4 lore raw
In reply to Sean Paul
Quoted 15 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.

---
 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 ed84cf44a222..6e636f373950 100644

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

Quoted 4 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0
@@ -338,7 +338,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);

From the kerneldoc: "This function is deprecated" Please don't use it in new code :-)

For locking all the state you want lock_all_ctx.
-Daniel

Quoted 12 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0
 			dpu_core_perf_crtc_release_bw(crtc);
+			drm_modeset_unlock_all(crtc->dev);
 		} else {
 			trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),
 								fevent->event);
Signature · 6 lines
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 20181121092957.GI4266-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org

#6 lore raw
Quoted 24 lines · Daniel Vetter
Quoted 15 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.

---
 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 ed84cf44a222..6e636f373950 100644

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

Quoted 4 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0
@@ -338,7 +338,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);

From the kerneldoc: "This function is deprecated" Please don't use it in new code :-)

Quoted 1 line

For locking all the state you want lock_all_ctx.

If you're looking for me, I'll be in my (frequently used) cone of shame :(

Thanks for catching this, v2 incoming!

Sean

Quoted 19 lines

-Daniel

Quoted 12 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +1 -0
 			dpu_core_perf_crtc_release_bw(crtc);
+			drm_modeset_unlock_all(crtc->dev);
 		} else {
 			trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),
 								fevent->event);
Signature · 6 lines
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno