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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Jocelyn Falempe <jfalempe@redhat.com>,
	airlied@redhat.com, kuohsiang_chou@aspeedtech.com,
	jammy_huang@aspeedtech.com, jani.nikula@linux.intel.com
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/ast: report connection status on Display Port.
Date: Mon, 10 Jul 2023 08:57:54 +0200	[thread overview]
Message-ID: <adbab7c1-2dee-1f00-dbf9-ef63bd9b51c9@suse.de> (raw)
In-Reply-To: <20230706100102.563458-1-jfalempe@redhat.com>

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

Hi Jocelyn

Am 06.07.23 um 11:58 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 formating of these paraghraphs looks off.

> 
> v2: Add .detect callback to the dp/dp501 connector (Jani Nikula)
> 
> Signed-off-by: Jocelyn Falempe <jfalempe@redhat.com>

Maybe add Fixes with the patch that added the DP default resolution. 
It's not really correct, but at least tells people to consider your patch.

I assume that you'll send another patch with the BMC connector?

> ---
>   drivers/gpu/drm/ast/ast_dp.c    |  9 ++++++++
>   drivers/gpu/drm/ast/ast_dp501.c | 37 ++++++++++++++++++++++-----------
>   drivers/gpu/drm/ast/ast_drv.h   |  2 ++
>   drivers/gpu/drm/ast/ast_mode.c  | 24 +++++++++++++++++++++
>   4 files changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c
> index 6dc1a09504e1..fbc154930fdf 100644
> --- a/drivers/gpu/drm/ast/ast_dp.c
> +++ b/drivers/gpu/drm/ast/ast_dp.c
> @@ -7,6 +7,15 @@
>   #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) &&
> +		ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDC, ASTDP_LINK_SUCCESS) &&
> +		ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xDF, ASTDP_HPD))

The indention of those two lines is off.

You could also rewrite this as

  if (!a)
    return false;
  if (!b)
    return false;
  if (!c)
    return false;
  return true;

> +		return true;
> +	return false;
> +}
> +
>   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 3f6e0c984523..99a24d779b9c 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -510,6 +510,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);
> @@ -518,6 +519,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 f711d592da52..ccf48d57d239 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1586,12 +1586,24 @@ static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs
>   	.get_modes = ast_dp501_connector_helper_get_modes,
>   };
>   
> +static enum drm_connector_status ast_dp501_connector_helper_detect(
> +	struct drm_connector *connector,
> +	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;
> +}

It seems more appropriate to implement this as connector helper func 
detect_ctx. It does the same in practice, but the detect funtionality is 
rather part of the helpers.

> +
>   static const struct drm_connector_funcs ast_dp501_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,
> +	.detect = ast_dp501_connector_helper_detect,

That might sound pedantic, but I'd prefer to have the assignments sorted 
as in the struct's definiation. :) So .detect would be set just below 
.reset.

>   };
>   
>   static int ast_dp501_connector_init(struct drm_device *dev, struct drm_connector *connector)
> @@ -1684,12 +1696,24 @@ static const struct drm_connector_helper_funcs ast_astdp_connector_helper_funcs
>   	.get_modes = ast_astdp_connector_helper_get_modes,
>   };
>   
> +static enum drm_connector_status ast_astdp_connector_helper_detect(
> +	struct drm_connector *connector,
> +	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_funcs ast_astdp_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,
> +	.detect = ast_astdp_connector_helper_detect,

Same here.

Best regards
Thomas

>   };
>   
>   static int ast_astdp_connector_init(struct drm_device *dev, struct drm_connector *connector)
> 
> 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 --]