Re: [DPU PATCH 01/11] drm/msm: Skip seamless disables in crtc/encoder

[jsanka@codeaurora.org]

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);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel