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

[Thomas Zimmermann]

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 at linux.intel.com>
>>>>>
>>>>> 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 at linux.intel.com>
>>>>> ---
>>>>>     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)