Re: drm/komeda: SW workaround for D71 doesn't flush shadow registers

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

On Fri, Sep 06, 2019 at 07:18:06AM +0000, Lowry Li (Arm Technology China) wrote:
> This is a SW workaround for shadow un-flushed when together with the
> DOU Timing-disable.
> 
> D71 HW doesn't update shadow registers when display output is turned
> off. So when we disable all pipeline components together with display
> output disabling by one flush or one operation, the disable operation
> updated registers will not be flushed or valid in HW, which may lead
> problem. To workaround this problem, introduce a two phase disable for
> pipeline disable.
> 
> Phase1: Disable components with display is on and flush it, this phase
>         for flushing or validating the shadow registers.
> Phase2: Turn-off display output.
> 
> Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com>
> ---
>  .../gpu/drm/arm/display/komeda/d71/d71_dev.c  | 16 ++++
>  .../gpu/drm/arm/display/komeda/komeda_crtc.c  | 73 ++++++++++++-------
>  .../drm/arm/display/komeda/komeda_pipeline.h  | 14 +++-
>  .../display/komeda/komeda_pipeline_state.c    | 30 +++++++-
>  4 files changed, 103 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> index 2151cb3f9e68..e74069ef3b7b 100644
> --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c
> @@ -487,6 +487,22 @@ static int d71_enum_resources(struct komeda_dev *mdev)
>  			err = PTR_ERR(pipe);
>  			goto err_cleanup;
>  		}
> +
> +		/* D71 HW doesn't update shadow registers when display output
> +		 * is turning off, so when we disable all pipeline components
> +		 * together with display output disable by one flush or one
> +		 * operation, the disable operation updated registers will not
> +		 * be flush to or valid in HW, which may leads problem.
> +		 * To workaround this problem, introduce a two phase disable.
> +		 * Phase1: Disabling components with display is on to make sure
> +		 *	   the disable can be flushed to HW.
> +		 * Phase2: Only turn-off display output.
> +		 */
> +		value = KOMEDA_PIPELINE_IMPROCS |
> +			BIT(KOMEDA_COMPONENT_TIMING_CTRLR);
> +
> +		pipe->standalone_disabled_comps = value;
> +
>  		d71->pipes[i] = to_d71_pipeline(pipe);
>  	}
>  
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 3155bb17ea1b..c0c803d56d5c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -280,20 +280,53 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
>  	komeda_crtc_do_flush(crtc, old);
>  }
>  
> +static void
> +komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
> +					 struct completion *input_flip_done)
> +{
> +	struct drm_device *drm = kcrtc->base.dev;
> +	struct komeda_dev *mdev = kcrtc->master->mdev;
> +	struct completion *flip_done;
> +	struct completion temp;
> +	int timeout;
> +
> +	/* if caller doesn't send a flip_done, use a private flip_done */
> +	if (input_flip_done) {
> +		flip_done = input_flip_done;
> +	} else {
> +		init_completion(&temp);
> +		kcrtc->disable_done = &temp;
> +		flip_done = &temp;
> +	}
> +
> +	mdev->funcs->flush(mdev, kcrtc->master->id, 0);
> +
> +	/* wait the flip take affect.*/
> +	timeout = wait_for_completion_timeout(flip_done, HZ);
> +	if (timeout == 0) {
> +		DRM_ERROR("wait pipe%d flip done timeout\n", kcrtc->master->id);
> +		if (!input_flip_done) {
> +			unsigned long flags;
> +
> +			spin_lock_irqsave(&drm->event_lock, flags);
> +			kcrtc->disable_done = NULL;
> +			spin_unlock_irqrestore(&drm->event_lock, flags);
> +		}
> +	}
> +}
> +
>  static void
>  komeda_crtc_atomic_disable(struct drm_crtc *crtc,
>  			   struct drm_crtc_state *old)
>  {
>  	struct komeda_crtc *kcrtc = to_kcrtc(crtc);
>  	struct komeda_crtc_state *old_st = to_kcrtc_st(old);
> -	struct komeda_dev *mdev = crtc->dev->dev_private;
>  	struct komeda_pipeline *master = kcrtc->master;
>  	struct komeda_pipeline *slave  = kcrtc->slave;
>  	struct completion *disable_done = &crtc->state->commit->flip_done;
> -	struct completion temp;
> -	int timeout;
> +	bool needs_phase2 = false;
>  
> -	DRM_DEBUG_ATOMIC("CRTC%d_DISABLE: active_pipes: 0x%x, affected: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("CRTC%d_DISABLE: active_pipes: 0x%x, affected: 0x%x\n",
>  			 drm_crtc_index(crtc),
>  			 old_st->active_pipes, old_st->affected_pipes);
>  
> @@ -301,7 +334,7 @@ komeda_crtc_atomic_disable(struct drm_crtc *crtc,
>  		komeda_pipeline_disable(slave, old->state);
>  
>  	if (has_bit(master->id, old_st->active_pipes))
> -		komeda_pipeline_disable(master, old->state);
> +		needs_phase2 = komeda_pipeline_disable(master, old->state);
>  
>  	/* crtc_disable has two scenarios according to the state->active switch.
>  	 * 1. active -> inactive
> @@ -320,30 +353,20 @@ komeda_crtc_atomic_disable(struct drm_crtc *crtc,
>  	 *    That's also the reason why skip modeset commit in
>  	 *    komeda_crtc_atomic_flush()
>  	 */
> -	if (crtc->state->active) {
> -		struct komeda_pipeline_state *pipe_st;
> -		/* clear the old active_comps to zero */
> -		pipe_st = komeda_pipeline_get_old_state(master, old->state);
> -		pipe_st->active_comps = 0;
> +	disable_done = (needs_phase2 || crtc->state->active) ?
> +		       NULL : &crtc->state->commit->flip_done;
>  
> -		init_completion(&temp);
> -		kcrtc->disable_done = &temp;
> -		disable_done = &temp;
> -	}
> +	/* wait phase 1 disable done */
> +	komeda_crtc_flush_and_wait_for_flip_done(kcrtc, disable_done);
>  
> -	mdev->funcs->flush(mdev, master->id, 0);
> +	/* phase 2 */
> +	if (needs_phase2) {
> +		komeda_pipeline_disable(kcrtc->master, old->state);
>  
> -	/* wait the disable take affect.*/
> -	timeout = wait_for_completion_timeout(disable_done, HZ);
> -	if (timeout == 0) {
> -		DRM_ERROR("disable pipeline%d timeout.\n", kcrtc->master->id);
> -		if (crtc->state->active) {
> -			unsigned long flags;
> +		disable_done = crtc->state->active ?
> +			       NULL : &crtc->state->commit->flip_done;
>  
> -			spin_lock_irqsave(&crtc->dev->event_lock, flags);
> -			kcrtc->disable_done = NULL;
> -			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> -		}
> +		komeda_crtc_flush_and_wait_for_flip_done(kcrtc, disable_done);
>  	}
>  
>  	drm_crtc_vblank_off(crtc);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> index 4eac23ef56c1..2afd07579138 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.h
> @@ -496,6 +496,18 @@ struct komeda_pipeline {
>  	int id;
>  	/** @avail_comps: available components mask of pipeline */
>  	u32 avail_comps;
> +	/**
> +	 * @standalone_disabled_comps:
> +	 *
> +	 * When disable the pipeline, some components can not be disabled
> +	 * together with others, but need a sparated and standalone disable.
> +	 * The standalone_disabled_comps are the components which need to be
> +	 * disabled standalone, and this concept also introduce concept of
> +	 * two phase.
> +	 * phase 1: for disabling the common components.
> +	 * phase 2: for disabling the standalong_disabled_comps.
> +	 */
> +	u32 standalone_disabled_comps;
>  	/** @n_layers: the number of layer on @layers */
>  	int n_layers;
>  	/** @layers: the pipeline layers */
> @@ -674,7 +686,7 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe,
>  struct komeda_pipeline_state *
>  komeda_pipeline_get_old_state(struct komeda_pipeline *pipe,
>  			      struct drm_atomic_state *state);
> -void komeda_pipeline_disable(struct komeda_pipeline *pipe,
> +bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>  			     struct drm_atomic_state *old_state);
>  void komeda_pipeline_update(struct komeda_pipeline *pipe,
>  			    struct drm_atomic_state *old_state);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index f2c5d6c8f508..7699322949bb 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -1899,7 +1899,17 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe,
>  	return 0;
>  }
>  
> -void komeda_pipeline_disable(struct komeda_pipeline *pipe,
> +/* Since standalong disabled components must be disabled separately and in the
> + * last, So a complete disable operation may needs to call pipeline_disable
> + * twice (two phase disabling).
> + * Phase 1: disable the common components, flush it.
> + * Phase 2: disable the standalone disabled components, flush it.
> + *
> + * RETURNS:
> + * true: disable is not complete, needs a phase 2 disable.
> + * false: disable is complete.
> + */
> +bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>  			     struct drm_atomic_state *old_state)
>  {
>  	struct komeda_pipeline_state *old;
> @@ -1909,9 +1919,14 @@ void komeda_pipeline_disable(struct komeda_pipeline *pipe,
>  
>  	old = komeda_pipeline_get_old_state(pipe, old_state);
>  
> -	disabling_comps = old->active_comps;
> -	DRM_DEBUG_ATOMIC("PIPE%d: disabling_comps: 0x%x.\n",
> -			 pipe->id, disabling_comps);
> +	disabling_comps = old->active_comps &
> +			  (~pipe->standalone_disabled_comps);
> +	if (!disabling_comps)
> +		disabling_comps = old->active_comps &
> +				  pipe->standalone_disabled_comps;
> +
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
> +			 pipe->id, old->active_comps, disabling_comps);
>  
>  	dp_for_each_set_bit(id, disabling_comps) {
>  		c = komeda_pipeline_get_component(pipe, id);
> @@ -1929,6 +1944,13 @@ void komeda_pipeline_disable(struct komeda_pipeline *pipe,
>  
>  		c->funcs->disable(c);
>  	}
> +
> +	/* Update the pipeline state, if there are components that are still
> +	 * active, return true for calling the phase 2 disable.
> +	 */
> +	old->active_comps &= ~disabling_comps;
> +
> +	return old->active_comps ? true : false;
>  }
>

Looks good it me.

will push it to drm-misc-next

Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com>

>  void komeda_pipeline_update(struct komeda_pipeline *pipe,