fbdev fixes

PATCH In review Lukas Wunner 3 participants 10 messages Nov 19, 2015

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Lukas Wunner

author

2015-11-19 15:45 UTC #0 lore raw

v3 cover fbdev fixes

Respin of <[email protected]> with the following changes as per feedback from Daniel and Ville:

- Order of the two patches is reversed.
- Patch 1 is rebased on Ville's async fbdev initialization changes.
- Patch 2 with reduced null pointer checks. (No need to check for fb or obj, now that ifbdev is destroyed on initialization failure.)

Caution: I'm using current_is_async() in patch 1 to avoid a deadlock, which is currently not exported. I've submitted a patch to export it with <[email protected]>.

Should the patch be rejected, it's possible to solve it by spliting intel_fbdev_fini() in two parts as suggested by Ville, however I think using current_is_async() is preferrable as it's simpler and less code.

Browsable on GitHub:
https://github.com/l1k/linux/commits/intel_fbdev_fixes

Best regards,

Lukas

Lukas Wunner (2):
drm/i915: Tear down fbdev if initialization fails drm/i915: Fix oops caused by fbdev initialization failure

 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_fbdev.c  | 16 +++++++++++-----
 3 files changed, 32 insertions(+), 18 deletions(-)

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lukas Wunner

author

2015-11-19 15:47 UTC #1 lore raw

PATCH v3 1/2 drm/i915: Tear down fbdev if initialization fails

Currently if intelfb_create() errors out, it unrefs the bo even though the fb now owns that reference. (Spotted by Ville Syrjälä.) We should unref the fb instead of the bo.

However the fb was not necessarily allocated by intelfb_create(), it could be inherited from BIOS (the fb struct was then allocated by dev_priv->display.get_initial_plane_config()) and be in active use by a crtc. In this case we should call drm_framebuffer_remove() instead of _unreference() to also disable the crtc.

Daniel Vetter suggested that "fbdev teardown code will take care of it. The correct approach is probably to not unref anything at all".

But if fbdev initialization fails, the fbdev isn't torn down and occupies memory even though it's unusable. Therefore clobber it in intel_fbdev_initial_config(). (Currently we ignore a negative return value there.) The idea is that if fbdev initialization fails, the driver behaves as if CONFIG_DRM_FBDEV_EMULATION wasn't set. Should X11 manage to start up without errors, it will at least be able to use the memory that would otherwise be hogged by the unusable fbdev.

Also, log errors in intelfb_create().

Don't call async_synchronize_full() in intel_fbdev_fini() when called from intel_fbdev_initial_config() to avoid deadlock.

v2: Instead of calling drm_framebuffer_unreference() (if fb was not
inherited from BIOS), call intel_fbdev_fini().

v3: Rebase on e00bf69644ba (drm/i915: Move the fbdev async_schedule()
into intel_fbdev.c), call async_synchronize_full() conditionally instead of moving it into i915_driver_unload().

Cc Daniel Vetter <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/i915/intel_fbdev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_fbdev.c +7 -3

index 98772d3..cd345c5 100644
@@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
+		DRM_ERROR("Failed to allocate fb_info\n");
 		ret = PTR_ERR(info);
 		goto out_unpin;
 	}

@@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
 			   size);
 	if (!info->screen_base) {
+		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
 		ret = -ENOSPC;
 		goto out_destroy_fbi;
 	}

@@ -285,7 +287,6 @@ out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
-	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }

@@ -711,7 +712,9 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
-	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
+	if (drm_fb_helper_initial_config(&ifbdev->helper,
+					 ifbdev->preferred_bpp))
+		intel_fbdev_fini(dev_priv->dev);
 }
 
 void intel_fbdev_initial_config_async(struct drm_device *dev)

@@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev)
 
 	flush_work(&dev_priv->fbdev_suspend_work);
 
-	async_synchronize_full();
+	if (!current_is_async())
+		async_synchronize_full();
 	intel_fbdev_destroy(dev, dev_priv->fbdev);
 	kfree(dev_priv->fbdev);
 	dev_priv->fbdev = NULL;

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter

2015-11-19 15:58 UTC #2 lore raw

Re: [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails

Quoted 79 lines · Lukas Wunner

Currently if intelfb_create() errors out, it unrefs the bo even though the fb now owns that reference. (Spotted by Ville Syrjälä.) We should unref the fb instead of the bo.
However the fb was not necessarily allocated by intelfb_create(), it could be inherited from BIOS (the fb struct was then allocated by dev_priv->display.get_initial_plane_config()) and be in active use by a crtc. In this case we should call drm_framebuffer_remove() instead of _unreference() to also disable the crtc.
Daniel Vetter suggested that "fbdev teardown code will take care of it. The correct approach is probably to not unref anything at all".
But if fbdev initialization fails, the fbdev isn't torn down and occupies memory even though it's unusable. Therefore clobber it in intel_fbdev_initial_config(). (Currently we ignore a negative return value there.) The idea is that if fbdev initialization fails, the driver behaves as if CONFIG_DRM_FBDEV_EMULATION wasn't set. Should X11 manage to start up without errors, it will at least be able to use the memory that would otherwise be hogged by the unusable fbdev.
Also, log errors in intelfb_create().
Don't call async_synchronize_full() in intel_fbdev_fini() when called from intel_fbdev_initial_config() to avoid deadlock.
v2: Instead of calling drm_framebuffer_unreference() (if fb was not
inherited from BIOS), call intel_fbdev_fini().
v3: Rebase on e00bf69644ba (drm/i915: Move the fbdev async_schedule()
into intel_fbdev.c), call async_synchronize_full() conditionally instead of moving it into i915_driver_unload().
Cc Daniel Vetter <[email protected]>
Signed-off-by Lukas Wunner <[email protected]>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +5 -2
index 98772d3..cd345c5 100644
@@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
+		DRM_ERROR("Failed to allocate fb_info\n");
 		ret = PTR_ERR(info);
 		goto out_unpin;
 	}
@@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
 			   size);
 	if (!info->screen_base) {
+		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
 		ret = -ENOSPC;
 		goto out_destroy_fbi;
 	}
@@ -285,7 +287,6 @@ out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
-	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
@@ -711,7 +712,9 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
-	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
+	if (drm_fb_helper_initial_config(&ifbdev->helper,
+					 ifbdev->preferred_bpp))
+		intel_fbdev_fini(dev_priv->dev);
 }
 
 void intel_fbdev_initial_config_async(struct drm_device *dev)

drivers/gpu/drm/i915/intel_fbdev.c void intel_fbdev_fini(struct drm_device *dev) · line 734

Quoted 6 lines

drivers/gpu/drm/i915/intel_fbdev.c +2 -1

@@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev)
 
 	flush_work(&dev_priv->fbdev_suspend_work);
 
-	async_synchronize_full();
+	if (!current_is_async())
+		async_synchronize_full();

I think this is a bit too fragile, and the core depency will make merging tricky. Can't we just push the async_synchronize_full into module unload for now?
-Daniel

Quoted 6 lines

drivers/gpu/drm/i915/intel_fbdev.c +0 -0
 	intel_fbdev_destroy(dev, dev_priv->fbdev);
 	kfree(dev_priv->fbdev);
 	dev_priv->fbdev = NULL;
Signature · 1 line
2.1.0

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lukas Wunner

author

2015-11-19 16:08 UTC #3 lore raw

Re: [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails

Hi,

Quoted 83 lines · Daniel Vetter

Quoted 82 lines · Lukas Wunner
Currently if intelfb_create() errors out, it unrefs the bo even though the fb now owns that reference. (Spotted by Ville Syrjälä.) We should unref the fb instead of the bo.
However the fb was not necessarily allocated by intelfb_create(), it could be inherited from BIOS (the fb struct was then allocated by dev_priv->display.get_initial_plane_config()) and be in active use by a crtc. In this case we should call drm_framebuffer_remove() instead of _unreference() to also disable the crtc.
Daniel Vetter suggested that "fbdev teardown code will take care of it. The correct approach is probably to not unref anything at all".
But if fbdev initialization fails, the fbdev isn't torn down and occupies memory even though it's unusable. Therefore clobber it in intel_fbdev_initial_config(). (Currently we ignore a negative return value there.) The idea is that if fbdev initialization fails, the driver behaves as if CONFIG_DRM_FBDEV_EMULATION wasn't set. Should X11 manage to start up without errors, it will at least be able to use the memory that would otherwise be hogged by the unusable fbdev.
Also, log errors in intelfb_create().
Don't call async_synchronize_full() in intel_fbdev_fini() when called from intel_fbdev_initial_config() to avoid deadlock.
v2: Instead of calling drm_framebuffer_unreference() (if fb was not
inherited from BIOS), call intel_fbdev_fini().
v3: Rebase on e00bf69644ba (drm/i915: Move the fbdev async_schedule()
into intel_fbdev.c), call async_synchronize_full() conditionally instead of moving it into i915_driver_unload().
Cc Daniel Vetter <[email protected]>
Signed-off-by Lukas Wunner <[email protected]>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +5 -2
index 98772d3..cd345c5 100644
@@ -227,6 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
+		DRM_ERROR("Failed to allocate fb_info\n");
 		ret = PTR_ERR(info);
 		goto out_unpin;
 	}
@@ -253,6 +254,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
 			   size);
 	if (!info->screen_base) {
+		DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
 		ret = -ENOSPC;
 		goto out_destroy_fbi;
 	}
@@ -285,7 +287,6 @@ out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_unpin:
 	i915_gem_object_ggtt_unpin(obj);
-	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
@@ -711,7 +712,9 @@ static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
 	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
-	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
+	if (drm_fb_helper_initial_config(&ifbdev->helper,
+					 ifbdev->preferred_bpp))
+		intel_fbdev_fini(dev_priv->dev);
 }
 
 void intel_fbdev_initial_config_async(struct drm_device *dev)
@@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev)
 
 	flush_work(&dev_priv->fbdev_suspend_work);
 

Quoted 7 lines

drivers/gpu/drm/i915/intel_fbdev.c void intel_fbdev_fini(struct drm_device *dev) · line 734
Quoted 3 lines
drivers/gpu/drm/i915/intel_fbdev.c +2 -1
-	async_synchronize_full();
+	if (!current_is_async())
+		async_synchronize_full();
I think this is a bit too fragile, and the core depency will make merging tricky. Can't we just push the async_synchronize_full into module unload for now?

That was my original suggestion but Ville didn't like it... :-)

Message-ID: <[email protected]>

Link http://lists.freedesktop.org/archives/intel-gfx/2015-November/079728.html

As for merging being tricky, if Tejun only acks the other patch maybe it can be merged through drm-intel-next-queued (barring any objections against the patch itself of course).

Best regards,

Lukas

Quoted 13 lines

-Daniel
Quoted 6 lines
drivers/gpu/drm/i915/intel_fbdev.c +0 -0
 	intel_fbdev_destroy(dev, dev_priv->fbdev);
 	kfree(dev_priv->fbdev);
 	dev_priv->fbdev = NULL;
Signature · 1 line
2.1.0
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson

2015-11-19 21:46 UTC #4 lore raw

Re: [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails

In reply to Daniel Vetter

Quoted 5 lines · Daniel Vetter

Quoted 4 lines · Lukas Wunner
drivers/gpu/drm/i915/intel_fbdev.c +0 -0
@@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev)
 
 	flush_work(&dev_priv->fbdev_suspend_work);
 

Quoted 7 lines

drivers/gpu/drm/i915/intel_fbdev.c void intel_fbdev_fini(struct drm_device *dev) · line 734
Quoted 3 lines
drivers/gpu/drm/i915/intel_fbdev.c +2 -1
-	async_synchronize_full();
+	if (!current_is_async())
+		async_synchronize_full();
I think this is a bit too fragile, and the core depency will make merging tricky. Can't we just push the async_synchronize_full into module unload for now?

(intel_fbdev_fini() is already module unload, right? Do you mean just move the async handling into i915_driver_unload() so that we have a single spot for all future potential users of the async framework?)

And optimising module unload to avoid one potential grace period when we already have a bunch of grace period waits seems overkill.

