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) <[email protected]> > --- > .../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) <[email protected]> > void komeda_pipeline_update(struct komeda_pipeline *pipe, _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
