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

[Thomas Zimmermann]

Hi

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

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

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