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 <seanp...@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 -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno