[PATCH v4 1/2] drm/ast: Add BMC virtual connector

[PATCH v4 1/2] drm/ast: Add BMC virtual connector

[Jocelyn Falempe]

Most aspeed devices have a BMC, which allows to remotely see the screen.
Also in the common use case, those servers don't have a display connected.
So add a Virtual connector, to reflect that even if no display is
connected, the framebuffer can still be seen remotely.
This prepares the work to implement a detect_ctx() for the Display port
connector.

v4: call drm_add_modes_noedid() with 4096x4096 (Thomas Zimmermann)
    remove useless struct field init to 0 (Thomas Zimmermann)
    don't use drm_simple_encoder_init() (Thomas Zimmermann)
    inline ast_bmc_connector_init() (Thomas Zimmermann)

Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
---
 drivers/gpu/drm/ast/ast_drv.h  |  4 +++
 drivers/gpu/drm/ast/ast_mode.c | 58 ++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 3f6e0c984523..c9659e72002f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -214,6 +214,10 @@ struct ast_device {
 			struct drm_encoder encoder;
 			struct drm_connector connector;
 		} astdp;
+		struct {
+			struct drm_encoder encoder;
+			struct drm_connector connector;
+		} bmc;
 	} output;
 
 	bool support_wide_screen;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index f711d592da52..1a8293162fec 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1735,6 +1735,61 @@ static int ast_astdp_output_init(struct ast_device *ast)
 	return 0;
 }
 
+/*
+ * BMC virtual Connector
+ */
+
+static int ast_bmc_connector_helper_get_modes(struct drm_connector *connector)
+{
+	return drm_add_modes_noedid(connector, 4096, 4096);
+}
+
+static const struct drm_connector_helper_funcs ast_bmc_connector_helper_funcs = {
+	.get_modes = ast_bmc_connector_helper_get_modes,
+};
+
+static const struct drm_connector_funcs ast_bmc_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static const struct drm_encoder_funcs ast_bmc_encoder_funcs = {
+	.destroy = drm_encoder_cleanup,
+};
+
+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 drm_connector *connector = &ast->output.bmc.connector;
+	int ret;
+
+
+	ret = drm_encoder_init(dev, encoder,
+				&ast_bmc_encoder_funcs,
+				DRM_MODE_ENCODER_VIRTUAL, "ast_bmc");
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	ret = drm_connector_init(dev, connector, &ast_bmc_connector_funcs,
+				 DRM_MODE_CONNECTOR_VIRTUAL);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(connector, &ast_bmc_connector_helper_funcs);
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /*
  * Mode config
  */
@@ -1842,6 +1897,9 @@ int ast_mode_config_init(struct ast_device *ast)
 		if (ret)
 			return ret;
 	}
+	ret = ast_bmc_output_init(ast);
+	if (ret)
+		return ret;
 
 	drm_mode_config_reset(dev);
 

base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2
-- 
2.41.0

[PATCH v4 2/2] drm/ast: report connection status on Display Port.

[Jocelyn Falempe]

Aspeed always report the display port as "connected", because it
doesn't set a .detect callback.
Fix this by providing the proper detect callback for astdp and dp501.

This also fixes the following regression:
Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no
EDID on DP")
The default resolution is now 640x480 when no monitor is connected. But
Aspeed graphics is mostly used in servers, where no monitor is attached.
This also affects the remote BMC resolution to 640x480, which is
inconvenient, and breaks the anaconda installer.

v2: Add .detect callback to the dp/dp501 connector (Jani Nikula)
v3: Use .detect_ctx callback, and refactors (Thomas Zimmermann)
    Add a BMC virtual connector
v4: Better indent detect_ctx() functions (Thomas Zimmermann)

Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_dp.c    | 11 ++++++++++
 drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++-----------
 drivers/gpu/drm/ast/ast_drv.h   |  2 ++
 drivers/gpu/drm/ast/ast_mode.c  | 25 ++++++++++++++++++++++
 4 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
index 6dc1a09504e1..bf78f3d4aa3f 100644
--- a/drivers/gpu/drm/ast/ast_dp.c
+++ b/drivers/gpu/drm/ast/ast_dp.c
@@ -7,6 +7,17 @@
 #include <drm/drm_print.h>
 #include "ast_drv.h"
 
+bool ast_astdp_is_connected(struct ast_device *ast)
+{
+	if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, ASTDP_MCU_FW_EXECUTING))
+		return false;
+	if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, ASTDP_LINK_SUCCESS))
+		return false;
+	if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD))
+		return false;
+	return true;
+}
+
 int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
 {
 	struct ast_device *ast = to_ast_device(dev);
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index a5d285850ffb..f10d53b0c94f 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -272,11 +272,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	return true;
 }
 
-bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
+bool ast_dp501_is_connected(struct ast_device *ast)
 {
-	struct ast_device *ast = to_ast_device(dev);
-	u32 i, boot_address, offset, data;
-	u32 *pEDIDidx;
+	u32 boot_address, offset, data;
 
 	if (ast->config_mode == ast_use_p2a) {
 		boot_address = get_fw_base(ast);
@@ -292,14 +290,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
 		data = ast_mindwm(ast, boot_address + offset);
 		if (!(data & AST_DP501_PNP_CONNECTED))
 			return false;
-
-		/* Read EDID */
-		offset = AST_DP501_EDID_DATA;
-		for (i = 0; i < 128; i += 4) {
-			data = ast_mindwm(ast, boot_address + offset + i);
-			pEDIDidx = (u32 *)(ediddata + i);
-			*pEDIDidx = data;
-		}
 	} else {
 		if (!ast->dp501_fw_buf)
 			return false;
@@ -319,7 +309,30 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
 		data = readl(ast->dp501_fw_buf + offset);
 		if (!(data & AST_DP501_PNP_CONNECTED))
 			return false;
+	}
+	return true;
+}
+
+bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
+{
+	struct ast_device *ast = to_ast_device(dev);
+	u32 i, boot_address, offset, data;
+	u32 *pEDIDidx;
+
+	if (!ast_dp501_is_connected(ast))
+		return false;
+
+	if (ast->config_mode == ast_use_p2a) {
+		boot_address = get_fw_base(ast);
 
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = ast_mindwm(ast, boot_address + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
+	} else {
 		/* Read EDID */
 		offset = AST_DP501_EDID_DATA;
 		for (i = 0; i < 128; i += 4) {
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index c9659e72002f..848a9f1403e8 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -514,6 +514,7 @@ void ast_patch_ahb_2500(struct ast_device *ast);
 /* ast dp501 */
 void ast_set_dp501_video_output(struct drm_device *dev, u8 mode);
 bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
+bool ast_dp501_is_connected(struct ast_device *ast);
 bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
 u8 ast_get_dp501_max_clk(struct drm_device *dev);
 void ast_init_3rdtx(struct drm_device *dev);
@@ -522,6 +523,7 @@ void ast_init_3rdtx(struct drm_device *dev);
 struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
 
 /* aspeed DP */
+bool ast_astdp_is_connected(struct ast_device *ast);
 int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
 void ast_dp_launch(struct drm_device *dev);
 void ast_dp_power_on_off(struct drm_device *dev, bool no);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 1a8293162fec..75a84817a57d 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1582,8 +1582,21 @@ static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
+static int ast_dp501_connector_helper_detect_ctx(struct drm_connector *connector,
+						 struct drm_modeset_acquire_ctx *ctx,
+						 bool force)
+{
+	struct ast_device *ast = to_ast_device(connector->dev);
+
+	if (ast_dp501_is_connected(ast))
+		return connector_status_connected;
+	return connector_status_disconnected;
+}
+
+
 static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
 	.get_modes = ast_dp501_connector_helper_get_modes,
+	.detect_ctx = ast_dp501_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs ast_dp501_connector_funcs = {
@@ -1680,8 +1693,20 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
+static int ast_astdp_connector_helper_detect_ctx(struct drm_connector *connector,
+						 struct drm_modeset_acquire_ctx *ctx,
+						 bool force)
+{
+	struct ast_device *ast = to_ast_device(connector->dev);
+
+	if (ast_astdp_is_connected(ast))
+		return connector_status_connected;
+	return connector_status_disconnected;
+}
+
 static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
 	.get_modes = ast_astdp_connector_helper_get_modes,
+	.detect_ctx = ast_astdp_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs ast_astdp_connector_funcs = {
-- 
2.41.0

Re: [PATCH v4 2/2] drm/ast: report connection status on Display Port.

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7831 bytes --]

Hi

Am 12.07.23 um 15:27 schrieb Jocelyn Falempe:
> Aspeed always report the display port as "connected", because it
> doesn't set a .detect callback.
> Fix this by providing the proper detect callback for astdp and dp501.
> 
> This also fixes the following regression:
> Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no
> EDID on DP")
> The default resolution is now 640x480 when no monitor is connected. But
> Aspeed graphics is mostly used in servers, where no monitor is attached.
> This also affects the remote BMC resolution to 640x480, which is
> inconvenient, and breaks the anaconda installer.

The formatting of these paragraphs still looks off.

It just occured to me that you probably want output polling to make 
detection work dynamically. See an old patch of mine on how that works:

https://patchwork.freedesktop.org/patch/488460/

We also get an interrupt when the connector state changes, but IDK how 
that has to be handled. So polling is the next best thing. Sorry for 
missing this in the earlier reviews.

Best regards
Thomas

> 
> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula)
> v3: Use .detect_ctx callback, and refactors (Thomas Zimmermann)
>      Add a BMC virtual connector
> v4: Better indent detect_ctx() functions (Thomas Zimmermann)
> 
> Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/ast_dp.c    | 11 ++++++++++
>   drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++-----------
>   drivers/gpu/drm/ast/ast_drv.h   |  2 ++
>   drivers/gpu/drm/ast/ast_mode.c  | 25 ++++++++++++++++++++++
>   4 files changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 6dc1a09504e1..bf78f3d4aa3f 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -7,6 +7,17 @@
>   #include <drm/drm_print.h>
>   #include "ast_drv.h"
>   
> +bool ast_astdp_is_connected(struct ast_device *ast)
> +{
> +	if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, ASTDP_MCU_FW_EXECUTING))
> +		return false;
> +	if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, ASTDP_LINK_SUCCESS))
> +		return false;
> +	if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD))
> +		return false;
> +	return true;
> +}
> +
>   int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
>   {
>   	struct ast_device *ast = to_ast_device(dev);
> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> index a5d285850ffb..f10d53b0c94f 100644
> --- a/drivers/gpu/drm/ast/ast_dp501.c
> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> @@ -272,11 +272,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
>   	return true;
>   }
>   
> -bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
> +bool ast_dp501_is_connected(struct ast_device *ast)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
> -	u32 i, boot_address, offset, data;
> -	u32 *pEDIDidx;
> +	u32 boot_address, offset, data;
>   
>   	if (ast->config_mode == ast_use_p2a) {
>   		boot_address = get_fw_base(ast);
> @@ -292,14 +290,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
>   		data = ast_mindwm(ast, boot_address + offset);
>   		if (!(data & AST_DP501_PNP_CONNECTED))
>   			return false;
> -
> -		/* Read EDID */
> -		offset = AST_DP501_EDID_DATA;
> -		for (i = 0; i < 128; i += 4) {
> -			data = ast_mindwm(ast, boot_address + offset + i);
> -			pEDIDidx = (u32 *)(ediddata + i);
> -			*pEDIDidx = data;
> -		}
>   	} else {
>   		if (!ast->dp501_fw_buf)
>   			return false;
> @@ -319,7 +309,30 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
>   		data = readl(ast->dp501_fw_buf + offset);
>   		if (!(data & AST_DP501_PNP_CONNECTED))
>   			return false;
> +	}
> +	return true;
> +}
> +
> +bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
> +{
> +	struct ast_device *ast = to_ast_device(dev);
> +	u32 i, boot_address, offset, data;
> +	u32 *pEDIDidx;
> +
> +	if (!ast_dp501_is_connected(ast))
> +		return false;
> +
> +	if (ast->config_mode == ast_use_p2a) {
> +		boot_address = get_fw_base(ast);
>   
> +		/* Read EDID */
> +		offset = AST_DP501_EDID_DATA;
> +		for (i = 0; i < 128; i += 4) {
> +			data = ast_mindwm(ast, boot_address + offset + i);
> +			pEDIDidx = (u32 *)(ediddata + i);
> +			*pEDIDidx = data;
> +		}
> +	} else {
>   		/* Read EDID */
>   		offset = AST_DP501_EDID_DATA;
>   		for (i = 0; i < 128; i += 4) {
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index c9659e72002f..848a9f1403e8 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -514,6 +514,7 @@ void ast_patch_ahb_2500(struct ast_device *ast);
>   /* ast dp501 */
>   void ast_set_dp501_video_output(struct drm_device *dev, u8 mode);
>   bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
> +bool ast_dp501_is_connected(struct ast_device *ast);
>   bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
>   u8 ast_get_dp501_max_clk(struct drm_device *dev);
>   void ast_init_3rdtx(struct drm_device *dev);
> @@ -522,6 +523,7 @@ void ast_init_3rdtx(struct drm_device *dev);
>   struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
>   
>   /* aspeed DP */
> +bool ast_astdp_is_connected(struct ast_device *ast);
>   int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
>   void ast_dp_launch(struct drm_device *dev);
>   void ast_dp_power_on_off(struct drm_device *dev, bool no);
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 1a8293162fec..75a84817a57d 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1582,8 +1582,21 @@ static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
>   	return 0;
>   }
>   
> +static int ast_dp501_connector_helper_detect_ctx(struct drm_connector *connector,
> +						 struct drm_modeset_acquire_ctx *ctx,
> +						 bool force)
> +{
> +	struct ast_device *ast = to_ast_device(connector->dev);
> +
> +	if (ast_dp501_is_connected(ast))
> +		return connector_status_connected;
> +	return connector_status_disconnected;
> +}
> +
> +
>   static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
>   	.get_modes = ast_dp501_connector_helper_get_modes,
> +	.detect_ctx = ast_dp501_connector_helper_detect_ctx,
>   };
>   
>   static const struct drm_connector_funcs ast_dp501_connector_funcs = {
> @@ -1680,8 +1693,20 @@ static int ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>   	return 0;
>   }
>   
> +static int ast_astdp_connector_helper_detect_ctx(struct drm_connector *connector,
> +						 struct drm_modeset_acquire_ctx *ctx,
> +						 bool force)
> +{
> +	struct ast_device *ast = to_ast_device(connector->dev);
> +
> +	if (ast_astdp_is_connected(ast))
> +		return connector_status_connected;
> +	return connector_status_disconnected;
> +}
> +
>   static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs = {
>   	.get_modes = ast_astdp_connector_helper_get_modes,
> +	.detect_ctx = ast_astdp_connector_helper_detect_ctx,
>   };
>   
>   static const struct drm_connector_funcs ast_astdp_connector_funcs = {

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 1/2] drm/ast: Add BMC virtual connector

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4411 bytes --]

Hi

Am 12.07.23 um 15:27 schrieb Jocelyn Falempe:
> Most aspeed devices have a BMC, which allows to remotely see the screen.
> Also in the common use case, those servers don't have a display connected.
> So add a Virtual connector, to reflect that even if no display is
> connected, the framebuffer can still be seen remotely.
> This prepares the work to implement a detect_ctx() for the Display port
> connector.
> 
> v4: call drm_add_modes_noedid() with 4096x4096 (Thomas Zimmermann)
>      remove useless struct field init to 0 (Thomas Zimmermann)
>      don't use drm_simple_encoder_init() (Thomas Zimmermann)
>      inline ast_bmc_connector_init() (Thomas Zimmermann)
> 
> Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  |  4 +++
>   drivers/gpu/drm/ast/ast_mode.c | 58 ++++++++++++++++++++++++++++++++++
>   2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 3f6e0c984523..c9659e72002f 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -214,6 +214,10 @@ struct ast_device {
>   			struct drm_encoder encoder;
>   			struct drm_connector connector;
>   		} astdp;
> +		struct {
> +			struct drm_encoder encoder;
> +			struct drm_connector connector;
> +		} bmc;
>   	} output;
>   
>   	bool support_wide_screen;
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index f711d592da52..1a8293162fec 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1735,6 +1735,61 @@ static int ast_astdp_output_init(struct ast_device *ast)
>   	return 0;
>   }
>   
> +/*
> + * BMC virtual Connector
> + */
> +
> +static int ast_bmc_connector_helper_get_modes(struct drm_connector *connector)
> +{
> +	return drm_add_modes_noedid(connector, 4096, 4096);
> +}
> +
> +static const struct drm_connector_helper_funcs ast_bmc_connector_helper_funcs = {
> +	.get_modes = ast_bmc_connector_helper_get_modes,
> +};
> +
> +static const struct drm_connector_funcs ast_bmc_connector_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_connector_cleanup,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static const struct drm_encoder_funcs ast_bmc_encoder_funcs = {
> +	.destroy = drm_encoder_cleanup,
> +};

Pedantic comment: The encoder goes right before the connector code. The 
order through the file is planes->crtc->encoder->connector, as that's 
the order in the pipeline and the order in which the pipeline is being 
set up. With this little fix:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> +
> +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 drm_connector *connector = &ast->output.bmc.connector;
> +	int ret;
> +
> +
> +	ret = drm_encoder_init(dev, encoder,
> +				&ast_bmc_encoder_funcs,
> +				DRM_MODE_ENCODER_VIRTUAL, "ast_bmc");
> +	if (ret)
> +		return ret;
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +
> +	ret = drm_connector_init(dev, connector, &ast_bmc_connector_funcs,
> +				 DRM_MODE_CONNECTOR_VIRTUAL);
> +	if (ret)
> +		return ret;
> +
> +	drm_connector_helper_add(connector, &ast_bmc_connector_helper_funcs);
> +
> +	ret = drm_connector_attach_encoder(connector, encoder);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   /*
>    * Mode config
>    */
> @@ -1842,6 +1897,9 @@ int ast_mode_config_init(struct ast_device *ast)
>   		if (ret)
>   			return ret;
>   	}
> +	ret = ast_bmc_output_init(ast);
> +	if (ret)
> +		return ret;
>   
>   	drm_mode_config_reset(dev);
>   
> 
> base-commit: b32d5a51f3c21843011d68a58e6ac0b897bce9f2

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 2/2] drm/ast: report connection status on Display Port.

[Jocelyn Falempe]

On 12/07/2023 15:55, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.07.23 um 15:27 schrieb Jocelyn Falempe:
>> Aspeed always report the display port as "connected", because it
>> doesn't set a .detect callback.
>> Fix this by providing the proper detect callback for astdp and dp501.
>>
>> This also fixes the following regression:
>> Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no
>> EDID on DP")
>> The default resolution is now 640x480 when no monitor is connected. But
>> Aspeed graphics is mostly used in servers, where no monitor is attached.
>> This also affects the remote BMC resolution to 640x480, which is
>> inconvenient, and breaks the anaconda installer.
> 
> The formatting of these paragraphs still looks off.

ok, I think it's because I start a new line in the middle of the 
sentence, before "The default resolution ..." ?
I will fix that.
> 
> It just occured to me that you probably want output polling to make 
> detection work dynamically. See an old patch of mine on how that works:
> 
> https://patchwork.freedesktop.org/patch/488460/

So, if I understand well, I just need to add:

connector->polled = DRM_CONNECTOR_POLL_CONNECT | 
DRM_CONNECTOR_POLL_DISCONNECT;

and call drm_kms_helper_poll_init() ?
> 
> We also get an interrupt when the connector state changes, but IDK how 
> that has to be handled. So polling is the next best thing. Sorry for 
> missing this in the earlier reviews.

Yes, there is no irq configured in the ast driver, so let's fallback to 
polling.

-- 

Jocelyn

