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(-)
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 *
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;
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" :"");
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",
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); }
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,
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));elseprimary->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
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));elseprimary->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.
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));elseprimary->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
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
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.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Jani Nikula -
Ville Syrjälä -
ville.syrjala＠linux.intel.com