[DPU PATCH 02/11] drm/msm: Don't duplicate modeset_enables atomic helper

[Jeykumar Sankaran]

On 2018-03-14 08:14, Sean Paul wrote:
> On Tue, Mar 13, 2018 at 04:57:35PM -0700, Jeykumar Sankaran wrote:
>> 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.
> 
> "doesn't look valid to me" doesn't give me much to go on. Could you 
> please
> explain why it doesn't look valid to you?
> 
The description in drm_modeset_helper_vtables.h says using enc->commit 
hook for enabling the
encoder is a deprecated method. Also, since we have an option to get rid 
of this
extra kms->funcs->commit (for which we needed this loop around in first 
place), we
can stick with the enc->enable hook itself.
>> 
>> 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?
> 
> Hmm, do you know why it was there in the first place? It seems like 
> this
> was the
> primary reason for copy/pasting the whole function, so it probably 
> wasn't
> done
> for nothing, it'd be nice to have some history on that.
> 
h/w programming sequence differs between DPU and MDP5. MDP5 triggers the
frame kickoff in crtc->flush hook. But actual frame transfer starts only
in encoder->enable where we turn on the interface timing engine and 
trigger
the CTL flush again(!). In DPU, we enable all the hw blocks associated 
with
the encoder before triggering the frame kickoff. So a new hook, 
kms->commit was
introduced to kickoff the frame after all the components are enabled. 
Since you
are moving the kms->funcs->commit after modeset_enables, we are already 
taking
care of the order.

As an alternative, I see drm_atomic_helper_commit_tail_rpm changing the 
order
of crtc->flush / enc->enable helper calls for drivers using runtime_pm. 
If we
can take care of triggering frames from crtc->flush (as MDP5 does), we 
can even get
rid of kms->funcs->commit and adapt more helpers. But that's for the 
future
patches!

> Presumably you needed to remove the needs_modeset check as well?
> 
Yes.
> Sean
> 
>> 
>> 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

-- 
Jeykumar S