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

[Thomas Zimmermann]

Hi

Am 02.08.24 um 10:03 schrieb Jani Nikula:
> On Thu, 01 Aug 2024, Thomas Zimmermann <tzimmermann at 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 at 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?

Right, it would make sense.

>
> 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.

That's what it does. Here, the outer-most loop tries to find the first 
enabled connector. For each of its modes, the inner loops test if that 
mode is also present on all other enabled connectors.

All of the client's mode-selection code is fairly obscure. I don't 
really dare touching it.

Best regards
Thomas

>
> 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;
>>   }

-- 
--
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)