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