[RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[John Stultz]

In some cases I've been seeing a race where two framebuffers
would be initialized, as kirin_fbdev_output_poll_changed()
might get called quickly in succession, resulting in the
initialization happening twice. This could cause the system
to boot up with a blank screen.

This patch adds a simple mutex to serialize it and seems to
avoid the race.

Suggestions or feedback would be greatly appreciated!

Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
Cc: Rongrong Zou <zourongrong@gmail.com>
Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Cc: Chen Feng <puck.chen@hisilicon.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++++
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 7ec93ae..b83556a 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -55,6 +55,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct kirin_drm_private *priv = dev->dev_private;
 
+	mutex_lock(&priv->fb_lock);
 	if (priv->fbdev) {
 		drm_fbdev_cma_hotplug_event(priv->fbdev);
 	} else {
@@ -63,6 +64,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
 		if (IS_ERR(priv->fbdev))
 			priv->fbdev = NULL;
 	}
+	mutex_unlock(&priv->fb_lock);
 }
 #endif
 
@@ -95,6 +97,8 @@ static int kirin_drm_kms_init(struct drm_device *dev)
 	if (!priv)
 		return -ENOMEM;
 
+	mutex_init(&priv->fb_lock);
+
 	dev->dev_private = priv;
 	dev_set_drvdata(dev->dev, dev);
 
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
index 7f60c649..9b6d2b1 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
@@ -23,6 +23,7 @@ struct kirin_drm_private {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_fbdev_cma *fbdev;
 #endif
+	struct mutex fb_lock;
 };
 
 extern const struct kirin_dc_ops ade_dc_ops;
-- 
2.7.4

Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[liuxinliang]

Hi John,

The patch seems good to me, except one minus comment.
Maybe change fb_lock to fbdev_lock would be better.

Thanks,
-xinliang

On 2017/2/25 9:25, John Stultz wrote:
> In some cases I've been seeing a race where two framebuffers
> would be initialized, as kirin_fbdev_output_poll_changed()
> might get called quickly in succession, resulting in the
> initialization happening twice. This could cause the system
> to boot up with a blank screen.
>
> This patch adds a simple mutex to serialize it and seems to
> avoid the race.
>
> Suggestions or feedback would be greatly appreciated!
>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Cc: Chen Feng <puck.chen@hisilicon.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>   drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++++
>   drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 1 +
>   2 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> index 7ec93ae..b83556a 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> @@ -55,6 +55,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
>   {
>   	struct kirin_drm_private *priv = dev->dev_private;
>   
> +	mutex_lock(&priv->fb_lock);
>   	if (priv->fbdev) {
>   		drm_fbdev_cma_hotplug_event(priv->fbdev);
>   	} else {
> @@ -63,6 +64,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
>   		if (IS_ERR(priv->fbdev))
>   			priv->fbdev = NULL;
>   	}
> +	mutex_unlock(&priv->fb_lock);
>   }
>   #endif
>   
> @@ -95,6 +97,8 @@ static int kirin_drm_kms_init(struct drm_device *dev)
>   	if (!priv)
>   		return -ENOMEM;
>   
> +	mutex_init(&priv->fb_lock);
> +
>   	dev->dev_private = priv;
>   	dev_set_drvdata(dev->dev, dev);
>   
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> index 7f60c649..9b6d2b1 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> @@ -23,6 +23,7 @@ struct kirin_drm_private {
>   #ifdef CONFIG_DRM_FBDEV_EMULATION
>   	struct drm_fbdev_cma *fbdev;
>   #endif
> +	struct mutex fb_lock;
>   };
>   
>   extern const struct kirin_dc_ops ade_dc_ops;

Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[John Stultz]

On Fri, Feb 24, 2017 at 5:39 PM, liuxinliang
<z.liuxinliang@hisilicon.com> wrote:
> Hi John,
>
> The patch seems good to me, except one minus comment.
> Maybe change fb_lock to fbdev_lock would be better.

Sure I'll change that too, but I'll wait before next week before
resending to see if anyone else has feedback.

thanks
-john

Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[liuxinliang]

