[DPU PATCH 01/11] drm/msm: Skip seamless disables in crtc/encoder
Jeykumar Sankaran
jsanka at codeaurora.org
Tue Mar 13 18:10:11 UTC 2018
On 2018-03-12 13:14, Sean Paul wrote:
> On Fri, Mar 02, 2018 at 04:04:24PM -0800, jsanka at 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 <seanpaul at chromium.org>
Reviewed-by: Jeykumar Sankaran <jsanka at codeaurora.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.
>
Actually, I meant removing the seamless check flags that you are moving
to
encode/crtc. But you can ignore that, I am planning to submit a seperate
change to remove the support from the whole pipeline.
>>
>> > ---
>> > 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);
--
Jeykumar S
More information about the dri-devel
mailing list