Re: [PATCH] drm/client: Use common display mode for cloned outputs

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 01 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> For cloned outputs, don't pick a default resolution of 1024x768 as
> most hardware can do better. Instead look through the modes of all
> connectors to find a common mode for all of them.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_client_modeset.c | 54 +++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 31af5cf37a09..67b422dc8e7f 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -266,7 +266,7 @@ static bool drm_client_target_cloned(struct drm_device *dev,
>  {
>  	int count, i, j;
>  	bool can_clone = false;
> -	struct drm_display_mode *dmt_mode, *mode;
> +	struct drm_display_mode *mode, *common_mode = NULL;
>  
>  	/* only contemplate cloning in the single crtc case */
>  	if (dev->mode_config.num_crtc > 1)
> @@ -309,35 +309,49 @@ static bool drm_client_target_cloned(struct drm_device *dev,
>  		return true;
>  	}
>  
> -	/* try and find a 1024x768 mode on each connector */
> -	can_clone = true;
> -	dmt_mode = drm_mode_find_dmt(dev, 1024, 768, 60, false);
> -
> -	if (!dmt_mode)
> -		goto fail;
> +	/* try and find a mode common among connectors */
>  
> +	can_clone = false;
>  	for (i = 0; i < connector_count; i++) {
>  		if (!enabled[i])
>  			continue;
>  
> -		list_for_each_entry(mode, &connectors[i]->modes, head) {
> -			if (drm_mode_match(mode, dmt_mode,
> -					   DRM_MODE_MATCH_TIMINGS |
> -					   DRM_MODE_MATCH_CLOCK |
> -					   DRM_MODE_MATCH_FLAGS |
> -					   DRM_MODE_MATCH_3D_FLAGS))
> -				modes[i] = mode;
> +		list_for_each_entry(common_mode, &connectors[i]->modes, head) {
> +			can_clone = true;
> +
> +			for (j = 1; j < connector_count; j++) {

Should this start from i instead of 1?

Anyway, I have a hard time wrapping my head around this whole thing. I
think it would greatly benefit from a helper function to search for a
mode from an array of connectors.

BR,
Jani.


> +				if (!enabled[i])
> +					continue;
> +
> +				can_clone = false;
> +				list_for_each_entry(mode, &connectors[j]->modes, head) {
> +					can_clone = drm_mode_match(common_mode, mode,
> +								   DRM_MODE_MATCH_TIMINGS |
> +							    DRM_MODE_MATCH_CLOCK |
> +							    DRM_MODE_MATCH_FLAGS |
> +							    DRM_MODE_MATCH_3D_FLAGS);
> +					if (can_clone)
> +						break; // found common mode on connector
> +				}
> +				if (!can_clone)
> +					break; // try next common mode
> +			}
> +			if (can_clone)
> +				break; // found common mode among all connectors
>  		}
> -		if (!modes[i])
> -			can_clone = false;
> +		break;
>  	}
> -	kfree(dmt_mode);
> -
>  	if (can_clone) {
> -		drm_dbg_kms(dev, "can clone using 1024x768\n");
> +		for (i = 0; i < connector_count; i++) {
> +			if (!enabled[i])
> +				continue;
> +			modes[i] = common_mode;
> +
> +		}
> +		drm_dbg_kms(dev, "can clone using" DRM_MODE_FMT "\n", DRM_MODE_ARG(common_mode));
>  		return true;
>  	}
> -fail:
> +
>  	drm_info(dev, "kms: can't enable cloning when we probably wanted to.\n");
>  	return false;
>  }

-- 
Jani Nikula, Intel