| Message ID | 1487985916-29450-1-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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
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; >
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
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 >
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
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;
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(+)