| Message ID | 1471563251-28754-3-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chromium.org> Reviewed-by: Yakir Yang <ykk@rock-chips.com> Also I have verified this patch on RK3399 Kevin, eDP PSR works rightly, so Tested-by: Yakir Yang <ykk@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;
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;
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@chromium.org> --- 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(-)