The alternative to using async_synchronize_full() would be to use an async-domain.
-Chris

Signature · 5 lines

Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lukas Wunner

author

2015-11-19 22:25 UTC #5 lore raw

Re: [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails

Hi Chris,

Quoted 14 lines · Chris Wilson

Quoted 5 lines · Daniel Vetter
Quoted 4 lines · Lukas Wunner
drivers/gpu/drm/i915/intel_fbdev.c +0 -0
@@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev)
 
 	flush_work(&dev_priv->fbdev_suspend_work);
 
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c void intel_fbdev_fini(struct drm_device *dev) · line 734
Quoted 3 lines
drivers/gpu/drm/i915/intel_fbdev.c +2 -1
-	async_synchronize_full();
+	if (!current_is_async())
+		async_synchronize_full();
I think this is a bit too fragile, and the core depency will make merging tricky. Can't we just push the async_synchronize_full into module unload for now?

Quoted 1 line

(intel_fbdev_fini() is already module unload, right?

With my patch it'd be called from a 2nd site: intel_fbdev_initial_config(). To tear down the fbdev if initialization failed.

However since that function is run asynchronously, async_synchronize_full() deadlocks as it waits forever for itself to finish.

Quoted 3 lines

Do you mean just
move the async handling into i915_driver_unload() so that we have a single spot for all future potential users of the async framework?)

That precisely was the motivation for Ville's cleanup e00bf69644ba a few days ago, to consolidate things in one place. However he chose to move everything into intel_fbdev.c. That leaves three options to avoid the deadlock:

(a) call async_synchronize_full() conditionally if (!current_is_async()), that's what I did. That way it only gets called when the function is entered from i915_driver_unload(), not when it's entered from intel_fbdev_initial_config().

(b) split intel_fbdev_fini() in two, first part is called only called on module unload.

Quoted 5 lines

And optimising module unload to avoid one potential grace period when we already have a bunch of grace period waits seems overkill.
The alternative to using async_synchronize_full() would be to use an async-domain.

But an async domain wouldn't solve the deadlock, would it?

Best regards,

Lukas
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lukas Wunner

author

2015-11-19 15:49 UTC #6 lore raw

PATCH v3 2/2 drm/i915: Fix oops caused by fbdev initialization failure

In reply to Lukas Wunner

intelfb_create() is called once on driver initialization. If it fails, ifbdev->helper.fbdev, ifbdev->fb or ifbdev->fb->obj may be NULL.

Further up in the call stack, intel_fbdev_initial_config() calls intel_fbdev_fini() to tear down the ifbdev on failure. This calls intel_fbdev_destroy() which dereferences ifbdev->fb. Fix the ensuing oops.

Also check in these functions if ifbdev is not NULL to avoid oops:

i915_gem_framebuffer_info() is called on access to debugfs file "i915_gem_framebuffer" and dereferences ifbdev, ifbdev->helper.fb and ifbdev->helper.fb->obj.

intel_connector_add_to_fbdev() / intel_connector_remove_from_fbdev() are called when registering / unregistering an mst connector and dereference ifbdev.

v3: Drop additional null pointer checks in intel_fbdev_set_suspend(),
intel_fbdev_output_poll_changed() and intel_fbdev_restore_mode() since they already check if ifbdev is not NULL, which is sufficient now that intel_fbdev_fini() is called on initialization failure. (Requested by Daniel Vetter <[email protected]>)

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_fbdev.c  |  6 ++++--
 3 files changed, 25 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +13 ~~-11~~

index 038d5c6..411a9c6 100644
@@ -1877,17 +1877,19 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	ifbdev = dev_priv->fbdev;
-	fb = to_intel_framebuffer(ifbdev->helper.fb);
-
-	seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
-		   fb->base.width,
-		   fb->base.height,
-		   fb->base.depth,
-		   fb->base.bits_per_pixel,
-		   fb->base.modifier[0],
-		   atomic_read(&fb->base.refcount.refcount));
-	describe_obj(m, fb->obj);
-	seq_putc(m, '\n');
+	if (ifbdev) {
+		fb = to_intel_framebuffer(ifbdev->helper.fb);
+
+		seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
+			   fb->base.width,
+			   fb->base.height,
+			   fb->base.depth,
+			   fb->base.bits_per_pixel,
+			   fb->base.modifier[0],
+			   atomic_read(&fb->base.refcount.refcount));
+		describe_obj(m, fb->obj);
+		seq_putc(m, '\n');
+	}
 #endif
 
 	mutex_lock(&dev->mode_config.fb_lock);

drivers/gpu/drm/i915/intel_dp_mst.c +8 -2

index 9d8a5b4..8c4e7df 100644
@@ -408,7 +408,10 @@ static void intel_connector_add_to_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
+						&connector->base);
 #endif
 }

@@ -416,7 +419,10 @@ static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
+						   &connector->base);
 #endif
 }

drivers/gpu/drm/i915/intel_fbdev.c +4 -2

index cd345c5..7ccde58 100644
@@ -530,8 +530,10 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 	drm_fb_helper_fini(&ifbdev->helper);
 
-	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	drm_framebuffer_remove(&ifbdev->fb->base);
+	if (ifbdev->fb) {
+		drm_framebuffer_unregister_private(&ifbdev->fb->base);
+		drm_framebuffer_remove(&ifbdev->fb->base);
+	}
 }
 
 /*

Signature · 6 lines

2.1.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter

2015-11-19 16:02 UTC #7 lore raw

Re: [PATCH v3 2/2] drm/i915: Fix oops caused by fbdev initialization failure

Quoted 18 lines · Lukas Wunner

intelfb_create() is called once on driver initialization. If it fails, ifbdev->helper.fbdev, ifbdev->fb or ifbdev->fb->obj may be NULL.
Further up in the call stack, intel_fbdev_initial_config() calls intel_fbdev_fini() to tear down the ifbdev on failure. This calls intel_fbdev_destroy() which dereferences ifbdev->fb. Fix the ensuing oops.
Also check in these functions if ifbdev is not NULL to avoid oops:
i915_gem_framebuffer_info() is called on access to debugfs file "i915_gem_framebuffer" and dereferences ifbdev, ifbdev->helper.fb and ifbdev->helper.fb->obj.
intel_connector_add_to_fbdev() / intel_connector_remove_from_fbdev() are called when registering / unregistering an mst connector and dereference ifbdev.

Quoted 7 lines

v3: Drop additional null pointer checks in intel_fbdev_set_suspend(),
intel_fbdev_output_poll_changed() and intel_fbdev_restore_mode() since they already check if ifbdev is not NULL, which is sufficient now that intel_fbdev_fini() is called on initialization failure. (Requested by Daniel Vetter <[email protected]>)
Signed-off-by Lukas Wunner <[email protected]>

Queued for -next, thanks for the patch. Aside, with this patch and the static inline dummies from Archit I think we can drop most of the #ifdef blocks (not the one in debugfs though). Care for a follow-up patch to remove them around add/remove_one_connector?
-Daniel

Quoted 89 lines

---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_fbdev.c  |  6 ++++--
 3 files changed, 25 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +13 ~~-11~~

index 038d5c6..411a9c6 100644
@@ -1877,17 +1877,19 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	ifbdev = dev_priv->fbdev;
-	fb = to_intel_framebuffer(ifbdev->helper.fb);
-
-	seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
-		   fb->base.width,
-		   fb->base.height,
-		   fb->base.depth,
-		   fb->base.bits_per_pixel,
-		   fb->base.modifier[0],
-		   atomic_read(&fb->base.refcount.refcount));
-	describe_obj(m, fb->obj);
-	seq_putc(m, '\n');
+	if (ifbdev) {
+		fb = to_intel_framebuffer(ifbdev->helper.fb);
+
+		seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
+			   fb->base.width,
+			   fb->base.height,
+			   fb->base.depth,
+			   fb->base.bits_per_pixel,
+			   fb->base.modifier[0],
+			   atomic_read(&fb->base.refcount.refcount));
+		describe_obj(m, fb->obj);
+		seq_putc(m, '\n');
+	}
 #endif
 
 	mutex_lock(&dev->mode_config.fb_lock);

drivers/gpu/drm/i915/intel_dp_mst.c +8 -2

index 9d8a5b4..8c4e7df 100644
@@ -408,7 +408,10 @@ static void intel_connector_add_to_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
+						&connector->base);
 #endif
 }

@@ -416,7 +419,10 @@ static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
+						   &connector->base);
 #endif
 }

drivers/gpu/drm/i915/intel_fbdev.c +4 -2

index cd345c5..7ccde58 100644
@@ -530,8 +530,10 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 	drm_fb_helper_fini(&ifbdev->helper);
 
-	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	drm_framebuffer_remove(&ifbdev->fb->base);
+	if (ifbdev->fb) {
+		drm_framebuffer_unregister_private(&ifbdev->fb->base);
+		drm_framebuffer_remove(&ifbdev->fb->base);
+	}
 }
 
 /*

Signature · 1 line

2.1.0

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lukas Wunner

author

2015-11-19 16:17 UTC #8 lore raw

Re: [PATCH v3 2/2] drm/i915: Fix oops caused by fbdev initialization failure

Hi,

Quoted 25 lines · Daniel Vetter

Quoted 24 lines · Lukas Wunner
intelfb_create() is called once on driver initialization. If it fails, ifbdev->helper.fbdev, ifbdev->fb or ifbdev->fb->obj may be NULL.
Further up in the call stack, intel_fbdev_initial_config() calls intel_fbdev_fini() to tear down the ifbdev on failure. This calls intel_fbdev_destroy() which dereferences ifbdev->fb. Fix the ensuing oops.
Also check in these functions if ifbdev is not NULL to avoid oops:
i915_gem_framebuffer_info() is called on access to debugfs file "i915_gem_framebuffer" and dereferences ifbdev, ifbdev->helper.fb and ifbdev->helper.fb->obj.
intel_connector_add_to_fbdev() / intel_connector_remove_from_fbdev() are called when registering / unregistering an mst connector and dereference ifbdev.
v3: Drop additional null pointer checks in intel_fbdev_set_suspend(),
intel_fbdev_output_poll_changed() and intel_fbdev_restore_mode() since they already check if ifbdev is not NULL, which is sufficient now that intel_fbdev_fini() is called on initialization failure. (Requested by Daniel Vetter <[email protected]>)

Quoted 6 lines

Quoted 1 line
Signed-off-by Lukas Wunner <[email protected]>
Queued for -next, thanks for the patch. Aside, with this patch and the static inline dummies from Archit I think we can drop most of the #ifdef blocks (not the one in debugfs though). Care for a follow-up patch to remove them around add/remove_one_connector?

Will do, I actually did check if they are obsolete now but thought they are not because the functions are in drm_fb_helper.c which is only compiled if CONFIG_DRM_FBDEV_EMULATION is defined. I simply forgot to check if there are static inlines.

Thanks,

Lukas

Quoted 86 lines

-Daniel

Quoted 85 lines

---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp_mst.c | 10 ++++++++--
 drivers/gpu/drm/i915/intel_fbdev.c  |  6 ++++--
 3 files changed, 25 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +13 ~~-11~~

index 038d5c6..411a9c6 100644
@@ -1877,17 +1877,19 @@ static int i915_gem_framebuffer_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	ifbdev = dev_priv->fbdev;
-	fb = to_intel_framebuffer(ifbdev->helper.fb);
-
-	seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
-		   fb->base.width,
-		   fb->base.height,
-		   fb->base.depth,
-		   fb->base.bits_per_pixel,
-		   fb->base.modifier[0],
-		   atomic_read(&fb->base.refcount.refcount));
-	describe_obj(m, fb->obj);
-	seq_putc(m, '\n');
+	if (ifbdev) {
+		fb = to_intel_framebuffer(ifbdev->helper.fb);
+
+		seq_printf(m, "fbcon size: %d x %d, depth %d, %d bpp, modifier 0x%llx, refcount %d, obj ",
+			   fb->base.width,
+			   fb->base.height,
+			   fb->base.depth,
+			   fb->base.bits_per_pixel,
+			   fb->base.modifier[0],
+			   atomic_read(&fb->base.refcount.refcount));
+		describe_obj(m, fb->obj);
+		seq_putc(m, '\n');
+	}
 #endif
 
 	mutex_lock(&dev->mode_config.fb_lock);

drivers/gpu/drm/i915/intel_dp_mst.c +8 -2

index 9d8a5b4..8c4e7df 100644
@@ -408,7 +408,10 @@ static void intel_connector_add_to_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
+						&connector->base);
 #endif
 }

@@ -416,7 +419,10 @@ static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
+						   &connector->base);
 #endif
 }

drivers/gpu/drm/i915/intel_fbdev.c +4 -2

index cd345c5..7ccde58 100644
@@ -530,8 +530,10 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 	drm_fb_helper_fini(&ifbdev->helper);
 
-	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	drm_framebuffer_remove(&ifbdev->fb->base);
+	if (ifbdev->fb) {
+		drm_framebuffer_unregister_private(&ifbdev->fb->base);
+		drm_framebuffer_remove(&ifbdev->fb->base);
+	}
 }

Quoted 9 lines

Quoted 4 lines
drivers/gpu/drm/i915/intel_fbdev.c +0 -0
 /*
Signature · 1 line
2.1.0
Signature · 3 lines
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lukas Wunner

author

2015-11-19 20:26 UTC #9 lore raw

Re: [PATCH v3 1/2] drm/i915: Tear down fbdev if initialization fails

In reply to Daniel Vetter

Hi again,

Quoted 15 lines · Daniel Vetter

Quoted 14 lines · Lukas Wunner

drivers/gpu/drm/i915/intel_dp_mst.c +4 -1

@@ -408,7 +408,10 @@ static void intel_connector_add_to_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_add_one_connector(&dev_priv->fbdev->helper,
+						&connector->base);
 #endif
 }

Quoted 16 lines

drivers/gpu/drm/i915/intel_dp_mst.c static void intel_connector_remove_from_fbdev(struct intel_connector *connector) · line 428
Quoted 12 lines
drivers/gpu/drm/i915/intel_dp_mst.c +4 -1
@@ -416,7 +419,10 @@ static void intel_connector_remove_from_fbdev(struct intel_connector *connector)
 {
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper, &connector->base);
+
+	if (dev_priv->fbdev)
+		drm_fb_helper_remove_one_connector(&dev_priv->fbdev->helper,
+						   &connector->base);
 #endif
 }
 
Queued for -next, thanks for the patch. Aside, with this patch and the static inline dummies from Archit I think we can drop most of the #ifdef blocks (not the one in debugfs though). Care for a follow-up patch to remove them around add/remove_one_connector?

Looking at it once more I've realized that the fbdev member in struct drm_i915_private is enclosed in an #ifdef CONFIG_DRM_FBDEV_EMULATION, so we can't remove the #ifdefs here: The compiler would complain about non-existence of the dev_priv->fbdev member.

Quoted 5 lines · Daniel Vetter

Quoted 4 lines · Lukas Wunner
drivers/gpu/drm/i915/intel_dp_mst.c +0 -0
@@ -727,7 +730,8 @@ void intel_fbdev_fini(struct drm_device *dev)
 
 	flush_work(&dev_priv->fbdev_suspend_work);
 

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_mst.c void intel_fbdev_fini(struct drm_device *dev) · line 734
Quoted 3 lines
drivers/gpu/drm/i915/intel_dp_mst.c +2 -1
-	async_synchronize_full();
+	if (!current_is_async())
+		async_synchronize_full();
I think this is a bit too fragile, and the core depency will make merging tricky. Can't we just push the async_synchronize_full into module unload for now?

Thinking about this a bit longer I believe that if anything the change should make it more robust rather than fragile. After all we eliminate the source of a deadlock that could occur here (async_synchronize_full() waiting forever for itself to finish).

That said I'm not married to this solution. If you do find it concerning I could change it according to Ville's suggestion, i.e. splitting intel_fbdev_fini() in two parts.

Moving the async_synchronize_full() to i915_driver_unload() would be contrary to the clarity Ville had sought by consolidating everything in intel_fbdev.c.

Thanks & best regards,

Lukas
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx