On Fri, Mar 02, 2018 at 04:04:24PM -0800, jsa...@codeaurora.org wrote:
> On 2018-02-28 11:18, Sean Paul wrote:
> > Instead of duplicating whole swaths of atomic helper functions (which
> > are already out-of-date), just skip the encoder/crtc disables in the
> > .disable hooks.
> > 
> > Change-Id: I7bd9183ae60624204fb1de9550656b776efc7202
> > Signed-off-by: Sean Paul <seanp...@chromium.org>
> 
> Can you consider getting rid of these checks?

Do you mean the Change-Id? Yeah, I forgot to strip them out before sending, I'll
make sure I clean it up before I apply.

> 
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |   8 +
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |   8 +
> >  drivers/gpu/drm/msm/msm_atomic.c            | 185 +-------------------
> >  3 files changed, 17 insertions(+), 184 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 3cdf1e3d9d96..a3ab6ed2bf1d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -3393,6 +3393,7 @@ static void dpu_crtc_disable(struct drm_crtc
> > *crtc)
> >  {
> >     struct dpu_crtc *dpu_crtc;
> >     struct dpu_crtc_state *cstate;
> > +   struct drm_display_mode *mode;
> >     struct drm_encoder *encoder;
> >     struct msm_drm_private *priv;
> >     unsigned long flags;
> > @@ -3407,8 +3408,15 @@ static void dpu_crtc_disable(struct drm_crtc
> > *crtc)
> >     }
> >     dpu_crtc = to_dpu_crtc(crtc);
> >     cstate = to_dpu_crtc_state(crtc->state);
> > +   mode = &cstate->base.adjusted_mode;
> >     priv = crtc->dev->dev_private;
> > 
> > +   if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> > ||
> > +       msm_is_mode_seamless_dms(mode)) {
> > +           DPU_DEBUG("Seamless mode is being applied, skip
> > disable\n");
> > +           return;
> > +   }
> > +
> Another topic of discussion which should be brought up with dri-devel.
> 
> May not be common in PC world, but there are a handful of mobile OEM's
> using panels which supports more than one resolution. Primary use cases
> involve "seamless" switching to optimized display resolution when
> streaming content changes resolutions or rendering lossless data.

Yeah, I think we can do this under the covers if the hardware supports it such
as this patch. We could probably do a better job of making this useful for other
drivers, but I was really just trying to get the seamless stuff out of the way
so we don't need to roll our own atomic commit.

Sean

> 
> Jeykumar S.
> 
> >     DPU_DEBUG("crtc%d\n", crtc->base.id);
> > 
> >     if (dpu_kms_is_suspend_state(crtc->dev))
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 3d168fa09f3f..28ceb589ee40 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -2183,6 +2183,7 @@ static void dpu_encoder_virt_disable(struct
> > drm_encoder *drm_enc)
> >     struct dpu_encoder_virt *dpu_enc = NULL;
> >     struct msm_drm_private *priv;
> >     struct dpu_kms *dpu_kms;
> > +   struct drm_display_mode *mode;
> >     int i = 0;
> > 
> >     if (!drm_enc) {
> > @@ -2196,6 +2197,13 @@ static void dpu_encoder_virt_disable(struct
> > drm_encoder *drm_enc)
> >             return;
> >     }
> > 
> > +   mode = &drm_enc->crtc->state->adjusted_mode;
> > +   if (msm_is_mode_seamless(mode) || msm_is_mode_seamless_vrr(mode)
> > ||
> > +       msm_is_mode_seamless_dms(mode)) {
> > +           DPU_DEBUG("Seamless mode is being applied, skip
> > disable\n");
> > +           return;
> > +   }
> > +
> >     dpu_enc = to_dpu_encoder_virt(drm_enc);
> >     DPU_DEBUG_ENC(dpu_enc, "\n");
> > 
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index 46536edb72ee..5cfb80345052 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -84,189 +84,6 @@ static void msm_atomic_wait_for_commit_done(
> >     }
> >  }
> > 
> > -static void
> > -msm_disable_outputs(struct drm_device *dev, struct drm_atomic_state
> > *old_state)
> > -{
> > -   struct drm_connector *connector;
> > -   struct drm_connector_state *old_conn_state, *new_conn_state;
> > -   struct drm_crtc *crtc;
> > -   struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> > -   int i;
> > -
> > -   for_each_oldnew_connector_in_state(old_state, connector,
> > old_conn_state, new_conn_state, i) {
> > -           const struct drm_encoder_helper_funcs *funcs;
> > -           struct drm_encoder *encoder;
> > -           struct drm_crtc_state *old_crtc_state;
> > -           unsigned int crtc_idx;
> > -
> > -           /*
> > -            * Shut down everything that's in the changeset and
> > currently
> > -            * still on. So need to check the old, saved state.
> > -            */
> > -           if (!old_conn_state->crtc)
> > -                   continue;
> > -
> > -           crtc_idx = drm_crtc_index(old_conn_state->crtc);
> > -           old_crtc_state = drm_atomic_get_old_crtc_state(old_state,
> > -
> > old_conn_state->crtc);
> > -
> > -           if (!old_crtc_state->active ||
> > -
> > !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> > -                   continue;
> > -
> > -           encoder = old_conn_state->best_encoder;
> > -
> > -           /* We shouldn't get this far if we didn't previously have
> > -            * an encoder.. but WARN_ON() rather than explode.
> > -            */
> > -           if (WARN_ON(!encoder))
> > -                   continue;
> > -
> > -           if (msm_is_mode_seamless(
> > -                   &connector->encoder->crtc->state->mode) ||
> > -                   msm_is_mode_seamless_vrr(
> > -                   &connector->encoder->crtc->state->adjusted_mode))
> > -                   continue;
> > -
> > -           if (msm_is_mode_seamless_dms(
> > -                   &connector->encoder->crtc->state->adjusted_mode))
> > -                   continue;
> > -
> > -           funcs = encoder->helper_private;
> > -
> > -           DRM_DEBUG_ATOMIC("disabling [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 disable hooks twice.
> > -            */
> > -           drm_bridge_disable(encoder->bridge);
> > -
> > -           /* Right function depends upon target state. */
> > -           if (new_conn_state->crtc && funcs->prepare)
> > -                   funcs->prepare(encoder);
> > -           else if (funcs->disable)
> > -                   funcs->disable(encoder);
> > -           else
> > -                   funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > -
> > -           drm_bridge_post_disable(encoder->bridge);
> > -   }
> > -
> > -   for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> > new_crtc_state, i) {
> > -           const struct drm_crtc_helper_funcs *funcs;
> > -
> > -           /* Shut down everything that needs a full modeset. */
> > -           if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
> > -                   continue;
> > -
> > -           if (!old_crtc_state->active)
> > -                   continue;
> > -
> > -           if (msm_is_mode_seamless(&crtc->state->mode) ||
> > -
> > msm_is_mode_seamless_vrr(&crtc->state->adjusted_mode))
> > -                   continue;
> > -
> > -           if (msm_is_mode_seamless_dms(&crtc->state->adjusted_mode))
> > -                   continue;
> > -
> > -           funcs = crtc->helper_private;
> > -
> > -           DRM_DEBUG_ATOMIC("disabling [CRTC:%d]\n",
> > -                            crtc->base.id);
> > -
> > -           /* Right function depends upon target state. */
> > -           if (new_crtc_state->enable && funcs->prepare)
> > -                   funcs->prepare(crtc);
> > -           else if (funcs->disable)
> > -                   funcs->disable(crtc);
> > -           else
> > -                   funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> > -   }
> > -}
> > -
> > -static void
> > -msm_crtc_set_mode(struct drm_device *dev, struct drm_atomic_state
> > *old_state)
> > -{
> > -   struct drm_crtc *crtc;
> > -   struct drm_crtc_state *new_crtc_state;
> > -   struct drm_connector *connector;
> > -   struct drm_connector_state *new_conn_state;
> > -   int i;
> > -
> > -   for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
> > -           const struct drm_crtc_helper_funcs *funcs;
> > -
> > -           if (!new_crtc_state->mode_changed)
> > -                   continue;
> > -
> > -           funcs = crtc->helper_private;
> > -
> > -           if (new_crtc_state->enable && funcs->mode_set_nofb) {
> > -                   DRM_DEBUG_ATOMIC("modeset on [CRTC:%d]\n",
> > -                                    crtc->base.id);
> > -
> > -                   funcs->mode_set_nofb(crtc);
> > -           }
> > -   }
> > -
> > -   for_each_new_connector_in_state(old_state, connector,
> > new_conn_state, i) {
> > -           const struct drm_encoder_helper_funcs *funcs;
> > -           struct drm_crtc_state *new_crtc_state;
> > -           struct drm_encoder *encoder;
> > -           struct drm_display_mode *mode, *adjusted_mode;
> > -
> > -           if (!new_conn_state->best_encoder)
> > -                   continue;
> > -
> > -           encoder = new_conn_state->best_encoder;
> > -           funcs = encoder->helper_private;
> > -           new_crtc_state = new_conn_state->crtc->state;
> > -           mode = &new_crtc_state->mode;
> > -           adjusted_mode = &new_crtc_state->adjusted_mode;
> > -
> > -           if (!new_crtc_state->mode_changed)
> > -                   continue;
> > -
> > -           DRM_DEBUG_ATOMIC("modeset on [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 mode_set hooks twice.
> > -            */
> > -           if (funcs->mode_set)
> > -                   funcs->mode_set(encoder, mode, adjusted_mode);
> > -
> > -           drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> > -   }
> > -}
> > -
> > -/**
> > - * msm_atomic_helper_commit_modeset_disables - modeset commit to
> > disable
> > outputs
> > - * @dev: DRM device
> > - * @old_state: atomic state object with old state structures
> > - *
> > - * This function shuts down all the outputs that need to be shut down
> > and
> > - * prepares them (if required) with the new mode.
> > - *
> > - * For compatibility with legacy crtc helpers this should be called
> > before
> > - * 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.
> > - */
> > -void msm_atomic_helper_commit_modeset_disables(struct drm_device *dev,
> > -           struct drm_atomic_state *old_state)
> > -{
> > -   msm_disable_outputs(dev, old_state);
> > -
> > -   drm_atomic_helper_update_legacy_modeset_state(dev, old_state);
> > -
> > -   msm_crtc_set_mode(dev, old_state);
> > -}
> > -
> >  /**
> >   * msm_atomic_helper_commit_modeset_enables - modeset commit to enable
> > outputs
> >   * @dev: DRM device
> > @@ -406,7 +223,7 @@ static void complete_commit(struct msm_commit *c)
> > 
> >     kms->funcs->prepare_commit(kms, state);
> > 
> > -   msm_atomic_helper_commit_modeset_disables(dev, state);
> > +   drm_atomic_helper_commit_modeset_disables(dev, state);
> > 
> >     drm_atomic_helper_commit_planes(dev, state, 0);

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