Hi,
On Thu, Nov 19, 2015 at 04:58:44PM +0100, Daniel Vetter wrote:
> 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?
That was my original suggestion but Ville didn't like it... :-)
Message-ID: <20151109110050.GW4437@xxxxxxxxx>
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
> -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
