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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 08.04.24 um 19:26 schrieb Ville Syrjälä:
> On Mon, Apr 08, 2024 at 09:46:44AM +0200, Thomas Zimmermann wrote:
> > 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.
> The tiled vs. not-tiled at least could be quite interesting.
> We know there are actual bugs in this code where some tiled
> connectors seem to incorrectly think they aren't tiled
> while others correctly think they are tiled. Seeing that
> spelled out more clearly in the logs might help with triage.
>
> > If the source of the mode is really important, the old messages seem
> > preferable to me.
> The old debugs were somewhat crap. They basically just said
> "looking for mode via <method X>", and then the last one of those
> you saw in the log you assumed was the method used in the end.
> But not all methods even had that debug print. So basically you
> could never be sure what method was used in the end.
>
> > Debugging code should be trivial and not add logic or
> > flow control to a function IMHO.
> It doesn't add anything of the sort. The control flow
> is 100% based on mode==NULL.

Well, you have my r-b for whatever version you want to commit.

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)