[PATCH 2/7] drm/probe-helper: Optionally report single connected output per CRTC
Thomas Zimmermann
tzimmermann at suse.de
Tue Jul 16 10:37:18 UTC 2024
Hi
Am 16.07.24 um 11:46 schrieb Daniel Vetter:
> On Mon, Jul 15, 2024 at 11:38:58AM +0200, Thomas Zimmermann wrote:
>> For CRTCs with multiple outputs (i.e., encoders plus connectors),
>> only report at most a single connected output to userspace. Make
>> this configurable via CONFIG_DRM_REPORT_SINGLE_CONNECTED_CRTC_OUTPUT.
>>
>> Having multiple connected outputs on the same CRTC complicates
>> display-mode and format selection, so most userspace does not
>> support this. This is mostly not a problem in practice, as modern
>> display hardware provides a separate CRTC for each output. On
>> old hardware or hardware with BMCs, a single CRTC often drives
>> multiple displays. Only reporting one of them as connected makes
>> the hardware compatible with common userspace.
>>
>> Add the field prioritized_connectors to struct drm_connector. The
>> bitmask signals which other connectors have priority. Also provide
>> the helper drm_probe_helper_prioritize_connectors() that sets
>> default priorities for a given set of connectors. Calling the
>> helper should be enough to set up the functionality for most drivers.
>>
>> With the prioritization bits in place, update connector-status
>> detection to test against prioritized conenctors. So when the probe
>> helpers detect a connector as connected, test against the prioritized
>> connectors. If any is also connected, set the connector status to
>> disconnected.
>>
>> Please note that this functionality is a workaround for limitations
>> in userspace. If your compositor supports multiple outputs per CRTC,
>> CONFIG_DRM_REPORT_SINGLE_CONNECTED_CRTC_OUTPUT should be disabled.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> ---
>> drivers/gpu/drm/Kconfig | 15 +++++
>> drivers/gpu/drm/drm_probe_helper.c | 104 +++++++++++++++++++++++++++++
>> include/drm/drm_connector.h | 2 +
>> include/drm/drm_probe_helper.h | 2 +
>> 4 files changed, 123 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index fd0749c0c630..d1afdbd2d93b 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -105,6 +105,21 @@ config DRM_KMS_HELPER
>> help
>> CRTC helpers for KMS drivers.
>>
>> +config DRM_REPORT_SINGLE_CONNECTED_CRTC_OUTPUT
>> + bool "Report only a single connected output per CRTC"
>> + depends on DRM
>> + default n
>> + help
>> + CRTCs can support multiple encoders and connectors for output.
>> + More than one pair can be connected to a display at a time. Most
>> + userspace only supports at most one connected output per CRTC at a
>> + time. Enable this option to let DRM report at most one connected
>> + output per CRTC. This is mostly relevant for low-end and old
>> + hardware. Most modern graphics hardware supports a separate CRTC
>> + per output and won't be affected by this setting.
>> +
>> + If in doubt, say "Y".
> Uh I think this is way too much, because this defacto makes this uapi for
> all drivers, forever.
>
> The reason we added the hacks for the bmc connectors was the old "no
> regressions" rule: Adding the BMC connectors broke the setup for existing
> users, we can't have that, hence why the hack was needed. For any new
> driver, or for new platforms, we don't have this regression problem.
It's a problem with userspace, not with drivers. The ast and mgag200
drivers would be fixed easily.
Wrt UAPI, drivers have to opt-in by setting the prioritized_connectors
bitmask. Anything but ast and mgag200 will not be affected in any case.
And for ast/mgag200 there's also no change from the current behavior.
>
> So I think the better way to lift this code from ast/mga is if we a lot
> more focused workaround:
>
> - Add a new probe helper for subordinate connectors, they will report
> disconnected if any other connector is connected.
There are no subordinate connectors. They are all equal. The current
patch does what you suggest, but in a generic fashion. I'd otherwise
have to introduce more abstraction to each affected driver to
differentiate between the reported status and the real status.
>
> - Put a really big warning onto that function that it should only be used
> as a workaround for the regression case, not anywhere else.
Isn't that what the patch already does in some way?
>
> - Ideally drivers also don't use that for any new chips where the "no
> regression" rule doesn't apply.
>
> - I wouldn't bother with the Kconfig, because if we make it a global
> option we cannot ever change it anyway. The only way to phase this out
> is by never applying this hack to new hardware support.
I liked Maxime's idea of providing a client cap for userspace that is
not affected.
I don't think that current userspace treats ast and mgag200 any
different from the other drivers. It's just that userspace doesn't
support these drivers' hardware layout. If we add a new driver for new
hardware with similar limitations, it would also be affected. I think
that userspace would end up in a whack-a-mole situation if they start
treating such hardware specifically.
The real fix here is userspace supporting (or at least actively
ignoring) multiple connected outputs per CRTC.
>
> I think it would be also good to link to the specific userspace that falls
> over, and how it falls over. At least hunting around in git history for
> ast/mga200 didn't reveal anything.
AFAIU it's most of current userspace. At least Gnome, but also KDE and
Weston.
Best regards
Thomas
>
> Cheers, Sima
>> +
>> config DRM_PANIC
>> bool "Display a user-friendly message when a kernel panic occurs"
>> depends on DRM && !(FRAMEBUFFER_CONSOLE && VT_CONSOLE)
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index f14301abf53f..fc0652635148 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -352,6 +352,74 @@ static int detect_connector_status(struct drm_connector *connector,
>> return connector_status_connected;
>> }
>>
>> +static int reported_connector_status(struct drm_connector *connector, int detected_status,
>> + struct drm_modeset_acquire_ctx *ctx, bool force)
>> +{
>> +#if defined(CONFIG_DRM_REPORT_SINGLE_CONNECTED_CRTC_OUTPUT)
>> + struct drm_connector *prio_connector = connector;
>> + struct drm_device *dev = connector->dev;
>> + struct drm_connector_list_iter iter;
>> + struct drm_connector *pos;
>> + u32 connector_mask;
>> + int ret = 0;
>> +
>> + if (!connector->prioritized_connectors)
>> + return detected_status;
>> +
>> + if (detected_status != connector_status_connected)
>> + return detected_status;
>> +
>> + connector_mask = drm_connector_mask(connector);
>> +
>> + /*
>> + * Find the connector with status 'connected' and a higher
>> + * priority.
>> + */
>> + drm_connector_list_iter_begin(dev, &iter);
>> + drm_for_each_connector_iter(pos, &iter) {
>> + if (!(drm_connector_mask(pos) & connector->prioritized_connectors))
>> + continue;
>> +
>> + /*
>> + * Warn if connector has priority over itself.
>> + */
>> + if (drm_WARN_ON_ONCE(dev, pos == connector))
>> + continue;
>> +
>> + /*
>> + * Warn if both connectors have priority over each other. Pick the
>> + * one with the lower index.
>> + */
>> + if (drm_WARN_ON_ONCE(dev, pos->prioritized_connectors & connector_mask)) {
>> + if (pos->index > connector->index)
>> + continue;
>> + }
>> +
>> + ret = detect_connector_status(pos, ctx, force);
>> + if (ret < 0)
>> + break;
>> + if (ret == connector_status_disconnected)
>> + continue;
>> +
>> + prio_connector = pos;
>> + break;
>> + }
>> + drm_connector_list_iter_end(&iter);
>> +
>> + if (ret < 0)
>> + return ret;
>> +
>> + /*
>> + * We've found another connected connector. Report our connector
>> + * as 'disconnected'.
>> + */
>> + if (prio_connector != connector)
>> + detected_status = connector_status_disconnected;
>> +#endif
>> +
>> + return detected_status;
>> +}
>> +
>> static enum drm_connector_status
>> drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
>> {
>> @@ -373,6 +441,12 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
>> if (WARN_ON(ret < 0))
>> ret = connector_status_unknown;
>>
>> + ret = reported_connector_status(connector, ret, &ctx, force);
>> + if (ret == -EDEADLK) {
>> + drm_modeset_backoff(&ctx);
>> + goto retry;
>> + }
>> +
>> if (ret != connector->status)
>> connector->epoch_counter += 1;
>>
>> @@ -408,6 +482,7 @@ drm_helper_probe_detect(struct drm_connector *connector,
>> return ret;
>>
>> ret = detect_connector_status(connector, ctx, force);
>> + ret = reported_connector_status(connector, ret, ctx, force);
>>
>> if (ret != connector->status)
>> connector->epoch_counter += 1;
>> @@ -416,6 +491,35 @@ drm_helper_probe_detect(struct drm_connector *connector,
>> }
>> EXPORT_SYMBOL(drm_helper_probe_detect);
>>
>> +/**
>> + * drm_probe_helper_prioritize_connectors - Set connector priorities
>> + * @dev: the DRM device with connectors
>> + * @connector_mask: Bitmask connector indices
>> + *
>> + * drm_probe_helper_prioritize_connectors() prioritizes all connectors
>> + * specified in @connector_mask. All given connectors are assumed to
>> + * interfere with each other. Connectors with a lower index have priority
>> + * over connectors with a higher index.
>> + */
>> +void drm_probe_helper_prioritize_connectors(struct drm_device *dev, u32 connector_mask)
>> +{
>> + struct drm_connector_list_iter iter;
>> + struct drm_connector *connector;
>> + u32 prioritized_connectors = 0;
>> +
>> + drm_connector_list_iter_begin(dev, &iter);
>> + drm_for_each_connector_iter(connector, &iter) {
>> + u32 mask = drm_connector_mask(connector);
>> +
>> + if (!(mask & connector_mask))
>> + continue;
>> + connector->prioritized_connectors = prioritized_connectors;
>> + prioritized_connectors |= mask;
>> + }
>> + drm_connector_list_iter_end(&iter);
>> +}
>> +EXPORT_SYMBOL(drm_probe_helper_prioritize_connectors);
>> +
>> static int drm_helper_probe_get_modes(struct drm_connector *connector)
>> {
>> const struct drm_connector_helper_funcs *connector_funcs =
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index 5ad735253413..e3039478e928 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1985,6 +1985,8 @@ struct drm_connector {
>> /** @epoch_counter: used to detect any other changes in connector, besides status */
>> u64 epoch_counter;
>>
>> + u32 prioritized_connectors;
>> +
>> /**
>> * @possible_encoders: Bit mask of encoders that can drive this
>> * connector, drm_encoder_index() determines the index into the bitfield
>> diff --git a/include/drm/drm_probe_helper.h b/include/drm/drm_probe_helper.h
>> index d6ce7b218b77..05e23485550d 100644
>> --- a/include/drm/drm_probe_helper.h
>> +++ b/include/drm/drm_probe_helper.h
>> @@ -17,6 +17,8 @@ int drm_helper_probe_detect(struct drm_connector *connector,
>> struct drm_modeset_acquire_ctx *ctx,
>> bool force);
>>
>> +void drm_probe_helper_prioritize_connectors(struct drm_device *dev, u32 connector_mask);
>> +
>> int drmm_kms_helper_poll_init(struct drm_device *dev);
>> void drm_kms_helper_poll_init(struct drm_device *dev);
>> void drm_kms_helper_poll_fini(struct drm_device *dev);
>> --
>> 2.45.2
>>
--
--
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)
More information about the dri-devel
mailing list