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

> > +
> > +   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

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Reply via email to