[PATCH 0/4] drm/i915: Abort if crtc/plane init fails

[PATCH 0/4] drm/i915: Abort if crtc/plane init fails

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I recently realized that we can't really continue with loading the driver
if we fail to initialize some of the crtcs or planes. So this series makes
us fail the load in those cases. The failures would be due to kmalloc()
failing anyway, so doesn't seem too drastic to abort entirely in that case.

I also reorder things so that we'll initialize the planes in an order that
matches the new rules for handling zpos conflicts between the planes. We don't
expose the zpos property yet, but I have some preliminary patches for that
as well sitting around in a branch. Actually only VLV, CHV and pre-g4x can
dynamically adjust the zpos of the planes, for the rest it's entirely fixed.

And finally I do a bit of house cleaning in the sprite init code.

Entire series available here:
git://github.com/vsyrjala/linux.git plane_init_order

Ville Syrjälä (4):
  drm/i915: Don't try to initialize sprite planes on pre-ilk
  drm/i915: Initialize planes in a reasonable order
  drm/i915: Bail if plane/crtc init fails
  drm/i915: Reorganize sprite init

 drivers/gpu/drm/i915/i915_drv.c          |   4 +-
 drivers/gpu/drm/i915/i915_drv.h          |   2 +-
 drivers/gpu/drm/i915/intel_device_info.c |   5 +-
 drivers/gpu/drm/i915/intel_display.c     | 107 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h         |   3 +-
 drivers/gpu/drm/i915/intel_sprite.c      |  81 ++++++++++-------------
 6 files changed, 112 insertions(+), 90 deletions(-)

-- 
2.7.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Don't try to initialize sprite planes on pre-ilk

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't currently implement support for sprite planes on pre-ilk
platforms, so let's leave num_sprites at 0 so that we don't get
spurious errors during driver init.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_device_info.c | 5 +++--
 drivers/gpu/drm/i915/intel_sprite.c      | 3 ---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index d6a8f11813d5..185e3bbc9ec9 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -282,12 +282,13 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv)
 		info->num_sprites[PIPE_A] = 2;
 		info->num_sprites[PIPE_B] = 2;
 		info->num_sprites[PIPE_C] = 1;
-	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		for_each_pipe(dev_priv, pipe)
 			info->num_sprites[pipe] = 2;
-	else
+	} else if (INTEL_GEN(dev_priv) >= 5) {
 		for_each_pipe(dev_priv, pipe)
 			info->num_sprites[pipe] = 1;
+	}
 
 	if (i915.disable_display) {
 		DRM_INFO("Display disabled (module parameter)\n");
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 43d0350856e7..ae1e3d47951b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1054,9 +1054,6 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	int num_plane_formats;
 	int ret;
 
-	if (INTEL_INFO(dev)->gen < 5)
-		return -ENODEV;
-
 	intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
 	if (!intel_plane) {
 		ret = -ENOMEM;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Initialize planes in a reasonable order

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The zpos magic sorting uses the object ID to solve conflicting zpos
values. Let's initialize our planes in an order that makes the object
IDs agree with the normal primary->sprites->cursor z order.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2a5a7c2868de..244a0f59d8f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15272,7 +15272,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	struct intel_crtc_state *crtc_state = NULL;
 	struct drm_plane *primary = NULL;
 	struct drm_plane *cursor = NULL;
-	int ret;
+	int sprite, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
@@ -15299,6 +15299,13 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	if (!primary)
 		goto fail;
 
+	for_each_sprite(dev_priv, pipe, sprite) {
+		ret = intel_plane_init(dev, pipe, sprite);
+		if (ret)
+			DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
+				      pipe_name(pipe), sprite_name(pipe, sprite), ret);
+	}
+
 	cursor = intel_cursor_plane_create(dev, pipe);
 	if (!cursor)
 		goto fail;
@@ -16423,7 +16430,6 @@ void intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
-	int sprite, ret;
 	enum pipe pipe;
 	struct intel_crtc *crtc;
 
@@ -16494,12 +16500,6 @@ void intel_modeset_init(struct drm_device *dev)
 
 	for_each_pipe(dev_priv, pipe) {
 		intel_crtc_init(dev, pipe);
-		for_each_sprite(dev_priv, pipe, sprite) {
-			ret = intel_plane_init(dev, pipe, sprite);
-			if (ret)
-				DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
-					      pipe_name(pipe), sprite_name(pipe, sprite), ret);
-		}
 	}
 
 	intel_update_czclk(dev_priv);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Bail if plane/crtc init fails

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Due to the plane->index not getting readjusted in drm_plane_cleanup(),
we can't continue initialization of some plane/crtc init fails.
Well, we sort of could I suppose if we left all initialized planes on
the list, but that would expose those planes to userspace as well.

But for crtcs the situation is even worse since we assume that
pipe==crtc index occasionally, so we can't really deal with a partially
initialize set of crtcs.

So seems safest to just abort the entire thing if anything goes wrong.
All the failure paths here are kmalloc()s anyway, so it seems unlikely
we'd get very far if these start failing.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |   4 +-
 drivers/gpu/drm/i915/i915_drv.h      |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 101 ++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +-
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +--
 5 files changed, 75 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index af3559d34328..0e393b5a988a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -592,7 +592,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	/* Important: The output setup functions called by modeset_init need
 	 * working irqs for e.g. gmbus and dp aux transfers. */
-	intel_modeset_init(dev);
+	ret = intel_modeset_init(dev);
+	if (ret)
+		goto cleanup_irq;
 
 	intel_guc_init(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7a621c74254e..a9308aeb2f1f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3700,7 +3700,7 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv);
 
 /* modesetting */
 extern void intel_modeset_init_hw(struct drm_device *dev);
-extern void intel_modeset_init(struct drm_device *dev);
+extern int intel_modeset_init(struct drm_device *dev);
 extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_connector_register(struct drm_connector *);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 244a0f59d8f7..d5a49d12dc88 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14975,9 +14975,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
  */
 void intel_plane_destroy(struct drm_plane *plane)
 {
-	if (!plane)
-		return;
-
 	drm_plane_cleanup(plane);
 	kfree(to_intel_plane(plane));
 }
@@ -14991,11 +14988,10 @@ const struct drm_plane_funcs intel_plane_funcs = {
 	.atomic_set_property = intel_plane_atomic_set_property,
 	.atomic_duplicate_state = intel_plane_duplicate_state,
 	.atomic_destroy_state = intel_plane_destroy_state,
-
 };
 
-static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
-						    int pipe)
+static struct intel_plane *
+intel_primary_plane_create(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *primary = NULL;
@@ -15006,12 +15002,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	int ret;
 
 	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
-	if (!primary)
+	if (!primary) {
+		ret = -ENOMEM;
 		goto fail;
+	}
 
 	state = intel_create_plane_state(&primary->base);
-	if (!state)
+	if (!state) {
+		ret = -ENOMEM;
 		goto fail;
+	}
+
 	primary->base.state = &state->base;
 
 	primary->can_scale = false;
@@ -15092,13 +15093,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 
 	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
 
-	return &primary->base;
+	return primary;
 
 fail:
 	kfree(state);
 	kfree(primary);
 
-	return NULL;
+	return ERR_PTR(ret);
 }
 
 static int
@@ -15194,8 +15195,8 @@ intel_update_cursor_plane(struct drm_plane *plane,
 	intel_crtc_update_cursor(crtc, state);
 }
 
-static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
-						   int pipe)
+static struct intel_plane *
+intel_cursor_plane_create(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *cursor = NULL;
@@ -15203,12 +15204,17 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	int ret;
 
 	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
-	if (!cursor)
+	if (!cursor) {
+		ret = -ENOMEM;
 		goto fail;
+	}
 
 	state = intel_create_plane_state(&cursor->base);
-	if (!state)
+	if (!state) {
+		ret = -ENOMEM;
 		goto fail;
+	}
+
 	cursor->base.state = &state->base;
 
 	cursor->can_scale = false;
@@ -15240,13 +15246,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 
 	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
 
-	return &cursor->base;
+	return cursor;
 
 fail:
 	kfree(state);
 	kfree(cursor);
 
-	return NULL;
+	return ERR_PTR(ret);
 }
 
 static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
@@ -15265,22 +15271,24 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
 	scaler_state->scaler_id = -1;
 }
 
-static void intel_crtc_init(struct drm_device *dev, int pipe)
+static int intel_crtc_init(struct drm_device *dev, enum pipe pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc;
 	struct intel_crtc_state *crtc_state = NULL;
-	struct drm_plane *primary = NULL;
-	struct drm_plane *cursor = NULL;
+	struct intel_plane *primary = NULL;
+	struct intel_plane *cursor = NULL;
 	int sprite, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
-	if (intel_crtc == NULL)
-		return;
+	if (!intel_crtc)
+		return -ENOMEM;
 
 	crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
-	if (!crtc_state)
+	if (!crtc_state) {
+		ret = -ENOMEM;
 		goto fail;
+	}
 	intel_crtc->config = crtc_state;
 	intel_crtc->base.state = &crtc_state->base;
 	crtc_state->base.crtc = &intel_crtc->base;
@@ -15296,22 +15304,30 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	}
 
 	primary = intel_primary_plane_create(dev, pipe);
-	if (!primary)
+	if (IS_ERR(primary)) {
+		ret = PTR_ERR(primary);
 		goto fail;
+	}
 
 	for_each_sprite(dev_priv, pipe, sprite) {
-		ret = intel_plane_init(dev, pipe, sprite);
-		if (ret)
-			DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
-				      pipe_name(pipe), sprite_name(pipe, sprite), ret);
+		struct intel_plane *plane;
+
+		plane = intel_sprite_plane_create(dev, pipe, sprite);
+		if (!plane) {
+			ret = PTR_ERR(plane);
+			goto fail;
+		}
 	}
 
 	cursor = intel_cursor_plane_create(dev, pipe);
-	if (!cursor)
+	if (!cursor) {
+		ret = PTR_ERR(cursor);
 		goto fail;
+	}
 
-	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
-					cursor, &intel_crtc_funcs,
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base,
+					&primary->base, &cursor->base,
+					&intel_crtc_funcs,
 					"pipe %c", pipe_name(pipe));
 	if (ret)
 		goto fail;
@@ -15343,13 +15359,18 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_color_init(&intel_crtc->base);
 
 	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
-	return;
+
+	return 0;
 
 fail:
-	intel_plane_destroy(primary);
-	intel_plane_destroy(cursor);
+	/*
+	 * drm_mode_config_cleanup() will free up any
+	 * crtcs/planes already initialized.
+	 */
 	kfree(crtc_state);
 	kfree(intel_crtc);
+
+	return ret;
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
@@ -16426,7 +16447,7 @@ static void sanitize_watermarks(struct drm_device *dev)
 	drm_modeset_acquire_fini(&ctx);
 }
 
-void intel_modeset_init(struct drm_device *dev)
+int intel_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
@@ -16450,7 +16471,7 @@ void intel_modeset_init(struct drm_device *dev)
 	intel_init_pm(dev);
 
 	if (INTEL_INFO(dev)->num_pipes == 0)
-		return;
+		return 0;
 
 	/*
 	 * There may be no VBT; and if the BIOS enabled SSC we can
@@ -16499,7 +16520,13 @@ void intel_modeset_init(struct drm_device *dev)
 		      INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
 
 	for_each_pipe(dev_priv, pipe) {
-		intel_crtc_init(dev, pipe);
+		int ret;
+
+		ret = intel_crtc_init(dev, pipe);
+		if (ret) {
+			drm_mode_config_cleanup(dev);
+			return ret;
+		}
 	}
 
 	intel_update_czclk(dev_priv);
@@ -16547,6 +16574,8 @@ void intel_modeset_init(struct drm_device *dev)
 	 * since the watermark calculation done here will use pstate->fb.
 	 */
 	sanitize_watermarks(dev);
+
+	return 0;
 }
 
 static void intel_enable_pipe_a(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7dda79df55d0..4fd7d74fd6dc 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1779,7 +1779,8 @@ bool intel_sdvo_init(struct drm_device *dev,
 /* intel_sprite.c */
 int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
 			     int usecs);
-int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
+struct intel_plane *intel_sprite_plane_create(struct drm_device *dev,
+					      enum pipe pipe, int plane);
 int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 			      struct drm_file *file_priv);
 void intel_pipe_update_start(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index ae1e3d47951b..41ae7f562eec 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1042,8 +1042,8 @@ static uint32_t skl_plane_formats[] = {
 	DRM_FORMAT_VYUY,
 };
 
-int
-intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
+struct intel_plane *
+intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = NULL;
@@ -1160,11 +1160,11 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 
 	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
 
-	return 0;
+	return intel_plane;
 
 fail:
 	kfree(state);
 	kfree(intel_plane);
 
-	return ret;
+	return ERR_PTR(ret);
 }
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Reorganize sprite init

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Kill the switch statement from the sprite init code and replace with a
more straightforward if ladder. Now each significant evolution of the
sprite hardware is in its own neat box.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 70 ++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 41ae7f562eec..70b50a27763e 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1067,25 +1067,25 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
 	}
 	intel_plane->base.state = &state->base;
 
-	switch (INTEL_INFO(dev)->gen) {
-	case 5:
-	case 6:
+	if (INTEL_GEN(dev_priv) >= 9) {
 		intel_plane->can_scale = true;
-		intel_plane->max_downscale = 16;
-		intel_plane->update_plane = ilk_update_plane;
-		intel_plane->disable_plane = ilk_disable_plane;
+		state->scaler_id = -1;
 
-		if (IS_GEN6(dev_priv)) {
-			plane_formats = snb_plane_formats;
-			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
-		} else {
-			plane_formats = ilk_plane_formats;
-			num_plane_formats = ARRAY_SIZE(ilk_plane_formats);
-		}
-		break;
+		intel_plane->update_plane = skl_update_plane;
+		intel_plane->disable_plane = skl_disable_plane;
+
+		plane_formats = skl_plane_formats;
+		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
+	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		intel_plane->can_scale = false;
+		intel_plane->max_downscale = 1;
+
+		intel_plane->update_plane = vlv_update_plane;
+		intel_plane->disable_plane = vlv_disable_plane;
 
-	case 7:
-	case 8:
+		plane_formats = vlv_plane_formats;
+		num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
+	} else if (INTEL_GEN(dev_priv) >= 7) {
 		if (IS_IVYBRIDGE(dev_priv)) {
 			intel_plane->can_scale = true;
 			intel_plane->max_downscale = 2;
@@ -1094,33 +1094,25 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
 			intel_plane->max_downscale = 1;
 		}
 
-		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
-			intel_plane->update_plane = vlv_update_plane;
-			intel_plane->disable_plane = vlv_disable_plane;
+		intel_plane->update_plane = ivb_update_plane;
+		intel_plane->disable_plane = ivb_disable_plane;
 
-			plane_formats = vlv_plane_formats;
-			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
-		} else {
-			intel_plane->update_plane = ivb_update_plane;
-			intel_plane->disable_plane = ivb_disable_plane;
+		plane_formats = snb_plane_formats;
+		num_plane_formats = ARRAY_SIZE(snb_plane_formats);
+	} else {
+		intel_plane->can_scale = true;
+		intel_plane->max_downscale = 16;
+
+		intel_plane->update_plane = ilk_update_plane;
+		intel_plane->disable_plane = ilk_disable_plane;
 
+		if (IS_GEN6(dev_priv)) {
 			plane_formats = snb_plane_formats;
 			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
+		} else {
+			plane_formats = ilk_plane_formats;
+			num_plane_formats = ARRAY_SIZE(ilk_plane_formats);
 		}
-		break;
-	case 9:
-		intel_plane->can_scale = true;
-		intel_plane->update_plane = skl_update_plane;
-		intel_plane->disable_plane = skl_disable_plane;
-		state->scaler_id = -1;
-
-		plane_formats = skl_plane_formats;
-		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
-		break;
-	default:
-		MISSING_CASE(INTEL_INFO(dev)->gen);
-		ret = -ENODEV;
-		goto fail;
 	}
 
 	if (INTEL_GEN(dev_priv) >= 9) {
@@ -1139,7 +1131,7 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
 
 	possible_crtcs = (1 << pipe);
 
-	if (INTEL_INFO(dev)->gen >= 9)
+	if (INTEL_GEN(dev_priv) >= 9)
 		ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
 					       &intel_plane_funcs,
 					       plane_formats, num_plane_formats,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/4] drm/i915: Don't try to initialize sprite planes on pre-ilk

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Don't try to initialize sprite planes on pre-ilk
URL   : https://patchwork.freedesktop.org/series/14347/
State : warning

== Summary ==

Series 14347v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/14347/revisions/1/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6770hq)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (fi-skl-6770hq)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> SKIP       (fi-byt-n2820)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                fail       -> PASS       (fi-skl-6770hq)

fi-bdw-5557u     total:246  pass:231  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:246  pass:204  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:246  pass:216  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-n2820     total:246  pass:210  dwarn:0   dfail:0   fail:0   skip:36 
fi-hsw-4770      total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:246  pass:223  dwarn:0   dfail:0   fail:0   skip:23 
fi-ilk-650       total:246  pass:185  dwarn:0   dfail:0   fail:0   skip:61 
fi-ivb-3520m     total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-ivb-3770      total:246  pass:220  dwarn:0   dfail:0   fail:0   skip:26 
fi-kbl-7200u     total:246  pass:222  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6260u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:246  pass:219  dwarn:4   dfail:0   fail:0   skip:23 
fi-skl-6700k     total:246  pass:222  dwarn:1   dfail:0   fail:0   skip:23 
fi-skl-6770hq    total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:246  pass:209  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:246  pass:208  dwarn:0   dfail:0   fail:0   skip:38 

3ce99d02d8f2b7d1414d20d5972f8e277b33693e drm-intel-nightly: 2016y-10m-25d-13h-55m-46s UTC integration manifest
1d8506a drm/i915: Reorganize sprite init
28f60dd drm/i915: Bail if plane/crtc init fails
df93569 drm/i915: Initialize planes in a reasonable order
59d4786 drm/i915: Don't try to initialize sprite planes on pre-ilk

Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2813/

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2813/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Bail if plane/crtc init fails

[Daniel Vetter]

On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Due to the plane->index not getting readjusted in drm_plane_cleanup(),
> we can't continue initialization of some plane/crtc init fails.
> Well, we sort of could I suppose if we left all initialized planes on
> the list, but that would expose those planes to userspace as well.
> 
> But for crtcs the situation is even worse since we assume that
> pipe==crtc index occasionally, so we can't really deal with a partially
> initialize set of crtcs.
> 
> So seems safest to just abort the entire thing if anything goes wrong.
> All the failure paths here are kmalloc()s anyway, so it seems unlikely
> we'd get very far if these start failing.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Some bikeshedding in this patch, but I like it. For patches 1-3:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |   4 +-
>  drivers/gpu/drm/i915/i915_drv.h      |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 101 ++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +--
>  5 files changed, 75 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index af3559d34328..0e393b5a988a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -592,7 +592,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  
>  	/* Important: The output setup functions called by modeset_init need
>  	 * working irqs for e.g. gmbus and dp aux transfers. */
> -	intel_modeset_init(dev);
> +	ret = intel_modeset_init(dev);
> +	if (ret)
> +		goto cleanup_irq;
>  
>  	intel_guc_init(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7a621c74254e..a9308aeb2f1f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3700,7 +3700,7 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv);
>  
>  /* modesetting */
>  extern void intel_modeset_init_hw(struct drm_device *dev);
> -extern void intel_modeset_init(struct drm_device *dev);
> +extern int intel_modeset_init(struct drm_device *dev);
>  extern void intel_modeset_gem_init(struct drm_device *dev);
>  extern void intel_modeset_cleanup(struct drm_device *dev);
>  extern int intel_connector_register(struct drm_connector *);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 244a0f59d8f7..d5a49d12dc88 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14975,9 +14975,6 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>   */
>  void intel_plane_destroy(struct drm_plane *plane)
>  {
> -	if (!plane)
> -		return;
> -
>  	drm_plane_cleanup(plane);
>  	kfree(to_intel_plane(plane));
>  }
> @@ -14991,11 +14988,10 @@ const struct drm_plane_funcs intel_plane_funcs = {
>  	.atomic_set_property = intel_plane_atomic_set_property,
>  	.atomic_duplicate_state = intel_plane_duplicate_state,
>  	.atomic_destroy_state = intel_plane_destroy_state,
> -
>  };
>  
> -static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> -						    int pipe)
> +static struct intel_plane *
> +intel_primary_plane_create(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *primary = NULL;
> @@ -15006,12 +15002,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	int ret;
>  
>  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> -	if (!primary)
> +	if (!primary) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
>  
>  	state = intel_create_plane_state(&primary->base);
> -	if (!state)
> +	if (!state) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
> +
>  	primary->base.state = &state->base;
>  
>  	primary->can_scale = false;
> @@ -15092,13 +15093,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  
>  	drm_plane_helper_add(&primary->base, &intel_plane_helper_funcs);
>  
> -	return &primary->base;
> +	return primary;
>  
>  fail:
>  	kfree(state);
>  	kfree(primary);
>  
> -	return NULL;
> +	return ERR_PTR(ret);
>  }
>  
>  static int
> @@ -15194,8 +15195,8 @@ intel_update_cursor_plane(struct drm_plane *plane,
>  	intel_crtc_update_cursor(crtc, state);
>  }
>  
> -static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> -						   int pipe)
> +static struct intel_plane *
> +intel_cursor_plane_create(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *cursor = NULL;
> @@ -15203,12 +15204,17 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	int ret;
>  
>  	cursor = kzalloc(sizeof(*cursor), GFP_KERNEL);
> -	if (!cursor)
> +	if (!cursor) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
>  
>  	state = intel_create_plane_state(&cursor->base);
> -	if (!state)
> +	if (!state) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
> +
>  	cursor->base.state = &state->base;
>  
>  	cursor->can_scale = false;
> @@ -15240,13 +15246,13 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  
>  	drm_plane_helper_add(&cursor->base, &intel_plane_helper_funcs);
>  
> -	return &cursor->base;
> +	return cursor;
>  
>  fail:
>  	kfree(state);
>  	kfree(cursor);
>  
> -	return NULL;
> +	return ERR_PTR(ret);
>  }
>  
>  static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc,
> @@ -15265,22 +15271,24 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
>  	scaler_state->scaler_id = -1;
>  }
>  
> -static void intel_crtc_init(struct drm_device *dev, int pipe)
> +static int intel_crtc_init(struct drm_device *dev, enum pipe pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc;
>  	struct intel_crtc_state *crtc_state = NULL;
> -	struct drm_plane *primary = NULL;
> -	struct drm_plane *cursor = NULL;
> +	struct intel_plane *primary = NULL;
> +	struct intel_plane *cursor = NULL;
>  	int sprite, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> -	if (intel_crtc == NULL)
> -		return;
> +	if (!intel_crtc)
> +		return -ENOMEM;
>  
>  	crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> -	if (!crtc_state)
> +	if (!crtc_state) {
> +		ret = -ENOMEM;
>  		goto fail;
> +	}
>  	intel_crtc->config = crtc_state;
>  	intel_crtc->base.state = &crtc_state->base;
>  	crtc_state->base.crtc = &intel_crtc->base;
> @@ -15296,22 +15304,30 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	}
>  
>  	primary = intel_primary_plane_create(dev, pipe);
> -	if (!primary)
> +	if (IS_ERR(primary)) {
> +		ret = PTR_ERR(primary);
>  		goto fail;
> +	}
>  
>  	for_each_sprite(dev_priv, pipe, sprite) {
> -		ret = intel_plane_init(dev, pipe, sprite);
> -		if (ret)
> -			DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> -				      pipe_name(pipe), sprite_name(pipe, sprite), ret);
> +		struct intel_plane *plane;
> +
> +		plane = intel_sprite_plane_create(dev, pipe, sprite);
> +		if (!plane) {
> +			ret = PTR_ERR(plane);
> +			goto fail;
> +		}
>  	}
>  
>  	cursor = intel_cursor_plane_create(dev, pipe);
> -	if (!cursor)
> +	if (!cursor) {
> +		ret = PTR_ERR(cursor);
>  		goto fail;
> +	}
>  
> -	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> -					cursor, &intel_crtc_funcs,
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base,
> +					&primary->base, &cursor->base,
> +					&intel_crtc_funcs,
>  					"pipe %c", pipe_name(pipe));
>  	if (ret)
>  		goto fail;
> @@ -15343,13 +15359,18 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_color_init(&intel_crtc->base);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> -	return;
> +
> +	return 0;
>  
>  fail:
> -	intel_plane_destroy(primary);
> -	intel_plane_destroy(cursor);
> +	/*
> +	 * drm_mode_config_cleanup() will free up any
> +	 * crtcs/planes already initialized.
> +	 */
>  	kfree(crtc_state);
>  	kfree(intel_crtc);
> +
> +	return ret;
>  }
>  
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
> @@ -16426,7 +16447,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	drm_modeset_acquire_fini(&ctx);
>  }
>  
> -void intel_modeset_init(struct drm_device *dev)
> +int intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> @@ -16450,7 +16471,7 @@ void intel_modeset_init(struct drm_device *dev)
>  	intel_init_pm(dev);
>  
>  	if (INTEL_INFO(dev)->num_pipes == 0)
> -		return;
> +		return 0;
>  
>  	/*
>  	 * There may be no VBT; and if the BIOS enabled SSC we can
> @@ -16499,7 +16520,13 @@ void intel_modeset_init(struct drm_device *dev)
>  		      INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
>  
>  	for_each_pipe(dev_priv, pipe) {
> -		intel_crtc_init(dev, pipe);
> +		int ret;
> +
> +		ret = intel_crtc_init(dev, pipe);
> +		if (ret) {
> +			drm_mode_config_cleanup(dev);
> +			return ret;
> +		}
>  	}
>  
>  	intel_update_czclk(dev_priv);
> @@ -16547,6 +16574,8 @@ void intel_modeset_init(struct drm_device *dev)
>  	 * since the watermark calculation done here will use pstate->fb.
>  	 */
>  	sanitize_watermarks(dev);
> +
> +	return 0;
>  }
>  
>  static void intel_enable_pipe_a(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7dda79df55d0..4fd7d74fd6dc 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1779,7 +1779,8 @@ bool intel_sdvo_init(struct drm_device *dev,
>  /* intel_sprite.c */
>  int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>  			     int usecs);
> -int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> +struct intel_plane *intel_sprite_plane_create(struct drm_device *dev,
> +					      enum pipe pipe, int plane);
>  int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  			      struct drm_file *file_priv);
>  void intel_pipe_update_start(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index ae1e3d47951b..41ae7f562eec 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1042,8 +1042,8 @@ static uint32_t skl_plane_formats[] = {
>  	DRM_FORMAT_VYUY,
>  };
>  
> -int
> -intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> +struct intel_plane *
> +intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = NULL;
> @@ -1160,11 +1160,11 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  
>  	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>  
> -	return 0;
> +	return intel_plane;
>  
>  fail:
>  	kfree(state);
>  	kfree(intel_plane);
>  
> -	return ret;
> +	return ERR_PTR(ret);
>  }
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Reorganize sprite init

[Daniel Vetter]

On Tue, Oct 25, 2016 at 06:58:03PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Kill the switch statement from the sprite init code and replace with a
> more straightforward if ladder. Now each significant evolution of the
> sprite hardware is in its own neat box.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 70 ++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 41ae7f562eec..70b50a27763e 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1067,25 +1067,25 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
>  	}
>  	intel_plane->base.state = &state->base;
>  
> -	switch (INTEL_INFO(dev)->gen) {
> -	case 5:
> -	case 6:
> +	if (INTEL_GEN(dev_priv) >= 9) {

Maybe do an s/dev/dev_priv/ on the function paramaters while at it? With
or wihtout that:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


>  		intel_plane->can_scale = true;
> -		intel_plane->max_downscale = 16;
> -		intel_plane->update_plane = ilk_update_plane;
> -		intel_plane->disable_plane = ilk_disable_plane;
> +		state->scaler_id = -1;
>  
> -		if (IS_GEN6(dev_priv)) {
> -			plane_formats = snb_plane_formats;
> -			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> -		} else {
> -			plane_formats = ilk_plane_formats;
> -			num_plane_formats = ARRAY_SIZE(ilk_plane_formats);
> -		}
> -		break;
> +		intel_plane->update_plane = skl_update_plane;
> +		intel_plane->disable_plane = skl_disable_plane;
> +
> +		plane_formats = skl_plane_formats;
> +		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> +	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		intel_plane->can_scale = false;
> +		intel_plane->max_downscale = 1;
> +
> +		intel_plane->update_plane = vlv_update_plane;
> +		intel_plane->disable_plane = vlv_disable_plane;
>  
> -	case 7:
> -	case 8:
> +		plane_formats = vlv_plane_formats;
> +		num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> +	} else if (INTEL_GEN(dev_priv) >= 7) {
>  		if (IS_IVYBRIDGE(dev_priv)) {
>  			intel_plane->can_scale = true;
>  			intel_plane->max_downscale = 2;
> @@ -1094,33 +1094,25 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
>  			intel_plane->max_downscale = 1;
>  		}
>  
> -		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -			intel_plane->update_plane = vlv_update_plane;
> -			intel_plane->disable_plane = vlv_disable_plane;
> +		intel_plane->update_plane = ivb_update_plane;
> +		intel_plane->disable_plane = ivb_disable_plane;
>  
> -			plane_formats = vlv_plane_formats;
> -			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> -		} else {
> -			intel_plane->update_plane = ivb_update_plane;
> -			intel_plane->disable_plane = ivb_disable_plane;
> +		plane_formats = snb_plane_formats;
> +		num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> +	} else {
> +		intel_plane->can_scale = true;
> +		intel_plane->max_downscale = 16;
> +
> +		intel_plane->update_plane = ilk_update_plane;
> +		intel_plane->disable_plane = ilk_disable_plane;
>  
> +		if (IS_GEN6(dev_priv)) {
>  			plane_formats = snb_plane_formats;
>  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> +		} else {
> +			plane_formats = ilk_plane_formats;
> +			num_plane_formats = ARRAY_SIZE(ilk_plane_formats);
>  		}
> -		break;
> -	case 9:
> -		intel_plane->can_scale = true;
> -		intel_plane->update_plane = skl_update_plane;
> -		intel_plane->disable_plane = skl_disable_plane;
> -		state->scaler_id = -1;
> -
> -		plane_formats = skl_plane_formats;
> -		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> -		break;
> -	default:
> -		MISSING_CASE(INTEL_INFO(dev)->gen);
> -		ret = -ENODEV;
> -		goto fail;
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 9) {
> @@ -1139,7 +1131,7 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
>  
>  	possible_crtcs = (1 << pipe);
>  
> -	if (INTEL_INFO(dev)->gen >= 9)
> +	if (INTEL_GEN(dev_priv) >= 9)
>  		ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
>  					       &intel_plane_funcs,
>  					       plane_formats, num_plane_formats,
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Reorganize sprite init

[Ville Syrjälä]

On Thu, Oct 27, 2016 at 09:07:13AM +0200, Daniel Vetter wrote:
> On Tue, Oct 25, 2016 at 06:58:03PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Kill the switch statement from the sprite init code and replace with a
> > more straightforward if ladder. Now each significant evolution of the
> > sprite hardware is in its own neat box.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_sprite.c | 70 ++++++++++++++++---------------------
> >  1 file changed, 31 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 41ae7f562eec..70b50a27763e 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -1067,25 +1067,25 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
> >  	}
> >  	intel_plane->base.state = &state->base;
> >  
> > -	switch (INTEL_INFO(dev)->gen) {
> > -	case 5:
> > -	case 6:
> > +	if (INTEL_GEN(dev_priv) >= 9) {
> 
> Maybe do an s/dev/dev_priv/ on the function paramaters while at it? With
> or wihtout that:

I went without. Looks like the change would like to propagate quite a
bit further, so I figured I'd so a separate series for it.

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Series pushed to dinq. Thanls for the review.

> 
> 
> >  		intel_plane->can_scale = true;
> > -		intel_plane->max_downscale = 16;
> > -		intel_plane->update_plane = ilk_update_plane;
> > -		intel_plane->disable_plane = ilk_disable_plane;
> > +		state->scaler_id = -1;
> >  
> > -		if (IS_GEN6(dev_priv)) {
> > -			plane_formats = snb_plane_formats;
> > -			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> > -		} else {
> > -			plane_formats = ilk_plane_formats;
> > -			num_plane_formats = ARRAY_SIZE(ilk_plane_formats);
> > -		}
> > -		break;
> > +		intel_plane->update_plane = skl_update_plane;
> > +		intel_plane->disable_plane = skl_disable_plane;
> > +
> > +		plane_formats = skl_plane_formats;
> > +		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> > +	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > +		intel_plane->can_scale = false;
> > +		intel_plane->max_downscale = 1;
> > +
> > +		intel_plane->update_plane = vlv_update_plane;
> > +		intel_plane->disable_plane = vlv_disable_plane;
> >  
> > -	case 7:
> > -	case 8:
> > +		plane_formats = vlv_plane_formats;
> > +		num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> > +	} else if (INTEL_GEN(dev_priv) >= 7) {
> >  		if (IS_IVYBRIDGE(dev_priv)) {
> >  			intel_plane->can_scale = true;
> >  			intel_plane->max_downscale = 2;
> > @@ -1094,33 +1094,25 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
> >  			intel_plane->max_downscale = 1;
> >  		}
> >  
> > -		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > -			intel_plane->update_plane = vlv_update_plane;
> > -			intel_plane->disable_plane = vlv_disable_plane;
> > +		intel_plane->update_plane = ivb_update_plane;
> > +		intel_plane->disable_plane = ivb_disable_plane;
> >  
> > -			plane_formats = vlv_plane_formats;
> > -			num_plane_formats = ARRAY_SIZE(vlv_plane_formats);
> > -		} else {
> > -			intel_plane->update_plane = ivb_update_plane;
> > -			intel_plane->disable_plane = ivb_disable_plane;
> > +		plane_formats = snb_plane_formats;
> > +		num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> > +	} else {
> > +		intel_plane->can_scale = true;
> > +		intel_plane->max_downscale = 16;
> > +
> > +		intel_plane->update_plane = ilk_update_plane;
> > +		intel_plane->disable_plane = ilk_disable_plane;
> >  
> > +		if (IS_GEN6(dev_priv)) {
> >  			plane_formats = snb_plane_formats;
> >  			num_plane_formats = ARRAY_SIZE(snb_plane_formats);
> > +		} else {
> > +			plane_formats = ilk_plane_formats;
> > +			num_plane_formats = ARRAY_SIZE(ilk_plane_formats);
> >  		}
> > -		break;
> > -	case 9:
> > -		intel_plane->can_scale = true;
> > -		intel_plane->update_plane = skl_update_plane;
> > -		intel_plane->disable_plane = skl_disable_plane;
> > -		state->scaler_id = -1;
> > -
> > -		plane_formats = skl_plane_formats;
> > -		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> > -		break;
> > -	default:
> > -		MISSING_CASE(INTEL_INFO(dev)->gen);
> > -		ret = -ENODEV;
> > -		goto fail;
> >  	}
> >  
> >  	if (INTEL_GEN(dev_priv) >= 9) {
> > @@ -1139,7 +1131,7 @@ intel_sprite_plane_create(struct drm_device *dev, enum pipe pipe, int plane)
> >  
> >  	possible_crtcs = (1 << pipe);
> >  
> > -	if (INTEL_INFO(dev)->gen >= 9)
> > +	if (INTEL_GEN(dev_priv) >= 9)
> >  		ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
> >  					       &intel_plane_funcs,
> >  					       plane_formats, num_plane_formats,
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Bail if plane/crtc init fails

[Chris Wilson]

On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Due to the plane->index not getting readjusted in drm_plane_cleanup(),
> we can't continue initialization of some plane/crtc init fails.
> Well, we sort of could I suppose if we left all initialized planes on
> the list, but that would expose those planes to userspace as well.
> 
> But for crtcs the situation is even worse since we assume that
> pipe==crtc index occasionally, so we can't really deal with a partially
> initialize set of crtcs.
> 
> So seems safest to just abort the entire thing if anything goes wrong.
> All the failure paths here are kmalloc()s anyway, so it seems unlikely
> we'd get very far if these start failing.

smatch spotted ERR_PTR(0)

> @@ -15296,22 +15304,30 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	}
>  
>  	primary = intel_primary_plane_create(dev, pipe);
> -	if (!primary)
> +	if (IS_ERR(primary)) {
> +		ret = PTR_ERR(primary);

Here...

>  		goto fail;
> +	}
>  
>  	for_each_sprite(dev_priv, pipe, sprite) {
> -		ret = intel_plane_init(dev, pipe, sprite);
> -		if (ret)
> -			DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> -				      pipe_name(pipe), sprite_name(pipe, sprite), ret);
> +		struct intel_plane *plane;
> +
> +		plane = intel_sprite_plane_create(dev, pipe, sprite);
> +		if (!plane) {
> +			ret = PTR_ERR(plane);

and here.

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Bail if plane/crtc init fails

[Ville Syrjälä]

On Fri, Nov 04, 2016 at 08:48:21PM +0000, Chris Wilson wrote:
> On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Due to the plane->index not getting readjusted in drm_plane_cleanup(),
> > we can't continue initialization of some plane/crtc init fails.
> > Well, we sort of could I suppose if we left all initialized planes on
> > the list, but that would expose those planes to userspace as well.
> > 
> > But for crtcs the situation is even worse since we assume that
> > pipe==crtc index occasionally, so we can't really deal with a partially
> > initialize set of crtcs.
> > 
> > So seems safest to just abort the entire thing if anything goes wrong.
> > All the failure paths here are kmalloc()s anyway, so it seems unlikely
> > we'd get very far if these start failing.
> 
> smatch spotted ERR_PTR(0)
> 
> > @@ -15296,22 +15304,30 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >  	}
> >  
> >  	primary = intel_primary_plane_create(dev, pipe);
> > -	if (!primary)
> > +	if (IS_ERR(primary)) {
> > +		ret = PTR_ERR(primary);
> 
> Here...

This looks correct to me, but the cursor and sprite paths are clearly
crap.

> 
> >  		goto fail;
> > +	}
> >  
> >  	for_each_sprite(dev_priv, pipe, sprite) {
> > -		ret = intel_plane_init(dev, pipe, sprite);
> > -		if (ret)
> > -			DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> > -				      pipe_name(pipe), sprite_name(pipe, sprite), ret);
> > +		struct intel_plane *plane;
> > +
> > +		plane = intel_sprite_plane_create(dev, pipe, sprite);
> > +		if (!plane) {
> > +			ret = PTR_ERR(plane);
> 
> and here.
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Bail if plane/crtc init fails

[Chris Wilson]

On Fri, Nov 04, 2016 at 11:07:26PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 04, 2016 at 08:48:21PM +0000, Chris Wilson wrote:
> > On Tue, Oct 25, 2016 at 06:58:02PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Due to the plane->index not getting readjusted in drm_plane_cleanup(),
> > > we can't continue initialization of some plane/crtc init fails.
> > > Well, we sort of could I suppose if we left all initialized planes on
> > > the list, but that would expose those planes to userspace as well.
> > > 
> > > But for crtcs the situation is even worse since we assume that
> > > pipe==crtc index occasionally, so we can't really deal with a partially
> > > initialize set of crtcs.
> > > 
> > > So seems safest to just abort the entire thing if anything goes wrong.
> > > All the failure paths here are kmalloc()s anyway, so it seems unlikely
> > > we'd get very far if these start failing.
> > 
> > smatch spotted ERR_PTR(0)
> > 
> > > @@ -15296,22 +15304,30 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >  	}
> > >  
> > >  	primary = intel_primary_plane_create(dev, pipe);
> > > -	if (!primary)
> > > +	if (IS_ERR(primary)) {
> > > +		ret = PTR_ERR(primary);
> > 
> > Here...
> 
> This looks correct to me, but the cursor and sprite paths are clearly
> crap.

Brain had already turned off. Yes, it was the plane and cursor, I just
goofed in trimming.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx