Re: [PATCH] drm/i915: Protect fbdev across slow or failed initialisation

[Lukas Wunner <lukas@wunner.de>]

Hi,

On Thu, Feb 04, 2016 at 09:21:17AM +0000, Li, Weinan Z wrote:
> We still need this patch. Seems 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> initialization fails") this 2 patches can???t cover this case. It???s access ifbdev->fb before the initialization
> finished, but not initialization failed. If don???t have any other patches or code update relative, it may still have in 4.5.

Okay, I see.

> 
> add info NULL check should be better, it is also initialized in the async queue
> >       info = ifbdev->helper.fbdev;
> > +     if (info == NULL)
> > +            return false;
> >       if (!info->screen_base)

So if there's indeed a race between fbdev initialization and the call to
intel_fbdev_restore_mode() (on lastclose), there's more that can go awry:
- intel_fbdev_restore_mode() dereferences ifbdev->fb->obj
- it calls __drm_atomic_helper_set_config() which WARNs if ifbdev->fb is NULL
- it calls drm_fb_helper_set_par() which dereferences ifbdev->helper.fbdev

Instead of checking all that it's probably simpler to call
async_synchronize_full() at the beginning of intel_fbdev_restore_mode(),
as Li Weinan suggested. I'll submit the corresponding one-liner patch
tomorrow if noone else does.

Chris' patch also modified intel_fbdev_set_suspend() as well as
intel_fbdev_output_poll_changed(), not sure if these are racy as well.
At least the stack traces posted by Li Weinan and Gustav Fägerlind
only indicate that lastclose is racy.

Best regards,

Lukas

> From: Li, Weinan Z
> Sent: Thursday, February 04, 2016 10:34 AM
> To: 'gustav.fagerlind@gmail.com'; Lukas Wunner
> Cc: Chris Wilson; intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
> 
> Thanks for your quick response.
> Yes it is not easily be reproduced in native. In  iVGT we startup several VMs  simultaneously, it can be reproduced in several cycles, upon 1/10 fail rate.
> Need to use GUI mode but not text mode to reproduce this issue.
> 
> BRs,
> Weinan Li
> 
> 
> From: Gustav Fägerlind [mailto:gustav.fagerlind@gmail.com]
> Sent: Thursday, February 04, 2016 1:08 AM
> To: Lukas Wunner
> Cc: Chris Wilson; intel-gfx@lists.freedesktop.org<mailto:intel-gfx@lists.freedesktop.org>; Li, Weinan Z
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Protect fbdev across slow or failed initialisation
> 
> Cool, thank you.
> I dont believe I can easily reproduce it, it has only happend few times (and i reboot my lappy >2 times per day).
> 
> //
> Gustav
> 
> 2016-02-03 14:25 GMT+01:00 Lukas Wunner <lukas@wunner.de<mailto:lukas@wunner.de>>:
> Hi,
> 
> On Wed, Feb 03, 2016 at 09:17:37AM +0000, Chris Wilson wrote:
> > If the initialisation fails, we may be left with a dangling pointer with
> > an incomplete fbdev structure.
> 
> This shouldn't happen with 4.5, the fbdev is now clobbered if initialization
> fails, the existing "if (dev_priv->fbdev)" checks should thus be sufficient.
> See 54632abe8ca3 ("drm/i915: Fix oops caused by fbdev initialization
> failure") as well as 366e39b4d2c5 ("drm/i915: Tear down fbdev if
> initialization fails").
> 
> Gustav Fagerlind and Li Weinan both reported this for 4.3. It would be
> interesting to know if it can be reproduced at all with 4.5-rc2.
> 
> Best regards,
> 
> Lukas
> 
> > Here we want to disable internal calls
> > into fbdev. Similarly, the initialisation may be slow and we haven't yet
> > enabled the fbdev (e.g. quick suspend or last-close before the async init
> > completes).
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580
> > Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com<mailto:weinan.z.li@intel.com>>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk<mailto:chris@chris-wilson.co.uk>>
> > ---
> >  drivers/gpu/drm/i915/intel_fbdev.c | 41 ++++++++++++++++++++++++--------------
> >  1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 09840f4380f9..6218bc5370a1 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -114,6 +114,20 @@ static struct fb_ops intelfb_ops = {
> >       .fb_debug_leave = drm_fb_helper_debug_leave,
> >  };
> >
> > +static bool intel_fbdev_active(struct intel_fbdev *ifbdev)
> > +{
> > +     struct fb_info *info;
> > +
> > +     if (ifbdev == NULL)
> > +             return false;
> > +
> > +     info = ifbdev->helper.fbdev;
> > +     if (!info->screen_base)
> > +             return false;
> > +
> > +     return info->state == FBINFO_STATE_RUNNING;
> > +}
> > +
> >  static int intelfb_alloc(struct drm_fb_helper *helper,
> >                        struct drm_fb_helper_surface_size *sizes)
> >  {
> > @@ -753,6 +767,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> >               return;
> >
> >       info = ifbdev->helper.fbdev;
> > +     if (!info->screen_base)
> > +             return;
> >
> >       if (synchronous) {
> >               /* Flush any pending work to turn the console on, and then
> > @@ -794,29 +810,24 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous
> >
> >  void intel_fbdev_output_poll_changed(struct drm_device *dev)
> >  {
> > -     struct drm_i915_private *dev_priv = dev->dev_private;
> > -     if (dev_priv->fbdev)
> > +     struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > +     if (intel_fbdev_active(dev_priv->fbdev))
> >               drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> >  }
> >
> >  void intel_fbdev_restore_mode(struct drm_device *dev)
> >  {
> > -     int ret;
> > -     struct drm_i915_private *dev_priv = dev->dev_private;
> > -     struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > -     struct drm_fb_helper *fb_helper;
> > +     struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> >
> > -     if (!ifbdev)
> > +     if (!intel_fbdev_active(ifbdev))
> >               return;
> >
> > -     fb_helper = &ifbdev->helper;
> > -
> > -     ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> > -     if (ret) {
> > -             DRM_DEBUG("failed to restore crtc mode\n");
> > -     } else {
> > -             mutex_lock(&fb_helper->dev->struct_mutex);
> > +     if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper) == 0) {
> > +             mutex_lock(&dev->struct_mutex);
> >               intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> > -             mutex_unlock(&fb_helper->dev->struct_mutex);
> > +             mutex_unlock(&dev->struct_mutex);
> > +     } else {
> > +             DRM_DEBUG("failed to restore crtc mode\n");
> >       }
> >  }
> > --
> > 2.7.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org<mailto:Intel-gfx@lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx