BIOS fb wrapping

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 1/5] drm/i915: make pitch_for_width take a tiled arg

[Jesse Barnes]

And move it up in the file for earlier usage.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2df2366..d4cc00c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5452,6 +5452,17 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
 	pipe_config->port_clock = clock.dot / 5;
 }
 
+static u32
+intel_framebuffer_pitch_for_width(int width, int bpp, bool tiled)
+{
+	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
+
+	if (tiled)
+		return ALIGN(pitch, 512);
+	else
+		return ALIGN(pitch, 64);
+}
+
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 				 struct intel_crtc_config *pipe_config)
 {
@@ -7629,16 +7640,10 @@ 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)
 {
-	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
+	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp,
+						      false);
 	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);
 }
 
@@ -7658,7 +7663,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
 	mode_cmd.width = mode->hdisplay;
 	mode_cmd.height = mode->vdisplay;
 	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
-								bpp);
+								bpp, false);
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
 
 	return intel_framebuffer_create(dev, &mode_cmd, obj);
@@ -7682,7 +7687,8 @@ mode_fits_in_fbdev(struct drm_device *dev,
 
 	fb = &dev_priv->fbdev->ifb.base;
 	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
-							       fb->bits_per_pixel))
+							       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: retrieve current fb config into new plane_config structure at init

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4377523..db6821a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -364,6 +364,7 @@ struct drm_i915_error_state {
 };
 
 struct intel_crtc_config;
+struct intel_plane_config;
 struct intel_crtc;
 struct intel_limit;
 struct dpll;
@@ -402,6 +403,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 d4cc00c..c3c4fc3 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)
 {
@@ -5463,6 +5484,81 @@ intel_framebuffer_pitch_for_width(int width, int bpp, bool tiled)
 		return ALIGN(pitch, 64);
 }
 
+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(plane_config->fb_width,
+						  plane_config->bpp,
+						  plane_config->tiled);
+
+	plane_config->size = ALIGN(plane_config->pitch *
+				   plane_config->fb_height, PAGE_SIZE);
+	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)
 {
@@ -10523,6 +10619,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;
@@ -10530,6 +10627,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;
@@ -10798,6 +10896,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);
@@ -10854,6 +10953,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
@@ -11224,8 +11335,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 b2d2cc1..cb72304 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -198,6 +198,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
@@ -347,6 +359,7 @@ struct intel_crtc {
 	bool cursor_visible;
 
 	struct intel_crtc_config config;
+	struct intel_plane_config plane_config;
 
 	uint32_t ddi_pll_sel;
 
@@ -696,6 +709,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

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 895fcb4..8fa946b 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 */
@@ -103,7 +99,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;
+
+	mutex_unlock(&dev->struct_mutex);
+	return 0;
+
+out_unpin:
+	i915_gem_object_unpin(obj);
+out_unref:
+	drm_gem_object_unreference(&obj->base);
+	mutex_unlock(&dev->struct_mutex);
+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;
+
+	if (!intel_fb->obj) {
+		DRM_ERROR("no BIOS fb, allocating a new one\n");
+		ret = intelfb_alloc(helper, sizes);
+		if (ret)
+			goto out;
+	} 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 +149,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;
@@ -177,7 +211,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 out_unpin:
 	i915_gem_object_unpin(obj);
-out_unref:
 	drm_gem_object_unreference(&obj->base);
 	mutex_unlock(&dev->struct_mutex);
 out:
-- 
1.8.4.2

[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

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

[Jesse Barnes]

It may be in use, let fbcon do it later if needed.

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

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index adf92dd..259f5ca 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -191,13 +191,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
 	drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
 
-	/* If the object is shmemfs backed, it will have given us zeroed pages.
-	 * If the object is stolen however, it will be full of whatever
-	 * garbage was left in there.
-	 */
-	if (ifbdev->ifb.obj->stolen)
-		memset_io(info->screen_base, 0, info->screen_size);
-
 	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
 
 	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n",
-- 
1.8.4.2

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

[Chris Wilson]

On Wed, Nov 13, 2013 at 10:20:48AM -0800, Jesse Barnes wrote:
> It may be in use, let fbcon do it later if needed.

Sadly we need to memset the stolen buffer in some circumstances, so we
need a bit more smarts. Upon resume and if !preallocated are the cases
that spring to mind. Maybe we can add a flag to ifbdev when we can skip
the clear?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/5] drm/i915: split fb allocation and initialization

[Chris Wilson]

On Wed, Nov 13, 2013 at 10:20:46AM -0800, Jesse Barnes wrote:
> 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.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Looks reasonable,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Wed, Nov 13, 2013 at 10:20:44AM -0800, Jesse Barnes wrote:
> And move it up in the file for earlier usage.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2df2366..d4cc00c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5452,6 +5452,17 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>  	pipe_config->port_clock = clock.dot / 5;
>  }
>  
> +static u32
> +intel_framebuffer_pitch_for_width(int width, int bpp, bool tiled)
> +{
> +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> +
> +	if (tiled)
> +		return ALIGN(pitch, 512);

Should this not go the whole hog and do pot alignment on gen2/3?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

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

> On Wed, Nov 13, 2013 at 10:20:48AM -0800, Jesse Barnes wrote:
> > It may be in use, let fbcon do it later if needed.
> 
> Sadly we need to memset the stolen buffer in some circumstances, so we
> need a bit more smarts. Upon resume and if !preallocated are the cases
> that spring to mind. Maybe we can add a flag to ifbdev when we can skip
> the clear?

Yeah, we need something... clearing creates ugly artifacts in some
cases that I'd like to avoid.

-- 
Jesse Barnes, Intel Open Source Technology Center

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 1/5] drm/i915: make pitch_for_width take a tiled arg

[Jesse Barnes]

On Wed, 13 Nov 2013 21:59:04 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Nov 13, 2013 at 10:20:44AM -0800, Jesse Barnes wrote:
> > And move it up in the file for earlier usage.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++----------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2df2366..d4cc00c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5452,6 +5452,17 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >  	pipe_config->port_clock = clock.dot / 5;
> >  }
> >  
> > +static u32
> > +intel_framebuffer_pitch_for_width(int width, int bpp, bool tiled)
> > +{
> > +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> > +
> > +	if (tiled)
> > +		return ALIGN(pitch, 512);
> 
> Should this not go the whole hog and do pot alignment on gen2/3?

Probably a good idea.  And this smells like common code, surely we must
be doing this somewhere else too?  Anyway could definitely be extended.

-- 
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 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 2/5] drm/i915: retrieve current fb config into new plane_config structure at init

[Chris Wilson]

On Wed, Nov 13, 2013 at 10:20:45AM -0800, Jesse Barnes wrote:
> +	plane_config->pitch =
> +		intel_framebuffer_pitch_for_width(plane_config->fb_width,
> +						  plane_config->bpp,
> +						  plane_config->tiled);
> +
> +	plane_config->size = ALIGN(plane_config->pitch *
> +				   plane_config->fb_height, PAGE_SIZE);
> +	plane_config->obj =
> +		i915_gem_object_create_stolen_for_preallocated(dev,
> +							       plane_config->base,
> +							       plane_config->base,
> +							       plane_config->size);
> +}

Oh I see now. Dramatic changes. Will need to dig up ken.
-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

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

[Chris Wilson]

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

I don't see where plane_config is freed afterwards?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Thu, 14 Nov 2013 15:06:59 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Wed, Nov 13, 2013 at 10:20:45AM -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.
> 
> I don't see where plane_config is freed afterwards?

We just keep it around like a piece of stale toast... guess I need to
find a place to free it (maybe after the first mode set or after the
fbdev BIOS init).

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Ville Syrjälä]

On Wed, Nov 13, 2013 at 10:20:45AM -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.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   3 +
>  drivers/gpu/drm/i915/intel_display.c | 113 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  14 +++++
>  3 files changed, 128 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4377523..db6821a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -364,6 +364,7 @@ struct drm_i915_error_state {
>  };
>  
>  struct intel_crtc_config;
> +struct intel_plane_config;
>  struct intel_crtc;
>  struct intel_limit;
>  struct dpll;
> @@ -402,6 +403,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 d4cc00c..c3c4fc3 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;
                                  ^
X

> +	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)
>  {
> @@ -5463,6 +5484,81 @@ intel_framebuffer_pitch_for_width(int width, int bpp, bool tiled)
>  		return ALIGN(pitch, 64);
>  }
>  
> +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;
> +	}

Maybe just intel_format_to_fourcc()+drm_format_plane_cpp() or something.
Just to avoid duplicating essentially the same code.

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

Why make the fb the size of htotal/vtotal? pipe src size should be all
we need.

> +
> +	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(plane_config->fb_width,
> +						  plane_config->bpp,
> +						  plane_config->tiled);
> +
> +	plane_config->size = ALIGN(plane_config->pitch *
> +				   plane_config->fb_height, PAGE_SIZE);
> +	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)
>  {
> @@ -10523,6 +10619,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;
> @@ -10530,6 +10627,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;
> @@ -10798,6 +10896,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);
> @@ -10854,6 +10953,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
> @@ -11224,8 +11335,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 b2d2cc1..cb72304 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -198,6 +198,18 @@ typedef struct dpll {
>  	int	p;
>  } intel_clock_t;
>  
> +struct intel_plane_config {
> +	int pixel_format; /* DRM fourcc code */

The comment doesn't match how you do things in the 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
> @@ -347,6 +359,7 @@ struct intel_crtc {
>  	bool cursor_visible;
>  
>  	struct intel_crtc_config config;
> +	struct intel_plane_config plane_config;
>  
>  	uint32_t ddi_pll_sel;
>  
> @@ -696,6 +709,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

-- 
Ville Syrjälä
Intel OTC

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

[Jesse Barnes]

On Wed, 20 Nov 2013 15:10:39 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > +	case DISPPLANE_8BPP:
> > +		return DRM_FORMAT_C8;
> > +	case DISPPLANE_BGRX555:
> > +		return DRM_FORMAT_ARGB1555;
>                                   ^
> X

Oops fixed, thanks.  These formats are a lot of typing.

> > +	case DISPPLANE_RGBA888:
> > +	case DISPPLANE_RGBX101010:
> > +	case DISPPLANE_RGBA101010:
> > +	case DISPPLANE_BGRX101010:
> > +		plane_config->bpp = 32;
> > +		break;
> > +	}
> 
> Maybe just intel_format_to_fourcc()+drm_format_plane_cpp() or something.
> Just to avoid duplicating essentially the same code.

Ah yeah, good idea, fixed.

> > +	val = I915_READ(HTOTAL(pipe));
> > +	plane_config->fb_width = (val & 0xffff) + 1;
> > +	val = I915_READ(VTOTAL(pipe));
> > +	plane_config->fb_height = (val & 0xffff) + 1;
> 
> Why make the fb the size of htotal/vtotal? pipe src size should be all
> we need.

Ok fixed.  Are there no cases where they'll be different?  Maybe
centered modes would have a pipesrc that differs from the
htotal/vtotal?  But even in that case we just want the pipesrc.

> > +struct intel_plane_config {
> > +	int pixel_format; /* DRM fourcc code */
> 
> The comment doesn't match how you do things in the code.

Yeah, Bob mentioned that too, fixed.

Does this look useful to you otherwise?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Nov 22, 2013 at 01:55:35PM -0800, Jesse Barnes wrote:
> On Wed, 20 Nov 2013 15:10:39 +0200
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > +	case DISPPLANE_8BPP:
> > > +		return DRM_FORMAT_C8;
> > > +	case DISPPLANE_BGRX555:
> > > +		return DRM_FORMAT_ARGB1555;
> >                                   ^
> > X
> 
> Oops fixed, thanks.  These formats are a lot of typing.
> 
> > > +	case DISPPLANE_RGBA888:
> > > +	case DISPPLANE_RGBX101010:
> > > +	case DISPPLANE_RGBA101010:
> > > +	case DISPPLANE_BGRX101010:
> > > +		plane_config->bpp = 32;
> > > +		break;
> > > +	}
> > 
> > Maybe just intel_format_to_fourcc()+drm_format_plane_cpp() or something.
> > Just to avoid duplicating essentially the same code.
> 
> Ah yeah, good idea, fixed.
> 
> > > +	val = I915_READ(HTOTAL(pipe));
> > > +	plane_config->fb_width = (val & 0xffff) + 1;
> > > +	val = I915_READ(VTOTAL(pipe));
> > > +	plane_config->fb_height = (val & 0xffff) + 1;
> > 
> > Why make the fb the size of htotal/vtotal? pipe src size should be all
> > we need.
> 
> Ok fixed.  Are there no cases where they'll be different?  Maybe
> centered modes would have a pipesrc that differs from the
> htotal/vtotal?  But even in that case we just want the pipesrc.

htotal/vtotal should have nothing to do with the fb dimensions.
pipesrc (or plane w/h if we had such registers for primary planes)
are the only things we can really tell. The information whether
the fb was orignally larger than that can't be recovered.

-- 
Ville Syrjälä
Intel OTC

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

[Jesse Barnes]

On Sat, 23 Nov 2013 01:08:17 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Nov 22, 2013 at 01:55:35PM -0800, Jesse Barnes wrote:
> > On Wed, 20 Nov 2013 15:10:39 +0200
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > +	case DISPPLANE_8BPP:
> > > > +		return DRM_FORMAT_C8;
> > > > +	case DISPPLANE_BGRX555:
> > > > +		return DRM_FORMAT_ARGB1555;
> > >                                   ^
> > > X
> > 
> > Oops fixed, thanks.  These formats are a lot of typing.
> > 
> > > > +	case DISPPLANE_RGBA888:
> > > > +	case DISPPLANE_RGBX101010:
> > > > +	case DISPPLANE_RGBA101010:
> > > > +	case DISPPLANE_BGRX101010:
> > > > +		plane_config->bpp = 32;
> > > > +		break;
> > > > +	}
> > > 
> > > Maybe just intel_format_to_fourcc()+drm_format_plane_cpp() or something.
> > > Just to avoid duplicating essentially the same code.
> > 
> > Ah yeah, good idea, fixed.
> > 
> > > > +	val = I915_READ(HTOTAL(pipe));
> > > > +	plane_config->fb_width = (val & 0xffff) + 1;
> > > > +	val = I915_READ(VTOTAL(pipe));
> > > > +	plane_config->fb_height = (val & 0xffff) + 1;
> > > 
> > > Why make the fb the size of htotal/vtotal? pipe src size should be all
> > > we need.
> > 
> > Ok fixed.  Are there no cases where they'll be different?  Maybe
> > centered modes would have a pipesrc that differs from the
> > htotal/vtotal?  But even in that case we just want the pipesrc.
> 
> htotal/vtotal should have nothing to do with the fb dimensions.
> pipesrc (or plane w/h if we had such registers for primary planes)
> are the only things we can really tell. The information whether
> the fb was orignally larger than that can't be recovered.

We couldn't get that from the scaling regs?  If you're up or
downscaling, don't you have to specify a factor somewhere?  Either in
the pfit regs or the sprite scaling regs?

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Fri, Nov 22, 2013 at 03:21:08PM -0800, Jesse Barnes wrote:
> On Sat, 23 Nov 2013 01:08:17 +0200
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Fri, Nov 22, 2013 at 01:55:35PM -0800, Jesse Barnes wrote:
> > > On Wed, 20 Nov 2013 15:10:39 +0200
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > > +	case DISPPLANE_8BPP:
> > > > > +		return DRM_FORMAT_C8;
> > > > > +	case DISPPLANE_BGRX555:
> > > > > +		return DRM_FORMAT_ARGB1555;
> > > >                                   ^
> > > > X
> > > 
> > > Oops fixed, thanks.  These formats are a lot of typing.
> > > 
> > > > > +	case DISPPLANE_RGBA888:
> > > > > +	case DISPPLANE_RGBX101010:
> > > > > +	case DISPPLANE_RGBA101010:
> > > > > +	case DISPPLANE_BGRX101010:
> > > > > +		plane_config->bpp = 32;
> > > > > +		break;
> > > > > +	}
> > > > 
> > > > Maybe just intel_format_to_fourcc()+drm_format_plane_cpp() or something.
> > > > Just to avoid duplicating essentially the same code.
> > > 
> > > Ah yeah, good idea, fixed.
> > > 
> > > > > +	val = I915_READ(HTOTAL(pipe));
> > > > > +	plane_config->fb_width = (val & 0xffff) + 1;
> > > > > +	val = I915_READ(VTOTAL(pipe));
> > > > > +	plane_config->fb_height = (val & 0xffff) + 1;
> > > > 
> > > > Why make the fb the size of htotal/vtotal? pipe src size should be all
> > > > we need.
> > > 
> > > Ok fixed.  Are there no cases where they'll be different?  Maybe
> > > centered modes would have a pipesrc that differs from the
> > > htotal/vtotal?  But even in that case we just want the pipesrc.
> > 
> > htotal/vtotal should have nothing to do with the fb dimensions.
> > pipesrc (or plane w/h if we had such registers for primary planes)
> > are the only things we can really tell. The information whether
> > the fb was orignally larger than that can't be recovered.
> 
> We couldn't get that from the scaling regs?  If you're up or
> downscaling, don't you have to specify a factor somewhere?  Either in
> the pfit regs or the sprite scaling regs?

Scaling doesn't matter. Pipesrc defines the size of the viewport
into the fb. Scaling regs would just tell us whether that gets up
or downscaled by some amount.

-- 
Ville Syrjäl 
Intel OTC

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

[Jesse Barnes]

On Sat, 23 Nov 2013 01:26:34 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Nov 22, 2013 at 03:21:08PM -0800, Jesse Barnes wrote:
> > On Sat, 23 Nov 2013 01:08:17 +0200
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > 
> > > On Fri, Nov 22, 2013 at 01:55:35PM -0800, Jesse Barnes wrote:
> > > > On Wed, 20 Nov 2013 15:10:39 +0200
> > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > > > > +	case DISPPLANE_8BPP:
> > > > > > +		return DRM_FORMAT_C8;
> > > > > > +	case DISPPLANE_BGRX555:
> > > > > > +		return DRM_FORMAT_ARGB1555;
> > > > >                                   ^
> > > > > X
> > > > 
> > > > Oops fixed, thanks.  These formats are a lot of typing.
> > > > 
> > > > > > +	case DISPPLANE_RGBA888:
> > > > > > +	case DISPPLANE_RGBX101010:
> > > > > > +	case DISPPLANE_RGBA101010:
> > > > > > +	case DISPPLANE_BGRX101010:
> > > > > > +		plane_config->bpp = 32;
> > > > > > +		break;
> > > > > > +	}
> > > > > 
> > > > > Maybe just intel_format_to_fourcc()+drm_format_plane_cpp() or something.
> > > > > Just to avoid duplicating essentially the same code.
> > > > 
> > > > Ah yeah, good idea, fixed.
> > > > 
> > > > > > +	val = I915_READ(HTOTAL(pipe));
> > > > > > +	plane_config->fb_width = (val & 0xffff) + 1;
> > > > > > +	val = I915_READ(VTOTAL(pipe));
> > > > > > +	plane_config->fb_height = (val & 0xffff) + 1;
> > > > > 
> > > > > Why make the fb the size of htotal/vtotal? pipe src size should be all
> > > > > we need.
> > > > 
> > > > Ok fixed.  Are there no cases where they'll be different?  Maybe
> > > > centered modes would have a pipesrc that differs from the
> > > > htotal/vtotal?  But even in that case we just want the pipesrc.
> > > 
> > > htotal/vtotal should have nothing to do with the fb dimensions.
> > > pipesrc (or plane w/h if we had such registers for primary planes)
> > > are the only things we can really tell. The information whether
> > > the fb was orignally larger than that can't be recovered.
> > 
> > We couldn't get that from the scaling regs?  If you're up or
> > downscaling, don't you have to specify a factor somewhere?  Either in
> > the pfit regs or the sprite scaling regs?
> 
> Scaling doesn't matter. Pipesrc defines the size of the viewport
> into the fb. Scaling regs would just tell us whether that gets up
> or downscaled by some amount.

Oh yeah for viewing just a portion of the fb... yeah in that case we'd
be hosed.  But otoh, this code would just preserve the viewable region
so maybe no one would be the wiser...

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx