From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7 Date: Thu, 12 Dec 2013 15:45:42 -0800 Message-ID: <20131212154542.42c71f34@jbarnes-desktop> References: <1386880917-2951-1-git-send-email-jbarnes@virtuousgeek.org> <1386880917-2951-3-git-send-email-jbarnes@virtuousgeek.org> <20131212225437.GT9804@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from alt-proxy16.mail.unifiedlayer.com (alt-proxy16.mail.unifiedlayer.com [70.40.197.35]) by gabe.freedesktop.org (Postfix) with SMTP id E3147FA407 for ; Thu, 12 Dec 2013 15:43:57 -0800 (PST) In-Reply-To: <20131212225437.GT9804@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 12 Dec 2013 23:54:37 +0100 Daniel Vetter 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 > > [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