On 2017/2/25 9:39, liuxinliang wrote:
> Hi John,
>
> The patch seems good to me, except one minus comment.
> Maybe change fb_lock to fbdev_lock would be better.
>
> Thanks,
> -xinliang
>
> On 2017/2/25 9:25, John Stultz wrote:
>> In some cases I've been seeing a race where two framebuffers
>> would be initialized, as kirin_fbdev_output_poll_changed()
>> might get called quickly in succession, resulting in the
>> initialization happening twice. This could cause the system
>> to boot up with a blank screen.
>>
>> This patch adds a simple mutex to serialize it and seems to
>> avoid the race.
>>
>> Suggestions or feedback would be greatly appreciated!
>>
>> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
>> Cc: Rongrong Zou <zourongrong@gmail.com>
>> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> Cc: Chen Feng <puck.chen@hisilicon.com>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>   drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++++
>>   drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 1 +
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c 
>> b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
>> index 7ec93ae..b83556a 100644
>> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
>> @@ -55,6 +55,7 @@ static void kirin_fbdev_output_poll_changed(struct 
>> drm_device *dev)
>>   {
>>       struct kirin_drm_private *priv = dev->dev_private;
>>   +    mutex_lock(&priv->fb_lock);
>>       if (priv->fbdev) {
>>           drm_fbdev_cma_hotplug_event(priv->fbdev);
>>       } else {
>> @@ -63,6 +64,7 @@ static void kirin_fbdev_output_poll_changed(struct 
>> drm_device *dev)
>>           if (IS_ERR(priv->fbdev))
>>               priv->fbdev = NULL;
>>       }
>> +    mutex_unlock(&priv->fb_lock);
>>   }
>>   #endif
>>   @@ -95,6 +97,8 @@ static int kirin_drm_kms_init(struct drm_device 
>> *dev)
>>       if (!priv)
>>           return -ENOMEM;
>>   +    mutex_init(&priv->fb_lock);
And put this line in CONFIG_DRM_FBDEV_EMULATION
>> +
>>       dev->dev_private = priv;
>>       dev_set_drvdata(dev->dev, dev);
>>   diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h 
>> b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
>> index 7f60c649..9b6d2b1 100644
>> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
>> @@ -23,6 +23,7 @@ struct kirin_drm_private {
>>   #ifdef CONFIG_DRM_FBDEV_EMULATION
>>       struct drm_fbdev_cma *fbdev;
>>   #endif
>> +    struct mutex fb_lock;
And here.

-xinliang

>>   };
>>     extern const struct kirin_dc_ops ade_dc_ops;
>

Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[John Stultz]

On Fri, Feb 24, 2017 at 5:45 PM, liuxinliang
<z.liuxinliang@hisilicon.com> wrote:
>
>
> On 2017/2/25 9:39, liuxinliang wrote:
>>
>> Hi John,
>>
>> The patch seems good to me, except one minus comment.
>> Maybe change fb_lock to fbdev_lock would be better.
>>
>> Thanks,
>> -xinliang
>>
>> On 2017/2/25 9:25, John Stultz wrote:
>>>
>>> In some cases I've been seeing a race where two framebuffers
>>> would be initialized, as kirin_fbdev_output_poll_changed()
>>> might get called quickly in succession, resulting in the
>>> initialization happening twice. This could cause the system
>>> to boot up with a blank screen.
>>>
>>> This patch adds a simple mutex to serialize it and seems to
>>> avoid the race.
>>>
>>> Suggestions or feedback would be greatly appreciated!
>>>
>>> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
>>> Cc: Rongrong Zou <zourongrong@gmail.com>
>>> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>> Cc: Chen Feng <puck.chen@hisilicon.com>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Sean Paul <seanpaul@chromium.org>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>>   drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++++
>>>   drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 1 +
>>>   2 files changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
>>> b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
>>> index 7ec93ae..b83556a 100644
>>> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
>>> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
>>> @@ -55,6 +55,7 @@ static void kirin_fbdev_output_poll_changed(struct
>>> drm_device *dev)
>>>   {
>>>       struct kirin_drm_private *priv = dev->dev_private;
>>>   +    mutex_lock(&priv->fb_lock);
>>>       if (priv->fbdev) {
>>>           drm_fbdev_cma_hotplug_event(priv->fbdev);
>>>       } else {
>>> @@ -63,6 +64,7 @@ static void kirin_fbdev_output_poll_changed(struct
>>> drm_device *dev)
>>>           if (IS_ERR(priv->fbdev))
>>>               priv->fbdev = NULL;
>>>       }
>>> +    mutex_unlock(&priv->fb_lock);
>>>   }
>>>   #endif
>>>   @@ -95,6 +97,8 @@ static int kirin_drm_kms_init(struct drm_device *dev)
>>>       if (!priv)
>>>           return -ENOMEM;
>>>   +    mutex_init(&priv->fb_lock);
>
> And put this line in CONFIG_DRM_FBDEV_EMULATION


Ok. Done! Thanks again for the review and feedback!
-john

Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[Daniel Vetter]

On Fri, Feb 24, 2017 at 05:25:16PM -0800, John Stultz wrote:
> In some cases I've been seeing a race where two framebuffers
> would be initialized, as kirin_fbdev_output_poll_changed()
> might get called quickly in succession, resulting in the
> initialization happening twice. This could cause the system
> to boot up with a blank screen.
> 
> This patch adds a simple mutex to serialize it and seems to
> avoid the race.
> 
> Suggestions or feedback would be greatly appreciated!

I feel a bit like a broken record, but:

Instead of reinventing broken delayed fbdev setup everywhere, can we
polish Thierry's patches to move that into the fbdev helpers in the core?
hisilicon isn't the only driver the (re)invents this weel, it'd be really
awesome if we could fix this bug just once ...

For reference, the patches:

http://markmail.org/message/d3jc4vebkndtvlkf#query:+page:1+mid:d3jc4vebkndtvlkf+state:results

I guess Thierry got sidetracked on these, but except for a bit of locking
scheme polish I think they've been mostly ready. Shouldn't be much work to
refresh them, hunt for other new drivers reinventing this wheel, do the
locking polish maybe and then get them landed.

Thanks, Daniel

> 
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Cc: Chen Feng <puck.chen@hisilicon.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 ++++
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> index 7ec93ae..b83556a 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> @@ -55,6 +55,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
>  {
>  	struct kirin_drm_private *priv = dev->dev_private;
>  
> +	mutex_lock(&priv->fb_lock);
>  	if (priv->fbdev) {
>  		drm_fbdev_cma_hotplug_event(priv->fbdev);
>  	} else {
> @@ -63,6 +64,7 @@ static void kirin_fbdev_output_poll_changed(struct drm_device *dev)
>  		if (IS_ERR(priv->fbdev))
>  			priv->fbdev = NULL;
>  	}
> +	mutex_unlock(&priv->fb_lock);
>  }
>  #endif
>  
> @@ -95,6 +97,8 @@ static int kirin_drm_kms_init(struct drm_device *dev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	mutex_init(&priv->fb_lock);
> +
>  	dev->dev_private = priv;
>  	dev_set_drvdata(dev->dev, dev);
>  
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> index 7f60c649..9b6d2b1 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> @@ -23,6 +23,7 @@ struct kirin_drm_private {
>  #ifdef CONFIG_DRM_FBDEV_EMULATION
>  	struct drm_fbdev_cma *fbdev;
>  #endif
> +	struct mutex fb_lock;
>  };
>  
>  extern const struct kirin_dc_ops ade_dc_ops;
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race

[John Stultz]

On Sat, Feb 25, 2017 at 11:36 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Feb 24, 2017 at 05:25:16PM -0800, John Stultz wrote:
>> In some cases I've been seeing a race where two framebuffers
>> would be initialized, as kirin_fbdev_output_poll_changed()
>> might get called quickly in succession, resulting in the
>> initialization happening twice. This could cause the system
>> to boot up with a blank screen.
>>
>> This patch adds a simple mutex to serialize it and seems to
>> avoid the race.
>>
>> Suggestions or feedback would be greatly appreciated!
>
> I feel a bit like a broken record, but:
>
> Instead of reinventing broken delayed fbdev setup everywhere, can we
> polish Thierry's patches to move that into the fbdev helpers in the core?
> hisilicon isn't the only driver the (re)invents this weel, it'd be really
> awesome if we could fix this bug just once ...
>
> For reference, the patches:
>
> http://markmail.org/message/d3jc4vebkndtvlkf#query:+page:1+mid:d3jc4vebkndtvlkf+state:results

Thanks for the pointer here, and apologies, I really am not very deep
into or do that much following DRM development. I just am chasing bugs
on my board and trying to fix them up, so I wasn't aware this was
something brought up before.

> I guess Thierry got sidetracked on these, but except for a bit of locking
> scheme polish I think they've been mostly ready. Shouldn't be much work to
> refresh them, hunt for other new drivers reinventing this wheel, do the
> locking polish maybe and then get them landed.

I'll take a look at it and see what I can sort out.

thanks
-john