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-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.

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
--
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