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

[Daniel Vetter <daniel@ffwll.ch>]

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 <chris@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