diff mbox

[v2,3/6] drm/rockchip: Use a spinlock to protect psr state

Message ID 1471396276-8222-4-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 handling of psr state is racey, shore that up with
a per-psr driver lock.

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 | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Comments

Yakir Yang Aug. 17, 2016, 2:33 a.m. UTC | #1 
On 08/17/2016 09:11 AM, Sean Paul wrote:
> The handling of psr state is racey, shore that up with
> a per-psr driver lock.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-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 | 26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> index 4c645d7..5bd54f2 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> @@ -30,6 +30,7 @@ struct psr_drv {
>   	struct list_head	list;
>   	struct drm_encoder	*encoder;
>   
> +	spinlock_t		lock;
>   	enum psr_state		state;
>   
>   	struct timer_list	flush_timer;
> @@ -55,7 +56,7 @@ out:
>   	return psr;
>   }
>   
> -static void psr_set_state(struct psr_drv *psr, enum psr_state state)
> +static void psr_set_state_locked(struct psr_drv *psr, enum psr_state state)
>   {
>   	/*
>   	 * Allowed finite state machine:
> @@ -75,7 +76,6 @@ static void psr_set_state(struct psr_drv *psr, enum psr_state state)
>   	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 */
> @@ -95,18 +95,28 @@ static void psr_set_state(struct psr_drv *psr, enum psr_state state)
>   	}
>   }
>   
> +static void psr_set_state(struct psr_drv *psr, enum psr_state state)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&psr->lock, flags);
> +	psr_set_state_locked(psr, state);
> +	spin_unlock_irqrestore(&psr->lock, flags);
> +}
> +
>   static void psr_flush_handler(unsigned long data)
>   {
>   	struct psr_drv *psr = (struct psr_drv *)data;
> +	unsigned long flags;
>   
>   	if (!psr)
>   		return;
>   
>   	/* State changed between flush time, then keep it */
> -	if (psr->state != PSR_FLUSH)
> -		return;
> -
> -	psr_set_state(psr, PSR_ENABLE);
> +	spin_lock_irqsave(&psr->lock, flags);
> +	if (psr->state == PSR_FLUSH)
> +		psr_set_state_locked(psr, PSR_ENABLE);
> +	spin_unlock_irqrestore(&psr->lock, flags);
>   }
>   
>   /**
> @@ -167,9 +177,6 @@ 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->state == PSR_DISABLE)
> -			continue;
> -
>   		mod_timer(&psr->flush_timer,
>   			  round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
>   
> @@ -202,6 +209,7 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,
>   		return -ENOMEM;
>   
>   	setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
> +	spin_lock_init(&psr->lock);
>   
>   	psr->state = PSR_DISABLE;
>   	psr->encoder = encoder;
diff mbox

Patch

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
index 4c645d7..5bd54f2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
@@ -30,6 +30,7 @@  struct psr_drv {
 	struct list_head	list;
 	struct drm_encoder	*encoder;
 
+	spinlock_t		lock;
 	enum psr_state		state;
 
 	struct timer_list	flush_timer;
@@ -55,7 +56,7 @@  out:
 	return psr;
 }
 
-static void psr_set_state(struct psr_drv *psr, enum psr_state state)
+static void psr_set_state_locked(struct psr_drv *psr, enum psr_state state)
 {
 	/*
 	 * Allowed finite state machine:
@@ -75,7 +76,6 @@  static void psr_set_state(struct psr_drv *psr, enum psr_state state)
 	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 */
@@ -95,18 +95,28 @@  static void psr_set_state(struct psr_drv *psr, enum psr_state state)
 	}
 }
 
+static void psr_set_state(struct psr_drv *psr, enum psr_state state)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&psr->lock, flags);
+	psr_set_state_locked(psr, state);
+	spin_unlock_irqrestore(&psr->lock, flags);
+}
+
 static void psr_flush_handler(unsigned long data)
 {
 	struct psr_drv *psr = (struct psr_drv *)data;
+	unsigned long flags;
 
 	if (!psr)
 		return;
 
 	/* State changed between flush time, then keep it */
-	if (psr->state != PSR_FLUSH)
-		return;
-
-	psr_set_state(psr, PSR_ENABLE);
+	spin_lock_irqsave(&psr->lock, flags);
+	if (psr->state == PSR_FLUSH)
+		psr_set_state_locked(psr, PSR_ENABLE);
+	spin_unlock_irqrestore(&psr->lock, flags);
 }
 
 /**
@@ -167,9 +177,6 @@  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->state == PSR_DISABLE)
-			continue;
-
 		mod_timer(&psr->flush_timer,
 			  round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
 
@@ -202,6 +209,7 @@  int rockchip_drm_psr_register(struct drm_encoder *encoder,
 		return -ENOMEM;
 
 	setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
+	spin_lock_init(&psr->lock);
 
 	psr->state = PSR_DISABLE;
 	psr->encoder = encoder;