| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190927080034.GA23652@jamwan02-TSP300>
Date: Fri, 27 Sep 2019 08:00:41 +0000
From: "james qian wang (Arm Technology China)" <james.qian.wang@....com>
To: "Lowry Li (Arm Technology China)" <Lowry.Li@....com>
CC: Liviu Dudau <Liviu.Dudau@....com>,
"maarten.lankhorst@...ux.intel.com"
<maarten.lankhorst@...ux.intel.com>,
"seanpaul@...omium.org" <seanpaul@...omium.org>,
"airlied@...ux.ie" <airlied@...ux.ie>,
Brian Starkey <Brian.Starkey@....com>,
Mihail Atanassov <Mihail.Atanassov@....com>,
Ayan Halder <Ayan.Halder@....com>,
"Jonathan Chai (Arm Technology China)" <Jonathan.Chai@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"Julien Yin (Arm Technology China)" <Julien.Yin@....com>,
nd <nd@....com>
Subject: 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@....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@....com>
> void komeda_pipeline_update(struct komeda_pipeline *pipe,
Powered by blists - more mailing lists