| Message ID | -LYZxtmyBTf36wklyxa0PphDQ1FecAgEF7TMnSvyCm9Y_EFmz-4AUROX6qc4HKUjomE0HumDgVrSIbHsUMJnRSrBR2c3gPCVDNUmz7klPkE=@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm: expose connector status values in uapi | expand |
On Fri, 26 Jun 2020 07:55:12 +0000 Simon Ser <contact@emersion.fr> wrote: > Right now user-space doesn't have access to connector status definitions. > This means user-space needs to maintain a separate enum for these, and > makes it difficult to document the uapi. > > Introduce defines in drm_mode.h, and copy over the docs. Keep using the > drm_connector_status enum in drivers, because it allows for nice things > when using the enum as a type. > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com> > --- > > - Would something like this be desirable? > - Docs are just copied over for now, not improved > - Same applies to e.g. the subpixel field Hi, xf86drmMode.h in libdrm already has: typedef enum { DRM_MODE_CONNECTED = 1, DRM_MODE_DISCONNECTED = 2, DRM_MODE_UNKNOWNCONNECTION = 3 } drmModeConnection; I have no opinion really if adding yet another set of the same definitions is good or not. We do need the UAPI doc, but that does not necessarily mean we also need definition code in UAPI headers. I give this one a *shrug*. Thanks, pq > include/drm/drm_connector.h | 30 ++++-------------------------- > include/uapi/drm/drm_mode.h | 27 ++++++++++++++++++++++++++- > 2 files changed, 30 insertions(+), 27 deletions(-) > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index fd543d1db9b2..f48f8072aa89 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -53,34 +53,12 @@ enum drm_connector_force { > /** > * enum drm_connector_status - status for a &drm_connector > * > - * This enum is used to track the connector status. There are no separate > - * #defines for the uapi! > + * This enum is used to track the connector status. See the uapi docs. > */ > enum drm_connector_status { > - /** > - * @connector_status_connected: The connector is definitely connected to > - * a sink device, and can be enabled. > - */ > - connector_status_connected = 1, > - /** > - * @connector_status_disconnected: The connector isn't connected to a > - * sink device which can be autodetect. For digital outputs like DP or > - * HDMI (which can be realiable probed) this means there's really > - * nothing there. It is driver-dependent whether a connector with this > - * status can be lit up or not. > - */ > - connector_status_disconnected = 2, > - /** > - * @connector_status_unknown: The connector's status could not be > - * reliably detected. This happens when probing would either cause > - * flicker (like load-detection when the connector is in use), or when a > - * hardware resource isn't available (like when load-detection needs a > - * free CRTC). It should be possible to light up the connector with one > - * of the listed fallback modes. For default configuration userspace > - * should only try to light up connectors with unknown status when > - * there's not connector with @connector_status_connected. > - */ > - connector_status_unknown = 3, > + connector_status_connected = DRM_MODE_STATUS_CONNECTED, > + connector_status_disconnected = DRM_MODE_STATUS_DISCONNECTED, > + connector_status_unknown = DRM_MODE_STATUS_UNKNOWN, > }; > > /** > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 735c8cfdaaa1..0deffda35487 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -363,6 +363,31 @@ enum drm_mode_subconnector { > #define DRM_MODE_CONNECTOR_WRITEBACK 18 > #define DRM_MODE_CONNECTOR_SPI 19 > > +/** > + * @DRM_MODE_STATUS_CONNECTED: The connector is definitely connected to > + * a sink device, and can be enabled. > + */ > +#define DRM_MODE_STATUS_CONNECTED 1 > +/** > + * @DRM_MODE_STATUS_DISCONNECTED: The connector isn't connected to a > + * sink device which can be autodetect. For digital outputs like DP or > + * HDMI (which can be realiable probed) this means there's really > + * nothing there. It is driver-dependent whether a connector with this > + * status can be lit up or not. > + */ > +#define DRM_MODE_STATUS_DISCONNECTED 2 > +/** > + * @DRM_MODE_STATUS_UNKNOWN: The connector's status could not be > + * reliably detected. This happens when probing would either cause > + * flicker (like load-detection when the connector is in use), or when a > + * hardware resource isn't available (like when load-detection needs a > + * free CRTC). It should be possible to light up the connector with one > + * of the listed fallback modes. For default configuration userspace > + * should only try to light up connectors with unknown status when > + * there's not connector with @connector_status_connected. > + */ > +#define DRM_MODE_STATUS_UNKNOWN 3 > + > struct drm_mode_get_connector { > > __u64 encoders_ptr; > @@ -379,7 +404,7 @@ struct drm_mode_get_connector { > __u32 connector_type; > __u32 connector_type_id; > > - __u32 connection; > + __u32 connection; /**< see DRM_MODE_STATUS_* */ > __u32 mm_width; /**< width in millimeters */ > __u32 mm_height; /**< height in millimeters */ > __u32 subpixel;
On Friday, June 26, 2020 11:15 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > I have no opinion really if adding yet another set of the same > definitions is good or not. We do need the UAPI doc, but that does not > necessarily mean we also need definition code in UAPI headers. > > I give this one a shrug. But then uapi docs don't document uapi, instead document internal kernel enums? And also user-space not using libdrm needs to have these hardcoded somewhere. The libdrm re-definitions are annoying. Maybe a better way forward would be to have a "status" prop, which could then also be used for the planned fine-grained uapi events.
On Fri, 26 Jun 2020 12:05:16 +0000 Simon Ser <contact@emersion.fr> wrote: > On Friday, June 26, 2020 11:15 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > I have no opinion really if adding yet another set of the same > > definitions is good or not. We do need the UAPI doc, but that does not > > necessarily mean we also need definition code in UAPI headers. > > > > I give this one a shrug. > > But then uapi docs don't document uapi, instead document internal > kernel enums? And also user-space not using libdrm needs to have these > hardcoded somewhere. DRM properties are already like this. You don't find property names or enum value names in UAPI headers, you only find them in UAPI docs. > The libdrm re-definitions are annoying. Maybe a better way forward > would be to have a "status" prop, which could then also be used for > the planned fine-grained uapi events. That might be nice. Thanks, pq
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index fd543d1db9b2..f48f8072aa89 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -53,34 +53,12 @@ enum drm_connector_force { /** * enum drm_connector_status - status for a &drm_connector * - * This enum is used to track the connector status. There are no separate - * #defines for the uapi! + * This enum is used to track the connector status. See the uapi docs. */ enum drm_connector_status { - /** - * @connector_status_connected: The connector is definitely connected to - * a sink device, and can be enabled. - */ - connector_status_connected = 1, - /** - * @connector_status_disconnected: The connector isn't connected to a - * sink device which can be autodetect. For digital outputs like DP or - * HDMI (which can be realiable probed) this means there's really - * nothing there. It is driver-dependent whether a connector with this - * status can be lit up or not. - */ - connector_status_disconnected = 2, - /** - * @connector_status_unknown: The connector's status could not be - * reliably detected. This happens when probing would either cause - * flicker (like load-detection when the connector is in use), or when a - * hardware resource isn't available (like when load-detection needs a - * free CRTC). It should be possible to light up the connector with one - * of the listed fallback modes. For default configuration userspace - * should only try to light up connectors with unknown status when - * there's not connector with @connector_status_connected. - */ - connector_status_unknown = 3, + connector_status_connected = DRM_MODE_STATUS_CONNECTED, + connector_status_disconnected = DRM_MODE_STATUS_DISCONNECTED, + connector_status_unknown = DRM_MODE_STATUS_UNKNOWN, }; /** diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 735c8cfdaaa1..0deffda35487 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -363,6 +363,31 @@ enum drm_mode_subconnector { #define DRM_MODE_CONNECTOR_WRITEBACK 18 #define DRM_MODE_CONNECTOR_SPI 19 +/** + * @DRM_MODE_STATUS_CONNECTED: The connector is definitely connected to + * a sink device, and can be enabled. + */ +#define DRM_MODE_STATUS_CONNECTED 1 +/** + * @DRM_MODE_STATUS_DISCONNECTED: The connector isn't connected to a + * sink device which can be autodetect. For digital outputs like DP or + * HDMI (which can be realiable probed) this means there's really + * nothing there. It is driver-dependent whether a connector with this + * status can be lit up or not. + */ +#define DRM_MODE_STATUS_DISCONNECTED 2 +/** + * @DRM_MODE_STATUS_UNKNOWN: The connector's status could not be + * reliably detected. This happens when probing would either cause + * flicker (like load-detection when the connector is in use), or when a + * hardware resource isn't available (like when load-detection needs a + * free CRTC). It should be possible to light up the connector with one + * of the listed fallback modes. For default configuration userspace + * should only try to light up connectors with unknown status when + * there's not connector with @connector_status_connected. + */ +#define DRM_MODE_STATUS_UNKNOWN 3 + struct drm_mode_get_connector { __u64 encoders_ptr; @@ -379,7 +404,7 @@ struct drm_mode_get_connector { __u32 connector_type; __u32 connector_type_id; - __u32 connection; + __u32 connection; /**< see DRM_MODE_STATUS_* */ __u32 mm_width; /**< width in millimeters */ __u32 mm_height; /**< height in millimeters */ __u32 subpixel;
Right now user-space doesn't have access to connector status definitions. This means user-space needs to maintain a separate enum for these, and makes it difficult to document the uapi. Introduce defines in drm_mode.h, and copy over the docs. Keep using the drm_connector_status enum in drivers, because it allows for nice things when using the enum as a type. Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Pekka Paalanen <pekka.paalanen@collabora.com> --- - Would something like this be desirable? - Docs are just copied over for now, not improved - Same applies to e.g. the subpixel field include/drm/drm_connector.h | 30 ++++-------------------------- include/uapi/drm/drm_mode.h | 27 ++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 27 deletions(-)