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

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

On Fri, 02 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 02.08.24 um 10:03 schrieb Jani Nikula:
>> 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?
>
> 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.

I mean just refactoring the above loops to smaller pieces.

BR,
Jani.

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

-- 
Jani Nikula, Intel