> 
> Best regards
> Thomas
> 
>>
>> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula)
>> v3: Use .detect_ctx callback, and refactors (Thomas Zimmermann)
>>      Add a BMC virtual connector
>> v4: Better indent detect_ctx() functions (Thomas Zimmermann)
>>
>> Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID 
>> on DP")
>> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_dp.c    | 11 ++++++++++
>>   drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++-----------
>>   drivers/gpu/drm/ast/ast_drv.h   |  2 ++
>>   drivers/gpu/drm/ast/ast_mode.c  | 25 ++++++++++++++++++++++
>>   4 files changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
>> index 6dc1a09504e1..bf78f3d4aa3f 100644
>> --- a/drivers/gpu/drm/ast/ast_dp.c
>> +++ b/drivers/gpu/drm/ast/ast_dp.c
>> @@ -7,6 +7,17 @@
>>   #include <drm/drm_print.h>
>>   #include "ast_drv.h"
>> +bool ast_astdp_is_connected(struct ast_device *ast)
>> +{
>> +    if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xD1, 
>> ASTDP_MCU_FW_EXECUTING))
>> +        return false;
>> +    if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, 
>> ASTDP_LINK_SUCCESS))
>> +        return false;
>> +    if (!ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD))
>> +        return false;
>> +    return true;
>> +}
>> +
>>   int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata)
>>   {
>>       struct ast_device *ast = to_ast_device(dev);
>> diff --git a/drivers/gpu/drm/ast/ast_dp501.c 
>> b/drivers/gpu/drm/ast/ast_dp501.c
>> index a5d285850ffb..f10d53b0c94f 100644
>> --- a/drivers/gpu/drm/ast/ast_dp501.c
>> +++ b/drivers/gpu/drm/ast/ast_dp501.c
>> @@ -272,11 +272,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
>>       return true;
>>   }
>> -bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
>> +bool ast_dp501_is_connected(struct ast_device *ast)
>>   {
>> -    struct ast_device *ast = to_ast_device(dev);
>> -    u32 i, boot_address, offset, data;
>> -    u32 *pEDIDidx;
>> +    u32 boot_address, offset, data;
>>       if (ast->config_mode == ast_use_p2a) {
>>           boot_address = get_fw_base(ast);
>> @@ -292,14 +290,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, 
>> u8 *ediddata)
>>           data = ast_mindwm(ast, boot_address + offset);
>>           if (!(data & AST_DP501_PNP_CONNECTED))
>>               return false;
>> -
>> -        /* Read EDID */
>> -        offset = AST_DP501_EDID_DATA;
>> -        for (i = 0; i < 128; i += 4) {
>> -            data = ast_mindwm(ast, boot_address + offset + i);
>> -            pEDIDidx = (u32 *)(ediddata + i);
>> -            *pEDIDidx = data;
>> -        }
>>       } else {
>>           if (!ast->dp501_fw_buf)
>>               return false;
>> @@ -319,7 +309,30 @@ bool ast_dp501_read_edid(struct drm_device *dev, 
>> u8 *ediddata)
>>           data = readl(ast->dp501_fw_buf + offset);
>>           if (!(data & AST_DP501_PNP_CONNECTED))
>>               return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
>> +{
>> +    struct ast_device *ast = to_ast_device(dev);
>> +    u32 i, boot_address, offset, data;
>> +    u32 *pEDIDidx;
>> +
>> +    if (!ast_dp501_is_connected(ast))
>> +        return false;
>> +
>> +    if (ast->config_mode == ast_use_p2a) {
>> +        boot_address = get_fw_base(ast);
>> +        /* Read EDID */
>> +        offset = AST_DP501_EDID_DATA;
>> +        for (i = 0; i < 128; i += 4) {
>> +            data = ast_mindwm(ast, boot_address + offset + i);
>> +            pEDIDidx = (u32 *)(ediddata + i);
>> +            *pEDIDidx = data;
>> +        }
>> +    } else {
>>           /* Read EDID */
>>           offset = AST_DP501_EDID_DATA;
>>           for (i = 0; i < 128; i += 4) {
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h 
>> b/drivers/gpu/drm/ast/ast_drv.h
>> index c9659e72002f..848a9f1403e8 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -514,6 +514,7 @@ void ast_patch_ahb_2500(struct ast_device *ast);
>>   /* ast dp501 */
>>   void ast_set_dp501_video_output(struct drm_device *dev, u8 mode);
>>   bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
>> +bool ast_dp501_is_connected(struct ast_device *ast);
>>   bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
>>   u8 ast_get_dp501_max_clk(struct drm_device *dev);
>>   void ast_init_3rdtx(struct drm_device *dev);
>> @@ -522,6 +523,7 @@ void ast_init_3rdtx(struct drm_device *dev);
>>   struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
>>   /* aspeed DP */
>> +bool ast_astdp_is_connected(struct ast_device *ast);
>>   int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
>>   void ast_dp_launch(struct drm_device *dev);
>>   void ast_dp_power_on_off(struct drm_device *dev, bool no);
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c 
>> b/drivers/gpu/drm/ast/ast_mode.c
>> index 1a8293162fec..75a84817a57d 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1582,8 +1582,21 @@ static int 
>> ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
>>       return 0;
>>   }
>> +static int ast_dp501_connector_helper_detect_ctx(struct drm_connector 
>> *connector,
>> +                         struct drm_modeset_acquire_ctx *ctx,
>> +                         bool force)
>> +{
>> +    struct ast_device *ast = to_ast_device(connector->dev);
>> +
>> +    if (ast_dp501_is_connected(ast))
>> +        return connector_status_connected;
>> +    return connector_status_disconnected;
>> +}
>> +
>> +
>>   static const struct drm_connector_helper_funcs 
>> ast_dp501_connector_helper_funcs = {
>>       .get_modes = ast_dp501_connector_helper_get_modes,
>> +    .detect_ctx = ast_dp501_connector_helper_detect_ctx,
>>   };
>>   static const struct drm_connector_funcs ast_dp501_connector_funcs = {
>> @@ -1680,8 +1693,20 @@ static int 
>> ast_astdp_connector_helper_get_modes(struct drm_connector *connector)
>>       return 0;
>>   }
>> +static int ast_astdp_connector_helper_detect_ctx(struct drm_connector 
>> *connector,
>> +                         struct drm_modeset_acquire_ctx *ctx,
>> +                         bool force)
>> +{
>> +    struct ast_device *ast = to_ast_device(connector->dev);
>> +
>> +    if (ast_astdp_is_connected(ast))
>> +        return connector_status_connected;
>> +    return connector_status_disconnected;
>> +}
>> +
>>   static const struct drm_connector_helper_funcs 
>> ast_astdp_connector_helper_funcs = {
>>       .get_modes = ast_astdp_connector_helper_get_modes,
>> +    .detect_ctx = ast_astdp_connector_helper_detect_ctx,
>>   };
>>   static const struct drm_connector_funcs ast_astdp_connector_funcs = {
> 

Re: [PATCH v4 2/2] drm/ast: report connection status on Display Port.

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2060 bytes --]

Hi

Am 12.07.23 um 16:39 schrieb Jocelyn Falempe:
> On 12/07/2023 15:55, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.07.23 um 15:27 schrieb Jocelyn Falempe:
>>> Aspeed always report the display port as "connected", because it
>>> doesn't set a .detect callback.
>>> Fix this by providing the proper detect callback for astdp and dp501.
>>>
>>> This also fixes the following regression:
>>> Since commit fae7d186403e ("drm/probe-helper: Default to 640x480 if no
>>> EDID on DP")
>>> The default resolution is now 640x480 when no monitor is connected. But
>>> Aspeed graphics is mostly used in servers, where no monitor is attached.
>>> This also affects the remote BMC resolution to 640x480, which is
>>> inconvenient, and breaks the anaconda installer.
>>
>> The formatting of these paragraphs still looks off.
> 
> ok, I think it's because I start a new line in the middle of the 
> sentence, before "The default resolution ..." ?
> I will fix that.
>>
>> It just occured to me that you probably want output polling to make 
>> detection work dynamically. See an old patch of mine on how that works:
>>
>> https://patchwork.freedesktop.org/patch/488460/
> 
> So, if I understand well, I just need to add:
> 
> connector->polled = DRM_CONNECTOR_POLL_CONNECT | 
> DRM_CONNECTOR_POLL_DISCONNECT;
> 
> and call drm_kms_helper_poll_init() ?

That's it, I think. The flags tell what to poll for and the call starts 
the polling timeout. It worked when I tested it with vga back then.

Best regards
Thomas

>>
>> We also get an interrupt when the connector state changes, but IDK how 
>> that has to be handled. So polling is the next best thing. Sorry for 
>> missing this in the earlier reviews.
> 
> Yes, there is no irq configured in the ast driver, so let's fallback to 
> polling.
> 

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]