[PATCH 1/6] drm/i915: split aligned height calculation out v2

[PATCH 1/6] drm/i915: split aligned height calculation out v2

[Jesse Barnes]

For use by get_plane_config.

v2: cleanup tile_height bits (Chris)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d4a0d9..5f0e4df 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1929,6 +1929,14 @@ static bool need_vtd_wa(struct drm_device *dev)
 	return false;
 }
 
+static int intel_align_height(struct drm_device *dev, int height, bool tiled)
+{
+	int tile_height;
+
+	tile_height = tiled ? (IS_GEN2(dev) ? 16 : 8) : 1;
+	return ALIGN(height, tile_height);
+}
+
 int
 intel_pin_and_fence_fb_obj(struct drm_device *dev,
 			   struct drm_i915_gem_object *obj,
@@ -10559,7 +10567,7 @@ int intel_framebuffer_init(struct drm_device *dev,
 			   struct drm_mode_fb_cmd2 *mode_cmd,
 			   struct drm_i915_gem_object *obj)
 {
-	int aligned_height, tile_height;
+	int aligned_height;
 	int pitch_limit;
 	int ret;
 
@@ -10653,9 +10661,8 @@ int intel_framebuffer_init(struct drm_device *dev,
 	if (mode_cmd->offsets[0] != 0)
 		return -EINVAL;
 
-	tile_height = IS_GEN2(dev) ? 16 : 8;
-	aligned_height = ALIGN(mode_cmd->height,
-			       obj->tiling_mode ? tile_height : 1);
+	aligned_height = intel_align_height(dev, mode_cmd->height,
+					    obj->tiling_mode);
 	/* FIXME drm helper for size checks (especially planar formats)? */
 	if (obj->base.size < aligned_height * mode_cmd->pitches[0])
 		return -EINVAL;
-- 
1.7.9.5

[PATCH 2/6] drm/i915: get_plane_config for i9xx v10

[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
v8: use proper height in get_plane_config (Chris)
v9: fix CONFIG_FB check for modular configs (Jani)
v10: add comment about stolen allocation stomping

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 728b9c3..bde0b47 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -393,6 +393,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;
@@ -431,6 +432,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 5f0e4df..783e785 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2033,6 +2033,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)
 {
@@ -5517,6 +5538,96 @@ 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;
+	int aligned_height;
+
+	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;
+
+	aligned_height = intel_align_height(dev, plane_config->fb->base.height,
+					    plane_config->tiled);
+
+	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
+				   aligned_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)
 {
@@ -10736,6 +10847,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;
@@ -10743,6 +10855,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;
@@ -11003,6 +11116,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);
@@ -11062,6 +11176,32 @@ 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);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
+			    base.head) {
+		if (!crtc->active)
+			continue;
+
+#if IS_ENABLED(CONFIG_FB)
+		/*
+		 * We don't have a good way of freeing the buffer w/o the FB
+		 * layer owning it...
+		 * Note that reserving the BIOS fb up front prevents us
+		 * from stuffing other stolen allocations like the ring
+		 * on top.  This prevents some ugliness at boot time, and
+		 * can even allow for smooth boot transitions if the BIOS
+		 * fb is large enough for the active pipe configuration.
+		 */
+		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 44067bc..1e5a0a6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -210,6 +210,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 {
 	int16_t cursor_width, cursor_height;
 	bool cursor_visible;
 
+	struct intel_plane_config plane_config;
 	struct intel_crtc_config config;
 	struct intel_crtc_config *new_config;
 	bool new_enabled;
@@ -727,6 +734,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.7.9.5

[PATCH 3/6] drm/i915: get_plane_config support for ILK+

[Jesse Barnes]

This should allow BIOS fb inheritance to work on ILK+ machines too.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 783e785..450bb40 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6571,6 +6571,96 @@ static void ironlake_get_pfit_config(struct intel_crtc *crtc,
 	}
 }
 
+static void ironlake_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;
+	int aligned_height;
+
+	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;
+
+	base = I915_READ(DSPSURF(plane)) & 0xfffff000;
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+		offset = I915_READ(DSPOFFSET(plane));
+	} else {
+		if (plane_config->tiled)
+			offset = I915_READ(DSPTILEOFF(plane));
+		else
+			offset = I915_READ(DSPLINOFF(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;
+
+	aligned_height = intel_align_height(dev, plane_config->fb->base.height,
+					    plane_config->tiled);
+
+	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
+				   aligned_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 ironlake_get_pipe_config(struct intel_crtc *crtc,
 				     struct intel_crtc_config *pipe_config)
 {
@@ -10833,6 +10923,7 @@ static void intel_init_display(struct drm_device *dev)
 
 	if (HAS_DDI(dev)) {
 		dev_priv->display.get_pipe_config = haswell_get_pipe_config;
+		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
@@ -10840,6 +10931,7 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.update_plane = ironlake_update_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
+		dev_priv->display.get_plane_config = ironlake_get_plane_config;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
-- 
1.7.9.5

[PATCH 4/6] drm/i915: alloc intel_fb in the intel_fbdev struct

[Jesse Barnes]

Allocate this struct instead, so we can re-use another allocated
elsewhere if needed.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 450bb40..112da42 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7975,11 +7975,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 1e5a0a6..8f5e798 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -110,7 +110,7 @@ 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;
 };
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index d6a8a71..fb07ba6 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;
@@ -126,11 +135,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	mutex_lock(&dev->struct_mutex);
 
-	if (!intel_fb->obj) {
+	if (!intel_fb || !intel_fb->obj) {
 		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
 		if (ret)
 			goto out_unlock;
+		intel_fb = ifbdev->fb;
 	} else {
 		DRM_DEBUG_KMS("re-using BIOS fb\n");
 		sizes->fb_width = intel_fb->base.width;
@@ -148,7 +158,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,7 +204,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	 * If the object is stolen however, it will be full of whatever
 	 * garbage was left in there.
 	 */
-	if (ifbdev->ifb.obj->stolen)
+	if (ifbdev->fb->obj->stolen)
 		memset_io(info->screen_base, 0, info->screen_size);
 
 	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
@@ -258,8 +268,9 @@ 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);
 }
 
 int intel_fbdev_init(struct drm_device *dev)
@@ -322,7 +333,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);
-- 
1.7.9.5

[PATCH 5/6] drm/i915: allow re-use BIOS connector config for initial fbdev config

[Jesse Barnes]

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.

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

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index fb07ba6..8ce3405 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -246,6 +246,97 @@ 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);
+			enabled[i] = false;
+			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,
-- 
1.7.9.5

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

[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)
v8: put under i915_fastboot option (Jesse)
    fix fb ptr checking (Jesse)
    inform drm_fb_helper if we fail to enable a connector (Jesse)
    drop unnecessary enabled[] modifications in failure cases (Chris)
    split from BIOS connector config readout (Daniel)
    don't memset the fb buffer if preallocated (Chris)
    alloc ifbdev up front and pass to init_bios (Chris)
    check for bad ifbdev in restore_mode too (Chris)
v9: fix up !fastboot bpp setting (Jesse)
    fix up !fastboot helper alloc (Jesse)
    make sure BIOS fb is sufficient for biggest active pipe (Jesse)
v10:fix up size calculation for proposed fbs (Chris)
    go back to two pass pipe fb assignment (Chris)
    add warning for active pipes w/o fbs (Chris)
    clean up num_pipes checks in fbdev_init and fbdev_restore_mode (Chris)
    move i915.fastboot into fbdev_init (Chris)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_drv.h   |    1 +
 drivers/gpu/drm/i915/intel_fbdev.c |  155 +++++++++++++++++++++++++++++++++---
 2 files changed, 145 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8f5e798..80e6ad2 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 *fb;
 	struct list_head fbdev_list;
 	struct drm_display_mode *our_mode;
+	int preferred_bpp;
 };
 
 struct intel_encoder {
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 8ce3405..71a57ed 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);
 
@@ -143,6 +144,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		intel_fb = ifbdev->fb;
 	} 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;
 	}
