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

[Thomas Zimmermann]

Hi,

thanks for this patch.

Am 12.07.23 um 10:35 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.
> 
> Fixes: fae7d186403e ("drm/probe-helper: Default to 640x480 if no EDID on DP")
> Signed-off-by: Jocelyn Falempe <jfalempe at redhat.com>
> ---
>   drivers/gpu/drm/ast/ast_drv.h  |  4 ++
>   drivers/gpu/drm/ast/ast_mode.c | 67 ++++++++++++++++++++++++++++++++++
>   2 files changed, 71 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..8896b22eb5cf 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1735,6 +1735,70 @@ 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, 1024, 768);

That's probably too small. The CRTC lists resolutions up to 1920x1200. 
Returning 1024x768 could easily filter out those higher-res modes.

The BMC can probably just use whatever the CRTC offers. So rather call 
drm_add_modes_noedid() with 4096x4096. At 32 bpp, this covers the max 
memory of 64 MiB.  The CRTC will filter out unsupported modes.


> +}
> +
> +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 int ast_bmc_connector_init(struct drm_device *dev,
> +				  struct drm_connector *connector)
> +{
> +	int ret;
> +
> +	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);
> +
> +	connector->interlace_allowed = 0;
> +	connector->doublescan_allowed = 0;
> +	connector->polled = 0;

It's zero-allocated memory. Please don't clear these fields manually. (I 
know that ast doesn't get this right.)

> +
> +	return 0;
> +}
> +
> +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_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);

Adding the simple_encoder helper was a mistake. Please open-code its 
functionality in ast. (Also something that ast currently does not.)

> +	if (ret)
> +		return ret;
> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> +
> +	ret = ast_bmc_connector_init(dev, connector);

Maybe just inline this call. It's simple enough.

Best regards
Thomas

> +	if (ret)
> +		return ret;
> +
> +	ret = drm_connector_attach_encoder(connector, encoder);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>   /*
>    * Mode config
>    */
> @@ -1842,6 +1906,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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230712/db1c8c21/attachment-0001.sig>