On Wed, Mar 04, 2015 at 10:38:08AM +0000, Chris Wilson wrote:
> Add a new API that allows the caller to skip any forced probing, which
> may require slow i2c to a remote display, and only report the currently
> active mode and encoder for a Connector. This is often the information
> of interest and is much, much faster than re-retrieving the link status
> and EDIDs, e.g. if the caller only wishes to count the number of active
> outputs.
>
> v2: Fix error path to avoid double free after a failed GETCONNECTOR
> ioctl.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> Cc: David Herrmann <dh.herrmann@xxxxxxxxxxxxxx>
Context for David: Apparently logind sees fit to probe drm connectors,
right when X starts and not using the cached state. Which needless adds up
to a few hundred ms delay since X then blocks when it does its own cached
getconnector. I've asked Chris to extract the code from the intel ddx to
make this a bit more official and known.
> ---
> tests/modeprint/modeprint.c | 18 ++++++++-
> xf86drmMode.c | 97 +++++++++++++++++++++++++++++++++++++++++++++
> xf86drmMode.h | 17 +++++++-
> 3 files changed, 128 insertions(+), 4 deletions(-)
>
> diff --git a/tests/modeprint/modeprint.c b/tests/modeprint/modeprint.c
> index 6f0d039..514d3ba 100644
> --- a/tests/modeprint/modeprint.c
> +++ b/tests/modeprint/modeprint.c
> @@ -43,6 +43,7 @@
>
> #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
>
> +int current;
> int connectors;
> int full_props;
> int edid;
> @@ -272,7 +273,7 @@ int printRes(int fd, drmModeResPtr res)
>
> if (connectors) {
> for (i = 0; i < res->count_connectors; i++) {
> - connector = drmModeGetConnector(fd, res->connectors[i]);
> + connector = (current ? drmModeGetConnectorCurrent : drmModeGetConnector) (fd, res->connectors[i]);
>
> if (!connector)
> printf("Could not get connector %i\n", res->connectors[i]);
> @@ -331,6 +332,7 @@ int printRes(int fd, drmModeResPtr res)
>
> void args(int argc, char **argv)
> {
> + int defaults = 1;
> int i;
>
> fbs = 0;
> @@ -341,32 +343,41 @@ void args(int argc, char **argv)
> full_modes = 0;
> full_props = 0;
> connectors = 0;
> + current = 0;
>
> module_name = argv[1];
>
> for (i = 2; i < argc; i++) {
> if (strcmp(argv[i], "-fb") == 0) {
> fbs = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-crtcs") == 0) {
> crtcs = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-cons") == 0) {
> connectors = 1;
> modes = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-modes") == 0) {
> connectors = 1;
> modes = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-full") == 0) {
> connectors = 1;
> modes = 1;
> full_modes = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-props") == 0) {
> connectors = 1;
> full_props = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-edids") == 0) {
> connectors = 1;
> edid = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-encoders") == 0) {
> encoders = 1;
> + defaults = 0;
> } else if (strcmp(argv[i], "-v") == 0) {
> fbs = 1;
> edid = 1;
> @@ -376,10 +387,13 @@ void args(int argc, char **argv)
> full_modes = 1;
> full_props = 1;
> connectors = 1;
> + defaults = 0;
> + } else if (strcmp(argv[i], "-current") == 0) {
> + current = 1;
> }
> }
>
> - if (argc == 2) {
> + if (defaults) {
> fbs = 1;
> edid = 1;
> crtcs = 1;
> diff --git a/xf86drmMode.c b/xf86drmMode.c
> index 9ea8fe7..4433dc7 100644
> --- a/xf86drmMode.c
> +++ b/xf86drmMode.c
> @@ -572,6 +572,103 @@ err_allocs:
> return r;
> }
>
> +drmModeConnectorPtr drmModeGetConnectorCurrent(int fd, uint32_t connector_id)
> +{
> + struct drm_mode_modeinfo mode;
> + struct drm_mode_get_encoder enc;
> + struct drm_mode_get_connector conn;
> + unsigned int num_props = 0;
> + drmModeConnectorPtr r;
> +
> + memclear(conn);
> + conn.connector_id = connector_id;
> + do {
> + if (conn.count_props) {
> + drmFree(U642VOID(conn.props_ptr));
> + conn.props_ptr = VOID2U64(drmMalloc(conn.count_props*sizeof(uint32_t)));
> + if (!conn.props_ptr)
> + goto err;
> +
> + drmFree(U642VOID(conn.prop_values_ptr));
> + conn.prop_values_ptr = VOID2U64(drmMalloc(conn.count_props*sizeof(uint64_t)));
> + if (!conn.prop_values_ptr)
> + goto err;
> +
> + num_props = conn.count_props;
> + }
> +
> + conn.count_modes = 1; /* skip detect */
> + conn.modes_ptr = (uintptr_t)&mode;
> + conn.count_encoders = 0;
> +
> + if (drmIoctl(fd, DRM_IOCTL_MODE_GETCONNECTOR, &conn))
> + goto err;
> + } while (conn.count_props != num_props);
> +
> + /* Retrieve the current mode */
> + memclear(mode);
> + memclear(enc);
> + if (conn.encoder_id) {
> + struct drm_mode_crtc crtc;
> +
> + enc.encoder_id = conn.encoder_id;
> + if (drmIoctl(fd, DRM_IOCTL_MODE_GETENCODER, &enc))
> + goto err;
> +
> + memclear(crtc);
> + crtc.crtc_id = enc.crtc_id;
> + if (drmIoctl(fd, DRM_IOCTL_MODE_GETCRTC, &crtc))
> + goto err;
> +
> + if (crtc.mode_valid)
> + mode = crtc.mode;
> + }
> +
> + if(!(r = drmMalloc(sizeof(*r))))
> + goto err;
> +
> + memset(r, 0, sizeof(*r));
> + r->connector_id = conn.connector_id;
> + r->encoder_id = conn.encoder_id;
> + r->connection = conn.connection;
> + r->mmWidth = conn.mm_width;
> + r->mmHeight = conn.mm_height;
> + /* convert subpixel from kernel to userspace */
> + r->subpixel = conn.subpixel + 1;
> +
> + if (mode.clock) {
> + r->count_modes = 1;
> + r->modes = drmAllocCpy(&mode, 1, sizeof(mode));
> + if (!r->modes)
> + goto err_copy;
> + }
> +
> + if (conn.encoder_id) {
> + r->count_encoders = 1;
This only works for i915 where we only ever have 1 encoder. Other drivers
reassign encoders depending upon output type (e.g. dvi-i vs dvi-d). Imo
it'd be cleaner to do something like the below:
diff --git a/xf86drmMode.c b/xf86drmMode.c
index 9ea8fe721842..3a9bb0b6560a 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -477,7 +477,7 @@ drmModeEncoderPtr drmModeGetEncoder(int fd, uint32_t encoder_id)
* Connector manipulation
*/
-drmModeConnectorPtr drmModeGetConnector(int fd, uint32_t connector_id)
+drmModeConnectorPtr __drmModeGetConnector(int fd, uint32_t connector_id, bool current)
{
struct drm_mode_get_connector conn, counts;
drmModeConnectorPtr r = NULL;
@@ -485,6 +485,7 @@ drmModeConnectorPtr drmModeGetConnector(int fd, uint32_t connector_id)
retry:
memclear(conn);
conn.connector_id = connector_id;
+ conn.count_modes = current;
if (drmIoctl(fd, DRM_IOCTL_MODE_GETCONNECTOR, &conn))
return 0;
@@ -572,6 +573,16 @@ err_allocs:
return r;
}
+drmModeConnectorPtr drmModeGetConnector(int fd, uint32_t connector_id)
+{
+ __drmModeGetConnector(fd, connector_id, false);
+}
+
+drmModeConnectorPtr drmModeGetConnector(int fd, uint32_t connector_id)
+{
+ __drmModeGetConnector(fd, connector_id, true);
+}
+
int drmModeAttachMode(int fd, uint32_t connector_id, drmModeModeInfoPtr mode_info)
{
struct drm_mode_mode_cmd res;
Totally untested diff ;-)
Cheers, Daniel
> + r->encoders = drmAllocCpy(&enc, 1, sizeof(enc));
> + if (!r->encoders)
> + goto err_copy;
> + }
> +
> + r->count_props = conn.count_props;
> + r->props = U642VOID(conn.props_ptr);
> + r->prop_values = U642VOID(conn.prop_values_ptr);
> +
> + r->connector_type = conn.connector_type;
> + r->connector_type_id = conn.connector_type_id;
> +
> + return r;
> +
> +err_copy:
> + drmFree(r->modes);
> + drmFree(r->encoders);
> + drmFree(r);
> +err:
> + drmFree(U642VOID(conn.props_ptr));
> + drmFree(U642VOID(conn.prop_values_ptr));
> + return 0;
> +}
> +
> int drmModeAttachMode(int fd, uint32_t connector_id, drmModeModeInfoPtr mode_info)
> {
> struct drm_mode_mode_cmd res;
> diff --git a/xf86drmMode.h b/xf86drmMode.h
> index 856a6bb..278da48 100644
> --- a/xf86drmMode.h
> +++ b/xf86drmMode.h
> @@ -422,10 +422,23 @@ drmModeEncoderPtr drmModeGetEncoder(int fd, uint32_t encoder_id);
> */
>
> /**
> - * Retrive information about the connector connectorId.
> + * Retrieve all information about the connector connectorId. This will do a
> + * forced probe on the connector to retrieve remote information such as EDIDs
> + * from the display device.
> */
> extern drmModeConnectorPtr drmModeGetConnector(int fd,
> - uint32_t connectorId);
> + uint32_t connectorId);
> +
> +/**
> + * Retrieve current information, i.e the currently active mode and encoder,
> + * about the connector connectorId. This will not do any probing on the
> + * connector or remote device, and only reports what is currently known.
> + * For the complete set of modes and encoders associated with the connector
> + * use drmModeGetConnector() which will do a probe to determine any display
> + * link changes first.
> + */
> +extern drmModeConnectorPtr drmModeGetConnectorCurrent(int fd,
> + uint32_t connector_id);
>
> /**
> * Attaches the given mode to an connector.
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
