| Message ID | 20241011065705.6728-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add physical status and BMC support to conenctor | expand |
On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann@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@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).
Hi Am 11.10.24 um 10:51 schrieb Jani Nikula: > On Fri, 11 Oct 2024, Thomas Zimmermann <tzimmermann@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@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).
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. + */ + enum drm_connector_status physical_status; + /** * @status: * One of the drm_connector_status enums (connected, not, or unknown).
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. 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@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(-)