On 08/17/2016 09:11 AM, Sean Paul wrote:

This patch converts the psr_list_mutex to a spinlock and locks all access to psr_list to avoid races (however unlikely they were).

Signed-off-by: Sean Paul seanpaul@chromium.org

Reviewed-by: Yakir Yang ykk@rock-chips.com

---

Changes in v2:

• Rebased on https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next

drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 2 +- drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 25 ++++++++++++++++++------- 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index b43fe5d9..76eaf1d 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -157,7 +157,7 @@ static int rockchip_drm_bind(struct device *dev) drm_dev->dev_private = private;

INIT_LIST_HEAD(&private->psr_list);

• mutex_init(&private->psr_list_mutex);

• spin_lock_init(&private->psr_list_lock);

  drm_mode_config_init(drm_dev);

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index 9c34c9e..5c69845 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -63,7 +63,7 @@ struct rockchip_drm_private { struct drm_atomic_state *state;

struct list_head psr_list;

• struct mutex psr_list_mutex;

• spinlock_t psr_list_lock; };

  int rockchip_register_crtc_funcs(struct drm_crtc *crtc,

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c index a6d3bd25..bd25273 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c @@ -45,12 +45,18 @@ static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc) { struct rockchip_drm_private *drm_drv = crtc->dev->dev_private; struct psr_drv *psr;

• unsigned long flags;

• list_for_each_entry(psr, &drm_drv->psr_list, list)

• spin_lock_irqsave(&drm_drv->psr_list_lock, flags);
• list_for_each_entry(psr, &drm_drv->psr_list, list) { if (psr->encoder->crtc == crtc)

• 	return psr;
  

• 	goto out;
  

• }
• psr = ERR_PTR(-ENODEV);

• return ERR_PTR(-ENODEV);

+out:

• spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);

• return psr; }

  static void psr_state_work(struct work_struct *work)

@@ -173,7 +179,9 @@ void rockchip_drm_psr_flush(struct drm_device *dev) { struct rockchip_drm_private *drm_drv = dev->dev_private; struct psr_drv *psr;

• unsigned long flags;

• spin_lock_irqsave(&drm_drv->psr_list_lock, flags); list_for_each_entry(psr, &drm_drv->psr_list, list) { if (psr->request_state == PSR_DISABLE) continue;

@@ -183,6 +191,7 @@ void rockchip_drm_psr_flush(struct drm_device *dev)

psr_set_state(psr, PSR_FLUSH);

}

• spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags); } EXPORT_SYMBOL(rockchip_drm_psr_flush);

@@ -199,6 +208,7 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder, { struct rockchip_drm_private *drm_drv = encoder->dev->dev_private; struct psr_drv *psr;

• unsigned long flags;

  if (!encoder || !psr_set) return -EINVAL;

@@ -215,9 +225,9 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder, psr->encoder = encoder; psr->set = psr_set;

• mutex_lock(&drm_drv->psr_list_mutex);

• spin_lock_irqsave(&drm_drv->psr_list_lock, flags); list_add_tail(&psr->list, &drm_drv->psr_list);

• mutex_unlock(&drm_drv->psr_list_mutex);

• spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);

  return 0; }

@@ -235,8 +245,9 @@ void rockchip_drm_psr_unregister(struct drm_encoder *encoder) { struct rockchip_drm_private *drm_drv = encoder->dev->dev_private; struct psr_drv *psr, *n;

• unsigned long flags;

• mutex_lock(&drm_drv->psr_list_mutex);

• spin_lock_irqsave(&drm_drv->psr_list_lock, flags); list_for_each_entry_safe(psr, n, &drm_drv->psr_list, list) { if (psr->encoder == encoder) { del_timer(&psr->flush_timer);

@@ -244,6 +255,6 @@ void rockchip_drm_psr_unregister(struct drm_encoder *encoder) kfree(psr); } }

• mutex_unlock(&drm_drv->psr_list_mutex);

• spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags); } EXPORT_SYMBOL(rockchip_drm_psr_unregister);

On 08/17/2016 09:11 AM, Sean Paul wrote:

The delayed worker isn't needed and is racey. Remove it and do the state change in line.

Signed-off-by: Sean Paul seanpaul@chromium.org

Reviewed-by: Yakir Yang ykk@rock-chips.com Tested-by: Yakir Yang ykk@rock-chips.com

---

Changes in v2:

• Rebased on https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next

drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 38 ++++++++--------------------- 1 file changed, 10 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c index bd25273..4c645d7 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c @@ -19,7 +19,6 @@ #include "rockchip_drm_psr.h"

