Re: [PATCH 2/5] drm/mgag200: vga-bmc: Transparently handle BMC

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 05 Aug 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> The VGA-BMC connector selects the VGA output if a display has been
> attached to the physical connector. Otherwise it selects the BMC
> output. In any case, the connector status is set to 'detected', so
> that the userspace compositor displays to it.
>
> Depending on the setting, the connector's display modes either come
> from the VGA monitor's EDID or from an internal list of BMC-compatible
> modes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_vga_bmc.c | 50 ++++++++++++++++++++++-
>  1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> index b6b90632b3c6..3a958c3587ac 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_vga_bmc.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_edid.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -11,9 +12,54 @@ static const struct drm_encoder_funcs mgag200_dac_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup
>  };
>  
> +static int mgag200_vga_bmc_connector_helper_get_modes(struct drm_connector *connector)
> +{
> +	struct mga_device *mdev = to_mga_device(connector->dev);
> +	const struct mgag200_device_info *minfo = mdev->info;
> +	int count;
> +
> +	count = drm_connector_helper_get_modes(connector);
> +
> +	if (!count) {
> +		/*
> +		 * There's no EDID data without a connected monitor. Set BMC-
> +		 * compatible modes in this case. The XGA default resolution
> +		 * should work well for all BMCs.
> +		 */
> +		count = drm_add_modes_noedid(connector, minfo->max_hdisplay, minfo->max_vdisplay);
> +		if (count)
> +			drm_set_preferred_mode(connector, 1024, 768);
> +	}
> +
> +	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)

This is now the only place outside of drm_edid.c that uses edid_blob_ptr
for anything.

Seems like you're using it as a proxy for "had a display connected".

I wish it could be kept private to the EDID code.


BR,
Jani.


> +		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 = drm_connector_helper_get_modes,
> -	.detect_ctx = drm_connector_helper_detect_from_ddc
> +	.get_modes = mgag200_vga_bmc_connector_helper_get_modes,
> +	.detect_ctx = mgag200_vga_bmc_connector_helper_detect_ctx,
>  };
>  
>  static const struct drm_connector_funcs mgag200_vga_connector_funcs = {

-- 
Jani Nikula, Intel