Re: [PATCH v2] mode: Retrieve only the current information for a Connector

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux