| Message ID | 20241211230328.4012496-3-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/connector: Expose only a properly inited connector | expand |
On Thu, 12 Dec 2024, Imre Deak <imre.deak@intel.com> wrote: > The connectors enumerated by the GETRESOURCES ioctl may not be fully > initialized yet wrt. to the state set up during connector registration > (for instance the connector's debugfs/sysfs interfaces may not exist > yet). This can happen in two ways: > > 1. Connectors initialized and added to the > drm_mode_config::connector_list during driver loading will be visible > to the GETRESOURCES ioctl caller once the driver is registered via > drm_dev_register()->drm_minor_register(DRM_MINOR_PRIMARY) and before > the connectors are registered via drm_dev_register()-> > drm_modeset_register_all(). > > 2. Dynamic connectors (MST) - after being initialized - may be added to > the connector_list after the driver is loaded and registered and before > the connector's userspace interfaces (debugfs, sysfs etc.) are added > in drm_connector_dynamic_register(). > > A solution for 1. would be to register the driver only after the > connectors are registered, for 2. to add the connector to connector_list > only after the userspace interfaces are registered. > > The fix would require a bigger change, not the topic of this patchset, > so adding only a FIXME comment here. References to "this patchset" become meaningless after this has been committed. Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > Suggested-by: Simona Vetter <simona.vetter@ffwll.ch> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/drm_mode_config.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 37d2e0a4ef4be..8642a2fb25a90 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -150,6 +150,15 @@ int drm_mode_getresources(struct drm_device *dev, void *data, > drm_connector_list_iter_begin(dev, &conn_iter); > count = 0; > connector_id = u64_to_user_ptr(card_res->connector_id_ptr); > + /* > + * FIXME: the connectors on the list may not be fully initialized yet, > + * if the ioctl is called before the connectors are registered. (See > + * drm_dev_register()->drm_modeset_register_all() for static and > + * drm_connector_dynamic_register() for dynamic connectors.) > + * The driver should only get registered after static connectors are > + * fully initialized and dynamic connectors should be added to the > + * connector list only after fully initializing them. > + */ > drm_for_each_connector_iter(connector, &conn_iter) { > /* only expose writeback connectors if userspace understands them */ > if (!file_priv->writeback_connectors &&
On Thu, Dec 12, 2024 at 12:06:12PM +0200, Jani Nikula wrote: > On Thu, 12 Dec 2024, Imre Deak <imre.deak@intel.com> wrote: > > The connectors enumerated by the GETRESOURCES ioctl may not be fully > > initialized yet wrt. to the state set up during connector registration > > (for instance the connector's debugfs/sysfs interfaces may not exist > > yet). This can happen in two ways: > > > > 1. Connectors initialized and added to the > > drm_mode_config::connector_list during driver loading will be visible > > to the GETRESOURCES ioctl caller once the driver is registered via > > drm_dev_register()->drm_minor_register(DRM_MINOR_PRIMARY) and before > > the connectors are registered via drm_dev_register()-> > > drm_modeset_register_all(). > > > > 2. Dynamic connectors (MST) - after being initialized - may be added to > > the connector_list after the driver is loaded and registered and before > > the connector's userspace interfaces (debugfs, sysfs etc.) are added > > in drm_connector_dynamic_register(). > > > > A solution for 1. would be to register the driver only after the > > connectors are registered, for 2. to add the connector to connector_list > > only after the userspace interfaces are registered. > > > > The fix would require a bigger change, not the topic of this patchset, > > so adding only a FIXME comment here. > > References to "this patchset" become meaningless after this has been > committed. I wondered if the Link: added when the patch is committed provides the context, but I suppose the link can get stale. I can reword the above to: "The fix requires a bigger change, for now adding a FIXME: comment for it." > Reviewed-by: Jani Nikula <jani.nikula@intel.com> > > > > > Suggested-by: Simona Vetter <simona.vetter@ffwll.ch> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/drm_mode_config.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > > index 37d2e0a4ef4be..8642a2fb25a90 100644 > > --- a/drivers/gpu/drm/drm_mode_config.c > > +++ b/drivers/gpu/drm/drm_mode_config.c > > @@ -150,6 +150,15 @@ int drm_mode_getresources(struct drm_device *dev, void *data, > > drm_connector_list_iter_begin(dev, &conn_iter); > > count = 0; > > connector_id = u64_to_user_ptr(card_res->connector_id_ptr); > > + /* > > + * FIXME: the connectors on the list may not be fully initialized yet, > > + * if the ioctl is called before the connectors are registered. (See > > + * drm_dev_register()->drm_modeset_register_all() for static and > > + * drm_connector_dynamic_register() for dynamic connectors.) > > + * The driver should only get registered after static connectors are > > + * fully initialized and dynamic connectors should be added to the > > + * connector list only after fully initializing them. > > + */ > > drm_for_each_connector_iter(connector, &conn_iter) { > > /* only expose writeback connectors if userspace understands them */ > > if (!file_priv->writeback_connectors && > > -- > Jani Nikula, Intel
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 37d2e0a4ef4be..8642a2fb25a90 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -150,6 +150,15 @@ int drm_mode_getresources(struct drm_device *dev, void *data, drm_connector_list_iter_begin(dev, &conn_iter); count = 0; connector_id = u64_to_user_ptr(card_res->connector_id_ptr); + /* + * FIXME: the connectors on the list may not be fully initialized yet, + * if the ioctl is called before the connectors are registered. (See + * drm_dev_register()->drm_modeset_register_all() for static and + * drm_connector_dynamic_register() for dynamic connectors.) + * The driver should only get registered after static connectors are + * fully initialized and dynamic connectors should be added to the + * connector list only after fully initializing them. + */ drm_for_each_connector_iter(connector, &conn_iter) { /* only expose writeback connectors if userspace understands them */ if (!file_priv->writeback_connectors &&
The connectors enumerated by the GETRESOURCES ioctl may not be fully initialized yet wrt. to the state set up during connector registration (for instance the connector's debugfs/sysfs interfaces may not exist yet). This can happen in two ways: 1. Connectors initialized and added to the drm_mode_config::connector_list during driver loading will be visible to the GETRESOURCES ioctl caller once the driver is registered via drm_dev_register()->drm_minor_register(DRM_MINOR_PRIMARY) and before the connectors are registered via drm_dev_register()-> drm_modeset_register_all(). 2. Dynamic connectors (MST) - after being initialized - may be added to the connector_list after the driver is loaded and registered and before the connector's userspace interfaces (debugfs, sysfs etc.) are added in drm_connector_dynamic_register(). A solution for 1. would be to register the driver only after the connectors are registered, for 2. to add the connector to connector_list only after the userspace interfaces are registered. The fix would require a bigger change, not the topic of this patchset, so adding only a FIXME comment here. Suggested-by: Simona Vetter <simona.vetter@ffwll.ch> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/drm_mode_config.c | 9 +++++++++ 1 file changed, 9 insertions(+)