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

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

[Jesse Barnes]

And move it up in the file for earlier usage.

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

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

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

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

[Jesse Barnes]

And move it up in the file for earlier usage.

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

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

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

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

[Jesse Barnes]

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

v2: fix locking (Jesse)

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

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14f250a..6569e93 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -365,6 +365,7 @@ struct drm_i915_error_state {
 
 struct intel_connector;
 struct intel_crtc_config;
+struct intel_plane_config;
 struct intel_crtc;
 struct intel_limit;
 struct dpll;
@@ -403,6 +404,8 @@ struct drm_i915_display_funcs {
 	 * fills out the pipe-config with the hw state. */
 	bool (*get_pipe_config)(struct intel_crtc *,
 				struct intel_crtc_config *);
+	void (*get_plane_config)(struct intel_crtc *,
+				 struct intel_plane_config *);
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     int x, int y,
 			     struct drm_framebuffer *old_fb);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 321d751..089128b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2002,6 +2002,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 	}
 }
 
+int intel_format_to_fourcc(int format)
+{
+	switch (format) {
+	case DISPPLANE_8BPP:
+		return DRM_FORMAT_C8;
+	case DISPPLANE_BGRX555:
+		return DRM_FORMAT_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)
 {
@@ -5474,6 +5495,95 @@ intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
 	return ALIGN(pitch, align);
 }
 
+static void i9xx_get_plane_config(struct intel_crtc *crtc,
+				  struct intel_plane_config *plane_config)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	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;
+
+	plane_config->fb->base.pitches[0] =
+		intel_framebuffer_pitch_for_width(dev_priv,
+						  plane_config->fb->base.width,
+						  plane_config->fb->base.bits_per_pixel,
+						  plane_config->tiled);
+
+	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 +10672,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 +10680,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 +10935,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);
@@ -10879,6 +10992,18 @@ void intel_modeset_init(struct drm_device *dev)
 
 	/* Just in case the BIOS is doing something questionable. */
 	intel_disable_fbc(dev);
+
+	intel_modeset_setup_hw_state(dev, false);
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
+			    base.head) {
+		if (!crtc->active)
+			continue;
+
+		if (dev_priv->display.get_plane_config)
+			dev_priv->display.get_plane_config(crtc,
+							   &crtc->plane_config);
+	}
 }
 
 static void
@@ -11248,8 +11373,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	intel_modeset_init_hw(dev);
 
 	intel_setup_overlay(dev);
-
-	intel_modeset_setup_hw_state(dev, false);
 }
 
 void intel_modeset_cleanup(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0231281..59f8f38 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 4/5] 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)

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, 223 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 089128b..dd35e74 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7830,11 +7830,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(dev_priv,
 							       mode->hdisplay,
 							       fb->bits_per_pixel,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 59f8f38..2d4faf9 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 fdb6dc9..c55d620 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;
@@ -127,7 +136,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	mutex_lock(&dev->struct_mutex);
 
 	if (!intel_fb->obj) {
-		DRM_ERROR("no BIOS fb, allocating a new one\n");
+		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
 		ret = intelfb_alloc(helper, sizes);
 		if (ret)
 			goto out_unlock;
@@ -147,7 +156,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;
@@ -193,14 +202,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);
@@ -235,6 +244,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,
@@ -257,8 +356,100 @@ 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;
+	}
+
+	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)
@@ -267,17 +458,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;
 	}
@@ -290,9 +489,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)
@@ -321,7 +521,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);
@@ -332,7 +532,8 @@ MODULE_LICENSE("GPL and additional rights");
 void intel_fbdev_output_poll_changed(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
+	if (dev_priv->fbdev)
+		drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
 }
 
 void intel_fbdev_restore_mode(struct drm_device *dev)
-- 
1.8.4.2

[PATCH 5/5] drm/i915: don't memset the fb buffer 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 c55d620..abe998c 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);
 
@@ -141,6 +142,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		if (ret)
 			goto out_unlock;
 	} else {
+		prealloc = true;
 		sizes->fb_width = intel_fb->base.width;
 		sizes->fb_height = intel_fb->base.height;
 	}
@@ -202,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->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

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

[Chris Wilson]

