On Thu, 12 Dec 2013 23:54:37 +0100
Daniel Vetter <dan...@ffwll.ch> wrote:
> On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote:
> > Retrieve current framebuffer config info from the regs and create an fb
> > object for the buffer the BIOS or boot loader left us. This should
> > allow for smooth transitions to userspace apps once we finish the
> > initial configuration construction.
> >
> > v2: check for non-native modes and adjust (Jesse)
> > fixup aperture and cmap frees (Imre)
> > use unlocked unref if init_bios fails (Jesse)
> > fix curly brace around DSPADDR check (Imre)
> > comment failure path for pin_and_fence (Imre)
> > v3: fixup fixup of aperture frees (Chris)
> > v4: update to current bits (locking & pin_and_fence hack) (Jesse)
> > v5: move fb config fetch to display code (Jesse)
> > re-order hw state readout on initial load to suit fb inherit (Jesse)
> > re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
> > v6: rename to plane_config (Daniel)
> > check for valid object when initializing BIOS fb (Jesse)
> > split from plane_config readout and other display changes (Jesse)
> > drop use_bios_fb option (Chris)
> > update comments (Jesse)
> > rework fbdev_init_bios for clarity (Jesse)
> > drop fb obj ref under lock (Chris)
> > v7: use fb object from plane_config instead (Ville)
> > take ref on fb object (Jesse)
> >
> > Signed-off-by: Jesse Barnes <jbar...@virtuousgeek.org>
>
> [snip]
>
> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device
> > *dev,
> >
> > drm_fb_helper_fini(&ifbdev->helper);
> >
> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base);
> > - intel_framebuffer_fini(&ifbdev->ifb);
> > + drm_framebuffer_unregister_private(&ifbdev->fb->base);
> > + intel_framebuffer_fini(ifbdev->fb);
> > + kfree(ifbdev->fb);
>
> No need to go the private fb route here anymore since now the fb is
> free-standing. Normal refcounting should work. But a separate prep/cleanup
> patch (prep since switching ifbdev->fb from struct to point would look
> neat as a separate patch).
>
> > +}
> > +
> > +/*
> > + * Build an intel_fbdev struct using a BIOS allocated framebuffer, if
> > possible.
> > + * The core display code will have read out the current plane
> > configuration,
> > + * so we use that to figure out if there's an object for us to use as the
> > + * fb, and if so, we re-use it for the fbdev configuration.
> > + *
> > + * Note we only support a single fb shared across pipes for boot (mostly
> > for
> > + * fbcon), so we just find the biggest and use that.
> > + */
> > +void intel_fbdev_init_bios(struct drm_device *dev)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_fbdev *ifbdev;
> > + struct intel_framebuffer *fb = NULL;
> > + struct drm_crtc *crtc;
> > + struct intel_crtc *intel_crtc;
> > + struct intel_plane_config *plane_config = NULL;
> > + unsigned int last_size = 0;
> > +
> > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> > + if (ifbdev == NULL) {
> > + DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> > + return;
> > + }
> > +
> > + /* Find the largest framebuffer to use, then free the others */
> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > + intel_crtc = to_intel_crtc(crtc);
> > +
> > + if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) {
> > + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> > + pipe_name(intel_crtc->pipe));
> > + continue;
> > + }
> > +
> > + if (intel_crtc->plane_config.size > last_size) {
> > + plane_config = &intel_crtc->plane_config;
> > + last_size = plane_config->size;
> > + fb = plane_config->fb;
> > + }
> > + }
> > +
> > + /* Free unused fbs */
> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > + struct intel_framebuffer *cur_fb;
> > +
> > + intel_crtc = to_intel_crtc(crtc);
> > + cur_fb = intel_crtc->plane_config.fb;
> > +
> > + if (cur_fb && cur_fb != fb)
> > + intel_framebuffer_fini(cur_fb);
> > + }
> > +
> > + if (!fb) {
> > + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> > + goto out_free;
> > + }
> > +
> > + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
> > + ifbdev->helper.funcs = &intel_fb_helper_funcs;
> > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> > + ifbdev->fb = fb;
> > +
> > + /* Assuming a single fb across all pipes here */
> > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > + intel_crtc = to_intel_crtc(crtc);
> > +
> > + if (!intel_crtc->active)
> > + continue;
> > +
> > + /*
> > + * This should only fail on the first one so we don't need
> > + * to cleanup any secondary crtc->fbs
> > + */
> > + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
> > + goto out_unref_obj;
> > +
> > + crtc->fb = &fb->base;
> > + drm_gem_object_reference(&fb->obj->base);
> > + drm_framebuffer_reference(&fb->base);
> > + }
> > +
> > + dev_priv->fbdev = ifbdev;
> > +
> > + DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> > + return;
> > +
> > +out_unref_obj:
> > + intel_framebuffer_fini(fb);
> > +out_free:
> > + kfree(ifbdev);
> > }
> >
> > int intel_fbdev_init(struct drm_device *dev)
> > @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > int ret;
> >
> > - ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> > - if (!ifbdev)
> > - return -ENOMEM;
> > + /* This may fail, if so, dev_priv->fbdev won't be set below */
>
> If you need a comment to explain your control flow, it's probably too
> clever ;-)
I could add a return value I suppose, but I didn't think it was too
clever...
>
> > + intel_fbdev_init_bios(dev);
> >
> > - dev_priv->fbdev = ifbdev;
> > - ifbdev->helper.funcs = &intel_fb_helper_funcs;
> > + if ((ifbdev = dev_priv->fbdev) == NULL) {
> > + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> > + if (ifbdev == NULL)
> > + return -ENOMEM;
> > +
> > + ifbdev->helper.funcs = &intel_fb_helper_funcs;
> > + ifbdev->preferred_bpp = 32;
> > +
> > + dev_priv->fbdev = ifbdev;
> > + }
> >
> > ret = drm_fb_helper_init(dev, &ifbdev->helper,
> > INTEL_INFO(dev)->num_pipes,
> > 4);
> > if (ret) {
> > + dev_priv->fbdev = NULL;
> > kfree(ifbdev);
> > return ret;
> > }
> > @@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev)
> > void intel_fbdev_initial_config(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_fbdev *ifbdev = dev_priv->fbdev;
> >
> > /* Due to peculiar init order wrt to hpd handling this is separate. */
> > - drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
> > + drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
> > }
> >
> > void intel_fbdev_fini(struct drm_device *dev)
> > @@ -322,7 +524,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev,
> > int state)
> > * been restored from swap. If the object is stolen however, it will be
> > * full of whatever garbage was left in there.
> > */
> > - if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
> > + if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
> > memset_io(info->screen_base, 0, info->screen_size);
> >
> > fb_set_suspend(info, state);
> > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
> > void intel_fbdev_output_poll_changed(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> > + if (dev_priv->fbdev)
> > + drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>
> Ok, here's the real reason I've actually replied to this patch. This looks
> like a separate bugfix. And there's not mention of it in the commit
> message. Please split it out and give it the nice commit message
> explanation it deserves (whatever it's doing here).
Oh this hunk may be spurious... I think it hit this case when we were
failing to init dev_priv->fbdev and got an early hotplug event. But
that should no longer be the case.
--
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
