| Message ID | 20160309115147.67B2B6E0D3@gabe.freedesktop.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 09, 2016 at 12:06:24PM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Fix races on fbdev (rev2) > URL : https://patchwork.freedesktop.org/series/4068/ > State : failure > > == Summary == > > Series 4068v2 drm/i915: Fix races on fbdev > http://patchwork.freedesktop.org/api/1.0/series/4068/revisions/2/mbox/ > > Test gem_ringfill: > Subgroup basic-default-s3: > pass -> DMESG-WARN (bsw-nuc-2) Unrelated to this patch according to paste kindly provided by Tomi Sarvela: http://paste.debian.net/413850/ > Test kms_flip: > Subgroup basic-flip-vs-modeset: > dmesg-warn -> PASS (bdw-ultra) > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-c: > dmesg-warn -> PASS (bsw-nuc-2) > > bdw-nuci7 total:186 pass:174 dwarn:0 dfail:0 fail:0 skip:12 > bdw-ultra total:186 pass:167 dwarn:0 dfail:0 fail:0 skip:19 > bsw-nuc-2 total:186 pass:150 dwarn:1 dfail:0 fail:0 skip:35 > byt-nuc total:186 pass:154 dwarn:0 dfail:0 fail:0 skip:32 > skl-i5k-2 total:186 pass:165 dwarn:0 dfail:0 fail:0 skip:21 > skl-i7k-2 total:186 pass:165 dwarn:0 dfail:0 fail:0 skip:21 > > Results at /archive/results/CI_IGT_test/Patchwork_1548/ > > ab403b26610034afe0e0c97d960782bad98b97d0 drm-intel-nightly: 2016y-03m-09d-09h-25m-31s UTC integration manifest > a06328bd06f956c0b41202589175e68665c09ab1 drm/i915: Fix races on fbdev >
On Wed, Mar 09, 2016 at 12:52:53PM +0100, Lukas Wunner wrote: > The ->lastclose callback invokes intel_fbdev_restore_mode() and has > been witnessed to run before intel_fbdev_initial_config_async() > has finished. > > We might likewise receive hotplug events before we've had a chance to > fully set up the fbdev. > > Fix by waiting for the asynchronous thread to finish. > > v2: > An async_synchronize_full() was also added to intel_fbdev_set_suspend() > in v1 which turned out to be entirely gratuitous. It caused a deadlock > on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela > for CI support) and was unnecessary since a device is never suspended > until its ->probe callback (and all asynchronous tasks it scheduled) > have finished. See dpm_prepare(), which calls wait_for_device_probe(), > which calls async_synchronize_full(). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580 > Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com> > Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: stable@vger.kernel.org > Signed-off-by: Lukas Wunner <lukas@wunner.de> Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/i915_dma.c | 8 +++----- > drivers/gpu/drm/i915/intel_fbdev.c | 3 +++ > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 4aa3db6..9d76dfb 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -430,11 +430,9 @@ static int i915_load_modeset_init(struct drm_device *dev) > * Some ports require correctly set-up hpd registers for detection to > * work properly (leading to ghost connected connector status), e.g. VGA > * on gm45. Hence we can only set up the initial fbdev config after hpd > - * irqs are fully enabled. Now we should scan for the initial config > - * only once hotplug handling is enabled, but due to screwed-up locking > - * around kms/fbdev init we can't protect the fdbev initial config > - * scanning against hotplug events. Hence do this first and ignore the > - * tiny window where we will loose hotplug notifactions. > + * irqs are fully enabled. We protect the fbdev initial config scanning > + * against hotplug events by waiting in intel_fbdev_output_poll_changed > + * until the asynchronous thread has finished. > */ > intel_fbdev_initial_config_async(dev); > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index ae9cf6f..7e73acc 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -800,6 +800,8 @@ 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; > + > + async_synchronize_full(); > if (dev_priv->fbdev) > drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); > } > @@ -811,6 +813,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev) > struct intel_fbdev *ifbdev = dev_priv->fbdev; > struct drm_fb_helper *fb_helper; > > + async_synchronize_full(); > if (!ifbdev) > return; > > -- > 1.8.5.2 (Apple Git-48) > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 4aa3db6..9d76dfb 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -430,11 +430,9 @@ static int i915_load_modeset_init(struct drm_device *dev) * Some ports require correctly set-up hpd registers for detection to * work properly (leading to ghost connected connector status), e.g. VGA * on gm45. Hence we can only set up the initial fbdev config after hpd - * irqs are fully enabled. Now we should scan for the initial config - * only once hotplug handling is enabled, but due to screwed-up locking - * around kms/fbdev init we can't protect the fdbev initial config - * scanning against hotplug events. Hence do this first and ignore the - * tiny window where we will loose hotplug notifactions. + * irqs are fully enabled. We protect the fbdev initial config scanning + * against hotplug events by waiting in intel_fbdev_output_poll_changed + * until the asynchronous thread has finished. */ intel_fbdev_initial_config_async(dev); diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index ae9cf6f..7e73acc 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -800,6 +800,8 @@ 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; + + async_synchronize_full(); if (dev_priv->fbdev) drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper); } @@ -811,6 +813,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev) struct intel_fbdev *ifbdev = dev_priv->fbdev; struct drm_fb_helper *fb_helper; + async_synchronize_full(); if (!ifbdev) return;
The ->lastclose callback invokes intel_fbdev_restore_mode() and has been witnessed to run before intel_fbdev_initial_config_async() has finished. We might likewise receive hotplug events before we've had a chance to fully set up the fbdev. Fix by waiting for the asynchronous thread to finish. v2: An async_synchronize_full() was also added to intel_fbdev_set_suspend() in v1 which turned out to be entirely gratuitous. It caused a deadlock on suspend (discovered by CI, thanks to Damien Lespiau and Tomi Sarvela for CI support) and was unnecessary since a device is never suspended until its ->probe callback (and all asynchronous tasks it scheduled) have finished. See dpm_prepare(), which calls wait_for_device_probe(), which calls async_synchronize_full(). Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93580 Reported-by: Gustav Fägerlind <gustav.fagerlind@gmail.com> Reported-by: "Li, Weinan Z" <weinan.z.li@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: stable@vger.kernel.org Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/gpu/drm/i915/i915_dma.c | 8 +++----- drivers/gpu/drm/i915/intel_fbdev.c | 3 +++ 2 files changed, 6 insertions(+), 5 deletions(-)