Re: [Intel-gfx] [PATCH 1/4] drm/i915/fbdev: Use an ordinary worker to avoid async deadlock

2018-10-19 Thread Chris Wilson
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

2018-10-19 Thread Daniel Vetter
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