[PATCH 1/9] drm/i915: init sprites with univeral plane init function

[PATCH 1/9] drm/i915: init sprites with univeral plane init function

[Gustavo Padovan]

From: Derek Foreman <derek.foreman@collabora.co.uk>

Really just for completeness - old init function ends up making the plane
exactly the same way due to the way the enums are set up.

Signed-off-by: Derek Foreman <derek.foreman@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_sprite.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0bdb00b..4cbe286 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1375,10 +1375,10 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->plane = plane;
 	intel_plane->rotation = BIT(DRM_ROTATE_0);
 	possible_crtcs = (1 << pipe);
-	ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs,
-			     &intel_plane_funcs,
-			     plane_formats, num_plane_formats,
-			     false);
+	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
+				       &intel_plane_funcs,
+				       plane_formats, num_plane_formats,
+				       DRM_PLANE_TYPE_OVERLAY);
 	if (ret) {
 		kfree(intel_plane);
 		goto out;
-- 
1.9.3

[PATCH 2/9] drm/i915: trivial: remove unneed set to NULL

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

At this point of the code the obj var is already NULL, so we don't
need to set it again to NULL.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b2e4eac..05937fe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8253,7 +8253,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	if (!obj) {
 		DRM_DEBUG_KMS("cursor off\n");
 		addr = 0;
-		obj = NULL;
 		mutex_lock(&dev->struct_mutex);
 		goto finish;
 	}
-- 
1.9.3

[PATCH 3/9] drm/i915: fix memleak in intel_set_config_save_state()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

If the save_encoder_crtcs or save_connector_encoders allocation fail
we need to free everything we have already allocated.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 05937fe..a8a8abe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11018,13 +11018,13 @@ static int intel_set_config_save_state(struct drm_device *dev,
 		kcalloc(dev->mode_config.num_encoder,
 			sizeof(struct drm_crtc *), GFP_KERNEL);
 	if (!config->save_encoder_crtcs)
-		return -ENOMEM;
+		goto free_crtc;
 
 	config->save_connector_encoders =
 		kcalloc(dev->mode_config.num_connector,
 			sizeof(struct drm_encoder *), GFP_KERNEL);
 	if (!config->save_connector_encoders)
-		return -ENOMEM;
+		goto free_encoder;
 
 	/* Copy data. Note that driver private data is not affected.
 	 * Should anything bad happen only the expected state is
@@ -11046,6 +11046,12 @@ static int intel_set_config_save_state(struct drm_device *dev,
 	}
 
 	return 0;
+
+free_encoder:
+	kfree(config->save_encoder_crtcs);
+free_crtc:
+	kfree(config->save_crtc_enabled);
+	return -ENOMEM;
 }
 
 static void intel_set_config_restore_state(struct drm_device *dev,
-- 
1.9.3

[PATCH 4/9] drm/i915: split intel_update_plane into check() and commit()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Due to the upcoming atomic modesetting feature we need to separate
some update functions into a check step that can fail and a commit
step that should, ideally, never fail.

This commit splits intel_update_plane() and its commit part can still
fail due to the fb pinning procedure.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_sprite.c | 128 ++++++++++++++++++++++++++----------
 1 file changed, 93 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 4cbe286..b1cb606 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -844,22 +844,32 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
 	return key.flags != I915_SET_COLORKEY_NONE;
 }
 
+static bool get_visible(struct drm_rect *src, struct drm_rect *dst,
+			const struct drm_rect *clip,
+			int min_scale, int max_scale)
+{
+	int hscale, vscale;
+
+	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
+	BUG_ON(hscale < 0);
+
+	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
+	BUG_ON(vscale < 0);
+
+	return drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
+}
+
 static int
-intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+intel_check_sprite_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
 		   unsigned int crtc_w, unsigned int crtc_h,
 		   uint32_t src_x, uint32_t src_y,
 		   uint32_t src_w, uint32_t src_h)
 {
-	struct drm_device *dev = plane->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_plane *intel_plane = to_intel_plane(plane);
-	enum pipe pipe = intel_crtc->pipe;
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
-	struct drm_i915_gem_object *old_obj = intel_plane->obj;
-	int ret;
-	bool primary_enabled;
 	bool visible;
 	int hscale, vscale;
 	int max_scale, min_scale;
@@ -882,20 +892,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
 		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
-	const struct {
-		int crtc_x, crtc_y;
-		unsigned int crtc_w, crtc_h;
-		uint32_t src_x, src_y, src_w, src_h;
-	} orig = {
-		.crtc_x = crtc_x,
-		.crtc_y = crtc_y,
-		.crtc_w = crtc_w,
-		.crtc_h = crtc_h,
-		.src_x = src_x,
-		.src_y = src_y,
-		.src_w = src_w,
-		.src_h = src_h,
-	};
 
 	/* Don't modify another pipe's plane */
 	if (intel_plane->pipe != intel_crtc->pipe) {
@@ -930,13 +926,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
 			intel_plane->rotation);
 
-	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
-	BUG_ON(hscale < 0);
-
-	vscale = drm_rect_calc_vscale_relaxed(&src, &dst, min_scale, max_scale);
-	BUG_ON(vscale < 0);
-
-	visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
+	visible = get_visible(&src, &dst, &clip, max_scale, min_scale);
 
 	crtc_x = dst.x1;
 	crtc_y = dst.y1;
@@ -1027,6 +1017,54 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		}
 	}
 
+	return 0;
+}
+
+static int
+intel_commit_sprite_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+		   unsigned int crtc_w, unsigned int crtc_h,
+		   uint32_t src_x, uint32_t src_y,
+		   uint32_t src_w, uint32_t src_h)
+{
+	struct drm_device *dev = plane->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	enum pipe pipe = intel_crtc->pipe;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+	struct drm_i915_gem_object *old_obj = intel_plane->obj;
+	int ret;
+	bool primary_enabled;
+	bool visible;
+	int max_scale, min_scale;
+	struct drm_rect src = {
+		/* sample coordinates in 16.16 fixed point */
+		.x1 = src_x,
+		.x2 = src_x + src_w,
+		.y1 = src_y,
+		.y2 = src_y + src_h,
+	};
+	struct drm_rect dst = {
+		/* integer pixels */
+		.x1 = crtc_x,
+		.x2 = crtc_x + crtc_w,
+		.y1 = crtc_y,
+		.y2 = crtc_y + crtc_h,
+	};
+	const struct drm_rect clip = {
+		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
+	};
+
+	max_scale = intel_plane->max_downscale << 16;
+	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
+
+	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
+			intel_plane->rotation);
+
+	visible = get_visible(&src, &dst, &clip, max_scale, min_scale);
+
 	dst.x1 = crtc_x;
 	dst.x2 = crtc_x + crtc_w;
 	dst.y1 = crtc_y;
@@ -1055,14 +1093,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (ret)
 		return ret;
 
-	intel_plane->crtc_x = orig.crtc_x;
-	intel_plane->crtc_y = orig.crtc_y;
-	intel_plane->crtc_w = orig.crtc_w;
-	intel_plane->crtc_h = orig.crtc_h;
-	intel_plane->src_x = orig.src_x;
-	intel_plane->src_y = orig.src_y;
-	intel_plane->src_w = orig.src_w;
-	intel_plane->src_h = orig.src_h;
+	intel_plane->crtc_x = crtc_x;
+	intel_plane->crtc_y = crtc_y;
+	intel_plane->crtc_w = crtc_w;
+	intel_plane->crtc_h = crtc_h;
+	intel_plane->src_x = src_x;
+	intel_plane->src_y = src_y;
+	intel_plane->src_w = src_w;
+	intel_plane->src_h = src_h;
 	intel_plane->obj = obj;
 
 	if (intel_crtc->active) {
@@ -1109,6 +1147,26 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 }
 
 static int
+intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+		   unsigned int crtc_w, unsigned int crtc_h,
+		   uint32_t src_x, uint32_t src_y,
+		   uint32_t src_w, uint32_t src_h)
+{
+	int ret;
+
+	ret = intel_check_sprite_plane(plane, crtc, fb, crtc_x, crtc_y,
+				       crtc_w, crtc_h, src_x, src_y,
+				       src_w, src_h);
+	if (ret)
+		return ret;
+
+	return intel_commit_sprite_plane(plane, crtc, fb, crtc_x, crtc_y,
+					 crtc_w, crtc_h, src_x, src_y,
+					 src_w, src_h);
+}
+
+static int
 intel_disable_plane(struct drm_plane *plane)
 {
 	struct drm_device *dev = plane->dev;
-- 
1.9.3

[PATCH 5/9] drm/i915: split intel_cursor_plane_update() into check() and commit()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Due to the upcoming atomic modesetting feature we need to separate
some update functions into a check step that can fail and a commit
step that should, ideally, never fail.

The commit part can still fail, but that should be solved in another
upcoming patch.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 54 ++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a8a8abe..0173c53 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11742,15 +11742,13 @@ intel_cursor_plane_disable(struct drm_plane *plane)
 }
 
 static int
-intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
+intel_check_cursor_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
 			  unsigned int crtc_w, unsigned int crtc_h,
-			  uint32_t src_x, uint32_t src_y,
-			  uint32_t src_w, uint32_t src_h)
+			  uint32_t src_x, uint32_t src_y, uint32_t src_w,
+			  uint32_t src_h, bool *visible)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
-	struct drm_i915_gem_object *obj = intel_fb->obj;
 	struct drm_rect dest = {
 		/* integer pixels */
 		.x1 = crtc_x,
@@ -11770,16 +11768,23 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
 		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
-	bool visible;
-	int ret;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
-					    &src, &dest, &clip,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    DRM_PLANE_HELPER_NO_SCALING,
-					    true, true, &visible);
-	if (ret)
-		return ret;
+	return drm_plane_helper_check_update(plane, crtc, fb,
+					     &src, &dest, &clip,
+					     DRM_PLANE_HELPER_NO_SCALING,
+					     DRM_PLANE_HELPER_NO_SCALING,
+					     true, true, visible);
+}
+
+static int
+intel_commit_cursor_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			  unsigned int crtc_w, unsigned int crtc_h,
+			  bool visible)
+{
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
 
 	crtc->cursor_x = crtc_x;
 	crtc->cursor_y = crtc_y;
@@ -11794,6 +11799,27 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 		return 0;
 	}
 }
+
+static int
+intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			  unsigned int crtc_w, unsigned int crtc_h,
+			  uint32_t src_x, uint32_t src_y,
+			  uint32_t src_w, uint32_t src_h)
+{
+	bool visible;
+	int ret;
+
+	ret = intel_check_cursor_plane(plane, crtc, fb, crtc_x, crtc_y, crtc_w,
+				       crtc_h, src_x, src_y, src_w, src_h,
+				       &visible);
+	if (ret)
+		return ret;
+
+	return intel_commit_cursor_plane(plane, crtc, fb, crtc_x, crtc_y,
+					 crtc_w, crtc_h, visible);
+}
+
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
 	.update_plane = intel_cursor_plane_update,
 	.disable_plane = intel_cursor_plane_disable,
-- 
1.9.3

[PATCH 6/9] drm/i915: split intel_crtc_cursor_set_obj() into check() and commit()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Create intel_crtc_cursor_create_obj() to check any need setting
before we call intel_crtc_cursor_set_obj() to apply the cursor updates.
intel_crtc_cursor_check_obj() must always be called before
intel_crtc_cursor_set_obj().

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 115 ++++++++++++++++++++++++-----------
 1 file changed, 80 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0173c53..86c8fa3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8232,30 +8232,25 @@ static bool cursor_size_ok(struct drm_device *dev,
 }
 
 /*
- * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
+ * intel_crtc_cursor_check_obj - Check settings for a specified GEM object
  *
- * Note that the object's reference will be consumed if the update fails.  If
- * the update succeeds, the reference of the old object (if any) will be
- * consumed.
+ * Note that the object's reference will be consumed if the check fails.  If
+ * the check succeeds, the reference of the old object (if any) will be
+ * consumed in intel_crtc_cursor_set_obj(). It must be called before
+ * intel_crtc_cursor_set_obj()
  */
-static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
+static int intel_crtc_cursor_check_obj(struct drm_crtc *crtc,
 				     struct drm_i915_gem_object *obj,
 				     uint32_t width, uint32_t height)
+
 {
 	struct drm_device *dev = crtc->dev;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width, stride;
-	uint32_t addr;
+	unsigned stride;
 	int ret;
 
 	/* if we want to turn off the cursor ignore width and height */
-	if (!obj) {
-		DRM_DEBUG_KMS("cursor off\n");
-		addr = 0;
-		mutex_lock(&dev->struct_mutex);
-		goto finish;
-	}
+	if (!obj)
+		return 0;
 
 	/* Check for which cursor types we support */
 	if (!cursor_size_ok(dev, width, height)) {
@@ -8302,7 +8297,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			goto fail_unpin;
 		}
 
-		addr = i915_gem_obj_ggtt_offset(obj);
 	} else {
 		int align = IS_I830(dev) ? 16 * 1024 : 256;
 		ret = i915_gem_object_attach_phys(obj, align);
@@ -8310,8 +8304,50 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 			DRM_DEBUG_KMS("failed to attach phys object\n");
 			goto fail_locked;
 		}
-		addr = obj->phys_handle->busaddr;
 	}
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+
+fail_unpin:
+	i915_gem_object_unpin_from_display_plane(obj);
+fail_locked:
+	mutex_unlock(&dev->struct_mutex);
+fail:
+	drm_gem_object_unreference_unlocked(&obj->base);
+	return ret;
+}
+
+/*
+ * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
+ *
+ * intel_crtc_cursor_check_obj() must be called before this function
+ * so check for failures can be done before apply the update.
+ */
+static void intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
+				     struct drm_i915_gem_object *obj,
+				     uint32_t width, uint32_t height)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
+	unsigned old_width;
+	uint32_t addr;
+
+	/* if we want to turn off the cursor ignore width and height */
+	if (!obj) {
+		DRM_DEBUG_KMS("cursor off\n");
+		addr = 0;
+		mutex_lock(&dev->struct_mutex);
+		goto finish;
+	}
+
+	/* we only need to pin inside GTT if cursor is non-phy */
+	mutex_lock(&dev->struct_mutex);
+	if (!INTEL_INFO(dev)->cursor_needs_physical)
+		addr = i915_gem_obj_ggtt_offset(obj);
+	else
+		addr = obj->phys_handle->busaddr;
 
  finish:
 	if (intel_crtc->cursor_bo) {
@@ -8337,15 +8373,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	}
 
 	intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_CURSOR(pipe));
-
-	return 0;
-fail_unpin:
-	i915_gem_object_unpin_from_display_plane(obj);
-fail_locked:
-	mutex_unlock(&dev->struct_mutex);
-fail:
-	drm_gem_object_unreference_unlocked(&obj->base);
-	return ret;
 }
 
 static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
@@ -11733,12 +11760,20 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 static int
 intel_cursor_plane_disable(struct drm_plane *plane)
 {
+	int ret;
+
 	if (!plane->fb)
 		return 0;
 
 	BUG_ON(!plane->crtc);
 
-	return intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+	ret = intel_crtc_cursor_check_obj(plane->crtc, NULL, 0, 0);
+	if (ret)
+		return ret;
+
+	intel_crtc_cursor_set_obj(plane->crtc, NULL, 0, 0);
+
+	return 0;
 }
 
 static int
@@ -11749,6 +11784,7 @@ intel_check_cursor_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			  uint32_t src_h, bool *visible)
 {
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_rect dest = {
 		/* integer pixels */
 		.x1 = crtc_x,
@@ -11768,12 +11804,21 @@ intel_check_cursor_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
 		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
+	int ret;
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
-					     &src, &dest, &clip,
-					     DRM_PLANE_HELPER_NO_SCALING,
-					     DRM_PLANE_HELPER_NO_SCALING,
-					     true, true, visible);
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
+					    &src, &dest, &clip,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    DRM_PLANE_HELPER_NO_SCALING,
+					    true, true, visible);
+	if (ret)
+		return ret;
+
+	if (fb != crtc->cursor->fb)
+		ret = intel_crtc_cursor_check_obj(crtc, intel_fb->obj,
+						  crtc_w, crtc_h);
+
+	return ret;
 }
 
 static int
@@ -11789,15 +11834,15 @@ intel_commit_cursor_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	crtc->cursor_x = crtc_x;
 	crtc->cursor_y = crtc_y;
 	if (fb != crtc->cursor->fb) {
-		return intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
+		intel_crtc_cursor_set_obj(crtc, obj, crtc_w, crtc_h);
 	} else {
 		intel_crtc_update_cursor(crtc, visible);
 
 		intel_frontbuffer_flip(crtc->dev,
 				       INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe));
-
-		return 0;
 	}
+
+	return 0;
 }
 
 static int
-- 
1.9.3

[PATCH 7/9] drm/i915: split intel_primary_plane_setplane() into check() and commit()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

As a preparation for atomic updates we need to split the code to check
everything we are going to commit first. This patch starts the work to
split intel_primary_plane_setplane() into check() and commit() parts.

More work is expected on this to get a better split of the two steps.
Ideally the commit() step should never fail.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 63 ++++++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 86c8fa3..42bd6c6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11594,16 +11594,13 @@ disable_unpin:
 }
 
 static int
-intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
-			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
-			     unsigned int crtc_w, unsigned int crtc_h,
-			     uint32_t src_x, uint32_t src_y,
-			     uint32_t src_w, uint32_t src_h)
+intel_check_primary_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+			  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			  unsigned int crtc_w, unsigned int crtc_h,
+			  uint32_t src_x, uint32_t src_y, uint32_t src_w,
+			  uint32_t src_h, bool *visible)
 {
-	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
 	struct drm_rect dest = {
 		/* integer pixels */
 		.x1 = crtc_x,
@@ -11623,17 +11620,33 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
 		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
-	bool visible;
-	int ret;
 
-	ret = drm_plane_helper_check_update(plane, crtc, fb,
+	return drm_plane_helper_check_update(plane, crtc, fb,
 					    &src, &dest, &clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
-					    false, true, &visible);
+					    false, true, visible);
+}
 
-	if (ret)
-		return ret;
+static int
+intel_commit_primary_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+			   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			   unsigned int crtc_w, unsigned int crtc_h,
+			   uint32_t src_x, uint32_t src_y, uint32_t src_w,
+			   uint32_t src_h, bool visible)
+{
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->fb);
+	struct drm_rect src = {
+		/* 16.16 fixed point */
+		.x1 = src_x,
+		.y1 = src_y,
+		.x2 = src_x + src_w,
+		.y2 = src_y + src_h,
+	};
+	int ret;
 
 	/*
 	 * If the CRTC isn't enabled, we're just pinning the framebuffer,
@@ -11710,6 +11723,28 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	return 0;
 }
 
+static int
+intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
+			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			     unsigned int crtc_w, unsigned int crtc_h,
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
+{
+	bool visible;
+	int ret;
+
+	ret = intel_check_primary_plane(plane, crtc, fb, crtc_x, crtc_y,
+					crtc_w, crtc_h, src_x, src_y,
+					src_w, src_h, &visible);
+	if (ret)
+		return ret;
+
+	intel_commit_primary_plane(plane, crtc, fb, crtc_x, crtc_y, crtc_w,
+				   crtc_h, src_x, src_y, src_w, src_h, visible);
+
+	return 0;
+}
+
 /* Common destruction function for both primary and cursor planes */
 static void intel_plane_destroy(struct drm_plane *plane)
 {
-- 
1.9.3

[PATCH 8/9] drm/i915: return error if fb is NULL

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

intel_pipe_check_base() should return an error is the fb received is
set to NULL.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 42bd6c6..eb6febf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2681,7 +2681,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	/* no fb bound */
 	if (!fb) {
 		DRM_ERROR("No FB bound\n");
-		return 0;
+		return -EINVAL;
 	}
 
 	if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) {
-- 
1.9.3

[PATCH 9/9] drm/i915: split intel_pipe_set_base() into check() and commit()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Take out some parts of  code that can fail from it and move them to
intel_pipe_check_base(), the only failure point in intel_pipe_set_base()
now is the fb pinning procudure.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index eb6febf..ead2f24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2661,17 +2661,11 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
 }
 
 static int
-intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
+intel_pipe_check_base(struct drm_crtc *crtc, int x, int y,
 		    struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	enum pipe pipe = intel_crtc->pipe;
-	struct drm_framebuffer *old_fb = crtc->primary->fb;
-	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
-	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
-	int ret;
 
 	if (intel_crtc_has_pending_flip(crtc)) {
 		DRM_ERROR("pipe is still busy with an old pageflip\n");
@@ -2691,6 +2685,22 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		return -EINVAL;
 	}
 
+	return 0;
+}
+
+static int
+intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
+		    struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
+	struct drm_framebuffer *old_fb = crtc->primary->fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
+	int ret;
+
 	mutex_lock(&dev->struct_mutex);
 	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
 	if (ret == 0)
@@ -9868,6 +9878,10 @@ free_work:
 	if (ret == -EIO) {
 out_hang:
 		intel_crtc_wait_for_pending_flips(crtc);
+		ret = intel_pipe_check_base(crtc, crtc->x, crtc->y, fb);
+		if (ret)
+			return ret;
+
 		ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb);
 		if (ret == 0 && event)
 			drm_send_vblank_event(dev, pipe, event);
@@ -11396,6 +11410,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 
 		intel_crtc_wait_for_pending_flips(set->crtc);
 
+		ret = intel_pipe_check_base(set->crtc, set->x, set->y, set->fb);
+		if (ret)
+			goto fail;
+
 		ret = intel_pipe_set_base(set->crtc,
 					  set->x, set->y, set->fb);
 
@@ -11620,12 +11638,17 @@ intel_check_primary_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
 		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
+	int ret;
 
-	return drm_plane_helper_check_update(plane, crtc, fb,
+	ret = drm_plane_helper_check_update(plane, crtc, fb,
 					    &src, &dest, &clip,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    DRM_PLANE_HELPER_NO_SCALING,
 					    false, true, visible);
+	if (ret)
+		return ret;
+
+	return intel_pipe_check_base(crtc, src.x1, src.y1, fb);
 }
 
 static int
-- 
1.9.3

Re: [PATCH 2/9] drm/i915: trivial: remove unneed set to NULL

[Jani Nikula]

On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> At this point of the code the obj var is already NULL, so we don't
> need to set it again to NULL.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b2e4eac..05937fe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8253,7 +8253,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	if (!obj) {
>  		DRM_DEBUG_KMS("cursor off\n");
>  		addr = 0;
> -		obj = NULL;
>  		mutex_lock(&dev->struct_mutex);
>  		goto finish;
>  	}
> -- 
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/9] drm/i915: fix memleak in intel_set_config_save_state()

[Jani Nikula]

On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> If the save_encoder_crtcs or save_connector_encoders allocation fail
> we need to free everything we have already allocated.

There is no memleak, because the caller of intel_set_config_save_state()
checks the return value, and subsequently handles the error with a call
to intel_set_config_free(), which does the right thing.

I know one could argue this should be done within
intel_set_config_save_state() but I'm not convinced. I'd let this be as
it is.

Just in case Daniel disagrees with me here and wants to merge, the patch
does look correct. So r-b for correctness, but nak on merging from
me. ;)

BR,
Jani.


>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 05937fe..a8a8abe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11018,13 +11018,13 @@ static int intel_set_config_save_state(struct drm_device *dev,
>  		kcalloc(dev->mode_config.num_encoder,
>  			sizeof(struct drm_crtc *), GFP_KERNEL);
>  	if (!config->save_encoder_crtcs)
> -		return -ENOMEM;
> +		goto free_crtc;
>  
>  	config->save_connector_encoders =
>  		kcalloc(dev->mode_config.num_connector,
>  			sizeof(struct drm_encoder *), GFP_KERNEL);
>  	if (!config->save_connector_encoders)
> -		return -ENOMEM;
> +		goto free_encoder;
>  
>  	/* Copy data. Note that driver private data is not affected.
>  	 * Should anything bad happen only the expected state is
> @@ -11046,6 +11046,12 @@ static int intel_set_config_save_state(struct drm_device *dev,
>  	}
>  
>  	return 0;
> +
> +free_encoder:
> +	kfree(config->save_encoder_crtcs);
> +free_crtc:
> +	kfree(config->save_crtc_enabled);
> +	return -ENOMEM;
>  }
>  
>  static void intel_set_config_restore_state(struct drm_device *dev,
> -- 
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/9] drm/i915: fix memleak in intel_set_config_save_state()

[Chris Wilson]

On Fri, Aug 29, 2014 at 10:38:43AM +0300, Jani Nikula wrote:
> On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > If the save_encoder_crtcs or save_connector_encoders allocation fail
> > we need to free everything we have already allocated.
> 
> There is no memleak, because the caller of intel_set_config_save_state()
> checks the return value, and subsequently handles the error with a call
> to intel_set_config_free(), which does the right thing.
> 
> I know one could argue this should be done within
> intel_set_config_save_state() but I'm not convinced. I'd let this be as
> it is.
> 
> Just in case Daniel disagrees with me here and wants to merge, the patch
> does look correct. So r-b for correctness, but nak on merging from
> me. ;)

You just said it triggers a double free... And you would be right.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/9] drm/i915: split intel_update_plane into check() and commit()

[Ville Syrjälä]

On Thu, Aug 28, 2014 at 02:40:08PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Due to the upcoming atomic modesetting feature we need to separate
> some update functions into a check step that can fail and a commit
> step that should, ideally, never fail.
> 
> This commit splits intel_update_plane() and its commit part can still
> fail due to the fb pinning procedure.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 128 ++++++++++++++++++++++++++----------
>  1 file changed, 93 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 4cbe286..b1cb606 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -844,22 +844,32 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
>  	return key.flags != I915_SET_COLORKEY_NONE;
>  }
>  
> +static bool get_visible(struct drm_rect *src, struct drm_rect *dst,

get_visisble() is not a good name here IMO, also I think this split is at
a slightly too low level. What we really want is to start creating some
kind of plane config struct that can be passed to both check and commit,
and then check can already store all the clipped coordinates etc. to the
plane config and commit can just look them up w/o recomputing them.

Initially you could just have one such struct on the stack in
intel_update_plane() and pass it to both functions. Later we'll need to
figure out how to pass around the plane configs for all planes during
the nuclear flip, but there's no need to worry about such things quite yet.

> +			const struct drm_rect *clip,
> +			int min_scale, int max_scale)
> +{
> +	int hscale, vscale;
> +
> +	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
> +	BUG_ON(hscale < 0);
> +
> +	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
> +	BUG_ON(vscale < 0);
> +
> +	return drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
> +}
> +
>  static int
> -intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> +intel_check_sprite_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
>  		   unsigned int crtc_w, unsigned int crtc_h,
>  		   uint32_t src_x, uint32_t src_y,
>  		   uint32_t src_w, uint32_t src_h)
>  {
> -	struct drm_device *dev = plane->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	enum pipe pipe = intel_crtc->pipe;
>  	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
>  	struct drm_i915_gem_object *obj = intel_fb->obj;
> -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> -	int ret;
> -	bool primary_enabled;
>  	bool visible;
>  	int hscale, vscale;
>  	int max_scale, min_scale;
> @@ -882,20 +892,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
>  		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
>  	};
> -	const struct {
> -		int crtc_x, crtc_y;
> -		unsigned int crtc_w, crtc_h;
> -		uint32_t src_x, src_y, src_w, src_h;
> -	} orig = {
> -		.crtc_x = crtc_x,
> -		.crtc_y = crtc_y,
> -		.crtc_w = crtc_w,
> -		.crtc_h = crtc_h,
> -		.src_x = src_x,
> -		.src_y = src_y,
> -		.src_w = src_w,
> -		.src_h = src_h,
> -	};
>  
>  	/* Don't modify another pipe's plane */
>  	if (intel_plane->pipe != intel_crtc->pipe) {
> @@ -930,13 +926,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
>  			intel_plane->rotation);
>  
> -	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
> -	BUG_ON(hscale < 0);
> -
> -	vscale = drm_rect_calc_vscale_relaxed(&src, &dst, min_scale, max_scale);
> -	BUG_ON(vscale < 0);
> -
> -	visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
> +	visible = get_visible(&src, &dst, &clip, max_scale, min_scale);
>  
>  	crtc_x = dst.x1;
>  	crtc_y = dst.y1;
> @@ -1027,6 +1017,54 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		}
>  	}
>  
> +	return 0;
> +}
> +
> +static int
> +intel_commit_sprite_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> +		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +		   unsigned int crtc_w, unsigned int crtc_h,
> +		   uint32_t src_x, uint32_t src_y,
> +		   uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	enum pipe pipe = intel_crtc->pipe;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> +	int ret;
> +	bool primary_enabled;
> +	bool visible;
> +	int max_scale, min_scale;
> +	struct drm_rect src = {
> +		/* sample coordinates in 16.16 fixed point */
> +		.x1 = src_x,
> +		.x2 = src_x + src_w,
> +		.y1 = src_y,
> +		.y2 = src_y + src_h,
> +	};
> +	struct drm_rect dst = {
> +		/* integer pixels */
> +		.x1 = crtc_x,
> +		.x2 = crtc_x + crtc_w,
> +		.y1 = crtc_y,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	const struct drm_rect clip = {
> +		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> +		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
> +	};
> +
> +	max_scale = intel_plane->max_downscale << 16;
> +	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> +
> +	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> +			intel_plane->rotation);
> +
> +	visible = get_visible(&src, &dst, &clip, max_scale, min_scale);
> +
>  	dst.x1 = crtc_x;
>  	dst.x2 = crtc_x + crtc_w;
>  	dst.y1 = crtc_y;
> @@ -1055,14 +1093,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	if (ret)
>  		return ret;
>  
> -	intel_plane->crtc_x = orig.crtc_x;
> -	intel_plane->crtc_y = orig.crtc_y;
> -	intel_plane->crtc_w = orig.crtc_w;
> -	intel_plane->crtc_h = orig.crtc_h;
> -	intel_plane->src_x = orig.src_x;
> -	intel_plane->src_y = orig.src_y;
> -	intel_plane->src_w = orig.src_w;
> -	intel_plane->src_h = orig.src_h;
> +	intel_plane->crtc_x = crtc_x;
> +	intel_plane->crtc_y = crtc_y;
> +	intel_plane->crtc_w = crtc_w;
> +	intel_plane->crtc_h = crtc_h;
> +	intel_plane->src_x = src_x;
> +	intel_plane->src_y = src_y;
> +	intel_plane->src_w = src_w;
> +	intel_plane->src_h = src_h;
>  	intel_plane->obj = obj;
>  
>  	if (intel_crtc->active) {
> @@ -1109,6 +1147,26 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  }
>  
>  static int
> +intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> +		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +		   unsigned int crtc_w, unsigned int crtc_h,
> +		   uint32_t src_x, uint32_t src_y,
> +		   uint32_t src_w, uint32_t src_h)
> +{
> +	int ret;
> +
> +	ret = intel_check_sprite_plane(plane, crtc, fb, crtc_x, crtc_y,
> +				       crtc_w, crtc_h, src_x, src_y,
> +				       src_w, src_h);
> +	if (ret)
> +		return ret;
> +
> +	return intel_commit_sprite_plane(plane, crtc, fb, crtc_x, crtc_y,
> +					 crtc_w, crtc_h, src_x, src_y,
> +					 src_w, src_h);
> +}
> +
> +static int
>  intel_disable_plane(struct drm_plane *plane)
>  {
>  	struct drm_device *dev = plane->dev;
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 9/9] drm/i915: split intel_pipe_set_base() into check() and commit()

[Ville Syrjälä]

On Thu, Aug 28, 2014 at 02:40:13PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Take out some parts of  code that can fail from it and move them to
> intel_pipe_check_base(), the only failure point in intel_pipe_set_base()
> now is the fb pinning procudure.

I'd like to see intel_pipe_set_base() replaced entirely with the
primary plane setplane. Right now we have duplicated code paths all over
the place due to intel_pipe_set_base().

After that's done the same plane config treatment should be applied to
primary planes as I suggested for sprites.

> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 39 ++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eb6febf..ead2f24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2661,17 +2661,11 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>  }
>  
>  static int
> -intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> +intel_pipe_check_base(struct drm_crtc *crtc, int x, int y,
>  		    struct drm_framebuffer *fb)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
> -	struct drm_framebuffer *old_fb = crtc->primary->fb;
> -	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> -	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
> -	int ret;
>  
>  	if (intel_crtc_has_pending_flip(crtc)) {
>  		DRM_ERROR("pipe is still busy with an old pageflip\n");
> @@ -2691,6 +2685,22 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  		return -EINVAL;
>  	}
>  
> +	return 0;
> +}
> +
> +static int
> +intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> +		    struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum pipe pipe = intel_crtc->pipe;
> +	struct drm_framebuffer *old_fb = crtc->primary->fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct drm_i915_gem_object *old_obj = intel_fb_obj(old_fb);
> +	int ret;
> +
>  	mutex_lock(&dev->struct_mutex);
>  	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
>  	if (ret == 0)
> @@ -9868,6 +9878,10 @@ free_work:
>  	if (ret == -EIO) {
>  out_hang:
>  		intel_crtc_wait_for_pending_flips(crtc);
> +		ret = intel_pipe_check_base(crtc, crtc->x, crtc->y, fb);
> +		if (ret)
> +			return ret;
> +
>  		ret = intel_pipe_set_base(crtc, crtc->x, crtc->y, fb);
>  		if (ret == 0 && event)
>  			drm_send_vblank_event(dev, pipe, event);
> @@ -11396,6 +11410,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  
>  		intel_crtc_wait_for_pending_flips(set->crtc);
>  
> +		ret = intel_pipe_check_base(set->crtc, set->x, set->y, set->fb);
> +		if (ret)
> +			goto fail;
> +
>  		ret = intel_pipe_set_base(set->crtc,
>  					  set->x, set->y, set->fb);
>  
> @@ -11620,12 +11638,17 @@ intel_check_primary_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
>  		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
>  	};
> +	int ret;
>  
> -	return drm_plane_helper_check_update(plane, crtc, fb,
> +	ret = drm_plane_helper_check_update(plane, crtc, fb,
>  					    &src, &dest, &clip,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    DRM_PLANE_HELPER_NO_SCALING,
>  					    false, true, visible);
> +	if (ret)
> +		return ret;
> +
> +	return intel_pipe_check_base(crtc, src.x1, src.y1, fb);
>  }
>  
>  static int
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/9] drm/i915: fix memleak in intel_set_config_save_state()

[Jani Nikula]

On Fri, 29 Aug 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Aug 29, 2014 at 10:38:43AM +0300, Jani Nikula wrote:
>> On Thu, 28 Aug 2014, Gustavo Padovan <gustavo@padovan.org> wrote:
>> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >
>> > If the save_encoder_crtcs or save_connector_encoders allocation fail
>> > we need to free everything we have already allocated.
>> 
>> There is no memleak, because the caller of intel_set_config_save_state()
>> checks the return value, and subsequently handles the error with a call
>> to intel_set_config_free(), which does the right thing.
>> 
>> I know one could argue this should be done within
>> intel_set_config_save_state() but I'm not convinced. I'd let this be as
>> it is.
>> 
>> Just in case Daniel disagrees with me here and wants to merge, the patch
>> does look correct. So r-b for correctness, but nak on merging from
>> me. ;)
>
> You just said it triggers a double free... And you would be right.

Thanks Chris. I'll retract any hint of r-b I was giving. NAK.

BR,
Jani.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 4/9] drm/i915: split intel_update_plane into check() and commit()

[Daniel Vetter]

On Fri, Aug 29, 2014 at 10:55:41AM +0300, Ville Syrjälä wrote:
> On Thu, Aug 28, 2014 at 02:40:08PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Due to the upcoming atomic modesetting feature we need to separate
> > some update functions into a check step that can fail and a commit
> > step that should, ideally, never fail.
> > 
> > This commit splits intel_update_plane() and its commit part can still
> > fail due to the fb pinning procedure.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 128 ++++++++++++++++++++++++++----------
> >  1 file changed, 93 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 4cbe286..b1cb606 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -844,22 +844,32 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
> >  	return key.flags != I915_SET_COLORKEY_NONE;
> >  }
> >  
> > +static bool get_visible(struct drm_rect *src, struct drm_rect *dst,
> 
> get_visisble() is not a good name here IMO, also I think this split is at
> a slightly too low level. What we really want is to start creating some
> kind of plane config struct that can be passed to both check and commit,
> and then check can already store all the clipped coordinates etc. to the
> plane config and commit can just look them up w/o recomputing them.
> 
> Initially you could just have one such struct on the stack in
> intel_update_plane() and pass it to both functions. Later we'll need to
> figure out how to pass around the plane configs for all planes during
> the nuclear flip, but there's no need to worry about such things quite yet.

To avoid too much rework I think it would be good to peak at the
drm_plane_state structure in either Rob's or my atomic branch (iirc
they're the same anyway), so that we can align as much as possible with
the atomic interface.

On the crtc side for full modesets we already have intel_crtc_config,
which doesn't really fully align with drm_crtc_state. So lots of work
required there.

Otherwise I think Ville's plan to start out with an on-stack
intel_plane_config sounds sane.
-Daniel

> 
> > +			const struct drm_rect *clip,
> > +			int min_scale, int max_scale)
> > +{
> > +	int hscale, vscale;
> > +
> > +	hscale = drm_rect_calc_hscale_relaxed(src, dst, min_scale, max_scale);
> > +	BUG_ON(hscale < 0);
> > +
> > +	vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale);
> > +	BUG_ON(vscale < 0);
> > +
> > +	return drm_rect_clip_scaled(src, dst, clip, hscale, vscale);
> > +}
> > +
> >  static int
> > -intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > +intel_check_sprite_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> >  		   unsigned int crtc_w, unsigned int crtc_h,
> >  		   uint32_t src_x, uint32_t src_y,
> >  		   uint32_t src_w, uint32_t src_h)
> >  {
> > -	struct drm_device *dev = plane->dev;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct intel_plane *intel_plane = to_intel_plane(plane);
> > -	enum pipe pipe = intel_crtc->pipe;
> >  	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> >  	struct drm_i915_gem_object *obj = intel_fb->obj;
> > -	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > -	int ret;
> > -	bool primary_enabled;
> >  	bool visible;
> >  	int hscale, vscale;
> >  	int max_scale, min_scale;
> > @@ -882,20 +892,6 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> >  		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
> >  	};
> > -	const struct {
> > -		int crtc_x, crtc_y;
> > -		unsigned int crtc_w, crtc_h;
> > -		uint32_t src_x, src_y, src_w, src_h;
> > -	} orig = {
> > -		.crtc_x = crtc_x,
> > -		.crtc_y = crtc_y,
> > -		.crtc_w = crtc_w,
> > -		.crtc_h = crtc_h,
> > -		.src_x = src_x,
> > -		.src_y = src_y,
> > -		.src_w = src_w,
> > -		.src_h = src_h,
> > -	};
> >  
> >  	/* Don't modify another pipe's plane */
> >  	if (intel_plane->pipe != intel_crtc->pipe) {
> > @@ -930,13 +926,7 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> >  			intel_plane->rotation);
> >  
> > -	hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale);
> > -	BUG_ON(hscale < 0);
> > -
> > -	vscale = drm_rect_calc_vscale_relaxed(&src, &dst, min_scale, max_scale);
> > -	BUG_ON(vscale < 0);
> > -
> > -	visible = drm_rect_clip_scaled(&src, &dst, &clip, hscale, vscale);
> > +	visible = get_visible(&src, &dst, &clip, max_scale, min_scale);
> >  
> >  	crtc_x = dst.x1;
> >  	crtc_y = dst.y1;
> > @@ -1027,6 +1017,54 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  		}
> >  	}
> >  
> > +	return 0;
> > +}
> > +
> > +static int
> > +intel_commit_sprite_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > +		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> > +		   unsigned int crtc_w, unsigned int crtc_h,
> > +		   uint32_t src_x, uint32_t src_y,
> > +		   uint32_t src_w, uint32_t src_h)
> > +{
> > +	struct drm_device *dev = plane->dev;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	enum pipe pipe = intel_crtc->pipe;
> > +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +	struct drm_i915_gem_object *obj = intel_fb->obj;
> > +	struct drm_i915_gem_object *old_obj = intel_plane->obj;
> > +	int ret;
> > +	bool primary_enabled;
> > +	bool visible;
> > +	int max_scale, min_scale;
> > +	struct drm_rect src = {
> > +		/* sample coordinates in 16.16 fixed point */
> > +		.x1 = src_x,
> > +		.x2 = src_x + src_w,
> > +		.y1 = src_y,
> > +		.y2 = src_y + src_h,
> > +	};
> > +	struct drm_rect dst = {
> > +		/* integer pixels */
> > +		.x1 = crtc_x,
> > +		.x2 = crtc_x + crtc_w,
> > +		.y1 = crtc_y,
> > +		.y2 = crtc_y + crtc_h,
> > +	};
> > +	const struct drm_rect clip = {
> > +		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> > +		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
> > +	};
> > +
> > +	max_scale = intel_plane->max_downscale << 16;
> > +	min_scale = intel_plane->can_scale ? 1 : (1 << 16);
> > +
> > +	drm_rect_rotate(&src, fb->width << 16, fb->height << 16,
> > +			intel_plane->rotation);
> > +
> > +	visible = get_visible(&src, &dst, &clip, max_scale, min_scale);
> > +
> >  	dst.x1 = crtc_x;
> >  	dst.x2 = crtc_x + crtc_w;
> >  	dst.y1 = crtc_y;
> > @@ -1055,14 +1093,14 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  	if (ret)
> >  		return ret;
> >  
> > -	intel_plane->crtc_x = orig.crtc_x;
> > -	intel_plane->crtc_y = orig.crtc_y;
> > -	intel_plane->crtc_w = orig.crtc_w;
> > -	intel_plane->crtc_h = orig.crtc_h;
> > -	intel_plane->src_x = orig.src_x;
> > -	intel_plane->src_y = orig.src_y;
> > -	intel_plane->src_w = orig.src_w;
> > -	intel_plane->src_h = orig.src_h;
> > +	intel_plane->crtc_x = crtc_x;
> > +	intel_plane->crtc_y = crtc_y;
> > +	intel_plane->crtc_w = crtc_w;
> > +	intel_plane->crtc_h = crtc_h;
> > +	intel_plane->src_x = src_x;
> > +	intel_plane->src_y = src_y;
> > +	intel_plane->src_w = src_w;
> > +	intel_plane->src_h = src_h;
> >  	intel_plane->obj = obj;
> >  
> >  	if (intel_crtc->active) {
> > @@ -1109,6 +1147,26 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >  }
> >  
> >  static int
> > +intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> > +		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> > +		   unsigned int crtc_w, unsigned int crtc_h,
> > +		   uint32_t src_x, uint32_t src_y,
> > +		   uint32_t src_w, uint32_t src_h)
> > +{
> > +	int ret;
> > +
> > +	ret = intel_check_sprite_plane(plane, crtc, fb, crtc_x, crtc_y,
> > +				       crtc_w, crtc_h, src_x, src_y,
> > +				       src_w, src_h);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return intel_commit_sprite_plane(plane, crtc, fb, crtc_x, crtc_y,
> > +					 crtc_w, crtc_h, src_x, src_y,
> > +					 src_w, src_h);
> > +}
> > +
> > +static int
> >  intel_disable_plane(struct drm_plane *plane)
> >  {
> >  	struct drm_device *dev = plane->dev;
> > -- 
> > 1.9.3
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> 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/9] drm/i915: split intel_update_plane into check() and commit()

[Gustavo Padovan]

2014-08-29 Ville Syrjälä <ville.syrjala@linux.intel.com>:

> On Thu, Aug 28, 2014 at 02:40:08PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Due to the upcoming atomic modesetting feature we need to separate
> > some update functions into a check step that can fail and a commit
> > step that should, ideally, never fail.
> > 
> > This commit splits intel_update_plane() and its commit part can still
> > fail due to the fb pinning procedure.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 128 ++++++++++++++++++++++++++----------
> >  1 file changed, 93 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 4cbe286..b1cb606 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -844,22 +844,32 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
> >  	return key.flags != I915_SET_COLORKEY_NONE;
> >  }
> >  
> > +static bool get_visible(struct drm_rect *src, struct drm_rect *dst,
> 
> get_visisble() is not a good name here IMO, also I think this split is at
> a slightly too low level. What we really want is to start creating some
> kind of plane config struct that can be passed to both check and commit,
> and then check can already store all the clipped coordinates etc. to the
> plane config and commit can just look them up w/o recomputing them.

What do you really mean by too low level here? Would grabbing
struct drm_plane_state from the atomic branch fix this for you?

I'll get a v2 of these patches working with struct drm_plane_state.

	Gustavo

Re: [PATCH 4/9] drm/i915: split intel_update_plane into check() and commit()

[Ville Syrjälä]

On Fri, Aug 29, 2014 at 12:09:39PM -0300, Gustavo Padovan wrote:
> 2014-08-29 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> 
> > On Thu, Aug 28, 2014 at 02:40:08PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Due to the upcoming atomic modesetting feature we need to separate
> > > some update functions into a check step that can fail and a commit
> > > step that should, ideally, never fail.
> > > 
> > > This commit splits intel_update_plane() and its commit part can still
> > > fail due to the fb pinning procedure.
> > > 
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c | 128 ++++++++++++++++++++++++++----------
> > >  1 file changed, 93 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 4cbe286..b1cb606 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -844,22 +844,32 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
> > >  	return key.flags != I915_SET_COLORKEY_NONE;
> > >  }
> > >  
> > > +static bool get_visible(struct drm_rect *src, struct drm_rect *dst,
> > 
> > get_visisble() is not a good name here IMO, also I think this split is at
> > a slightly too low level. What we really want is to start creating some
> > kind of plane config struct that can be passed to both check and commit,
> > and then check can already store all the clipped coordinates etc. to the
> > plane config and commit can just look them up w/o recomputing them.
> 
> What do you really mean by too low level here?

I mean you just roughtly cut it in half from the middle and duplicated a
bit of the clipping logic in both halves. That's actually fairly broken
since you seem to have left the extra src coordinate frobbing (align) that
we do after clipping only to the check() part. So you'd need to yank all
that extra code to the "get_visible" function as well.

But with the plane config you can avoid doing all that work twice since
check will do it and just pass the adjusted coordinates to commit.

> Would grabbing
> struct drm_plane_state from the atomic branch fix this for you?

That looks like it's meant to house the user requested coordinates
rather than the clipped ones. What you could do is just shovel the
drm_rects we use during the clipping into a new i915 specific plane
config struct and pass that to both check and commit. We can later
look into moving that stuff into some core struct if seems like a win
for more than one driver.

> 
> I'll get a v2 of these patches working with struct drm_plane_state.
> 
> 	Gustavo

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 4/9] drm/i915: split intel_update_plane into check() and commit()

[Daniel Vetter]

[-- Attachment #1.1: Type: text/plain, Size: 949 bytes --]

On Fri, Aug 29, 2014 at 5:38 PM, Ville Syrjälä <
ville.syrjala@linux.intel.com> wrote:

> > Would grabbing
> > struct drm_plane_state from the atomic branch fix this for you?
>
> That looks like it's meant to house the user requested coordinates
> rather than the clipped ones. What you could do is just shovel the
> drm_rects we use during the clipping into a new i915 specific plane
> config struct and pass that to both check and commit. We can later
> look into moving that stuff into some core struct if seems like a win
> for more than one driver.


Creating a new intel_plane_config will also be better for merging, since if
you depend upon drm_plane_state directly your patch will be blocked. Ofc
that means we need to do a bit of refactoring once atomic has landed, but
that shouldn't be too onerous really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[-- Attachment #1.2: Type: text/html, Size: 1414 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel