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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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@xxxxxxxxxxxx>
> > > ---
> > >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux