[PATCH 1/8] drm/i915: unconditionally copy mode into crtc at boot time

[PATCH 1/8] drm/i915: unconditionally copy mode into crtc at boot time

[Jesse Barnes]

The BIOS code will need this to properly inherit the mode.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af3717a..1ae3d44 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	 */
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
 			    base.head) {
-		if (crtc->active && i915_fastboot) {
+		if (crtc->active) {
 			intel_crtc_mode_from_pipe_config(crtc, &crtc->config);
 
 			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
-- 
1.8.4.2

[PATCH 2/8] drm/i915/vlv: add early DPIO init v3

[Jesse Barnes]

Just add an early init since we may need to access DPIO regs early on.
The init call in modeset_init_hw is also needed for the resume case,
when we need to reset DPIO to keep things happy.

v2: split reset and reg init
v3: split patches (Daniel)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1ae3d44..a30176b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10861,6 +10861,8 @@ void intel_modeset_init(struct drm_device *dev)
 		}
 	}
 
+	intel_init_dpio(dev);
+
 	intel_cpu_pll_init(dev);
 	intel_shared_dpll_init(dev);
 
-- 
1.8.4.2

[PATCH 3/8] drm/i915/vlv: split DPIO init and reset

[Jesse Barnes]

We only need to init the reg offset for DPIO once, but we need to reset
DPIO at resume time and at init time.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a30176b..a2cd692 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1368,6 +1368,15 @@ static void intel_init_dpio(struct drm_device *dev)
 		return;
 
 	DPIO_PHY_IOSF_PORT(DPIO_PHY0) = IOSF_PORT_DPIO;
+}
+
+static void intel_reset_dpio(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!IS_VALLEYVIEW(dev))
+		return;
+
 	/*
 	 * From VLV2A0_DP_eDP_DPIO_driver_vbios_notes_10.docx -
 	 *  6.	De-assert cmn_reset/side_reset. Same as VLV X0.
@@ -10799,7 +10808,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
 		I915_WRITE(DPLL(PIPE_B), I915_READ(DPLL(PIPE_B)) |
 			   DPLL_INTEGRATED_CRI_CLK_VLV);
 
-	intel_init_dpio(dev);
+	intel_reset_dpio(dev);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_enable_gt_powersave(dev);
@@ -10862,6 +10871,7 @@ void intel_modeset_init(struct drm_device *dev)
 	}
 
 	intel_init_dpio(dev);
+	intel_reset_dpio(dev);
 
 	intel_cpu_pll_init(dev);
 	intel_shared_dpll_init(dev);
-- 
1.8.4.2

[PATCH 4/8] drm/i915: read out hw state earlier

[Jesse Barnes]

We want to do this early on before we try to fetch the plane config,
which depends on some of the pipe config state.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a2cd692..8f3d033 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10882,6 +10882,11 @@ void intel_modeset_init(struct drm_device *dev)
 
 	/* Just in case the BIOS is doing something questionable. */
 	intel_disable_fbc(dev);
+
+	drm_modeset_lock_all(dev);
+	drm_mode_config_reset(dev);
+	intel_modeset_setup_hw_state(dev, false);
+	drm_modeset_unlock_all(dev);
 }
 
 static void
@@ -11249,11 +11254,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	intel_modeset_init_hw(dev);
 
 	intel_setup_overlay(dev);
-
-	drm_modeset_lock_all(dev);
-	drm_mode_config_reset(dev);
-	intel_modeset_setup_hw_state(dev, false);
-	drm_modeset_unlock_all(dev);
 }
 
 void intel_modeset_cleanup(struct drm_device *dev)
-- 
1.8.4.2

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

[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)
v3: s/ARGB/XRGB (Ville)
    use pipesrc width/height (Ville)
    fix fourcc comment (Bob)
    use drm_format_plane_cpp (Ville)
v4: use fb for tracking fb data object (Ville)
v5: fix up gen2 pitch limits (Ville)
v6: read out stride as well (Daniel)
v7: split out init ordering changes (Daniel)
    don't fetch config if !CONFIG_FB

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index efc57fe..2ea151d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -366,6 +366,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;
@@ -404,6 +405,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 8f3d033..41f625b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2017,6 +2017,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_XRGB1555;
+	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)
 {
@@ -5472,6 +5493,92 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
 	pipe_config->port_clock = clock.dot / 5;
 }
 
+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;
+	struct drm_i915_gem_object *obj = NULL;
+	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+	u32 val, base, offset;
+	int pipe = crtc->pipe, plane = crtc->plane;
+	int fourcc, pixel_format;
+
+	plane_config->fb = kzalloc(sizeof(*plane_config->fb), GFP_KERNEL);
+	if (!plane_config->fb) {
+		DRM_DEBUG_KMS("failed to alloc fb\n");
+		return;
+	}
+
+	val = I915_READ(DSPCNTR(plane));
+
+	if (INTEL_INFO(dev)->gen >= 4)
+		if (val & DISPPLANE_TILED)
+			plane_config->tiled = true;
+
+	pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
+	fourcc = intel_format_to_fourcc(pixel_format);
+	plane_config->fb->base.pixel_format = fourcc;
+	plane_config->fb->base.bits_per_pixel =
+		drm_format_plane_cpp(fourcc, 0) * 8;
+
+	if (INTEL_INFO(dev)->gen >= 4) {
+		if (plane_config->tiled)
+			offset = I915_READ(DSPTILEOFF(plane));
+		else
+			offset = I915_READ(DSPLINOFF(plane));
+		base = I915_READ(DSPSURF(plane)) & 0xfffff000;
+	} else {
+		base = I915_READ(DSPADDR(plane));
+	}
+
+	val = I915_READ(PIPESRC(pipe));
+	plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
+	plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
+
+	val = I915_READ(DSPSTRIDE(pipe));
+	plane_config->fb->base.pitches[0] = val & 0xffffff80;
+
+	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
+				   plane_config->fb->base.height, PAGE_SIZE);
+
+	DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x, pitch %d, size 0x%x\n",
+		      pipe, plane, plane_config->fb->base.width,
+		      plane_config->fb->base.height,
+		      plane_config->fb->base.bits_per_pixel, base,
+		      plane_config->fb->base.pitches[0],
+		      plane_config->size);
+
+	/*
+	 * If the fb is shared between multiple heads, we'll just get the
+	 * first one.
+	 */
+	obj = i915_gem_object_create_stolen_for_preallocated(dev, base, base,
+							     plane_config->size);
+	if (!obj)
+		return;
+
+	mode_cmd.pixel_format = fourcc;
+	mode_cmd.width = plane_config->fb->base.width;
+	mode_cmd.height = plane_config->fb->base.height;
+	mode_cmd.pitches[0] = plane_config->fb->base.pitches[0];
+
+	mutex_lock(&dev->struct_mutex);
+
+	if (intel_framebuffer_init(dev, plane_config->fb, &mode_cmd, obj)) {
+		DRM_DEBUG_KMS("intel fb init failed\n");
+		goto out_unref_obj;
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+	DRM_DEBUG_KMS("plane fb obj %p\n", plane_config->fb->obj);
+	return;
+
+out_unref_obj:
+	drm_gem_object_unreference(&obj->base);
+	mutex_unlock(&dev->struct_mutex);
+}
+
 static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 				 struct intel_crtc_config *pipe_config)
 {
@@ -10562,6 +10669,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;
@@ -10569,6 +10677,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;
@@ -10823,6 +10932,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);
@@ -10887,6 +10997,22 @@ void intel_modeset_init(struct drm_device *dev)
 	drm_mode_config_reset(dev);
 	intel_modeset_setup_hw_state(dev, false);
 	drm_modeset_unlock_all(dev);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
+			    base.head) {
+		if (!crtc->active)
+			continue;
+
+#ifdef CONFIG_FB
+		/*
+		 * We don't have a good way of freeing the buffer w/o the FB
+		 * layer owning it...
+		 */
+		if (dev_priv->display.get_plane_config)
+			dev_priv->display.get_plane_config(crtc,
+							   &crtc->plane_config);
+#endif
+	}
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ea62673..4787773 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -209,6 +209,12 @@ typedef struct dpll {
 	int	p;
 } intel_clock_t;
 
+struct intel_plane_config {
+	struct intel_framebuffer *fb; /* ends up managed by intel_fbdev.c */
+	bool tiled;
+	int size;
+};
+
 struct intel_crtc_config {
 	/**
 	 * quirks - bitfield with hw state readout quirks
@@ -358,6 +364,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 +714,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 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[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)
v7: use fb object from plane_config instead (Ville)
    take ref on fb object (Jesse)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |   4 +-
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 drivers/gpu/drm/i915/intel_fbdev.c   | 235 ++++++++++++++++++++++++++++++++---
 3 files changed, 224 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 41f625b..7d73b13 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7826,11 +7826,11 @@ mode_fits_in_fbdev(struct drm_device *dev,
 	if (dev_priv->fbdev == NULL)
 		return NULL;
 
-	obj = dev_priv->fbdev->ifb.obj;
+	obj = dev_priv->fbdev->fb->obj;
 	if (obj == NULL)
 		return NULL;
 
-	fb = &dev_priv->fbdev->ifb.base;
+	fb = &dev_priv->fbdev->fb->base;
 	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
 							       fb->bits_per_pixel))
 		return NULL;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4787773..5f9182e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -110,9 +110,10 @@ struct intel_framebuffer {
 
 struct intel_fbdev {
 	struct drm_fb_helper helper;
-	struct intel_framebuffer ifb;
+	struct intel_framebuffer *fb;
 	struct list_head fbdev_list;
 	struct drm_display_mode *our_mode;
+	int preferred_bpp;
 };
 
 struct intel_encoder {
@@ -671,6 +672,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 284c3eb..db75f22 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -62,11 +62,20 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 {
 	struct intel_fbdev *ifbdev =
 		container_of(helper, struct intel_fbdev, helper);
+	struct intel_framebuffer *fb;
 	struct drm_device *dev = helper->dev;
 	struct drm_mode_fb_cmd2 mode_cmd = {};
 	struct drm_i915_gem_object *obj;
 	int size, ret;
 
+	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
+	if (!fb) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ifbdev->fb = fb;
+
 	/* we don't do packed 24bpp */
 	if (sizes->surface_bpp == 24)
 		sizes->surface_bpp = 32;
@@ -97,7 +106,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 		goto out_unref;
 	}
 
-	ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
+	ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
 	if (ret)
 		goto out_unpin;
 
@@ -116,7 +125,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 {
 	struct intel_fbdev *ifbdev =
 		container_of(helper, struct intel_fbdev, helper);
-	struct intel_framebuffer *intel_fb = &ifbdev->ifb;
+	struct intel_framebuffer *intel_fb = ifbdev->fb;
 	struct drm_device *dev = helper->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct fb_info *info;
@@ -148,7 +157,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info->par = helper;
 
-	fb = &ifbdev->ifb.base;
+	fb = &ifbdev->fb->base;
 
 	ifbdev->helper.fb = fb;
 	ifbdev->helper.fbdev = info;
@@ -194,14 +203,14 @@ 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->fb->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",
+	DRM_DEBUG_KMS("allocated %dx%d fb: map %p, bo %p, size 0x%lx\n",
 		      fb->width, fb->height,
-		      i915_gem_obj_ggtt_offset(obj), obj);
+		      info->screen_base, obj, info->screen_size);
 
 	mutex_unlock(&dev->struct_mutex);
 	vga_switcheroo_client_fb_set(dev->pdev, info);
@@ -236,6 +245,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,
@@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 	drm_fb_helper_fini(&ifbdev->helper);
 
-	drm_framebuffer_unregister_private(&ifbdev->ifb.base);
-	intel_framebuffer_fini(&ifbdev->ifb);
+	drm_framebuffer_unregister_private(&ifbdev->fb->base);
+	intel_framebuffer_fini(ifbdev->fb);
+	kfree(ifbdev->fb);
+}
+
+/*
+ * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
+ * The core display code will have read out the current plane configuration,
+ * so we use that to figure out if there's an object for us to use as the
+ * fb, and if so, we re-use it for the fbdev configuration.
+ *
+ * Note we only support a single fb shared across pipes for boot (mostly for
+ * fbcon), so we just find the biggest and use that.
+ */
+void intel_fbdev_init_bios(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_fbdev *ifbdev;
+	struct intel_framebuffer *fb = NULL;
+	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
+	struct intel_plane_config *plane_config = NULL;
+	unsigned int last_size = 0;
+
+	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+	if (ifbdev == NULL) {
+		DRM_DEBUG_KMS("failed to alloc intel fbdev\n");
+		return;
+	}
+
+	/* Find the largest framebuffer to use, then free the others */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!intel_crtc->active || !intel_crtc->plane_config.fb->obj) {
+			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
+				      pipe_name(intel_crtc->pipe));
+			continue;
+		}
+
+		if (intel_crtc->plane_config.size > last_size) {
+			plane_config = &intel_crtc->plane_config;
+			last_size = plane_config->size;
+			fb = plane_config->fb;
+		}
+	}
+
+	/* Free unused fbs */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_framebuffer *cur_fb;
+
+		intel_crtc = to_intel_crtc(crtc);
+		cur_fb = intel_crtc->plane_config.fb;
+
+		if (cur_fb && cur_fb != fb)
+			intel_framebuffer_fini(cur_fb);
+	}
+
+	if (!fb) {
+		DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
+		goto out_free;
+	}
+
+	ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
+	ifbdev->helper.funcs = &intel_fb_helper_funcs;
+	ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
+	ifbdev->fb = fb;
+
+	/* Assuming a single fb across all pipes here */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!intel_crtc->active)
+			continue;
+
+		/*
+		 * This should only fail on the first one so we don't need
+		 * to cleanup any secondary crtc->fbs
+		 */
+		if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
+			goto out_unref_obj;
+
+		crtc->fb = &fb->base;
+		drm_gem_object_reference(&fb->obj->base);
+		drm_framebuffer_reference(&fb->base);
+	}
+
+	dev_priv->fbdev = ifbdev;
+
+	DRM_DEBUG_KMS("using BIOS fb for initial console\n");
+	return;
+
+out_unref_obj:
+	intel_framebuffer_fini(fb);
+out_free:
+	kfree(ifbdev);
 }
 
 int intel_fbdev_init(struct drm_device *dev)
@@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
-	if (!ifbdev)
-		return -ENOMEM;
+	/* This may fail, if so, dev_priv->fbdev won't be set below */
+	intel_fbdev_init_bios(dev);
 
-	dev_priv->fbdev = ifbdev;
-	ifbdev->helper.funcs = &intel_fb_helper_funcs;
+	if ((ifbdev = dev_priv->fbdev) == NULL) {
+		ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+		if (ifbdev == NULL)
+			return -ENOMEM;
+
+		ifbdev->helper.funcs = &intel_fb_helper_funcs;
+		ifbdev->preferred_bpp = 32;
+
+		dev_priv->fbdev = ifbdev;
+	}
 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
 				 INTEL_INFO(dev)->num_pipes,
 				 4);
 	if (ret) {
+		dev_priv->fbdev = NULL;
 		kfree(ifbdev);
 		return ret;
 	}
@@ -291,9 +492,10 @@ int intel_fbdev_init(struct drm_device *dev)
 void intel_fbdev_initial_config(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_fbdev *ifbdev = dev_priv->fbdev;
 
 	/* Due to peculiar init order wrt to hpd handling this is separate. */
-	drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32);
+	drm_fb_helper_initial_config(&ifbdev->helper, ifbdev->preferred_bpp);
 }
 
 void intel_fbdev_fini(struct drm_device *dev)
@@ -322,7 +524,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
 	 * been restored from swap. If the object is stolen however, it will be
 	 * full of whatever garbage was left in there.
 	 */
-	if (state == FBINFO_STATE_RUNNING && ifbdev->ifb.obj->stolen)
+	if (state == FBINFO_STATE_RUNNING && ifbdev->fb->obj->stolen)
 		memset_io(info->screen_base, 0, info->screen_size);
 
 	fb_set_suspend(info, state);
@@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+	if (dev_priv->fbdev)
+		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
 }
 
 void intel_fbdev_restore_mode(struct drm_device *dev)
-- 
1.8.4.2

[PATCH 7/8] 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 db75f22..53675d2 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -132,6 +132,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);
 
@@ -142,6 +143,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 			goto out_unlock;
 	} else {
 		DRM_DEBUG_KMS("re-using BIOS fb\n");
+		prealloc = true;
 		sizes->fb_width = intel_fb->base.width;
 		sizes->fb_height = intel_fb->base.height;
 	}
@@ -203,7 +205,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->fb->obj->stolen)
+	if (ifbdev->fb->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

[PATCH 8/8] drm/i915: inform drm_fb_helper if we abandoned a connected output v2

[Jesse Barnes]

Otherwise subsequent fb activity will try to do blank/unblank on outputs
that were never fully enabled.

v2: drop unnecessary enabled[] modifications in failure cases (Chris)

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

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 53675d2..202ee31 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -308,6 +308,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		if (!encoder || !encoder->crtc) {
 			DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
 				      connector->base.id);
+			enabled[i] = false;
 			continue;
 		}
 
-- 
1.8.4.2

Re: [PATCH 4/8] drm/i915: read out hw state earlier

[Daniel Vetter]

On Mon, Dec 16, 2013 at 04:34:25PM -0800, Jesse Barnes wrote:
> We want to do this early on before we try to fetch the plane config,
> which depends on some of the pipe config state.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I've merged patches 2&3 from this series. This one here needs a backmerge
which atm is blocked on Dave moving drm-fixes ahead ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Chris Wilson]

On Mon, Dec 16, 2013 at 04:34:26PM -0800, Jesse Barnes wrote:
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		if (plane_config->tiled)
> +			offset = I915_READ(DSPTILEOFF(plane));
> +		else
> +			offset = I915_READ(DSPLINOFF(plane));
> +		base = I915_READ(DSPSURF(plane)) & 0xfffff000;
> +	} else {
> +		base = I915_READ(DSPADDR(plane));
> +	}
> +
> +	val = I915_READ(PIPESRC(pipe));
> +	plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
> +	plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
> +
> +	val = I915_READ(DSPSTRIDE(pipe));
> +	plane_config->fb->base.pitches[0] = val & 0xffffff80;
> +
> +	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> +				   plane_config->fb->base.height, PAGE_SIZE);

This underestimates the size of a tiled framebuffer. Height should be
aligned to 1,16,64 depending on tiling.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Chris Wilson]

On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
> @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
>  void intel_fbdev_output_poll_changed(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> +	if (dev_priv->fbdev)
> +		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>  }

Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Chris Wilson]

On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
>  int intel_fbdev_init(struct drm_device *dev)
> @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> -	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> -	if (!ifbdev)
> -		return -ENOMEM;
> +	/* This may fail, if so, dev_priv->fbdev won't be set below */
> +	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;
> +	}

Since Daniel also mentioned the same bikeshed, I think this would look
neater with the allocation here and ifbdev being passed to
intel_fbdev_init_bios to fill in:

	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
	if (ifbdev == NULL)
		return -ENODEV;
	
	if (!intel_fbdev_init_bios(dev, ifbdev)) {
		ifbdev->helper.funcs = &intel_fb_helper_funcs;
		ifbdev->preferred_bpp = 32;
	}

	dev_priv->fbdev = ifbdev;
	return 0;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Tue, 17 Dec 2013 10:03:12 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, Dec 16, 2013 at 04:34:26PM -0800, Jesse Barnes wrote:
> > +	if (INTEL_INFO(dev)->gen >= 4) {
> > +		if (plane_config->tiled)
> > +			offset = I915_READ(DSPTILEOFF(plane));
> > +		else
> > +			offset = I915_READ(DSPLINOFF(plane));
> > +		base = I915_READ(DSPSURF(plane)) & 0xfffff000;
> > +	} else {
> > +		base = I915_READ(DSPADDR(plane));
> > +	}
> > +
> > +	val = I915_READ(PIPESRC(pipe));
> > +	plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
> > +	plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
> > +
> > +	val = I915_READ(DSPSTRIDE(pipe));
> > +	plane_config->fb->base.pitches[0] = val & 0xffffff80;
> > +
> > +	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> > +				   plane_config->fb->base.height, PAGE_SIZE);
> 
> This underestimates the size of a tiled framebuffer. Height should be
> aligned to 1,16,64 depending on tiling.

Fixed, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Jesse Barnes]

On Tue, 17 Dec 2013 10:34:39 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
> > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
> >  void intel_fbdev_output_poll_changed(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> > +	if (dev_priv->fbdev)
> > +		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> >  }
> 
> Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.

Fixed.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Jesse Barnes]

On Tue, 17 Dec 2013 19:29:00 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
> >  int intel_fbdev_init(struct drm_device *dev)
> > @@ -268,17 +461,25 @@ int intel_fbdev_init(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ret;
> >  
> > -	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> > -	if (!ifbdev)
> > -		return -ENOMEM;
> > +	/* This may fail, if so, dev_priv->fbdev won't be set below */
> > +	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;
> > +	}
> 
> Since Daniel also mentioned the same bikeshed, I think this would look
> neater with the allocation here and ifbdev being passed to
> intel_fbdev_init_bios to fill in:
> 
> 	ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> 	if (ifbdev == NULL)
> 		return -ENODEV;
> 	
> 	if (!intel_fbdev_init_bios(dev, ifbdev)) {
> 		ifbdev->helper.funcs = &intel_fb_helper_funcs;
> 		ifbdev->preferred_bpp = 32;
> 	}
> 
> 	dev_priv->fbdev = ifbdev;
> 	return 0;

Yeah, looks better that way.  Fixed.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Daniel Vetter]

On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
>> > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
>> >  void intel_fbdev_output_poll_changed(struct drm_device *dev)
>> >  {
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> > -   drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>> > +   if (dev_priv->fbdev)
>> > +           drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>> >  }
>>
>> Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
>
> Fixed.

I still don't get why we need this check - for CONFIG_FB=n we have a
special dummy function and we are really careful in the setup code to
only enable the interrupt handling code once fbdev is fully set up. Or
do I miss some change here which makes this required? If so the right
fix imo would be to shuffle the init sequence again (and update all
the tons of comments about it, ofc).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Jesse Barnes]

On Tue, 17 Dec 2013 22:17:22 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
> >> > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
> >> >  void intel_fbdev_output_poll_changed(struct drm_device *dev)
> >> >  {
> >> >     struct drm_i915_private *dev_priv = dev->dev_private;
> >> > -   drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> >> > +   if (dev_priv->fbdev)
> >> > +           drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> >> >  }
> >>
> >> Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
> >
> > Fixed.
> 
> I still don't get why we need this check - for CONFIG_FB=n we have a
> special dummy function and we are really careful in the setup code to
> only enable the interrupt handling code once fbdev is fully set up. Or
> do I miss some change here which makes this required? If so the right
> fix imo would be to shuffle the init sequence again (and update all
> the tons of comments about it, ofc).

In the init code I'm more careful now to avoid leaving a bogus
pointer around:

 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
 				 INTEL_INFO(dev)->num_pipes,
 				 4);
 	if (ret) {
+		dev_priv->fbdev = NULL;
 		kfree(ifbdev);
 		return ret;
 	}

So in the unlikely event that the fb helper code fails I don't want to
fall over.

But that shouldn't happen in practice.  I only have the checks in place
to catch when I failed to set the fbdev field in one path (which is now
fixed).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Daniel Vetter]

On Tue, Dec 17, 2013 at 10:30 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> On Tue, Dec 17, 2013 at 10:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> >> On Mon, Dec 16, 2013 at 04:34:27PM -0800, Jesse Barnes wrote:
>> >> > @@ -333,7 +535,8 @@ MODULE_LICENSE("GPL and additional rights");
>> >> >  void intel_fbdev_output_poll_changed(struct drm_device *dev)
>> >> >  {
>> >> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> >> > -   drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>> >> > +   if (dev_priv->fbdev)
>> >> > +           drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
>> >> >  }
>> >>
>> >> Also intel_fbdev_restore_mode() needs the NULL fbdev safeguard.
>> >
>> > Fixed.
>>
>> I still don't get why we need this check - for CONFIG_FB=n we have a
>> special dummy function and we are really careful in the setup code to
>> only enable the interrupt handling code once fbdev is fully set up. Or
>> do I miss some change here which makes this required? If so the right
>> fix imo would be to shuffle the init sequence again (and update all
>> the tons of comments about it, ofc).
>
> In the init code I'm more careful now to avoid leaving a bogus
> pointer around:
>
>
>         ret = drm_fb_helper_init(dev, &ifbdev->helper,
>                                  INTEL_INFO(dev)->num_pipes,
>                                  4);
>         if (ret) {
> +               dev_priv->fbdev = NULL;
>                 kfree(ifbdev);
>                 return ret;
>         }
>
> So in the unlikely event that the fb helper code fails I don't want to
> fall over.
>
> But that shouldn't happen in practice.  I only have the checks in place
> to catch when I failed to set the fbdev field in one path (which is now
> fixed)

Imo if a core piece of the driver fails to initialize we should just
fail driver loading. We've had tons of crazy lore around contexts
failing, ppgtt failing and other similar stuff, and I think it just
makes the normal code more fragile with no real gain.

If you think something slips through the cracks maybe just throw a
BUG_ON in there instead.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Chris Wilson]

On Tue, Dec 17, 2013 at 10:58:51PM +0100, Daniel Vetter wrote:
> On Tue, Dec 17, 2013 at 10:30 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > So in the unlikely event that the fb helper code fails I don't want to
> > fall over.
> >
> > But that shouldn't happen in practice.  I only have the checks in place
> > to catch when I failed to set the fbdev field in one path (which is now
> > fixed)
> 
> Imo if a core piece of the driver fails to initialize we should just
> fail driver loading. We've had tons of crazy lore around contexts
> failing, ppgtt failing and other similar stuff, and I think it just
> makes the normal code more fragile with no real gain.
> 
> If you think something slips through the cracks maybe just throw a
> BUG_ON in there instead.

I tripped over it earlier when I was disabling parts of the driver.
I think allowing dev_priv->fbdev to be NULL fits nicely with your
crusade against fbcon.
(In fact it was quite fun to load the driver without modesetting as a
headless accelerator...)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/8] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Daniel Vetter]

On Tue, Dec 17, 2013 at 11:51 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Dec 17, 2013 at 10:58:51PM +0100, Daniel Vetter wrote:
>> On Tue, Dec 17, 2013 at 10:30 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > So in the unlikely event that the fb helper code fails I don't want to
>> > fall over.
>> >
>> > But that shouldn't happen in practice.  I only have the checks in place
>> > to catch when I failed to set the fbdev field in one path (which is now
>> > fixed)
>>
>> Imo if a core piece of the driver fails to initialize we should just
>> fail driver loading. We've had tons of crazy lore around contexts
>> failing, ppgtt failing and other similar stuff, and I think it just
>> makes the normal code more fragile with no real gain.
>>
>> If you think something slips through the cracks maybe just throw a
>> BUG_ON in there instead.
>
> I tripped over it earlier when I was disabling parts of the driver.
> I think allowing dev_priv->fbdev to be NULL fits nicely with your
> crusade against fbcon.
> (In fact it was quite fun to load the driver without modesetting as a
> headless accelerator...)

Well for fully disabled fbdev we nuke all those functions completely.
If we want to go with this safety net here then I think at least a
WARN_ON or something should be put in there, since this really
shouldn't happen.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch