Sean,
Thanks for this good fix.
On 08/19/2016 07:34 AM, Sean Paul wrote:
> Instead of keying off vblank for psr, just flush every time
> we get an atomic update. This ensures that cursor updates
> will properly disable psr (without turning vblank on/off),
> and unifies the paths between fb_dirty and atomic psr
> enable/disable.
>
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
Reviewed-by: Yakir Yang <ykk at rock-chips.com>
Also I have verified this patch on RK3399 Kevin, eDP PSR works rightly, so
Tested-by: Yakir Yang <ykk at rock-chips.com>
- Yakir
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 2 +-
> drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 72
> ++++++++++++++++++++---------
> drivers/gpu/drm/rockchip/rockchip_drm_psr.h | 8 ++--
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 10 ++--
> 4 files changed, 62 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index ba45d9d..10cafbc 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -70,7 +70,7 @@ static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> struct drm_clip_rect *clips,
> unsigned int num_clips)
> {
> - rockchip_drm_psr_flush(fb->dev);
> + rockchip_drm_psr_flush_all(fb->dev);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> index c6ac5d0..de6252f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> @@ -31,6 +31,7 @@ struct psr_drv {
> struct drm_encoder *encoder;
>
> spinlock_t lock;
> + bool active;
> enum psr_state state;
>
> struct timer_list flush_timer;
> @@ -67,11 +68,7 @@ static void psr_set_state_locked(struct psr_drv *psr, enum
> psr_state state)
> * v | |
> * PSR_DISABLE < - - - - - - - - -
> */
> - if (state == psr->state)
> - return;
> -
> - /* Requesting a flush when disabled is a noop */
> - if (state == PSR_FLUSH && psr->state == PSR_DISABLE)
> + if (state == psr->state || !psr->active)
> return;
>
> psr->state = state;
> @@ -115,45 +112,79 @@ static void psr_flush_handler(unsigned long data)
> }
>
> /**
> - * rockchip_drm_psr_enable - enable the encoder PSR which bind to given CRTC
> + * rockchip_drm_psr_activate - activate PSR on the given pipe
> * @crtc: CRTC to obtain the PSR encoder
> *
> * Returns:
> * Zero on success, negative errno on failure.
> */
> -int rockchip_drm_psr_enable(struct drm_crtc *crtc)
> +int rockchip_drm_psr_activate(struct drm_crtc *crtc)
> {
> struct psr_drv *psr = find_psr_by_crtc(crtc);
> + unsigned long flags;
>
> if (IS_ERR(psr))
> return PTR_ERR(psr);
>
> - psr_set_state(psr, PSR_ENABLE);
> + spin_lock_irqsave(&psr->lock, flags);
> + psr->active = true;
> + spin_unlock_irqrestore(&psr->lock, flags);
> +
> return 0;
> }
> -EXPORT_SYMBOL(rockchip_drm_psr_enable);
> +EXPORT_SYMBOL(rockchip_drm_psr_activate);
>
> /**
> - * rockchip_drm_psr_disable - disable the encoder PSR which bind to given
> CRTC
> + * rockchip_drm_psr_deactivate - deactivate PSR on the given pipe
> * @crtc: CRTC to obtain the PSR encoder
> *
> * Returns:
> * Zero on success, negative errno on failure.
> */
> -int rockchip_drm_psr_disable(struct drm_crtc *crtc)
> +int rockchip_drm_psr_deactivate(struct drm_crtc *crtc)
> {
> struct psr_drv *psr = find_psr_by_crtc(crtc);
> + unsigned long flags;
> +
> + if (IS_ERR(psr))
> + return PTR_ERR(psr);
> +
> + spin_lock_irqsave(&psr->lock, flags);
> + psr->active = false;
> + spin_unlock_irqrestore(&psr->lock, flags);
> + del_timer_sync(&psr->flush_timer);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(rockchip_drm_psr_deactivate);
> +
> +static void rockchip_drm_do_flush(struct psr_drv *psr)
> +{
> + mod_timer(&psr->flush_timer,
> + round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
> + psr_set_state(psr, PSR_FLUSH);
> +}
>
> +/**
> + * rockchip_drm_psr_flush - flush a single pipe
> + * @crtc: CRTC of the pipe to flush
> + *
> + * Returns:
> + * 0 on success, -errno on fail
> + */
> +int rockchip_drm_psr_flush(struct drm_crtc *crtc)
> +{
> + struct psr_drv *psr = find_psr_by_crtc(crtc);
> if (IS_ERR(psr))
> return PTR_ERR(psr);
>
> - psr_set_state(psr, PSR_DISABLE);
> + rockchip_drm_do_flush(psr);
> return 0;
> }
> -EXPORT_SYMBOL(rockchip_drm_psr_disable);
> +EXPORT_SYMBOL(rockchip_drm_psr_flush);
>
> /**
> - * rockchip_drm_psr_flush - force to flush all registered PSR encoders
> + * rockchip_drm_psr_flush_all - force to flush all registered PSR encoders
> * @dev: drm device
> *
> * Disable the PSR function for all registered encoders, and then enable the
> @@ -164,22 +195,18 @@ EXPORT_SYMBOL(rockchip_drm_psr_disable);
> * Returns:
> * Zero on success, negative errno on failure.
> */
> -void rockchip_drm_psr_flush(struct drm_device *dev)
> +void rockchip_drm_psr_flush_all(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) {
> - mod_timer(&psr->flush_timer,
> - round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
> -
> - psr_set_state(psr, PSR_FLUSH);
> - }
> + list_for_each_entry(psr, &drm_drv->psr_list, list)
> + rockchip_drm_do_flush(psr);
> spin_unlock_irqrestore(&drm_drv->psr_list_lock, flags);
> }
> -EXPORT_SYMBOL(rockchip_drm_psr_flush);
> +EXPORT_SYMBOL(rockchip_drm_psr_flush_all);
>
> /**
> * rockchip_drm_psr_register - register encoder to psr driver
> @@ -206,6 +233,7 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,
> setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
> spin_lock_init(&psr->lock);
>
> + psr->active = true;
> psr->state = PSR_DISABLE;
> psr->encoder = encoder;
> psr->set = psr_set;
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> index c35b688..b420cf1 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> @@ -15,9 +15,11 @@
> #ifndef __ROCKCHIP_DRM_PSR___
> #define __ROCKCHIP_DRM_PSR___
>
> -void rockchip_drm_psr_flush(struct drm_device *dev);
> -int rockchip_drm_psr_enable(struct drm_crtc *crtc);
> -int rockchip_drm_psr_disable(struct drm_crtc *crtc);
> +void rockchip_drm_psr_flush_all(struct drm_device *dev);
> +int rockchip_drm_psr_flush(struct drm_crtc *crtc);
> +
> +int rockchip_drm_psr_activate(struct drm_crtc *crtc);
> +int rockchip_drm_psr_deactivate(struct drm_crtc *crtc);
>
> int rockchip_drm_psr_register(struct drm_encoder *encoder,
> void (*psr_set)(struct drm_encoder *, bool enable));
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 7003e4d..354f814 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -573,6 +573,8 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>
> WARN_ON(vop->event);
>
> + rockchip_drm_psr_deactivate(&vop->crtc);
> +
> /*
> * We need to make sure that all windows are disabled before we
> * disable that crtc. Otherwise we might try to scan from a destroyed
> @@ -938,8 +940,6 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>
> spin_unlock_irqrestore(&vop->irq_lock, flags);
>
> - rockchip_drm_psr_disable(&vop->crtc);
> -
> return 0;
> }
>
> @@ -956,8 +956,6 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
> VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 0);
>
> spin_unlock_irqrestore(&vop->irq_lock, flags);
> -
> - rockchip_drm_psr_enable(&vop->crtc);
> }
>
> static void vop_crtc_wait_for_update(struct drm_crtc *crtc)
> @@ -1084,6 +1082,8 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
> clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
>
> VOP_CTRL_SET(vop, standby, 0);
> +
> + rockchip_drm_psr_activate(&vop->crtc);
> }
>
> static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
> @@ -1106,6 +1106,8 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
> {
> struct vop *vop = to_vop(crtc);
>
> + rockchip_drm_psr_flush(crtc);
> +
> spin_lock_irq(&crtc->dev->event_lock);
> if (crtc->state->event) {
> vop->event = crtc->state->event;
