Re: [PATCH 4/5] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon

Daniel Vetter
2013-11-15T00:28:56Z

On Thu, Nov 14, 2013 at 04:04:24PM -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)
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Just for the record I'm still freaked out who we're supposed to keep this
working. But since all the ideas I could come up are somewhere between
groos and "that's not going to work" I'm leaning towards ignoring it.

But doing a module reload with the displays still on (but only if we have
an fbdev framebuffer in stolen ofc) would still be fun ...
-Daniel

> ---
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_fbdev.c | 222 +++++++++++++++++++++++++++++++++++--
> 2 files changed, 216 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcc0b08..b9e7cff 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -113,6 +113,7 @@ struct intel_fbdev {
> struct intel_framebuffer ifb;
> struct list_head fbdev_list;
> struct drm_display_mode *our_mode;
> + int preferred_bpp;
> };
>
> struct intel_encoder {
> @@ -677,6 +678,7 @@ int intel_framebuffer_init(struct drm_device *dev,
> struct intel_framebuffer *ifb,
> struct drm_mode_fb_cmd2 *mode_cmd,
> struct drm_i915_gem_object *obj);
> +void intel_fbdev_init_bios(struct drm_device *dev);
> void intel_framebuffer_fini(struct intel_framebuffer *fb);
> void intel_prepare_page_flip(struct drm_device *dev, int plane);
> void intel_finish_page_flip(struct drm_device *dev, int pipe);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index fdb6dc9..5ee5dc0 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -127,7 +127,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> mutex_lock(&dev->struct_mutex);
>
> if (!intel_fb->obj) {
> - DRM_ERROR("no BIOS fb, allocating a new one\n");
> + DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
> ret = intelfb_alloc(helper, sizes);
> if (ret)
> goto out_unlock;
> @@ -235,6 +235,96 @@ static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
> *blue = intel_crtc->lut_b[regno] << 8;
> }
>
> +static struct drm_fb_helper_crtc *
> +intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
> +{
> + int i;
> +
> + for (i = 0; i < fb_helper->crtc_count; i++)
> + if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
> + return &fb_helper->crtc_info[i];
> +
> + return NULL;
> +}
> +
> +/*
> + * Try to read the BIOS display configuration and use it for the initial
> + * fb configuration.
> + *
> + * The BIOS or boot loader will generally create an initial display
> + * configuration for us that includes some set of active pipes and displays.
> + * This routine tries to figure out which pipes and connectors are active
> + * and stuffs them into the crtcs and modes array given to us by the
> + * drm_fb_helper code.
> + *
> + * The overall sequence is:
> + * intel_fbdev_init - from driver load
> + * intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data
> + * drm_fb_helper_init - build fb helper structs
> + * drm_fb_helper_single_add_all_connectors - more fb helper structs
> + * intel_fbdev_initial_config - apply the config
> + * drm_fb_helper_initial_config - call ->probe then register_framebuffer()
> + * drm_setup_crtcs - build crtc config for fbdev
> + * intel_fb_initial_config - find active connectors etc
> + * drm_fb_helper_single_fb_probe - set up fbdev
> + * intelfb_create - re-use or alloc fb, build out fbdev structs
> + *
> + * If the BIOS or boot loader leaves the display in VGA mode, there's not
> + * much we can do; switching out of that mode involves allocating a new,
> + * high res buffer, and also recalculating bandwidth requirements for the
> + * new bpp configuration.
> + */
> +static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
> + struct drm_fb_helper_crtc **crtcs,
> + struct drm_display_mode **modes,
> + bool *enabled, int width, int height)
> +{
> + int i;
> +
> + for (i = 0; i < fb_helper->connector_count; i++) {
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> +
> + connector = fb_helper->connector_info[i]->connector;
> + if (!enabled[i]) {
> + DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
> + connector->base.id);
> + continue;
> + }
> +
> + encoder = connector->encoder;
> + if (!encoder || !encoder->crtc) {
> + DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
> + connector->base.id);
> + continue;
> + }
> +
> + if (WARN_ON(!encoder->crtc->enabled)) {
> + DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent state, aborting\n",
> + drm_get_connector_name(connector),
> + encoder->crtc->base.id);
> + return false;
> + }
> +
> + if (!to_intel_crtc(encoder->crtc)->active) {
> + DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n",
> + drm_get_connector_name(connector),
> + encoder->crtc->base.id);
> + return false;
> + }
> +
> + modes[i] = &encoder->crtc->mode;
> + crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
> +
> + DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
> + drm_get_connector_name(connector),
> + encoder->crtc->base.id,
> + modes[i]->name);
> + }
> +
> + return true;
> +}
> +
> static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
> .gamma_set = intel_crtc_fb_gamma_set,
> .gamma_get = intel_crtc_fb_gamma_get,
> @@ -261,23 +351,137 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> intel_framebuffer_fini(&ifbdev->ifb);
> }
>
> +/*
> + * 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 drm_crtc *crtc;
> + struct intel_crtc *intel_crtc;
> + struct intel_plane_config *plane_config;
> + struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> + struct drm_i915_gem_object *obj = 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.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;
> + obj = plane_config->obj;
> + }
> + }
> +
> + /* Free unused fbs */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + struct drm_i915_gem_object *cur_obj;
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cur_obj = intel_crtc->plane_config.obj;
> +
> + if (cur_obj != obj)
> + drm_gem_object_unreference_unlocked(&cur_obj->base);
> + }
> +
> + if (!obj) {
> + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> + goto out_free;
> + }
> +
> + ifbdev->preferred_bpp = plane_config->bpp;
> + ifbdev->helper.funcs = &intel_fb_helper_funcs;
> + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> +
> + mode_cmd.pixel_format =
> + intel_format_to_fourcc(plane_config->pixel_format);
> + mode_cmd.width = plane_config->fb_width;
> + mode_cmd.height = plane_config->fb_height;
> + mode_cmd.pitches[0] = plane_config->pitch;
> +
> + mutex_lock(&dev->struct_mutex);
> +
> + if (intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj)) {
> + DRM_DEBUG_KMS("intel fb init failed\n");
> + goto out_unref_obj;
> + }
> +
> + /* 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, obj, NULL))
> + goto out_unref_obj;
> +
> + crtc->fb = &ifbdev->ifb.base;
> + }
> +
> + dev_priv->fbdev = ifbdev;
> +
> + DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> + mutex_unlock(&dev->struct_mutex);
> + return;
> +
> +out_unref_obj:
> + drm_gem_object_unreference(&obj->base);
> + mutex_unlock(&dev->struct_mutex);
> +out_free:
> + kfree(ifbdev);
> +}
> +
> int intel_fbdev_init(struct drm_device *dev)
> {
> struct intel_fbdev *ifbdev;
> 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 */
> + 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;
> }
> @@ -290,9 +494,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)
> @@ -332,7 +537,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);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> --
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch