[DPU PATCH 02/11] drm/msm: Don't duplicate modeset_enables atomic helper
Jeykumar Sankaran
jsanka at codeaurora.org
Tue Mar 13 23:57:35 UTC 2018
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 <seanpaul at 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.
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?
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
More information about the dri-devel
mailing list