[PATCH 0/7] drm/probe-helpers: Work around multi-outputs-per-CRTC problem

[PATCH 0/7] drm/probe-helpers: Work around multi-outputs-per-CRTC problem

[Thomas Zimmermann]

Old or simple hardware only supports a single CRTC with multiple
conenctoed outputs. This breaks most userspace compositors, which
only support a single output per CRTC. This currently happens with
ast and mgag200 drivers. The drivers contain a work around that
dynamically disables all but one connected output.

Patches 1 and 2 push the workaround into probe helpers and make it
configurable in the kernel config. For each connector, the driver
needs to specify a bitmask of connectors with higher priority. If
one of them is connected, the connector at hand is always reported
as disconnected. Connectors without priority bitmask as not affected.

Patches 3 to 5 update and simplify the ast drivers. The new workaround
now allows to have multiple physical conenctors in ast. So patch 5
finally allows VGA and DisplayPort on the same device.

Patches 6 and 7 update mgag200.

Any future driver that exposes the same problem as ast and mgag200
can simply hook into the workaround. Hopefully userspace can be fixed
at some point.

Thomas Zimmermann (7):
  drm/probe-helper: Call connector detect functions in single helper
  drm/probe-helper: Optionally report single connected output per CRTC
  drm/ast: Set connector priorities
  drm/ast: Remove struct ast_bmc_connector
  drm/ast: Support ASTDP and VGA at the same time
  drm/mgag200: Set connector priorities
  drm/mgag200: Remove struct mgag200_bmc_connector

 drivers/gpu/drm/Kconfig                   |  15 +++
 drivers/gpu/drm/ast/ast_drv.h             |  17 +--
 drivers/gpu/drm/ast/ast_main.c            |   2 +-
 drivers/gpu/drm/ast/ast_mode.c            |  49 ++------
 drivers/gpu/drm/drm_probe_helper.c        | 137 +++++++++++++++++++---
 drivers/gpu/drm/mgag200/mgag200_bmc.c     |  44 +------
 drivers/gpu/drm/mgag200/mgag200_drv.h     |   9 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  |   4 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c |   4 +-
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |   4 +-
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |   4 +-
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |   4 +-
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |   4 +-
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |   4 +-
 include/drm/drm_connector.h               |   2 +
 include/drm/drm_probe_helper.h            |   2 +
 16 files changed, 177 insertions(+), 128 deletions(-)

-- 
2.45.2

[PATCH 1/7] drm/probe-helper: Call connector detect functions in single helper

[Thomas Zimmermann]

Move the logic to call the connector's detect helper into a single
internal function. Reduces code dupliction.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 33 +++++++++++++++---------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index bb49d552e671..f14301abf53f 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -338,10 +338,23 @@ void drm_kms_helper_poll_reschedule(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_reschedule);
 
+static int detect_connector_status(struct drm_connector *connector,
+				   struct drm_modeset_acquire_ctx *ctx,
+				   bool force)
+{
+	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+
+	if (funcs->detect_ctx)
+		return funcs->detect_ctx(connector, ctx, force);
+	else if (connector->funcs->detect)
+		return connector->funcs->detect(connector, force);
+
+	return connector_status_connected;
+}
+
 static enum drm_connector_status
 drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 {
-	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
 	struct drm_modeset_acquire_ctx ctx;
 	int ret;
 
@@ -349,14 +362,8 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 
 retry:
 	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
-	if (!ret) {
-		if (funcs->detect_ctx)
-			ret = funcs->detect_ctx(connector, &ctx, force);
-		else if (connector->funcs->detect)
-			ret = connector->funcs->detect(connector, force);
-		else
-			ret = connector_status_connected;
-	}
+	if (!ret)
+		ret = detect_connector_status(connector, &ctx, force);
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
@@ -390,7 +397,6 @@ drm_helper_probe_detect(struct drm_connector *connector,
 			struct drm_modeset_acquire_ctx *ctx,
 			bool force)
 {
-	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
 	struct drm_device *dev = connector->dev;
 	int ret;
 
@@ -401,12 +407,7 @@ drm_helper_probe_detect(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (funcs->detect_ctx)
-		ret = funcs->detect_ctx(connector, ctx, force);
-	else if (connector->funcs->detect)
-		ret = connector->funcs->detect(connector, force);
-	else
-		ret = connector_status_connected;
+	ret = detect_connector_status(connector, ctx, force);
 
 	if (ret != connector->status)
 		connector->epoch_counter += 1;
-- 
2.45.2

Re: [PATCH 1/7] drm/probe-helper: Call connector detect functions in single helper

[Maxime Ripard]

On Mon, 15 Jul 2024 11:38:57 +0200, Thomas Zimmermann wrote:
> Move the logic to call the connector's detect helper into a single
> internal function. Reduces code dupliction.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[PATCH 2/7] drm/probe-helper: Optionally report single connected output per CRTC

[Thomas Zimmermann]

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@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".
+
 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

Re: [PATCH 2/7] drm/probe-helper: Optionally report single connected output per CRTC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

Hi,

On Mon, Jul 15, 2024 at 11:38:58AM GMT, 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.

Do they?

At least the RaspberryPi has multiple connectors on a single CRTC, and
for multiple CRTCs.

My understanding is that it's definitely expected, and any decent
user-space should expect it.

Did you have any bug with this?

And if it was the case, we wouldn't support cloning at all. I couldn't
really find how cloning works exactly, but my understanding was that
it's the difference between drm_encoder.possible_crtcs and
possible_clones: possible_crtcs lists the CRTCs it can be connected to,
and possible_clones the other encoders that can run with in parallel.

> 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@suse.de>

The whole "is it actually needed" discussion aside, I'm not sure it's a
good idea to use a config option for that. Chances are distros will
either enable it or disable it depending on what they/their customers
workload will typically look like, and it won't make the kernel or
compositor's job any easier.

Could we use a client capability for that maybe?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/7] drm/probe-helper: Optionally report single connected output per CRTC

[Thomas Zimmermann]

Hi

Am 16.07.24 um 11:01 schrieb Maxime Ripard:
> Hi,
>
> On Mon, Jul 15, 2024 at 11:38:58AM GMT, 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.
> Do they?

That was my understanding from discussions with Gnome devs. Or at least, 
it's what they officially support.

>
> At least the RaspberryPi has multiple connectors on a single CRTC, and
> for multiple CRTCs.

I think userspace wants at least one CRTC per output, but doesn't care 
about details.

>
> My understanding is that it's definitely expected, and any decent
> user-space should expect it.
>
> Did you have any bug with this?

https://gitlab.gnome.org/GNOME/mutter/-/issues/3157

There was also a lengthy discussion on IRC a few months ago IIRC.

> And if it was the case, we wouldn't support cloning at all. I couldn't
> really find how cloning works exactly, but my understanding was that
> it's the difference between drm_encoder.possible_crtcs and
> possible_clones: possible_crtcs lists the CRTCs it can be connected to,
> and possible_clones the other encoders that can run with in parallel.

I'd prefer to solve this with the possible_clones field. But it didn't 
work. Any ideas on that?

>
>> 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@suse.de>
> The whole "is it actually needed" discussion aside, I'm not sure it's a
> good idea to use a config option for that. Chances are distros will
> either enable it or disable it depending on what they/their customers
> workload will typically look like, and it won't make the kernel or
> compositor's job any easier.
>
> Could we use a client capability for that maybe?

I like this idea.

Best regards
Thomas

>
> Maxime

-- 
--
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)

Re: [PATCH 2/7] drm/probe-helper: Optionally report single connected output per CRTC

[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@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.

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.

- Put a really big warning onto that function that it should only be used
  as a workaround for the regression case, not anywhere else.

- 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 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.

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/7] drm/probe-helper: Optionally report single connected output per CRTC

[Thomas Zimmermann]

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@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)

[PATCH 3/7] drm/ast: Set connector priorities

[Thomas Zimmermann]

Call drm_probe_helper_prioritize_connectors() to set connector
priorities. Guarantees that only a single output is connected at a
time.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index dc8f639e82fd..ac8a3c6b8bf8 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1970,6 +1970,9 @@ int ast_mode_config_init(struct ast_device *ast)
 	if (ret)
 		return ret;
 
+	/* All connectors interfere with each other. */
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	drm_mode_config_reset(dev);
 
 	ret = drmm_kms_helper_poll_init(dev);
-- 
2.45.2

[PATCH 4/7] drm/ast: Remove struct ast_bmc_connector

[Thomas Zimmermann]

Ast's BMC connector tracks the status of an underlying physical
connector and set BMC status accordingly. This functionality has
been moved into probe helpers. The BMC is always connected and the
helpers compute the correct status. Hence, remove the driver's
dedicated code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  | 17 +------------
 drivers/gpu/drm/ast/ast_mode.c | 46 ++++------------------------------
 2 files changed, 6 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index ba3d86973995..faafb696dc74 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -146,21 +146,6 @@ static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
 	return container_of(plane, struct ast_plane, base);
 }
 
-/*
- * BMC
- */
-
-struct ast_bmc_connector {
-	struct drm_connector base;
-	struct drm_connector *physical_connector;
-};
-
-static inline struct ast_bmc_connector *
-to_ast_bmc_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct ast_bmc_connector, base);
-}
-
 /*
  * Device
  */
@@ -208,7 +193,7 @@ struct ast_device {
 		} astdp;
 		struct {
 			struct drm_encoder encoder;
-			struct ast_bmc_connector bmc_connector;
+			struct drm_connector connector;
 		} bmc;
 	} output;
 
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index ac8a3c6b8bf8..f59cb0f8fbb2 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1761,30 +1761,6 @@ static const struct drm_encoder_funcs ast_bmc_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
-static int ast_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
-					       struct drm_modeset_acquire_ctx *ctx,
-					       bool force)
-{
-	struct ast_bmc_connector *bmc_connector = to_ast_bmc_connector(connector);
-	struct drm_connector *physical_connector = bmc_connector->physical_connector;
-
-	/*
-	 * Most user-space compositors cannot handle more than one connected
-	 * connector per CRTC. Hence, we only mark the BMC as connected if the
-	 * physical connector is disconnected. If the physical connector's status
-	 * is connected or unknown, the BMC remains disconnected. This has no
-	 * effect on the output of the BMC.
-	 *
-	 * FIXME: Remove this logic once user-space compositors can handle more
-	 *        than one connector per CRTC. The BMC should always be connected.
-	 */
-
-	if (physical_connector && physical_connector->status == connector_status_disconnected)
-		return connector_status_connected;
-
-	return connector_status_disconnected;
-}
-
 static int ast_bmc_connector_helper_get_modes(struct drm_connector *connector)
 {
 	return drm_add_modes_noedid(connector, 4096, 4096);
@@ -1792,7 +1768,6 @@ static int ast_bmc_connector_helper_get_modes(struct drm_connector *connector)
 
 static const struct drm_connector_helper_funcs ast_bmc_connector_helper_funcs = {
 	.get_modes = ast_bmc_connector_helper_get_modes,
-	.detect_ctx = ast_bmc_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs ast_bmc_connector_funcs = {
@@ -1804,10 +1779,8 @@ static const struct drm_connector_funcs ast_bmc_connector_funcs = {
 };
 
 static int ast_bmc_connector_init(struct drm_device *dev,
-				  struct ast_bmc_connector *bmc_connector,
-				  struct drm_connector *physical_connector)
+				  struct drm_connector *connector)
 {
-	struct drm_connector *connector = &bmc_connector->base;
 	int ret;
 
 	ret = drm_connector_init(dev, connector, &ast_bmc_connector_funcs,
@@ -1817,19 +1790,15 @@ static int ast_bmc_connector_init(struct drm_device *dev,
 
 	drm_connector_helper_add(connector, &ast_bmc_connector_helper_funcs);
 
-	bmc_connector->physical_connector = physical_connector;
-
 	return 0;
 }
 
-static int ast_bmc_output_init(struct ast_device *ast,
-			       struct drm_connector *physical_connector)
+static int ast_bmc_output_init(struct ast_device *ast)
 {
 	struct drm_device *dev = &ast->base;
 	struct drm_crtc *crtc = &ast->crtc;
 	struct drm_encoder *encoder = &ast->output.bmc.encoder;
-	struct ast_bmc_connector *bmc_connector = &ast->output.bmc.bmc_connector;
-	struct drm_connector *connector = &bmc_connector->base;
+	struct drm_connector *connector = &ast->output.bmc.connector;
 	int ret;
 
 	ret = drm_encoder_init(dev, encoder,
@@ -1839,7 +1808,7 @@ static int ast_bmc_output_init(struct ast_device *ast,
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	ret = ast_bmc_connector_init(dev, bmc_connector, physical_connector);
+	ret = ast_bmc_connector_init(dev, connector);
 	if (ret)
 		return ret;
 
@@ -1901,7 +1870,6 @@ static const struct drm_mode_config_funcs ast_mode_config_funcs = {
 int ast_mode_config_init(struct ast_device *ast)
 {
 	struct drm_device *dev = &ast->base;
-	struct drm_connector *physical_connector = NULL;
 	int ret;
 
 	ret = drmm_mutex_init(dev, &ast->modeset_lock);
@@ -1946,27 +1914,23 @@ int ast_mode_config_init(struct ast_device *ast)
 		ret = ast_vga_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.vga.connector;
 	}
 	if (ast->tx_chip_types & AST_TX_SIL164_BIT) {
 		ret = ast_sil164_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.sil164.connector;
 	}
 	if (ast->tx_chip_types & AST_TX_DP501_BIT) {
 		ret = ast_dp501_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.dp501.connector;
 	}
 	if (ast->tx_chip_types & AST_TX_ASTDP_BIT) {
 		ret = ast_astdp_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.astdp.connector;
 	}
-	ret = ast_bmc_output_init(ast, physical_connector);
+	ret = ast_bmc_output_init(ast);
 	if (ret)
 		return ret;
 
-- 
2.45.2

[PATCH 5/7] drm/ast: Support ASTDP and VGA at the same time

[Thomas Zimmermann]

AST2600 can host VGA and DisplayPort outputs. Support both on the
same device. As userspace can often only support a single output per
CRTC, connectors are prioritized against each other by the probe
helpers.

Reported-by: Shixiong Ou <oushixiong@kylinos.cn>
Closes: https://lore.kernel.org/dri-devel/20240711090102.352213-1-oushixiong1025@163.com/
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 0637abb70361..d43aedaa8dd0 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -115,7 +115,7 @@ static void ast_detect_tx_chip(struct ast_device *ast, bool need_post)
 	} else if (IS_AST_GEN7(ast)) {
 		if (ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xD1, TX_TYPE_MASK) ==
 		    ASTDP_DPMCU_TX) {
-			ast->tx_chip_types = AST_TX_ASTDP_BIT;
+			ast->tx_chip_types |= AST_TX_ASTDP_BIT;
 			ast_dp_launch(&ast->base);
 		}
 	}
-- 
2.45.2

Re: [PATCH 5/7] drm/ast: Support ASTDP and VGA at the same time

[oushixiong]

Tested-by: Shixiong Ou <oushixiong@kylinos.cn>

在 2024/7/15 17:39, Thomas Zimmermann 写道:
> AST2600 can host VGA and DisplayPort outputs. Support both on the
> same device. As userspace can often only support a single output per
> CRTC, connectors are prioritized against each other by the probe
> helpers.
>
> Reported-by: Shixiong Ou <oushixiong@kylinos.cn>
> Closes: https://lore.kernel.org/dri-devel/20240711090102.352213-1-oushixiong1025@163.com/
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 0637abb70361..d43aedaa8dd0 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -115,7 +115,7 @@ static void ast_detect_tx_chip(struct ast_device *ast, bool need_post)
>   	} else if (IS_AST_GEN7(ast)) {
>   		if (ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xD1, TX_TYPE_MASK) ==
>   		    ASTDP_DPMCU_TX) {
> -			ast->tx_chip_types = AST_TX_ASTDP_BIT;
> +			ast->tx_chip_types |= AST_TX_ASTDP_BIT;
>   			ast_dp_launch(&ast->base);
>   		}
>   	}

[PATCH 6/7] drm/mgag200: Set connector priorities

[Thomas Zimmermann]

Call drm_probe_helper_prioritize_connectors() to set connector
priorities. Guarantees that only a single output is connected at a
time.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  | 2 ++
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 2 ++
 drivers/gpu/drm/mgag200/mgag200_g200er.c  | 2 ++
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  | 2 ++
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 2 ++
 drivers/gpu/drm/mgag200/mgag200_g200se.c  | 2 ++
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  | 2 ++
 7 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
index 52bf49ead5c5..c8204e8a1daf 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
@@ -222,6 +222,8 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
index e7f89b2a59fd..749baefb0a7e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
@@ -126,6 +126,8 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index 737a48aa9160..e8c84d3258aa 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -262,6 +262,8 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index 8d1ccc2ad94a..e2b13a90b405 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -263,6 +263,8 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index 265f3e95830a..384041e7b839 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -135,6 +135,8 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index cf7f6897838f..95514dcb8c53 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -394,6 +394,8 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index e25477347c3e..0100b4d03b89 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -269,6 +269,8 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
+	drm_probe_helper_prioritize_connectors(dev, GENMASK(dev->mode_config.num_connector, 0));
+
 	return 0;
 }
 
-- 
2.45.2

[PATCH 7/7] drm/mgag200: Remove struct mgag200_bmc_connector

[Thomas Zimmermann]

Mgag200's BMC connector tracks the status of an underlying physical
connector and set BMC status accordingly. This functionality has
been moved into probe helpers. The BMC is always connected and the
helpers compute the correct status. Hence, remove the driver's
dedicated code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_bmc.c     | 44 +++--------------------
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  9 ++---
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  2 +-
 9 files changed, 13 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_bmc.c b/drivers/gpu/drm/mgag200/mgag200_bmc.c
index 45e35dffb3ea..1e0537f9cbea 100644
--- a/drivers/gpu/drm/mgag200/mgag200_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_bmc.c
@@ -9,11 +9,6 @@
 
 #include "mgag200_drv.h"
 
-static struct mgag200_bmc_connector *to_mgag200_bmc_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct mgag200_bmc_connector, base);
-}
-
 void mgag200_bmc_stop_scanout(struct mga_device *mdev)
 {
 	u8 tmp;
@@ -107,30 +102,6 @@ static const struct drm_encoder_funcs mgag200_bmc_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
-static int mgag200_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
-						   struct drm_modeset_acquire_ctx *ctx,
-						   bool force)
-{
-	struct mgag200_bmc_connector *bmc_connector = to_mgag200_bmc_connector(connector);
-	struct drm_connector *physical_connector = bmc_connector->physical_connector;
-
-	/*
-	 * Most user-space compositors cannot handle more than one connected
-	 * connector per CRTC. Hence, we only mark the BMC as connected if the
-	 * physical connector is disconnected. If the physical connector's status
-	 * is connected or unknown, the BMC remains disconnected. This has no
-	 * effect on the output of the BMC.
-	 *
-	 * FIXME: Remove this logic once user-space compositors can handle more
-	 *        than one connector per CRTC. The BMC should always be connected.
-	 */
-
-	if (physical_connector && physical_connector->status == connector_status_disconnected)
-		return connector_status_connected;
-
-	return connector_status_disconnected;
-}
-
 static int mgag200_bmc_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
@@ -142,7 +113,6 @@ static int mgag200_bmc_connector_helper_get_modes(struct drm_connector *connecto
 
 static const struct drm_connector_helper_funcs mgag200_bmc_connector_helper_funcs = {
 	.get_modes = mgag200_bmc_connector_helper_get_modes,
-	.detect_ctx = mgag200_bmc_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs mgag200_bmc_connector_funcs = {
@@ -154,10 +124,8 @@ static const struct drm_connector_funcs mgag200_bmc_connector_funcs = {
 };
 
 static int mgag200_bmc_connector_init(struct drm_device *dev,
-				      struct mgag200_bmc_connector *bmc_connector,
-				      struct drm_connector *physical_connector)
+				      struct drm_connector *connector)
 {
-	struct drm_connector *connector = &bmc_connector->base;
 	int ret;
 
 	ret = drm_connector_init(dev, connector, &mgag200_bmc_connector_funcs,
@@ -166,17 +134,14 @@ static int mgag200_bmc_connector_init(struct drm_device *dev,
 		return ret;
 	drm_connector_helper_add(connector, &mgag200_bmc_connector_helper_funcs);
 
-	bmc_connector->physical_connector = physical_connector;
-
 	return 0;
 }
 
-int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector)
+int mgag200_bmc_output_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder;
-	struct mgag200_bmc_connector *bmc_connector;
 	struct drm_connector *connector;
 	int ret;
 
@@ -187,11 +152,10 @@ int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physi
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	bmc_connector = &mdev->output.bmc.bmc_connector;
-	ret = mgag200_bmc_connector_init(dev, bmc_connector, physical_connector);
+	connector = &mdev->output.bmc.connector;
+	ret = mgag200_bmc_connector_init(dev, connector);
 	if (ret)
 		return ret;
-	connector = &bmc_connector->base;
 
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index f97eaa49b089..abc7a58ed762 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -188,11 +188,6 @@ static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_s
 	return container_of(base, struct mgag200_crtc_state, base);
 }
 
-struct mgag200_bmc_connector {
-	struct drm_connector base;
-	struct drm_connector *physical_connector;
-};
-
 enum mga_type {
 	G200_PCI,
 	G200_AGP,
@@ -285,7 +280,7 @@ struct mga_device {
 		} vga;
 		struct {
 			struct drm_encoder encoder;
-			struct mgag200_bmc_connector bmc_connector;
+			struct drm_connector connector;
 		} bmc;
 	} output;
 };
@@ -433,6 +428,6 @@ int mgag200_vga_output_init(struct mga_device *mdev);
 /* mgag200_bmc.c */
 void mgag200_bmc_stop_scanout(struct mga_device *mdev);
 void mgag200_bmc_start_scanout(struct mga_device *mdev);
-int mgag200_bmc_output_init(struct mga_device *mdev, struct drm_connector *physical_connector);
+int mgag200_bmc_output_init(struct mga_device *mdev);
 
 #endif				/* __MGAG200_DRV_H__ */
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
index c8204e8a1daf..a0f860c394b3 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
@@ -218,7 +218,7 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
+	ret = mgag200_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
index 749baefb0a7e..a7373d6572e7 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
@@ -122,7 +122,7 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
+	ret = mgag200_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index e8c84d3258aa..0407ee540b20 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -258,7 +258,7 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
+	ret = mgag200_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index e2b13a90b405..51530831d5f0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -259,7 +259,7 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
+	ret = mgag200_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index 384041e7b839..f18122230ffb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -131,7 +131,7 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
+	ret = mgag200_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index 95514dcb8c53..847f0859cf62 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -390,7 +390,7 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
+	ret = mgag200_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index 0100b4d03b89..be60237680ca 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -265,7 +265,7 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
 	if (ret)
 		return ret;
 
-	ret = mgag200_bmc_output_init(mdev, &mdev->output.vga.connector);
+	ret = mgag200_bmc_output_init(mdev);
 	if (ret)
 		return ret;
 
-- 
2.45.2

Re: [PATCH 0/7] drm/probe-helpers: Work around multi-outputs-per-CRTC problem

[Thomas Zimmermann]

As discussed on irc, we rather improve the in-kernel DRM client's 
support for cloned outputs (for fbcon) and then remove the hacks from 
the ast and mgag200 drivers. Userspace compositors need to support 
cloned outputs to a minimum.

https://dri.freedesktop.org/~cbrill/dri-log/index.php?channel=dri-devel&date=2024-07-17

Best regards
Thomas

Am 15.07.24 um 11:38 schrieb Thomas Zimmermann:
> Old or simple hardware only supports a single CRTC with multiple
> conenctoed outputs. This breaks most userspace compositors, which
> only support a single output per CRTC. This currently happens with
> ast and mgag200 drivers. The drivers contain a work around that
> dynamically disables all but one connected output.
>
> Patches 1 and 2 push the workaround into probe helpers and make it
> configurable in the kernel config. For each connector, the driver
> needs to specify a bitmask of connectors with higher priority. If
> one of them is connected, the connector at hand is always reported
> as disconnected. Connectors without priority bitmask as not affected.
>
> Patches 3 to 5 update and simplify the ast drivers. The new workaround
> now allows to have multiple physical conenctors in ast. So patch 5
> finally allows VGA and DisplayPort on the same device.
>
> Patches 6 and 7 update mgag200.
>
> Any future driver that exposes the same problem as ast and mgag200
> can simply hook into the workaround. Hopefully userspace can be fixed
> at some point.
>
> Thomas Zimmermann (7):
>    drm/probe-helper: Call connector detect functions in single helper
>    drm/probe-helper: Optionally report single connected output per CRTC
>    drm/ast: Set connector priorities
>    drm/ast: Remove struct ast_bmc_connector
>    drm/ast: Support ASTDP and VGA at the same time
>    drm/mgag200: Set connector priorities
>    drm/mgag200: Remove struct mgag200_bmc_connector
>
>   drivers/gpu/drm/Kconfig                   |  15 +++
>   drivers/gpu/drm/ast/ast_drv.h             |  17 +--
>   drivers/gpu/drm/ast/ast_main.c            |   2 +-
>   drivers/gpu/drm/ast/ast_mode.c            |  49 ++------
>   drivers/gpu/drm/drm_probe_helper.c        | 137 +++++++++++++++++++---
>   drivers/gpu/drm/mgag200/mgag200_bmc.c     |  44 +------
>   drivers/gpu/drm/mgag200/mgag200_drv.h     |   9 +-
>   drivers/gpu/drm/mgag200/mgag200_g200eh.c  |   4 +-
>   drivers/gpu/drm/mgag200/mgag200_g200eh3.c |   4 +-
>   drivers/gpu/drm/mgag200/mgag200_g200er.c  |   4 +-
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c  |   4 +-
>   drivers/gpu/drm/mgag200/mgag200_g200ew3.c |   4 +-
>   drivers/gpu/drm/mgag200/mgag200_g200se.c  |   4 +-
>   drivers/gpu/drm/mgag200/mgag200_g200wb.c  |   4 +-
>   include/drm/drm_connector.h               |   2 +
>   include/drm/drm_probe_helper.h            |   2 +
>   16 files changed, 177 insertions(+), 128 deletions(-)
>

-- 
--
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)

[PATCH 0/7] drm: Add physical status and BMC support to conenctor

[Thomas Zimmermann]

Track a connector's physical status separately from the logical status
and implement BMC support for DRM drivers. Connectors with virtual BMC
stay connected even if no display is physically connected. DRM clients
then continue displaying output to the BMC.

Ast and mgag200 have been doing this for a while. Moving this into
struct drm_connector and probe helpers simplifies htese divers and
makes the functionality available to others. Hibmc is a candidate here.

Patch just simplifies code in probe helpers and has been acked as part
of the series at [1].

Pathces 2 and 3 add the physical status and a BMC flag to struct
drm_connector. Usually physical connector status and regular, logical
status are in sync, so nothing changes for most drivers. If the the
BMC flag has been set, the logical status is always connected. The
probe helpers also take care of sending hotplug events if the physical
status changes.

Patches 4 to 7 update ast and mgag200. Both drivers already implement
their own tracking of physical status, which is now handled by DRM
helpers. Ast also receives two simple bug fixes for cleaning up EDID
properties in the BMC case.

Tested on ast and mgag200 hardware. Another driver that could make use
of this functionality is hibmc.

[1] https://patchwork.freedesktop.org/series/136084/

Thomas Zimmermann (7):
  drm/probe-helper: Call connector detect functions in single helper
  drm: Add physical status to connector
  drm: Add bmc_attached flag to connector
  drm/ast: sil164: Clear EDID if no display is connected
  drm/ast: vga: Clear EDID if no display is connected
  drm/ast: Track physical connector status in struct drm_connector
  drm/mgag200: Track physical connector status in struct drm_connector

 drivers/gpu/drm/ast/ast_dp.c              | 21 +++-------
 drivers/gpu/drm/ast/ast_dp501.c           | 17 ++------
 drivers/gpu/drm/ast/ast_drv.h             | 24 ++---------
 drivers/gpu/drm/ast/ast_sil164.c          | 19 +++------
 drivers/gpu/drm/ast/ast_vga.c             | 30 +++-----------
 drivers/gpu/drm/drm_connector.c           |  1 +
 drivers/gpu/drm/drm_probe_helper.c        | 50 +++++++++++++----------
 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 32 +++------------
 include/drm/drm_connector.h               | 15 +++++++
 9 files changed, 77 insertions(+), 132 deletions(-)

-- 
2.46.0

[PATCH 1/7] drm/probe-helper: Call connector detect functions in single helper

[Thomas Zimmermann]

Move the logic to call the connector's detect helper into a single
internal function. Reduces code duplication.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Acked-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_probe_helper.c | 33 +++++++++++++++---------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 92f21764246f..62a2e5bcb315 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -338,10 +338,23 @@ void drm_kms_helper_poll_reschedule(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_kms_helper_poll_reschedule);
 
+static int detect_connector_status(struct drm_connector *connector,
+				   struct drm_modeset_acquire_ctx *ctx,
+				   bool force)
+{
+	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+
+	if (funcs->detect_ctx)
+		return funcs->detect_ctx(connector, ctx, force);
+	else if (connector->funcs->detect)
+		return connector->funcs->detect(connector, force);
+
+	return connector_status_connected;
+}
+
 static enum drm_connector_status
 drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 {
-	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
 	struct drm_modeset_acquire_ctx ctx;
 	int ret;
 
@@ -349,14 +362,8 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 
 retry:
 	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
-	if (!ret) {
-		if (funcs->detect_ctx)
-			ret = funcs->detect_ctx(connector, &ctx, force);
-		else if (connector->funcs->detect)
-			ret = connector->funcs->detect(connector, force);
-		else
-			ret = connector_status_connected;
-	}
+	if (!ret)
+		ret = detect_connector_status(connector, &ctx, force);
 
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
@@ -390,7 +397,6 @@ drm_helper_probe_detect(struct drm_connector *connector,
 			struct drm_modeset_acquire_ctx *ctx,
 			bool force)
 {
-	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
 	struct drm_device *dev = connector->dev;
 	int ret;
 
@@ -401,12 +407,7 @@ drm_helper_probe_detect(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (funcs->detect_ctx)
-		ret = funcs->detect_ctx(connector, ctx, force);
-	else if (connector->funcs->detect)
-		ret = connector->funcs->detect(connector, force);
-	else
-		ret = connector_status_connected;
+	ret = detect_connector_status(connector, ctx, force);
 
 	if (ret != connector->status)
 		connector->epoch_counter += 1;
-- 
2.46.0