* [PATCH] drm/client: Use common display mode for cloned outputs
@ 2024-08-01 13:04 Thomas Zimmermann
2024-08-02 8:03 ` Jani Nikula
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Zimmermann @ 2024-08-01 13:04 UTC (permalink / raw)
To: daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel, Thomas Zimmermann
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++) {
+ 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;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/client: Use common display mode for cloned outputs
2024-08-01 13:04 [PATCH] drm/client: Use common display mode for cloned outputs Thomas Zimmermann
@ 2024-08-02 8:03 ` Jani Nikula
2024-08-02 9:12 ` Thomas Zimmermann
0 siblings, 1 reply; 4+ messages in thread
From: Jani Nikula @ 2024-08-02 8:03 UTC (permalink / raw)
To: Thomas Zimmermann, daniel, airlied, mripard, maarten.lankhorst
Cc: dri-devel, Thomas Zimmermann
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/client: Use common display mode for cloned outputs
2024-08-02 8:03 ` Jani Nikula
@ 2024-08-02 9:12 ` Thomas Zimmermann
2024-08-02 9:23 ` Jani Nikula
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Zimmermann @ 2024-08-02 9:12 UTC (permalink / raw)
To: Jani Nikula, daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel
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.
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)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/client: Use common display mode for cloned outputs
2024-08-02 9:12 ` Thomas Zimmermann
@ 2024-08-02 9:23 ` Jani Nikula
0 siblings, 0 replies; 4+ messages in thread
From: Jani Nikula @ 2024-08-02 9:23 UTC (permalink / raw)
To: Thomas Zimmermann, daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-08-02 9:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-01 13:04 [PATCH] drm/client: Use common display mode for cloned outputs Thomas Zimmermann
2024-08-02 8:03 ` Jani Nikula
2024-08-02 9:12 ` Thomas Zimmermann
2024-08-02 9:23 ` Jani Nikula
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.