[PATCH 0/6] drm: Give crtcs and planes actual names

[PATCH 0/6] drm: Give crtcs and planes actual names

[ville.syrjala]

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

I got sick and tired of staring at the line noise produced by drm.debug=0x1e,
so I decided to give the crtcs and planes human readable names. Each driver
can give whatever names it wants, and for i915 I gave something that makes
some sense w.r.t. to the spec.

The only magic gotcha here is that if the driver dynamically allocates the
name, it must be careful around drm_{crtc,plane}_cleanup() cause those
guys just memset the entire structure to 0. I didn't want to put the kfree()
into the cleanup functions to avoid having to kstrdup("") in the fallback
case or forcing drivers to always use a dynamic allocation.

Ville Syrjälä (6):
  drm: Add crtc->name and use it in debug messages
  drm: Add plane->name and use it in debug prints
  drm/i915: Use crtc->name in debug messages
  drm/i915: Use plane->name in debug prints
  drm/i915: Set crtc->name to "pipe A", "pipe B", etc.
  drm/i915: Give meaningful names to all the planes

 drivers/gpu/drm/drm_atomic.c         |  53 ++++++++-------
 drivers/gpu/drm/drm_atomic_helper.c  |  60 +++++++++--------
 drivers/gpu/drm/drm_crtc.c           |  11 ++-
 drivers/gpu/drm/drm_crtc_helper.c    |  24 ++++---
 drivers/gpu/drm/i915/intel_display.c | 127 +++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_fbdev.c   |   5 +-
 drivers/gpu/drm/i915/intel_sprite.c  |  14 ++++
 include/drm/drm_crtc.h               |   4 ++
 8 files changed, 185 insertions(+), 113 deletions(-)

-- 
2.4.10

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

[PATCH 1/6] drm: Add crtc->name and use it in debug messages

[ville.syrjala]

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

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 41 ++++++++++++++-------------
 drivers/gpu/drm/drm_atomic_helper.c | 56 +++++++++++++++++++------------------
 drivers/gpu/drm/drm_crtc.c          |  8 ++++--
 drivers/gpu/drm/drm_crtc_helper.c   | 24 ++++++++--------
 include/drm/drm_crtc.h              |  2 ++
 5 files changed, 71 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7bb3845..2944655 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -288,8 +288,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
 	state->crtcs[index] = crtc;
 	crtc_state->state = state;
 
-	DRM_DEBUG_ATOMIC("Added [CRTC:%d] %p state to %p\n",
-			 crtc->base.id, crtc_state, state);
+	DRM_DEBUG_ATOMIC("Added [CRTC:%d:%s] %p state to %p\n",
+			 crtc->base.id, crtc->name, crtc_state, state);
 
 	return crtc_state;
 }
@@ -480,8 +480,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
 	 */
 
 	if (state->active && !state->enable) {
-		DRM_DEBUG_ATOMIC("[CRTC:%d] active without enabled\n",
-				 crtc->base.id);
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active without enabled\n",
+				 crtc->base.id, crtc->name);
 		return -EINVAL;
 	}
 
@@ -490,15 +490,15 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
 	 * be able to trigger. */
 	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
 	    WARN_ON(state->enable && !state->mode_blob)) {
-		DRM_DEBUG_ATOMIC("[CRTC:%d] enabled without mode blob\n",
-				 crtc->base.id);
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled without mode blob\n",
+				 crtc->base.id, crtc->name);
 		return -EINVAL;
 	}
 
 	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
 	    WARN_ON(!state->enable && state->mode_blob)) {
-		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled with mode blob\n",
-				 crtc->base.id);
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled with mode blob\n",
+				 crtc->base.id, crtc->name);
 		return -EINVAL;
 	}
 
@@ -980,8 +980,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 	}
 
 	if (crtc)
-		DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d]\n",
-				 plane_state, crtc->base.id);
+		DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d:%s]\n",
+				 plane_state, crtc->base.id, crtc->name);
 	else
 		DRM_DEBUG_ATOMIC("Link plane state %p to [NOCRTC]\n",
 				 plane_state);
@@ -1048,8 +1048,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 	conn_state->crtc = crtc;
 
 	if (crtc)
-		DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d]\n",
-				 conn_state, crtc->base.id);
+		DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n",
+				 conn_state, crtc->base.id, crtc->name);
 	else
 		DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n",
 				 conn_state);
@@ -1088,8 +1088,8 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 	if (ret)
 		return ret;
 
-	DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d] to %p\n",
-			 crtc->base.id, state);
+	DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d:%s] to %p\n",
+			 crtc->base.id, crtc->name, state);
 
 	/*
 	 * Changed connectors are already in @state, so only need to look at the
@@ -1169,8 +1169,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
 			num_connected_connectors++;
 	}
 
-	DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d]\n",
-			 state, num_connected_connectors, crtc->base.id);
+	DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n",
+			 state, num_connected_connectors,
+			 crtc->base.id, crtc->name);
 
 	return num_connected_connectors;
 }
@@ -1237,8 +1238,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		ret = drm_atomic_crtc_check(crtc, crtc_state);
 		if (ret) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] atomic core check failed\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check failed\n",
+					 crtc->base.id, crtc->name);
 			return ret;
 		}
 	}
@@ -1249,8 +1250,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	if (!state->allow_modeset) {
 		for_each_crtc_in_state(state, crtc, crtc_state, i) {
 			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
-				DRM_DEBUG_ATOMIC("[CRTC:%d] requires full modeset\n",
-						 crtc->base.id);
+				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full modeset\n",
+						 crtc->base.id, crtc->name);
 				return -EINVAL;
 			}
 		}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0c6f621..fc90af50 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -116,9 +116,9 @@ steal_encoder(struct drm_atomic_state *state,
 	 */
 	WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
 
-	DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d], stealing it\n",
+	DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], stealing it\n",
 			 encoder->base.id, encoder->name,
-			 encoder_crtc->base.id);
+			 encoder_crtc->base.id, encoder_crtc->name);
 
 	crtc_state = drm_atomic_get_crtc_state(state, encoder_crtc);
 	if (IS_ERR(crtc_state))
@@ -211,12 +211,13 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 	}
 
 	if (new_encoder == connector_state->best_encoder) {
-		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d]\n",
+		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d:%s]\n",
 				 connector->base.id,
 				 connector->name,
 				 new_encoder->base.id,
 				 new_encoder->name,
-				 connector_state->crtc->base.id);
+				 connector_state->crtc->base.id,
+				 connector_state->crtc->name);
 
 		return 0;
 	}
@@ -243,12 +244,13 @@ update_connector_routing(struct drm_atomic_state *state, int conn_idx)
 	crtc_state = state->crtc_states[idx];
 	crtc_state->connectors_changed = true;
 
-	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
+	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
 			 connector->base.id,
 			 connector->name,
 			 new_encoder->base.id,
 			 new_encoder->name,
-			 connector_state->crtc->base.id);
+			 connector_state->crtc->base.id,
+			 connector_state->crtc->name);
 
 	return 0;
 }
@@ -332,8 +334,8 @@ mode_fixup(struct drm_atomic_state *state)
 		ret = funcs->mode_fixup(crtc, &crtc_state->mode,
 					&crtc_state->adjusted_mode);
 		if (!ret) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] fixup failed\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] fixup failed\n",
+					 crtc->base.id, crtc->name);
 			return -EINVAL;
 		}
 	}
@@ -380,14 +382,14 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] mode changed\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode changed\n",
+					 crtc->base.id, crtc->name);
 			crtc_state->mode_changed = true;
 		}
 
 		if (crtc->state->enable != crtc_state->enable) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enable changed\n",
+					 crtc->base.id, crtc->name);
 
 			/*
 			 * For clarity this assignment is done here, but
@@ -428,18 +430,18 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		 * a full modeset because update_connector_routing force that.
 		 */
 		if (crtc->state->active != crtc_state->active) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] active changed\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active changed\n",
+					 crtc->base.id, crtc->name);
 			crtc_state->active_changed = true;
 		}
 
 		if (!drm_atomic_crtc_needs_modeset(crtc_state))
 			continue;
 
-		DRM_DEBUG_ATOMIC("[CRTC:%d] needs all connectors, enable: %c, active: %c\n",
-				 crtc->base.id,
+		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] needs all connectors, enable: %c, active: %c\n",
+				 crtc->base.id, crtc->name,
 				 crtc_state->enable ? 'y' : 'n',
-			      crtc_state->active ? 'y' : 'n');
+				 crtc_state->active ? 'y' : 'n');
 
 		ret = drm_atomic_add_affected_connectors(state, crtc);
 		if (ret != 0)
@@ -453,8 +455,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 								crtc);
 
 		if (crtc_state->enable != !!num_connectors) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors mismatch\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n",
+					 crtc->base.id, crtc->name);
 
 			return -EINVAL;
 		}
@@ -517,8 +519,8 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 
 		ret = funcs->atomic_check(crtc, state->crtc_states[i]);
 		if (ret) {
-			DRM_DEBUG_ATOMIC("[CRTC:%d] atomic driver check failed\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic driver check failed\n",
+					 crtc->base.id, crtc->name);
 			return ret;
 		}
 	}
@@ -631,8 +633,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		funcs = crtc->helper_private;
 
-		DRM_DEBUG_ATOMIC("disabling [CRTC:%d]\n",
-				 crtc->base.id);
+		DRM_DEBUG_ATOMIC("disabling [CRTC:%d:%s]\n",
+				 crtc->base.id, crtc->name);
 
 
 		/* Right function depends upon target state. */
@@ -743,8 +745,8 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
 		funcs = crtc->helper_private;
 
 		if (crtc->state->enable && funcs->mode_set_nofb) {
-			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d]\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
+					 crtc->base.id, crtc->name);
 
 			funcs->mode_set_nofb(crtc);
 		}
@@ -843,8 +845,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		funcs = crtc->helper_private;
 
 		if (crtc->state->enable) {
-			DRM_DEBUG_ATOMIC("enabling [CRTC:%d]\n",
-					 crtc->base.id);
+			DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
+					 crtc->base.id, crtc->name);
 
 			if (funcs->enable)
 				funcs->enable(crtc);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 24c5434..ea00a69 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -677,6 +677,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	crtc->dev = dev;
 	crtc->funcs = funcs;
 
+	if (!crtc->name)
+		crtc->name = "";
+
 	drm_modeset_lock_init(&crtc->mutex);
 	ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
 	if (ret)
@@ -1801,7 +1804,8 @@ int drm_mode_getresources(struct drm_device *dev, void *data,
 		copied = 0;
 		crtc_id = (uint32_t __user *)(unsigned long)card_res->crtc_id_ptr;
 		drm_for_each_crtc(crtc, dev) {
-			DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
+			DRM_DEBUG_KMS("[CRTC:%d:%s]\n",
+				      crtc->base.id, crtc->name);
 			if (put_user(crtc->base.id, crtc_id + copied)) {
 				ret = -EFAULT;
 				goto out;
@@ -2646,7 +2650,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		ret = -ENOENT;
 		goto out;
 	}
-	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
+	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
 
 	if (crtc_req->mode_valid) {
 		/* If we have a mode we need a framebuffer. */
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index ef53475..302ee32 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -329,7 +329,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
 		DRM_DEBUG_KMS("CRTC fixup failed\n");
 		goto done;
 	}
-	DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
+	DRM_DEBUG_KMS("[CRTC:%d:%s]\n", crtc->base.id, crtc->name);
 
 	crtc->hwmode = *adjusted_mode;
 
@@ -484,11 +484,13 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 		set->fb = NULL;
 
 	if (set->fb) {
-		DRM_DEBUG_KMS("[CRTC:%d] [FB:%d] #connectors=%d (x y) (%i %i)\n",
-				set->crtc->base.id, set->fb->base.id,
-				(int)set->num_connectors, set->x, set->y);
+		DRM_DEBUG_KMS("[CRTC:%d:%s] [FB:%d] #connectors=%d (x y) (%i %i)\n",
+			      set->crtc->base.id, set->crtc->name,
+			      set->fb->base.id,
+			      (int)set->num_connectors, set->x, set->y);
 	} else {
-		DRM_DEBUG_KMS("[CRTC:%d] [NOFB]\n", set->crtc->base.id);
+		DRM_DEBUG_KMS("[CRTC:%d:%s] [NOFB]\n",
+			      set->crtc->base.id, set->crtc->name);
 		drm_crtc_helper_disable(set->crtc);
 		return 0;
 	}
@@ -628,12 +630,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 			connector->encoder->crtc = new_crtc;
 		}
 		if (new_crtc) {
-			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d]\n",
-				connector->base.id, connector->name,
-				new_crtc->base.id);
+			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [CRTC:%d:%s]\n",
+				      connector->base.id, connector->name,
+				      new_crtc->base.id, new_crtc->name);
 		} else {
 			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] to [NOCRTC]\n",
-				connector->base.id, connector->name);
+				      connector->base.id, connector->name);
 		}
 	}
 
@@ -650,8 +652,8 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
 			if (!drm_crtc_helper_set_mode(set->crtc, set->mode,
 						      set->x, set->y,
 						      save_set.fb)) {
-				DRM_ERROR("failed to set mode on [CRTC:%d]\n",
-					  set->crtc->base.id);
+				DRM_ERROR("failed to set mode on [CRTC:%d:%s]\n",
+					  set->crtc->base.id, set->crtc->name);
 				set->crtc->primary->fb = save_set.fb;
 				ret = -EINVAL;
 				goto fail;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3f0c690..a8279b4 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -426,6 +426,8 @@ struct drm_crtc {
 	struct device_node *port;
 	struct list_head head;
 
+	char *name;
+
 	/*
 	 * crtc mutex
 	 *
-- 
2.4.10

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

[PATCH 2/6] drm: Add plane->name and use it in debug prints

[ville.syrjala]

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

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        | 12 ++++++------
 drivers/gpu/drm/drm_atomic_helper.c |  4 ++--
 drivers/gpu/drm/drm_crtc.c          |  3 +++
 include/drm/drm_crtc.h              |  2 ++
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 2944655..df84060 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -543,8 +543,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
 	state->planes[index] = plane;
 	plane_state->state = state;
 
-	DRM_DEBUG_ATOMIC("Added [PLANE:%d] %p state to %p\n",
-			 plane->base.id, plane_state, state);
+	DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
+			 plane->base.id, plane->name, plane_state, state);
 
 	if (plane_state->crtc) {
 		struct drm_crtc_state *crtc_state;
@@ -755,8 +755,8 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
 	}
 
 	if (plane_switching_crtc(state->state, plane, state)) {
-		DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
-				 plane->base.id);
+		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
+				 plane->base.id, plane->name);
 		return -EINVAL;
 	}
 
@@ -1229,8 +1229,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 	for_each_plane_in_state(state, plane, plane_state, i) {
 		ret = drm_atomic_plane_check(plane, plane_state);
 		if (ret) {
-			DRM_DEBUG_ATOMIC("[PLANE:%d] atomic core check failed\n",
-					 plane->base.id);
+			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n",
+					 plane->base.id, plane->name);
 			return ret;
 		}
 	}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fc90af50..387d95c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -503,8 +503,8 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 
 		ret = funcs->atomic_check(plane, plane_state);
 		if (ret) {
-			DRM_DEBUG_ATOMIC("[PLANE:%d] atomic driver check failed\n",
-					 plane->base.id);
+			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic driver check failed\n",
+					 plane->base.id, plane->name);
 			return ret;
 		}
 	}
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index ea00a69..8dc4052 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1174,6 +1174,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	drm_modeset_lock_init(&plane->mutex);
 
+	if (!plane->name)
+		plane->name = "";
+
 	plane->base.properties = &plane->properties;
 	plane->dev = dev;
 	plane->funcs = funcs;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a8279b4..a08e256 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -848,6 +848,8 @@ struct drm_plane {
 	struct drm_device *dev;
 	struct list_head head;
 
+	char *name;
+
 	struct drm_modeset_lock mutex;
 
 	struct drm_mode_object base;
-- 
2.4.10

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

[PATCH 3/6] drm/i915: Use crtc->name in debug messages

[ville.syrjala]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5f7493..9845687 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4220,8 +4220,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		i = (enum intel_dpll_id) crtc->pipe;
 		pll = &dev_priv->shared_dplls[i];
 
-		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
-			      crtc->base.base.id, pll->name);
+		DRM_DEBUG_KMS("[CRTC:%d:%s] using pre-allocated %s\n",
+			      crtc->base.base.id, crtc->base.name,
+			      pll->name);
 
 		WARN_ON(shared_dpll[i].crtc_mask);
 
@@ -4241,8 +4242,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		/* 1:1 mapping between ports and PLLs */
 		i = (enum intel_dpll_id)intel_dig_port->port;
 		pll = &dev_priv->shared_dplls[i];
-		DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
-			crtc->base.base.id, pll->name);
+		DRM_DEBUG_KMS("[CRTC:%d:%s] using pre-allocated %s\n",
+			      crtc->base.base.id, crtc->base.name,
+			      pll->name);
 		WARN_ON(shared_dpll[i].crtc_mask);
 
 		goto found;
@@ -4258,9 +4260,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 		if (memcmp(&crtc_state->dpll_hw_state,
 			   &shared_dpll[i].hw_state,
 			   sizeof(crtc_state->dpll_hw_state)) == 0) {
-			DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
-				      crtc->base.base.id, pll->name,
-				      shared_dpll[i].crtc_mask,
+			DRM_DEBUG_KMS("[CRTC:%d:%s] sharing existing %s (crtc mask 0x%08x, ative %d)\n",
+				      crtc->base.base.id, crtc->base.name,
+				      pll->name, shared_dpll[i].crtc_mask,
 				      pll->active);
 			goto found;
 		}
@@ -4270,8 +4272,9 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
 		if (shared_dpll[i].crtc_mask == 0) {
-			DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
-				      crtc->base.base.id, pll->name);
+			DRM_DEBUG_KMS("[CRTC:%d:%s] allocated %s\n",
+				      crtc->base.base.id, crtc->base.name,
+				      pll->name);
 			goto found;
 		}
 	}
@@ -4398,8 +4401,9 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
 	struct intel_crtc *intel_crtc = to_intel_crtc(state->base.crtc);
 	const struct drm_display_mode *adjusted_mode = &state->base.adjusted_mode;
 
-	DRM_DEBUG_KMS("Updating scaler for [CRTC:%i] scaler_user index %u.%u\n",
-		      intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX);
+	DRM_DEBUG_KMS("Updating scaler for [CRTC:%d:%s] scaler_user index %u.%u\n",
+		      intel_crtc->base.base.id, intel_crtc->base.name,
+		      intel_crtc->pipe, SKL_CRTC_INDEX);
 
 	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
 		&state->scaler_state.scaler_id, DRM_ROTATE_0,
@@ -11643,13 +11647,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
-	int idx = intel_crtc->base.base.id, ret;
 	int i = drm_plane_index(plane);
 	bool mode_changed = needs_modeset(crtc_state);
 	bool was_crtc_enabled = crtc->state->active;
 	bool is_crtc_enabled = crtc_state->active;
 	bool turn_off, turn_on, visible, was_visible;
 	struct drm_framebuffer *fb = plane_state->fb;
+	int ret;
 
 	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
 	    plane->type != DRM_PLANE_TYPE_CURSOR) {
@@ -11675,7 +11679,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);
 
-	DRM_DEBUG_ATOMIC("[CRTC:%i] has [PLANE:%i] with fb %i\n", idx,
+	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%i] with fb %i\n",
+			 intel_crtc->base.base.id,
+			 intel_crtc->base.name,
 			 plane->base.id, fb ? fb->base.id : -1);
 
 	DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n",
@@ -11981,7 +11987,8 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 	struct intel_plane_state *state;
 	struct drm_framebuffer *fb;
 
-	DRM_DEBUG_KMS("[CRTC:%d]%s config %p for pipe %c\n", crtc->base.base.id,
+	DRM_DEBUG_KMS("[CRTC:%d:%s]%s config %p for pipe %c\n",
+		      crtc->base.base.id, crtc->base.name,
 		      context, pipe_config, pipe_name(crtc->pipe));
 
 	DRM_DEBUG_KMS("cpu_transcoder: %c\n", transcoder_name(pipe_config->cpu_transcoder));
@@ -12753,8 +12760,8 @@ check_crtc_state(struct drm_device *dev, struct drm_atomic_state *old_state)
 		pipe_config->base.crtc = crtc;
 		pipe_config->base.state = old_state;
 
-		DRM_DEBUG_KMS("[CRTC:%d]\n",
-			      crtc->base.id);
+		DRM_DEBUG_KMS("[CRTC:%d:%s]\n",
+			      crtc->base.id, crtc->name);
 
 		active = dev_priv->display.get_pipe_config(intel_crtc,
 							   pipe_config);
@@ -13390,8 +13397,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
 
 	state = drm_atomic_state_alloc(dev);
 	if (!state) {
-		DRM_DEBUG_KMS("[CRTC:%d] crtc restore failed, out of memory",
-			      crtc->base.id);
+		DRM_DEBUG_KMS("[CRTC:%d:%s] crtc restore failed, out of memory",
+			      crtc->base.id, crtc->name);
 		return;
 	}
 
@@ -15193,8 +15200,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	if (INTEL_INFO(dev)->gen < 4 && !intel_check_plane_mapping(crtc)) {
 		bool plane;
 
-		DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n",
-			      crtc->base.base.id);
+		DRM_DEBUG_KMS("[CRTC:%d:%s] wrong plane connection detected!\n",
+			      crtc->base.base.id, crtc->base.name);
 
 		/* Pipe has the wrong plane attached and the plane is active.
 		 * Temporarily change the plane mapping and disable everything
@@ -15227,8 +15234,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 * functions or because of calls to intel_crtc_disable_noatomic,
 		 * or because the pipe is force-enabled due to the
 		 * pipe A quirk. */
-		DRM_DEBUG_KMS("[CRTC:%d] hw state adjusted, was %s, now %s\n",
-			      crtc->base.base.id,
+		DRM_DEBUG_KMS("[CRTC:%d:%s] hw state adjusted, was %s, now %s\n",
+			      crtc->base.base.id, crtc->base.name,
 			      crtc->base.state->enable ? "enabled" : "disabled",
 			      crtc->active ? "enabled" : "disabled");
 
@@ -15390,8 +15397,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		readout_plane_state(crtc);
 
-		DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n",
-			      crtc->base.base.id,
+		DRM_DEBUG_KMS("[CRTC:%d:%s] hw state readout: %s\n",
+			      crtc->base.base.id, crtc->base.name,
 			      crtc->active ? "enabled" : "disabled");
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 98772d3..7670440 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -472,10 +472,9 @@ retry:
 		}
 		crtcs[i] = new_crtc;
 
-		DRM_DEBUG_KMS("connector %s on pipe %c [CRTC:%d]: %dx%d%s\n",
+		DRM_DEBUG_KMS("connector %s on [CRTC:%d:%s]: %dx%d%s\n",
 			      connector->name,
-			      pipe_name(to_intel_crtc(encoder->crtc)->pipe),
-			      encoder->crtc->base.id,
+			      encoder->crtc->base.id, encoder->crtc->name,
 			      modes[i]->hdisplay, modes[i]->vdisplay,
 			      modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" :"");
 
-- 
2.4.10

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

[PATCH 4/6] drm/i915: Use plane->name in debug prints

[ville.syrjala]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9845687..b628dab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4433,9 +4433,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 
 	bool force_detach = !fb || !plane_state->visible;
 
-	DRM_DEBUG_KMS("Updating scaler for [PLANE:%d] scaler_user index %u.%u\n",
-		      intel_plane->base.base.id, intel_crtc->pipe,
-		      drm_plane_index(&intel_plane->base));
+	DRM_DEBUG_KMS("Updating scaler for [PLANE:%d:%s] scaler_user index %u.%u\n",
+		      intel_plane->base.base.id, intel_plane->base.name,
+		      intel_crtc->pipe, drm_plane_index(&intel_plane->base));
 
 	ret = skl_update_scaler(crtc_state, force_detach,
 				drm_plane_index(&intel_plane->base),
@@ -4451,8 +4451,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 
 	/* check colorkey */
 	if (plane_state->ckey.flags != I915_SET_COLORKEY_NONE) {
-		DRM_DEBUG_KMS("[PLANE:%d] scaling with color key not allowed",
-			      intel_plane->base.base.id);
+		DRM_DEBUG_KMS("[PLANE:%d:%s] scaling with color key not allowed",
+			      intel_plane->base.base.id,
+			      intel_plane->base.name);
 		return -EINVAL;
 	}
 
@@ -4471,8 +4472,9 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 	case DRM_FORMAT_VYUY:
 		break;
 	default:
-		DRM_DEBUG_KMS("[PLANE:%d] FB:%d unsupported scaling format 0x%x\n",
-			intel_plane->base.base.id, fb->base.id, fb->pixel_format);
+		DRM_DEBUG_KMS("[PLANE:%d:%s] FB:%d unsupported scaling format 0x%x\n",
+			      intel_plane->base.base.id, intel_plane->base.name,
+			      fb->base.id, fb->pixel_format);
 		return -EINVAL;
 	}
 
@@ -11679,13 +11681,15 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	turn_off = was_visible && (!visible || mode_changed);
 	turn_on = visible && (!was_visible || mode_changed);
 
-	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%i] with fb %i\n",
+	DRM_DEBUG_ATOMIC("[CRTC:%d:%s] has [PLANE:%d:%s] with fb %i\n",
 			 intel_crtc->base.base.id,
 			 intel_crtc->base.name,
-			 plane->base.id, fb ? fb->base.id : -1);
+			 plane->base.id, plane->name,
+			 fb ? fb->base.id : -1);
 
-	DRM_DEBUG_ATOMIC("[PLANE:%i] visible %i -> %i, off %i, on %i, ms %i\n",
-			 plane->base.id, was_visible, visible,
+	DRM_DEBUG_ATOMIC("[PLANE:%d:%s] visible %i -> %i, off %i, on %i, ms %i\n",
+			 plane->base.id, plane->name,
+			 was_visible, visible,
 			 turn_off, turn_on, mode_changed);
 
 	if (turn_on) {
@@ -12088,18 +12092,20 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 		state = to_intel_plane_state(plane->state);
 		fb = state->base.fb;
 		if (!fb) {
-			DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d "
-				"disabled, scaler_id = %d\n",
+			DRM_DEBUG_KMS("%s [PLANE:%d:%s] plane: %u.%u idx: %d "
+				      "disabled, scaler_id = %d\n",
 				plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
-				plane->base.id, intel_plane->pipe,
+				plane->base.id, plane->name,
+				intel_plane->pipe,
 				(crtc->base.primary == plane) ? 0 : intel_plane->plane + 1,
 				drm_plane_index(plane), state->scaler_id);
 			continue;
 		}
 
-		DRM_DEBUG_KMS("%s PLANE:%d plane: %u.%u idx: %d enabled",
+		DRM_DEBUG_KMS("%s [PLANE:%d:%s] plane: %u.%u idx: %d enabled",
 			plane->type == DRM_PLANE_TYPE_CURSOR ? "CURSOR" : "STANDARD",
-			plane->base.id, intel_plane->pipe,
+			plane->base.id, plane->name,
+			intel_plane->pipe,
 			crtc->base.primary == plane ? 0 : intel_plane->plane + 1,
 			drm_plane_index(plane));
 		DRM_DEBUG_KMS("\tFB:%d, fb = %ux%u format = 0x%x",
-- 
2.4.10

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

[PATCH 5/6] drm/i915: Set crtc->name to "pipe A", "pipe B", etc.

[ville.syrjala]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b628dab..2b5e81a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10711,6 +10711,7 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct intel_unpin_work *work;
+	char *name;
 
 	spin_lock_irq(&dev->event_lock);
 	work = intel_crtc->unpin_work;
@@ -10722,8 +10723,13 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 		kfree(work);
 	}
 
+	/*
+	 * drm_crtc_cleanup() zeroes the structure, so
+	 * need an extra dance to avoid leaking the name.
+	 */
+	name = crtc->name;
 	drm_crtc_cleanup(crtc);
-
+	kfree(name);
 	kfree(intel_crtc);
 }
 
@@ -14036,6 +14042,11 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	if (intel_crtc == NULL)
 		return;
 
+	intel_crtc->base.name = kasprintf(GFP_KERNEL, "pipe %c",
+					  pipe_name(pipe));
+	if (!intel_crtc->base.name)
+		return;
+
 	crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
 	if (!crtc_state)
 		goto fail;
@@ -14106,6 +14117,7 @@ fail:
 	if (cursor)
 		drm_plane_cleanup(cursor);
 	kfree(crtc_state);
+	kfree(intel_crtc->base.name);
 	kfree(intel_crtc);
 }
 
-- 
2.4.10

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

[PATCH 6/6] drm/i915: Give meaningful names to all the planes

[ville.syrjala]

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

Let's name our planes in a way that makes sense wrt. the spec:
- skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc.
- g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc.
- pre-g4x -> "plane A", "cursor B" etc.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2b5e81a..82b2f58 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
 void intel_plane_destroy(struct drm_plane *plane)
 {
 	struct intel_plane *intel_plane = to_intel_plane(plane);
+	char *name;
+
+	/*
+	 * drm_plane_cleanup() zeroes the structure, so
+	 * need an extra dance to avoid leaking the name.
+	 */
+	name = plane->name;
 	drm_plane_cleanup(plane);
+	kfree(name);
 	kfree(intel_plane);
 }
 
@@ -13838,6 +13846,21 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
 		primary->plane = !pipe;
 
+	if (INTEL_INFO(dev)->gen >= 9)
+		primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c",
+					       pipe_name(pipe));
+	else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
+		primary->base.name = kasprintf(GFP_KERNEL, "primary %c",
+					       pipe_name(pipe));
+	else
+		primary->base.name = kasprintf(GFP_KERNEL, "plane %c",
+					       plane_name(primary->plane));
+	if (!primary->base.name) {
+		kfree(state);
+		kfree(primary);
+		return NULL;
+	}
+
 	if (INTEL_INFO(dev)->gen >= 9) {
 		intel_primary_formats = skl_primary_formats;
 		num_formats = ARRAY_SIZE(skl_primary_formats);
@@ -13987,6 +14010,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
 	cursor->commit_plane = intel_commit_cursor_plane;
 	cursor->disable_plane = intel_disable_cursor_plane;
 
+	cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c",
+				      pipe_name(pipe));
+	if (!cursor->base.name) {
+		kfree(state);
+		kfree(cursor);
+		return NULL;
+	}
+
 	drm_universal_plane_init(dev, &cursor->base, 0,
 				 &intel_plane_funcs,
 				 intel_cursor_formats,
@@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 fail:
 	if (primary)
-		drm_plane_cleanup(primary);
+		intel_plane_destroy(primary);
 	if (cursor)
-		drm_plane_cleanup(cursor);
+		intel_plane_destroy(cursor);
 	kfree(crtc_state);
 	kfree(intel_crtc->base.name);
 	kfree(intel_crtc);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a2c15f8..b1520f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1119,7 +1119,21 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 	intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, plane);
 	intel_plane->check_plane = intel_check_sprite_plane;
 	intel_plane->commit_plane = intel_commit_sprite_plane;
+
+	if (INTEL_INFO(dev)->gen >= 9)
+		intel_plane->base.name = kasprintf(GFP_KERNEL, "plane %d%c",
+						   plane + 2, pipe_name(pipe));
+	else
+		intel_plane->base.name = kasprintf(GFP_KERNEL, "sprite %c",
+						   sprite_name(pipe, plane));
+	if (!intel_plane->base.name) {
+		kfree(state);
+		kfree(intel_plane);
+		return -ENOMEM;
+	}
+
 	possible_crtcs = (1 << pipe);
+
 	ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
 				       &intel_plane_funcs,
 				       plane_formats, num_plane_formats,
-- 
2.4.10

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

Re: [PATCH 6/6] drm/i915: Give meaningful names to all the planes

[Emil Velikov]

Hi Ville,

On 12 November 2015 at 16:52,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Let's name our planes in a way that makes sense wrt. the spec:
> - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc.
> - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc.
> - pre-g4x -> "plane A", "cursor B" etc.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_sprite.c  | 14 ++++++++++++++
>  2 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2b5e81a..82b2f58 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
>  void intel_plane_destroy(struct drm_plane *plane)
>  {
>         struct intel_plane *intel_plane = to_intel_plane(plane);
> +       char *name;
> +
> +       /*
> +        * drm_plane_cleanup() zeroes the structure, so
> +        * need an extra dance to avoid leaking the name.
> +        */
> +       name = plane->name;
>         drm_plane_cleanup(plane);
> +       kfree(name);
>         kfree(intel_plane);
>  }
>
> @@ -13838,6 +13846,21 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>         if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
>                 primary->plane = !pipe;
>
> +       if (INTEL_INFO(dev)->gen >= 9)
> +               primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c",
> +                                              pipe_name(pipe));
> +       else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> +               primary->base.name = kasprintf(GFP_KERNEL, "primary %c",
> +                                              pipe_name(pipe));
> +       else
> +               primary->base.name = kasprintf(GFP_KERNEL, "plane %c",
> +                                              plane_name(primary->plane));
> +       if (!primary->base.name) {
> +               kfree(state);
> +               kfree(primary);
> +               return NULL;
Worth adding a label and doing all the teardown there ? (same goes for
the rest of the patch)

> +       }
> +
>         if (INTEL_INFO(dev)->gen >= 9) {
>                 intel_primary_formats = skl_primary_formats;
>                 num_formats = ARRAY_SIZE(skl_primary_formats);
> @@ -13987,6 +14010,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>         cursor->commit_plane = intel_commit_cursor_plane;
>         cursor->disable_plane = intel_disable_cursor_plane;
>
> +       cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c",
> +                                     pipe_name(pipe));
> +       if (!cursor->base.name) {
> +               kfree(state);
> +               kfree(cursor);
> +               return NULL;
> +       }
> +
>         drm_universal_plane_init(dev, &cursor->base, 0,
>                                  &intel_plane_funcs,
>                                  intel_cursor_formats,
> @@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>
>  fail:
>         if (primary)
> -               drm_plane_cleanup(primary);
> +               intel_plane_destroy(primary);
>         if (cursor)
> -               drm_plane_cleanup(cursor);
> +               intel_plane_destroy(cursor);
Something feels strange here. We are either leaking memory before or
we'll end up with double free after your patch. Worth
checking/mentioning in the commit message ?

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

Re: [PATCH 6/6] drm/i915: Give meaningful names to all the planes

[Ville Syrjälä]

On Thu, Nov 12, 2015 at 05:38:48PM +0000, Emil Velikov wrote:
> Hi Ville,
> 
> On 12 November 2015 at 16:52,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Let's name our planes in a way that makes sense wrt. the spec:
> > - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc.
> > - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc.
> > - pre-g4x -> "plane A", "cursor B" etc.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_sprite.c  | 14 ++++++++++++++
> >  2 files changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2b5e81a..82b2f58 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> >  void intel_plane_destroy(struct drm_plane *plane)
> >  {
> >         struct intel_plane *intel_plane = to_intel_plane(plane);
> > +       char *name;
> > +
> > +       /*
> > +        * drm_plane_cleanup() zeroes the structure, so
> > +        * need an extra dance to avoid leaking the name.
> > +        */
> > +       name = plane->name;
> >         drm_plane_cleanup(plane);
> > +       kfree(name);
> >         kfree(intel_plane);
> >  }
> >
> > @@ -13838,6 +13846,21 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> >         if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> >                 primary->plane = !pipe;
> >
> > +       if (INTEL_INFO(dev)->gen >= 9)
> > +               primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c",
> > +                                              pipe_name(pipe));
> > +       else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> > +               primary->base.name = kasprintf(GFP_KERNEL, "primary %c",
> > +                                              pipe_name(pipe));
> > +       else
> > +               primary->base.name = kasprintf(GFP_KERNEL, "plane %c",
> > +                                              plane_name(primary->plane));
> > +       if (!primary->base.name) {
> > +               kfree(state);
> > +               kfree(primary);
> > +               return NULL;
> Worth adding a label and doing all the teardown there ? (same goes for
> the rest of the patch)

Dunno. Was feeling lazy, and so didn't go the extra mile.

> 
> > +       }
> > +
> >         if (INTEL_INFO(dev)->gen >= 9) {
> >                 intel_primary_formats = skl_primary_formats;
> >                 num_formats = ARRAY_SIZE(skl_primary_formats);
> > @@ -13987,6 +14010,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> >         cursor->commit_plane = intel_commit_cursor_plane;
> >         cursor->disable_plane = intel_disable_cursor_plane;
> >
> > +       cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c",
> > +                                     pipe_name(pipe));
> > +       if (!cursor->base.name) {
> > +               kfree(state);
> > +               kfree(cursor);
> > +               return NULL;
> > +       }
> > +
> >         drm_universal_plane_init(dev, &cursor->base, 0,
> >                                  &intel_plane_funcs,
> >                                  intel_cursor_formats,
> > @@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >
> >  fail:
> >         if (primary)
> > -               drm_plane_cleanup(primary);
> > +               intel_plane_destroy(primary);
> >         if (cursor)
> > -               drm_plane_cleanup(cursor);
> > +               intel_plane_destroy(cursor);
> Something feels strange here. We are either leaking memory before or
> we'll end up with double free after your patch. Worth
> checking/mentioning in the commit message ?

Yeah, I think we were leaking here. Forgot to add a note.

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

Re: [PATCH 6/6] drm/i915: Give meaningful names to all the planes

[Ville Syrjälä]

On Thu, Nov 12, 2015 at 07:49:19PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 12, 2015 at 05:38:48PM +0000, Emil Velikov wrote:
> > Hi Ville,
> > 
> > On 12 November 2015 at 16:52,  <ville.syrjala@linux.intel.com> wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Let's name our planes in a way that makes sense wrt. the spec:
> > > - skl+ -> "plane 1A", "plane 2A", "plane 1C", "cursor A" etc.
> > > - g4x+ -> "primary A", "primary B", "sprite A", "cursor C" etc.
> > > - pre-g4x -> "plane A", "cursor B" etc.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/i915/intel_sprite.c  | 14 ++++++++++++++
> > >  2 files changed, 47 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 2b5e81a..82b2f58 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13788,7 +13788,15 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> > >  void intel_plane_destroy(struct drm_plane *plane)
> > >  {
> > >         struct intel_plane *intel_plane = to_intel_plane(plane);
> > > +       char *name;
> > > +
> > > +       /*
> > > +        * drm_plane_cleanup() zeroes the structure, so
> > > +        * need an extra dance to avoid leaking the name.
> > > +        */
> > > +       name = plane->name;
> > >         drm_plane_cleanup(plane);
> > > +       kfree(name);
> > >         kfree(intel_plane);
> > >  }
> > >
> > > @@ -13838,6 +13846,21 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > >         if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
> > >                 primary->plane = !pipe;
> > >
> > > +       if (INTEL_INFO(dev)->gen >= 9)
> > > +               primary->base.name = kasprintf(GFP_KERNEL, "plane 1%c",
> > > +                                              pipe_name(pipe));
> > > +       else if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> > > +               primary->base.name = kasprintf(GFP_KERNEL, "primary %c",
> > > +                                              pipe_name(pipe));
> > > +       else
> > > +               primary->base.name = kasprintf(GFP_KERNEL, "plane %c",
> > > +                                              plane_name(primary->plane));
> > > +       if (!primary->base.name) {
> > > +               kfree(state);
> > > +               kfree(primary);
> > > +               return NULL;
> > Worth adding a label and doing all the teardown there ? (same goes for
> > the rest of the patch)
> 
> Dunno. Was feeling lazy, and so didn't go the extra mile.

After a better look I saw that I fumbled the error paths in the crtc
name patch too. So I went on to clean things up a bit. I think I'll
repost the lot since there are now more patches.

> 
> > 
> > > +       }
> > > +
> > >         if (INTEL_INFO(dev)->gen >= 9) {
> > >                 intel_primary_formats = skl_primary_formats;
> > >                 num_formats = ARRAY_SIZE(skl_primary_formats);
> > > @@ -13987,6 +14010,14 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
> > >         cursor->commit_plane = intel_commit_cursor_plane;
> > >         cursor->disable_plane = intel_disable_cursor_plane;
> > >
> > > +       cursor->base.name = kasprintf(GFP_KERNEL, "cursor %c",
> > > +                                     pipe_name(pipe));
> > > +       if (!cursor->base.name) {
> > > +               kfree(state);
> > > +               kfree(cursor);
> > > +               return NULL;
> > > +       }
> > > +
> > >         drm_universal_plane_init(dev, &cursor->base, 0,
> > >                                  &intel_plane_funcs,
> > >                                  intel_cursor_formats,
> > > @@ -14113,9 +14144,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> > >
> > >  fail:
> > >         if (primary)
> > > -               drm_plane_cleanup(primary);
> > > +               intel_plane_destroy(primary);
> > >         if (cursor)
> > > -               drm_plane_cleanup(cursor);
> > > +               intel_plane_destroy(cursor);
> > Something feels strange here. We are either leaking memory before or
> > we'll end up with double free after your patch. Worth
> > checking/mentioning in the commit message ?
> 
> Yeah, I think we were leaking here. Forgot to add a note.
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 0/6] drm: Give crtcs and planes actual names

[Daniel Vetter]

On Thu, Nov 12, 2015 at 06:52:20PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I got sick and tired of staring at the line noise produced by drm.debug=0x1e,
> so I decided to give the crtcs and planes human readable names. Each driver
> can give whatever names it wants, and for i915 I gave something that makes
> some sense w.r.t. to the spec.
> 
> The only magic gotcha here is that if the driver dynamically allocates the
> name, it must be careful around drm_{crtc,plane}_cleanup() cause those
> guys just memset the entire structure to 0. I didn't want to put the kfree()
> into the cleanup functions to avoid having to kstrdup("") in the fallback
> case or forcing drivers to always use a dynamic allocation.

I avoiding the kstrdup("") is a bit a hack, especially since we could put
somethinig useful in there like "idx-%i", drm_plane_index(). The index is
used by a bunch of things (both internally and in ioctl structs), so
pretty handy.
-Daniel

> 
> Ville Syrjälä (6):
>   drm: Add crtc->name and use it in debug messages
>   drm: Add plane->name and use it in debug prints
>   drm/i915: Use crtc->name in debug messages
>   drm/i915: Use plane->name in debug prints
>   drm/i915: Set crtc->name to "pipe A", "pipe B", etc.
>   drm/i915: Give meaningful names to all the planes
> 
>  drivers/gpu/drm/drm_atomic.c         |  53 ++++++++-------
>  drivers/gpu/drm/drm_atomic_helper.c  |  60 +++++++++--------
>  drivers/gpu/drm/drm_crtc.c           |  11 ++-
>  drivers/gpu/drm/drm_crtc_helper.c    |  24 ++++---
>  drivers/gpu/drm/i915/intel_display.c | 127 +++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_fbdev.c   |   5 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |  14 ++++
>  include/drm/drm_crtc.h               |   4 ++
>  8 files changed, 185 insertions(+), 113 deletions(-)
> 
> -- 
> 2.4.10
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [Intel-gfx] [PATCH 0/6] drm: Give crtcs and planes actual names

[Jani Nikula]

On Tue, 17 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Nov 12, 2015 at 06:52:20PM +0200, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> I got sick and tired of staring at the line noise produced by drm.debug=0x1e,
>> so I decided to give the crtcs and planes human readable names. Each driver
>> can give whatever names it wants, and for i915 I gave something that makes
>> some sense w.r.t. to the spec.
>> 
>> The only magic gotcha here is that if the driver dynamically allocates the
>> name, it must be careful around drm_{crtc,plane}_cleanup() cause those
>> guys just memset the entire structure to 0. I didn't want to put the kfree()
>> into the cleanup functions to avoid having to kstrdup("") in the fallback
>> case or forcing drivers to always use a dynamic allocation.
>
> I avoiding the kstrdup("") is a bit a hack, especially since we could put
> somethinig useful in there like "idx-%i", drm_plane_index(). The index is
> used by a bunch of things (both internally and in ioctl structs), so
> pretty handy.

Find the latest version of the series first. ;)

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel