From 7580d774b0466fff28aab19db4f36dac37a3d1a9 Mon Sep 17 00:00:00 2001 From: Maarten Lankhorst Date: Tue, 18 Aug 2015 13:40:06 +0200 Subject: drm/i915: Wait for object idle without locks in atomic_commit, v2. Make pinning and waiting a separate step, and wait for object idle without struct_mutex held. Changes since v1: - Do not wait when a reset is in progress. - Remove call to i915_gem_object_wait_rendering for intel_overlay_do_put_image (Chris Wilson) Signed-off-by: Maarten Lankhorst Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/intel_fbdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/i915/intel_fbdev.c') diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 4fd5fdfef6bd..840d6bf5e8d5 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -161,7 +161,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, } /* Flush everything out, we'll be doing GTT only from now on */ - ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL); + ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL); if (ret) { DRM_ERROR("failed to pin obj: %d\n", ret); goto out_fb; -- cgit v1.2.3 From ca40ba855c9e3f19f2715fd8a1ced5128359d3d9 Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Thu, 22 Oct 2015 13:37:18 +0200 Subject: drm/i915: Fix double unref in intelfb_alloc failure path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In intelfb_alloc(), if the call to intel_pin_and_fence_fb_obj() fails, the bo is unrefed twice: By drm_framebuffer_remove() and once more by drm_gem_object_unreference(). Fix it. Reported-by: Ville Syrjälä Signed-off-by: Lukas Wunner Reviewed-by: Daniel Vetter Link: http://patchwork.freedesktop.org/patch/msgid/cd7b33330621a350b0159ec5e098297b139cfaf7.1446892879.git.lukas@wunner.de Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_fbdev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/gpu/drm/i915/intel_fbdev.c') diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 840d6bf5e8d5..2240e709ede6 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -156,8 +156,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper, fb = __intel_framebuffer_create(dev, &mode_cmd, obj); if (IS_ERR(fb)) { + drm_gem_object_unreference(&obj->base); ret = PTR_ERR(fb); - goto out_unref; + goto out; } /* Flush everything out, we'll be doing GTT only from now on */ @@ -173,8 +174,6 @@ static int intelfb_alloc(struct drm_fb_helper *helper, out_fb: drm_framebuffer_remove(fb); -out_unref: - drm_gem_object_unreference(&obj->base); out: return ret; } -- cgit v1.2.3 From 51f1385b90c1ad30896bd62b1ff97aa4edb1a163 Mon Sep 17 00:00:00 2001 From: Tvrtko Ursulin Date: Tue, 30 Jun 2015 10:06:27 +0100 Subject: drm/i915: Fix failure paths around initial fbdev allocation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We had two failure modes here: 1. Deadlock in intelfb_alloc failure path where it calls drm_framebuffer_remove, which grabs the struct mutex and intelfb_create (caller of intelfb_alloc) was already holding it. 2. Deadlock in intelfb_create failure path where it calls drm_framebuffer_unreference, which grabs the struct mutex and intelfb_create was already holding it. [Daniel Vetter on why struct_mutex needs to be locked in the second half of intelfb_create: "The vma [for the fbdev] is pinned, the problem is that we re-lookup it a few times, which is racy. We should instead track the vma directly, but oh well we don't."] v2: * Reformat commit msg to 72 chars. (Lukas Wunner) * Add third failure mode. (Lukas Wunner) v5: * Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC, rephrase commit message. (Jani Nicula) v6: * In intelfb_alloc, if __intel_framebuffer_create failed, fb will be an ERR_PTR, thus not null. So in the failure path we need to check for IS_ERR_OR_NULL to avoid calling drm_framebuffer_remove on the ERR_PTR. (Lukas Wunner) * Since this is init code a drm_framebuffer_unreference should be all we need. drm_framebuffer_remove is for framebuffers that userspace has created - and is getting somewhat defeatured. (Daniel Vetter) v7: * Clarify why struct_mutex needs to be locked in the second half of intelfb_create. (Daniel Vetter) Fixes: 60a5ca015ffd ("drm/i915: Add locking around framebuffer_references--") Reported-by: Lukas Wunner Signed-off-by: Tvrtko Ursulin [Lukas: Create v3 + v4 + v5 + v6 + v7 based on Tvrtko's v2] Signed-off-by: Lukas Wunner Cc: Chris Wilson Cc: Ville Syrjälä Reviewed-by: Daniel Vetter Link: http://patchwork.freedesktop.org/patch/msgid/47d4e88c91b3bf0f7a280cabec54c8c8cf0cf6f2.1446892879.git.lukas@wunner.de Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) (limited to 'drivers/gpu/drm/i915/intel_fbdev.c') diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 2240e709ede6..db82ad45de13 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, { struct intel_fbdev *ifbdev = container_of(helper, struct intel_fbdev, helper); - struct drm_framebuffer *fb; + struct drm_framebuffer *fb = NULL; struct drm_device *dev = helper->dev; struct drm_i915_private *dev_priv = to_i915(dev); struct drm_mode_fb_cmd2 mode_cmd = {}; @@ -138,6 +138,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper, mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, sizes->surface_depth); + mutex_lock(&dev->struct_mutex); + size = mode_cmd.pitches[0] * mode_cmd.height; size = PAGE_ALIGN(size); @@ -165,16 +167,19 @@ static int intelfb_alloc(struct drm_fb_helper *helper, ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL); if (ret) { DRM_ERROR("failed to pin obj: %d\n", ret); - goto out_fb; + goto out; } + mutex_unlock(&dev->struct_mutex); + ifbdev->fb = to_intel_framebuffer(fb); return 0; -out_fb: - drm_framebuffer_remove(fb); out: + mutex_unlock(&dev->struct_mutex); + if (!IS_ERR_OR_NULL(fb)) + drm_framebuffer_unreference(fb); return ret; } @@ -192,8 +197,6 @@ static int intelfb_create(struct drm_fb_helper *helper, int size, ret; bool prealloc = false; - mutex_lock(&dev->struct_mutex); - if (intel_fb && (sizes->fb_width > intel_fb->base.width || sizes->fb_height > intel_fb->base.height)) { @@ -208,7 +211,7 @@ static int intelfb_create(struct drm_fb_helper *helper, DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n"); ret = intelfb_alloc(helper, sizes); if (ret) - goto out_unlock; + return ret; intel_fb = ifbdev->fb; } else { DRM_DEBUG_KMS("re-using BIOS fb\n"); @@ -220,6 +223,8 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; + mutex_lock(&dev->struct_mutex); + info = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { ret = PTR_ERR(info); @@ -281,7 +286,6 @@ out_destroy_fbi: out_unpin: i915_gem_object_ggtt_unpin(obj); drm_gem_object_unreference(&obj->base); -out_unlock: mutex_unlock(&dev->struct_mutex); return ret; } -- cgit v1.2.3 From e00bf69644ba01163209db7cf0942fb645f4daae Mon Sep 17 00:00:00 2001 From: Ville Syrjälä Date: Fri, 6 Nov 2015 15:08:33 +0200 Subject: drm/i915: Move the fbdev async_schedule() into intel_fbdev.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading the driver load/unload code leaves one confused as there's an async_schedule() in the load, but not async_synchronize_full() in sight. In fact it's hidden inside intel_fbdev.c. So let's move the async_schedule() into intel_fbdev.c as well so that it's next to the async_synchronize_full(), which should make the relationship easier to see. Plus this way we won't schedule a nop function call when fbdev is disabled. And we were passing a pointer to a static inline function to async_schedule(), which seems rather dubious to me. Signed-off-by: Ville Syrjälä Link: http://patchwork.freedesktop.org/patch/msgid/1446815313-9490-4-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/i915_dma.c | 3 +-- drivers/gpu/drm/i915/intel_drv.h | 4 ++-- drivers/gpu/drm/i915/intel_fbdev.c | 7 ++++++- 3 files changed, 9 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm/i915/intel_fbdev.c') diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 561ad8d537bf..37bab58f1edf 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -28,7 +28,6 @@ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt -#include #include #include #include @@ -469,7 +468,7 @@ static int i915_load_modeset_init(struct drm_device *dev) * scanning against hotplug events. Hence do this first and ignore the * tiny window where we will loose hotplug notifactions. */ - async_schedule(intel_fbdev_initial_config, dev_priv); + intel_fbdev_initial_config_async(dev); drm_kms_helper_poll_init(dev); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3f80816dea69..e3794d3fdb62 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1297,7 +1297,7 @@ void intel_dvo_init(struct drm_device *dev); /* legacy fbdev emulation in intel_fbdev.c */ #ifdef CONFIG_DRM_FBDEV_EMULATION extern int intel_fbdev_init(struct drm_device *dev); -extern void intel_fbdev_initial_config(void *data, async_cookie_t cookie); +extern void intel_fbdev_initial_config_async(struct drm_device *dev); extern void intel_fbdev_fini(struct drm_device *dev); extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, bool synchronous); extern void intel_fbdev_output_poll_changed(struct drm_device *dev); @@ -1308,7 +1308,7 @@ static inline int intel_fbdev_init(struct drm_device *dev) return 0; } -static inline void intel_fbdev_initial_config(void *data, async_cookie_t cookie) +static inline void intel_fbdev_initial_config_async(struct drm_device *dev) { } diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index db82ad45de13..98772d37b09f 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -705,7 +705,7 @@ int intel_fbdev_init(struct drm_device *dev) return 0; } -void intel_fbdev_initial_config(void *data, async_cookie_t cookie) +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; @@ -714,6 +714,11 @@ void intel_fbdev_initial_config(void *data, async_cookie_t cookie) drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp); } +void intel_fbdev_initial_config_async(struct drm_device *dev) +{ + async_schedule(intel_fbdev_initial_config, to_i915(dev)); +} + void intel_fbdev_fini(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; -- cgit v1.2.3 From 54632abe8ca3db8621673b186c7cc0e869c0032f Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 18 Nov 2015 13:43:20 +0100 Subject: drm/i915: Fix oops caused by fbdev initialization failure 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 ) Signed-off-by: Lukas Wunner Link: http://patchwork.freedesktop.org/patch/msgid/d05f0edf121264a9d0adb8ca713fd8cc4ae068bf.1447938059.git.lukas@wunner.de Signed-off-by: Daniel Vetter --- 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(-) (limited to 'drivers/gpu/drm/i915/intel_fbdev.c') diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index a9af884dadd5..08eb40292933 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1878,17 +1878,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); diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c index 8a604ac797aa..a12d1c7ee0e7 100644 --- a/drivers/gpu/drm/i915/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/intel_dp_mst.c @@ -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 } diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 98772d37b09f..cdbef32897f0 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -529,8 +529,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); + } } /* -- cgit v1.2.3 From 366e39b4d2c53b198b072f984caa0ebc4220ad4d Mon Sep 17 00:00:00 2001 From: Lukas Wunner Date: Wed, 18 Nov 2015 16:29:51 +0100 Subject: drm/i915: Tear down fbdev if initialization fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Lukas Wunner Link: http://patchwork.freedesktop.org/patch/msgid/49ce5f0daead24b7598ec78591731046c333c18d.1447938059.git.lukas@wunner.de Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_fbdev.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'drivers/gpu/drm/i915/intel_fbdev.c') diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index cdbef32897f0..7ccde58f8c98 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -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; } @@ -713,7 +714,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) @@ -729,7 +732,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; -- cgit v1.2.3