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 at chromium.org>
Reviewed-by: Yakir Yang <ykk at rock-chips.com>
Tested-by: Yakir Yang <ykk at 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;