This is a follow-on set to Yakir's original PSR set here: https://lkml.org/lkml/2016/7/24/34
There are a few issues with the code that needed to be shored up.
(1) The use of mutexes instead of spinlocks caused issues calling the psr functions from vblank_enable/disable.
(2) The proliferation of workers due to (1)
(3) vblank is not enabled unless an event is requested, this breaks a lot of things, but most noticeable was cursor.
----------------------------------------------------------------------
Sean Paul (5): drm/rockchip: Change psr list mutex to spinlock drm/rockchip: Change state_mutex to spinlock drm/rockchip: Remove delayed work to enable/disable psr drm/rockchip: Improve analogix-dp psr handling drm/rockchip: Enable vblank without event
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 - drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 24 ++++--- 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 | 84 ++++++++++++++-------- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 43 +++++------ 6 files changed, 90 insertions(+), 66 deletions(-)
Since it needs to be taken in the vblank_enable/disable stack, which is protected by another spinlock. This lock should be completely uncontested, since it's only taken in find_psr and register, which is only called on init.
Signed-off-by: Sean Paul seanpaul@chromium.org --- 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 | 30 ++++++++++++++++------------- 3 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index fe89ea5..0f57f3b 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -189,7 +189,7 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags) 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 6281ac6..653aeb8 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -75,7 +75,7 @@ struct rockchip_drm_private { struct drm_mm mm;
struct list_head psr_list; - struct mutex psr_list_mutex; + spinlock_t psr_list_lock; };
void rockchip_drm_atomic_work(struct work_struct *work); diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c index 04d1c71..6a51851 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c @@ -41,17 +41,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;
- mutex_lock(&drm_drv->psr_list_mutex); + spin_lock_irqsave(&drm_drv->psr_list_lock, flags); list_for_each_entry(psr, &drm_drv->psr_list, list) { - if (psr->encoder->crtc == crtc) { - mutex_unlock(&drm_drv->psr_list_mutex); - return psr; - } + if (psr->encoder->crtc == crtc) + goto out; } - mutex_unlock(&drm_drv->psr_list_mutex); + psr = ERR_PTR(-ENODEV);
- return ERR_PTR(-ENODEV); +out: + spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags); + return psr; }
static void psr_set_state(struct psr_drv *psr, enum psr_state state) @@ -142,8 +143,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;
- mutex_lock(&drm_drv->psr_list_mutex); + 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; @@ -153,7 +155,7 @@ void rockchip_drm_psr_flush(struct drm_device *dev)
psr_set_state(psr, PSR_FLUSH); } - mutex_unlock(&drm_drv->psr_list_mutex); + spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags); } EXPORT_SYMBOL(rockchip_drm_psr_flush);
@@ -170,6 +172,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; @@ -186,9 +189,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; } @@ -206,8 +209,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); @@ -215,6 +219,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);
So we can change the state while holding another spinlock (such as vblank_lock). Also add some locking around the flush function to ensure there are no races.
Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 54 +++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c index 6a51851..1fe271b 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c @@ -29,7 +29,7 @@ enum psr_state { struct psr_drv { struct list_head list; enum psr_state state; - struct mutex state_mutex; + spinlock_t lock;
struct timer_list flush_timer;
@@ -55,16 +55,33 @@ 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) { - mutex_lock(&psr->state_mutex); + /* + * Allowed finite state machine: + * + * PSR_ENABLE < = = = = = > PSR_FLUSH + * | ^ | + * | | | + * v | | + * PSR_DISABLE < - - - - - - - - - + */ + + /* Forbid no state change */ + if (state == psr->state) + return;
- if (psr->state == state) { - mutex_unlock(&psr->state_mutex); + /* Forbid DISABLE change to FLUSH */ + if (state == PSR_FLUSH && psr->state == PSR_DISABLE) return; - }
psr->state = state; + + /* Allow but no need hardware change, just need assign the state */ + if (state == PSR_DISABLE && psr->state == PSR_FLUSH) + return; + + /* Refact to hardware state change */ switch (state) { case PSR_ENABLE: psr->set(psr->encoder, true); @@ -74,19 +91,30 @@ static void psr_set_state(struct psr_drv *psr, enum psr_state state) case PSR_FLUSH: psr->set(psr->encoder, false); break; - }; + } +}
- mutex_unlock(&psr->state_mutex); +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 || psr->state != PSR_FLUSH) + if (!psr) 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); }
/** @@ -147,9 +175,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));
@@ -182,8 +207,7 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder, return -ENOMEM;
setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr); - - mutex_init(&psr->state_mutex); + spin_lock_init(&psr->lock);
psr->state = PSR_DISABLE; psr->encoder = encoder;
There's no need for this to be in a worker, much less a delayed worker.
Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index c179933..86d60ff 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -88,7 +88,6 @@ #define to_vop_win(x) container_of(x, struct vop_win, base) #define to_vop_plane_state(x) container_of(x, struct vop_plane_state, base)
-#define VOP_PSR_SET_DELAY_TIME msecs_to_jiffies(10)
struct vop_plane_state { struct drm_plane_state base; @@ -120,9 +119,6 @@ struct vop { struct completion wait_update_complete; struct drm_pending_vblank_event *event;
- bool psr_enabled; - struct delayed_work psr_work; - struct completion line_flag_completion;
const struct vop_data *data; @@ -628,10 +624,7 @@ static void vop_crtc_disable(struct drm_crtc *crtc) clk_disable(vop->hclk); pm_runtime_put(vop->dev);
- if (vop->psr_enabled) { - vop->psr_enabled = false; - schedule_delayed_work(&vop->psr_work, VOP_PSR_SET_DELAY_TIME); - } + rockchip_drm_psr_disable(&vop->crtc); }
static void vop_plane_destroy(struct drm_plane *plane) @@ -915,16 +908,6 @@ static const struct drm_plane_funcs vop_plane_funcs = { .atomic_destroy_state = vop_atomic_plane_destroy_state, };
-static void vop_psr_work(struct work_struct *work) -{ - struct vop *vop = container_of(work, typeof(*vop), psr_work.work); - - if (vop->psr_enabled) - rockchip_drm_psr_enable(&vop->crtc); - else - rockchip_drm_psr_disable(&vop->crtc); -} - static int vop_crtc_enable_vblank(struct drm_crtc *crtc) { struct vop *vop = to_vop(crtc); @@ -939,8 +922,7 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
spin_unlock_irqrestore(&vop->irq_lock, flags);
- vop->psr_enabled = false; - schedule_delayed_work(&vop->psr_work, VOP_PSR_SET_DELAY_TIME); + rockchip_drm_psr_disable(&vop->crtc);
return 0; } @@ -959,8 +941,7 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
spin_unlock_irqrestore(&vop->irq_lock, flags);
- vop->psr_enabled = true; - schedule_delayed_work(&vop->psr_work, VOP_PSR_SET_DELAY_TIME); + rockchip_drm_psr_enable(&vop->crtc); }
static void vop_crtc_wait_for_update(struct drm_crtc *crtc) @@ -1645,8 +1626,7 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
pm_runtime_enable(&pdev->dev);
- vop->psr_enabled = false; - INIT_DELAYED_WORK(&vop->psr_work, vop_psr_work); + rockchip_drm_psr_disable(&vop->crtc);
return 0; }
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 --- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 - drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 24 ++++++++++++++-------- 2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index 6a5dd84..2f4d76b 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -105,7 +105,6 @@ int analogix_dp_enable_psr(struct device *dev) if (!dp->psr_support) return -EINVAL;
- /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */ memset(&psr_vsc, 0, sizeof(psr_vsc)); psr_vsc.sdp_header.HB0 = 0; diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index 9586de2..8c44dba 100644 --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -35,7 +35,6 @@ #include "rockchip_drm_psr.h" #include "rockchip_drm_vop.h"
-#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) @@ -64,7 +63,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; @@ -75,30 +75,34 @@ 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;
- vact_end = crtc->mode.vtotal - crtc->mode.vsync_start + crtc->mode.vdisplay; + vact_end = crtc->mode.vtotal - crtc->mode.vsync_start + + crtc->mode.vdisplay;
ret = rockchip_drm_wait_line_flag(dp->encoder.crtc, vact_end, PSR_WAIT_LINE_FLAG_TIMEOUT_MS); @@ -107,10 +111,12 @@ static void analogix_dp_psr_work(struct work_struct *work) return; }
- if (psr_state == EDP_VSC_PSR_STATE_ACTIVE) + spin_lock_irqsave(&dp->psr_lock, flags); + if (dp->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) @@ -146,6 +152,7 @@ static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data) { struct rockchip_dp_device *dp = to_dp(plat_data);
+ cancel_work_sync(&dp->psr_work); clk_disable_unprepare(dp->pclk);
return 0; @@ -390,8 +397,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);
vblank should be enabled regardless of whether an event is expected back. This is especially important for a cursor plane.
Signed-off-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 86d60ff..c0fbe0a 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -111,6 +111,7 @@ struct vop { struct device *dev; struct drm_device *drm_dev; bool is_enabled; + bool vblank_active;
/* mutex vsync_ work */ struct mutex vsync_mutex; @@ -961,6 +962,7 @@ static void vop_crtc_cancel_pending_vblank(struct drm_crtc *crtc, unsigned long flags;
spin_lock_irqsave(&drm->event_lock, flags); + e = vop->event; if (e && e->base.file_priv == file_priv) { vop->event = NULL; @@ -1108,9 +1110,10 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc, { struct vop *vop = to_vop(crtc);
- if (crtc->state->event) { - WARN_ON(drm_crtc_vblank_get(crtc) != 0); + WARN_ON(drm_crtc_vblank_get(crtc) != 0); + vop->vblank_active = true;
+ if (crtc->state->event) { vop->event = crtc->state->event; crtc->state->event = NULL; } @@ -1200,11 +1203,16 @@ static void vop_handle_vblank(struct vop *vop) spin_lock_irqsave(&drm->event_lock, flags);
drm_crtc_send_vblank_event(crtc, vop->event); - drm_crtc_vblank_put(crtc); vop->event = NULL;
spin_unlock_irqrestore(&drm->event_lock, flags); } + + if (vop->vblank_active) { + vop->vblank_active = false; + drm_crtc_vblank_put(crtc); + } + if (!completion_done(&vop->wait_update_complete)) complete(&vop->wait_update_complete); } @@ -1485,6 +1493,7 @@ static int vop_initial(struct vop *vop) clk_disable(vop->aclk);
vop->is_enabled = false; + vop->vblank_active = false;
return 0;
dri-devel@lists.freedesktop.org
-
Sean Paul