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

[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

[PATCH 2/7] drm: Add physical status to connector

[Thomas Zimmermann]

Track the connector's physical status in addition to its logical
status. The latter is directly derived from the former and for most
connectors both values are in sync.

Server chips with BMC, such as Aspeed, Matrox and HiSilicon, often
provide virtual outputs for remote management. Without a connected
display, fbcon or userspace compositors disabek the output and stop
displaying to the BMC.

Connectors have therefore to remain in connected status, even if the
display has been physically disconnected. Tracking both physical and
logical state in separate fields will enable that. The physical status
is transparent to drivers and clients, but changes update the epoch
counter. This generates a hotplug events for clients. Clients will then
pick up changes to resolutions supported, if any.

The ast driver already contains code to track the physical status. This
commit generalizes the logic for use with other drivers. Candidates are
mgag200 and hibmc.

This commit adds the physical status and makes the regular, logical
status a copy of it. A later change will add the flag for BMC support.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_connector.c    |  1 +
 drivers/gpu/drm/drm_probe_helper.c | 13 ++++++++-----
 include/drm/drm_connector.h        |  7 +++++++
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index fc35f47e2849..901d73416f98 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -282,6 +282,7 @@ static int __drm_connector_init(struct drm_device *dev,
 	connector->edid_blob_ptr = NULL;
 	connector->epoch_counter = 0;
 	connector->tile_blob_ptr = NULL;
+	connector->physical_status = connector_status_unknown;
 	connector->status = connector_status_unknown;
 	connector->display_info.panel_orientation =
 		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 62a2e5bcb315..df44be128e72 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -373,7 +373,7 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
 	if (WARN_ON(ret < 0))
 		ret = connector_status_unknown;
 
-	if (ret != connector->status)
+	if (ret != connector->physical_status)
 		connector->epoch_counter += 1;
 
 	drm_modeset_drop_locks(&ctx);
@@ -409,7 +409,7 @@ drm_helper_probe_detect(struct drm_connector *connector,
 
 	ret = detect_connector_status(connector, ctx, force);
 
-	if (ret != connector->status)
+	if (ret != connector->physical_status)
 		connector->epoch_counter += 1;
 
 	return ret;
@@ -588,9 +588,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 	if (connector->force) {
 		if (connector->force == DRM_FORCE_ON ||
 		    connector->force == DRM_FORCE_ON_DIGITAL)
-			connector->status = connector_status_connected;
+			connector->physical_status = connector_status_connected;
 		else
-			connector->status = connector_status_disconnected;
+			connector->physical_status = connector_status_disconnected;
+		connector->status = connector->physical_status;
+
 		if (connector->funcs->force)
 			connector->funcs->force(connector);
 	} else {
@@ -602,7 +604,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 		} else if (WARN(ret < 0, "Invalid return value %i for connector detection\n", ret))
 			ret = connector_status_unknown;
 
-		connector->status = ret;
+		connector->physical_status = ret;
+		connector->status = connector->physical_status;
 	}
 
 	/*
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index e3fa43291f44..37e951f04ae8 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1817,6 +1817,13 @@ struct drm_connector {
 	 */
 	struct list_head modes;
 
+	/**
+	 * @physical_status:
+	 * One of the drm_connector_status enums (connected, not, or unknown).
+	 * Protected by &drm_mode_config.mutex.
+	 */
+	enum drm_connector_status physical_status;
+
 	/**
 	 * @status:
 	 * One of the drm_connector_status enums (connected, not, or unknown).
-- 
2.46.0

[PATCH 3/7] drm: Add bmc_attached flag to connector

[Thomas Zimmermann]

Add the bmc_attached flag to struct drm_connector to signal the
presence of a virtual BMC output. The connector reports to be in
status connected even without a physically connected display. Fbcon
or userspace compositors would otherwise stop displaying to the
BMC.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_probe_helper.c | 6 +++++-
 include/drm/drm_connector.h        | 8 ++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index df44be128e72..83c3f2d42d49 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -605,7 +605,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 			ret = connector_status_unknown;
 
 		connector->physical_status = ret;
-		connector->status = connector->physical_status;
+
+		if (connector->bmc_attached)
+			connector->status = connector_status_connected;
+		else
+			connector->status = connector->physical_status;
 	}
 
 	/*
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 37e951f04ae8..ed360ae35f21 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1802,6 +1802,14 @@ struct drm_connector {
 	 */
 	bool ycbcr_420_allowed;
 
+	/**
+	 * @ bmc_attached:
+	 * The connector has a BMC transparently attached to it. It has to
+	 * report a connected status, even without a physically connected
+	 * display.
+	 */
+	bool bmc_attached;
+
 	/**
 	 * @registration_state: Is this connector initializing, exposed
 	 * (registered) with userspace, or unregistered?
-- 
2.46.0

[PATCH 4/7] drm/ast: sil164: Clear EDID if no display is connected

[Thomas Zimmermann]

Do not keep the obsolete EDID around after unplugging the display
form the connector.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d20c2f846428 ("drm/ast: sil164: Transparently handle BMC support")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/ast/ast_sil164.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_sil164.c b/drivers/gpu/drm/ast/ast_sil164.c
index 6a72268d2314..be01254dd48a 100644
--- a/drivers/gpu/drm/ast/ast_sil164.c
+++ b/drivers/gpu/drm/ast/ast_sil164.c
@@ -29,6 +29,8 @@ static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector
 	if (ast_connector->physical_status == connector_status_connected) {
 		count = drm_connector_helper_get_modes(connector);
 	} else {
+		drm_edid_connector_update(connector, NULL);
+
 		/*
 		 * There's no EDID data without a connected monitor. Set BMC-
 		 * compatible modes in this case. The XGA default resolution
-- 
2.46.0

[PATCH 5/7] drm/ast: vga: Clear EDID if no display is connected

[Thomas Zimmermann]

Do not keep the obsolete EDID around after unplugging the display
form the connector.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 2a2391f857cd ("drm/ast: vga: Transparently handle BMC support")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/ast/ast_vga.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_vga.c b/drivers/gpu/drm/ast/ast_vga.c
index 5c79b773af57..abe0fff8485c 100644
--- a/drivers/gpu/drm/ast/ast_vga.c
+++ b/drivers/gpu/drm/ast/ast_vga.c
@@ -29,6 +29,8 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 	if (ast_connector->physical_status == connector_status_connected) {
 		count = drm_connector_helper_get_modes(connector);
 	} else {
+		drm_edid_connector_update(connector, NULL);
+
 		/*
 		 * There's no EDID data without a connected monitor. Set BMC-
 		 * compatible modes in this case. The XGA default resolution
-- 
2.46.0

[PATCH 6/7] drm/ast: Track physical connector status in struct drm_connector

[Thomas Zimmermann]

Set bmc_attached for all connectors and let DRM's probe helpers
track the physical and logical connector state. Remove such logic
and related data structures from ast.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 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 | 17 ++++-------------
 drivers/gpu/drm/ast/ast_vga.c    | 28 ++++------------------------
 5 files changed, 22 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
index 0e282b7b167c..b62c39479367 100644
--- a/drivers/gpu/drm/ast/ast_dp.c
+++ b/drivers/gpu/drm/ast/ast_dp.c
@@ -328,9 +328,9 @@ static void ast_astdp_encoder_helper_atomic_enable(struct drm_encoder *encoder,
 						   struct drm_atomic_state *state)
 {
 	struct ast_device *ast = to_ast_device(encoder->dev);
-	struct ast_connector *ast_connector = &ast->output.astdp.connector;
+	struct drm_connector *connector = &ast->output.astdp.connector;
 
-	if (ast_connector->physical_status == connector_status_connected) {
+	if (connector->physical_status == connector_status_connected) {
 		ast_dp_set_phy_sleep(ast, false);
 		ast_dp_link_training(ast);
 
@@ -360,10 +360,9 @@ static const struct drm_encoder_helper_funcs ast_astdp_encoder_helper_funcs = {
 
 static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
 	int count;
 
-	if (ast_connector->physical_status == connector_status_connected) {
+	if (connector->physical_status == connector_status_connected) {
 		struct ast_device *ast = to_ast_device(connector->dev);
 		const struct drm_edid *drm_edid;
 
@@ -391,7 +390,6 @@ static int ast_astdp_connector_helper_detect_ctx(struct drm_connector *connector
 						 struct drm_modeset_acquire_ctx *ctx,
 						 bool force)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
 	struct ast_device *ast = to_ast_device(connector->dev);
 	enum drm_connector_status status = connector_status_disconnected;
 	bool phy_sleep;
@@ -410,11 +408,7 @@ static int ast_astdp_connector_helper_detect_ctx(struct drm_connector *connector
 
 	mutex_unlock(&ast->modeset_lock);
 
-	if (status != ast_connector->physical_status)
-		++connector->epoch_counter;
-	ast_connector->physical_status = status;
-
-	return connector_status_connected;
+	return status;
 }
 
 static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
@@ -439,7 +433,6 @@ int ast_astdp_output_init(struct ast_device *ast)
 	struct drm_device *dev = &ast->base;
 	struct drm_crtc *crtc = &ast->crtc;
 	struct drm_encoder *encoder;
-	struct ast_connector *ast_connector;
 	struct drm_connector *connector;
 	int ret;
 
@@ -456,8 +449,7 @@ int ast_astdp_output_init(struct ast_device *ast)
 
 	/* connector */
 
-	ast_connector = &ast->output.astdp.connector;
-	connector = &ast_connector->base;
+	connector = &ast->output.astdp.connector;
 	ret = drm_connector_init(dev, connector, &ast_astdp_connector_funcs,
 				 DRM_MODE_CONNECTOR_DisplayPort);
 	if (ret)
@@ -466,10 +458,9 @@ int ast_astdp_output_init(struct ast_device *ast)
 
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
+	connector->bmc_attached = true;
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
-	ast_connector->physical_status = connector->status;
-
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 9e19d8c17730..8ffe30c74d3d 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -503,10 +503,9 @@ static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = {
 
 static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
 	int count;
 
-	if (ast_connector->physical_status == connector_status_connected) {
+	if (connector->physical_status == connector_status_connected) {
 		struct ast_device *ast = to_ast_device(connector->dev);
 		const struct drm_edid *drm_edid;
 
@@ -534,18 +533,13 @@ static int ast_dp501_connector_helper_detect_ctx(struct drm_connector *connector
 						 struct drm_modeset_acquire_ctx *ctx,
 						 bool force)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
 	struct ast_device *ast = to_ast_device(connector->dev);
 	enum drm_connector_status status = connector_status_disconnected;
 
 	if (ast_dp501_is_connected(ast))
 		status = connector_status_connected;
 
-	if (status != ast_connector->physical_status)
-		++connector->epoch_counter;
-	ast_connector->physical_status = status;
-
-	return connector_status_connected;
+	return status;
 }
 
 static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
@@ -570,7 +564,6 @@ int ast_dp501_output_init(struct ast_device *ast)
 	struct drm_device *dev = &ast->base;
 	struct drm_crtc *crtc = &ast->crtc;
 	struct drm_encoder *encoder;
-	struct ast_connector *ast_connector;
 	struct drm_connector *connector;
 	int ret;
 
@@ -587,8 +580,7 @@ int ast_dp501_output_init(struct ast_device *ast)
 
 	/* connector */
 
-	ast_connector = &ast->output.dp501.connector;
-	connector = &ast_connector->base;
+	connector = &ast->output.dp501.connector;
 	ret = drm_connector_init(dev, connector, &ast_dp501_connector_funcs,
 				 DRM_MODE_CONNECTOR_DisplayPort);
 	if (ret)
@@ -597,10 +589,9 @@ int ast_dp501_output_init(struct ast_device *ast)
 
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
+	connector->bmc_attached = true;
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
-	ast_connector->physical_status = connector->status;
-
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 21ce3769bf0d..a6887e90dc17 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -141,22 +141,6 @@ static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
 	return container_of(plane, struct ast_plane, base);
 }
 
-/*
- * Connector
- */
-
-struct ast_connector {
-	struct drm_connector base;
-
-	enum drm_connector_status physical_status;
-};
-
-static inline struct ast_connector *
-to_ast_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct ast_connector, base);
-}
-
 /*
  * Device
  */
@@ -190,19 +174,19 @@ struct ast_device {
 	union {
 		struct {
 			struct drm_encoder encoder;
-			struct ast_connector connector;
+			struct drm_connector connector;
 		} vga;
 		struct {
 			struct drm_encoder encoder;
-			struct ast_connector connector;
+			struct drm_connector connector;
 		} sil164;
 		struct {
 			struct drm_encoder encoder;
-			struct ast_connector connector;
+			struct drm_connector connector;
 		} dp501;
 		struct {
 			struct drm_encoder encoder;
-			struct ast_connector connector;
+			struct drm_connector connector;
 		} astdp;
 	} output;
 
diff --git a/drivers/gpu/drm/ast/ast_sil164.c b/drivers/gpu/drm/ast/ast_sil164.c
index be01254dd48a..aba5b8aa4307 100644
--- a/drivers/gpu/drm/ast/ast_sil164.c
+++ b/drivers/gpu/drm/ast/ast_sil164.c
@@ -23,10 +23,9 @@ static const struct drm_encoder_funcs ast_sil164_encoder_funcs = {
 
 static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
 	int count;
 
-	if (ast_connector->physical_status == connector_status_connected) {
+	if (connector->physical_status == connector_status_connected) {
 		count = drm_connector_helper_get_modes(connector);
 	} else {
 		drm_edid_connector_update(connector, NULL);
@@ -48,16 +47,11 @@ static int ast_sil164_connector_helper_detect_ctx(struct drm_connector *connecto
 						  struct drm_modeset_acquire_ctx *ctx,
 						  bool force)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
 	enum drm_connector_status status;
 
 	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
 
-	if (status != ast_connector->physical_status)
-		++connector->epoch_counter;
-	ast_connector->physical_status = status;
-
-	return connector_status_connected;
+	return status;
 }
 
 static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
@@ -83,7 +77,6 @@ int ast_sil164_output_init(struct ast_device *ast)
 	struct drm_crtc *crtc = &ast->crtc;
 	struct i2c_adapter *ddc;
 	struct drm_encoder *encoder;
-	struct ast_connector *ast_connector;
 	struct drm_connector *connector;
 	int ret;
 
@@ -104,8 +97,7 @@ int ast_sil164_output_init(struct ast_device *ast)
 
 	/* connector */
 
-	ast_connector = &ast->output.sil164.connector;
-	connector = &ast_connector->base;
+	connector = &ast->output.sil164.connector;
 	ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
 					  DRM_MODE_CONNECTOR_DVII, ddc);
 	if (ret)
@@ -114,10 +106,9 @@ int ast_sil164_output_init(struct ast_device *ast)
 
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
+	connector->bmc_attached = true;
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
-	ast_connector->physical_status = connector->status;
-
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/ast/ast_vga.c b/drivers/gpu/drm/ast/ast_vga.c
index abe0fff8485c..d78f00c47cc5 100644
--- a/drivers/gpu/drm/ast/ast_vga.c
+++ b/drivers/gpu/drm/ast/ast_vga.c
@@ -23,10 +23,9 @@ static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
 
 static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
 	int count;
 
-	if (ast_connector->physical_status == connector_status_connected) {
+	if (connector->physical_status == connector_status_connected) {
 		count = drm_connector_helper_get_modes(connector);
 	} else {
 		drm_edid_connector_update(connector, NULL);
@@ -44,25 +43,9 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 	return count;
 }
 
-static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
-					       struct drm_modeset_acquire_ctx *ctx,
-					       bool force)
-{
-	struct ast_connector *ast_connector = to_ast_connector(connector);
-	enum drm_connector_status status;
-
-	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
-
-	if (status != ast_connector->physical_status)
-		++connector->epoch_counter;
-	ast_connector->physical_status = status;
-
-	return connector_status_connected;
-}
-
 static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
 	.get_modes = ast_vga_connector_helper_get_modes,
-	.detect_ctx = ast_vga_connector_helper_detect_ctx,
+	.detect_ctx = drm_connector_helper_detect_from_ddc,
 };
 
 static const struct drm_connector_funcs ast_vga_connector_funcs = {
@@ -83,7 +66,6 @@ int ast_vga_output_init(struct ast_device *ast)
 	struct drm_crtc *crtc = &ast->crtc;
 	struct i2c_adapter *ddc;
 	struct drm_encoder *encoder;
-	struct ast_connector *ast_connector;
 	struct drm_connector *connector;
 	int ret;
 
@@ -104,8 +86,7 @@ int ast_vga_output_init(struct ast_device *ast)
 
 	/* connector */
 
-	ast_connector = &ast->output.vga.connector;
-	connector = &ast_connector->base;
+	connector = &ast->output.vga.connector;
 	ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
 					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret)
@@ -114,10 +95,9 @@ int ast_vga_output_init(struct ast_device *ast)
 
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
+	connector->bmc_attached = true;
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
-	ast_connector->physical_status = connector->status;
-
 	ret = drm_connector_attach_encoder(connector, encoder);
 	if (ret)
 		return ret;
-- 
2.46.0

[PATCH 7/7] drm/mgag200: Track physical connector status in struct drm_connector

[Thomas Zimmermann]

Set bmc_attached for the VGA connector on servers and let DRM's
probe helpers track the physical and logical connector state. Remove
similar logic from mgag200.

Also resolve a design issue, where mgag200 uses the connector's
edid_blob_ptr. It is an internal value that drivers should not access
directly.

Reported-by: Jani Nikula <jani.nikula@linux.intel.com>
Closes: https://lore.kernel.org/dri-devel/87msjtxk8f.fsf@intel.com/raw
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 32 +++++------------------
 1 file changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
index a5a3ac108bd5..cff333572b29 100644
--- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
@@ -54,9 +54,11 @@ static int mgag200_vga_bmc_connector_helper_get_modes(struct drm_connector *conn
 	const struct mgag200_device_info *minfo = mdev->info;
 	int count;
 
-	count = drm_connector_helper_get_modes(connector);
+	if (connector->physical_status == connector_status_connected) {
+		count = drm_connector_helper_get_modes(connector);
+	} else {
+		drm_edid_connector_update(connector, NULL);
 
-	if (!count) {
 		/*
 		 * There's no EDID data without a connected monitor. Set BMC-
 		 * compatible modes in this case. The XGA default resolution
@@ -70,32 +72,9 @@ static int mgag200_vga_bmc_connector_helper_get_modes(struct drm_connector *conn
 	return count;
 }
 
-/*
- * There's no monitor connected if the DDC did not return an EDID. Still
- * return 'connected' as there's always a BMC. Incrementing the connector's
- * epoch counter triggers an update of the related properties.
- */
-static int mgag200_vga_bmc_connector_helper_detect_ctx(struct drm_connector *connector,
-						       struct drm_modeset_acquire_ctx *ctx,
-						       bool force)
-{
-	enum drm_connector_status old_status, status;
-
-	if (connector->edid_blob_ptr)
-		old_status = connector_status_connected;
-	else
-		old_status = connector_status_disconnected;
-
-	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
-
-	if (status != old_status)
-		++connector->epoch_counter;
-	return connector_status_connected;
-}
-
 static const struct drm_connector_helper_funcs mgag200_vga_connector_helper_funcs = {
 	.get_modes = mgag200_vga_bmc_connector_helper_get_modes,
-	.detect_ctx = mgag200_vga_bmc_connector_helper_detect_ctx,
+	.detect_ctx = drm_connector_helper_detect_from_ddc,
 };
 
 static const struct drm_connector_funcs mgag200_vga_connector_funcs = {
@@ -143,6 +122,7 @@ int mgag200_vga_bmc_output_init(struct mga_device *mdev)
 	}
 	drm_connector_helper_add(connector, &mgag200_vga_connector_helper_funcs);
 
+	connector->bmc_attached = true;
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT |
 			    DRM_CONNECTOR_POLL_DISCONNECT;
 
-- 
2.46.0

Re: [PATCH 2/7] drm: Add physical status to connector

[Jani Nikula]

On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Track the connector's physical status in addition to its logical
> status. The latter is directly derived from the former and for most
> connectors both values are in sync.
>
> Server chips with BMC, such as Aspeed, Matrox and HiSilicon, often
> provide virtual outputs for remote management. Without a connected
> display, fbcon or userspace compositors disabek the output and stop
> displaying to the BMC.

Please don't assume people know what "BMC" means.

> Connectors have therefore to remain in connected status, even if the
> display has been physically disconnected. Tracking both physical and
> logical state in separate fields will enable that. The physical status
> is transparent to drivers and clients, but changes update the epoch
> counter. This generates a hotplug events for clients. Clients will then
> pick up changes to resolutions supported, if any.
>
> The ast driver already contains code to track the physical status. This
> commit generalizes the logic for use with other drivers. Candidates are
> mgag200 and hibmc.
>
> This commit adds the physical status and makes the regular, logical
> status a copy of it. A later change will add the flag for BMC support.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_connector.c    |  1 +
>  drivers/gpu/drm/drm_probe_helper.c | 13 ++++++++-----
>  include/drm/drm_connector.h        |  7 +++++++
>  3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index fc35f47e2849..901d73416f98 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -282,6 +282,7 @@ static int __drm_connector_init(struct drm_device *dev,
>  	connector->edid_blob_ptr = NULL;
>  	connector->epoch_counter = 0;
>  	connector->tile_blob_ptr = NULL;
> +	connector->physical_status = connector_status_unknown;
>  	connector->status = connector_status_unknown;
>  	connector->display_info.panel_orientation =
>  		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 62a2e5bcb315..df44be128e72 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -373,7 +373,7 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
>  	if (WARN_ON(ret < 0))
>  		ret = connector_status_unknown;
>  
> -	if (ret != connector->status)
> +	if (ret != connector->physical_status)
>  		connector->epoch_counter += 1;
>  
>  	drm_modeset_drop_locks(&ctx);
> @@ -409,7 +409,7 @@ drm_helper_probe_detect(struct drm_connector *connector,
>  
>  	ret = detect_connector_status(connector, ctx, force);
>  
> -	if (ret != connector->status)
> +	if (ret != connector->physical_status)
>  		connector->epoch_counter += 1;
>  
>  	return ret;
> @@ -588,9 +588,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  	if (connector->force) {
>  		if (connector->force == DRM_FORCE_ON ||
>  		    connector->force == DRM_FORCE_ON_DIGITAL)
> -			connector->status = connector_status_connected;
> +			connector->physical_status = connector_status_connected;
>  		else
> -			connector->status = connector_status_disconnected;
> +			connector->physical_status = connector_status_disconnected;
> +		connector->status = connector->physical_status;
> +
>  		if (connector->funcs->force)
>  			connector->funcs->force(connector);
>  	} else {
> @@ -602,7 +604,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		} else if (WARN(ret < 0, "Invalid return value %i for connector detection\n", ret))
>  			ret = connector_status_unknown;
>  
> -		connector->status = ret;
> +		connector->physical_status = ret;
> +		connector->status = connector->physical_status;
>  	}
>  
>  	/*
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index e3fa43291f44..37e951f04ae8 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1817,6 +1817,13 @@ struct drm_connector {
>  	 */
>  	struct list_head modes;
>  
> +	/**
> +	 * @physical_status:
> +	 * One of the drm_connector_status enums (connected, not, or unknown).
> +	 * Protected by &drm_mode_config.mutex.
> +	 */

I don't think that's anywhere near enough documentation. It's just
copy-paste from status. The values aren't important, the difference
between status and physical_status is.

And I think we need to have both status and physical_status
documentation explain what they mean, when they change, who can change
them, etc. And crucially, tell folks not to mess with physical_status
except in the narrow use case.

Side note, this probably indicates a few places where drivers are
messing with connector status in a way they shouldn't:

	git grep "connector->status = " -- drivers/gpu/drm

BR,
Jani.


> +	enum drm_connector_status physical_status;
> +
>  	/**
>  	 * @status:
>  	 * One of the drm_connector_status enums (connected, not, or unknown).

-- 
Jani Nikula, Intel

Re: [PATCH 3/7] drm: Add bmc_attached flag to connector

[Jani Nikula]

On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Add the bmc_attached flag to struct drm_connector to signal the
> presence of a virtual BMC output. The connector reports to be in
> status connected even without a physically connected display. Fbcon
> or userspace compositors would otherwise stop displaying to the
> BMC.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 6 +++++-
>  include/drm/drm_connector.h        | 8 ++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index df44be128e72..83c3f2d42d49 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -605,7 +605,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  			ret = connector_status_unknown;
>  
>  		connector->physical_status = ret;
> -		connector->status = connector->physical_status;
> +
> +		if (connector->bmc_attached)
> +			connector->status = connector_status_connected;
> +		else
> +			connector->status = connector->physical_status;

Perhaps all this would make more sense squashed into the previous patch?

>  	}
>  
>  	/*
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 37e951f04ae8..ed360ae35f21 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1802,6 +1802,14 @@ struct drm_connector {
>  	 */
>  	bool ycbcr_420_allowed;
>  
> +	/**
> +	 * @ bmc_attached:
> +	 * The connector has a BMC transparently attached to it. It has to
> +	 * report a connected status, even without a physically connected
> +	 * display.
> +	 */
> +	bool bmc_attached;

Who is supposed to set this and when? Can it be changed during connector
life cycle? (I find myself wishing for C++ public/private members,
*gasp*!)

Again, there's bound to be people for whom "BMC" means nothing, and
therefore this member means nothing. I'm wondering if the name could be
something with a higher level of abstraction. In some ways, this is
related to or a special case of connector->force, but without the
uapi. You know, "logical force", with the use case being BMC.

BR,
Jani.


> +
>  	/**
>  	 * @registration_state: Is this connector initializing, exposed
>  	 * (registered) with userspace, or unregistered?

-- 
Jani Nikula, Intel

Re: [PATCH 2/7] drm: Add physical status to connector

[Thomas Zimmermann]

Hi

Am 11.10.24 um 10:51 schrieb Jani Nikula:
> On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Track the connector's physical status in addition to its logical
>> status. The latter is directly derived from the former and for most
>> connectors both values are in sync.
>>
>> Server chips with BMC, such as Aspeed, Matrox and HiSilicon, often
>> provide virtual outputs for remote management. Without a connected
>> display, fbcon or userspace compositors disabek the output and stop
>> displaying to the BMC.
> Please don't assume people know what "BMC" means.

Apologies. I'll include that information in any follow-up and the kernel 
docs.

FTR it's the baseboard management controller.

  https://en.wikipedia.org/wiki/Intelligent_Platform_Management_Interface#Baseboard_management_controller

Best regards
Thomas

>
>> Connectors have therefore to remain in connected status, even if the
>> display has been physically disconnected. Tracking both physical and
>> logical state in separate fields will enable that. The physical status
>> is transparent to drivers and clients, but changes update the epoch
>> counter. This generates a hotplug events for clients. Clients will then
>> pick up changes to resolutions supported, if any.
>>
>> The ast driver already contains code to track the physical status. This
>> commit generalizes the logic for use with other drivers. Candidates are
>> mgag200 and hibmc.
>>
>> This commit adds the physical status and makes the regular, logical
>> status a copy of it. A later change will add the flag for BMC support.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_connector.c    |  1 +
>>   drivers/gpu/drm/drm_probe_helper.c | 13 ++++++++-----
>>   include/drm/drm_connector.h        |  7 +++++++
>>   3 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>> index fc35f47e2849..901d73416f98 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -282,6 +282,7 @@ static int __drm_connector_init(struct drm_device *dev,
>>   	connector->edid_blob_ptr = NULL;
>>   	connector->epoch_counter = 0;
>>   	connector->tile_blob_ptr = NULL;
>> +	connector->physical_status = connector_status_unknown;
>>   	connector->status = connector_status_unknown;
>>   	connector->display_info.panel_orientation =
>>   		DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 62a2e5bcb315..df44be128e72 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -373,7 +373,7 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
>>   	if (WARN_ON(ret < 0))
>>   		ret = connector_status_unknown;
>>   
>> -	if (ret != connector->status)
>> +	if (ret != connector->physical_status)
>>   		connector->epoch_counter += 1;
>>   
>>   	drm_modeset_drop_locks(&ctx);
>> @@ -409,7 +409,7 @@ drm_helper_probe_detect(struct drm_connector *connector,
>>   
>>   	ret = detect_connector_status(connector, ctx, force);
>>   
>> -	if (ret != connector->status)
>> +	if (ret != connector->physical_status)
>>   		connector->epoch_counter += 1;
>>   
>>   	return ret;
>> @@ -588,9 +588,11 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>   	if (connector->force) {
>>   		if (connector->force == DRM_FORCE_ON ||
>>   		    connector->force == DRM_FORCE_ON_DIGITAL)
>> -			connector->status = connector_status_connected;
>> +			connector->physical_status = connector_status_connected;
>>   		else
>> -			connector->status = connector_status_disconnected;
>> +			connector->physical_status = connector_status_disconnected;
>> +		connector->status = connector->physical_status;
>> +
>>   		if (connector->funcs->force)
>>   			connector->funcs->force(connector);
>>   	} else {
>> @@ -602,7 +604,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>   		} else if (WARN(ret < 0, "Invalid return value %i for connector detection\n", ret))
>>   			ret = connector_status_unknown;
>>   
>> -		connector->status = ret;
>> +		connector->physical_status = ret;
>> +		connector->status = connector->physical_status;
>>   	}
>>   
>>   	/*
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index e3fa43291f44..37e951f04ae8 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -1817,6 +1817,13 @@ struct drm_connector {
>>   	 */
>>   	struct list_head modes;
>>   
>> +	/**
>> +	 * @physical_status:
>> +	 * One of the drm_connector_status enums (connected, not, or unknown).
>> +	 * Protected by &drm_mode_config.mutex.
>> +	 */
> I don't think that's anywhere near enough documentation. It's just
> copy-paste from status. The values aren't important, the difference
> between status and physical_status is.
>
> And I think we need to have both status and physical_status
> documentation explain what they mean, when they change, who can change
> them, etc. And crucially, tell folks not to mess with physical_status
> except in the narrow use case.
>
> Side note, this probably indicates a few places where drivers are
> messing with connector status in a way they shouldn't:
>
> 	git grep "connector->status = " -- drivers/gpu/drm
>
> BR,
> Jani.
>
>
>> +	enum drm_connector_status physical_status;
>> +
>>   	/**
>>   	 * @status:
>>   	 * One of the drm_connector_status enums (connected, not, or unknown).

-- 
--
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 6/7] drm/ast: Track physical connector status in struct drm_connector

[Jani Nikula]

On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Set bmc_attached for all connectors and let DRM's probe helpers
> track the physical and logical connector state. Remove such logic
> and related data structures from ast.

Yeah, nice cleanups.

Still, I think this emphasizes my point about improved documentation in
earlier patches. I think people are going to cargo cult this, and if
they don't have anything in the documentation to go by, they'll think
"physical status" is the thing to use when it's about physically
connecting displays.

BR,
Jani.

>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  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 | 17 ++++-------------
>  drivers/gpu/drm/ast/ast_vga.c    | 28 ++++------------------------
>  5 files changed, 22 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 0e282b7b167c..b62c39479367 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -328,9 +328,9 @@ static void ast_astdp_encoder_helper_atomic_enable(struct drm_encoder *encoder,
>  						   struct drm_atomic_state *state)
>  {
>  	struct ast_device *ast = to_ast_device(encoder->dev);
> -	struct ast_connector *ast_connector = &ast->output.astdp.connector;
> +	struct drm_connector *connector = &ast->output.astdp.connector;
>  
> -	if (ast_connector->physical_status == connector_status_connected) {
> +	if (connector->physical_status == connector_status_connected) {
>  		ast_dp_set_phy_sleep(ast, false);
>  		ast_dp_link_training(ast);
>  
> @@ -360,10 +360,9 @@ static const struct drm_encoder_helper_funcs ast_astdp_encoder_helper_funcs = {
>  
>  static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	int count;
>  
> -	if (ast_connector->physical_status == connector_status_connected) {
> +	if (connector->physical_status == connector_status_connected) {
>  		struct ast_device *ast = to_ast_device(connector->dev);
>  		const struct drm_edid *drm_edid;
>  
> @@ -391,7 +390,6 @@ static int ast_astdp_connector_helper_detect_ctx(struct drm_connector *connector
>  						 struct drm_modeset_acquire_ctx *ctx,
>  						 bool force)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	struct ast_device *ast = to_ast_device(connector->dev);
>  	enum drm_connector_status status = connector_status_disconnected;
>  	bool phy_sleep;
> @@ -410,11 +408,7 @@ static int ast_astdp_connector_helper_detect_ctx(struct drm_connector *connector
>  
>  	mutex_unlock(&ast->modeset_lock);
>  
> -	if (status != ast_connector->physical_status)
> -		++connector->epoch_counter;
> -	ast_connector->physical_status = status;
> -
> -	return connector_status_connected;
> +	return status;
>  }
>  
>  static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
> @@ -439,7 +433,6 @@ int ast_astdp_output_init(struct ast_device *ast)
>  	struct drm_device *dev = &ast->base;
>  	struct drm_crtc *crtc = &ast->crtc;
>  	struct drm_encoder *encoder;
> -	struct ast_connector *ast_connector;
>  	struct drm_connector *connector;
>  	int ret;
>  
> @@ -456,8 +449,7 @@ int ast_astdp_output_init(struct ast_device *ast)
>  
>  	/* connector */
>  
> -	ast_connector = &ast->output.astdp.connector;
> -	connector = &ast_connector->base;
> +	connector = &ast->output.astdp.connector;
>  	ret = drm_connector_init(dev, connector, &ast_astdp_connector_funcs,
>  				 DRM_MODE_CONNECTOR_DisplayPort);
>  	if (ret)
> @@ -466,10 +458,9 @@ int ast_astdp_output_init(struct ast_device *ast)
>  
>  	connector->interlace_allowed = 0;
>  	connector->doublescan_allowed = 0;
> +	connector->bmc_attached = true;
>  	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  
> -	ast_connector->physical_status = connector->status;
> -
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> index 9e19d8c17730..8ffe30c74d3d 100644
> --- a/drivers/gpu/drm/ast/ast_dp501.c
> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> @@ -503,10 +503,9 @@ static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = {
>  
>  static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	int count;
>  
> -	if (ast_connector->physical_status == connector_status_connected) {
> +	if (connector->physical_status == connector_status_connected) {
>  		struct ast_device *ast = to_ast_device(connector->dev);
>  		const struct drm_edid *drm_edid;
>  
> @@ -534,18 +533,13 @@ static int ast_dp501_connector_helper_detect_ctx(struct drm_connector *connector
>  						 struct drm_modeset_acquire_ctx *ctx,
>  						 bool force)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	struct ast_device *ast = to_ast_device(connector->dev);
>  	enum drm_connector_status status = connector_status_disconnected;
>  
>  	if (ast_dp501_is_connected(ast))
>  		status = connector_status_connected;
>  
> -	if (status != ast_connector->physical_status)
> -		++connector->epoch_counter;
> -	ast_connector->physical_status = status;
> -
> -	return connector_status_connected;
> +	return status;
>  }
>  
>  static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
> @@ -570,7 +564,6 @@ int ast_dp501_output_init(struct ast_device *ast)
>  	struct drm_device *dev = &ast->base;
>  	struct drm_crtc *crtc = &ast->crtc;
>  	struct drm_encoder *encoder;
> -	struct ast_connector *ast_connector;
>  	struct drm_connector *connector;
>  	int ret;
>  
> @@ -587,8 +580,7 @@ int ast_dp501_output_init(struct ast_device *ast)
>  
>  	/* connector */
>  
> -	ast_connector = &ast->output.dp501.connector;
> -	connector = &ast_connector->base;
> +	connector = &ast->output.dp501.connector;
>  	ret = drm_connector_init(dev, connector, &ast_dp501_connector_funcs,
>  				 DRM_MODE_CONNECTOR_DisplayPort);
>  	if (ret)
> @@ -597,10 +589,9 @@ int ast_dp501_output_init(struct ast_device *ast)
>  
>  	connector->interlace_allowed = 0;
>  	connector->doublescan_allowed = 0;
> +	connector->bmc_attached = true;
>  	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  
> -	ast_connector->physical_status = connector->status;
> -
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 21ce3769bf0d..a6887e90dc17 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -141,22 +141,6 @@ static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
>  	return container_of(plane, struct ast_plane, base);
>  }
>  
> -/*
> - * Connector
> - */
> -
> -struct ast_connector {
> -	struct drm_connector base;
> -
> -	enum drm_connector_status physical_status;
> -};
> -
> -static inline struct ast_connector *
> -to_ast_connector(struct drm_connector *connector)
> -{
> -	return container_of(connector, struct ast_connector, base);
> -}
> -
>  /*
>   * Device
>   */
> @@ -190,19 +174,19 @@ struct ast_device {
>  	union {
>  		struct {
>  			struct drm_encoder encoder;
> -			struct ast_connector connector;
> +			struct drm_connector connector;
>  		} vga;
>  		struct {
>  			struct drm_encoder encoder;
> -			struct ast_connector connector;
> +			struct drm_connector connector;
>  		} sil164;
>  		struct {
>  			struct drm_encoder encoder;
> -			struct ast_connector connector;
> +			struct drm_connector connector;
>  		} dp501;
>  		struct {
>  			struct drm_encoder encoder;
> -			struct ast_connector connector;
> +			struct drm_connector connector;
>  		} astdp;
>  	} output;
>  
> diff --git a/drivers/gpu/drm/ast/ast_sil164.c b/drivers/gpu/drm/ast/ast_sil164.c
> index be01254dd48a..aba5b8aa4307 100644
> --- a/drivers/gpu/drm/ast/ast_sil164.c
> +++ b/drivers/gpu/drm/ast/ast_sil164.c
> @@ -23,10 +23,9 @@ static const struct drm_encoder_funcs ast_sil164_encoder_funcs = {
>  
>  static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	int count;
>  
> -	if (ast_connector->physical_status == connector_status_connected) {
> +	if (connector->physical_status == connector_status_connected) {
>  		count = drm_connector_helper_get_modes(connector);
>  	} else {
>  		drm_edid_connector_update(connector, NULL);
> @@ -48,16 +47,11 @@ static int ast_sil164_connector_helper_detect_ctx(struct drm_connector *connecto
>  						  struct drm_modeset_acquire_ctx *ctx,
>  						  bool force)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	enum drm_connector_status status;
>  
>  	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
>  
> -	if (status != ast_connector->physical_status)
> -		++connector->epoch_counter;
> -	ast_connector->physical_status = status;
> -
> -	return connector_status_connected;
> +	return status;
>  }
>  
>  static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
> @@ -83,7 +77,6 @@ int ast_sil164_output_init(struct ast_device *ast)
>  	struct drm_crtc *crtc = &ast->crtc;
>  	struct i2c_adapter *ddc;
>  	struct drm_encoder *encoder;
> -	struct ast_connector *ast_connector;
>  	struct drm_connector *connector;
>  	int ret;
>  
> @@ -104,8 +97,7 @@ int ast_sil164_output_init(struct ast_device *ast)
>  
>  	/* connector */
>  
> -	ast_connector = &ast->output.sil164.connector;
> -	connector = &ast_connector->base;
> +	connector = &ast->output.sil164.connector;
>  	ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
>  					  DRM_MODE_CONNECTOR_DVII, ddc);
>  	if (ret)
> @@ -114,10 +106,9 @@ int ast_sil164_output_init(struct ast_device *ast)
>  
>  	connector->interlace_allowed = 0;
>  	connector->doublescan_allowed = 0;
> +	connector->bmc_attached = true;
>  	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  
> -	ast_connector->physical_status = connector->status;
> -
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/ast/ast_vga.c b/drivers/gpu/drm/ast/ast_vga.c
> index abe0fff8485c..d78f00c47cc5 100644
> --- a/drivers/gpu/drm/ast/ast_vga.c
> +++ b/drivers/gpu/drm/ast/ast_vga.c
> @@ -23,10 +23,9 @@ static const struct drm_encoder_funcs ast_vga_encoder_funcs = {
>  
>  static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
>  {
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
>  	int count;
>  
> -	if (ast_connector->physical_status == connector_status_connected) {
> +	if (connector->physical_status == connector_status_connected) {
>  		count = drm_connector_helper_get_modes(connector);
>  	} else {
>  		drm_edid_connector_update(connector, NULL);
> @@ -44,25 +43,9 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
>  	return count;
>  }
>  
> -static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
> -					       struct drm_modeset_acquire_ctx *ctx,
> -					       bool force)
> -{
> -	struct ast_connector *ast_connector = to_ast_connector(connector);
> -	enum drm_connector_status status;
> -
> -	status = drm_connector_helper_detect_from_ddc(connector, ctx, force);
> -
> -	if (status != ast_connector->physical_status)
> -		++connector->epoch_counter;
> -	ast_connector->physical_status = status;
> -
> -	return connector_status_connected;
> -}
> -
>  static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
>  	.get_modes = ast_vga_connector_helper_get_modes,
> -	.detect_ctx = ast_vga_connector_helper_detect_ctx,
> +	.detect_ctx = drm_connector_helper_detect_from_ddc,
>  };
>  
>  static const struct drm_connector_funcs ast_vga_connector_funcs = {
> @@ -83,7 +66,6 @@ int ast_vga_output_init(struct ast_device *ast)
>  	struct drm_crtc *crtc = &ast->crtc;
>  	struct i2c_adapter *ddc;
>  	struct drm_encoder *encoder;
> -	struct ast_connector *ast_connector;
>  	struct drm_connector *connector;
>  	int ret;
>  
> @@ -104,8 +86,7 @@ int ast_vga_output_init(struct ast_device *ast)
>  
>  	/* connector */
>  
> -	ast_connector = &ast->output.vga.connector;
> -	connector = &ast_connector->base;
> +	connector = &ast->output.vga.connector;
>  	ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
>  					  DRM_MODE_CONNECTOR_VGA, ddc);
>  	if (ret)
> @@ -114,10 +95,9 @@ int ast_vga_output_init(struct ast_device *ast)
>  
>  	connector->interlace_allowed = 0;
>  	connector->doublescan_allowed = 0;
> +	connector->bmc_attached = true;
>  	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>  
> -	ast_connector->physical_status = connector->status;
> -
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret)
>  		return ret;

-- 
Jani Nikula, Intel

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

[Jani Nikula]

On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 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.

Overall,

Acked-by: Jani Nikula <jani.nikula@intel.com>

but please do improve the documentation, and consider alternatives to
the bmc_attached naming.

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

-- 
Jani Nikula, Intel

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

[Maxime Ripard]

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

Hi,

On Fri, Oct 11, 2024 at 08:43:05AM GMT, Thomas Zimmermann wrote:
> 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.

Generally speaking, it looks ok, but given how much of a corner case it
is, we should have kunit tests to cover the whole thing.

Maxime

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