Re: [PATCH 2/3] drm/rockchip: Don't key off vblank for psr

[Yakir Yang <ykk@xxxxxxxxxxxxxx>]

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@xxxxxxxxxxxx>

Reviewed-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>

Also I have verified this patch on RK3399 Kevin, eDP PSR works rightly, so

Tested-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>

- 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;


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel