drm/msm: dpu: Grab the modeset locks in frame_event
Sean Paul
reviewerPATCH 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
@@ -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
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
@@ -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, ¶ms); }
@@ -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
@@ -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
PATCH 3/3 drm/msm: dpu: Add modeset lock checks where applicable
In reply to a message not in the archiveFrom: 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
@@ -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
@@ -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
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.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
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
-0dpu_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
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.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
-0drivers/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
-0dpu_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-develSignature · 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