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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 11.10.24 um 10:51 schrieb Jani Nikula:
> 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.

Apologies. I'll include that information in any follow-up and the kernel 
docs.

FTR it's the baseboard management controller.

 https://en.wikipedia.org/wiki/Intelligent_Platform_Management_Interface#Baseboard_management_controller

Best regards
Thomas

> > 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).

--
--
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)