#define PSR_FLUSH_TIMEOUT msecs_to_jiffies(3000) /* 3 seconds */ -#define PSR_SET_DELAY_TIME msecs_to_jiffies(10)

enum psr_state { PSR_FLUSH, @@ -31,11 +30,8 @@ struct psr_drv { struct list_head list; struct drm_encoder *encoder;

• enum psr_state request_state; enum psr_state state;

• struct delayed_work state_work;

• struct timer_list flush_timer;

  void (*set)(struct drm_encoder *encoder, bool enable);

@@ -59,11 +55,8 @@ out: return psr; }

-static void psr_state_work(struct work_struct *work) +static void psr_set_state(struct psr_drv *psr, enum psr_state state) {

• struct psr_drv *psr = container_of(work, typeof(*psr), state_work.work);
• enum psr_state request_state = psr->request_state;
• /*
  • Allowed finite state machine:

@@ -75,24 +68,22 @@ static void psr_state_work(struct work_struct *work) */

/* Forbid no state change */

• if (request_state == psr->state)

• if (state == psr->state) return;

  /* Forbid DISABLE change to FLUSH */

• if (request_state == PSR_FLUSH && psr->state == PSR_DISABLE)

• if (state == PSR_FLUSH && psr->state == PSR_DISABLE) return;

• /* Only wrote in this work, no need lock protection */

• psr->state = state;

• /* Allow but no need hardware change, just need assign the state */

• if (request_state == PSR_DISABLE && psr->state == PSR_FLUSH) {

• psr->state = request_state;
  

• if (state == PSR_DISABLE && psr->state == PSR_FLUSH) return;

• }

• /* Only wrote in this work, no need lock protection */

• psr->state = request_state;

  /* Refact to hardware state change */

• switch (request_state) {

• switch (psr->state) { case PSR_ENABLE: psr->set(psr->encoder, true); break;

@@ -104,13 +95,6 @@ static void psr_state_work(struct work_struct *work) } }

-static void psr_set_state(struct psr_drv *psr, enum psr_state state) -{

• psr->request_state = state;
• schedule_delayed_work(&psr->state_work, PSR_SET_DELAY_TIME);

-}

• static void psr_flush_handler(unsigned long data) { struct psr_drv *psr = (struct psr_drv *)data;

@@ -119,7 +103,7 @@ static void psr_flush_handler(unsigned long data) return;

/* State changed between flush time, then keep it */

• if (psr->request_state != PSR_FLUSH)

• if (psr->state != PSR_FLUSH) return;

  psr_set_state(psr, PSR_ENABLE);

@@ -183,7 +167,7 @@ void rockchip_drm_psr_flush(struct drm_device *dev)

spin_lock_irqsave(&drm_drv->psr_list_lock, flags); list_for_each_entry(psr, &drm_drv->psr_list, list) {

• if (psr->request_state == PSR_DISABLE)
  

• if (psr->state == PSR_DISABLE)
  continue;
  
  

  mod_timer(&psr->flush_timer,

@@ -219,8 +203,6 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,

setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);

• INIT_DELAYED_WORK(&psr->state_work, psr_state_work);
• psr->state = PSR_DISABLE; psr->encoder = encoder; psr->set = psr_set;

On 08/17/2016 09:11 AM, Sean Paul wrote:

The handling of psr state is racey, shore that up with a per-psr driver lock.

Signed-off-by: Sean Paul seanpaul@chromium.org

Reviewed-by: Yakir Yang ykk@rock-chips.com

---

Changes in v2:

• Rebased on https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next

drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c index 4c645d7..5bd54f2 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c @@ -30,6 +30,7 @@ struct psr_drv { struct list_head list; struct drm_encoder *encoder;

• spinlock_t lock; enum psr_state state;

  struct timer_list flush_timer;

@@ -55,7 +56,7 @@ out: return psr; }

-static void psr_set_state(struct psr_drv *psr, enum psr_state state) +static void psr_set_state_locked(struct psr_drv *psr, enum psr_state state) { /* * Allowed finite state machine: @@ -75,7 +76,6 @@ static void psr_set_state(struct psr_drv *psr, enum psr_state state) if (state == PSR_FLUSH && psr->state == PSR_DISABLE) return;

• /* Only wrote in this work, no need lock protection */ psr->state = state;

  /* Allow but no need hardware change, just need assign the state */

@@ -95,18 +95,28 @@ static void psr_set_state(struct psr_drv *psr, enum psr_state state) } }

+static void psr_set_state(struct psr_drv *psr, enum psr_state state) +{

• unsigned long flags;
• spin_lock_irqsave(&psr->lock, flags);
• psr_set_state_locked(psr, state);
• spin_unlock_irqrestore(&psr->lock, flags);

+}

• static void psr_flush_handler(unsigned long data) { struct psr_drv *psr = (struct psr_drv *)data;

• unsigned long flags;

  if (!psr) return;

  /* State changed between flush time, then keep it */

• if (psr->state != PSR_FLUSH)

• return;
  

• psr_set_state(psr, PSR_ENABLE);

• spin_lock_irqsave(&psr->lock, flags);

• if (psr->state == PSR_FLUSH)

• psr_set_state_locked(psr, PSR_ENABLE);
  

• spin_unlock_irqrestore(&psr->lock, flags); }

  /**

@@ -167,9 +177,6 @@ void rockchip_drm_psr_flush(struct drm_device *dev)

spin_lock_irqsave(&drm_drv->psr_list_lock, flags); list_for_each_entry(psr, &drm_drv->psr_list, list) {

• if (psr->state == PSR_DISABLE)
  

• 	continue;
  

• mod_timer(&psr->flush_timer, round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));

@@ -202,6 +209,7 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder, return -ENOMEM;

setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);

• spin_lock_init(&psr->lock);

  psr->state = PSR_DISABLE; psr->encoder = encoder;

On 08/17/2016 09:11 AM, Sean Paul wrote:

A few things that need tidying up, no functional changes.

Signed-off-by: Sean Paul seanpaul@chromium.org

Reviewed-by: Yakir Yang ykk@rock-chips.com

---

Changes in v2:

• Introduced

drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c index 5bd54f2..c6ac5d0 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c @@ -62,27 +62,25 @@ static void psr_set_state_locked(struct psr_drv *psr, enum psr_state state) * Allowed finite state machine: * * PSR_ENABLE < = = = = = > PSR_FLUSH

•  *      | ^                        |
  

•  *      | |                        |
  

•  *      v |                        |
  

• *       | ^                        |
  

• *       | |                        |
  

• *       v |                        |
  

  • PSR_DISABLE < - - - - - - - - -
  */

• /* Forbid no state change */ if (state == psr->state) return;

• /* Forbid DISABLE change to FLUSH */

• /* Requesting a flush when disabled is a noop */ if (state == PSR_FLUSH && psr->state == PSR_DISABLE) return;

  psr->state = state;

• /* Allow but no need hardware change, just need assign the state */

• /* Already disabled in flush, change the state, but not the hardware */ if (state == PSR_DISABLE && psr->state == PSR_FLUSH) return;

• /* Refact to hardware state change */

• /* Actually commit the state change to hardware */ switch (psr->state) { case PSR_ENABLE: psr->set(psr->encoder, true);

@@ -109,10 +107,7 @@ static void psr_flush_handler(unsigned long data) struct psr_drv *psr = (struct psr_drv *)data; unsigned long flags;

• if (!psr)

• return;
  

• /* State changed between flush time, then keep it */

• /* If the state has changed since we initiated the flush, do nothing */ spin_lock_irqsave(&psr->lock, flags); if (psr->state == PSR_FLUSH) psr_set_state_locked(psr, PSR_ENABLE);

On 08/17/2016 09:11 AM, Sean Paul wrote:

Remove the delayed worker, opting instead for the non-delayed variety. Also introduce a lock to ensure we don't have races with the worker and psr_state. Finally, cancel and wait for the worker to finish when disabling the bridge.

Signed-off-by: Sean Paul seanpaul@chromium.org

Reviewed-by: Yakir Yang ykk@rock-chips.com

---

Changes in v2:

• Rebased on https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next

drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index d6d0751..439b933 100644 --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -42,7 +42,6 @@

#define HIWORD_UPDATE(val, mask) (val | (mask) << 16)

-#define PSR_SET_DELAY_TIME msecs_to_jiffies(10) #define PSR_WAIT_LINE_FLAG_TIMEOUT_MS 100

#define to_dp(nm) container_of(nm, struct rockchip_dp_device, nm) @@ -72,7 +71,8 @@ struct rockchip_dp_device { struct regmap *grf; struct reset_control *rst;

• struct delayed_work psr_work;

• struct work_struct psr_work;

• spinlock_t psr_lock; unsigned int psr_state;

  const struct rockchip_dp_chip_data *data;

@@ -83,25 +83,29 @@ struct rockchip_dp_device { static void analogix_dp_psr_set(struct drm_encoder *encoder, bool enabled) { struct rockchip_dp_device *dp = to_dp(encoder);

• unsigned long flags;

  dev_dbg(dp->dev, "%s PSR...\n", enabled ? "Entry" : "Exit");

• spin_lock_irqsave(&dp->psr_lock, flags); if (enabled) dp->psr_state = EDP_VSC_PSR_STATE_ACTIVE; else dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;

• schedule_delayed_work(&dp->psr_work, PSR_SET_DELAY_TIME);

• schedule_work(&dp->psr_work);

• spin_unlock_irqrestore(&dp->psr_lock, flags); }

  static void analogix_dp_psr_work(struct work_struct *work) { struct rockchip_dp_device *dp =

• 		container_of(work, typeof(*dp), psr_work.work);
  

• 		container_of(work, typeof(*dp), psr_work);
  

  struct drm_crtc *crtc = dp->encoder.crtc; int psr_state = dp->psr_state; int vact_end; int ret;

• unsigned long flags;

  if (!crtc) return;

@@ -115,10 +119,12 @@ static void analogix_dp_psr_work(struct work_struct *work) return; }

• spin_lock_irqsave(&dp->psr_lock, flags); if (psr_state == EDP_VSC_PSR_STATE_ACTIVE) analogix_dp_enable_psr(dp->dev); else analogix_dp_disable_psr(dp->dev);

• spin_unlock_irqrestore(&dp->psr_lock, flags); }

  static int rockchip_dp_pre_init(struct rockchip_dp_device *dp)

@@ -135,6 +141,8 @@ static int rockchip_dp_poweron(struct analogix_dp_plat_data *plat_data) struct rockchip_dp_device *dp = to_dp(plat_data); int ret;

• cancel_work_sync(&dp->psr_work);
• ret = clk_prepare_enable(dp->pclk); if (ret < 0) { dev_err(dp->dev, "failed to enable pclk %d\n", ret);

@@ -390,8 +398,9 @@ static int rockchip_dp_bind(struct device *dev, struct device *master, dp->plat_data.power_off = rockchip_dp_powerdown; dp->plat_data.get_modes = rockchip_dp_get_modes;

• spin_lock_init(&dp->psr_lock); dp->psr_state = ~EDP_VSC_PSR_STATE_ACTIVE;

• INIT_DELAYED_WORK(&dp->psr_work, analogix_dp_psr_work);

• INIT_WORK(&dp->psr_work, analogix_dp_psr_work);

  rockchip_drm_psr_register(&dp->encoder, analogix_dp_psr_set);

Sean,

On 08/17/2016 09:11 AM, Sean Paul wrote:

vblank should be enabled regardless of whether an event is expected back. This is especially important for a cursor plane.

Yep, I also found that sometimes vblank haven't been enabled when I move the mouse lightly, that would cause eDP panel wound't exit from PSR active state, and then nothing would be updated on panel. After apply this patch, things work rightly now, thanks for fixing.

Reviewed-by: Yakir Yang ykk@rock-chips.com Tested-by: Yakir Yang ykk@rock-chip.com

Signed-off-by: Sean Paul seanpaul@chromium.org

Changes in v2:

• Rebased on https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next

drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index d1e0e06..1787084 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -112,6 +112,7 @@ struct vop { struct device *dev; struct drm_device *drm_dev; bool is_enabled;

• bool vblank_active;

  /* mutex vsync_ work */ struct mutex vsync_mutex;

@@ -1107,10 +1108,11 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc, struct vop *vop = to_vop(crtc);

spin_lock_irq(&crtc->dev->event_lock);

• if (crtc->state->event) {

• WARN_ON(drm_crtc_vblank_get(crtc) != 0);
  

• WARN_ON(vop->event);
  

• vop->vblank_active = true;

• WARN_ON(drm_crtc_vblank_get(crtc) != 0);

• WARN_ON(vop->event);

• if (crtc->state->event) { vop->event = crtc->state->event; crtc->state->event = NULL; }

@@ -1197,12 +1199,14 @@ static void vop_handle_vblank(struct vop *vop)

spin_lock_irqsave(&drm->event_lock, flags); if (vop->event) {

• drm_crtc_send_vblank_event(crtc, vop->event);

• drm_crtc_vblank_put(crtc);
  

  vop->event = NULL;

  }

• if (vop->vblank_active) {

• vop->vblank_active = false;
  

• drm_crtc_vblank_put(crtc);
  

• } spin_unlock_irqrestore(&drm->event_lock, flags);

  if (!completion_done(&vop->wait_update_complete))

@@ -1472,6 +1476,7 @@ static int vop_initial(struct vop *vop) clk_disable(vop->aclk);

vop->is_enabled = false;

• vop->vblank_active = false;

  return 0;