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

[Jani Nikula]

On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann at 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 at 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