Re: [Intel-gfx] [PATCH 1/4] drm/i915/fbdev: Use an ordinary worker to avoid async deadlock
Quoting Daniel Vetter (2018-10-19 09:23:54) > On Mon, Oct 15, 2018 at 12:17:39PM +0100, Chris Wilson wrote: > > We try to avoid a deadlock of synchronizing the async fbdev task by > > skipping the synchronisation from the async worker, but that prevents us > > from using an async worker for the device probe. As we have our own > > barrier and do not rely on the global async flush, we can simply replace > > the async task with an explicit worker. > > > > References: 366e39b4d2c5 ("drm/i915: Tear down fbdev if initialization > > fails") > > Signed-off-by: Chris Wilson > > Does this now mean that fbdev setup lags behind module load? Afaiui that > will annoy userspace, which assumes that once the kms driver is loaded, > fbdev works. Or at least I have vague memories of pains in this area. I have no idea what pain you remember, I just ask wtf is fbdev. Since we register the driver before module load completes, there's a race window for third parties who can make that same assumption, so what's the difference: fbdev is available as soon as it is registered and not before. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915/fbdev: Use an ordinary worker to avoid async deadlock
On Mon, Oct 15, 2018 at 12:17:39PM +0100, Chris Wilson wrote: > We try to avoid a deadlock of synchronizing the async fbdev task by > skipping the synchronisation from the async worker, but that prevents us > from using an async worker for the device probe. As we have our own > barrier and do not rely on the global async flush, we can simply replace > the async task with an explicit worker. > > References: 366e39b4d2c5 ("drm/i915: Tear down fbdev if initialization fails") > Signed-off-by: Chris Wilson Does this now mean that fbdev setup lags behind module load? Afaiui that will annoy userspace, which assumes that once the kms driver is loaded, fbdev works. Or at least I have vague memories of pains in this area. -Daniel > --- > drivers/gpu/drm/i915/intel_drv.h | 3 +-- > drivers/gpu/drm/i915/intel_fbdev.c | 35 +++--- > 2 files changed, 18 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 3dea7a1bda7f..15bbf604724d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -25,7 +25,6 @@ > #ifndef __INTEL_DRV_H__ > #define __INTEL_DRV_H__ > > -#include > #include > #include > #include > @@ -207,8 +206,8 @@ struct intel_fbdev { > struct intel_framebuffer *fb; > struct i915_vma *vma; > unsigned long vma_flags; > - async_cookie_t cookie; > int preferred_bpp; > + struct work_struct work; > }; > > struct intel_encoder { > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > b/drivers/gpu/drm/i915/intel_fbdev.c > index 2480c7d6edee..265cc947aede 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -24,7 +24,6 @@ > * David Airlie > */ > > -#include > #include > #include > #include > @@ -666,6 +665,16 @@ static void intel_fbdev_suspend_worker(struct > work_struct *work) > true); > } > > +static void intel_fbdev_initial_config(struct work_struct *work) > +{ > + struct intel_fbdev *ifbdev = container_of(work, typeof(*ifbdev), work); > + > + /* Due to peculiar init order wrt to hpd handling this is separate. */ > + if (drm_fb_helper_initial_config(>helper, > + ifbdev->preferred_bpp)) > + intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); > +} > + > int intel_fbdev_init(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = to_i915(dev); > @@ -679,6 +688,8 @@ int intel_fbdev_init(struct drm_device *dev) > if (ifbdev == NULL) > return -ENOMEM; > > + INIT_WORK(>work, intel_fbdev_initial_config); > + > drm_fb_helper_prepare(dev, >helper, _fb_helper_funcs); > > if (!intel_fbdev_init_bios(dev, ifbdev)) > @@ -698,16 +709,6 @@ int intel_fbdev_init(struct drm_device *dev) > return 0; > } > > -static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) > -{ > - struct intel_fbdev *ifbdev = data; > - > - /* Due to peculiar init order wrt to hpd handling this is separate. */ > - if (drm_fb_helper_initial_config(>helper, > - ifbdev->preferred_bpp)) > - intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); > -} > - > void intel_fbdev_initial_config_async(struct drm_device *dev) > { > struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; > @@ -715,17 +716,16 @@ void intel_fbdev_initial_config_async(struct drm_device > *dev) > if (!ifbdev) > return; > > - ifbdev->cookie = async_schedule(intel_fbdev_initial_config, ifbdev); > + schedule_work(>work); > } > > static void intel_fbdev_sync(struct intel_fbdev *ifbdev) > { > - if (!ifbdev->cookie) > + if (!READ_ONCE(ifbdev->work.func)) > return; > > - /* Only serialises with all preceding async calls, hence +1 */ > - async_synchronize_cookie(ifbdev->cookie + 1); > - ifbdev->cookie = 0; > + flush_work(>work); > + ifbdev->work.func = NULL; > } > > void intel_fbdev_unregister(struct drm_i915_private *dev_priv) > @@ -736,8 +736,7 @@ void intel_fbdev_unregister(struct drm_i915_private > *dev_priv) > return; > > cancel_work_sync(_priv->fbdev_suspend_work); > - if (!current_is_async()) > - intel_fbdev_sync(ifbdev); > + intel_fbdev_sync(ifbdev); > > drm_fb_helper_unregister_fbi(>helper); > } > -- > 2.19.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915/fbdev: Use an ordinary worker to avoid async deadlock
We try to avoid a deadlock of synchronizing the async fbdev task by skipping the synchronisation from the async worker, but that prevents us from using an async worker for the device probe. As we have our own barrier and do not rely on the global async flush, we can simply replace the async task with an explicit worker. References: 366e39b4d2c5 ("drm/i915: Tear down fbdev if initialization fails") Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/intel_drv.h | 3 +-- drivers/gpu/drm/i915/intel_fbdev.c | 35 +++--- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3dea7a1bda7f..15bbf604724d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -25,7 +25,6 @@ #ifndef __INTEL_DRV_H__ #define __INTEL_DRV_H__ -#include #include #include #include @@ -207,8 +206,8 @@ struct intel_fbdev { struct intel_framebuffer *fb; struct i915_vma *vma; unsigned long vma_flags; - async_cookie_t cookie; int preferred_bpp; + struct work_struct work; }; struct intel_encoder { diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 2480c7d6edee..265cc947aede 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -24,7 +24,6 @@ * David Airlie */ -#include #include #include #include @@ -666,6 +665,16 @@ static void intel_fbdev_suspend_worker(struct work_struct *work) true); } +static void intel_fbdev_initial_config(struct work_struct *work) +{ + struct intel_fbdev *ifbdev = container_of(work, typeof(*ifbdev), work); + + /* Due to peculiar init order wrt to hpd handling this is separate. */ + if (drm_fb_helper_initial_config(>helper, +ifbdev->preferred_bpp)) + intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); +} + int intel_fbdev_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); @@ -679,6 +688,8 @@ int intel_fbdev_init(struct drm_device *dev) if (ifbdev == NULL) return -ENOMEM; + INIT_WORK(>work, intel_fbdev_initial_config); + drm_fb_helper_prepare(dev, >helper, _fb_helper_funcs); if (!intel_fbdev_init_bios(dev, ifbdev)) @@ -698,16 +709,6 @@ int intel_fbdev_init(struct drm_device *dev) return 0; } -static void intel_fbdev_initial_config(void *data, async_cookie_t cookie) -{ - struct intel_fbdev *ifbdev = data; - - /* Due to peculiar init order wrt to hpd handling this is separate. */ - if (drm_fb_helper_initial_config(>helper, -ifbdev->preferred_bpp)) - intel_fbdev_unregister(to_i915(ifbdev->helper.dev)); -} - void intel_fbdev_initial_config_async(struct drm_device *dev) { struct intel_fbdev *ifbdev = to_i915(dev)->fbdev; @@ -715,17 +716,16 @@ void intel_fbdev_initial_config_async(struct drm_device *dev) if (!ifbdev) return; - ifbdev->cookie = async_schedule(intel_fbdev_initial_config, ifbdev); + schedule_work(>work); } static void intel_fbdev_sync(struct intel_fbdev *ifbdev) { - if (!ifbdev->cookie) + if (!READ_ONCE(ifbdev->work.func)) return; - /* Only serialises with all preceding async calls, hence +1 */ - async_synchronize_cookie(ifbdev->cookie + 1); - ifbdev->cookie = 0; + flush_work(>work); + ifbdev->work.func = NULL; } void intel_fbdev_unregister(struct drm_i915_private *dev_priv) @@ -736,8 +736,7 @@ void intel_fbdev_unregister(struct drm_i915_private *dev_priv) return; cancel_work_sync(_priv->fbdev_suspend_work); - if (!current_is_async()) - intel_fbdev_sync(ifbdev); + intel_fbdev_sync(ifbdev); drm_fb_helper_unregister_fbi(>helper); } -- 2.19.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx