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 <ch...@chris-wilson.co.uk>

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 <linux/async.h>
>  #include <linux/i2c.h>
>  #include <linux/hdmi.h>
>  #include <linux/sched/clock.h>
> @@ -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 <linux/async.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/console.h>
> @@ -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(&ifbdev->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(&ifbdev->work, intel_fbdev_initial_config);
> +
>       drm_fb_helper_prepare(dev, &ifbdev->helper, &intel_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(&ifbdev->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(&ifbdev->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(&ifbdev->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(&dev_priv->fbdev_suspend_work);
> -     if (!current_is_async())
> -             intel_fbdev_sync(ifbdev);
> +     intel_fbdev_sync(ifbdev);
>  
>       drm_fb_helper_unregister_fbi(&ifbdev->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

Reply via email to