| Message ID | 20180228191906.185417-3-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-02-28 11:18, Sean Paul wrote: > Instead, shuffle things around so we kickoff crtc after enabling > encoder > during modesets. Also moves the vblank wait to after the frame. > > Change-Id: I16c7b7f9390d04f6050aa20e17a5335fbf49eba3 > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 31 ++++- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 + > drivers/gpu/drm/msm/msm_atomic.c | 132 +------------------- > 5 files changed, 48 insertions(+), 131 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index a3ab6ed2bf1d..94fab2dcca5b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -3525,6 +3525,12 @@ static void dpu_crtc_enable(struct drm_crtc > *crtc, > DPU_EVT32_VERBOSE(DRMID(crtc)); > dpu_crtc = to_dpu_crtc(crtc); > > + if (msm_is_mode_seamless(&crtc->state->adjusted_mode) || > + msm_is_mode_seamless_vrr(&crtc->state->adjusted_mode)) { > + DPU_DEBUG("Skipping crtc enable, seamless mode\n"); > + return; > + } > + > pm_runtime_get_sync(crtc->dev->dev); > > drm_for_each_encoder(encoder, crtc->dev) { > @@ -3572,6 +3578,9 @@ static void dpu_crtc_enable(struct drm_crtc > *crtc, > DPU_POWER_EVENT_POST_ENABLE | DPU_POWER_EVENT_POST_DISABLE > | > DPU_POWER_EVENT_PRE_DISABLE, > dpu_crtc_handle_power_event, crtc, dpu_crtc->name); > + > + if (msm_needs_vblank_pre_modeset(&crtc->state->adjusted_mode)) > + drm_crtc_wait_one_vblank(crtc); > } > > struct plane_state { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 28ceb589ee40..4d1e3652dbf4 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -3693,8 +3693,11 @@ static void > dpu_encoder_frame_done_timeout(struct > timer_list *t) > static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs > = { > .mode_set = dpu_encoder_virt_mode_set, > .disable = dpu_encoder_virt_disable, > - .enable = dpu_encoder_virt_enable, > + .enable = dpu_kms_encoder_enable, > .atomic_check = dpu_encoder_virt_atomic_check, > + > + /* This is called by dpu_kms_encoder_enable */ > + .commit = dpu_encoder_virt_enable, > }; > > static const struct drm_encoder_funcs dpu_encoder_funcs = { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 81fd3a429e9f..3d83037e8305 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -425,14 +425,37 @@ static void dpu_kms_prepare_commit(struct msm_kms > *kms, > dpu_encoder_prepare_commit(encoder); > } > > -static void dpu_kms_commit(struct msm_kms *kms, > - struct drm_atomic_state *old_state) > +/* > + * Override the encoder enable since we need to setup the inline > rotator > and do > + * some crtc magic before enabling any bridge that might be present. > + */ > +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; > + > + /* Forward this enable call to the commit hook */ > + if (funcs && funcs->commit) > + funcs->commit(encoder); The purpose of this function is not clear. Where are we setting up the inline rotator? Why do we need a kickoff here? > + > + if (crtc && crtc->state->active) { > + DPU_EVT32(DRMID(crtc)); > + dpu_crtc_commit_kickoff(crtc); > + } > +} > + > +static void dpu_kms_commit(struct msm_kms *kms, struct > drm_atomic_state > *state) > { > struct drm_crtc *crtc; > - struct drm_crtc_state *old_crtc_state; > + struct drm_crtc_state *crtc_state; > + struct dpu_crtc_state *cstate; > int i; > > - for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) { > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > + /* If modeset is required, kickoff is run in > encoder_enable */ > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > + continue; > + > if (crtc->state->active) { > DPU_EVT32(DRMID(crtc)); > dpu_crtc_commit_kickoff(crtc); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > index 8cadd29a48b1..42c809ed9467 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > @@ -529,4 +529,6 @@ int dpu_kms_fbo_reference(struct dpu_kms_fbo *fbo); > */ > void dpu_kms_fbo_unreference(struct dpu_kms_fbo *fbo); > > +void dpu_kms_encoder_enable(struct drm_encoder *encoder); > + > #endif /* __dpu_kms_H__ */ > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > b/drivers/gpu/drm/msm/msm_atomic.c > index 5cfb80345052..f5794dce25dd 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -84,131 +84,6 @@ static void msm_atomic_wait_for_commit_done( > } > } > > -/** > - * msm_atomic_helper_commit_modeset_enables - modeset commit to enable > outputs > - * @dev: DRM device > - * @old_state: atomic state object with old state structures > - * > - * This function enables all the outputs with the new configuration > which > had to > - * be turned off for the update. > - * > - * For compatibility with legacy crtc helpers this should be called > after > - * drm_atomic_helper_commit_planes(), which is what the default commit > function > - * does. But drivers with different needs can group the modeset > commits > together > - * and do the plane commits at the end. This is useful for drivers > doing > runtime > - * PM since planes updates then only happen when the CRTC is actually > enabled. > - */ > -static void msm_atomic_helper_commit_modeset_enables(struct drm_device > *dev, > - struct drm_atomic_state *old_state) > -{ > - struct drm_crtc *crtc; > - struct drm_crtc_state *old_crtc_state; > - struct drm_crtc_state *new_crtc_state; > - struct drm_connector *connector; > - struct drm_connector_state *new_conn_state; > - struct msm_drm_private *priv = dev->dev_private; > - struct msm_kms *kms = priv->kms; > - int bridge_enable_count = 0; > - int i; > - > - for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > - new_crtc_state, i) { > - const struct drm_crtc_helper_funcs *funcs; > - > - /* Need to filter out CRTCs where only planes change. */ > - if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) > - continue; > - > - if (!new_crtc_state->active) > - continue; > - > - if (msm_is_mode_seamless(&new_crtc_state->mode) || > - msm_is_mode_seamless_vrr( > - &new_crtc_state->adjusted_mode)) > - continue; > - > - funcs = crtc->helper_private; > - > - if (crtc->state->enable) { > - DRM_DEBUG_ATOMIC("enabling [CRTC:%d]\n", > - crtc->base.id); > - > - if (funcs->atomic_enable) > - funcs->atomic_enable(crtc, > old_crtc_state); > - else > - funcs->commit(crtc); > - } > - > - if (msm_needs_vblank_pre_modeset( > - &new_crtc_state->adjusted_mode)) > - drm_crtc_wait_one_vblank(crtc); > - } > - > - /* ensure bridge/encoder updates happen on same vblank */ > - msm_atomic_wait_for_commit_done(dev, old_state); > - > - for_each_new_connector_in_state(old_state, connector, > - new_conn_state, i) { > - const struct drm_encoder_helper_funcs *funcs; > - struct drm_encoder *encoder; > - > - if (!new_conn_state->best_encoder) > - continue; > - > - if (!new_conn_state->crtc->state->active || > - !drm_atomic_crtc_needs_modeset( > - new_conn_state->crtc->state)) > - continue; > - > - encoder = new_conn_state->best_encoder; > - funcs = encoder->helper_private; > - > - DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n", > - encoder->base.id, encoder->name); > - > - /* > - * Each encoder has at most one connector (since we always > steal > - * it away), so we won't call enable hooks twice. > - */ > - drm_bridge_pre_enable(encoder->bridge); > - ++bridge_enable_count; > - > - if (funcs->enable) > - funcs->enable(encoder); > - else > - funcs->commit(encoder); > - } > - > - if (kms->funcs->commit) { > - DRM_DEBUG_ATOMIC("triggering commit\n"); > - kms->funcs->commit(kms, old_state); > - } > - > - /* If no bridges were pre_enabled, skip iterating over them again > */ > - if (bridge_enable_count == 0) > - return; > - > - for_each_new_connector_in_state(old_state, connector, > - new_conn_state, i) { > - struct drm_encoder *encoder; > - > - if (!new_conn_state->best_encoder) > - continue; > - > - if (!new_conn_state->crtc->state->active || > - !drm_atomic_crtc_needs_modeset( > - new_conn_state->crtc->state)) > - continue; > - > - encoder = new_conn_state->best_encoder; > - > - DRM_DEBUG_ATOMIC("bridge enable enabling > [ENCODER:%d:%s]\n", > - encoder->base.id, encoder->name); > - > - drm_bridge_enable(encoder->bridge); > - } > -} > - > /* The (potentially) asynchronous part of the commit. At this point > * nothing can fail short of armageddon. > */ > @@ -227,7 +102,12 @@ static void complete_commit(struct msm_commit *c) > > drm_atomic_helper_commit_planes(dev, state, 0); > > - msm_atomic_helper_commit_modeset_enables(dev, state); > + drm_atomic_helper_commit_modeset_enables(dev, state); > + > + if (kms->funcs->commit) { > + DRM_DEBUG_ATOMIC("triggering commit\n"); > + kms->funcs->commit(kms, state); > + } > > /* NOTE: _wait_for_vblanks() only waits for vblank on > * enabled CRTCs. So we end up faulting when disabling
On Thu, Mar 08, 2018 at 04:56:01PM -0800, Jeykumar Sankaran wrote: > On 2018-02-28 11:18, Sean Paul wrote: > > Instead, shuffle things around so we kickoff crtc after enabling encoder > > during modesets. Also moves the vblank wait to after the frame. > > > > Change-Id: I16c7b7f9390d04f6050aa20e17a5335fbf49eba3 > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++ > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 31 ++++- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 + > > drivers/gpu/drm/msm/msm_atomic.c | 132 +------------------- > > 5 files changed, 48 insertions(+), 131 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index a3ab6ed2bf1d..94fab2dcca5b 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -3525,6 +3525,12 @@ static void dpu_crtc_enable(struct drm_crtc > > *crtc, > > DPU_EVT32_VERBOSE(DRMID(crtc)); > > dpu_crtc = to_dpu_crtc(crtc); > > > > + if (msm_is_mode_seamless(&crtc->state->adjusted_mode) || > > + msm_is_mode_seamless_vrr(&crtc->state->adjusted_mode)) { > > + DPU_DEBUG("Skipping crtc enable, seamless mode\n"); > > + return; > > + } > > + > > pm_runtime_get_sync(crtc->dev->dev); > > > > drm_for_each_encoder(encoder, crtc->dev) { > > @@ -3572,6 +3578,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, > > DPU_POWER_EVENT_POST_ENABLE | DPU_POWER_EVENT_POST_DISABLE > > | > > DPU_POWER_EVENT_PRE_DISABLE, > > dpu_crtc_handle_power_event, crtc, dpu_crtc->name); > > + > > + if (msm_needs_vblank_pre_modeset(&crtc->state->adjusted_mode)) > > + drm_crtc_wait_one_vblank(crtc); > > } > > > > struct plane_state { > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 28ceb589ee40..4d1e3652dbf4 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -3693,8 +3693,11 @@ static void dpu_encoder_frame_done_timeout(struct > > timer_list *t) > > static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = > > { > > .mode_set = dpu_encoder_virt_mode_set, > > .disable = dpu_encoder_virt_disable, > > - .enable = dpu_encoder_virt_enable, > > + .enable = dpu_kms_encoder_enable, > > .atomic_check = dpu_encoder_virt_atomic_check, > > + > > + /* This is called by dpu_kms_encoder_enable */ > > + .commit = dpu_encoder_virt_enable, > > }; > > > > static const struct drm_encoder_funcs dpu_encoder_funcs = { > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > index 81fd3a429e9f..3d83037e8305 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > @@ -425,14 +425,37 @@ static void dpu_kms_prepare_commit(struct msm_kms > > *kms, > > dpu_encoder_prepare_commit(encoder); > > } > > > > -static void dpu_kms_commit(struct msm_kms *kms, > > - struct drm_atomic_state *old_state) > > +/* > > + * Override the encoder enable since we need to setup the inline > > rotator > > and do > > + * some crtc magic before enabling any bridge that might be present. > > + */ > > +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; > > + > > + /* Forward this enable call to the commit hook */ > > + if (funcs && funcs->commit) > > + funcs->commit(encoder); > > The purpose of this function is not clear. Where are we setting up the > inline rotator? > Why do we need a kickoff here? The reason the code is shuffled is to avoid duplicating the entire atomic helper function. By moving calls into the ->enable hooks, we can avoid having to hand roll the helpers. The kickoff is preserved from the helper copy when you call kms->funcs->commit in between the encoder enable and bridge enable. If this can be removed, that'd be even better. I was simply trying to preserve the call order of everything. Sean > > + > > + if (crtc && crtc->state->active) { > > + DPU_EVT32(DRMID(crtc)); > > + dpu_crtc_commit_kickoff(crtc); > > + } > > +} > > + > > +static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state > > *state) > > { > > struct drm_crtc *crtc; > > - struct drm_crtc_state *old_crtc_state; > > + struct drm_crtc_state *crtc_state; > > + struct dpu_crtc_state *cstate; > > int i; > > > > - for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) { > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > > + /* If modeset is required, kickoff is run in > > encoder_enable */ > > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > > + continue; > > + > > if (crtc->state->active) { > > DPU_EVT32(DRMID(crtc)); > > dpu_crtc_commit_kickoff(crtc); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > index 8cadd29a48b1..42c809ed9467 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > @@ -529,4 +529,6 @@ int dpu_kms_fbo_reference(struct dpu_kms_fbo *fbo); > > */ > > void dpu_kms_fbo_unreference(struct dpu_kms_fbo *fbo); > > > > +void dpu_kms_encoder_enable(struct drm_encoder *encoder); > > + > > #endif /* __dpu_kms_H__ */ > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > > b/drivers/gpu/drm/msm/msm_atomic.c > > index 5cfb80345052..f5794dce25dd 100644 > > --- a/drivers/gpu/drm/msm/msm_atomic.c > > +++ b/drivers/gpu/drm/msm/msm_atomic.c > > @@ -84,131 +84,6 @@ static void msm_atomic_wait_for_commit_done( > > } > > } > > > > -/** > > - * msm_atomic_helper_commit_modeset_enables - modeset commit to enable > > outputs > > - * @dev: DRM device > > - * @old_state: atomic state object with old state structures > > - * > > - * This function enables all the outputs with the new configuration > > which > > had to > > - * be turned off for the update. > > - * > > - * For compatibility with legacy crtc helpers this should be called > > after > > - * drm_atomic_helper_commit_planes(), which is what the default commit > > function > > - * does. But drivers with different needs can group the modeset commits > > together > > - * and do the plane commits at the end. This is useful for drivers > > doing > > runtime > > - * PM since planes updates then only happen when the CRTC is actually > > enabled. > > - */ > > -static void msm_atomic_helper_commit_modeset_enables(struct drm_device > > *dev, > > - struct drm_atomic_state *old_state) > > -{ > > - struct drm_crtc *crtc; > > - struct drm_crtc_state *old_crtc_state; > > - struct drm_crtc_state *new_crtc_state; > > - struct drm_connector *connector; > > - struct drm_connector_state *new_conn_state; > > - struct msm_drm_private *priv = dev->dev_private; > > - struct msm_kms *kms = priv->kms; > > - int bridge_enable_count = 0; > > - int i; > > - > > - for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > > - new_crtc_state, i) { > > - const struct drm_crtc_helper_funcs *funcs; > > - > > - /* Need to filter out CRTCs where only planes change. */ > > - if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) > > - continue; > > - > > - if (!new_crtc_state->active) > > - continue; > > - > > - if (msm_is_mode_seamless(&new_crtc_state->mode) || > > - msm_is_mode_seamless_vrr( > > - &new_crtc_state->adjusted_mode)) > > - continue; > > - > > - funcs = crtc->helper_private; > > - > > - if (crtc->state->enable) { > > - DRM_DEBUG_ATOMIC("enabling [CRTC:%d]\n", > > - crtc->base.id); > > - > > - if (funcs->atomic_enable) > > - funcs->atomic_enable(crtc, > > old_crtc_state); > > - else > > - funcs->commit(crtc); > > - } > > - > > - if (msm_needs_vblank_pre_modeset( > > - &new_crtc_state->adjusted_mode)) > > - drm_crtc_wait_one_vblank(crtc); > > - } > > - > > - /* ensure bridge/encoder updates happen on same vblank */ > > - msm_atomic_wait_for_commit_done(dev, old_state); > > - > > - for_each_new_connector_in_state(old_state, connector, > > - new_conn_state, i) { > > - const struct drm_encoder_helper_funcs *funcs; > > - struct drm_encoder *encoder; > > - > > - if (!new_conn_state->best_encoder) > > - continue; > > - > > - if (!new_conn_state->crtc->state->active || > > - !drm_atomic_crtc_needs_modeset( > > - new_conn_state->crtc->state)) > > - continue; > > - > > - encoder = new_conn_state->best_encoder; > > - funcs = encoder->helper_private; > > - > > - DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n", > > - encoder->base.id, encoder->name); > > - > > - /* > > - * Each encoder has at most one connector (since we always > > steal > > - * it away), so we won't call enable hooks twice. > > - */ > > - drm_bridge_pre_enable(encoder->bridge); > > - ++bridge_enable_count; > > - > > - if (funcs->enable) > > - funcs->enable(encoder); > > - else > > - funcs->commit(encoder); > > - } > > - > > - if (kms->funcs->commit) { > > - DRM_DEBUG_ATOMIC("triggering commit\n"); > > - kms->funcs->commit(kms, old_state); > > - } > > - > > - /* If no bridges were pre_enabled, skip iterating over them again > > */ > > - if (bridge_enable_count == 0) > > - return; > > - > > - for_each_new_connector_in_state(old_state, connector, > > - new_conn_state, i) { > > - struct drm_encoder *encoder; > > - > > - if (!new_conn_state->best_encoder) > > - continue; > > - > > - if (!new_conn_state->crtc->state->active || > > - !drm_atomic_crtc_needs_modeset( > > - new_conn_state->crtc->state)) > > - continue; > > - > > - encoder = new_conn_state->best_encoder; > > - > > - DRM_DEBUG_ATOMIC("bridge enable enabling > > [ENCODER:%d:%s]\n", > > - encoder->base.id, encoder->name); > > - > > - drm_bridge_enable(encoder->bridge); > > - } > > -} > > - > > /* The (potentially) asynchronous part of the commit. At this point > > * nothing can fail short of armageddon. > > */ > > @@ -227,7 +102,12 @@ static void complete_commit(struct msm_commit *c) > > > > drm_atomic_helper_commit_planes(dev, state, 0); > > > > - msm_atomic_helper_commit_modeset_enables(dev, state); > > + drm_atomic_helper_commit_modeset_enables(dev, state); > > + > > + if (kms->funcs->commit) { > > + DRM_DEBUG_ATOMIC("triggering commit\n"); > > + kms->funcs->commit(kms, state); > > + } > > > > /* NOTE: _wait_for_vblanks() only waits for vblank on > > * enabled CRTCs. So we end up faulting when disabling > > -- > Jeykumar S
On 2018-03-12 13:21, Sean Paul wrote: > On Thu, Mar 08, 2018 at 04:56:01PM -0800, Jeykumar Sankaran wrote: >> On 2018-02-28 11:18, Sean Paul wrote: >> > Instead, shuffle things around so we kickoff crtc after enabling > encoder >> > during modesets. Also moves the vblank wait to after the frame. >> > >> > Change-Id: I16c7b7f9390d04f6050aa20e17a5335fbf49eba3 >> > Signed-off-by: Sean Paul <seanpaul@chromium.org> >> > --- >> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++ >> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +- >> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 31 ++++- >> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 + >> > drivers/gpu/drm/msm/msm_atomic.c | 132 > +------------------- >> > 5 files changed, 48 insertions(+), 131 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > index a3ab6ed2bf1d..94fab2dcca5b 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > @@ -3525,6 +3525,12 @@ static void dpu_crtc_enable(struct drm_crtc >> > *crtc, >> > DPU_EVT32_VERBOSE(DRMID(crtc)); >> > dpu_crtc = to_dpu_crtc(crtc); >> > >> > + if (msm_is_mode_seamless(&crtc->state->adjusted_mode) || >> > + msm_is_mode_seamless_vrr(&crtc->state->adjusted_mode)) { >> > + DPU_DEBUG("Skipping crtc enable, seamless mode\n"); >> > + return; >> > + } >> > + >> > pm_runtime_get_sync(crtc->dev->dev); >> > >> > drm_for_each_encoder(encoder, crtc->dev) { >> > @@ -3572,6 +3578,9 @@ static void dpu_crtc_enable(struct drm_crtc > *crtc, >> > DPU_POWER_EVENT_POST_ENABLE | DPU_POWER_EVENT_POST_DISABLE >> > | >> > DPU_POWER_EVENT_PRE_DISABLE, >> > dpu_crtc_handle_power_event, crtc, dpu_crtc->name); >> > + >> > + if (msm_needs_vblank_pre_modeset(&crtc->state->adjusted_mode)) >> > + drm_crtc_wait_one_vblank(crtc); >> > } >> > >> > struct plane_state { >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > index 28ceb589ee40..4d1e3652dbf4 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > @@ -3693,8 +3693,11 @@ static void > dpu_encoder_frame_done_timeout(struct >> > timer_list *t) >> > static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs > = >> > { >> > .mode_set = dpu_encoder_virt_mode_set, >> > .disable = dpu_encoder_virt_disable, >> > - .enable = dpu_encoder_virt_enable, >> > + .enable = dpu_kms_encoder_enable, >> > .atomic_check = dpu_encoder_virt_atomic_check, >> > + >> > + /* This is called by dpu_kms_encoder_enable */ >> > + .commit = dpu_encoder_virt_enable, >> > }; >> > >> > static const struct drm_encoder_funcs dpu_encoder_funcs = { >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > index 81fd3a429e9f..3d83037e8305 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > @@ -425,14 +425,37 @@ static void dpu_kms_prepare_commit(struct > msm_kms >> > *kms, >> > dpu_encoder_prepare_commit(encoder); >> > } >> > >> > -static void dpu_kms_commit(struct msm_kms *kms, >> > - struct drm_atomic_state *old_state) >> > +/* >> > + * Override the encoder enable since we need to setup the inline >> > rotator >> > and do >> > + * some crtc magic before enabling any bridge that might be present. >> > + */ >> > +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; >> > + >> > + /* Forward this enable call to the commit hook */ >> > + if (funcs && funcs->commit) >> > + funcs->commit(encoder); >> >> The purpose of this function is not clear. Where are we setting up the >> inline rotator? >> Why do we need a kickoff here? > > The reason the code is shuffled is to avoid duplicating the entire > atomic > helper > function. By moving calls into the ->enable hooks, we can avoid having > to > hand > roll the helpers. > > The kickoff is preserved from the helper copy when you call > kms->funcs->commit > in between the encoder enable and bridge enable. If this can be > removed, > that'd > be even better. I was simply trying to preserve the call order of > everything. > > Sean I am with you on cleaning up the atomic helper copy. But using enc->commit helper to call into encoder_enable doesn't look valid to me. Also, I verified removing the kms->funcs->commit call between encoder enable and bridge enable. It works fine with your newly placed kms->funcs->commit after drm_atomic_helper_commit_modeset_enables. So can you revisit this function? Jeykumar S > >> > + >> > + if (crtc && crtc->state->active) { >> > + DPU_EVT32(DRMID(crtc)); >> > + dpu_crtc_commit_kickoff(crtc); >> > + } >> > +} >> > + >> > +static void dpu_kms_commit(struct msm_kms *kms, struct > drm_atomic_state >> > *state) >> > { >> > struct drm_crtc *crtc; >> > - struct drm_crtc_state *old_crtc_state; >> > + struct drm_crtc_state *crtc_state; >> > + struct dpu_crtc_state *cstate; >> > int i; >> > >> > - for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) { >> > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { >> > + /* If modeset is required, kickoff is run in >> > encoder_enable */ >> > + if (drm_atomic_crtc_needs_modeset(crtc_state)) >> > + continue; >> > + >> > if (crtc->state->active) { >> > DPU_EVT32(DRMID(crtc)); >> > dpu_crtc_commit_kickoff(crtc); >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > index 8cadd29a48b1..42c809ed9467 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > @@ -529,4 +529,6 @@ int dpu_kms_fbo_reference(struct dpu_kms_fbo > *fbo); >> > */ >> > void dpu_kms_fbo_unreference(struct dpu_kms_fbo *fbo); >> > >> > +void dpu_kms_encoder_enable(struct drm_encoder *encoder); >> > + >> > #endif /* __dpu_kms_H__ */ >> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c >> > b/drivers/gpu/drm/msm/msm_atomic.c >> > index 5cfb80345052..f5794dce25dd 100644 >> > --- a/drivers/gpu/drm/msm/msm_atomic.c >> > +++ b/drivers/gpu/drm/msm/msm_atomic.c >> > @@ -84,131 +84,6 @@ static void msm_atomic_wait_for_commit_done( >> > } >> > } >> > >> > -/** >> > - * msm_atomic_helper_commit_modeset_enables - modeset commit to > enable >> > outputs >> > - * @dev: DRM device >> > - * @old_state: atomic state object with old state structures >> > - * >> > - * This function enables all the outputs with the new configuration >> > which >> > had to >> > - * be turned off for the update. >> > - * >> > - * For compatibility with legacy crtc helpers this should be called >> > after >> > - * drm_atomic_helper_commit_planes(), which is what the default > commit >> > function >> > - * does. But drivers with different needs can group the modeset > commits >> > together >> > - * and do the plane commits at the end. This is useful for drivers >> > doing >> > runtime >> > - * PM since planes updates then only happen when the CRTC is actually >> > enabled. >> > - */ >> > -static void msm_atomic_helper_commit_modeset_enables(struct > drm_device >> > *dev, >> > - struct drm_atomic_state *old_state) >> > -{ >> > - struct drm_crtc *crtc; >> > - struct drm_crtc_state *old_crtc_state; >> > - struct drm_crtc_state *new_crtc_state; >> > - struct drm_connector *connector; >> > - struct drm_connector_state *new_conn_state; >> > - struct msm_drm_private *priv = dev->dev_private; >> > - struct msm_kms *kms = priv->kms; >> > - int bridge_enable_count = 0; >> > - int i; >> > - >> > - for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, >> > - new_crtc_state, i) { >> > - const struct drm_crtc_helper_funcs *funcs; >> > - >> > - /* Need to filter out CRTCs where only planes change. */ >> > - if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) >> > - continue; >> > - >> > - if (!new_crtc_state->active) >> > - continue; >> > - >> > - if (msm_is_mode_seamless(&new_crtc_state->mode) || >> > - msm_is_mode_seamless_vrr( >> > - &new_crtc_state->adjusted_mode)) >> > - continue; >> > - >> > - funcs = crtc->helper_private; >> > - >> > - if (crtc->state->enable) { >> > - DRM_DEBUG_ATOMIC("enabling [CRTC:%d]\n", >> > - crtc->base.id); >> > - >> > - if (funcs->atomic_enable) >> > - funcs->atomic_enable(crtc, >> > old_crtc_state); >> > - else >> > - funcs->commit(crtc); >> > - } >> > - >> > - if (msm_needs_vblank_pre_modeset( >> > - &new_crtc_state->adjusted_mode)) >> > - drm_crtc_wait_one_vblank(crtc); >> > - } >> > - >> > - /* ensure bridge/encoder updates happen on same vblank */ >> > - msm_atomic_wait_for_commit_done(dev, old_state); >> > - >> > - for_each_new_connector_in_state(old_state, connector, >> > - new_conn_state, i) { >> > - const struct drm_encoder_helper_funcs *funcs; >> > - struct drm_encoder *encoder; >> > - >> > - if (!new_conn_state->best_encoder) >> > - continue; >> > - >> > - if (!new_conn_state->crtc->state->active || >> > - !drm_atomic_crtc_needs_modeset( >> > - new_conn_state->crtc->state)) >> > - continue; >> > - >> > - encoder = new_conn_state->best_encoder; >> > - funcs = encoder->helper_private; >> > - >> > - DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n", >> > - encoder->base.id, encoder->name); >> > - >> > - /* >> > - * Each encoder has at most one connector (since we always >> > steal >> > - * it away), so we won't call enable hooks twice. >> > - */ >> > - drm_bridge_pre_enable(encoder->bridge); >> > - ++bridge_enable_count; >> > - >> > - if (funcs->enable) >> > - funcs->enable(encoder); >> > - else >> > - funcs->commit(encoder); >> > - } >> > - >> > - if (kms->funcs->commit) { >> > - DRM_DEBUG_ATOMIC("triggering commit\n"); >> > - kms->funcs->commit(kms, old_state); >> > - } >> > - >> > - /* If no bridges were pre_enabled, skip iterating over them again >> > */ >> > - if (bridge_enable_count == 0) >> > - return; >> > - >> > - for_each_new_connector_in_state(old_state, connector, >> > - new_conn_state, i) { >> > - struct drm_encoder *encoder; >> > - >> > - if (!new_conn_state->best_encoder) >> > - continue; >> > - >> > - if (!new_conn_state->crtc->state->active || >> > - !drm_atomic_crtc_needs_modeset( >> > - new_conn_state->crtc->state)) >> > - continue; >> > - >> > - encoder = new_conn_state->best_encoder; >> > - >> > - DRM_DEBUG_ATOMIC("bridge enable enabling >> > [ENCODER:%d:%s]\n", >> > - encoder->base.id, encoder->name); >> > - >> > - drm_bridge_enable(encoder->bridge); >> > - } >> > -} >> > - >> > /* The (potentially) asynchronous part of the commit. At this point >> > * nothing can fail short of armageddon. >> > */ >> > @@ -227,7 +102,12 @@ static void complete_commit(struct msm_commit *c) >> > >> > drm_atomic_helper_commit_planes(dev, state, 0); >> > >> > - msm_atomic_helper_commit_modeset_enables(dev, state); >> > + drm_atomic_helper_commit_modeset_enables(dev, state); >> > + >> > + if (kms->funcs->commit) { >> > + DRM_DEBUG_ATOMIC("triggering commit\n"); >> > + kms->funcs->commit(kms, state); >> > + } >> > >> > /* NOTE: _wait_for_vblanks() only waits for vblank on >> > * enabled CRTCs. So we end up faulting when disabling >> >> -- >> Jeykumar S
On Tue, Mar 13, 2018 at 04:57:35PM -0700, Jeykumar Sankaran wrote: > On 2018-03-12 13:21, Sean Paul wrote: > > On Thu, Mar 08, 2018 at 04:56:01PM -0800, Jeykumar Sankaran wrote: > > > On 2018-02-28 11:18, Sean Paul wrote: > > > > Instead, shuffle things around so we kickoff crtc after enabling > > encoder > > > > during modesets. Also moves the vblank wait to after the frame. > > > > > > > > Change-Id: I16c7b7f9390d04f6050aa20e17a5335fbf49eba3 > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > --- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++ > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 31 ++++- > > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 + > > > > drivers/gpu/drm/msm/msm_atomic.c | 132 > > +------------------- > > > > 5 files changed, 48 insertions(+), 131 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > index a3ab6ed2bf1d..94fab2dcca5b 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > > @@ -3525,6 +3525,12 @@ static void dpu_crtc_enable(struct drm_crtc > > > > *crtc, > > > > DPU_EVT32_VERBOSE(DRMID(crtc)); > > > > dpu_crtc = to_dpu_crtc(crtc); > > > > > > > > + if (msm_is_mode_seamless(&crtc->state->adjusted_mode) || > > > > + msm_is_mode_seamless_vrr(&crtc->state->adjusted_mode)) { > > > > + DPU_DEBUG("Skipping crtc enable, seamless mode\n"); > > > > + return; > > > > + } > > > > + > > > > pm_runtime_get_sync(crtc->dev->dev); > > > > > > > > drm_for_each_encoder(encoder, crtc->dev) { > > > > @@ -3572,6 +3578,9 @@ static void dpu_crtc_enable(struct drm_crtc > > *crtc, > > > > DPU_POWER_EVENT_POST_ENABLE | DPU_POWER_EVENT_POST_DISABLE > > > > | > > > > DPU_POWER_EVENT_PRE_DISABLE, > > > > dpu_crtc_handle_power_event, crtc, dpu_crtc->name); > > > > + > > > > + if (msm_needs_vblank_pre_modeset(&crtc->state->adjusted_mode)) > > > > + drm_crtc_wait_one_vblank(crtc); > > > > } > > > > > > > > struct plane_state { > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > index 28ceb589ee40..4d1e3652dbf4 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > > @@ -3693,8 +3693,11 @@ static void > > dpu_encoder_frame_done_timeout(struct > > > > timer_list *t) > > > > static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs > > = > > > > { > > > > .mode_set = dpu_encoder_virt_mode_set, > > > > .disable = dpu_encoder_virt_disable, > > > > - .enable = dpu_encoder_virt_enable, > > > > + .enable = dpu_kms_encoder_enable, > > > > .atomic_check = dpu_encoder_virt_atomic_check, > > > > + > > > > + /* This is called by dpu_kms_encoder_enable */ > > > > + .commit = dpu_encoder_virt_enable, > > > > }; > > > > > > > > static const struct drm_encoder_funcs dpu_encoder_funcs = { > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > > > index 81fd3a429e9f..3d83037e8305 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > > > @@ -425,14 +425,37 @@ static void dpu_kms_prepare_commit(struct > > msm_kms > > > > *kms, > > > > dpu_encoder_prepare_commit(encoder); > > > > } > > > > > > > > -static void dpu_kms_commit(struct msm_kms *kms, > > > > - struct drm_atomic_state *old_state) > > > > +/* > > > > + * Override the encoder enable since we need to setup the inline > > > > rotator > > > > and do > > > > + * some crtc magic before enabling any bridge that might be present. > > > > + */ > > > > +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; > > > > + > > > > + /* Forward this enable call to the commit hook */ > > > > + if (funcs && funcs->commit) > > > > + funcs->commit(encoder); > > > > > > The purpose of this function is not clear. Where are we setting up the > > > inline rotator? > > > Why do we need a kickoff here? > > > > The reason the code is shuffled is to avoid duplicating the entire > > atomic > > helper > > function. By moving calls into the ->enable hooks, we can avoid having > > to > > hand > > roll the helpers. > > > > The kickoff is preserved from the helper copy when you call > > kms->funcs->commit > > in between the encoder enable and bridge enable. If this can be removed, > > that'd > > be even better. I was simply trying to preserve the call order of > > everything. > > > > Sean > I am with you on cleaning up the atomic helper copy. But using enc->commit > helper > to call into encoder_enable doesn't look valid to me. "doesn't look valid to me" doesn't give me much to go on. Could you please explain why it doesn't look valid to you? > > Also, I verified removing the kms->funcs->commit call between encoder enable > and > bridge enable. It works fine with your newly placed kms->funcs->commit after > drm_atomic_helper_commit_modeset_enables. So can you revisit this function? Hmm, do you know why it was there in the first place? It seems like this was the primary reason for copy/pasting the whole function, so it probably wasn't done for nothing, it'd be nice to have some history on that. Presumably you needed to remove the needs_modeset check as well? Sean > > Jeykumar S > > > > > > + > > > > + if (crtc && crtc->state->active) { > > > > + DPU_EVT32(DRMID(crtc)); > > > > + dpu_crtc_commit_kickoff(crtc); > > > > + } > > > > +} > > > > + > > > > +static void dpu_kms_commit(struct msm_kms *kms, struct > > drm_atomic_state > > > > *state) > > > > { > > > > struct drm_crtc *crtc; > > > > - struct drm_crtc_state *old_crtc_state; > > > > + struct drm_crtc_state *crtc_state; > > > > + struct dpu_crtc_state *cstate; > > > > int i; > > > > > > > > - for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) { > > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > > > > + /* If modeset is required, kickoff is run in > > > > encoder_enable */ > > > > + if (drm_atomic_crtc_needs_modeset(crtc_state)) > > > > + continue; > > > > + > > > > if (crtc->state->active) { > > > > DPU_EVT32(DRMID(crtc)); > > > > dpu_crtc_commit_kickoff(crtc); > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > > > index 8cadd29a48b1..42c809ed9467 100644 > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > > > @@ -529,4 +529,6 @@ int dpu_kms_fbo_reference(struct dpu_kms_fbo > > *fbo); > > > > */ > > > > void dpu_kms_fbo_unreference(struct dpu_kms_fbo *fbo); > > > > > > > > +void dpu_kms_encoder_enable(struct drm_encoder *encoder); > > > > + > > > > #endif /* __dpu_kms_H__ */ > > > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c > > > > b/drivers/gpu/drm/msm/msm_atomic.c > > > > index 5cfb80345052..f5794dce25dd 100644 > > > > --- a/drivers/gpu/drm/msm/msm_atomic.c > > > > +++ b/drivers/gpu/drm/msm/msm_atomic.c > > > > @@ -84,131 +84,6 @@ static void msm_atomic_wait_for_commit_done( > > > > } > > > > } > > > > > > > > -/** > > > > - * msm_atomic_helper_commit_modeset_enables - modeset commit to > > enable > > > > outputs > > > > - * @dev: DRM device > > > > - * @old_state: atomic state object with old state structures > > > > - * > > > > - * This function enables all the outputs with the new configuration > > > > which > > > > had to > > > > - * be turned off for the update. > > > > - * > > > > - * For compatibility with legacy crtc helpers this should be called > > > > after > > > > - * drm_atomic_helper_commit_planes(), which is what the default > > commit > > > > function > > > > - * does. But drivers with different needs can group the modeset > > commits > > > > together > > > > - * and do the plane commits at the end. This is useful for drivers > > > > doing > > > > runtime > > > > - * PM since planes updates then only happen when the CRTC is actually > > > > enabled. > > > > - */ > > > > -static void msm_atomic_helper_commit_modeset_enables(struct > > drm_device > > > > *dev, > > > > - struct drm_atomic_state *old_state) > > > > -{ > > > > - struct drm_crtc *crtc; > > > > - struct drm_crtc_state *old_crtc_state; > > > > - struct drm_crtc_state *new_crtc_state; > > > > - struct drm_connector *connector; > > > > - struct drm_connector_state *new_conn_state; > > > > - struct msm_drm_private *priv = dev->dev_private; > > > > - struct msm_kms *kms = priv->kms; > > > > - int bridge_enable_count = 0; > > > > - int i; > > > > - > > > > - for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, > > > > - new_crtc_state, i) { > > > > - const struct drm_crtc_helper_funcs *funcs; > > > > - > > > > - /* Need to filter out CRTCs where only planes change. */ > > > > - if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) > > > > - continue; > > > > - > > > > - if (!new_crtc_state->active) > > > > - continue; > > > > - > > > > - if (msm_is_mode_seamless(&new_crtc_state->mode) || > > > > - msm_is_mode_seamless_vrr( > > > > - &new_crtc_state->adjusted_mode)) > > > > - continue; > > > > - > > > > - funcs = crtc->helper_private; > > > > - > > > > - if (crtc->state->enable) { > > > > - DRM_DEBUG_ATOMIC("enabling [CRTC:%d]\n", > > > > - crtc->base.id); > > > > - > > > > - if (funcs->atomic_enable) > > > > - funcs->atomic_enable(crtc, > > > > old_crtc_state); > > > > - else > > > > - funcs->commit(crtc); > > > > - } > > > > - > > > > - if (msm_needs_vblank_pre_modeset( > > > > - &new_crtc_state->adjusted_mode)) > > > > - drm_crtc_wait_one_vblank(crtc); > > > > - } > > > > - > > > > - /* ensure bridge/encoder updates happen on same vblank */ > > > > - msm_atomic_wait_for_commit_done(dev, old_state); > > > > - > > > > - for_each_new_connector_in_state(old_state, connector, > > > > - new_conn_state, i) { > > > > - const struct drm_encoder_helper_funcs *funcs; > > > > - struct drm_encoder *encoder; > > > > - > > > > - if (!new_conn_state->best_encoder) > > > > - continue; > > > > - > > > > - if (!new_conn_state->crtc->state->active || > > > > - !drm_atomic_crtc_needs_modeset( > > > > - new_conn_state->crtc->state)) > > > > - continue; > > > > - > > > > - encoder = new_conn_state->best_encoder; > > > > - funcs = encoder->helper_private; > > > > - > > > > - DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n", > > > > - encoder->base.id, encoder->name); > > > > - > > > > - /* > > > > - * Each encoder has at most one connector (since we always > > > > steal > > > > - * it away), so we won't call enable hooks twice. > > > > - */ > > > > - drm_bridge_pre_enable(encoder->bridge); > > > > - ++bridge_enable_count; > > > > - > > > > - if (funcs->enable) > > > > - funcs->enable(encoder); > > > > - else > > > > - funcs->commit(encoder); > > > > - } > > > > - > > > > - if (kms->funcs->commit) { > > > > - DRM_DEBUG_ATOMIC("triggering commit\n"); > > > > - kms->funcs->commit(kms, old_state); > > > > - } > > > > - > > > > - /* If no bridges were pre_enabled, skip iterating over them again > > > > */ > > > > - if (bridge_enable_count == 0) > > > > - return; > > > > - > > > > - for_each_new_connector_in_state(old_state, connector, > > > > - new_conn_state, i) { > > > > - struct drm_encoder *encoder; > > > > - > > > > - if (!new_conn_state->best_encoder) > > > > - continue; > > > > - > > > > - if (!new_conn_state->crtc->state->active || > > > > - !drm_atomic_crtc_needs_modeset( > > > > - new_conn_state->crtc->state)) > > > > - continue; > > > > - > > > > - encoder = new_conn_state->best_encoder; > > > > - > > > > - DRM_DEBUG_ATOMIC("bridge enable enabling > > > > [ENCODER:%d:%s]\n", > > > > - encoder->base.id, encoder->name); > > > > - > > > > - drm_bridge_enable(encoder->bridge); > > > > - } > > > > -} > > > > - > > > > /* The (potentially) asynchronous part of the commit. At this point > > > > * nothing can fail short of armageddon. > > > > */ > > > > @@ -227,7 +102,12 @@ static void complete_commit(struct msm_commit *c) > > > > > > > > drm_atomic_helper_commit_planes(dev, state, 0); > > > > > > > > - msm_atomic_helper_commit_modeset_enables(dev, state); > > > > + drm_atomic_helper_commit_modeset_enables(dev, state); > > > > + > > > > + if (kms->funcs->commit) { > > > > + DRM_DEBUG_ATOMIC("triggering commit\n"); > > > > + kms->funcs->commit(kms, state); > > > > + } > > > > > > > > /* NOTE: _wait_for_vblanks() only waits for vblank on > > > > * enabled CRTCs. So we end up faulting when disabling > > > > > > -- > > > Jeykumar S > > -- > Jeykumar S
On 2018-03-14 08:14, Sean Paul wrote: > On Tue, Mar 13, 2018 at 04:57:35PM -0700, Jeykumar Sankaran wrote: >> On 2018-03-12 13:21, Sean Paul wrote: >> > On Thu, Mar 08, 2018 at 04:56:01PM -0800, Jeykumar Sankaran wrote: >> > > On 2018-02-28 11:18, Sean Paul wrote: >> > > > Instead, shuffle things around so we kickoff crtc after enabling >> > encoder >> > > > during modesets. Also moves the vblank wait to after the frame. >> > > > >> > > > Change-Id: I16c7b7f9390d04f6050aa20e17a5335fbf49eba3 >> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> >> > > > --- >> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++ >> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +- >> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 31 ++++- >> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 + >> > > > drivers/gpu/drm/msm/msm_atomic.c | 132 >> > +------------------- >> > > > 5 files changed, 48 insertions(+), 131 deletions(-) >> > > > >> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > > > index a3ab6ed2bf1d..94fab2dcca5b 100644 >> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> > > > @@ -3525,6 +3525,12 @@ static void dpu_crtc_enable(struct drm_crtc >> > > > *crtc, >> > > > DPU_EVT32_VERBOSE(DRMID(crtc)); >> > > > dpu_crtc = to_dpu_crtc(crtc); >> > > > >> > > > + if (msm_is_mode_seamless(&crtc->state->adjusted_mode) || >> > > > + msm_is_mode_seamless_vrr(&crtc->state->adjusted_mode)) > { >> > > > + DPU_DEBUG("Skipping crtc enable, seamless > mode\n"); >> > > > + return; >> > > > + } >> > > > + >> > > > pm_runtime_get_sync(crtc->dev->dev); >> > > > >> > > > drm_for_each_encoder(encoder, crtc->dev) { >> > > > @@ -3572,6 +3578,9 @@ static void dpu_crtc_enable(struct drm_crtc >> > *crtc, >> > > > DPU_POWER_EVENT_POST_ENABLE | > DPU_POWER_EVENT_POST_DISABLE >> > > > | >> > > > DPU_POWER_EVENT_PRE_DISABLE, >> > > > dpu_crtc_handle_power_event, crtc, > dpu_crtc->name); >> > > > + >> > > > + if > (msm_needs_vblank_pre_modeset(&crtc->state->adjusted_mode)) >> > > > + drm_crtc_wait_one_vblank(crtc); >> > > > } >> > > > >> > > > struct plane_state { >> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > > > index 28ceb589ee40..4d1e3652dbf4 100644 >> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> > > > @@ -3693,8 +3693,11 @@ static void >> > dpu_encoder_frame_done_timeout(struct >> > > > timer_list *t) >> > > > static const struct drm_encoder_helper_funcs > dpu_encoder_helper_funcs >> > = >> > > > { >> > > > .mode_set = dpu_encoder_virt_mode_set, >> > > > .disable = dpu_encoder_virt_disable, >> > > > - .enable = dpu_encoder_virt_enable, >> > > > + .enable = dpu_kms_encoder_enable, >> > > > .atomic_check = dpu_encoder_virt_atomic_check, >> > > > + >> > > > + /* This is called by dpu_kms_encoder_enable */ >> > > > + .commit = dpu_encoder_virt_enable, >> > > > }; >> > > > >> > > > static const struct drm_encoder_funcs dpu_encoder_funcs = { >> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > > > index 81fd3a429e9f..3d83037e8305 100644 >> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> > > > @@ -425,14 +425,37 @@ static void dpu_kms_prepare_commit(struct >> > msm_kms >> > > > *kms, >> > > > dpu_encoder_prepare_commit(encoder); >> > > > } >> > > > >> > > > -static void dpu_kms_commit(struct msm_kms *kms, >> > > > - struct drm_atomic_state *old_state) >> > > > +/* >> > > > + * Override the encoder enable since we need to setup the inline >> > > > rotator >> > > > and do >> > > > + * some crtc magic before enabling any bridge that might be > present. >> > > > + */ >> > > > +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; >> > > > + >> > > > + /* Forward this enable call to the commit hook */ >> > > > + if (funcs && funcs->commit) >> > > > + funcs->commit(encoder); >> > > >> > > The purpose of this function is not clear. Where are we setting up > the >> > > inline rotator? >> > > Why do we need a kickoff here? >> > >> > The reason the code is shuffled is to avoid duplicating the entire >> > atomic >> > helper >> > function. By moving calls into the ->enable hooks, we can avoid having >> > to >> > hand >> > roll the helpers. >> > >> > The kickoff is preserved from the helper copy when you call >> > kms->funcs->commit >> > in between the encoder enable and bridge enable. If this can be > removed, >> > that'd >> > be even better. I was simply trying to preserve the call order of >> > everything. >> > >> > Sean >> I am with you on cleaning up the atomic helper copy. But using > enc->commit >> helper >> to call into encoder_enable doesn't look valid to me. > > "doesn't look valid to me" doesn't give me much to go on. Could you > please > explain why it doesn't look valid to you? > The description in drm_modeset_helper_vtables.h says using enc->commit hook for enabling the encoder is a deprecated method. Also, since we have an option to get rid of this extra kms->funcs->commit (for which we needed this loop around in first place), we can stick with the enc->enable hook itself. >> >> Also, I verified removing the kms->funcs->commit call between encoder > enable >> and >> bridge enable. It works fine with your newly placed kms->funcs->commit > after >> drm_atomic_helper_commit_modeset_enables. So can you revisit this > function? > > Hmm, do you know why it was there in the first place? It seems like > this > was the > primary reason for copy/pasting the whole function, so it probably > wasn't > done > for nothing, it'd be nice to have some history on that. > h/w programming sequence differs between DPU and MDP5. MDP5 triggers the frame kickoff in crtc->flush hook. But actual frame transfer starts only in encoder->enable where we turn on the interface timing engine and trigger the CTL flush again(!). In DPU, we enable all the hw blocks associated with the encoder before triggering the frame kickoff. So a new hook, kms->commit was introduced to kickoff the frame after all the components are enabled. Since you are moving the kms->funcs->commit after modeset_enables, we are already taking care of the order. As an alternative, I see drm_atomic_helper_commit_tail_rpm changing the order of crtc->flush / enc->enable helper calls for drivers using runtime_pm. If we can take care of triggering frames from crtc->flush (as MDP5 does), we can even get rid of kms->funcs->commit and adapt more helpers. But that's for the future patches! > Presumably you needed to remove the needs_modeset check as well? > Yes. > Sean > >> >> Jeykumar S >> > >> > > > + >> > > > + if (crtc && crtc->state->active) { >> > > > + DPU_EVT32(DRMID(crtc)); >> > > > + dpu_crtc_commit_kickoff(crtc); >> > > > + } >> > > > +} >> > > > + >> > > > +static void dpu_kms_commit(struct msm_kms *kms, struct >> > drm_atomic_state >> > > > *state) >> > > > { >> > > > struct drm_crtc *crtc; >> > > > - struct drm_crtc_state *old_crtc_state; >> > > > + struct drm_crtc_state *crtc_state; >> > > > + struct dpu_crtc_state *cstate; >> > > > int i; >> > > > >> > > > - for_each_old_crtc_in_state(old_state, crtc, > old_crtc_state, i) { >> > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { >> > > > + /* If modeset is required, kickoff is run in >> > > > encoder_enable */ >> > > > + if (drm_atomic_crtc_needs_modeset(crtc_state)) >> > > > + continue; >> > > > + >> > > > if (crtc->state->active) { >> > > > DPU_EVT32(DRMID(crtc)); >> > > > dpu_crtc_commit_kickoff(crtc); >> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > > > index 8cadd29a48b1..42c809ed9467 100644 >> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >> > > > @@ -529,4 +529,6 @@ int dpu_kms_fbo_reference(struct dpu_kms_fbo >> > *fbo); >> > > > */ >> > > > void dpu_kms_fbo_unreference(struct dpu_kms_fbo *fbo); >> > > > >> > > > +void dpu_kms_encoder_enable(struct drm_encoder *encoder); >> > > > + >> > > > #endif /* __dpu_kms_H__ */ >> > > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c >> > > > b/drivers/gpu/drm/msm/msm_atomic.c >> > > > index 5cfb80345052..f5794dce25dd 100644 >> > > > --- a/drivers/gpu/drm/msm/msm_atomic.c >> > > > +++ b/drivers/gpu/drm/msm/msm_atomic.c >> > > > @@ -84,131 +84,6 @@ static void msm_atomic_wait_for_commit_done( >> > > > } >> > > > } >> > > > >> > > > -/** >> > > > - * msm_atomic_helper_commit_modeset_enables - modeset commit to >> > enable >> > > > outputs >> > > > - * @dev: DRM device >> > > > - * @old_state: atomic state object with old state structures >> > > > - * >> > > > - * This function enables all the outputs with the new > configuration >> > > > which >> > > > had to >> > > > - * be turned off for the update. >> > > > - * >> > > > - * For compatibility with legacy crtc helpers this should be > called >> > > > after >> > > > - * drm_atomic_helper_commit_planes(), which is what the default >> > commit >> > > > function >> > > > - * does. But drivers with different needs can group the modeset >> > commits >> > > > together >> > > > - * and do the plane commits at the end. This is useful for > drivers >> > > > doing >> > > > runtime >> > > > - * PM since planes updates then only happen when the CRTC is > actually >> > > > enabled. >> > > > - */ >> > > > -static void msm_atomic_helper_commit_modeset_enables(struct >> > drm_device >> > > > *dev, >> > > > - struct drm_atomic_state *old_state) >> > > > -{ >> > > > - struct drm_crtc *crtc; >> > > > - struct drm_crtc_state *old_crtc_state; >> > > > - struct drm_crtc_state *new_crtc_state; >> > > > - struct drm_connector *connector; >> > > > - struct drm_connector_state *new_conn_state; >> > > > - struct msm_drm_private *priv = dev->dev_private; >> > > > - struct msm_kms *kms = priv->kms; >> > > > - int bridge_enable_count = 0; >> > > > - int i; >> > > > - >> > > > - for_each_oldnew_crtc_in_state(old_state, crtc, > old_crtc_state, >> > > > - new_crtc_state, i) { >> > > > - const struct drm_crtc_helper_funcs *funcs; >> > > > - >> > > > - /* Need to filter out CRTCs where only planes > change. */ >> > > > - if > (!drm_atomic_crtc_needs_modeset(new_crtc_state)) >> > > > - continue; >> > > > - >> > > > - if (!new_crtc_state->active) >> > > > - continue; >> > > > - >> > > > - if (msm_is_mode_seamless(&new_crtc_state->mode) || >> > > > - msm_is_mode_seamless_vrr( >> > > > - &new_crtc_state->adjusted_mode)) >> > > > - continue; >> > > > - >> > > > - funcs = crtc->helper_private; >> > > > - >> > > > - if (crtc->state->enable) { >> > > > - DRM_DEBUG_ATOMIC("enabling [CRTC:%d]\n", >> > > > - crtc->base.id); >> > > > - >> > > > - if (funcs->atomic_enable) >> > > > - funcs->atomic_enable(crtc, >> > > > old_crtc_state); >> > > > - else >> > > > - funcs->commit(crtc); >> > > > - } >> > > > - >> > > > - if (msm_needs_vblank_pre_modeset( >> > > > - > &new_crtc_state->adjusted_mode)) >> > > > - drm_crtc_wait_one_vblank(crtc); >> > > > - } >> > > > - >> > > > - /* ensure bridge/encoder updates happen on same vblank */ >> > > > - msm_atomic_wait_for_commit_done(dev, old_state); >> > > > - >> > > > - for_each_new_connector_in_state(old_state, connector, >> > > > - new_conn_state, i) { >> > > > - const struct drm_encoder_helper_funcs *funcs; >> > > > - struct drm_encoder *encoder; >> > > > - >> > > > - if (!new_conn_state->best_encoder) >> > > > - continue; >> > > > - >> > > > - if (!new_conn_state->crtc->state->active || >> > > > - !drm_atomic_crtc_needs_modeset( >> > > > - new_conn_state->crtc->state)) >> > > > - continue; >> > > > - >> > > > - encoder = new_conn_state->best_encoder; >> > > > - funcs = encoder->helper_private; >> > > > - >> > > > - DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n", >> > > > - encoder->base.id, encoder->name); >> > > > - >> > > > - /* >> > > > - * Each encoder has at most one connector (since > we always >> > > > steal >> > > > - * it away), so we won't call enable hooks twice. >> > > > - */ >> > > > - drm_bridge_pre_enable(encoder->bridge); >> > > > - ++bridge_enable_count; >> > > > - >> > > > - if (funcs->enable) >> > > > - funcs->enable(encoder); >> > > > - else >> > > > - funcs->commit(encoder); >> > > > - } >> > > > - >> > > > - if (kms->funcs->commit) { >> > > > - DRM_DEBUG_ATOMIC("triggering commit\n"); >> > > > - kms->funcs->commit(kms, old_state); >> > > > - } >> > > > - >> > > > - /* If no bridges were pre_enabled, skip iterating over > them again >> > > > */ >> > > > - if (bridge_enable_count == 0) >> > > > - return; >> > > > - >> > > > - for_each_new_connector_in_state(old_state, connector, >> > > > - new_conn_state, i) { >> > > > - struct drm_encoder *encoder; >> > > > - >> > > > - if (!new_conn_state->best_encoder) >> > > > - continue; >> > > > - >> > > > - if (!new_conn_state->crtc->state->active || >> > > > - !drm_atomic_crtc_needs_modeset( >> > > > - new_conn_state->crtc->state)) >> > > > - continue; >> > > > - >> > > > - encoder = new_conn_state->best_encoder; >> > > > - >> > > > - DRM_DEBUG_ATOMIC("bridge enable enabling >> > > > [ENCODER:%d:%s]\n", >> > > > - encoder->base.id, encoder->name); >> > > > - >> > > > - drm_bridge_enable(encoder->bridge); >> > > > - } >> > > > -} >> > > > - >> > > > /* The (potentially) asynchronous part of the commit. At this > point >> > > > * nothing can fail short of armageddon. >> > > > */ >> > > > @@ -227,7 +102,12 @@ static void complete_commit(struct msm_commit > *c) >> > > > >> > > > drm_atomic_helper_commit_planes(dev, state, 0); >> > > > >> > > > - msm_atomic_helper_commit_modeset_enables(dev, state); >> > > > + drm_atomic_helper_commit_modeset_enables(dev, state); >> > > > + >> > > > + if (kms->funcs->commit) { >> > > > + DRM_DEBUG_ATOMIC("triggering commit\n"); >> > > > + kms->funcs->commit(kms, state); >> > > > + } >> > > > >> > > > /* NOTE: _wait_for_vblanks() only waits for vblank on >> > > > * enabled CRTCs. So we end up faulting when disabling >> > > >> > > -- >> > > Jeykumar S >> >> -- >> Jeykumar S
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index a3ab6ed2bf1d..94fab2dcca5b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -3525,6 +3525,12 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, DPU_EVT32_VERBOSE(DRMID(crtc)); dpu_crtc = to_dpu_crtc(crtc); + if (msm_is_mode_seamless(&crtc->state->adjusted_mode) || + msm_is_mode_seamless_vrr(&crtc->state->adjusted_mode)) { + DPU_DEBUG("Skipping crtc enable, seamless mode\n"); + return; + } + pm_runtime_get_sync(crtc->dev->dev); drm_for_each_encoder(encoder, crtc->dev) { @@ -3572,6 +3578,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, DPU_POWER_EVENT_POST_ENABLE | DPU_POWER_EVENT_POST_DISABLE | DPU_POWER_EVENT_PRE_DISABLE, dpu_crtc_handle_power_event, crtc, dpu_crtc->name); + + if (msm_needs_vblank_pre_modeset(&crtc->state->adjusted_mode)) + drm_crtc_wait_one_vblank(crtc); } struct plane_state { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 28ceb589ee40..4d1e3652dbf4 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -3693,8 +3693,11 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = { .mode_set = dpu_encoder_virt_mode_set, .disable = dpu_encoder_virt_disable, - .enable = dpu_encoder_virt_enable, + .enable = dpu_kms_encoder_enable, .atomic_check = dpu_encoder_virt_atomic_check, + + /* This is called by dpu_kms_encoder_enable */ + .commit = dpu_encoder_virt_enable, }; static const struct drm_encoder_funcs dpu_encoder_funcs = { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 81fd3a429e9f..3d83037e8305 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -425,14 +425,37 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms, dpu_encoder_prepare_commit(encoder); } -static void dpu_kms_commit(struct msm_kms *kms, - struct drm_atomic_state *old_state) +/* + * Override the encoder enable since we need to setup the inline rotator and do + * some crtc magic before enabling any bridge that might be present. + */ +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; + + /* Forward this enable call to the commit hook */ + if (funcs && funcs->commit) + funcs->commit(encoder); + + if (crtc && crtc->state->active) { + DPU_EVT32(DRMID(crtc)); + dpu_crtc_commit_kickoff(crtc); + } +} + +static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state) { struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *crtc_state; + struct dpu_crtc_state *cstate; int i; - for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { + /* If modeset is required, kickoff is run in encoder_enable */ + if (drm_atomic_crtc_needs_modeset(crtc_state)) + continue; + if (crtc->state->active) { DPU_EVT32(DRMID(crtc)); dpu_crtc_commit_kickoff(crtc); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index 8cadd29a48b1..42c809ed9467 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -529,4 +529,6 @@ int dpu_kms_fbo_reference(struct dpu_kms_fbo *fbo); */ void dpu_kms_fbo_unreference(struct dpu_kms_fbo *fbo); +void dpu_kms_encoder_enable(struct drm_encoder *encoder); + #endif /* __dpu_kms_H__ */ diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 5cfb80345052..f5794dce25dd 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -84,131 +84,6 @@ static void msm_atomic_wait_for_commit_done( } } -/** - * msm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs - * @dev: DRM device - * @old_state: atomic state object with old state structures - * - * This function enables all the outputs with the new configuration which had to - * be turned off for the update. - * - * For compatibility with legacy crtc helpers this should be called after - * drm_atomic_helper_commit_planes(), which is what the default commit function - * does. But drivers with different needs can group the modeset commits together - * and do the plane commits at the end. This is useful for drivers doing runtime - * PM since planes updates then only happen when the CRTC is actually enabled. - */ -static void msm_atomic_helper_commit_modeset_enables(struct drm_device *dev, - struct drm_atomic_state *old_state) -{ - struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; - struct drm_crtc_state *new_crtc_state; - struct drm_connector *connector; - struct drm_connector_state *new_conn_state; - struct msm_drm_private *priv = dev->dev_private; - struct msm_kms *kms = priv->kms; - int bridge_enable_count = 0; - int i; - - for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, - new_crtc_state, i) { - const struct drm_crtc_helper_funcs *funcs; - - /* Need to filter out CRTCs where only planes change. */ - if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) - continue; - - if (!new_crtc_state->active) - continue; - - if (msm_is_mode_seamless(&new_crtc_state->mode) || - msm_is_mode_seamless_vrr( - &new_crtc_state->adjusted_mode)) - continue; - - funcs = crtc->helper_private; - - if (crtc->state->enable) { - DRM_DEBUG_ATOMIC("enabling [CRTC:%d]\n", - crtc->base.id); - - if (funcs->atomic_enable) - funcs->atomic_enable(crtc, old_crtc_state); - else - funcs->commit(crtc); - } - - if (msm_needs_vblank_pre_modeset( - &new_crtc_state->adjusted_mode)) - drm_crtc_wait_one_vblank(crtc); - } - - /* ensure bridge/encoder updates happen on same vblank */ - msm_atomic_wait_for_commit_done(dev, old_state); - - for_each_new_connector_in_state(old_state, connector, - new_conn_state, i) { - const struct drm_encoder_helper_funcs *funcs; - struct drm_encoder *encoder; - - if (!new_conn_state->best_encoder) - continue; - - if (!new_conn_state->crtc->state->active || - !drm_atomic_crtc_needs_modeset( - new_conn_state->crtc->state)) - continue; - - encoder = new_conn_state->best_encoder; - funcs = encoder->helper_private; - - DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n", - encoder->base.id, encoder->name); - - /* - * Each encoder has at most one connector (since we always steal - * it away), so we won't call enable hooks twice. - */ - drm_bridge_pre_enable(encoder->bridge); - ++bridge_enable_count; - - if (funcs->enable) - funcs->enable(encoder); - else - funcs->commit(encoder); - } - - if (kms->funcs->commit) { - DRM_DEBUG_ATOMIC("triggering commit\n"); - kms->funcs->commit(kms, old_state); - } - - /* If no bridges were pre_enabled, skip iterating over them again */ - if (bridge_enable_count == 0) - return; - - for_each_new_connector_in_state(old_state, connector, - new_conn_state, i) { - struct drm_encoder *encoder; - - if (!new_conn_state->best_encoder) - continue; - - if (!new_conn_state->crtc->state->active || - !drm_atomic_crtc_needs_modeset( - new_conn_state->crtc->state)) - continue; - - encoder = new_conn_state->best_encoder; - - DRM_DEBUG_ATOMIC("bridge enable enabling [ENCODER:%d:%s]\n", - encoder->base.id, encoder->name); - - drm_bridge_enable(encoder->bridge); - } -} - /* The (potentially) asynchronous part of the commit. At this point * nothing can fail short of armageddon. */ @@ -227,7 +102,12 @@ static void complete_commit(struct msm_commit *c) drm_atomic_helper_commit_planes(dev, state, 0); - msm_atomic_helper_commit_modeset_enables(dev, state); + drm_atomic_helper_commit_modeset_enables(dev, state); + + if (kms->funcs->commit) { + DRM_DEBUG_ATOMIC("triggering commit\n"); + kms->funcs->commit(kms, state); + } /* NOTE: _wait_for_vblanks() only waits for vblank on * enabled CRTCs. So we end up faulting when disabling
Instead, shuffle things around so we kickoff crtc after enabling encoder during modesets. Also moves the vblank wait to after the frame. Change-Id: I16c7b7f9390d04f6050aa20e17a5335fbf49eba3 Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 31 ++++- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 + drivers/gpu/drm/msm/msm_atomic.c | 132 +------------------- 5 files changed, 48 insertions(+), 131 deletions(-)