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

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,