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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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)