@@ -204,7 +206,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) */
@@ -319,7 +321,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		}
 
 		if (!to_intel_crtc(encoder->crtc)->active) {
-			DRM_DEBUG_KMS("connector %s on inactive crtc %d, borting\n",
+			DRM_DEBUG_KMS("connector %s on inactive crtc %d, aborting\n",
 				      drm_get_connector_name(connector),
 				      encoder->crtc->base.id);
 			return false;
@@ -364,27 +366,156 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 	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.
+ */
+static bool intel_fbdev_init_bios(struct drm_device *dev,
+				 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, max_size = 0;
+
+	if (!i915.fastboot)
+		return false;
+
+	/* Find how big the fb would need to be for the largest pipe */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		unsigned int tmp, stride;
+
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!intel_crtc->active || !intel_crtc->plane_config.fb) {
+			DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
+				      pipe_name(intel_crtc->pipe));
+			continue;
+		}
+
+		if (intel_crtc->plane_config.fb) {
+			stride = intel_crtc->plane_config.fb->base.pitches[0];
+		} else {
+			stride = intel_crtc->config.adjusted_mode.crtc_hdisplay;
+			stride *= 8; /* presumed worst case */
+		}
+		tmp = intel_crtc->config.adjusted_mode.crtc_vdisplay * stride;
+		max_size = max(max_size, tmp);
+	}
+
+	/* Find one that fits in the largest pipe, 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) {
+			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 &&
+		    intel_crtc->plane_config.size >= max_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;
+	}
+
+	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);
+	}
+
+	/* Final pass to check if any active pipes don't have fbs */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		intel_crtc = to_intel_crtc(crtc);
+
+		if (!intel_crtc->active)
+			continue;
+
+		WARN(!crtc->fb,
+		     "re-used BIOS config but lost an fb on crtc %d\n",
+		     crtc->base.id);
+	}
+
+
+	DRM_DEBUG_KMS("using BIOS fb for initial console\n");
+	return true;
+
+out_unref_obj:
+	intel_framebuffer_fini(fb);
+out:
+
+	return false;
+}
+
 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)
+	if (WARN_ON(INTEL_INFO(dev)->num_pipes == 0))
+		return -ENODEV;
+
+	ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
+	if (ifbdev == NULL)
 		return -ENOMEM;
 
-	dev_priv->fbdev = ifbdev;
-	ifbdev->helper.funcs = &intel_fb_helper_funcs;
+	if (!intel_fbdev_init_bios(dev, ifbdev)) {
+		ifbdev->helper.funcs = &intel_fb_helper_funcs;
+		ifbdev->preferred_bpp = 32;
+	}
 
 	ret = drm_fb_helper_init(dev, &ifbdev->helper,
-				 INTEL_INFO(dev)->num_pipes,
-				 4);
+				 INTEL_INFO(dev)->num_pipes, 4);
 	if (ret) {
 		kfree(ifbdev);
 		return ret;
 	}
 
+	dev_priv->fbdev = ifbdev;
 	drm_fb_helper_single_add_all_connectors(&ifbdev->helper);
 
 	return 0;
@@ -393,9 +524,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)
@@ -433,7 +565,8 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int state)
 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)
@@ -441,7 +574,7 @@ void intel_fbdev_restore_mode(struct drm_device *dev)
 	int ret;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (INTEL_INFO(dev)->num_pipes == 0)
+	if (!dev_priv->fbdev)
 		return;
 
 	drm_modeset_lock_all(dev);
-- 
1.7.9.5

Re: [PATCH 4/6] drm/i915: alloc intel_fb in the intel_fbdev struct

[Daniel Vetter]

On Fri, Feb 07, 2014 at 12:10:38PM -0800, Jesse Barnes wrote:
> Allocate this struct instead, so we can re-use another allocated
> elsewhere if needed.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    4 ++--
>  drivers/gpu/drm/i915/intel_drv.h     |    2 +-
>  drivers/gpu/drm/i915/intel_fbdev.c   |   27 +++++++++++++++++++--------
>  3 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 450bb40..112da42 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7975,11 +7975,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 1e5a0a6..8f5e798 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -110,7 +110,7 @@ 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;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index d6a8a71..fb07ba6 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;
> @@ -126,11 +135,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> -	if (!intel_fb->obj) {
> +	if (!intel_fb || !intel_fb->obj) {

The ->obj check is redundant since an fb without backing storage is a bug.
I've wrapped it up into a WARN. Note that the original ->obj check was
just a proxy for checking whether we have an fb, which isn't possible any
other way.
-Daniel

>  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>  		ret = intelfb_alloc(helper, sizes);
>  		if (ret)
>  			goto out_unlock;
> +		intel_fb = ifbdev->fb;
>  	} else {
>  		DRM_DEBUG_KMS("re-using BIOS fb\n");
>  		sizes->fb_width = intel_fb->base.width;
> @@ -148,7 +158,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,7 +204,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	 * If the object is stolen however, it will be full of whatever
>  	 * garbage was left in there.
>  	 */
> -	if (ifbdev->ifb.obj->stolen)
> +	if (ifbdev->fb->obj->stolen)
>  		memset_io(info->screen_base, 0, info->screen_size);
>  
>  	/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
> @@ -258,8 +268,9 @@ 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);
>  }
>  
>  int intel_fbdev_init(struct drm_device *dev)
> @@ -322,7 +333,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);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 1/2] drm/i915: Use normal fb deref for the fbcon framebuffer

[Daniel Vetter]

Now that it's a normally kmalloce buffer we can use the usual cleanup
paths. The upside here is that if we get the refcounting wrong will be
able to catch it, since the drm core will complain about leftover
framebuffers and kref about underflows.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index cd969c3c301e..e4f45293ccf5 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -268,9 +268,7 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 	drm_fb_helper_fini(&ifbdev->helper);
 
-	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	intel_framebuffer_fini(ifbdev->fb);
-	kfree(ifbdev->fb);
+	drm_framebuffer_unreference(&ifbdev->fb->base);
 }
 
 int intel_fbdev_init(struct drm_device *dev)
-- 
1.8.5.2

[PATCH 2/2] drm/i915: Fix error path leak in fbdev fb allocation

[Daniel Vetter]

In Jesse's patch to switch the fbdev framebuffer from an embedded
struct to a pointer the kfree in case of an error was missed. Fix this
up by using our own internal fb allocation helper directly instead of
reinventing that wheel.

We need a to_intel_framebuffer cast unfortunately since all the other
callers of _create still look better whith using a drm_framebuffer as
return pointer.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |  7 ++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  8 ++++----
 drivers/gpu/drm/i915/intel_fbdev.c   | 20 ++++++++------------
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ac0f6b5cfdfa..11c326c2c4b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7690,7 +7690,12 @@ static struct drm_display_mode load_detect_mode = {
 		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 };
 
-static struct drm_framebuffer *
+static 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);
+
+struct drm_framebuffer *
 intel_framebuffer_create(struct drm_device *dev,
 			 struct drm_mode_fb_cmd2 *mode_cmd,
 			 struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4386faf34a9b..a3978dd96ec2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -681,10 +681,10 @@ int intel_pin_and_fence_fb_obj(struct drm_device *dev,
 			       struct drm_i915_gem_object *obj,
 			       struct intel_ring_buffer *pipelined);
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj);
-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);
+struct drm_framebuffer *
+intel_framebuffer_create(struct drm_device *dev,
+			 struct drm_mode_fb_cmd2 *mode_cmd,
+			 struct drm_i915_gem_object *obj);
 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 e4f45293ccf5..ac6b50c41af1 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -62,20 +62,12 @@ 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_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;
@@ -102,13 +94,17 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	/* Flush everything out, we'll be doing GTT only from now on */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	if (ret) {
-		DRM_ERROR("failed to pin fb: %d\n", ret);
+		DRM_ERROR("failed to pin obj: %d\n", ret);
 		goto out_unref;
 	}
 
-	ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
-	if (ret)
+	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+	if (IS_ERR(fb)) {
+		ret = PTR_ERR(fb);
 		goto out_unpin;
+	}
+
+	ifbdev->fb = to_intel_framebuffer(fb);
 
 	return 0;
 
-- 
1.8.5.2

Re: [PATCH 5/6] drm/i915: allow re-use BIOS connector config for initial fbdev config

[Daniel Vetter]

On Fri, Feb 07, 2014 at 12:10:39PM -0800, Jesse Barnes wrote:
> 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.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Bunch of comments below.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_fbdev.c |   91 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index fb07ba6..8ce3405 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -246,6 +246,97 @@ 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

I'd shovel this great sequence overview into the commit message - too high
a chance it'll grow stale without getting updated. And git blame on our
intial_config can always bring it up again.

> + *
> + * 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.

This sounds like a comment for fastboot, so feels a bit misplaced here.
Maybe a simple

"Note that we don't make special consideration whether we could actually
switch to the selected modes without a full modeset. E.g. when the display
is in VGA mode we need to recalculate watermarks and set a new high-res
framebuffer anyway."

instead? Feel free to bikeshed a bit more ...

> + */
> +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);
> +			enabled[i] = false;
> +			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;
> +		}

The above two crtc->enabled/active state checks are redundant same for
checking encoder->crtc, the hw state santizer we run at boot will make
sure that if connector->encoder is set, everything else is valid (and
enabled), too. So either replace them with BUG_ONs or just remove it.

> +
> +		modes[i] = &encoder->crtc->mode;

This confused me a bit until I've realized that our fastboot code smashes
the adjusted mode into crtc->mode. I think a comment here would be good:

/*
 * IMPORTANT: We want to use the adjusted mode (i.e. after the panel
 * fitter upscaling) as the initial config, not the input mode, which is
 * what crtc->mode usually contains. But since our current fastboot code
 * puts a mode derived from the post-pfit timings into crtc->mode this
 * works out correctly.
 */

Another thing: I expect this to fall over with fastboot=0, since then
crtc->mode isn't pre-filled. We might want to call
intel_crtc_mode_from_pipe_config directly to avoid this.

> +		crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);

You need to check here whether the crtc is already in use - the bios uses
a lot more cloning configurations than we do, so if we suggest such a
cloned config the resulting errors might be surprising. Having a 1:1
connector:crtc connection is the easiest option and also what we're
currently using.

> +
> +		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,
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 1/2] drm/i915: Use normal fb deref for the fbcon framebuffer

[Daniel Vetter]

Now that it's a normally kmalloce buffer we can use the usual cleanup
paths. The upside here is that if we get the refcounting wrong will be
able to catch it, since the drm core will complain about leftover
framebuffers and kref about underflows.

v2: Kill intel_framebuffer_fini - no longer needed now that we
refcount all fbs properly and only confusing.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 11 +++--------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_fbdev.c   |  4 +---
 3 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b2faa44764b..6600931f213c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10526,18 +10526,13 @@ static void intel_setup_outputs(struct drm_device *dev)
 	drm_helper_move_panel_connectors_to_head(dev);
 }
 
-void intel_framebuffer_fini(struct intel_framebuffer *fb)
-{
-	drm_framebuffer_cleanup(&fb->base);
-	WARN_ON(!fb->obj->framebuffer_references--);
-	drm_gem_object_unreference_unlocked(&fb->obj->base);
-}
-
 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 
-	intel_framebuffer_fini(intel_fb);
+	drm_framebuffer_cleanup(fb);
+	WARN_ON(!intel_fb->obj->framebuffer_references--);
+	drm_gem_object_unreference_unlocked(&intel_fb->obj->base);
 	kfree(intel_fb);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4386faf34a9b..59348a4d0238 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -685,7 +685,6 @@ 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_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);
 void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index cd969c3c301e..e4f45293ccf5 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -268,9 +268,7 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 
 	drm_fb_helper_fini(&ifbdev->helper);
 
-	drm_framebuffer_unregister_private(&ifbdev->fb->base);
-	intel_framebuffer_fini(ifbdev->fb);
-	kfree(ifbdev->fb);
+	drm_framebuffer_unreference(&ifbdev->fb->base);
 }
 
 int intel_fbdev_init(struct drm_device *dev)
-- 
1.8.5.2

[PATCH 2/2] drm/i915: Fix error path leak in fbdev fb allocation

[Daniel Vetter]

In Jesse's patch to switch the fbdev framebuffer from an embedded
struct to a pointer the kfree in case of an error was missed. Fix this
up by using our own internal fb allocation helper directly instead of
reinventing that wheel.

We need a to_intel_framebuffer cast unfortunately since all the other
callers of _create still look better whith using a drm_framebuffer as
return pointer.

v2: Add an unlocked __intel_framebuffer_create function since our
dev->struct_mutex locking is too much a mess. With ppgtt we even need
it to take a look at the global gtt offset of pinned objects, since
the vma list might chance from underneath us. At least with the
current global gtt lookup functions. Reported by Mika.

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
 drivers/gpu/drm/i915/intel_fbdev.c   | 20 ++++++++------------
 3 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6600931f213c..6ac4c23acc77 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7690,10 +7690,15 @@ static struct drm_display_mode load_detect_mode = {
 		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
 };
 
-static struct drm_framebuffer *
-intel_framebuffer_create(struct drm_device *dev,
-			 struct drm_mode_fb_cmd2 *mode_cmd,
-			 struct drm_i915_gem_object *obj)
+static 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);
+
+struct drm_framebuffer *
+__intel_framebuffer_create(struct drm_device *dev,
+			   struct drm_mode_fb_cmd2 *mode_cmd,
+			   struct drm_i915_gem_object *obj)
 {
 	struct intel_framebuffer *intel_fb;
 	int ret;
@@ -7704,12 +7709,7 @@ intel_framebuffer_create(struct drm_device *dev,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		goto err;
-
 	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
-	mutex_unlock(&dev->struct_mutex);
 	if (ret)
 		goto err;
 
@@ -7721,6 +7721,23 @@ err:
 	return ERR_PTR(ret);
 }
 
+struct drm_framebuffer *
+intel_framebuffer_create(struct drm_device *dev,
+			 struct drm_mode_fb_cmd2 *mode_cmd,
+			 struct drm_i915_gem_object *obj)
+{
+	struct drm_framebuffer *fb;
+	int ret;
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ERR_PTR(ret);
+	fb = __intel_framebuffer_create(dev, mode_cmd, obj);
+	mutex_unlock(&dev->struct_mutex);
+
+	return fb;
+}
+
 static u32
 intel_framebuffer_pitch_for_width(int width, int bpp)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 59348a4d0238..aff9171a91d8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -681,8 +681,8 @@ int intel_pin_and_fence_fb_obj(struct drm_device *dev,
 			       struct drm_i915_gem_object *obj,
 			       struct intel_ring_buffer *pipelined);
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj);
-int intel_framebuffer_init(struct drm_device *dev,
-			   struct intel_framebuffer *ifb,
+struct drm_framebuffer *
+__intel_framebuffer_create(struct drm_device *dev,
 			   struct drm_mode_fb_cmd2 *mode_cmd,
 			   struct drm_i915_gem_object *obj);
 void intel_prepare_page_flip(struct drm_device *dev, int plane);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index e4f45293ccf5..12cc19d3eecb 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -62,20 +62,12 @@ 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_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;
@@ -102,13 +94,17 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	/* Flush everything out, we'll be doing GTT only from now on */
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	if (ret) {
-		DRM_ERROR("failed to pin fb: %d\n", ret);
+		DRM_ERROR("failed to pin obj: %d\n", ret);
 		goto out_unref;
 	}
 
-	ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
-	if (ret)
+	fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
+	if (IS_ERR(fb)) {
+		ret = PTR_ERR(fb);
 		goto out_unpin;
+	}
+
+	ifbdev->fb = to_intel_framebuffer(fb);
 
 	return 0;
 
-- 
1.8.5.2

Re: [PATCH 1/2] drm/i915: Use normal fb deref for the fbcon framebuffer

[Jesse Barnes]

On Mon, 10 Feb 2014 18:00:38 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Now that it's a normally kmalloce buffer we can use the usual cleanup
> paths. The upside here is that if we get the refcounting wrong will be
> able to catch it, since the drm core will complain about leftover
> framebuffers and kref about underflows.
> 
> v2: Kill intel_framebuffer_fini - no longer needed now that we
> refcount all fbs properly and only confusing.
> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 +++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  drivers/gpu/drm/i915/intel_fbdev.c   |  4 +---
>  3 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1b2faa44764b..6600931f213c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10526,18 +10526,13 @@ static void intel_setup_outputs(struct drm_device *dev)
>  	drm_helper_move_panel_connectors_to_head(dev);
>  }
>  
> -void intel_framebuffer_fini(struct intel_framebuffer *fb)
> -{
> -	drm_framebuffer_cleanup(&fb->base);
> -	WARN_ON(!fb->obj->framebuffer_references--);
> -	drm_gem_object_unreference_unlocked(&fb->obj->base);
> -}
> -
>  static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  {
>  	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
>  
> -	intel_framebuffer_fini(intel_fb);
> +	drm_framebuffer_cleanup(fb);
> +	WARN_ON(!intel_fb->obj->framebuffer_references--);
> +	drm_gem_object_unreference_unlocked(&intel_fb->obj->base);
>  	kfree(intel_fb);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4386faf34a9b..59348a4d0238 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -685,7 +685,6 @@ 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_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);
>  void intel_finish_page_flip_plane(struct drm_device *dev, int plane);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index cd969c3c301e..e4f45293ccf5 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -268,9 +268,7 @@ static void intel_fbdev_destroy(struct drm_device *dev,
>  
>  	drm_fb_helper_fini(&ifbdev->helper);
>  
> -	drm_framebuffer_unregister_private(&ifbdev->fb->base);
> -	intel_framebuffer_fini(ifbdev->fb);
> -	kfree(ifbdev->fb);
> +	drm_framebuffer_unreference(&ifbdev->fb->base);
>  }
>  
>  int intel_fbdev_init(struct drm_device *dev)

Yeah, this looks correct, and gets rid of one of the functions in this
maze of fb handling...

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: Fix error path leak in fbdev fb allocation

[Jesse Barnes]

On Mon, 10 Feb 2014 18:00:39 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> In Jesse's patch to switch the fbdev framebuffer from an embedded
> struct to a pointer the kfree in case of an error was missed. Fix this
> up by using our own internal fb allocation helper directly instead of
> reinventing that wheel.
> 
> We need a to_intel_framebuffer cast unfortunately since all the other
> callers of _create still look better whith using a drm_framebuffer as
> return pointer.
> 
> v2: Add an unlocked __intel_framebuffer_create function since our
> dev->struct_mutex locking is too much a mess. With ppgtt we even need
> it to take a look at the global gtt offset of pinned objects, since
> the vma list might chance from underneath us. At least with the
> current global gtt lookup functions. Reported by Mika.
> 
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
>  drivers/gpu/drm/i915/intel_fbdev.c   | 20 ++++++++------------
>  3 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6600931f213c..6ac4c23acc77 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7690,10 +7690,15 @@ static struct drm_display_mode load_detect_mode = {
>  		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
>  };
>  
> -static struct drm_framebuffer *
> -intel_framebuffer_create(struct drm_device *dev,
> -			 struct drm_mode_fb_cmd2 *mode_cmd,
> -			 struct drm_i915_gem_object *obj)
> +static 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);
> +
> +struct drm_framebuffer *
> +__intel_framebuffer_create(struct drm_device *dev,
> +			   struct drm_mode_fb_cmd2 *mode_cmd,
> +			   struct drm_i915_gem_object *obj)
>  {
>  	struct intel_framebuffer *intel_fb;
>  	int ret;
> @@ -7704,12 +7709,7 @@ intel_framebuffer_create(struct drm_device *dev,
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> -	ret = i915_mutex_lock_interruptible(dev);
> -	if (ret)
> -		goto err;
> -
>  	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
> -	mutex_unlock(&dev->struct_mutex);
>  	if (ret)
>  		goto err;
>  
> @@ -7721,6 +7721,23 @@ err:
>  	return ERR_PTR(ret);
>  }
>  
> +struct drm_framebuffer *
> +intel_framebuffer_create(struct drm_device *dev,
> +			 struct drm_mode_fb_cmd2 *mode_cmd,
> +			 struct drm_i915_gem_object *obj)
> +{
> +	struct drm_framebuffer *fb;
> +	int ret;
> +
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	fb = __intel_framebuffer_create(dev, mode_cmd, obj);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return fb;
> +}
> +
>  static u32
>  intel_framebuffer_pitch_for_width(int width, int bpp)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 59348a4d0238..aff9171a91d8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -681,8 +681,8 @@ int intel_pin_and_fence_fb_obj(struct drm_device *dev,
>  			       struct drm_i915_gem_object *obj,
>  			       struct intel_ring_buffer *pipelined);
>  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj);
> -int intel_framebuffer_init(struct drm_device *dev,
> -			   struct intel_framebuffer *ifb,
> +struct drm_framebuffer *
> +__intel_framebuffer_create(struct drm_device *dev,
>  			   struct drm_mode_fb_cmd2 *mode_cmd,
>  			   struct drm_i915_gem_object *obj);
>  void intel_prepare_page_flip(struct drm_device *dev, int plane);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index e4f45293ccf5..12cc19d3eecb 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -62,20 +62,12 @@ 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_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;
> @@ -102,13 +94,17 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	/* Flush everything out, we'll be doing GTT only from now on */
>  	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
>  	if (ret) {
> -		DRM_ERROR("failed to pin fb: %d\n", ret);
> +		DRM_ERROR("failed to pin obj: %d\n", ret);
>  		goto out_unref;
>  	}
>  
> -	ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
> -	if (ret)
> +	fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
> +	if (IS_ERR(fb)) {
> +		ret = PTR_ERR(fb);
>  		goto out_unpin;
> +	}
> +
> +	ifbdev->fb = to_intel_framebuffer(fb);
>  
>  	return 0;
>  

Yeah I think this looks ok, though I'm not sure if it makes things
clearer or not.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: Fix error path leak in fbdev fb allocation

[Daniel Vetter]

On Mon, Feb 10, 2014 at 09:47:10AM -0800, Jesse Barnes wrote:
> On Mon, 10 Feb 2014 18:00:39 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > In Jesse's patch to switch the fbdev framebuffer from an embedded
> > struct to a pointer the kfree in case of an error was missed. Fix this
> > up by using our own internal fb allocation helper directly instead of
> > reinventing that wheel.
> > 
> > We need a to_intel_framebuffer cast unfortunately since all the other
> > callers of _create still look better whith using a drm_framebuffer as
> > return pointer.
> > 
> > v2: Add an unlocked __intel_framebuffer_create function since our
> > dev->struct_mutex locking is too much a mess. With ppgtt we even need
> > it to take a look at the global gtt offset of pinned objects, since
> > the vma list might chance from underneath us. At least with the
> > current global gtt lookup functions. Reported by Mika.
> > 
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 35 ++++++++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/intel_drv.h     |  4 ++--
> >  drivers/gpu/drm/i915/intel_fbdev.c   | 20 ++++++++------------
> >  3 files changed, 36 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 6600931f213c..6ac4c23acc77 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7690,10 +7690,15 @@ static struct drm_display_mode load_detect_mode = {
> >  		 704, 832, 0, 480, 489, 491, 520, 0, DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC),
> >  };
> >  
> > -static struct drm_framebuffer *
> > -intel_framebuffer_create(struct drm_device *dev,
> > -			 struct drm_mode_fb_cmd2 *mode_cmd,
> > -			 struct drm_i915_gem_object *obj)
> > +static 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);
> > +
> > +struct drm_framebuffer *
> > +__intel_framebuffer_create(struct drm_device *dev,
> > +			   struct drm_mode_fb_cmd2 *mode_cmd,
> > +			   struct drm_i915_gem_object *obj)
> >  {
> >  	struct intel_framebuffer *intel_fb;
> >  	int ret;
> > @@ -7704,12 +7709,7 @@ intel_framebuffer_create(struct drm_device *dev,
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> >  
> > -	ret = i915_mutex_lock_interruptible(dev);
> > -	if (ret)
> > -		goto err;
> > -
> >  	ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
> > -	mutex_unlock(&dev->struct_mutex);
> >  	if (ret)
> >  		goto err;
> >  
> > @@ -7721,6 +7721,23 @@ err:
> >  	return ERR_PTR(ret);
> >  }
> >  
> > +struct drm_framebuffer *
> > +intel_framebuffer_create(struct drm_device *dev,
> > +			 struct drm_mode_fb_cmd2 *mode_cmd,
> > +			 struct drm_i915_gem_object *obj)
> > +{
> > +	struct drm_framebuffer *fb;
> > +	int ret;
> > +
> > +	ret = i915_mutex_lock_interruptible(dev);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +	fb = __intel_framebuffer_create(dev, mode_cmd, obj);
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	return fb;
> > +}
> > +
> >  static u32
> >  intel_framebuffer_pitch_for_width(int width, int bpp)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 59348a4d0238..aff9171a91d8 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -681,8 +681,8 @@ int intel_pin_and_fence_fb_obj(struct drm_device *dev,
> >  			       struct drm_i915_gem_object *obj,
> >  			       struct intel_ring_buffer *pipelined);
> >  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj);
> > -int intel_framebuffer_init(struct drm_device *dev,
> > -			   struct intel_framebuffer *ifb,
> > +struct drm_framebuffer *
> > +__intel_framebuffer_create(struct drm_device *dev,
> >  			   struct drm_mode_fb_cmd2 *mode_cmd,
> >  			   struct drm_i915_gem_object *obj);
> >  void intel_prepare_page_flip(struct drm_device *dev, int plane);
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index e4f45293ccf5..12cc19d3eecb 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -62,20 +62,12 @@ 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_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;
> > @@ -102,13 +94,17 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  	/* Flush everything out, we'll be doing GTT only from now on */
> >  	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> >  	if (ret) {
> > -		DRM_ERROR("failed to pin fb: %d\n", ret);
> > +		DRM_ERROR("failed to pin obj: %d\n", ret);
> >  		goto out_unref;
> >  	}
> >  
> > -	ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
> > -	if (ret)
> > +	fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
> > +	if (IS_ERR(fb)) {
> > +		ret = PTR_ERR(fb);
> >  		goto out_unpin;
> > +	}
> > +
> > +	ifbdev->fb = to_intel_framebuffer(fb);
> >  
> >  	return 0;
> >  
> 
> Yeah I think this looks ok, though I'm not sure if it makes things
> clearer or not.

It looked much better before I had to add the __ version due to
dev->struct_mutex madness ... Maybe we should extract the framebuffer
handling code into intel_fb.c to clear up this maze a bit. And maybe we'll
eventually get saner locking ;-)

> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Pulled in both patches, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm/i915: get_plane_config for i9xx v10

[Daniel Vetter]

On Fri, Feb 07, 2014 at 12:10:36PM -0800, Jesse Barnes wrote:
> Read out the current plane configuration at init time into a new
> plane_config structure.  This allows us to track any existing
> framebuffers attached to the plane and potentially re-use them in our
> fbdev code for a smooth handoff.
> 
> v2: update for new pitch_for_width function (Jesse)
>     comment how get_plane_config works with shared fbs (Jesse)
> 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
> v8: use proper height in get_plane_config (Chris)
> v9: fix CONFIG_FB check for modular configs (Jani)
> v10: add comment about stolen allocation stomping
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---

[snip]

> @@ -11062,6 +11176,32 @@ 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);

This smells like rebase fail - the hunk from older patches where you drop
this code seems to have been lost. Also I've originally asked you to split
this out with a bit of justification why we need this at a different place
for the fb reconstruction. I just like to give this a spent independent of
the other bits a bit, since the setup sequence is known to be really
brittle.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch