Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1588025 > unrolled thread
| Started by | John Stultz <john.stultz@linaro.org> |
|---|---|
| First post | 2017-02-25 02:30 +0100 |
| Last post | 2017-02-28 00:40 +0100 |
| Articles | 7 — 3 participants |
Back to article view | Back to linux.kernel
[RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race John Stultz <john.stultz@linaro.org> - 2017-02-25 02:30 +0100
Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race liuxinliang <z.liuxinliang@hisilicon.com> - 2017-02-25 02:50 +0100
Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race John Stultz <john.stultz@linaro.org> - 2017-02-25 02:50 +0100
Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race liuxinliang <z.liuxinliang@hisilicon.com> - 2017-02-25 02:50 +0100
Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race John Stultz <john.stultz@linaro.org> - 2017-02-25 03:00 +0100
Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race Daniel Vetter <daniel@ffwll.ch> - 2017-02-25 20:50 +0100
Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race John Stultz <john.stultz@linaro.org> - 2017-02-28 00:40 +0100
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2017-02-25 02:30 +0100 |
| Subject | [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race |
| Message-ID | <tezlo-mo-3@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | liuxinliang <z.liuxinliang@hisilicon.com> |
|---|---|
| Date | 2017-02-25 02:50 +0100 |
| Subject | Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race |
| Message-ID | <tezEJ-t9-1@gated-at.bofh.it> |
| In reply to | #1588025 |
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;
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2017-02-25 02:50 +0100 |
| Subject | Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race |
| Message-ID | <tezEK-t9-17@gated-at.bofh.it> |
| In reply to | #1588028 |
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
[toc] | [prev] | [next] | [standalone]
| From | liuxinliang <z.liuxinliang@hisilicon.com> |
|---|---|
| Date | 2017-02-25 02:50 +0100 |
| Subject | Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race |
| Message-ID | <tezEJ-t9-9@gated-at.bofh.it> |
| In reply to | #1588028 |
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;
>
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2017-02-25 03:00 +0100 |
| Subject | Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race |
| Message-ID | <tezOq-wv-1@gated-at.bofh.it> |
| In reply to | #1588032 |
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
[toc] | [prev] | [next] | [standalone]
| From | Daniel Vetter <daniel@ffwll.ch> |
|---|---|
| Date | 2017-02-25 20:50 +0100 |
| Subject | Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race |
| Message-ID | <teQvT-46r-3@gated-at.bofh.it> |
| In reply to | #1588025 |
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
[toc] | [prev] | [next] | [standalone]
| From | John Stultz <john.stultz@linaro.org> |
|---|---|
| Date | 2017-02-28 00:40 +0100 |
| Subject | Re: [RFC][PATCH v2] drm: kirin: Add a mutex to avoid fb initialization race |
| Message-ID | <tfD3z-4kp-5@gated-at.bofh.it> |
| In reply to | #1588209 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web