On Wed, Nov 18, 2015 at 04:29:51PM +0100, Lukas Wunner wrote:
> 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 <daniel.vetter@xxxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 98772d3..cd345c5 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;
> }
> @@ -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();
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
> intel_fbdev_destroy(dev, dev_priv->fbdev);
> kfree(dev_priv->fbdev);
> dev_priv->fbdev = NULL;
> --
> 2.1.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
