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