Quick set of async fbdev fixes

Quick set of async fbdev fixes

[Chris Wilson]

Some easy to review fbdev vs async_domain fixes.
-Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915/fbdev: Perform async fbdev initialisation much later

[Chris Wilson]

Setting up fbdev requires everything ready and registered (in particular
the connectors). In the next patch, we defer registration of the KMS
objects and unless we defer setting off fbdev, it may run before they are
registered and oops.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 24b670f72ed4..0127e969a912 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -525,18 +525,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	intel_hpd_init(dev_priv);
 
-	/*
-	 * 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.
-	 */
-	intel_fbdev_initial_config_async(dev);
-
 	drm_kms_helper_poll_init(dev);
 
 	return 0;
@@ -1413,6 +1401,18 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 		intel_gpu_ips_init(dev_priv);
 
 	i915_audio_component_init(dev_priv);
+
+	/*
+	 * 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.
+	 */
+	intel_fbdev_initial_config_async(dev);
 }
 
 /**
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915/fbdev: Limit the global async-domain synchronization

[Chris Wilson]

During cleanup we have to synchronise with the async task we are using
to initialise and register our fbdev. Currently, we are using a full
synchronisation on the global domain, but we can restrict this to just
synchronising up to our task if we remember our cookie.

v2: async_synchronize_cookie() takes an exclusive upper bound, to
synchronize with our task we have to pass in the next cookie.
v3: Drop premature disregarding of the active cookie (we need to wait
until the task is complete before continuing in the teardown).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_drv.h   |  1 +
 drivers/gpu/drm/i915/intel_fbdev.c | 29 ++++++++++++++++-------------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0c1dc9bae170..b657ddd2d078 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -159,6 +159,7 @@ struct intel_framebuffer {
 struct intel_fbdev {
 	struct drm_fb_helper helper;
 	struct intel_framebuffer *fb;
+	async_cookie_t cookie;
 	int preferred_bpp;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 4babefc51eb2..638e420a59cb 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -538,8 +538,7 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
 	.fb_probe = intelfb_create,
 };
 
-static void intel_fbdev_destroy(struct drm_device *dev,
-				struct intel_fbdev *ifbdev)
+static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
 {
 	/* We rely on the object-free to release the VMA pinning for
 	 * the info->screen_base mmaping. Leaking the VMA is simpler than
@@ -552,12 +551,14 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 	drm_fb_helper_fini(&ifbdev->helper);
 
 	if (ifbdev->fb) {
-		mutex_lock(&dev->struct_mutex);
+		mutex_lock(&ifbdev->helper.dev->struct_mutex);
 		intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0));
-		mutex_unlock(&dev->struct_mutex);
+		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
 
 		drm_framebuffer_remove(&ifbdev->fb->base);
 	}
+
+	kfree(ifbdev);
 }
 
 /*
@@ -732,32 +733,34 @@ int intel_fbdev_init(struct drm_device *dev)
 
 static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
 {
-	struct drm_i915_private *dev_priv = data;
-	struct intel_fbdev *ifbdev = dev_priv->fbdev;
+	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_fini(dev_priv->dev);
+		intel_fbdev_fini(ifbdev->helper.dev);
 }
 
 void intel_fbdev_initial_config_async(struct drm_device *dev)
 {
-	async_schedule(intel_fbdev_initial_config, to_i915(dev));
+	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
+
+	ifbdev->cookie = async_schedule(intel_fbdev_initial_config, ifbdev);
 }
 
 void intel_fbdev_fini(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	if (!dev_priv->fbdev)
+	struct intel_fbdev *ifbdev = dev_priv->fbdev;
+
+	if (!ifbdev)
 		return;
 
 	flush_work(&dev_priv->fbdev_suspend_work);
+	if (ifbdev->cookie && !current_is_async())
+		async_synchronize_cookie(ifbdev->cookie + 1);
 
-	if (!current_is_async())
-		async_synchronize_full();
-	intel_fbdev_destroy(dev, dev_priv->fbdev);
-	kfree(dev_priv->fbdev);
+	intel_fbdev_destroy(ifbdev);
 	dev_priv->fbdev = NULL;
 }
 
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/fbdev: Flush mode configuration before lastclose

[Chris Wilson]

During lastclose, we call intel_fbdev_restore_mode() to switch back to
the fbcon configuration on return to VT. However, if we have not yet
finished the asynchronous fbdev initialisation, the current mode will be
invalid and trigger WARNs upon application.

Serialise with the outstanding initialisation if the first application
exits quickly. Note that to hit this in practice requires using an
unregistered async_domain as otherwise modprobe will force a full
synchronisation prior to init() completing.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 638e420a59cb..a19d621d8815 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -830,6 +830,11 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
 	if (!ifbdev)
 		return;
 
+	if (ifbdev->cookie) {
+		async_synchronize_cookie(ifbdev->cookie + 1);
+		ifbdev->cookie = 0;
+	}
+
 	fb_helper = &ifbdev->helper;
 
 	ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/fbdev: Flush mode configuration before lastclose

[Lukas Wunner]

On Fri, Jun 17, 2016 at 06:54:49PM +0100, Chris Wilson wrote:
> During lastclose, we call intel_fbdev_restore_mode() to switch back to
> the fbcon configuration on return to VT. However, if we have not yet
> finished the asynchronous fbdev initialisation, the current mode will be
> invalid and trigger WARNs upon application.
> 
> Serialise with the outstanding initialisation if the first application
> exits quickly. Note that to hit this in practice requires using an
> unregistered async_domain as otherwise modprobe will force a full
> synchronisation prior to init() completing.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

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: stable@vger.kernel.org

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 638e420a59cb..a19d621d8815 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -830,6 +830,11 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
>  	if (!ifbdev)
>  		return;
>  
> +	if (ifbdev->cookie) {
> +		async_synchronize_cookie(ifbdev->cookie + 1);
> +		ifbdev->cookie = 0;
> +	}
> +
>  	fb_helper = &ifbdev->helper;
>  
>  	ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: warning for series starting with [1/3] drm/i915/fbdev: Perform async fbdev initialisation much later

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/fbdev: Perform async fbdev initialisation much later
URL   : https://patchwork.freedesktop.org/series/8856/
State : warning

== Summary ==

Series 8856v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/8856/revisions/1/mbox

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                fail       -> PASS       (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ro-snb-i7-2620M)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-skl-i7-6700k  total:213  pass:185  dwarn:1   dfail:0   fail:2   skip:25 
ro-bdw-i5-5250u  total:213  pass:194  dwarn:2   dfail:0   fail:2   skip:15 
ro-bdw-i7-5600u  total:213  pass:182  dwarn:1   dfail:0   fail:2   skip:28 
ro-bsw-n3050     total:213  pass:169  dwarn:1   dfail:0   fail:4   skip:39 
ro-byt-n2820     total:213  pass:171  dwarn:1   dfail:0   fail:4   skip:37 
ro-hsw-i3-4010u  total:213  pass:187  dwarn:1   dfail:0   fail:2   skip:23 
ro-hsw-i7-4770r  total:213  pass:187  dwarn:1   dfail:0   fail:2   skip:23 
ro-ilk-i7-620lm  total:213  pass:147  dwarn:1   dfail:0   fail:3   skip:62 
ro-ilk1-i5-650   total:208  pass:147  dwarn:1   dfail:0   fail:3   skip:57 
ro-ivb-i7-3770   total:213  pass:178  dwarn:1   dfail:0   fail:2   skip:32 
ro-ivb2-i7-3770  total:213  pass:182  dwarn:1   dfail:0   fail:2   skip:28 
ro-skl3-i5-6260u total:213  pass:199  dwarn:1   dfail:0   fail:2   skip:11 
ro-snb-i7-2620M  total:213  pass:170  dwarn:2   dfail:0   fail:3   skip:38 
fi-hsw-i7-4770k failed to connect after reboot
fi-skl-i5-6260u failed to connect after reboot
fi-snb-i7-2600 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1221/

8bf2b76 drm-intel-nightly: 2016y-06m-17d-19h-51m-02s UTC integration manifest
a016f62 drm/i915/fbdev: Flush mode configuration before lastclose
b3a759c drm/i915/fbdev: Limit the global async-domain synchronization
f726a71 drm/i915/fbdev: Perform async fbdev initialisation much later

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/fbdev: Perform async fbdev initialisation much later

[Daniel Vetter]

On Fri, Jun 17, 2016 at 06:54:47PM +0100, Chris Wilson wrote:
> Setting up fbdev requires everything ready and registered (in particular
> the connectors). In the next patch, we defer registration of the KMS
> objects and unless we defer setting off fbdev, it may run before they are
> registered and oops.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 24b670f72ed4..0127e969a912 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -525,18 +525,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	/* Only enable hotplug handling once the fbdev is fully set up. */
>  	intel_hpd_init(dev_priv);
>  
> -	/*
> -	 * 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.
> -	 */
> -	intel_fbdev_initial_config_async(dev);
> -
>  	drm_kms_helper_poll_init(dev);
>  
>  	return 0;
> @@ -1413,6 +1401,18 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>  		intel_gpu_ips_init(dev_priv);
>  
>  	i915_audio_component_init(dev_priv);
> +
> +	/*
> +	 * 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.
> +	 */

Comment seems bogus now - we're definitely _after_ hotplug enabling now.
Wrt fbdev locking: Thierry Redding is working on patches to arbitrarily
delay fbdev setup until the first sink shows up. Which will fix the
locking. I'd remove all that noise and just keep the part that explains
why we need hpd enabled first. With that fixed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +	intel_fbdev_initial_config_async(dev);
>  }
>  
>  /**
> -- 
> 2.8.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

Re: [PATCH 2/3] drm/i915/fbdev: Limit the global async-domain synchronization

[Daniel Vetter]

On Fri, Jun 17, 2016 at 06:54:48PM +0100, Chris Wilson wrote:
> During cleanup we have to synchronise with the async task we are using
> to initialise and register our fbdev. Currently, we are using a full
> synchronisation on the global domain, but we can restrict this to just
> synchronising up to our task if we remember our cookie.
> 
> v2: async_synchronize_cookie() takes an exclusive upper bound, to
> synchronize with our task we have to pass in the next cookie.
> v3: Drop premature disregarding of the active cookie (we need to wait
> until the task is complete before continuing in the teardown).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_fbdev.c | 29 ++++++++++++++++-------------
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0c1dc9bae170..b657ddd2d078 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -159,6 +159,7 @@ struct intel_framebuffer {
>  struct intel_fbdev {
>  	struct drm_fb_helper helper;
>  	struct intel_framebuffer *fb;
> +	async_cookie_t cookie;
>  	int preferred_bpp;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 4babefc51eb2..638e420a59cb 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -538,8 +538,7 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>  	.fb_probe = intelfb_create,
>  };
>  
> -static void intel_fbdev_destroy(struct drm_device *dev,
> -				struct intel_fbdev *ifbdev)
> +static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
>  {
>  	/* We rely on the object-free to release the VMA pinning for
>  	 * the info->screen_base mmaping. Leaking the VMA is simpler than
> @@ -552,12 +551,14 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>  	drm_fb_helper_fini(&ifbdev->helper);
>  
>  	if (ifbdev->fb) {
> -		mutex_lock(&dev->struct_mutex);
> +		mutex_lock(&ifbdev->helper.dev->struct_mutex);
>  		intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0));
> -		mutex_unlock(&dev->struct_mutex);
> +		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
>  
>  		drm_framebuffer_remove(&ifbdev->fb->base);
>  	}
> +
> +	kfree(ifbdev);
>  }
>  
>  /*
> @@ -732,32 +733,34 @@ int intel_fbdev_init(struct drm_device *dev)
>  
>  static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
>  {
> -	struct drm_i915_private *dev_priv = data;
> -	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> +	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_fini(dev_priv->dev);
> +		intel_fbdev_fini(ifbdev->helper.dev);
>  }
>  
>  void intel_fbdev_initial_config_async(struct drm_device *dev)
>  {
> -	async_schedule(intel_fbdev_initial_config, to_i915(dev));
> +	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> +
> +	ifbdev->cookie = async_schedule(intel_fbdev_initial_config, ifbdev);
>  }
>  
>  void intel_fbdev_fini(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	if (!dev_priv->fbdev)
> +	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> +
> +	if (!ifbdev)
>  		return;
>  
>  	flush_work(&dev_priv->fbdev_suspend_work);
> +	if (ifbdev->cookie && !current_is_async())
> +		async_synchronize_cookie(ifbdev->cookie + 1);

First I went like wtf about the cookie+1, but the main use case for this
function (or intended use-case at least) is to synchronize with everything
before your own async task when you register. To uphold deterministic dev
node ordering ...

Needs a comment in the code imo, this is too suprising:

	/* Only synchronizes with all _preceeding_ async tasks, hence + 1 */

Or whatever you feel like.


>  
> -	if (!current_is_async())
> -		async_synchronize_full();
> -	intel_fbdev_destroy(dev, dev_priv->fbdev);
> -	kfree(dev_priv->fbdev);
> +	intel_fbdev_destroy(ifbdev);

"While at it simplify fbdev functions paramaters." ;-)

With the bikesheds addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	dev_priv->fbdev = NULL;
>  }
>  
> -- 
> 2.8.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

Re: [PATCH 3/3] drm/i915/fbdev: Flush mode configuration before lastclose

[Daniel Vetter]

On Fri, Jun 17, 2016 at 06:54:49PM +0100, Chris Wilson wrote:
> During lastclose, we call intel_fbdev_restore_mode() to switch back to
> the fbcon configuration on return to VT. However, if we have not yet
> finished the asynchronous fbdev initialisation, the current mode will be
> invalid and trigger WARNs upon application.
> 
> Serialise with the outstanding initialisation if the first application
> exits quickly. Note that to hit this in practice requires using an
> unregistered async_domain as otherwise modprobe will force a full
> synchronisation prior to init() completing.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 638e420a59cb..a19d621d8815 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -830,6 +830,11 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
>  	if (!ifbdev)
>  		return;
>  
> +	if (ifbdev->cookie) {
> +		async_synchronize_cookie(ifbdev->cookie + 1);
> +		ifbdev->cookie = 0;
> +	}

I kinda wonder whether we shouldn't move all the fbdev async stuff into
the drm helpers. Same request here about adding a small comment about the
+1, with that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	fb_helper = &ifbdev->helper;
>  
>  	ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> -- 
> 2.8.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

Re: [PATCH 2/3] drm/i915/fbdev: Limit the global async-domain synchronization

[Chris Wilson]

On Tue, Jun 21, 2016 at 09:35:44AM +0200, Daniel Vetter wrote:
> On Fri, Jun 17, 2016 at 06:54:48PM +0100, Chris Wilson wrote:
> > During cleanup we have to synchronise with the async task we are using
> > to initialise and register our fbdev. Currently, we are using a full
> > synchronisation on the global domain, but we can restrict this to just
> > synchronising up to our task if we remember our cookie.
> > 
> > v2: async_synchronize_cookie() takes an exclusive upper bound, to
> > synchronize with our task we have to pass in the next cookie.
> > v3: Drop premature disregarding of the active cookie (we need to wait
> > until the task is complete before continuing in the teardown).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h   |  1 +
> >  drivers/gpu/drm/i915/intel_fbdev.c | 29 ++++++++++++++++-------------
> >  2 files changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0c1dc9bae170..b657ddd2d078 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -159,6 +159,7 @@ struct intel_framebuffer {
> >  struct intel_fbdev {
> >  	struct drm_fb_helper helper;
> >  	struct intel_framebuffer *fb;
> > +	async_cookie_t cookie;
> >  	int preferred_bpp;
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 4babefc51eb2..638e420a59cb 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -538,8 +538,7 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> >  	.fb_probe = intelfb_create,
> >  };
> >  
> > -static void intel_fbdev_destroy(struct drm_device *dev,
> > -				struct intel_fbdev *ifbdev)
> > +static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> >  {
> >  	/* We rely on the object-free to release the VMA pinning for
> >  	 * the info->screen_base mmaping. Leaking the VMA is simpler than
> > @@ -552,12 +551,14 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> >  	drm_fb_helper_fini(&ifbdev->helper);
> >  
> >  	if (ifbdev->fb) {
> > -		mutex_lock(&dev->struct_mutex);
> > +		mutex_lock(&ifbdev->helper.dev->struct_mutex);
> >  		intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0));
> > -		mutex_unlock(&dev->struct_mutex);
> > +		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
> >  
> >  		drm_framebuffer_remove(&ifbdev->fb->base);
> >  	}
> > +
> > +	kfree(ifbdev);
> >  }
> >  
> >  /*
> > @@ -732,32 +733,34 @@ int intel_fbdev_init(struct drm_device *dev)
> >  
> >  static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
> >  {
> > -	struct drm_i915_private *dev_priv = data;
> > -	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > +	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_fini(dev_priv->dev);
> > +		intel_fbdev_fini(ifbdev->helper.dev);
> >  }
> >  
> >  void intel_fbdev_initial_config_async(struct drm_device *dev)
> >  {
> > -	async_schedule(intel_fbdev_initial_config, to_i915(dev));
> > +	struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> > +
> > +	ifbdev->cookie = async_schedule(intel_fbdev_initial_config, ifbdev);
> >  }
> >  
> >  void intel_fbdev_fini(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	if (!dev_priv->fbdev)
> > +	struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > +
> > +	if (!ifbdev)
> >  		return;
> >  
> >  	flush_work(&dev_priv->fbdev_suspend_work);
> > +	if (ifbdev->cookie && !current_is_async())
> > +		async_synchronize_cookie(ifbdev->cookie + 1);
> 
> First I went like wtf about the cookie+1, but the main use case for this
> function (or intended use-case at least) is to synchronize with everything
> before your own async task when you register. To uphold deterministic dev
> node ordering ...

Yup, it's a total wtf. Definitely scores high on Rusty's how to screw
with your API consumers. The whole async-vs-sync kernel is the same. If
only the kernel had fences as a completion variable...

> Needs a comment in the code imo, this is too suprising:
> 
> 	/* Only synchronizes with all _preceeding_ async tasks, hence + 1 */
> 
> Or whatever you feel like.

Ok.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx