diff mbox series

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

Message ID 20240404203336.10454-12-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/client: Use after free and debug improvements | expand

Commit Message

Ville Syrjala April 4, 2024, 8:33 p.m. UTC
From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
---
 drivers/gpu/drm/drm_client_modeset.c | 65 +++++++++++++---------------
 1 file changed, 31 insertions(+), 34 deletions(-)

Comments

Thomas Zimmermann April 5, 2024, 7:49 a.m. UTC | #1
Hi

this patch should probably go before [09/12].

Best regards
Thomas

Am 04.04.24 um 22:33 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@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@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");
>   		conn_configured |= BIT_ULL(i);
>   	}
>   
> @@ -624,6 +630,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>   		struct drm_connector *connector;
>   		struct drm_encoder *encoder;
>   		struct drm_crtc *new_crtc;
> +		const char *mode_type;
>   
>   		connector = connectors[i];
>   
> @@ -673,29 +680,22 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>   		 */
>   		for (j = 0; j < count; j++) {
>   			if (crtcs[j] == new_crtc) {
> -				drm_dbg_kms(dev, "fallback: cloned configuration\n");
> +				drm_dbg_kms(dev, "[CONNECTOR:%d:%s] fallback: cloned configuration\n",
> +					    connector->base.id, connector->name);
>   				goto bail;
>   			}
>   		}
>   
> -		drm_dbg_kms(dev, "looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> -			    connector->base.id, connector->name);
> -
> -		/* go for command line mode first */
> +		mode_type = "cmdline";
>   		modes[i] = drm_connector_pick_cmdline_mode(connector);
>   
> -		/* try for preferred next */
>   		if (!modes[i]) {
> -			drm_dbg_kms(dev, "looking for preferred mode on [CONNECTOR:%d:%s] (tiled? %s)\n",
> -				    connector->base.id, connector->name,
> -				    str_yes_no(connector->has_tile));
> +			mode_type = "preferred";
>   			modes[i] = drm_connector_preferred_mode(connector, width, height);
>   		}
>   
> -		/* No preferred mode marked by the EDID? Are there any modes? */
> -		if (!modes[i] && !list_empty(&connector->modes)) {
> -			drm_dbg_kms(dev, "using first mode listed on [CONNECTOR:%d:%s]\n",
> -				    connector->base.id, connector->name);
> +		if (!modes[i]) {
> +			mode_type = "first";
>   			modes[i] = drm_connector_first_mode(connector);
>   		}
>   
> @@ -706,28 +706,25 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>   			 * is dodgy. Switch to crtc->state->mode, after taking
>   			 * care of the resulting locking/lifetime issues.
>   			 */
> -			drm_dbg_kms(dev, "looking for current mode on [CONNECTOR:%d:%s]\n",
> -				    connector->base.id, connector->name);
> +			mode_type = "current";
>   			modes[i] = &connector->state->crtc->mode;
>   		}
> +
>   		/*
>   		 * In case of tiled modes, if all tiles are not present
>   		 * then fallback to a non tiled mode.
>   		 */
>   		if (connector->has_tile &&
>   		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> -			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);
>   		}
>   		crtcs[i] = new_crtc;
>   
> -		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] on [CRTC:%d:%s]: %dx%d%s\n",
> +		drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
>   			    connector->base.id, connector->name,
> -			    connector->state->crtc->base.id,
> -			    connector->state->crtc->name,
> -			    modes[i]->hdisplay, modes[i]->vdisplay,
> -			    modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" : "");
> +			    new_crtc->base.id, new_crtc->name,
> +			    mode_type, modes[i]->name);
>   
>   		fallback = false;
>   		conn_configured |= BIT(i);
Thomas Zimmermann April 5, 2024, 7:57 a.m. UTC | #2
Hi

Am 04.04.24 um 22:33 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@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@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.

If no mode has been found, the code will later print a warning anyway.

Best regards
Thomas

>   		conn_configured |= BIT_ULL(i);
>   	}
>   
> @@ -624,6 +630,7 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>   		struct drm_connector *connector;
>   		struct drm_encoder *encoder;
>   		struct drm_crtc *new_crtc;
> +		const char *mode_type;
>   
>   		connector = connectors[i];
>   
> @@ -673,29 +680,22 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>   		 */
>   		for (j = 0; j < count; j++) {
>   			if (crtcs[j] == new_crtc) {
> -				drm_dbg_kms(dev, "fallback: cloned configuration\n");
> +				drm_dbg_kms(dev, "[CONNECTOR:%d:%s] fallback: cloned configuration\n",
> +					    connector->base.id, connector->name);
>   				goto bail;
>   			}
>   		}
>   
> -		drm_dbg_kms(dev, "looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> -			    connector->base.id, connector->name);
> -
> -		/* go for command line mode first */
> +		mode_type = "cmdline";
>   		modes[i] = drm_connector_pick_cmdline_mode(connector);
>   
> -		/* try for preferred next */
>   		if (!modes[i]) {
> -			drm_dbg_kms(dev, "looking for preferred mode on [CONNECTOR:%d:%s] (tiled? %s)\n",
> -				    connector->base.id, connector->name,
> -				    str_yes_no(connector->has_tile));
> +			mode_type = "preferred";
>   			modes[i] = drm_connector_preferred_mode(connector, width, height);
>   		}
>   
> -		/* No preferred mode marked by the EDID? Are there any modes? */
> -		if (!modes[i] && !list_empty(&connector->modes)) {
> -			drm_dbg_kms(dev, "using first mode listed on [CONNECTOR:%d:%s]\n",
> -				    connector->base.id, connector->name);
> +		if (!modes[i]) {
> +			mode_type = "first";
>   			modes[i] = drm_connector_first_mode(connector);
>   		}
>   
> @@ -706,28 +706,25 @@ static bool drm_client_firmware_config(struct drm_client_dev *client,
>   			 * is dodgy. Switch to crtc->state->mode, after taking
>   			 * care of the resulting locking/lifetime issues.
>   			 */
> -			drm_dbg_kms(dev, "looking for current mode on [CONNECTOR:%d:%s]\n",
> -				    connector->base.id, connector->name);
> +			mode_type = "current";
>   			modes[i] = &connector->state->crtc->mode;
>   		}
> +
>   		/*
>   		 * In case of tiled modes, if all tiles are not present
>   		 * then fallback to a non tiled mode.
>   		 */
>   		if (connector->has_tile &&
>   		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
> -			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);
>   		}
>   		crtcs[i] = new_crtc;
>   
> -		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] on [CRTC:%d:%s]: %dx%d%s\n",
> +		drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
>   			    connector->base.id, connector->name,
> -			    connector->state->crtc->base.id,
> -			    connector->state->crtc->name,
> -			    modes[i]->hdisplay, modes[i]->vdisplay,
> -			    modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" : "");
> +			    new_crtc->base.id, new_crtc->name,
> +			    mode_type, modes[i]->name);
>   
>   		fallback = false;
>   		conn_configured |= BIT(i);
Ville Syrjala April 5, 2024, 7:58 p.m. UTC | #3
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@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@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.
Thomas Zimmermann April 8, 2024, 7:46 a.m. UTC | #4
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@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@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.

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

>
Ville Syrjala April 8, 2024, 5:26 p.m. UTC | #5
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@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@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.
Thomas Zimmermann April 9, 2024, 8:01 a.m. UTC | #6
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@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@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
diff mbox series

Patch

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");
 		conn_configured |= BIT_ULL(i);
 	}
 
@@ -624,6 +630,7 @@  static bool drm_client_firmware_config(struct drm_client_dev *client,
 		struct drm_connector *connector;
 		struct drm_encoder *encoder;
 		struct drm_crtc *new_crtc;
+		const char *mode_type;
 
 		connector = connectors[i];
 
@@ -673,29 +680,22 @@  static bool drm_client_firmware_config(struct drm_client_dev *client,
 		 */
 		for (j = 0; j < count; j++) {
 			if (crtcs[j] == new_crtc) {
-				drm_dbg_kms(dev, "fallback: cloned configuration\n");
+				drm_dbg_kms(dev, "[CONNECTOR:%d:%s] fallback: cloned configuration\n",
+					    connector->base.id, connector->name);
 				goto bail;
 			}
 		}
 
-		drm_dbg_kms(dev, "looking for cmdline mode on [CONNECTOR:%d:%s]\n",
-			    connector->base.id, connector->name);
-
-		/* go for command line mode first */
+		mode_type = "cmdline";
 		modes[i] = drm_connector_pick_cmdline_mode(connector);
 
-		/* try for preferred next */
 		if (!modes[i]) {
-			drm_dbg_kms(dev, "looking for preferred mode on [CONNECTOR:%d:%s] (tiled? %s)\n",
-				    connector->base.id, connector->name,
-				    str_yes_no(connector->has_tile));
+			mode_type = "preferred";
 			modes[i] = drm_connector_preferred_mode(connector, width, height);
 		}
 
-		/* No preferred mode marked by the EDID? Are there any modes? */
-		if (!modes[i] && !list_empty(&connector->modes)) {
-			drm_dbg_kms(dev, "using first mode listed on [CONNECTOR:%d:%s]\n",
-				    connector->base.id, connector->name);
+		if (!modes[i]) {
+			mode_type = "first";
 			modes[i] = drm_connector_first_mode(connector);
 		}
 
@@ -706,28 +706,25 @@  static bool drm_client_firmware_config(struct drm_client_dev *client,
 			 * is dodgy. Switch to crtc->state->mode, after taking
 			 * care of the resulting locking/lifetime issues.
 			 */
-			drm_dbg_kms(dev, "looking for current mode on [CONNECTOR:%d:%s]\n",
-				    connector->base.id, connector->name);
+			mode_type = "current";
 			modes[i] = &connector->state->crtc->mode;
 		}
+
 		/*
 		 * In case of tiled modes, if all tiles are not present
 		 * then fallback to a non tiled mode.
 		 */
 		if (connector->has_tile &&
 		    num_tiled_conns < connector->num_h_tile * connector->num_v_tile) {
-			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);
 		}
 		crtcs[i] = new_crtc;
 
-		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] on [CRTC:%d:%s]: %dx%d%s\n",
+		drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n",
 			    connector->base.id, connector->name,
-			    connector->state->crtc->base.id,
-			    connector->state->crtc->name,
-			    modes[i]->hdisplay, modes[i]->vdisplay,
-			    modes[i]->flags & DRM_MODE_FLAG_INTERLACE ? "i" : "");
+			    new_crtc->base.id, new_crtc->name,
+			    mode_type, modes[i]->name);
 
 		fallback = false;
 		conn_configured |= BIT(i);