Re: [PATCH 11/12] drm/client: Streamline mode selection debugs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 05.04.24 um 21:58 schrieb Ville Syrjälä:
On Fri, Apr 05, 2024 at 09:57:07AM +0200, Thomas Zimmermann wrote:
Hi

Am 04.04.24 um 22:33 schrieb Ville Syrjala:
From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>

Get rid of all the redundant debugs and just wait until the end
to print which mode (and of which type) we picked.

Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
---
   drivers/gpu/drm/drm_client_modeset.c | 65 +++++++++++++---------------
   1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 415d1799337b..ad88c11037d8 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -408,6 +408,8 @@ static bool drm_client_target_preferred(struct drm_device *dev,
retry:
   	for (i = 0; i < connector_count; i++) {
+		const char *mode_type;
+
   		connector = connectors[i];
if (conn_configured & BIT_ULL(i))
@@ -440,20 +442,20 @@ static bool drm_client_target_preferred(struct drm_device *dev,
   			drm_client_get_tile_offsets(dev, connectors, connector_count, modes, offsets, i,
   						    connector->tile_h_loc, connector->tile_v_loc);
   		}
-		drm_dbg_kms(dev, "looking for cmdline mode on [CONNECTOR:%d:%s]\n",
-			    connector->base.id, connector->name);
- /* got for command line mode first */
+		mode_type = "cmdline";
   		modes[i] = drm_connector_pick_cmdline_mode(connector);
+
   		if (!modes[i]) {
-			drm_dbg_kms(dev, "looking for preferred mode on [CONNECTOR:%d:%s] (tile group: %d)\n",
-				    connector->base.id, connector->name,
-				    connector->tile_group ? connector->tile_group->id : 0);
+			mode_type = "preferred";
   			modes[i] = drm_connector_preferred_mode(connector, width, height);
   		}
-		/* No preferred modes, pick one off the list */
-		if (!modes[i])
+
+		if (!modes[i]) {
+			mode_type = "first";
   			modes[i] = drm_connector_first_mode(connector);
+		}
+
   		/*
   		 * In case of tiled mode if all tiles not present fallback to
   		 * first available non tiled mode.
@@ -468,16 +470,20 @@ static bool drm_client_target_preferred(struct drm_device *dev,
   			    (connector->tile_h_loc == 0 &&
   			     connector->tile_v_loc == 0 &&
   			     !drm_connector_get_tiled_mode(connector))) {
-				drm_dbg_kms(dev, "Falling back to non tiled mode on [CONNECTOR:%d:%s]\n",
-					    connector->base.id, connector->name);
+				mode_type = "non tiled";
   				modes[i] = drm_connector_fallback_non_tiled_mode(connector);
   			} else {
+				mode_type = "tiled";
   				modes[i] = drm_connector_get_tiled_mode(connector);
   			}
   		}
- drm_dbg_kms(dev, "found mode %s\n",
-			    modes[i] ? modes[i]->name : "none");
+		if (!modes[i])
+			mode_type = "no";
+
+		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] found %s mode: %s\n",
+			    connector->base.id, connector->name,
+			    mode_type, modes[i] ? modes[i]->name : "none");
Instead of tracking the whole mode_type thing, maybe just do

if (!modes[i])
      drm_dbg_kms(dev, "[CONNECTOR:%d:%s] found mode: " DRM_MODE_FMT,
DRM_MODE_ARG(modes[i]) );

to print the full mode.
The point of the mode_type is to indicate how we derived
that mode. Printing the full modeline doesn't help with that.

But do we care where the mode comes from? At least from my experience, it's much more important to know which modes had been available.

If the source of the mode is really important, the old messages seem preferable to me. Debugging code should be trivial and not add logic or flow control to a function IMHO.

Best regards
Thomas



--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux