diff mbox

[v2,2/6] drm/rockchip: Don't use a delayed worker for psr state changes

Message ID 1471396276-8222-3-git-send-email-seanpaul@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Paul Aug. 17, 2016, 1:11 a.m. UTC 
The delayed worker isn't needed and is racey. Remove it and do
the state change in line.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---

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(-)

Comments

Yakir Yang Aug. 17, 2016, 2:31 a.m. UTC | #1 
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@chromium.org>
Reviewed-by: Yakir Yang <ykk@rock-chips.com>
Tested-by: Yakir Yang <ykk@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;
diff mbox

Patch

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;