[igt-dev] [RFC incomplete PATCH i-g-t] lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()

[igt-dev] [RFC incomplete PATCH i-g-t] lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()

[Maarten Lankhorst]

When booting with fbdev disabled, all outputs are set to UNKNOWNCONNECTION.
In this case we have to reprobe, but in all others we can safely assume that
the output connector status is valid.

Enumerate all outputs, and for good measure recheck after we probed, to add
and DP-MST we may have missed.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Lyude <lyude@redhat.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
Unfortunately we lack a way to determine whether the probing will enumerate
additional DP-MST displays, so the only way right now to test DP-MST would be
to add a sleep(5) in igt_display_check_hotplug.

Any thoughts how we can make this more reliable for DP-MST?

~Maarten

lib/igt_kms.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 lib/igt_kms.h |   2 +-
 2 files changed, 99 insertions(+), 23 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 9e32f40673bb..2417af2fc082 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -974,7 +974,7 @@ _kmstest_connector_config_find_encoder(int drm_fd, drmModeConnector *connector,
 static bool _kmstest_connector_config(int drm_fd, uint32_t connector_id,
 				      unsigned long crtc_idx_mask,
 				      struct kmstest_connector_config *config,
-				      bool probe)
+				      int probe)
 {
 	drmModeRes *resources;
 	drmModeConnector *connector;
@@ -988,11 +988,19 @@ static bool _kmstest_connector_config(int drm_fd, uint32_t connector_id,
 	}
 
 	/* First, find the connector & mode */
-	if (probe)
+	if (probe > 0)
 		connector = drmModeGetConnector(drm_fd, connector_id);
-	else
+	else {
 		connector = drmModeGetConnectorCurrent(drm_fd, connector_id);
 
+		if (probe < 0 && connector &&
+		    connector->connection == DRM_MODE_UNKNOWNCONNECTION) {
+			drmModeFreeConnector(connector);
+
+			connector = drmModeGetConnector(drm_fd, connector_id);
+		}
+	}
+
 	if (!connector)
 		goto err2;
 
@@ -1802,6 +1810,87 @@ void igt_display_reset(igt_display_t *display)
 	}
 }
 
+static void __igt_display_append_output(igt_display_t *display, unsigned id)
+{
+	igt_output_t *output;
+
+	output = &display->outputs[display->n_outputs++];
+	memset(output, 0, sizeof(*output));
+
+	/*
+	 * We don't assign each output a pipe unless
+	 * a pipe is set with igt_output_set_pipe().
+	 */
+	output->id = id;
+	output->display = display;
+	output->pending_pipe = PIPE_NONE;
+	output->force_reprobe = -1;
+
+	igt_output_refresh(output);
+}
+
+static void igt_display_append_output(igt_display_t *display, unsigned id)
+{
+	display->outputs = realloc(display->outputs, sizeof(*display->outputs) * (1 + display->n_outputs));
+	igt_assert(display->outputs);
+
+	__igt_display_append_output(display, id);
+
+	igt_info("HOTPLUG! Added new output %s\n", output->name);
+}
+
+static void igt_display_check_hotplug(igt_display_t *display)
+{
+	int i, j, num_outputs;
+	uint64_t connector_mask = 0;
+	igt_output_t *new_outputs;
+	drmModeRes *resources;
+
+	/* Outputs may have been discovered by igt_output_refresh(), retry.. */
+	resources = drmModeGetResources(display->drm_fd);
+
+	for (i = 0; i < resources->count_connectors; i++) {
+		unsigned id = resources->connectors[i];
+		bool found = false;
+
+		for (j = 0; j < display->n_outputs; j++) {
+			igt_output_t *output = &display->outputs[j];
+
+			if (output->id != id)
+				continue;
+
+			found = true;
+			break;
+		}
+
+		if (!found)
+			igt_display_append_output(display, id);
+
+		connector_mask |= 1ULL << j;
+	}
+
+	drmModeFreeResources(resources);
+
+	num_outputs = __builtin_popcountll(connector_mask);
+	if (num_outputs == display->n_outputs)
+		return;
+
+	new_outputs = malloc(sizeof(*new_outputs) * num_outputs);
+
+	/* Garbage collect dead outputs. */
+	for (i = j = 0; i < display->n_outputs; i++) {
+		if (!(connector_mask & (1ULL << i))) {
+			igt_info("Output %s disappeared!\n", display->outputs[i].name);
+			continue;
+		}
+
+		new_outputs[j++] = display->outputs[i];
+	}
+
+	free(display->outputs);
+	display->outputs = new_outputs;
+}
+
 static void igt_fill_plane_format_mod(igt_display_t *display, igt_plane_t *plane);
 static void igt_fill_display_format_mod(igt_display_t *display);
 
@@ -2012,31 +2101,18 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 
 	igt_fill_display_format_mod(display);
 
-	/*
-	 * The number of connectors is set, so we just initialize the outputs
-	 * array in _init(). This may change when we need dynamic connectors
-	 * (say DisplayPort MST).
-	 */
-	display->n_outputs = resources->count_connectors;
-	display->outputs = calloc(display->n_outputs, sizeof(igt_output_t));
+	display->n_outputs = 0;
+	display->outputs = malloc(display->n_outputs * sizeof(igt_output_t));
 	igt_assert_f(display->outputs, "Failed to allocate memory for %d outputs\n", display->n_outputs);
 
-	for (i = 0; i < display->n_outputs; i++) {
-		igt_output_t *output = &display->outputs[i];
-
-		/*
-		 * We don't assign each output a pipe unless
-		 * a pipe is set with igt_output_set_pipe().
-		 */
-		output->id = resources->connectors[i];
-		output->display = display;
-
-		igt_output_refresh(output);
-	}
+	for (i = 0; i < display->n_outputs; i++)
+		__igt_display_append_output(display, resources->connectors[i]);
 
 	drmModeFreePlaneResources(plane_resources);
 	drmModeFreeResources(resources);
 
+	igt_display_check_hotplug(display);
+
 	/* Set reasonable default values for every object in the display. */
 	igt_display_reset(display);
 
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 6f16dc8cf85e..9e734ab1a5b8 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -340,7 +340,7 @@ typedef struct {
 	uint32_t id;					/* KMS id */
 	struct kmstest_connector_config config;
 	char *name;
-	bool force_reprobe;
+	int force_reprobe;
 	enum pipe pending_pipe;
 	bool use_override_mode;
 	drmModeModeInfo override_mode;
-- 
2.16.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [RFC incomplete PATCH i-g-t] lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()

[Ville Syrjälä]

On Fri, Mar 23, 2018 at 01:48:28PM +0100, Maarten Lankhorst wrote:
> When booting with fbdev disabled, all outputs are set to UNKNOWNCONNECTION.
> In this case we have to reprobe, but in all others we can safely assume that
> the output connector status is valid.
> 
> Enumerate all outputs, and for good measure recheck after we probed, to add
> and DP-MST we may have missed.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Lyude <lyude@redhat.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
> Unfortunately we lack a way to determine whether the probing will enumerate
> additional DP-MST displays, so the only way right now to test DP-MST would be
> to add a sleep(5) in igt_display_check_hotplug.
> 
> Any thoughts how we can make this more reliable for DP-MST?

When do we fill out the debugfs mst topology information?

> 
> ~Maarten
> 
> lib/igt_kms.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>  lib/igt_kms.h |   2 +-
>  2 files changed, 99 insertions(+), 23 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 9e32f40673bb..2417af2fc082 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -974,7 +974,7 @@ _kmstest_connector_config_find_encoder(int drm_fd, drmModeConnector *connector,
>  static bool _kmstest_connector_config(int drm_fd, uint32_t connector_id,
>  				      unsigned long crtc_idx_mask,
>  				      struct kmstest_connector_config *config,
> -				      bool probe)
> +				      int probe)
>  {
>  	drmModeRes *resources;
>  	drmModeConnector *connector;
> @@ -988,11 +988,19 @@ static bool _kmstest_connector_config(int drm_fd, uint32_t connector_id,
>  	}
>  
>  	/* First, find the connector & mode */
> -	if (probe)
> +	if (probe > 0)
>  		connector = drmModeGetConnector(drm_fd, connector_id);
> -	else
> +	else {
>  		connector = drmModeGetConnectorCurrent(drm_fd, connector_id);
>  
> +		if (probe < 0 && connector &&
> +		    connector->connection == DRM_MODE_UNKNOWNCONNECTION) {
> +			drmModeFreeConnector(connector);
> +
> +			connector = drmModeGetConnector(drm_fd, connector_id);
> +		}
> +	}
> +
>  	if (!connector)
>  		goto err2;
>  
> @@ -1802,6 +1810,87 @@ void igt_display_reset(igt_display_t *display)
>  	}
>  }
>  
> +static void __igt_display_append_output(igt_display_t *display, unsigned id)
> +{
> +	igt_output_t *output;
> +
> +	output = &display->outputs[display->n_outputs++];
> +	memset(output, 0, sizeof(*output));
> +
> +	/*
> +	 * We don't assign each output a pipe unless
> +	 * a pipe is set with igt_output_set_pipe().
> +	 */
> +	output->id = id;
> +	output->display = display;
> +	output->pending_pipe = PIPE_NONE;
> +	output->force_reprobe = -1;
> +
> +	igt_output_refresh(output);
> +}
> +
> +static void igt_display_append_output(igt_display_t *display, unsigned id)
> +{
> +	display->outputs = realloc(display->outputs, sizeof(*display->outputs) * (1 + display->n_outputs));
> +	igt_assert(display->outputs);
> +
> +	__igt_display_append_output(display, id);
> +
> +	igt_info("HOTPLUG! Added new output %s\n", output->name);
> +}
> +
> +static void igt_display_check_hotplug(igt_display_t *display)
> +{
> +	int i, j, num_outputs;
> +	uint64_t connector_mask = 0;
> +	igt_output_t *new_outputs;
> +	drmModeRes *resources;
> +
> +	/* Outputs may have been discovered by igt_output_refresh(), retry.. */
> +	resources = drmModeGetResources(display->drm_fd);
> +
> +	for (i = 0; i < resources->count_connectors; i++) {
> +		unsigned id = resources->connectors[i];
> +		bool found = false;
> +
> +		for (j = 0; j < display->n_outputs; j++) {
> +			igt_output_t *output = &display->outputs[j];
> +
> +			if (output->id != id)
> +				continue;
> +
> +			found = true;
> +			break;
> +		}
> +
> +		if (!found)
> +			igt_display_append_output(display, id);
> +
> +		connector_mask |= 1ULL << j;
> +	}
> +
> +	drmModeFreeResources(resources);
> +
> +	num_outputs = __builtin_popcountll(connector_mask);
> +	if (num_outputs == display->n_outputs)
> +		return;
> +
> +	new_outputs = malloc(sizeof(*new_outputs) * num_outputs);
> +
> +	/* Garbage collect dead outputs. */
> +	for (i = j = 0; i < display->n_outputs; i++) {
> +		if (!(connector_mask & (1ULL << i))) {
> +			igt_info("Output %s disappeared!\n", display->outputs[i].name);
> +			continue;
> +		}
> +
> +		new_outputs[j++] = display->outputs[i];
> +	}
> +
> +	free(display->outputs);
> +	display->outputs = new_outputs;
> +}
> +
>  static void igt_fill_plane_format_mod(igt_display_t *display, igt_plane_t *plane);
>  static void igt_fill_display_format_mod(igt_display_t *display);
>  
> @@ -2012,31 +2101,18 @@ void igt_display_init(igt_display_t *display, int drm_fd)
>  
>  	igt_fill_display_format_mod(display);
>  
> -	/*
> -	 * The number of connectors is set, so we just initialize the outputs
> -	 * array in _init(). This may change when we need dynamic connectors
> -	 * (say DisplayPort MST).
> -	 */
> -	display->n_outputs = resources->count_connectors;
> -	display->outputs = calloc(display->n_outputs, sizeof(igt_output_t));
> +	display->n_outputs = 0;
> +	display->outputs = malloc(display->n_outputs * sizeof(igt_output_t));
>  	igt_assert_f(display->outputs, "Failed to allocate memory for %d outputs\n", display->n_outputs);
>  
> -	for (i = 0; i < display->n_outputs; i++) {
> -		igt_output_t *output = &display->outputs[i];
> -
> -		/*
> -		 * We don't assign each output a pipe unless
> -		 * a pipe is set with igt_output_set_pipe().
> -		 */
> -		output->id = resources->connectors[i];
> -		output->display = display;
> -
> -		igt_output_refresh(output);
> -	}
> +	for (i = 0; i < display->n_outputs; i++)
> +		__igt_display_append_output(display, resources->connectors[i]);
>  
>  	drmModeFreePlaneResources(plane_resources);
>  	drmModeFreeResources(resources);
>  
> +	igt_display_check_hotplug(display);
> +
>  	/* Set reasonable default values for every object in the display. */
>  	igt_display_reset(display);
>  
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 6f16dc8cf85e..9e734ab1a5b8 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -340,7 +340,7 @@ typedef struct {
>  	uint32_t id;					/* KMS id */
>  	struct kmstest_connector_config config;
>  	char *name;
> -	bool force_reprobe;
> +	int force_reprobe;
>  	enum pipe pending_pipe;
>  	bool use_override_mode;
>  	drmModeModeInfo override_mode;
> -- 
> 2.16.2
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [RFC incomplete PATCH i-g-t] lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()

[Maarten Lankhorst]

Op 23-03-18 om 14:03 schreef Ville Syrjälä:
> On Fri, Mar 23, 2018 at 01:48:28PM +0100, Maarten Lankhorst wrote:
>> When booting with fbdev disabled, all outputs are set to UNKNOWNCONNECTION.
>> In this case we have to reprobe, but in all others we can safely assume that
>> the output connector status is valid.
>>
>> Enumerate all outputs, and for good measure recheck after we probed, to add
>> and DP-MST we may have missed.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Lyude <lyude@redhat.com>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> ---
>> Unfortunately we lack a way to determine whether the probing will enumerate
>> additional DP-MST displays, so the only way right now to test DP-MST would be
>> to add a sleep(5) in igt_display_check_hotplug.
>>
>> Any thoughts how we can make this more reliable for DP-MST?
> When do we fill out the debugfs mst topology information?
I have no idea..

drm_dp_mst_link_probe_work seems to be called from irq, I don't think we initially do discovery, maybe we should..
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [RFC incomplete PATCH i-g-t] lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()

[Pandiyan, Dhinakaran]

On Fri, 2018-03-23 at 14:55 +0100, Maarten Lankhorst wrote:
> Op 23-03-18 om 14:03 schreef Ville Syrjälä:
> > On Fri, Mar 23, 2018 at 01:48:28PM +0100, Maarten Lankhorst wrote:
> >> When booting with fbdev disabled, all outputs are set to UNKNOWNCONNECTION.
> >> In this case we have to reprobe, but in all others we can safely assume that
> >> the output connector status is valid.
> >>
> >> Enumerate all outputs, and for good measure recheck after we probed, to add
> >> and DP-MST we may have missed.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Lyude <lyude@redhat.com>
> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> ---
> >> Unfortunately we lack a way to determine whether the probing will enumerate
> >> additional DP-MST displays, so the only way right now to test DP-MST would be
> >> to add a sleep(5) in igt_display_check_hotplug.
> >>
> >> Any thoughts how we can make this more reliable for DP-MST?
> > When do we fill out the debugfs mst topology information?
> I have no idea..
> 
> drm_dp_mst_link_probe_work seems to be called from irq, I don't think we initially do discovery, maybe we should..

Correct me if I am wrong.

getconnector->fill_modes->detect->long_pulse->intel_dp_configure_mst->drm_dp_mst_link_probe_work should lead to
/sys/kernel/debug/dri/0/i915_dp_mst_info populating the status of downstream connectors.

Isn't it possible to wait on that?


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()

[Patchwork]

== Series Details ==

Series: lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()
URL   : https://patchwork.freedesktop.org/series/40569/
State : failure

== Summary ==

Applying: lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()
Patch failed at 0001 lib/igt_kms: Reduce probing and allow initial hotplug during igt_display_init()
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev