* [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
@ 2021-11-13 19:27 Claudio Suarez
2021-11-13 20:39 ` Sam Ravnborg
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Claudio Suarez @ 2021-11-13 19:27 UTC (permalink / raw)
To: dri-devel; +Cc: David Airlie
The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
drm core programs.
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Claudio Suarez <cssk@net-c.es>
---
drivers/gpu/drm/drm_client_modeset.c | 51 ++++++++++++++--------------
drivers/gpu/drm/drm_connector.c | 12 ++++---
drivers/gpu/drm/drm_edid.c | 36 ++++++++++----------
drivers/gpu/drm/drm_edid_load.c | 11 +++---
drivers/gpu/drm/drm_mode_config.c | 3 +-
5 files changed, 59 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index ced09c7c06f9..4f35dc375bdd 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -240,7 +240,7 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors,
for (i = 0; i < connector_count; i++) {
connector = connectors[i];
enabled[i] = drm_connector_enabled(connector, true);
- DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
+ DRM_DEBUG_KMS("[CONNECTOR:%d;%s] enabled? %s\n", connector->base.id, connector->name,
connector->display_info.non_desktop ? "non desktop" : enabled[i] ? "yes" : "no");
any_enabled |= enabled[i];
@@ -350,8 +350,8 @@ static int drm_client_get_tile_offsets(struct drm_connector **connectors,
continue;
if (!modes[i] && (h_idx || v_idx)) {
- DRM_DEBUG_KMS("no modes for connector tiled %d %d\n", i,
- connector->base.id);
+ DRM_DEBUG_KMS("no modes for [CONNECTOR:%d:%s] tiled %d\n",
+ connector->base.id, connector->name, i);
continue;
}
if (connector->tile_h_loc < h_idx)
@@ -419,14 +419,15 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
drm_client_get_tile_offsets(connectors, connector_count, modes, offsets, i,
connector->tile_h_loc, connector->tile_v_loc);
}
- DRM_DEBUG_KMS("looking for cmdline mode on connector %d\n",
- connector->base.id);
+ DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
+ connector->base.id, connector->name);
/* got for command line mode first */
modes[i] = drm_connector_pick_cmdline_mode(connector);
if (!modes[i]) {
- DRM_DEBUG_KMS("looking for preferred mode on connector %d %d\n",
- connector->base.id, connector->tile_group ? connector->tile_group->id : 0);
+ DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
+ connector->base.id, connector->name,
+ connector->tile_group ? connector->tile_group->id : 0);
modes[i] = drm_connector_has_preferred_mode(connector, width, height);
}
/* No preferred modes, pick one off the list */
@@ -448,8 +449,8 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
(connector->tile_h_loc == 0 &&
connector->tile_v_loc == 0 &&
!drm_connector_get_tiled_mode(connector))) {
- DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
- connector->base.id);
+ DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
+ connector->base.id, connector->name);
modes[i] = drm_connector_fallback_non_tiled_mode(connector);
} else {
modes[i] = drm_connector_get_tiled_mode(connector);
@@ -617,15 +618,15 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
num_connectors_detected++;
if (!enabled[i]) {
- DRM_DEBUG_KMS("connector %s not enabled, skipping\n",
- connector->name);
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not enabled, skipping\n",
+ connector->base.id, connector->name);
conn_configured |= BIT(i);
continue;
}
if (connector->force == DRM_FORCE_OFF) {
- DRM_DEBUG_KMS("connector %s is disabled by user, skipping\n",
- connector->name);
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s] is disabled by user, skipping\n",
+ connector->base.id, connector->name);
enabled[i] = false;
continue;
}
@@ -635,8 +636,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
if (connector->force > DRM_FORCE_OFF)
goto bail;
- DRM_DEBUG_KMS("connector %s has no encoder or crtc, skipping\n",
- connector->name);
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s] has no encoder or crtc, skipping\n",
+ connector->base.id, connector->name);
enabled[i] = false;
conn_configured |= BIT(i);
continue;
@@ -658,23 +659,23 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
}
}
- DRM_DEBUG_KMS("looking for cmdline mode on connector %s\n",
- connector->name);
+ DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
+ connector->base.id, connector->name);
/* go for command line mode first */
modes[i] = drm_connector_pick_cmdline_mode(connector);
/* try for preferred next */
if (!modes[i]) {
- DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
- connector->name, connector->has_tile);
+ DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
+ connector->base.id, connector->name, connector->has_tile);
modes[i] = drm_connector_has_preferred_mode(connector, width, height);
}
/* No preferred mode marked by the EDID? Are there any modes? */
if (!modes[i] && !list_empty(&connector->modes)) {
- DRM_DEBUG_KMS("using first mode listed on connector %s\n",
- connector->name);
+ DRM_DEBUG_KMS("using first mode listed on [CONNECTOR:%d:%s]\n",
+ connector->base.id, connector->name);
modes[i] = list_first_entry(&connector->modes,
struct drm_display_mode,
head);
@@ -693,8 +694,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
* This is crtc->mode and not crtc->state->mode for the
* fastboot check to work correctly.
*/
- DRM_DEBUG_KMS("looking for current mode on connector %s\n",
- connector->name);
+ DRM_DEBUG_KMS("looking for current mode on [CONNECTOR:%d:%s]\n",
+ connector->base.id, connector->name);
modes[i] = &connector->state->crtc->mode;
}
/*
@@ -703,8 +704,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
*/
if (connector->has_tile &&
num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
- DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
- connector->base.id);
+ DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
+ connector->base.id, connector->name);
modes[i] = drm_connector_fallback_non_tiled_mode(connector);
}
crtcs[i] = new_crtc;
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 3bc782b630b9..a7085f65865f 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -161,20 +161,22 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
return;
if (mode->force) {
- DRM_INFO("forcing %s connector %s\n", connector->name,
+ DRM_INFO("forcing [CONNECTOR:%d:%s]: %s\n",
+ connector->base.id, connector->name,
drm_get_connector_force_name(mode->force));
connector->force = mode->force;
}
if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) {
- DRM_INFO("cmdline forces connector %s panel_orientation to %d\n",
- connector->name, mode->panel_orientation);
+ DRM_INFO("cmdline forces [CONNECTOR:%d:%s] panel_orientation to %d\n",
+ connector->base.id, connector->name,
+ mode->panel_orientation);
drm_connector_set_panel_orientation(connector,
mode->panel_orientation);
}
- DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
- connector->name, mode->name,
+ DRM_DEBUG_KMS("cmdline mode for [CONNECTOR:%d:%s] %s %dx%d@%dHz%s%s%s\n",
+ connector->base.id, connector->name, mode->name,
mode->xres, mode->yres,
mode->refresh_specified ? mode->refresh : 60,
mode->rb ? " reduced blanking" : "",
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 12893e7be89b..242dfcdb7ecb 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5076,32 +5076,32 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
dc_bpc = 10;
info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30;
- DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
- connector->name);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 30.\n",
+ connector->base.id, connector->name);
}
if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
dc_bpc = 12;
info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36;
- DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
- connector->name);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 36.\n",
+ connector->base.id, connector->name);
}
if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
dc_bpc = 16;
info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48;
- DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
- connector->name);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 48.\n",
+ connector->base.id, connector->name);
}
if (dc_bpc == 0) {
- DRM_DEBUG("%s: No deep color support on this HDMI sink.\n",
- connector->name);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: No deep color support on this HDMI sink.\n",
+ connector->base.id, connector->name);
return;
}
- DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
- connector->name, dc_bpc);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning HDMI sink color depth as %d bpc.\n",
+ connector->base.id, connector->name, dc_bpc);
info->bpc = dc_bpc;
/*
@@ -5114,8 +5114,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
/* YCRCB444 is optional according to spec. */
if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
- DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
- connector->name);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does YCRCB444 in deep color.\n",
+ connector->base.id, connector->name);
}
/*
@@ -5123,8 +5123,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
* then deep color 36 bit must be supported.
*/
if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
- DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
- connector->name);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink should do DC_36, but does not!\n",
+ connector->base.id, connector->name);
}
}
@@ -5357,8 +5357,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
if (info->bpc == 0 && edid->revision == 3 &&
edid->input & DRM_EDID_DIGITAL_DFP_1_X) {
info->bpc = 8;
- DRM_DEBUG("%s: Assigning DFP sink color depth as %d bpc.\n",
- connector->name, info->bpc);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning DFP sink color depth as %d bpc.\n",
+ connector->base.id, connector->name, info->bpc);
}
/* Only defined for 1.4 with digital displays */
@@ -5390,8 +5390,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
break;
}
- DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
- connector->name, info->bpc);
+ DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
+ connector->base.id, connector->name, info->bpc);
info->color_formats |= DRM_COLOR_FORMAT_RGB444;
if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..f3f6801927f1 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -188,7 +188,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
if (IS_ERR(pdev)) {
DRM_ERROR("Failed to register EDID firmware platform device "
- "for connector \"%s\"\n", connector_name);
+ "for [CONNECTOR:%d:%s]\n",
+ connector->base.id, connector_name);
return ERR_CAST(pdev);
}
@@ -243,8 +244,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
DRM_INFO("Found %d valid extensions instead of %d in EDID data "
- "\"%s\" for connector \"%s\"\n", valid_extensions,
- edid[0x7e], name, connector_name);
+ "\"%s\" for [CONNECTOR:%d:%s]\n", valid_extensions,
+ edid[0x7e], name, connector->base.id, connector_name);
edid[0x7e] = valid_extensions;
new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
@@ -254,9 +255,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
}
DRM_INFO("Got %s EDID base block and %d extension%s from "
- "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
+ "\"%s\" for [CONNECTOR:%d:%s]\n", (builtin >= 0) ? "built-in" :
"external", valid_extensions, valid_extensions == 1 ? "" : "s",
- name, connector_name);
+ name, connector->base.id, connector_name);
out:
release_firmware(fw);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 37b4b9f0e468..e46dcd31faa3 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -504,7 +504,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
drm_connector_list_iter_begin(dev, &conn_iter);
drm_for_each_connector_iter(connector, &conn_iter)
- DRM_ERROR("connector %s leaked!\n", connector->name);
+ DRM_ERROR("[CONNECTOR:%d:%s] leaked!\n",
+ connector->base.id, connector->name);
drm_connector_list_iter_end(&conn_iter);
}
--
2.33.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-13 19:27 [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name Claudio Suarez
@ 2021-11-13 20:39 ` Sam Ravnborg
2021-11-14 17:34 ` Claudio Suarez
2021-11-14 17:22 ` Claudio Suarez
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Sam Ravnborg @ 2021-11-13 20:39 UTC (permalink / raw)
To: Claudio Suarez; +Cc: David Airlie, dri-devel
Hi Claudio,
On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
> The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> drm core programs.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Claudio Suarez <cssk@net-c.es>
While touching all these logging calls, could you convernt them to the
newer drm_dbg_kms variants?
DRM_DEBUG_* are all deprecated.
Sam
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-13 19:27 [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name Claudio Suarez
2021-11-13 20:39 ` Sam Ravnborg
@ 2021-11-14 17:22 ` Claudio Suarez
2021-11-14 17:25 ` Claudio Suarez
2021-11-15 10:22 ` Jani Nikula
3 siblings, 0 replies; 14+ messages in thread
From: Claudio Suarez @ 2021-11-14 17:22 UTC (permalink / raw)
To: dri-devel; +Cc: David Airlie
+CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
+CC: Daniel Vetter <daniel@ffwll.ch>
On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
> The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> drm core programs.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Claudio Suarez <cssk@net-c.es>
> ---
> drivers/gpu/drm/drm_client_modeset.c | 51 ++++++++++++++--------------
> drivers/gpu/drm/drm_connector.c | 12 ++++---
> drivers/gpu/drm/drm_edid.c | 36 ++++++++++----------
> drivers/gpu/drm/drm_edid_load.c | 11 +++---
> drivers/gpu/drm/drm_mode_config.c | 3 +-
> 5 files changed, 59 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index ced09c7c06f9..4f35dc375bdd 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -240,7 +240,7 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors,
> for (i = 0; i < connector_count; i++) {
> connector = connectors[i];
> enabled[i] = drm_connector_enabled(connector, true);
> - DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> + DRM_DEBUG_KMS("[CONNECTOR:%d;%s] enabled? %s\n", connector->base.id, connector->name,
> connector->display_info.non_desktop ? "non desktop" : enabled[i] ? "yes" : "no");
>
> any_enabled |= enabled[i];
> @@ -350,8 +350,8 @@ static int drm_client_get_tile_offsets(struct drm_connector **connectors,
> continue;
>
> if (!modes[i] && (h_idx || v_idx)) {
> - DRM_DEBUG_KMS("no modes for connector tiled %d %d\n", i,
> - connector->base.id);
> + DRM_DEBUG_KMS("no modes for [CONNECTOR:%d:%s] tiled %d\n",
> + connector->base.id, connector->name, i);
> continue;
> }
> if (connector->tile_h_loc < h_idx)
> @@ -419,14 +419,15 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
> drm_client_get_tile_offsets(connectors, connector_count, modes, offsets, i,
> connector->tile_h_loc, connector->tile_v_loc);
> }
> - DRM_DEBUG_KMS("looking for cmdline mode on connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
>
> /* got for command line mode first */
> modes[i] = drm_connector_pick_cmdline_mode(connector);
> if (!modes[i]) {
> - DRM_DEBUG_KMS("looking for preferred mode on connector %d %d\n",
> - connector->base.id, connector->tile_group ? connector->tile_group->id : 0);
> + DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
> + connector->base.id, connector->name,
> + connector->tile_group ? connector->tile_group->id : 0);
> modes[i] = drm_connector_has_preferred_mode(connector, width, height);
> }
> /* No preferred modes, pick one off the list */
> @@ -448,8 +449,8 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
> (connector->tile_h_loc == 0 &&
> connector->tile_v_loc == 0 &&
> !drm_connector_get_tiled_mode(connector))) {
> - DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = drm_connector_fallback_non_tiled_mode(connector);
> } else {
> modes[i] = drm_connector_get_tiled_mode(connector);
> @@ -617,15 +618,15 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> num_connectors_detected++;
>
> if (!enabled[i]) {
> - DRM_DEBUG_KMS("connector %s not enabled, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not enabled, skipping\n",
> + connector->base.id, connector->name);
> conn_configured |= BIT(i);
> continue;
> }
>
> if (connector->force == DRM_FORCE_OFF) {
> - DRM_DEBUG_KMS("connector %s is disabled by user, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] is disabled by user, skipping\n",
> + connector->base.id, connector->name);
> enabled[i] = false;
> continue;
> }
> @@ -635,8 +636,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> if (connector->force > DRM_FORCE_OFF)
> goto bail;
>
> - DRM_DEBUG_KMS("connector %s has no encoder or crtc, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] has no encoder or crtc, skipping\n",
> + connector->base.id, connector->name);
> enabled[i] = false;
> conn_configured |= BIT(i);
> continue;
> @@ -658,23 +659,23 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> }
> }
>
> - DRM_DEBUG_KMS("looking for cmdline mode on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
>
> /* go for command line mode first */
> modes[i] = drm_connector_pick_cmdline_mode(connector);
>
> /* try for preferred next */
> if (!modes[i]) {
> - DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
> - connector->name, connector->has_tile);
> + DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
> + connector->base.id, connector->name, connector->has_tile);
> modes[i] = drm_connector_has_preferred_mode(connector, width, height);
> }
>
> /* No preferred mode marked by the EDID? Are there any modes? */
> if (!modes[i] && !list_empty(&connector->modes)) {
> - DRM_DEBUG_KMS("using first mode listed on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("using first mode listed on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = list_first_entry(&connector->modes,
> struct drm_display_mode,
> head);
> @@ -693,8 +694,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> * This is crtc->mode and not crtc->state->mode for the
> * fastboot check to work correctly.
> */
> - DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("looking for current mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = &connector->state->crtc->mode;
> }
> /*
> @@ -703,8 +704,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> */
> if (connector->has_tile &&
> num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> - DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = drm_connector_fallback_non_tiled_mode(connector);
> }
> crtcs[i] = new_crtc;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3bc782b630b9..a7085f65865f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -161,20 +161,22 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
> return;
>
> if (mode->force) {
> - DRM_INFO("forcing %s connector %s\n", connector->name,
> + DRM_INFO("forcing [CONNECTOR:%d:%s]: %s\n",
> + connector->base.id, connector->name,
> drm_get_connector_force_name(mode->force));
> connector->force = mode->force;
> }
>
> if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) {
> - DRM_INFO("cmdline forces connector %s panel_orientation to %d\n",
> - connector->name, mode->panel_orientation);
> + DRM_INFO("cmdline forces [CONNECTOR:%d:%s] panel_orientation to %d\n",
> + connector->base.id, connector->name,
> + mode->panel_orientation);
> drm_connector_set_panel_orientation(connector,
> mode->panel_orientation);
> }
>
> - DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
> - connector->name, mode->name,
> + DRM_DEBUG_KMS("cmdline mode for [CONNECTOR:%d:%s] %s %dx%d@%dHz%s%s%s\n",
> + connector->base.id, connector->name, mode->name,
> mode->xres, mode->yres,
> mode->refresh_specified ? mode->refresh : 60,
> mode->rb ? " reduced blanking" : "",
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 12893e7be89b..242dfcdb7ecb 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5076,32 +5076,32 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
> dc_bpc = 10;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30;
> - DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 30.\n",
> + connector->base.id, connector->name);
> }
>
> if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
> dc_bpc = 12;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36;
> - DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 36.\n",
> + connector->base.id, connector->name);
> }
>
> if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
> dc_bpc = 16;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48;
> - DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 48.\n",
> + connector->base.id, connector->name);
> }
>
> if (dc_bpc == 0) {
> - DRM_DEBUG("%s: No deep color support on this HDMI sink.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: No deep color support on this HDMI sink.\n",
> + connector->base.id, connector->name);
> return;
> }
>
> - DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
> - connector->name, dc_bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning HDMI sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, dc_bpc);
> info->bpc = dc_bpc;
>
> /*
> @@ -5114,8 +5114,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> /* YCRCB444 is optional according to spec. */
> if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
> info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
> - DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does YCRCB444 in deep color.\n",
> + connector->base.id, connector->name);
> }
>
> /*
> @@ -5123,8 +5123,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> * then deep color 36 bit must be supported.
> */
> if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
> - DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink should do DC_36, but does not!\n",
> + connector->base.id, connector->name);
> }
> }
>
> @@ -5357,8 +5357,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> if (info->bpc == 0 && edid->revision == 3 &&
> edid->input & DRM_EDID_DIGITAL_DFP_1_X) {
> info->bpc = 8;
> - DRM_DEBUG("%s: Assigning DFP sink color depth as %d bpc.\n",
> - connector->name, info->bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning DFP sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, info->bpc);
> }
>
> /* Only defined for 1.4 with digital displays */
> @@ -5390,8 +5390,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> break;
> }
>
> - DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> - connector->name, info->bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, info->bpc);
>
> info->color_formats |= DRM_COLOR_FORMAT_RGB444;
> if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..f3f6801927f1 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -188,7 +188,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
> pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> if (IS_ERR(pdev)) {
> DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> + "for [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector_name);
> return ERR_CAST(pdev);
> }
>
> @@ -243,8 +244,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>
> edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> DRM_INFO("Found %d valid extensions instead of %d in EDID data "
> - "\"%s\" for connector \"%s\"\n", valid_extensions,
> - edid[0x7e], name, connector_name);
> + "\"%s\" for [CONNECTOR:%d:%s]\n", valid_extensions,
> + edid[0x7e], name, connector->base.id, connector_name);
> edid[0x7e] = valid_extensions;
>
> new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> @@ -254,9 +255,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
> }
>
> DRM_INFO("Got %s EDID base block and %d extension%s from "
> - "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
> + "\"%s\" for [CONNECTOR:%d:%s]\n", (builtin >= 0) ? "built-in" :
> "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> - name, connector_name);
> + name, connector->base.id, connector_name);
>
> out:
> release_firmware(fw);
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 37b4b9f0e468..e46dcd31faa3 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -504,7 +504,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
> drm_connector_list_iter_begin(dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter)
> - DRM_ERROR("connector %s leaked!\n", connector->name);
> + DRM_ERROR("[CONNECTOR:%d:%s] leaked!\n",
> + connector->base.id, connector->name);
> drm_connector_list_iter_end(&conn_iter);
> }
>
> --
> 2.33.0
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-13 19:27 [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name Claudio Suarez
2021-11-13 20:39 ` Sam Ravnborg
2021-11-14 17:22 ` Claudio Suarez
@ 2021-11-14 17:25 ` Claudio Suarez
2021-11-15 10:22 ` Jani Nikula
3 siblings, 0 replies; 14+ messages in thread
From: Claudio Suarez @ 2021-11-14 17:25 UTC (permalink / raw)
To: dri-devel; +Cc: David Airlie
On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
> The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> drm core programs.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Claudio Suarez <cssk@net-c.es>
> ---
> drivers/gpu/drm/drm_client_modeset.c | 51 ++++++++++++++--------------
> drivers/gpu/drm/drm_connector.c | 12 ++++---
> drivers/gpu/drm/drm_edid.c | 36 ++++++++++----------
> drivers/gpu/drm/drm_edid_load.c | 11 +++---
> drivers/gpu/drm/drm_mode_config.c | 3 +-
> 5 files changed, 59 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index ced09c7c06f9..4f35dc375bdd 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -240,7 +240,7 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors,
> for (i = 0; i < connector_count; i++) {
> connector = connectors[i];
> enabled[i] = drm_connector_enabled(connector, true);
> - DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> + DRM_DEBUG_KMS("[CONNECTOR:%d;%s] enabled? %s\n", connector->base.id, connector->name,
> connector->display_info.non_desktop ? "non desktop" : enabled[i] ? "yes" : "no");
>
> any_enabled |= enabled[i];
> @@ -350,8 +350,8 @@ static int drm_client_get_tile_offsets(struct drm_connector **connectors,
> continue;
>
> if (!modes[i] && (h_idx || v_idx)) {
> - DRM_DEBUG_KMS("no modes for connector tiled %d %d\n", i,
> - connector->base.id);
> + DRM_DEBUG_KMS("no modes for [CONNECTOR:%d:%s] tiled %d\n",
> + connector->base.id, connector->name, i);
> continue;
> }
> if (connector->tile_h_loc < h_idx)
> @@ -419,14 +419,15 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
> drm_client_get_tile_offsets(connectors, connector_count, modes, offsets, i,
> connector->tile_h_loc, connector->tile_v_loc);
> }
> - DRM_DEBUG_KMS("looking for cmdline mode on connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
>
> /* got for command line mode first */
> modes[i] = drm_connector_pick_cmdline_mode(connector);
> if (!modes[i]) {
> - DRM_DEBUG_KMS("looking for preferred mode on connector %d %d\n",
> - connector->base.id, connector->tile_group ? connector->tile_group->id : 0);
> + DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
> + connector->base.id, connector->name,
> + connector->tile_group ? connector->tile_group->id : 0);
> modes[i] = drm_connector_has_preferred_mode(connector, width, height);
> }
> /* No preferred modes, pick one off the list */
> @@ -448,8 +449,8 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
> (connector->tile_h_loc == 0 &&
> connector->tile_v_loc == 0 &&
> !drm_connector_get_tiled_mode(connector))) {
> - DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = drm_connector_fallback_non_tiled_mode(connector);
> } else {
> modes[i] = drm_connector_get_tiled_mode(connector);
> @@ -617,15 +618,15 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> num_connectors_detected++;
>
> if (!enabled[i]) {
> - DRM_DEBUG_KMS("connector %s not enabled, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not enabled, skipping\n",
> + connector->base.id, connector->name);
> conn_configured |= BIT(i);
> continue;
> }
>
> if (connector->force == DRM_FORCE_OFF) {
> - DRM_DEBUG_KMS("connector %s is disabled by user, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] is disabled by user, skipping\n",
> + connector->base.id, connector->name);
> enabled[i] = false;
> continue;
> }
> @@ -635,8 +636,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> if (connector->force > DRM_FORCE_OFF)
> goto bail;
>
> - DRM_DEBUG_KMS("connector %s has no encoder or crtc, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] has no encoder or crtc, skipping\n",
> + connector->base.id, connector->name);
> enabled[i] = false;
> conn_configured |= BIT(i);
> continue;
> @@ -658,23 +659,23 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> }
> }
>
> - DRM_DEBUG_KMS("looking for cmdline mode on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
>
> /* go for command line mode first */
> modes[i] = drm_connector_pick_cmdline_mode(connector);
>
> /* try for preferred next */
> if (!modes[i]) {
> - DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
> - connector->name, connector->has_tile);
> + DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
> + connector->base.id, connector->name, connector->has_tile);
> modes[i] = drm_connector_has_preferred_mode(connector, width, height);
> }
>
> /* No preferred mode marked by the EDID? Are there any modes? */
> if (!modes[i] && !list_empty(&connector->modes)) {
> - DRM_DEBUG_KMS("using first mode listed on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("using first mode listed on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = list_first_entry(&connector->modes,
> struct drm_display_mode,
> head);
> @@ -693,8 +694,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> * This is crtc->mode and not crtc->state->mode for the
> * fastboot check to work correctly.
> */
> - DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("looking for current mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = &connector->state->crtc->mode;
> }
> /*
> @@ -703,8 +704,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> */
> if (connector->has_tile &&
> num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> - DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = drm_connector_fallback_non_tiled_mode(connector);
> }
> crtcs[i] = new_crtc;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3bc782b630b9..a7085f65865f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -161,20 +161,22 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
> return;
>
> if (mode->force) {
> - DRM_INFO("forcing %s connector %s\n", connector->name,
> + DRM_INFO("forcing [CONNECTOR:%d:%s]: %s\n",
> + connector->base.id, connector->name,
> drm_get_connector_force_name(mode->force));
> connector->force = mode->force;
> }
>
> if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) {
> - DRM_INFO("cmdline forces connector %s panel_orientation to %d\n",
> - connector->name, mode->panel_orientation);
> + DRM_INFO("cmdline forces [CONNECTOR:%d:%s] panel_orientation to %d\n",
> + connector->base.id, connector->name,
> + mode->panel_orientation);
> drm_connector_set_panel_orientation(connector,
> mode->panel_orientation);
> }
>
> - DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
> - connector->name, mode->name,
> + DRM_DEBUG_KMS("cmdline mode for [CONNECTOR:%d:%s] %s %dx%d@%dHz%s%s%s\n",
> + connector->base.id, connector->name, mode->name,
> mode->xres, mode->yres,
> mode->refresh_specified ? mode->refresh : 60,
> mode->rb ? " reduced blanking" : "",
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 12893e7be89b..242dfcdb7ecb 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5076,32 +5076,32 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
> dc_bpc = 10;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30;
> - DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 30.\n",
> + connector->base.id, connector->name);
> }
>
> if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
> dc_bpc = 12;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36;
> - DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 36.\n",
> + connector->base.id, connector->name);
> }
>
> if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
> dc_bpc = 16;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48;
> - DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 48.\n",
> + connector->base.id, connector->name);
> }
>
> if (dc_bpc == 0) {
> - DRM_DEBUG("%s: No deep color support on this HDMI sink.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: No deep color support on this HDMI sink.\n",
> + connector->base.id, connector->name);
> return;
> }
>
> - DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
> - connector->name, dc_bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning HDMI sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, dc_bpc);
> info->bpc = dc_bpc;
>
> /*
> @@ -5114,8 +5114,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> /* YCRCB444 is optional according to spec. */
> if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
> info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
> - DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does YCRCB444 in deep color.\n",
> + connector->base.id, connector->name);
> }
>
> /*
> @@ -5123,8 +5123,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> * then deep color 36 bit must be supported.
> */
> if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
> - DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink should do DC_36, but does not!\n",
> + connector->base.id, connector->name);
> }
> }
>
> @@ -5357,8 +5357,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> if (info->bpc == 0 && edid->revision == 3 &&
> edid->input & DRM_EDID_DIGITAL_DFP_1_X) {
> info->bpc = 8;
> - DRM_DEBUG("%s: Assigning DFP sink color depth as %d bpc.\n",
> - connector->name, info->bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning DFP sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, info->bpc);
> }
>
> /* Only defined for 1.4 with digital displays */
> @@ -5390,8 +5390,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> break;
> }
>
> - DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> - connector->name, info->bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, info->bpc);
>
> info->color_formats |= DRM_COLOR_FORMAT_RGB444;
> if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..f3f6801927f1 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -188,7 +188,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
> pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> if (IS_ERR(pdev)) {
> DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> + "for [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector_name);
> return ERR_CAST(pdev);
> }
>
> @@ -243,8 +244,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>
> edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> DRM_INFO("Found %d valid extensions instead of %d in EDID data "
> - "\"%s\" for connector \"%s\"\n", valid_extensions,
> - edid[0x7e], name, connector_name);
> + "\"%s\" for [CONNECTOR:%d:%s]\n", valid_extensions,
> + edid[0x7e], name, connector->base.id, connector_name);
> edid[0x7e] = valid_extensions;
>
> new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> @@ -254,9 +255,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
> }
>
> DRM_INFO("Got %s EDID base block and %d extension%s from "
> - "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
> + "\"%s\" for [CONNECTOR:%d:%s]\n", (builtin >= 0) ? "built-in" :
> "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> - name, connector_name);
> + name, connector->base.id, connector_name);
>
> out:
> release_firmware(fw);
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 37b4b9f0e468..e46dcd31faa3 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -504,7 +504,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
> drm_connector_list_iter_begin(dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter)
> - DRM_ERROR("connector %s leaked!\n", connector->name);
> + DRM_ERROR("[CONNECTOR:%d:%s] leaked!\n",
> + connector->base.id, connector->name);
> drm_connector_list_iter_end(&conn_iter);
> }
>
> --
> 2.33.0
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-13 20:39 ` Sam Ravnborg
@ 2021-11-14 17:34 ` Claudio Suarez
2021-11-15 10:24 ` Jani Nikula
0 siblings, 1 reply; 14+ messages in thread
From: Claudio Suarez @ 2021-11-14 17:34 UTC (permalink / raw)
To: Sam Ravnborg, Daniel Vetter, Ville Syrjälä
Cc: David Airlie, dri-devel
On Sat, Nov 13, 2021 at 09:39:46PM +0100, Sam Ravnborg wrote:
> Hi Claudio,
>
> On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> > drm core programs.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Claudio Suarez <cssk@net-c.es>
>
> While touching all these logging calls, could you convernt them to the
> newer drm_dbg_kms variants?
> DRM_DEBUG_* are all deprecated.
>
Yes, I can, but it is recommended to do it in a different patch:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
C&P:
"Separate your changes
Separate each logical change into a separate patch.
For example, if your changes include both bug fixes and performance enhancements..."
I will study and send a new separate patch with your suggestion.
Best regards,
Claudio Suarez
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-13 19:27 [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name Claudio Suarez
` (2 preceding siblings ...)
2021-11-14 17:25 ` Claudio Suarez
@ 2021-11-15 10:22 ` Jani Nikula
2021-11-15 10:24 ` Simon Ser
2021-11-15 19:02 ` Claudio Suarez
3 siblings, 2 replies; 14+ messages in thread
From: Jani Nikula @ 2021-11-15 10:22 UTC (permalink / raw)
To: Claudio Suarez, dri-devel; +Cc: David Airlie, Sam Ravnborg
On Sat, 13 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> drm core programs.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Claudio Suarez <cssk@net-c.es>
> ---
> drivers/gpu/drm/drm_client_modeset.c | 51 ++++++++++++++--------------
> drivers/gpu/drm/drm_connector.c | 12 ++++---
> drivers/gpu/drm/drm_edid.c | 36 ++++++++++----------
> drivers/gpu/drm/drm_edid_load.c | 11 +++---
> drivers/gpu/drm/drm_mode_config.c | 3 +-
> 5 files changed, 59 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index ced09c7c06f9..4f35dc375bdd 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -240,7 +240,7 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors,
> for (i = 0; i < connector_count; i++) {
> connector = connectors[i];
> enabled[i] = drm_connector_enabled(connector, true);
> - DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> + DRM_DEBUG_KMS("[CONNECTOR:%d;%s] enabled? %s\n", connector->base.id, connector->name,
> connector->display_info.non_desktop ? "non desktop" : enabled[i] ? "yes" : "no");
You have a semicolon instead of a colon there.
Not to block this patch, but I've wondered about bigger changes such as:
- Adding "debug_name" member or similar in drm_connector, which would be
an allocated string with "[CONNECTOR:id:name]" that you could use with
just %s while debug logging.
- Adding drm_dbg_connector() which would take drm_connector as context,
and do drm_dbg_kms() with the above prefix.
>
> any_enabled |= enabled[i];
> @@ -350,8 +350,8 @@ static int drm_client_get_tile_offsets(struct drm_connector **connectors,
> continue;
>
> if (!modes[i] && (h_idx || v_idx)) {
> - DRM_DEBUG_KMS("no modes for connector tiled %d %d\n", i,
> - connector->base.id);
> + DRM_DEBUG_KMS("no modes for [CONNECTOR:%d:%s] tiled %d\n",
> + connector->base.id, connector->name, i);
Personally I'd prefer adding [CONNECTOR:id:name] as a prefix in the
beginning, throughout, not in the middle.
BR,
Jani.
> continue;
> }
> if (connector->tile_h_loc < h_idx)
> @@ -419,14 +419,15 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
> drm_client_get_tile_offsets(connectors, connector_count, modes, offsets, i,
> connector->tile_h_loc, connector->tile_v_loc);
> }
> - DRM_DEBUG_KMS("looking for cmdline mode on connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
>
> /* got for command line mode first */
> modes[i] = drm_connector_pick_cmdline_mode(connector);
> if (!modes[i]) {
> - DRM_DEBUG_KMS("looking for preferred mode on connector %d %d\n",
> - connector->base.id, connector->tile_group ? connector->tile_group->id : 0);
> + DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
> + connector->base.id, connector->name,
> + connector->tile_group ? connector->tile_group->id : 0);
> modes[i] = drm_connector_has_preferred_mode(connector, width, height);
> }
> /* No preferred modes, pick one off the list */
> @@ -448,8 +449,8 @@ static bool drm_client_target_preferred(struct drm_connector **connectors,
> (connector->tile_h_loc == 0 &&
> connector->tile_v_loc == 0 &&
> !drm_connector_get_tiled_mode(connector))) {
> - DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = drm_connector_fallback_non_tiled_mode(connector);
> } else {
> modes[i] = drm_connector_get_tiled_mode(connector);
> @@ -617,15 +618,15 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> num_connectors_detected++;
>
> if (!enabled[i]) {
> - DRM_DEBUG_KMS("connector %s not enabled, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] not enabled, skipping\n",
> + connector->base.id, connector->name);
> conn_configured |= BIT(i);
> continue;
> }
>
> if (connector->force == DRM_FORCE_OFF) {
> - DRM_DEBUG_KMS("connector %s is disabled by user, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] is disabled by user, skipping\n",
> + connector->base.id, connector->name);
> enabled[i] = false;
> continue;
> }
> @@ -635,8 +636,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> if (connector->force > DRM_FORCE_OFF)
> goto bail;
>
> - DRM_DEBUG_KMS("connector %s has no encoder or crtc, skipping\n",
> - connector->name);
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] has no encoder or crtc, skipping\n",
> + connector->base.id, connector->name);
> enabled[i] = false;
> conn_configured |= BIT(i);
> continue;
> @@ -658,23 +659,23 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> }
> }
>
> - DRM_DEBUG_KMS("looking for cmdline mode on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
>
> /* go for command line mode first */
> modes[i] = drm_connector_pick_cmdline_mode(connector);
>
> /* try for preferred next */
> if (!modes[i]) {
> - DRM_DEBUG_KMS("looking for preferred mode on connector %s %d\n",
> - connector->name, connector->has_tile);
> + DRM_DEBUG_KMS("looking for preferred mode on [CONNECTOR:%d:%s] %d\n",
> + connector->base.id, connector->name, connector->has_tile);
> modes[i] = drm_connector_has_preferred_mode(connector, width, height);
> }
>
> /* No preferred mode marked by the EDID? Are there any modes? */
> if (!modes[i] && !list_empty(&connector->modes)) {
> - DRM_DEBUG_KMS("using first mode listed on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("using first mode listed on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = list_first_entry(&connector->modes,
> struct drm_display_mode,
> head);
> @@ -693,8 +694,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> * This is crtc->mode and not crtc->state->mode for the
> * fastboot check to work correctly.
> */
> - DRM_DEBUG_KMS("looking for current mode on connector %s\n",
> - connector->name);
> + DRM_DEBUG_KMS("looking for current mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = &connector->state->crtc->mode;
> }
> /*
> @@ -703,8 +704,8 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
> */
> if (connector->has_tile &&
> num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> - DRM_DEBUG_KMS("Falling back to non tiled mode on Connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> modes[i] = drm_connector_fallback_non_tiled_mode(connector);
> }
> crtcs[i] = new_crtc;
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3bc782b630b9..a7085f65865f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -161,20 +161,22 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
> return;
>
> if (mode->force) {
> - DRM_INFO("forcing %s connector %s\n", connector->name,
> + DRM_INFO("forcing [CONNECTOR:%d:%s]: %s\n",
> + connector->base.id, connector->name,
> drm_get_connector_force_name(mode->force));
> connector->force = mode->force;
> }
>
> if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) {
> - DRM_INFO("cmdline forces connector %s panel_orientation to %d\n",
> - connector->name, mode->panel_orientation);
> + DRM_INFO("cmdline forces [CONNECTOR:%d:%s] panel_orientation to %d\n",
> + connector->base.id, connector->name,
> + mode->panel_orientation);
> drm_connector_set_panel_orientation(connector,
> mode->panel_orientation);
> }
>
> - DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
> - connector->name, mode->name,
> + DRM_DEBUG_KMS("cmdline mode for [CONNECTOR:%d:%s] %s %dx%d@%dHz%s%s%s\n",
> + connector->base.id, connector->name, mode->name,
> mode->xres, mode->yres,
> mode->refresh_specified ? mode->refresh : 60,
> mode->rb ? " reduced blanking" : "",
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 12893e7be89b..242dfcdb7ecb 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5076,32 +5076,32 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
> dc_bpc = 10;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30;
> - DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 30.\n",
> + connector->base.id, connector->name);
> }
>
> if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
> dc_bpc = 12;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36;
> - DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 36.\n",
> + connector->base.id, connector->name);
> }
>
> if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
> dc_bpc = 16;
> info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48;
> - DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does deep color 48.\n",
> + connector->base.id, connector->name);
> }
>
> if (dc_bpc == 0) {
> - DRM_DEBUG("%s: No deep color support on this HDMI sink.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: No deep color support on this HDMI sink.\n",
> + connector->base.id, connector->name);
> return;
> }
>
> - DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
> - connector->name, dc_bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning HDMI sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, dc_bpc);
> info->bpc = dc_bpc;
>
> /*
> @@ -5114,8 +5114,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> /* YCRCB444 is optional according to spec. */
> if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
> info->color_formats |= DRM_COLOR_FORMAT_YCRCB444;
> - DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink does YCRCB444 in deep color.\n",
> + connector->base.id, connector->name);
> }
>
> /*
> @@ -5123,8 +5123,8 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
> * then deep color 36 bit must be supported.
> */
> if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
> - DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
> - connector->name);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: HDMI sink should do DC_36, but does not!\n",
> + connector->base.id, connector->name);
> }
> }
>
> @@ -5357,8 +5357,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> if (info->bpc == 0 && edid->revision == 3 &&
> edid->input & DRM_EDID_DIGITAL_DFP_1_X) {
> info->bpc = 8;
> - DRM_DEBUG("%s: Assigning DFP sink color depth as %d bpc.\n",
> - connector->name, info->bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning DFP sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, info->bpc);
> }
>
> /* Only defined for 1.4 with digital displays */
> @@ -5390,8 +5390,8 @@ u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edi
> break;
> }
>
> - DRM_DEBUG("%s: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> - connector->name, info->bpc);
> + DRM_DEBUG("[CONNECTOR:%d:%s]: Assigning EDID-1.4 digital sink color depth as %d bpc.\n",
> + connector->base.id, connector->name, info->bpc);
>
> info->color_formats |= DRM_COLOR_FORMAT_RGB444;
> if (edid->features & DRM_EDID_FEATURE_RGB_YCRCB444)
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 37d8ba3ddb46..f3f6801927f1 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -188,7 +188,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
> pdev = platform_device_register_simple(connector_name, -1, NULL, 0);
> if (IS_ERR(pdev)) {
> DRM_ERROR("Failed to register EDID firmware platform device "
> - "for connector \"%s\"\n", connector_name);
> + "for [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector_name);
> return ERR_CAST(pdev);
> }
>
> @@ -243,8 +244,8 @@ static void *edid_load(struct drm_connector *connector, const char *name,
>
> edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> DRM_INFO("Found %d valid extensions instead of %d in EDID data "
> - "\"%s\" for connector \"%s\"\n", valid_extensions,
> - edid[0x7e], name, connector_name);
> + "\"%s\" for [CONNECTOR:%d:%s]\n", valid_extensions,
> + edid[0x7e], name, connector->base.id, connector_name);
> edid[0x7e] = valid_extensions;
>
> new_edid = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> @@ -254,9 +255,9 @@ static void *edid_load(struct drm_connector *connector, const char *name,
> }
>
> DRM_INFO("Got %s EDID base block and %d extension%s from "
> - "\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
> + "\"%s\" for [CONNECTOR:%d:%s]\n", (builtin >= 0) ? "built-in" :
> "external", valid_extensions, valid_extensions == 1 ? "" : "s",
> - name, connector_name);
> + name, connector->base.id, connector_name);
>
> out:
> release_firmware(fw);
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index 37b4b9f0e468..e46dcd31faa3 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -504,7 +504,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
> drm_connector_list_iter_begin(dev, &conn_iter);
> drm_for_each_connector_iter(connector, &conn_iter)
> - DRM_ERROR("connector %s leaked!\n", connector->name);
> + DRM_ERROR("[CONNECTOR:%d:%s] leaked!\n",
> + connector->base.id, connector->name);
> drm_connector_list_iter_end(&conn_iter);
> }
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-14 17:34 ` Claudio Suarez
@ 2021-11-15 10:24 ` Jani Nikula
2021-11-15 19:40 ` Claudio Suarez
0 siblings, 1 reply; 14+ messages in thread
From: Jani Nikula @ 2021-11-15 10:24 UTC (permalink / raw)
To: Claudio Suarez, Sam Ravnborg, Daniel Vetter, Ville Syrjälä
Cc: David Airlie, dri-devel
On Sun, 14 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> On Sat, Nov 13, 2021 at 09:39:46PM +0100, Sam Ravnborg wrote:
>> Hi Claudio,
>>
>> On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
>> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
>> > drm core programs.
>> >
>> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Claudio Suarez <cssk@net-c.es>
>>
>> While touching all these logging calls, could you convernt them to the
>> newer drm_dbg_kms variants?
>> DRM_DEBUG_* are all deprecated.
>>
>
> Yes, I can, but it is recommended to do it in a different patch:
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
>
> C&P:
> "Separate your changes
> Separate each logical change into a separate patch.
> For example, if your changes include both bug fixes and performance enhancements..."
>
>
> I will study and send a new separate patch with your suggestion.
I feel these logging changes are the types of changes where I'd err on
the side of fewer patches than strictly following the above guidelines.
BR,
Jani.
>
> Best regards,
> Claudio Suarez
>
>
>
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-15 10:22 ` Jani Nikula
@ 2021-11-15 10:24 ` Simon Ser
2021-11-15 10:38 ` Jani Nikula
2021-11-15 19:02 ` Claudio Suarez
1 sibling, 1 reply; 14+ messages in thread
From: Simon Ser @ 2021-11-15 10:24 UTC (permalink / raw)
To: Jani Nikula; +Cc: David Airlie, Sam Ravnborg, Claudio Suarez, dri-devel
On Monday, November 15th, 2021 at 11:22, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> - Adding drm_dbg_connector() which would take drm_connector as context,
> and do drm_dbg_kms() with the above prefix.
This wouldn't work great in case multiple connectors/planes/etc are involved,
or when drm_dbg_atomic() is used.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-15 10:24 ` Simon Ser
@ 2021-11-15 10:38 ` Jani Nikula
0 siblings, 0 replies; 14+ messages in thread
From: Jani Nikula @ 2021-11-15 10:38 UTC (permalink / raw)
To: Simon Ser; +Cc: David Airlie, Sam Ravnborg, Claudio Suarez, dri-devel
On Mon, 15 Nov 2021, Simon Ser <contact@emersion.fr> wrote:
> On Monday, November 15th, 2021 at 11:22, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> - Adding drm_dbg_connector() which would take drm_connector as context,
>> and do drm_dbg_kms() with the above prefix.
>
> This wouldn't work great in case multiple connectors/planes/etc are involved,
> or when drm_dbg_atomic() is used.
That's a good point, though you could still roll those cases manually.
It's also misleading as otherwise drm_dbg_* are categories that can be
enabled/disabled via drm.debug.
Again, just musing here.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-15 10:22 ` Jani Nikula
2021-11-15 10:24 ` Simon Ser
@ 2021-11-15 19:02 ` Claudio Suarez
1 sibling, 0 replies; 14+ messages in thread
From: Claudio Suarez @ 2021-11-15 19:02 UTC (permalink / raw)
To: Jani Nikula; +Cc: David Airlie, Sam Ravnborg, dri-devel
On Mon, Nov 15, 2021 at 12:22:08PM +0200, Jani Nikula wrote:
> On Sat, 13 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> > drm core programs.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Claudio Suarez <cssk@net-c.es>
> > ---
> > drivers/gpu/drm/drm_client_modeset.c | 51 ++++++++++++++--------------
> > drivers/gpu/drm/drm_connector.c | 12 ++++---
> > drivers/gpu/drm/drm_edid.c | 36 ++++++++++----------
> > drivers/gpu/drm/drm_edid_load.c | 11 +++---
> > drivers/gpu/drm/drm_mode_config.c | 3 +-
> > 5 files changed, 59 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> > index ced09c7c06f9..4f35dc375bdd 100644
> > --- a/drivers/gpu/drm/drm_client_modeset.c
> > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > @@ -240,7 +240,7 @@ static void drm_client_connectors_enabled(struct drm_connector **connectors,
> > for (i = 0; i < connector_count; i++) {
> > connector = connectors[i];
> > enabled[i] = drm_connector_enabled(connector, true);
> > - DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> > + DRM_DEBUG_KMS("[CONNECTOR:%d;%s] enabled? %s\n", connector->base.id, connector->name,
> > connector->display_info.non_desktop ? "non desktop" : enabled[i] ? "yes" : "no");
>
> You have a semicolon instead of a colon there.
Sorry my typo. I am going to do a new version.
>
> Not to block this patch, but I've wondered about bigger changes such as:
>
> - Adding "debug_name" member or similar in drm_connector, which would be
> an allocated string with "[CONNECTOR:id:name]" that you could use with
> just %s while debug logging.
>
> - Adding drm_dbg_connector() which would take drm_connector as context,
> and do drm_dbg_kms() with the above prefix.
>
> >
> > any_enabled |= enabled[i];
> > @@ -350,8 +350,8 @@ static int drm_client_get_tile_offsets(struct drm_connector **connectors,
> > continue;
> >
> > if (!modes[i] && (h_idx || v_idx)) {
> > - DRM_DEBUG_KMS("no modes for connector tiled %d %d\n", i,
> > - connector->base.id);
> > + DRM_DEBUG_KMS("no modes for [CONNECTOR:%d:%s] tiled %d\n",
> > + connector->base.id, connector->name, i);
>
> Personally I'd prefer adding [CONNECTOR:id:name] as a prefix in the
> beginning, throughout, not in the middle.
I'd prefer too. I am going to change it in the new version.
B.R.
Claudio Suarez.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-15 10:24 ` Jani Nikula
@ 2021-11-15 19:40 ` Claudio Suarez
2021-11-15 20:17 ` Jani Nikula
2021-11-18 3:41 ` jim.cromie
0 siblings, 2 replies; 14+ messages in thread
From: Claudio Suarez @ 2021-11-15 19:40 UTC (permalink / raw)
To: Jani Nikula; +Cc: David Airlie, dri-devel, Sam Ravnborg
On Mon, Nov 15, 2021 at 12:24:26PM +0200, Jani Nikula wrote:
> On Sun, 14 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> > On Sat, Nov 13, 2021 at 09:39:46PM +0100, Sam Ravnborg wrote:
> >> Hi Claudio,
> >>
> >> On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
> >> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> >> > drm core programs.
> >> >
> >> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Signed-off-by: Claudio Suarez <cssk@net-c.es>
> >>
> >> While touching all these logging calls, could you convernt them to the
> >> newer drm_dbg_kms variants?
> >> DRM_DEBUG_* are all deprecated.
> >>
> >
> > Yes, I can, but it is recommended to do it in a different patch:
> >
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
> >
> > C&P:
> > "Separate your changes
> > Separate each logical change into a separate patch.
> > For example, if your changes include both bug fixes and performance enhancements..."
> >
> >
> > I will study and send a new separate patch with your suggestion.
>
> I feel these logging changes are the types of changes where I'd err on
> the side of fewer patches than strictly following the above guidelines.
To size the problem:
- there are about 3434 references to DRM_DEBUG_* in all the drm code, all drivers.
- there are 413 references to DRM_DEBUG_* in the drm core code, only 66 of
them are related to connectors.
- there are 62 references to DRM_ERR* and 7 references to DRM_INFO in
the drm core programs.
I meant I can make two patches:
1 - this one with the change to CONNECTOR:id:name (29 changes)
2 - a new and bigger patch to change 413 + 62 + 7 references to DRM_{DEBUG,ERR,INFO}
in the drm core programs.
The second patch will be ready in a few days.
Is it a good plan ? or can it be improved ?
Best regards,
Claudio Suarez.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-15 19:40 ` Claudio Suarez
@ 2021-11-15 20:17 ` Jani Nikula
2021-11-16 16:55 ` Claudio Suarez
2021-11-18 3:41 ` jim.cromie
1 sibling, 1 reply; 14+ messages in thread
From: Jani Nikula @ 2021-11-15 20:17 UTC (permalink / raw)
To: Claudio Suarez; +Cc: David Airlie, dri-devel, Sam Ravnborg
On Mon, 15 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> On Mon, Nov 15, 2021 at 12:24:26PM +0200, Jani Nikula wrote:
>> On Sun, 14 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
>> > On Sat, Nov 13, 2021 at 09:39:46PM +0100, Sam Ravnborg wrote:
>> >> Hi Claudio,
>> >>
>> >> On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
>> >> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
>> >> > drm core programs.
>> >> >
>> >> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> > Signed-off-by: Claudio Suarez <cssk@net-c.es>
>> >>
>> >> While touching all these logging calls, could you convernt them to the
>> >> newer drm_dbg_kms variants?
>> >> DRM_DEBUG_* are all deprecated.
>> >>
>> >
>> > Yes, I can, but it is recommended to do it in a different patch:
>> >
>> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
>> >
>> > C&P:
>> > "Separate your changes
>> > Separate each logical change into a separate patch.
>> > For example, if your changes include both bug fixes and performance enhancements..."
>> >
>> >
>> > I will study and send a new separate patch with your suggestion.
>>
>> I feel these logging changes are the types of changes where I'd err on
>> the side of fewer patches than strictly following the above guidelines.
>
> To size the problem:
> - there are about 3434 references to DRM_DEBUG_* in all the drm code, all drivers.
> - there are 413 references to DRM_DEBUG_* in the drm core code, only 66 of
> them are related to connectors.
> - there are 62 references to DRM_ERR* and 7 references to DRM_INFO in
> the drm core programs.
>
> I meant I can make two patches:
> 1 - this one with the change to CONNECTOR:id:name (29 changes)
> 2 - a new and bigger patch to change 413 + 62 + 7 references to DRM_{DEBUG,ERR,INFO}
> in the drm core programs.
The second one is an on-going change that will have to happen gradually,
file by file. Changing connector references while at it seems like a
reasonable drive-by-change to me. (Others may disagree.)
> The second patch will be ready in a few days.
>
> Is it a good plan ? or can it be improved ?
It can't be a single patch. It needs to be split to smaller changes.
BR,
Jani.
>
> Best regards,
> Claudio Suarez.
>
>
>
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-15 20:17 ` Jani Nikula
@ 2021-11-16 16:55 ` Claudio Suarez
0 siblings, 0 replies; 14+ messages in thread
From: Claudio Suarez @ 2021-11-16 16:55 UTC (permalink / raw)
To: Jani Nikula; +Cc: David Airlie, dri-devel, Sam Ravnborg
On Mon, Nov 15, 2021 at 10:17:58PM +0200, Jani Nikula wrote:
> On Mon, 15 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> > On Mon, Nov 15, 2021 at 12:24:26PM +0200, Jani Nikula wrote:
> >> On Sun, 14 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> >> > On Sat, Nov 13, 2021 at 09:39:46PM +0100, Sam Ravnborg wrote:
> >> >> Hi Claudio,
> >> >>
> >> >> On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
> >> >> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> >> >> > drm core programs.
> >> >> >
> >> >> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >> > Signed-off-by: Claudio Suarez <cssk@net-c.es>
> >> >>
> >> >> While touching all these logging calls, could you convernt them to the
> >> >> newer drm_dbg_kms variants?
> >> >> DRM_DEBUG_* are all deprecated.
> >> >>
> >> >
> >> > Yes, I can, but it is recommended to do it in a different patch:
> >> >
> >> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
> >> >
> >> > C&P:
> >> > "Separate your changes
> >> > Separate each logical change into a separate patch.
> >> > For example, if your changes include both bug fixes and performance enhancements..."
> >> >
> >> >
> >> > I will study and send a new separate patch with your suggestion.
> >>
> >> I feel these logging changes are the types of changes where I'd err on
> >> the side of fewer patches than strictly following the above guidelines.
> >
> > To size the problem:
> > - there are about 3434 references to DRM_DEBUG_* in all the drm code, all drivers.
> > - there are 413 references to DRM_DEBUG_* in the drm core code, only 66 of
> > them are related to connectors.
> > - there are 62 references to DRM_ERR* and 7 references to DRM_INFO in
> > the drm core programs.
> >
> > I meant I can make two patches:
> > 1 - this one with the change to CONNECTOR:id:name (29 changes)
> > 2 - a new and bigger patch to change 413 + 62 + 7 references to DRM_{DEBUG,ERR,INFO}
> > in the drm core programs.
>
> The second one is an on-going change that will have to happen gradually,
> file by file. Changing connector references while at it seems like a
> reasonable drive-by-change to me. (Others may disagree.)
There are about 50 files = 50 patches. It seems excessive to me, but
I get the point: smaller changes, so they are easier to control/review/...
I am going so send the patch 1 and one of the patches 2 and we can see.
Thanks,
Claudio Suarez
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
2021-11-15 19:40 ` Claudio Suarez
2021-11-15 20:17 ` Jani Nikula
@ 2021-11-18 3:41 ` jim.cromie
1 sibling, 0 replies; 14+ messages in thread
From: jim.cromie @ 2021-11-18 3:41 UTC (permalink / raw)
To: Claudio Suarez; +Cc: David Airlie, Sam Ravnborg, dri-devel
On Mon, Nov 15, 2021 at 12:40 PM Claudio Suarez <cssk@net-c.es> wrote:
>
> On Mon, Nov 15, 2021 at 12:24:26PM +0200, Jani Nikula wrote:
> > On Sun, 14 Nov 2021, Claudio Suarez <cssk@net-c.es> wrote:
> > > On Sat, Nov 13, 2021 at 09:39:46PM +0100, Sam Ravnborg wrote:
> > >> Hi Claudio,
> > >>
> > >> On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote:
> > >> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> > >> > drm core programs.
> > >> >
> > >> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > Signed-off-by: Claudio Suarez <cssk@net-c.es>
> > >>
> > >> While touching all these logging calls, could you convernt them to the
> > >> newer drm_dbg_kms variants?
> > >> DRM_DEBUG_* are all deprecated.
> > >>
> > >
> > > Yes, I can, but it is recommended to do it in a different patch:
> > >
> > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
> > >
> > > C&P:
> > > "Separate your changes
> > > Separate each logical change into a separate patch.
> > > For example, if your changes include both bug fixes and performance enhancements..."
> > >
> > >
> > > I will study and send a new separate patch with your suggestion.
> >
> > I feel these logging changes are the types of changes where I'd err on
> > the side of fewer patches than strictly following the above guidelines.
>
> To size the problem:
> - there are about 3434 references to DRM_DEBUG_* in all the drm code, all drivers.
> - there are 413 references to DRM_DEBUG_* in the drm core code, only 66 of
> them are related to connectors.
> - there are 62 references to DRM_ERR* and 7 references to DRM_INFO in
> the drm core programs.
>
> I meant I can make two patches:
> 1 - this one with the change to CONNECTOR:id:name (29 changes)
Is there a "committee" that makes or coordinates these log-facing changes ?
The reason I ask is that Ive proposed that the DRM_UT_<Categories>
be replaced by a set of prefix strings; "drm:core:" "drm:kms:" etc.
If the DRM_DEBUG_* etc api were converted to use pr_debug,
then dyndbg could optimize away the drm_debug_enabled() overheads.
dyndbg would enable those classes of drm-debug callsites using
#> echo module drm format drm:kms: > /proc/dyndbg/control
the patchset includes a macro to declare a bit-field to implement drm.debug
https://patchwork.freedesktop.org/series/96328/
how would one (hope to) coordinate changes like this ?
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-11-18 3:41 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-13 19:27 [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name Claudio Suarez
2021-11-13 20:39 ` Sam Ravnborg
2021-11-14 17:34 ` Claudio Suarez
2021-11-15 10:24 ` Jani Nikula
2021-11-15 19:40 ` Claudio Suarez
2021-11-15 20:17 ` Jani Nikula
2021-11-16 16:55 ` Claudio Suarez
2021-11-18 3:41 ` jim.cromie
2021-11-14 17:22 ` Claudio Suarez
2021-11-14 17:25 ` Claudio Suarez
2021-11-15 10:22 ` Jani Nikula
2021-11-15 10:24 ` Simon Ser
2021-11-15 10:38 ` Jani Nikula
2021-11-15 19:02 ` Claudio Suarez
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.