| Message ID | 20180228191906.185417-2-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chromium.org> Can you consider getting rid of these checks? > --- > 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. 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);
On Fri, Mar 02, 2018 at 04:04:24PM -0800, jsanka@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@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);
On 2018-03-12 13:14, Sean Paul wrote: > On Fri, Mar 02, 2018 at 04:04:24PM -0800, jsanka@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@chromium.org> Reviewed-by: Jeykumar Sankaran <jsanka@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);
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; + } + 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);
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@chromium.org> --- 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(-)