On Tue, Aug 20, 2019 at 04:12:28PM -0700, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Previously, dpu_crtc_frame_event_work() would try to aquire all the modeset locks in order to check whether it can release bandwidth. (If we only have cmd-mode display, bandwidth can be released at frame-done time.)
The problem with this is that it is also responsible for signalling frame_done_comp, which dpu_crtc_commit_kickoff() waits on if there is already a frame pending. This is called in the msm_atomic_commit_tail() path.. which means that for non-nonblock commits, at least some of the modeset locks are already held.
Re-work this scheme to use a reference count to track our need to have clocks enabled. It is incremented for each atomic commit, and decremented in the corresponding frame-done. Additionally, any crtc used in video mode hold an extra reference while they are enabled. The net effect is that we can determine in frame-done whether it is safe to drop bandwidth without needing to aquire any modeset locks.
Signed-off-by: Rob Clark robdclark@chromium.org
All looks good to me, nice cleanup!
Reviewed-by: Sean Paul sean@chromium.org
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 16 +------ drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 45 +++++++++++-------- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 8 ++++ 4 files changed, 38 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c index 5cda96875e03..09a49b59bb5b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c @@ -214,7 +214,6 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms, */ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc) {
- struct drm_crtc *tmp_crtc; struct dpu_crtc *dpu_crtc; struct dpu_crtc_state *dpu_cstate; struct dpu_kms *kms;
@@ -233,22 +232,9 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc) dpu_crtc = to_dpu_crtc(crtc); dpu_cstate = to_dpu_crtc_state(crtc->state);
- /* only do this for command mode rt client */
- if (dpu_crtc_get_intf_mode(crtc) != INTF_MODE_CMD)
- if (atomic_dec_return(&kms->bandwidth_ref) > 0) return;
- /*
* If video interface present, cmd panel bandwidth cannot be* released.*/- if (dpu_crtc_get_intf_mode(crtc) == INTF_MODE_CMD)
drm_for_each_crtc(tmp_crtc, crtc->dev) {if (tmp_crtc->enabled &&dpu_crtc_get_intf_mode(tmp_crtc) ==INTF_MODE_VIDEO)return;}- /* Release the bandwidth */ if (kms->perf.enable_bw_release) { trace_dpu_cmd_release_bw(crtc->base.id);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index b3417d56032d..4e54550c4a80 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -292,19 +292,6 @@ void dpu_crtc_vblank_callback(struct drm_crtc *crtc) 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) { struct dpu_crtc_frame_event *fevent = container_of(work, @@ -334,7 +321,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_crtc_release_bw_unlocked(crtc);
dpu_core_perf_crtc_release_bw(crtc);@@ -650,7 +637,7 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async) dpu_encoder_prepare_for_kickoff(encoder, async);
if (!async) {
/* wait for frame_event_done completion */
/* wait for previous frame_event_done completion */@@ -729,6 +716,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, struct drm_encoder *encoder; struct msm_drm_private *priv; unsigned long flags;
bool release_bandwidth = false;
if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) { DPU_ERROR("invalid crtc\n");
@@ -745,8 +733,15 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, drm_crtc_vblank_off(crtc);
drm_for_each_encoder_mask(encoder, crtc->dev,
old_crtc_state->encoder_mask)
old_crtc_state->encoder_mask) {/* in video mode, we hold an extra bandwidth reference* as we cannot drop bandwidth at frame-done if any* crtc is being used in video mode.*/if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)release_bandwidth = true;dpu_encoder_assign_crtc(encoder, NULL);
}
/* wait for frame_event_done completion */ if (_dpu_crtc_wait_for_frame_done(crtc))
@@ -760,7 +755,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, if (atomic_read(&dpu_crtc->frame_pending)) { trace_dpu_crtc_disable_frame_pending(DRMID(crtc), atomic_read(&dpu_crtc->frame_pending));
dpu_core_perf_crtc_release_bw(crtc);
if (release_bandwidth)dpu_core_perf_crtc_release_bw(crtc);@@ -792,6 +788,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, struct dpu_crtc *dpu_crtc; struct drm_encoder *encoder; struct msm_drm_private *priv;
bool request_bandwidth;
if (!crtc || !crtc->dev || !crtc->dev->dev_private) { DPU_ERROR("invalid crtc\n");
@@ -804,9 +801,19 @@ 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_mask(encoder, crtc->dev, crtc->state->encoder_mask)
drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) {
/* in video mode, we hold an extra bandwidth reference* as we cannot drop bandwidth at frame-done if any* crtc is being used in video mode.*/if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)request_bandwidth = true;dpu_encoder_register_frame_event_callback(encoder, dpu_crtc_frame_event_cb, (void *)crtc);
}
if (request_bandwidth)
atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref);trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); dpu_crtc->enabled = true;
@@ -981,6 +988,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, } }
- atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref);
- rc = dpu_core_perf_crtc_check(crtc, state); if (rc) { DPU_ERROR("crtc%d failed performance check %d\n",
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 99d0bd569c38..681955eb286f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -800,6 +800,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms) return rc; }
- atomic_set(&dpu_kms->bandwidth_ref, 0);
- dpu_kms->mmio = msm_ioremap(dpu_kms->pdev, "mdp", "mdp"); if (IS_ERR(dpu_kms->mmio)) { rc = PTR_ERR(dpu_kms->mmio);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index 9e40f559c51f..fdef016f5ca3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -120,6 +120,14 @@ struct dpu_kms { struct platform_device *pdev; bool rpm_enabled; struct dss_module_power mp;
- /* reference count bandwidth requests, so we know when we can
* release bandwidth. Each atomic update increments, and frame-* done event decrements. Additionally, for video mode, the* reference is incremented when crtc is enabled, and decremented* when disabled.*/- atomic_t bandwidth_ref;
};
struct vsync_info {
2.21.0