| Message ID | 20240801130449.104645-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/client: Use common display mode for cloned outputs | expand |
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; > }
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; >> }
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; >>> }
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; }
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(-)