On Mon, Nov 25, 2013 at 03:51:15PM -0800, Jesse Barnes wrote:
> And move it up in the file for earlier usage.
> 
> v2: add pre-gen4 support as well (Chris)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Mon, Nov 25, 2013 at 03:51:17PM -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)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---

> @@ -10879,6 +10992,18 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  	/* Just in case the BIOS is doing something questionable. */
>  	intel_disable_fbc(dev);
> +
> +	intel_modeset_setup_hw_state(dev, false);
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> +			    base.head) {
> +		if (!crtc->active)
> +			continue;
> +
> +		if (dev_priv->display.get_plane_config)
> +			dev_priv->display.get_plane_config(crtc,
> +							   &crtc->plane_config);

The trick is that here if we do not retreive the current config,
including the preallocated fb, we *must* disable the output. In this
case, it would be a step in intel_sanitize_crtc() to disable the CRTC if
it is enabled but we have no preserved fb.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote:
> Retrieve current framebuffer config info from the regs and create an fb
> object for the buffer the BIOS or boot loader left us.  This should
> allow for smooth transitions to userspace apps once we finish the
> initial configuration construction.
> 
> v2: check for non-native modes and adjust (Jesse)
>     fixup aperture and cmap frees (Imre)
>     use unlocked unref if init_bios fails (Jesse)
>     fix curly brace around DSPADDR check (Imre)
>     comment failure path for pin_and_fence (Imre)
> v3: fixup fixup of aperture frees (Chris)
> v4: update to current bits (locking & pin_and_fence hack) (Jesse)
> v5: move fb config fetch to display code (Jesse)
>     re-order hw state readout on initial load to suit fb inherit (Jesse)
>     re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
> v6: rename to plane_config (Daniel)
>     check for valid object when initializing BIOS fb (Jesse)
>     split from plane_config readout and other display changes (Jesse)
>     drop use_bios_fb option (Chris)
>     update comments (Jesse)
>     rework fbdev_init_bios for clarity (Jesse)
>     drop fb obj ref under lock (Chris)
> v7: use fb object from plane_config instead (Ville)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Hmm, quietly steals plane_config->fb you mean. Other than bikeshedding
the kzalloc(intel_fbdev) and the clarity of
intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb
lifetime of plane_config->fb is extremely ugly though (the theft could
be made a little more obvious for instance) and still leaked upon failure?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Mon, Nov 25, 2013 at 03:51:19PM -0800, Jesse Barnes wrote:
> We want to preserve the BIOS/bootloader contents for later.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Decided this was better than my bikeshed,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jesse Barnes]

On Tue, 26 Nov 2013 13:57:10 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, Nov 25, 2013 at 03:51:17PM -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)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> 
> > @@ -10879,6 +10992,18 @@ void intel_modeset_init(struct drm_device *dev)
> >  
> >  	/* Just in case the BIOS is doing something questionable. */
> >  	intel_disable_fbc(dev);
> > +
> > +	intel_modeset_setup_hw_state(dev, false);
> > +
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > +			    base.head) {
> > +		if (!crtc->active)
> > +			continue;
> > +
> > +		if (dev_priv->display.get_plane_config)
> > +			dev_priv->display.get_plane_config(crtc,
> > +							   &crtc->plane_config);
> 
> The trick is that here if we do not retreive the current config,
> including the preallocated fb, we *must* disable the output. In this
> case, it would be a step in intel_sanitize_crtc() to disable the CRTC if
> it is enabled but we have no preserved fb.

Yeah, but I thought since that's been broken for awhile, it should come
in as a separate bug fix.  I can do a patch on top of this if the rest
looks ok.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Jesse Barnes]

On Tue, 26 Nov 2013 14:09:48 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote:
> > Retrieve current framebuffer config info from the regs and create an fb
> > object for the buffer the BIOS or boot loader left us.  This should
> > allow for smooth transitions to userspace apps once we finish the
> > initial configuration construction.
> > 
> > v2: check for non-native modes and adjust (Jesse)
> >     fixup aperture and cmap frees (Imre)
> >     use unlocked unref if init_bios fails (Jesse)
> >     fix curly brace around DSPADDR check (Imre)
> >     comment failure path for pin_and_fence (Imre)
> > v3: fixup fixup of aperture frees (Chris)
> > v4: update to current bits (locking & pin_and_fence hack) (Jesse)
> > v5: move fb config fetch to display code (Jesse)
> >     re-order hw state readout on initial load to suit fb inherit (Jesse)
> >     re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
> > v6: rename to plane_config (Daniel)
> >     check for valid object when initializing BIOS fb (Jesse)
> >     split from plane_config readout and other display changes (Jesse)
> >     drop use_bios_fb option (Chris)
> >     update comments (Jesse)
> >     rework fbdev_init_bios for clarity (Jesse)
> >     drop fb obj ref under lock (Chris)
> > v7: use fb object from plane_config instead (Ville)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Hmm, quietly steals plane_config->fb you mean. Other than bikeshedding
> the kzalloc(intel_fbdev) and the clarity of
> intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb
> lifetime of plane_config->fb is extremely ugly though (the theft could
> be made a little more obvious for instance) and still leaked upon failure?

I free it on failure in fb_init_bios, but maybe I missed an error path?

I agree on the lifetime handling, but it was either this or an ugly
copy of the fb object in the bios function.  And even then we'd have to
find a place to free it that made sense (there is no such place today).

I did comment the field in the struct at least...

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Ville Syrjälä]

On Mon, Nov 25, 2013 at 03:51:15PM -0800, Jesse Barnes wrote:
> And move it up in the file for earlier usage.
> 
> v2: add pre-gen4 support as well (Chris)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e85d838..321d751 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5452,6 +5452,28 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>  	pipe_config->port_clock = clock.dot / 5;
>  }
>  
> +static u32
> +intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
> +				  int bpp, bool tiled)
> +{
> +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> +	int align;
> +
> +	if (tiled) {
> +		if (INTEL_INFO(dev_priv->dev)->gen < 4) {
> +			/* Pre-965 needs power of two tile width */
> +			for (align = 512; align < pitch; align <<= 1)
> +				;
> +		} else {
> +			align = 512;
> +		}

Gen2 tiles are 128 bytes wide, not 512 bytes.

So maybe something like this:
 if (IS_GEN2()) {
   return roundup_power_of_two(max(pitch, 128));
 else if (IS_GEN3())
   return roundup_power_of_two(max(pitch, 512));
 else
   return ALIGN(pitch, 512);

> +	} else {
> +		align = 64;

Also I just noticed in the docs that gen2/3 only seem to require 32byte
alignment for linear buffers. But relaxing that would require a change
to intel_framebuffer_init() as well, so it looks like material for
another patch. Also would need to be tested on real hardware.

> +	}
> +
> +	return ALIGN(pitch, align);
> +}
> +
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  				 struct intel_crtc_config *pipe_config)
>  {
> @@ -7652,16 +7674,12 @@ err:
>  }
>  
>  static u32
> -intel_framebuffer_pitch_for_width(int width, int bpp)
> -{
> -	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> -	return ALIGN(pitch, 64);
> -}
> -
> -static u32
> -intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
> +intel_framebuffer_size_for_mode(struct drm_i915_private *dev_priv,
> +				struct drm_display_mode *mode, int bpp)
>  {
> -	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> +	u32 pitch = intel_framebuffer_pitch_for_width(dev_priv,
> +						      mode->hdisplay, bpp,
> +						      false);
>  	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);

This should also align the fb height to tile height, otherwise
intel_framebuffer_init() might reject it.

>  }
>  
> @@ -7670,18 +7688,21 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
>  				  struct drm_display_mode *mode,
>  				  int depth, int bpp)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> +	int size;
>  
> -	obj = i915_gem_alloc_object(dev,
> -				    intel_framebuffer_size_for_mode(mode, bpp));
> +	size =  intel_framebuffer_size_for_mode(dev_priv, mode, bpp);

> +	obj = i915_gem_alloc_object(dev, size);
>  	if (obj == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
>  	mode_cmd.width = mode->hdisplay;
>  	mode_cmd.height = mode->vdisplay;
> -	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
> -								bpp);
> +	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(dev_priv,
> +								mode_cmd.width,
> +								bpp, false);
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
>  
>  	return intel_framebuffer_create(dev, &mode_cmd, obj);
> @@ -7704,8 +7725,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
>  		return NULL;
>  
>  	fb = &dev_priv->fbdev->ifb.base;
> -	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
> -							       fb->bits_per_pixel))
> +	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(dev_priv,
> +							       mode->hdisplay,
> +							       fb->bits_per_pixel,
> +							       false))
>  		return NULL;
>  
>  	if (obj->base.size < mode->vdisplay * fb->pitches[0])
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Jesse Barnes]

On Tue, 26 Nov 2013 19:28:27 +0200
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Nov 25, 2013 at 03:51:15PM -0800, Jesse Barnes wrote:
> > And move it up in the file for earlier usage.
> > 
> > v2: add pre-gen4 support as well (Chris)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++++++----------
> >  1 file changed, 38 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e85d838..321d751 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5452,6 +5452,28 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> >  	pipe_config->port_clock = clock.dot / 5;
> >  }
> >  
> > +static u32
> > +intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
> > +				  int bpp, bool tiled)
> > +{
> > +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> > +	int align;
> > +
> > +	if (tiled) {
> > +		if (INTEL_INFO(dev_priv->dev)->gen < 4) {
> > +			/* Pre-965 needs power of two tile width */
> > +			for (align = 512; align < pitch; align <<= 1)
> > +				;
> > +		} else {
> > +			align = 512;
> > +		}
> 
> Gen2 tiles are 128 bytes wide, not 512 bytes.
> 
> So maybe something like this:
>  if (IS_GEN2()) {
>    return roundup_power_of_two(max(pitch, 128));
>  else if (IS_GEN3())
>    return roundup_power_of_two(max(pitch, 512));
>  else
>    return ALIGN(pitch, 512);
> 
> > +	} else {
> > +		align = 64;

Ah good old gen2.  Pretty sure we've just treated it as 512 wide in the
past?  But I'm not going to go digging around the DDX to see... :)

I was looking for that POT helper but didn't see it, that makes things
a little nicer.

> Also I just noticed in the docs that gen2/3 only seem to require 32byte
> alignment for linear buffers. But relaxing that would require a change
> to intel_framebuffer_init() as well, so it looks like material for
> another patch. Also would need to be tested on real hardware.

Yeah separate patch.  It'll save us gobs of memory. :)

> > -intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
> > +intel_framebuffer_size_for_mode(struct drm_i915_private *dev_priv,
> > +				struct drm_display_mode *mode, int bpp)
> >  {
> > -	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> > +	u32 pitch = intel_framebuffer_pitch_for_width(dev_priv,
> > +						      mode->hdisplay, bpp,
> > +						      false);
> >  	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);
> 
> This should also align the fb height to tile height, otherwise
> intel_framebuffer_init() might reject it.

Hm another existing bug.  Separate patch.

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

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

[Daniel Vetter]

On Mon, Nov 25, 2013 at 03:51:17PM -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)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   3 +
>  drivers/gpu/drm/i915/intel_display.c | 127 ++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |   8 +++
>  3 files changed, 136 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 14f250a..6569e93 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -365,6 +365,7 @@ struct drm_i915_error_state {
>  
>  struct intel_connector;
>  struct intel_crtc_config;
> +struct intel_plane_config;
>  struct intel_crtc;
>  struct intel_limit;
>  struct dpll;
> @@ -403,6 +404,8 @@ struct drm_i915_display_funcs {
>  	 * fills out the pipe-config with the hw state. */
>  	bool (*get_pipe_config)(struct intel_crtc *,
>  				struct intel_crtc_config *);
> +	void (*get_plane_config)(struct intel_crtc *,
> +				 struct intel_plane_config *);
>  	int (*crtc_mode_set)(struct drm_crtc *crtc,
>  			     int x, int y,
>  			     struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 321d751..089128b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2002,6 +2002,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
>  	}
>  }
>  
> +int intel_format_to_fourcc(int format)
> +{
> +	switch (format) {
> +	case DISPPLANE_8BPP:
> +		return DRM_FORMAT_C8;
> +	case DISPPLANE_BGRX555:
> +		return DRM_FORMAT_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)
>  {
> @@ -5474,6 +5495,95 @@ intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
>  	return ALIGN(pitch, align);
>  }
>  
> +static void i9xx_get_plane_config(struct intel_crtc *crtc,
> +				  struct intel_plane_config *plane_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	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;
> +
> +	plane_config->fb->base.pitches[0] =
> +		intel_framebuffer_pitch_for_width(dev_priv,
> +						  plane_config->fb->base.width,
> +						  plane_config->fb->base.bits_per_pixel,
> +						  plane_config->tiled);

Shouldn't we read out the stride from the respective hw register, too?

> +
> +	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> +				   plane_config->fb->base.height, PAGE_SIZE);

If you bother with tiling I think you also need to bother with correct
size alignement.

> +
> +	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 +10672,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = ironlake_update_plane;

Sad Haswell is sad it seems.

>  	} 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 +10680,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 +10935,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);
> @@ -10879,6 +10992,18 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  	/* Just in case the BIOS is doing something questionable. */
>  	intel_disable_fbc(dev);
> +
> +	intel_modeset_setup_hw_state(dev, false);
> +
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> +			    base.head) {
> +		if (!crtc->active)
> +			continue;
> +
> +		if (dev_priv->display.get_plane_config)
> +			dev_priv->display.get_plane_config(crtc,
> +							   &crtc->plane_config);
> +	}

If we go with Chris' suggestion to disable the crtc if we can't get at a
sane framebuffer for it then this would make much more sense in the
hardware state readout code. Especially since always having framebuffers
around would allow us to ditch a bit of special-casing code all over the
place.

>  }
>  
>  static void
> @@ -11248,8 +11373,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	intel_modeset_init_hw(dev);
>  
>  	intel_setup_overlay(dev);
> -
> -	intel_modeset_setup_hw_state(dev, false);
>  }
>  
>  void intel_modeset_cleanup(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0231281..59f8f38 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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Daniel Vetter]

On Tue, Nov 26, 2013 at 02:09:48PM +0000, Chris Wilson wrote:
> On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote:
> > Retrieve current framebuffer config info from the regs and create an fb
> > object for the buffer the BIOS or boot loader left us.  This should
> > allow for smooth transitions to userspace apps once we finish the
> > initial configuration construction.
> > 
> > v2: check for non-native modes and adjust (Jesse)
> >     fixup aperture and cmap frees (Imre)
> >     use unlocked unref if init_bios fails (Jesse)
> >     fix curly brace around DSPADDR check (Imre)
> >     comment failure path for pin_and_fence (Imre)
> > v3: fixup fixup of aperture frees (Chris)
> > v4: update to current bits (locking & pin_and_fence hack) (Jesse)
> > v5: move fb config fetch to display code (Jesse)
> >     re-order hw state readout on initial load to suit fb inherit (Jesse)
> >     re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
> > v6: rename to plane_config (Daniel)
> >     check for valid object when initializing BIOS fb (Jesse)
> >     split from plane_config readout and other display changes (Jesse)
> >     drop use_bios_fb option (Chris)
> >     update comments (Jesse)
> >     rework fbdev_init_bios for clarity (Jesse)
> >     drop fb obj ref under lock (Chris)
> > v7: use fb object from plane_config instead (Ville)
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Hmm, quietly steals plane_config->fb you mean. Other than bikeshedding
> the kzalloc(intel_fbdev) and the clarity of
> intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb
> lifetime of plane_config->fb is extremely ugly though (the theft could
> be made a little more obvious for instance) and still leaked upon failure?

I think the lifetime stuff for the stolen fb is actually ok. But there's
other stuff that will probably gives us some good fireworks:
- intel_crtc->plane_config seems to be left hanging in the air. Imo
  duplicating the crtc->fb pointer isnt' really all that good. I'd much
  prefer when we just shovel the invented fb into the crtc->fb pointer. Of
  course that requires us to properly adjust the refcount.
- fb->obj has a very high chance to cause utter havoc on multi-pipe
  configs, since the bios loves to set up shared framebuffers. I guess
  this is untested? For now it's probably simplest to just not bother with
  the 2nd/3rd pipe.
- We need to clean up the fbs we've created somehow - intel_framebuffer_init
  will at least register the framebuffer with the drm core. But since it's a
  driver-private fb and since we hold a reference already it'll never disappear
  I think.
- Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Mon, Nov 25, 2013 at 03:51:15PM -0800, Jesse Barnes wrote:
> And move it up in the file for earlier usage.
> 
> v2: add pre-gen4 support as well (Chris)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 53 ++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e85d838..321d751 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5452,6 +5452,28 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
>  	pipe_config->port_clock = clock.dot / 5;
>  }
>  
> +static u32
> +intel_framebuffer_pitch_for_width(struct drm_i915_private *dev_priv, int width,
> +				  int bpp, bool tiled)
> +{
> +	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> +	int align;
> +
> +	if (tiled) {
> +		if (INTEL_INFO(dev_priv->dev)->gen < 4) {
> +			/* Pre-965 needs power of two tile width */
> +			for (align = 512; align < pitch; align <<= 1)
> +				;
> +		} else {
> +			align = 512;
> +		}

Imo we should just read this out from the hardware registers, that avoids
us to have another copypasted version of some tile size limit code hanging
around.
-Daniel


> +	} else {
> +		align = 64;
> +	}
> +
> +	return ALIGN(pitch, align);
> +}
> +
>  static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  				 struct intel_crtc_config *pipe_config)
>  {
> @@ -7652,16 +7674,12 @@ err:
>  }
>  
>  static u32
> -intel_framebuffer_pitch_for_width(int width, int bpp)
> -{
> -	u32 pitch = DIV_ROUND_UP(width * bpp, 8);
> -	return ALIGN(pitch, 64);
> -}
> -
> -static u32
> -intel_framebuffer_size_for_mode(struct drm_display_mode *mode, int bpp)
> +intel_framebuffer_size_for_mode(struct drm_i915_private *dev_priv,
> +				struct drm_display_mode *mode, int bpp)
>  {
> -	u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp);
> +	u32 pitch = intel_framebuffer_pitch_for_width(dev_priv,
> +						      mode->hdisplay, bpp,
> +						      false);
>  	return ALIGN(pitch * mode->vdisplay, PAGE_SIZE);
>  }
>  
> @@ -7670,18 +7688,21 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
>  				  struct drm_display_mode *mode,
>  				  int depth, int bpp)
>  {
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_mode_fb_cmd2 mode_cmd = { 0 };
> +	int size;
>  
> -	obj = i915_gem_alloc_object(dev,
> -				    intel_framebuffer_size_for_mode(mode, bpp));
> +	size =  intel_framebuffer_size_for_mode(dev_priv, mode, bpp);
> +	obj = i915_gem_alloc_object(dev, size);
>  	if (obj == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
>  	mode_cmd.width = mode->hdisplay;
>  	mode_cmd.height = mode->vdisplay;
> -	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width,
> -								bpp);
> +	mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(dev_priv,
> +								mode_cmd.width,
> +								bpp, false);
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
>  
>  	return intel_framebuffer_create(dev, &mode_cmd, obj);
> @@ -7704,8 +7725,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
>  		return NULL;
>  
>  	fb = &dev_priv->fbdev->ifb.base;
> -	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
> -							       fb->bits_per_pixel))
> +	if (fb->pitches[0] < intel_framebuffer_pitch_for_width(dev_priv,
> +							       mode->hdisplay,
> +							       fb->bits_per_pixel,
> +							       false))
>  		return NULL;
>  
>  	if (obj->base.size < mode->vdisplay * fb->pitches[0])
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jesse Barnes]

On Tue, 26 Nov 2013 18:43:53 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:
> > +	plane_config->fb->base.pitches[0] =
> > +		intel_framebuffer_pitch_for_width(dev_priv,
> > +						  plane_config->fb->base.width,
> > +						  plane_config->fb->base.bits_per_pixel,
> > +						  plane_config->tiled);
> 
> Shouldn't we read out the stride from the respective hw register, too?

Hm that's quite an idea.

> 
> > +
> > +	plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
> > +				   plane_config->fb->base.height, PAGE_SIZE);
> 
> If you bother with tiling I think you also need to bother with correct
> size alignement.

Yeah just fixed up the framebuffer size function per Ville's comments.
I should be able to just use that.

> > @@ -10562,6 +10672,7 @@ static void intel_init_display(struct drm_device *dev)
> >  		dev_priv->display.update_plane = ironlake_update_plane;
> 
> Sad Haswell is sad it seems.

Yeah for two reasons: I'm not testing HSW at all, and I'm still fairly
ignorant of HSW display.  Well that and I don't want to pile on more
work for this patchset which has already seen a bunch of churn...

> > +		if (dev_priv->display.get_plane_config)
> > +			dev_priv->display.get_plane_config(crtc,
> > +							   &crtc->plane_config);
> > +	}
> 
> If we go with Chris' suggestion to disable the crtc if we can't get at a
> sane framebuffer for it then this would make much more sense in the
> hardware state readout code. Especially since always having framebuffers
> around would allow us to ditch a bit of special-casing code all over the
> place.

I don't think so; reading out and allocating an fb on every plane
config readout would be overkill.  We'd need to poke around in the
struct for cross checking, then free it somewhere.  Plus it won't
always be preallocated, and creating a duplicate fb when the object
still exists would fail.

This is actually another argument for simply duplicating a few fields
from the fb struct into the plane config struct.  That makes cross
checking and readout simple, and allows us to allocate if possible in
the BIOS functions.

But damnit, then I'd have to drop back to an earlier version of the
patch and lose some changes... ugg

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Jesse Barnes]

On Tue, 26 Nov 2013 18:54:04 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Nov 26, 2013 at 02:09:48PM +0000, Chris Wilson wrote:
> > On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote:
> > > Retrieve current framebuffer config info from the regs and create an fb
> > > object for the buffer the BIOS or boot loader left us.  This should
> > > allow for smooth transitions to userspace apps once we finish the
> > > initial configuration construction.
> > > 
> > > v2: check for non-native modes and adjust (Jesse)
> > >     fixup aperture and cmap frees (Imre)
> > >     use unlocked unref if init_bios fails (Jesse)
> > >     fix curly brace around DSPADDR check (Imre)
> > >     comment failure path for pin_and_fence (Imre)
> > > v3: fixup fixup of aperture frees (Chris)
> > > v4: update to current bits (locking & pin_and_fence hack) (Jesse)
> > > v5: move fb config fetch to display code (Jesse)
> > >     re-order hw state readout on initial load to suit fb inherit (Jesse)
> > >     re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
> > > v6: rename to plane_config (Daniel)
> > >     check for valid object when initializing BIOS fb (Jesse)
> > >     split from plane_config readout and other display changes (Jesse)
> > >     drop use_bios_fb option (Chris)
> > >     update comments (Jesse)
> > >     rework fbdev_init_bios for clarity (Jesse)
> > >     drop fb obj ref under lock (Chris)
> > > v7: use fb object from plane_config instead (Ville)
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > Hmm, quietly steals plane_config->fb you mean. Other than bikeshedding
> > the kzalloc(intel_fbdev) and the clarity of
> > intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb
> > lifetime of plane_config->fb is extremely ugly though (the theft could
> > be made a little more obvious for instance) and still leaked upon failure?
> 
> I think the lifetime stuff for the stolen fb is actually ok. But there's
> other stuff that will probably gives us some good fireworks:
> - intel_crtc->plane_config seems to be left hanging in the air. Imo
>   duplicating the crtc->fb pointer isnt' really all that good. I'd much
>   prefer when we just shovel the invented fb into the crtc->fb pointer. Of
>   course that requires us to properly adjust the refcount.

plane_config is just part of intel_crtc.  No hanging, the struct is
just bigger.  Saves me from dealing with alloc/free of it.

> - fb->obj has a very high chance to cause utter havoc on multi-pipe
>   configs, since the bios loves to set up shared framebuffers. I guess
>   this is untested? For now it's probably simplest to just not bother with
>   the 2nd/3rd pipe.

There should be no havoc.  The first allocation will succeed, and
following ones will fail since they overlap.  The BIOS takeover code
will then use the one that was allocated (or in the unlikely case of
multiple fbs, pick the biggest one).

Or did you see some other fail there?

> - We need to clean up the fbs we've created somehow - intel_framebuffer_init
>   will at least register the framebuffer with the drm core. But since it's a
>   driver-private fb and since we hold a reference already it'll never disappear
>   I think.

I don't think so... it should look just like a user allocated buffer
from the drm core POV.  But generally our fbdev allocations do tend to
live forever, so in that sense you're right, but it's not different
than what we have today.

-- 
Jesse Barnes, Intel Open Source Technology Center