[PATCH 1/5] drm/i915: make pitch_for_width take a tiled arg v2

[PATCH 1/5] drm/i915: make pitch_for_width take a tiled arg v2

[Jesse Barnes]

And move it up in the file for earlier usage.

v2: add pre-gen4 support as well (Chris)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b7f1c4..4cab78d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5452,6 +5452,28 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
 	pipe_config->port_clock = clock.dot / 5;
 }
 
+static u32
+intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
+				  int bpp, bool tiled)
+{
+	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
+	int align;
+
+	if (tiled) {
+		if (INTEL_INFO(dev_priv->dev)->gen < 4) {
+			/* Pre-965 needs power of two tile width */
+			for (align = 512; align < pitch; align <<= 1)
+				;
+		} else {
+			align = 512;
+		}
+	} else {
+		align = 64;
+	}
+
+	return ALIGN(pitch, align);
+}
+
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 				 struct intel_crtc_config *pipe_config)
 {
@@ -7629,16 +7651,12 @@ err:
 }
 
 static u32
-intel_framebuffer_pitch_for_width(int width, int bpp)
-{
-	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
-	return ALIGN(pitch, 64);
-}
-
-static u32
-intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
+intel_framebuffer_size_for_mode(struct drm_i915_private *dev_priv,
+				struct drm_display_mode *mode, int bpp)
 {
-	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
+	u32 pitch = intel_framebuffer_pitch_for_width(dev_priv,
+						      mode->hdisplay, bpp,
+						      false);
 	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);
 }
 
@@ -7647,18 +7665,21 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 				  struct drm_display_mode *mode,
 				  int depth, int bpp)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+	int size;
 
-	obj = i915_gem_alloc_object(dev,
-				    intel_framebuffer_size_for_mode(mode, bpp));
+	size =  intel_framebuffer_size_for_mode(dev_priv, mode, bpp);
+	obj = i915_gem_alloc_object(dev, size);
 	if (obj == NULL)
 		return ERR_PTR(-ENOMEM);
 
 	mode_cmd.width = mode->hdisplay;
 	mode_cmd.height = mode->vdisplay;
-	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
-								bpp);
+	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(dev_priv,
+								mode_cmd.width,
+								bpp, false);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
 	return intel_framebuffer_create(dev, &mode_cmd, obj);
@@ -7681,8 +7702,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
 		return NULL;
 
 	fb = &dev_priv->fbdev->ifb.base;
-	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
-							       fb->bits_per_pixel))
+	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(dev_priv,
+							       mode->hdisplay,
+							       fb->bits_per_pixel,
+							       false))
 		return NULL;
 
 	if (obj->base.size < mode->vdisplay * fb->pitches[0])
-- 
1.8.4.2

[PATCH 2/5] drm/i915: split fb allocation and initialization v2

[Jesse Barnes]

If we use a stolen buffer, our probe callback shouldn't allocate a new
buffer; we should re-use the one from the BIOS instead if possible.

v2: fix locking (Jesse)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 62 ++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 895fcb4..fdb6dc9 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -57,18 +57,14 @@ static struct fb_ops intelfb_ops = {
 	.fb_debug_leave = drm_fb_helper_debug_leave,
 };
 
-static int intelfb_create(struct drm_fb_helper *helper,
-			  struct drm_fb_helper_surface_size *sizes)
+static int intelfb_alloc(struct drm_fb_helper *helper,
+			 struct drm_fb_helper_surface_size *sizes)
 {
 	struct intel_fbdev *ifbdev =
 		container_of(helper, struct intel_fbdev, helper);
 	struct drm_device *dev = helper->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct fb_info *info;
-	struct drm_framebuffer *fb;
 	struct drm_mode_fb_cmd2 mode_cmd = {};
 	struct drm_i915_gem_object *obj;
-	struct device *device = &dev->pdev->dev;
 	int size, ret;
 
 	/* we don't do packed 24bpp */
@@ -94,8 +90,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		goto out;
 	}
 
-	mutex_lock(&dev->struct_mutex);
-
 	/* Flush everything out, we'll be doing GTT only from now on */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	if (ret) {
@@ -103,7 +97,49 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		goto out_unref;
 	}
 
-	info = framebuffer_alloc(0, device);
+	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
+	if (ret)
+		goto out_unpin;
+
+	return 0;
+
+out_unpin:
+	i915_gem_object_unpin(obj);
+out_unref:
+	drm_gem_object_unreference(&obj->base);
+out:
+	return ret;
+}
+
+static int intelfb_create(struct drm_fb_helper *helper,
+			  struct drm_fb_helper_surface_size *sizes)
+{
+	struct intel_fbdev *ifbdev =
+		container_of(helper, struct intel_fbdev, helper);
+	struct intel_framebuffer *intel_fb = &ifbdev->ifb;
+	struct drm_device *dev = helper->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct fb_info *info;
+	struct drm_framebuffer *fb;
+	struct drm_i915_gem_object *obj;
+	int size, ret;
+
+	mutex_lock(&dev->struct_mutex);
+
+	if (!intel_fb->obj) {
+		DRM_ERROR("no BIOS fb, allocating a new one\n");
+		ret = intelfb_alloc(helper, sizes);
+		if (ret)
+			goto out_unlock;
+	} else {
+		sizes->fb_width = intel_fb->base.width;
+		sizes->fb_height = intel_fb->base.height;
+	}
+
+	obj = intel_fb->obj;
+	size = obj->base.size;
+
+	info = framebuffer_alloc(0, &dev->pdev->dev);
 	if (!info) {
 		ret = -ENOMEM;
 		goto out_unpin;
@@ -111,10 +147,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info->par = helper;
 
-	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
-	if (ret)
-		goto out_unpin;
-
 	fb = &ifbdev->ifb.base;
 
 	ifbdev->helper.fb = fb;
@@ -170,17 +202,15 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		      fb->width, fb->height,
 		      i915_gem_obj_ggtt_offset(obj), obj);
 
-
 	mutex_unlock(&dev->struct_mutex);
 	vga_switcheroo_client_fb_set(dev->pdev, info);
 	return 0;
 
 out_unpin:
 	i915_gem_object_unpin(obj);
-out_unref:
 	drm_gem_object_unreference(&obj->base);
+out_unlock:
 	mutex_unlock(&dev->struct_mutex);
-out:
 	return ret;
 }
 
-- 
1.8.4.2

[PATCH 3/5] drm/i915: retrieve current fb config into new plane_config structure at init v2

[Jesse Barnes]

Read out the current plane configuration at init time into a new
plane_config structure.  This allows us to track any existing
framebuffers attached to the plane and potentially re-use them in our
fbdev code for a smooth handoff.

v2: update for new pitch_for_width function (Jesse)
    comment how get_plane_config works with shared fbs (Jesse)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_display.c | 118 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  14 +++++
 3 files changed, 133 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6b96e91..aac58ec 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -365,6 +365,7 @@ struct drm_i915_error_state {
 
 struct intel_connector;
 struct intel_crtc_config;
+struct intel_plane_config;
 struct intel_crtc;
 struct intel_limit;
 struct dpll;
@@ -403,6 +404,8 @@ struct drm_i915_display_funcs {
 	 * fills out the pipe-config with the hw state. */
 	bool (*get_pipe_config)(struct intel_crtc *,
 				struct intel_crtc_config *);
+	void (*get_plane_config)(struct intel_crtc *,
+				 struct intel_plane_config *);
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4cab78d..81200c4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2002,6 +2002,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 	}
 }
 
+int intel_format_to_fourcc(int format)
+{
+	switch (format) {
+	case DISPPLANE_8BPP:
+		return DRM_FORMAT_C8;
+	case DISPPLANE_BGRX555:
+		return DRM_FORMAT_ARGB1555;
+	case DISPPLANE_BGRX565:
+		return DRM_FORMAT_RGB565;
+	default:
+	case DISPPLANE_BGRX888:
+		return DRM_FORMAT_XRGB8888;
+	case DISPPLANE_RGBX888:
+		return DRM_FORMAT_XBGR8888;
+	case DISPPLANE_BGRX101010:
+		return DRM_FORMAT_XRGB2101010;
+	case DISPPLANE_RGBX101010:
+		return DRM_FORMAT_XBGR2101010;
+	}
+}
+
 static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 			     int x, int y)
 {
@@ -5474,6 +5495,86 @@ intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
 	return ALIGN(pitch, align);
 }
 
+static void i9xx_get_plane_config(struct intel_crtc *crtc,
+				  struct intel_plane_config *plane_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = crtc->pipe, plane = crtc->plane;
+	u32 val;
+
+	val = I915_READ(DSPCNTR(plane));
+
+	if (INTEL_INFO(dev)->gen >= 4)
+		if (val & DISPPLANE_TILED)
+			plane_config->tiled = true;
+
+	plane_config->pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
+
+	switch (plane_config->pixel_format) {
+	case DISPPLANE_8BPP:
+	case DISPPLANE_YUV422:
+		plane_config->bpp = 8;
+		break;
+	case DISPPLANE_BGRX555:
+	case DISPPLANE_BGRX565:
+	case DISPPLANE_BGRA555:
+		plane_config->bpp = 16;
+		break;
+	case DISPPLANE_BGRX888:
+	case DISPPLANE_BGRA888:
+	case DISPPLANE_RGBX888:
+	case DISPPLANE_RGBA888:
+	case DISPPLANE_RGBX101010:
+	case DISPPLANE_RGBA101010:
+	case DISPPLANE_BGRX101010:
+		plane_config->bpp = 32;
+		break;
+	}
+
+	if (INTEL_INFO(dev)->gen >= 4) {
+		if (plane_config->tiled)
+			plane_config->offset = I915_READ(DSPTILEOFF(plane));
+		else
+			plane_config->offset = I915_READ(DSPLINOFF(plane));
+		plane_config->base = I915_READ(DSPSURF(plane)) & 0xfffff000;
+	} else {
+		plane_config->base = I915_READ(DSPADDR(plane));
+	}
+
+	val = I915_READ(PIPESRC(pipe));
+	plane_config->pipe_width = ((val >> 16) & 0xfff) + 1;
+	plane_config->pipe_height = ((val >> 0) & 0xfff) + 1;
+
+	val = I915_READ(HTOTAL(pipe));
+	plane_config->fb_width = (val & 0xffff) + 1;
+	val = I915_READ(VTOTAL(pipe));
+	plane_config->fb_height = (val & 0xffff) + 1;
+
+	DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x\n",
+		      pipe, plane, plane_config->fb_width,
+		      plane_config->fb_height, plane_config->bpp,
+		      plane_config->base);
+
+	plane_config->pitch =
+		intel_framebuffer_pitch_for_width(dev_priv,
+						  plane_config->fb_width,
+						  plane_config->bpp,
+						  plane_config->tiled);
+
+	plane_config->size = ALIGN(plane_config->pitch *
+				   plane_config->fb_height, PAGE_SIZE);
+	/*
+	 * If the fb is shared between multiple heads, we'll just get the
+	 * first one.
+	 */
+	plane_config->obj =
+		i915_gem_object_create_stolen_for_preallocated(dev,
+							       plane_config->base,
+							       plane_config->base,
+							       plane_config->size);
+}
+
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 				 struct intel_crtc_config *pipe_config)
 {
@@ -10540,6 +10641,7 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.update_plane = ironlake_update_plane;
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
+		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
@@ -10547,6 +10649,7 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.update_plane = i9xx_update_plane;
 	} else {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
+		dev_priv->display.get_plane_config = i9xx_get_plane_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
@@ -10801,6 +10904,7 @@ void intel_modeset_suspend_hw(struct drm_device *dev)
 void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *crtc;
 	int i, j, ret;
 
 	drm_mode_config_init(dev);
@@ -10857,6 +10961,18 @@ void intel_modeset_init(struct drm_device *dev)
 
 	/* Just in case the BIOS is doing something questionable. */
 	intel_disable_fbc(dev);
+
+	intel_modeset_setup_hw_state(dev, false);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
+			    base.head) {
+		if (!crtc->active)
+			continue;
+
+		if (dev_priv->display.get_plane_config)
+			dev_priv->display.get_plane_config(crtc,
+							   &crtc->plane_config);
+	}
 }
 
 static void
@@ -11227,8 +11343,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	intel_modeset_init_hw(dev);
 
 	intel_setup_overlay(dev);
-
-	intel_modeset_setup_hw_state(dev, false);
 }
 
 void intel_modeset_cleanup(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0231281..bcc0b08 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -209,6 +209,18 @@ typedef struct dpll {
 	int	p;
 } intel_clock_t;
 
+struct intel_plane_config {
+	int pixel_format; /* DRM fourcc code */
+	int bpp;
+	bool tiled;
+	int base, offset;
+	int fb_width, fb_height;
+	int pipe_width, pipe_height;
+	int pitch;
+	int size;
+	struct drm_i915_gem_object *obj;
+};
+
 struct intel_crtc_config {
 	/**
 	 * quirks - bitfield with hw state readout quirks
@@ -358,6 +370,7 @@ struct intel_crtc {
 	bool cursor_visible;
 
 	struct intel_crtc_config config;
+	struct intel_plane_config plane_config;
 
 	uint32_t ddi_pll_sel;
 
@@ -707,6 +720,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
 void intel_display_set_init_power(struct drm_device *dev, bool enable);
 int valleyview_get_vco(struct drm_i915_private *dev_priv);
+int intel_format_to_fourcc(int format);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.8.4.2

Re: [PATCH 3/5] drm/i915: retrieve current fb config into new plane_config structure at init v2

[Daniel Vetter]

On Thu, Nov 14, 2013 at 04:04:23PM -0800, Jesse Barnes wrote:
> Read out the current plane configuration at init time into a new
> plane_config structure.  This allows us to track any existing
> framebuffers attached to the plane and potentially re-use them in our
> fbdev code for a smooth handoff.
> 
> v2: update for new pitch_for_width function (Jesse)
>     comment how get_plane_config works with shared fbs (Jesse)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

To me sturct intel_plane_config still looks like a copy of an
intel_framebuffer ... Imo tracking a stand-alone config struct is only
really worth it if we use it for e.g. cross checks. So not sold.

Or is there an ugly init-time depency that I'm missing?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   3 +
>  drivers/gpu/drm/i915/intel_display.c | 118 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  14 +++++
>  3 files changed, 133 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6b96e91..aac58ec 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -365,6 +365,7 @@ struct drm_i915_error_state {
>  
>  struct intel_connector;
>  struct intel_crtc_config;
> +struct intel_plane_config;
>  struct intel_crtc;
>  struct intel_limit;
>  struct dpll;
> @@ -403,6 +404,8 @@ struct drm_i915_display_funcs {
>  	 * fills out the pipe-config with the hw state. */
>  	bool (*get_pipe_config)(struct intel_crtc *,
>  				struct intel_crtc_config *);
> +	void (*get_plane_config)(struct intel_crtc *,
> +				 struct intel_plane_config *);
>  	int (*crtc_mode_set)(struct drm_crtc *crtc,
>  			     int x, int y,
>  			     struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4cab78d..81200c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2002,6 +2002,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
>  	}
>  }
>  
> +int intel_format_to_fourcc(int format)
> +{
> +	switch (format) {
> +	case DISPPLANE_8BPP:
> +		return DRM_FORMAT_C8;
> +	case DISPPLANE_BGRX555:
> +		return DRM_FORMAT_ARGB1555;
> +	case DISPPLANE_BGRX565:
> +		return DRM_FORMAT_RGB565;
> +	default:
> +	case DISPPLANE_BGRX888:
> +		return DRM_FORMAT_XRGB8888;
> +	case DISPPLANE_RGBX888:
> +		return DRM_FORMAT_XBGR8888;
> +	case DISPPLANE_BGRX101010:
> +		return DRM_FORMAT_XRGB2101010;
> +	case DISPPLANE_RGBX101010:
> +		return DRM_FORMAT_XBGR2101010;
> +	}
> +}
> +
>  static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  			     int x, int y)
>  {
> @@ -5474,6 +5495,86 @@ intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
>  	return ALIGN(pitch, align);
>  }
>  
> +static void i9xx_get_plane_config(struct intel_crtc *crtc,
> +				  struct intel_plane_config *plane_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int pipe = crtc->pipe, plane = crtc->plane;
> +	u32 val;
> +
> +	val = I915_READ(DSPCNTR(plane));
> +
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		if (val & DISPPLANE_TILED)
> +			plane_config->tiled = true;
> +
> +	plane_config->pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> +
> +	switch (plane_config->pixel_format) {
> +	case DISPPLANE_8BPP:
> +	case DISPPLANE_YUV422:
> +		plane_config->bpp = 8;
> +		break;
> +	case DISPPLANE_BGRX555:
> +	case DISPPLANE_BGRX565:
> +	case DISPPLANE_BGRA555:
> +		plane_config->bpp = 16;
> +		break;
> +	case DISPPLANE_BGRX888:
> +	case DISPPLANE_BGRA888:
> +	case DISPPLANE_RGBX888:
> +	case DISPPLANE_RGBA888:
> +	case DISPPLANE_RGBX101010:
> +	case DISPPLANE_RGBA101010:
> +	case DISPPLANE_BGRX101010:
> +		plane_config->bpp = 32;
> +		break;
> +	}
> +
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		if (plane_config->tiled)
> +			plane_config->offset = I915_READ(DSPTILEOFF(plane));
> +		else
> +			plane_config->offset = I915_READ(DSPLINOFF(plane));
> +		plane_config->base = I915_READ(DSPSURF(plane)) & 0xfffff000;
> +	} else {
> +		plane_config->base = I915_READ(DSPADDR(plane));
> +	}
> +
> +	val = I915_READ(PIPESRC(pipe));
> +	plane_config->pipe_width = ((val >> 16) & 0xfff) + 1;
> +	plane_config->pipe_height = ((val >> 0) & 0xfff) + 1;
> +
> +	val = I915_READ(HTOTAL(pipe));
> +	plane_config->fb_width = (val & 0xffff) + 1;
> +	val = I915_READ(VTOTAL(pipe));
> +	plane_config->fb_height = (val & 0xffff) + 1;
> +
> +	DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x\n",
> +		      pipe, plane, plane_config->fb_width,
> +		      plane_config->fb_height, plane_config->bpp,
> +		      plane_config->base);
> +
> +	plane_config->pitch =
> +		intel_framebuffer_pitch_for_width(dev_priv,
> +						  plane_config->fb_width,
> +						  plane_config->bpp,
> +						  plane_config->tiled);
> +
> +	plane_config->size = ALIGN(plane_config->pitch *
> +				   plane_config->fb_height, PAGE_SIZE);
> +	/*
> +	 * If the fb is shared between multiple heads, we'll just get the
> +	 * first one.
> +	 */
> +	plane_config->obj =
> +		i915_gem_object_create_stolen_for_preallocated(dev,
> +							       plane_config->base,
> +							       plane_config->base,
> +							       plane_config->size);
> +}
> +
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  				 struct intel_crtc_config *pipe_config)
>  {
> @@ -10540,6 +10641,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> +		dev_priv->display.get_plane_config = i9xx_get_plane_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = valleyview_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -10547,6 +10649,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = i9xx_update_plane;
>  	} else {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> +		dev_priv->display.get_plane_config = i9xx_get_plane_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = i9xx_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -10801,6 +10904,7 @@ void intel_modeset_suspend_hw(struct drm_device *dev)
>  void intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc;
>  	int i, j, ret;
>  
>  	drm_mode_config_init(dev);
> @@ -10857,6 +10961,18 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  	/* Just in case the BIOS is doing something questionable. */
>  	intel_disable_fbc(dev);
> +
> +	intel_modeset_setup_hw_state(dev, false);
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> +			    base.head) {
> +		if (!crtc->active)
> +			continue;
> +
> +		if (dev_priv->display.get_plane_config)
> +			dev_priv->display.get_plane_config(crtc,
> +							   &crtc->plane_config);
> +	}
>  }
>  
>  static void
> @@ -11227,8 +11343,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	intel_modeset_init_hw(dev);
>  
>  	intel_setup_overlay(dev);
> -
> -	intel_modeset_setup_hw_state(dev, false);
>  }
>  
>  void intel_modeset_cleanup(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0231281..bcc0b08 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -209,6 +209,18 @@ typedef struct dpll {
>  	int	p;
>  } intel_clock_t;
>  
> +struct intel_plane_config {
> +	int pixel_format; /* DRM fourcc code */
> +	int bpp;
> +	bool tiled;
> +	int base, offset;
> +	int fb_width, fb_height;
> +	int pipe_width, pipe_height;
> +	int pitch;
> +	int size;
> +	struct drm_i915_gem_object *obj;
> +};
> +
>  struct intel_crtc_config {
>  	/**
>  	 * quirks - bitfield with hw state readout quirks
> @@ -358,6 +370,7 @@ struct intel_crtc {
>  	bool cursor_visible;
>  
>  	struct intel_crtc_config config;
> +	struct intel_plane_config plane_config;
>  
>  	uint32_t ddi_pll_sel;
>  
> @@ -707,6 +720,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
>  void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
> +int intel_format_to_fourcc(int format);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> -- 
> 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

Re: [PATCH 3/5] drm/i915: retrieve current fb config into new plane_config structure at init v2

[Bob Paauwe]

On Thu, 14 Nov 2013 16:04:23 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Read out the current plane configuration at init time into a new
> plane_config structure.  This allows us to track any existing
> framebuffers attached to the plane and potentially re-use them in our
> fbdev code for a smooth handoff.
> 
> v2: update for new pitch_for_width function (Jesse)
>     comment how get_plane_config works with shared fbs (Jesse)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   3 +
>  drivers/gpu/drm/i915/intel_display.c | 118 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  14 +++++
>  3 files changed, 133 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6b96e91..aac58ec 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -365,6 +365,7 @@ struct drm_i915_error_state {
>  
>  struct intel_connector;
>  struct intel_crtc_config;
> +struct intel_plane_config;
>  struct intel_crtc;
>  struct intel_limit;
>  struct dpll;
> @@ -403,6 +404,8 @@ struct drm_i915_display_funcs {
>  	 * fills out the pipe-config with the hw state. */
>  	bool (*get_pipe_config)(struct intel_crtc *,
>  				struct intel_crtc_config *);
> +	void (*get_plane_config)(struct intel_crtc *,
> +				 struct intel_plane_config *);
>  	int (*crtc_mode_set)(struct drm_crtc *crtc,
>  			     int x, int y,
>  			     struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4cab78d..81200c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2002,6 +2002,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
>  	}
>  }
>  
> +int intel_format_to_fourcc(int format)
> +{
> +	switch (format) {
> +	case DISPPLANE_8BPP:
> +		return DRM_FORMAT_C8;
> +	case DISPPLANE_BGRX555:
> +		return DRM_FORMAT_ARGB1555;
> +	case DISPPLANE_BGRX565:
> +		return DRM_FORMAT_RGB565;
> +	default:
> +	case DISPPLANE_BGRX888:
> +		return DRM_FORMAT_XRGB8888;
> +	case DISPPLANE_RGBX888:
> +		return DRM_FORMAT_XBGR8888;
> +	case DISPPLANE_BGRX101010:
> +		return DRM_FORMAT_XRGB2101010;
> +	case DISPPLANE_RGBX101010:
> +		return DRM_FORMAT_XBGR2101010;
> +	}
> +}
> +
>  static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  			     int x, int y)
>  {
> @@ -5474,6 +5495,86 @@ intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
>  	return ALIGN(pitch, align);
>  }
>  
> +static void i9xx_get_plane_config(struct intel_crtc *crtc,
> +				  struct intel_plane_config *plane_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int pipe = crtc->pipe, plane = crtc->plane;
> +	u32 val;
> +
> +	val = I915_READ(DSPCNTR(plane));
> +
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		if (val & DISPPLANE_TILED)
> +			plane_config->tiled = true;
> +
> +	plane_config->pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> +
> +	switch (plane_config->pixel_format) {
> +	case DISPPLANE_8BPP:
> +	case DISPPLANE_YUV422:
> +		plane_config->bpp = 8;
> +		break;
> +	case DISPPLANE_BGRX555:
> +	case DISPPLANE_BGRX565:
> +	case DISPPLANE_BGRA555:
> +		plane_config->bpp = 16;
> +		break;
> +	case DISPPLANE_BGRX888:
> +	case DISPPLANE_BGRA888:
> +	case DISPPLANE_RGBX888:
> +	case DISPPLANE_RGBA888:
> +	case DISPPLANE_RGBX101010:
> +	case DISPPLANE_RGBA101010:
> +	case DISPPLANE_BGRX101010:
> +		plane_config->bpp = 32;
> +		break;
> +	}
> +
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		if (plane_config->tiled)
> +			plane_config->offset = I915_READ(DSPTILEOFF(plane));
> +		else
> +			plane_config->offset = I915_READ(DSPLINOFF(plane));
> +		plane_config->base = I915_READ(DSPSURF(plane)) & 0xfffff000;
> +	} else {
> +		plane_config->base = I915_READ(DSPADDR(plane));
> +	}
> +
> +	val = I915_READ(PIPESRC(pipe));
> +	plane_config->pipe_width = ((val >> 16) & 0xfff) + 1;
> +	plane_config->pipe_height = ((val >> 0) & 0xfff) + 1;
> +
> +	val = I915_READ(HTOTAL(pipe));
> +	plane_config->fb_width = (val & 0xffff) + 1;
> +	val = I915_READ(VTOTAL(pipe));
> +	plane_config->fb_height = (val & 0xffff) + 1;
> +
> +	DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x\n",
> +		      pipe, plane, plane_config->fb_width,
> +		      plane_config->fb_height, plane_config->bpp,
> +		      plane_config->base);
> +
> +	plane_config->pitch =
> +		intel_framebuffer_pitch_for_width(dev_priv,
> +						  plane_config->fb_width,
> +						  plane_config->bpp,
> +						  plane_config->tiled);
> +
> +	plane_config->size = ALIGN(plane_config->pitch *
> +				   plane_config->fb_height, PAGE_SIZE);
> +	/*
> +	 * If the fb is shared between multiple heads, we'll just get the
> +	 * first one.
> +	 */
> +	plane_config->obj =
> +		i915_gem_object_create_stolen_for_preallocated(dev,
> +							       plane_config->base,
> +							       plane_config->base,
> +							       plane_config->size);
> +}
> +
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  				 struct intel_crtc_config *pipe_config)
>  {
> @@ -10540,6 +10641,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> +		dev_priv->display.get_plane_config = i9xx_get_plane_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = valleyview_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -10547,6 +10649,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = i9xx_update_plane;
>  	} else {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> +		dev_priv->display.get_plane_config = i9xx_get_plane_config;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = i9xx_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -10801,6 +10904,7 @@ void intel_modeset_suspend_hw(struct drm_device *dev)
>  void intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc;
>  	int i, j, ret;
>  
>  	drm_mode_config_init(dev);
> @@ -10857,6 +10961,18 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  	/* Just in case the BIOS is doing something questionable. */
>  	intel_disable_fbc(dev);
> +
> +	intel_modeset_setup_hw_state(dev, false);
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> +			    base.head) {
> +		if (!crtc->active)
> +			continue;
> +
> +		if (dev_priv->display.get_plane_config)
> +			dev_priv->display.get_plane_config(crtc,
> +							   &crtc->plane_config);
> +	}
>  }
>  
>  static void
> @@ -11227,8 +11343,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	intel_modeset_init_hw(dev);
>  
>  	intel_setup_overlay(dev);
> -
> -	intel_modeset_setup_hw_state(dev, false);
>  }
>  
>  void intel_modeset_cleanup(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0231281..bcc0b08 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -209,6 +209,18 @@ typedef struct dpll {
>  	int	p;
>  } intel_clock_t;
>  
> +struct intel_plane_config {
> +	int pixel_format; /* DRM fourcc code */

Comment isn't right unless I'm missing something. Looks like this pixel
format is set to the intel pixel format values, not the DRM fourcc
values.

> +	int bpp;
> +	bool tiled;
> +	int base, offset;
> +	int fb_width, fb_height;
> +	int pipe_width, pipe_height;
> +	int pitch;
> +	int size;
> +	struct drm_i915_gem_object *obj;
> +};
> +
>  struct intel_crtc_config {
>  	/**
>  	 * quirks - bitfield with hw state readout quirks
> @@ -358,6 +370,7 @@ struct intel_crtc {
>  	bool cursor_visible;
>  
>  	struct intel_crtc_config config;
> +	struct intel_plane_config plane_config;
>  
>  	uint32_t ddi_pll_sel;
>  
> @@ -707,6 +720,7 @@ void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
>  void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
> +int intel_format_to_fourcc(int format);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);

I'm still learning the i915 codebase but FWIW, this series looks like it
does what it's supposed to so other than the comment above, for the
series:

Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com>

Re: [PATCH 3/5] drm/i915: retrieve current fb config into new plane_config structure at init v2

[Jesse Barnes]

On Thu, 14 Nov 2013 16:44:21 -0800
Bob Paauwe <bob.j.paauwe@intel.com> wrote:

> > +struct intel_plane_config {
> > +	int pixel_format; /* DRM fourcc code */  
> 
> Comment isn't right unless I'm missing something. Looks like this pixel
> format is set to the intel pixel format values, not the DRM fourcc
> values.
> 

Stale comment, I changed my mind on what to store here.  Thanks for
catching it.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Jesse Barnes]

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>
---
 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

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

[Daniel Vetter]

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

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

[Chris Wilson]

On Fri, Nov 15, 2013 at 01:29:30AM +0100, Daniel Vetter wrote:
> 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.

This used to disable the outputs attached to the current bios fb if we
failed to takeover the fb. This is required to prevent the machine
reading out garbage whilst we modify the GTT and potentially hanging the
GPU. This is also a regression from about 3.2.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 5/5] drm/i915: don't memset the fb buffer if preallocated

[Jesse Barnes]

We want to preserve the BIOS/bootloader contents for later.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 5ee5dc0..c7b1adf 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -123,6 +123,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
 	int size, ret;
+	bool prealloc = false;
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -132,6 +133,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		if (ret)
 			goto out_unlock;
 	} else {
+		prealloc = true;
 		sizes->fb_width = intel_fb->base.width;
 		sizes->fb_height = intel_fb->base.height;
 	}
@@ -193,7 +195,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	 * If the object is stolen however, it will be full of whatever
 	 * garbage was left in there.
 	 */
-	if (ifbdev->ifb.obj->stolen)
+	if (ifbdev->ifb.obj->stolen && !prealloc)
 		memset_io(info->screen_base, 0, info->screen_size);
 
 	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
-- 
1.8.4.2

BIOS fb wrapping

[Jesse Barnes]

I split these out for clarity and ease of review.  They seem to be
working ok here now, and correctly taking the previous fb and using it.

The memset patch makes plymouth look better on this machine (it doesn't
flicker black near the end).

I haven't tested this on as many configs as the old patchset (dual head,
misc grub configs, etc), so extra testing would also be welcome.

Thanks,
Jesse

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

[Jesse Barnes]

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)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.c    |   5 +
 drivers/gpu/drm/i915/i915_drv.h    |   1 +
 drivers/gpu/drm/i915/intel_drv.h   |   2 +
 drivers/gpu/drm/i915/intel_fbdev.c | 187 +++++++++++++++++++++++++++++++++++--
 4 files changed, 189 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 30f4fe7..bcad343 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -154,6 +154,11 @@ module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
 MODULE_PARM_DESC(prefault_disable,
 		"Disable page prefaulting for pread/pwrite/reloc (default:false). For developers only.");
 
+bool i915_use_bios_fb __read_mostly = 1;
+module_param_named(use_bios_fb, i915_use_bios_fb, bool, 0600);
+MODULE_PARM_DESC(use_bios_fb,
+		 "Use BIOS allocated framebuffer for fbcon (default: true)");
+
 static struct drm_driver driver;
 #if IS_ENABLED(CONFIG_AGP_INTEL)
 extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index db6821a..500ccc7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1877,6 +1877,7 @@ extern bool i915_fastboot __read_mostly;
 extern int i915_enable_pc8 __read_mostly;
 extern int i915_pc8_timeout __read_mostly;
 extern bool i915_prefault_disable __read_mostly;
+extern bool i915_use_bios_fb __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
 extern int i915_resume(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cb72304..61dfffd 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 {
@@ -666,6 +667,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 8fa946b..adf92dd 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -238,6 +238,69 @@ 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;
+}
+
+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,
@@ -264,23 +327,133 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 	intel_framebuffer_fini(&ifbdev->ifb);
 }
 
+/*
+ * 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 are active, what resolutions
+ * are being displayed, and then allocates a framebuffer and initial fb
+ * config based on that data.
+ *
+ * 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.
+ *
+ * However, if we're loaded into an existing, high res mode, we should
+ * be able to allocate a buffer big enough to handle the largest active
+ * mode, create a mode_set for it, and pass it to the fb helper to create
+ * the configuration.
+ */
+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 };
+	u32 active = 0;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!intel_crtc->active) {
+			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
+				      pipe_name(intel_crtc->pipe));
+			continue;
+		}
+
+		active |= 1 << intel_crtc->pipe;
+		break;
+	}
+
+	plane_config = &intel_crtc->plane_config;
+
+	if (active == 0 || !plane_config->obj) {
+		DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
+		return;
+	}
+
+	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+	if (ifbdev == NULL) {
+		DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
+		return;
+	}
+
+	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,
+				   plane_config->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) {
+		if ((active & (1 << to_intel_crtc(crtc)->pipe)) == 0)
+			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, plane_config->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:
+	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(&plane_config->obj->base);
+	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;
+	if (i915_use_bios_fb)
+		intel_fbdev_init_bios(dev);
+
+	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;
+	}
 
-	dev_priv->fbdev = ifbdev;
 	ifbdev->helper.funcs = &intel_fb_helper_funcs;
 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
 				 INTEL_INFO(dev)->num_pipes,
 				 4);
 	if (ret) {
+		dev_priv->fbdev = NULL;
 		kfree(ifbdev);
 		return ret;
 	}
@@ -293,9 +466,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)
@@ -335,7 +509,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

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

[Bob Paauwe]

On Wed, 13 Nov 2013 10:20:47 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> 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)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c    |   5 +
>  drivers/gpu/drm/i915/i915_drv.h    |   1 +
>  drivers/gpu/drm/i915/intel_drv.h   |   2 +
>  drivers/gpu/drm/i915/intel_fbdev.c | 187 +++++++++++++++++++++++++++++++++++--
>  4 files changed, 189 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 30f4fe7..bcad343 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -154,6 +154,11 @@ module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
>  MODULE_PARM_DESC(prefault_disable,
>  		"Disable page prefaulting for pread/pwrite/reloc (default:false). For developers only.");
>  
> +bool i915_use_bios_fb __read_mostly = 1;
> +module_param_named(use_bios_fb, i915_use_bios_fb, bool, 0600);
> +MODULE_PARM_DESC(use_bios_fb,
> +		 "Use BIOS allocated framebuffer for fbcon (default: true)");
> +
>  static struct drm_driver driver;
>  #if IS_ENABLED(CONFIG_AGP_INTEL)
>  extern int intel_agp_enabled;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index db6821a..500ccc7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1877,6 +1877,7 @@ extern bool i915_fastboot __read_mostly;
>  extern int i915_enable_pc8 __read_mostly;
>  extern int i915_pc8_timeout __read_mostly;
>  extern bool i915_prefault_disable __read_mostly;
> +extern bool i915_use_bios_fb __read_mostly;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
>  extern int i915_resume(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cb72304..61dfffd 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 {
> @@ -666,6 +667,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 8fa946b..adf92dd 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -238,6 +238,69 @@ 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;
> +}
> +
> +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,
> @@ -264,23 +327,133 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>  	intel_framebuffer_fini(&ifbdev->ifb);
>  }
>  
> +/*
> + * 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 are active, what resolutions
> + * are being displayed, and then allocates a framebuffer and initial fb
> + * config based on that data.
> + *
> + * 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.
> + *
> + * However, if we're loaded into an existing, high res mode, we should
> + * be able to allocate a buffer big enough to handle the largest active
> + * mode, create a mode_set for it, and pass it to the fb helper to create
> + * the configuration.
> + */
> +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 };
> +	u32 active = 0;
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		intel_crtc = to_intel_crtc(crtc);
> +
> +		if (!intel_crtc->active) {
> +			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
> +				      pipe_name(intel_crtc->pipe));
> +			continue;
> +		}
> +
> +		active |= 1 << intel_crtc->pipe;
> +		break;
> +	}
> +
> +	plane_config = &intel_crtc->plane_config;
> +
> +	if (active == 0 || !plane_config->obj) {
> +		DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> +		return;
> +	}
> +
> +	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> +	if (ifbdev == NULL) {
> +		DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> +		return;
> +	}
> +
> +	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,
> +				   plane_config->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) {
> +		if ((active & (1 << to_intel_crtc(crtc)->pipe)) == 0)
> +			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, plane_config->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:
> +	mutex_unlock(&dev->struct_mutex);
> +	drm_gem_object_unreference_unlocked(&plane_config->obj->base);
> +	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;
> +	if (i915_use_bios_fb)
> +		intel_fbdev_init_bios(dev);
> +
> +	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;
> +	}
>  
> -	dev_priv->fbdev = ifbdev;
>  	ifbdev->helper.funcs = &intel_fb_helper_funcs;

Should helper.funcs still be set here?  It looks like it will be set by
intel_fbdev_init_bios if that is called/succeeds and otherwise set
a few lines above inside the if.  If it is set in intel_fbdev_init_bios
then this will override the change made there to the initial_config
helper.


>  
>  	ret = drm_fb_helper_init(dev, &ifbdev->helper,
>  				 INTEL_INFO(dev)->num_pipes,
>  				 4);
>  	if (ret) {
> +		dev_priv->fbdev = NULL;
>  		kfree(ifbdev);
>  		return ret;
>  	}
> @@ -293,9 +466,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)
> @@ -335,7 +509,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)

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

[Jesse Barnes]

On Wed, 13 Nov 2013 14:05:44 -0800
Bob Paauwe <bob.j.paauwe@intel.com> wrote:

> On Wed, 13 Nov 2013 10:20:47 -0800
> Jesse Barnes <jbarnes@virtuousgeek.org> 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)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c    |   5 +
> >  drivers/gpu/drm/i915/i915_drv.h    |   1 +
> >  drivers/gpu/drm/i915/intel_drv.h   |   2 +
> >  drivers/gpu/drm/i915/intel_fbdev.c | 187 +++++++++++++++++++++++++++++++++++--
> >  4 files changed, 189 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 30f4fe7..bcad343 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -154,6 +154,11 @@ module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
> >  MODULE_PARM_DESC(prefault_disable,
> >  		"Disable page prefaulting for pread/pwrite/reloc (default:false). For developers only.");
> >  
> > +bool i915_use_bios_fb __read_mostly = 1;
> > +module_param_named(use_bios_fb, i915_use_bios_fb, bool, 0600);
> > +MODULE_PARM_DESC(use_bios_fb,
> > +		 "Use BIOS allocated framebuffer for fbcon (default: true)");
> > +
> >  static struct drm_driver driver;
> >  #if IS_ENABLED(CONFIG_AGP_INTEL)
> >  extern int intel_agp_enabled;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index db6821a..500ccc7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1877,6 +1877,7 @@ extern bool i915_fastboot __read_mostly;
> >  extern int i915_enable_pc8 __read_mostly;
> >  extern int i915_pc8_timeout __read_mostly;
> >  extern bool i915_prefault_disable __read_mostly;
> > +extern bool i915_use_bios_fb __read_mostly;
> >  
> >  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> >  extern int i915_resume(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index cb72304..61dfffd 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 {
> > @@ -666,6 +667,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 8fa946b..adf92dd 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -238,6 +238,69 @@ 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;
> > +}
> > +
> > +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,
> > @@ -264,23 +327,133 @@ static void intel_fbdev_destroy(struct drm_device *dev,
> >  	intel_framebuffer_fini(&ifbdev->ifb);
> >  }
> >  
> > +/*
> > + * 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 are active, what resolutions
> > + * are being displayed, and then allocates a framebuffer and initial fb
> > + * config based on that data.
> > + *
> > + * 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.
> > + *
> > + * However, if we're loaded into an existing, high res mode, we should
> > + * be able to allocate a buffer big enough to handle the largest active
> > + * mode, create a mode_set for it, and pass it to the fb helper to create
> > + * the configuration.
> > + */
> > +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 };
> > +	u32 active = 0;
> > +
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +		intel_crtc = to_intel_crtc(crtc);
> > +
> > +		if (!intel_crtc->active) {
> > +			DRM_DEBUG_KMS("pipe %c not active, skipping\n",
> > +				      pipe_name(intel_crtc->pipe));
> > +			continue;
> > +		}
> > +
> > +		active |= 1 << intel_crtc->pipe;
> > +		break;
> > +	}
> > +
> > +	plane_config = &intel_crtc->plane_config;
> > +
> > +	if (active == 0 || !plane_config->obj) {
> > +		DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> > +		return;
> > +	}
> > +
> > +	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> > +	if (ifbdev == NULL) {
> > +		DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
> > +		return;
> > +	}
> > +
> > +	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,
> > +				   plane_config->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) {
> > +		if ((active & (1 << to_intel_crtc(crtc)->pipe)) == 0)
> > +			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, plane_config->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:
> > +	mutex_unlock(&dev->struct_mutex);
> > +	drm_gem_object_unreference_unlocked(&plane_config->obj->base);
> > +	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;
> > +	if (i915_use_bios_fb)
> > +		intel_fbdev_init_bios(dev);
> > +
> > +	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;
> > +	}
> >  
> > -	dev_priv->fbdev = ifbdev;
> >  	ifbdev->helper.funcs = &intel_fb_helper_funcs;
> 
> Should helper.funcs still be set here?  It looks like it will be set by
> intel_fbdev_init_bios if that is called/succeeds and otherwise set
> a few lines above inside the if.  If it is set in intel_fbdev_init_bios
> then this will override the change made there to the initial_config
> helper.

Yeah it could be clearer.  But the fbdev_init_bios function just
modifies the actual intel_fb_helper_funcs struct, so we can still
assign the address and not clobber what it did.

But it is confusing, I'll come up with something better.  The comment
could use some updating too, as this is a twisty maze between
drm_fb_helper and our code.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Chris Wilson]

On Wed, Nov 13, 2013 at 10:20:47AM -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)

Weirdness... What happened to calling fbdev_init_bios() prior to
clobbering the GTT and outputs?

> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c    |   5 +
>  drivers/gpu/drm/i915/i915_drv.h    |   1 +
>  drivers/gpu/drm/i915/intel_drv.h   |   2 +
>  drivers/gpu/drm/i915/intel_fbdev.c | 187 +++++++++++++++++++++++++++++++++++--
>  4 files changed, 189 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 30f4fe7..bcad343 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -154,6 +154,11 @@ module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
>  MODULE_PARM_DESC(prefault_disable,
>  		"Disable page prefaulting for pread/pwrite/reloc (default:false). For developers only.");
>  
> +bool i915_use_bios_fb __read_mostly = 1;
> +module_param_named(use_bios_fb, i915_use_bios_fb, bool, 0600);

mode 0400, there is no point in allowing it to be changed at runtime. Is
a parameter justified? Do we really foresee circumstances where we want
fbcon to reallocate?

> +MODULE_PARM_DESC(use_bios_fb,
> +		 "Use BIOS allocated framebuffer for fbcon (default: true)");
> +
>  static struct drm_driver driver;
>  #if IS_ENABLED(CONFIG_AGP_INTEL)
>  extern int intel_agp_enabled;


> +void intel_fbdev_init_bios(struct drm_device *dev)
> +{
[snip]
> +out_unref_obj:
> +	mutex_unlock(&dev->struct_mutex);
> +	drm_gem_object_unreference_unlocked(&plane_config->obj->base);

Would be cleaner to reverse these two lines and use
drm_gem_object_unreference()
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Wed, 13 Nov 2013 22:07:26 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Nov 13, 2013 at 10:20:47AM -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)
> 
> Weirdness... What happened to calling fbdev_init_bios() prior to
> clobbering the GTT and outputs?

Hm I'll have to check on the outputs, if we're clobbering them that's
definitely not what I intended.

For the GTT though we allocate the buffer in the plane_config readout,
so it should be safe across GTT init.  It does need to be freed though
if we decide against using it, so that's another bug to fix.

> > +bool i915_use_bios_fb __read_mostly = 1;
> > +module_param_named(use_bios_fb, i915_use_bios_fb, bool, 0600);
> 
> mode 0400, there is no point in allowing it to be changed at runtime. Is
> a parameter justified? Do we really foresee circumstances where we want
> fbcon to reallocate?

I'll drop it to be optimistic. :)

> > +MODULE_PARM_DESC(use_bios_fb,
> > +		 "Use BIOS allocated framebuffer for fbcon (default: true)");
> > +
> >  static struct drm_driver driver;
> >  #if IS_ENABLED(CONFIG_AGP_INTEL)
> >  extern int intel_agp_enabled;
> 
> 
> > +void intel_fbdev_init_bios(struct drm_device *dev)
> > +{
> [snip]
> > +out_unref_obj:
> > +	mutex_unlock(&dev->struct_mutex);
> > +	drm_gem_object_unreference_unlocked(&plane_config->obj->base);
> 
> Would be cleaner to reverse these two lines and use
> drm_gem_object_unreference()

Ok.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center