Re: [igt-dev] [PATCH i-g-t 1/5] lib/igt_kms: Add functions to get only a single output for a pipe.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 14, 2018 at 12:20:17PM +0100, Maarten Lankhorst wrote:
> igt_get_single_output_for_pipe() will give a valid output for a pipe,
> for_each_pipe_with_single_output will iterate over all pipes, and
> will be called for each pipe with an output once.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  lib/igt_kms.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_kms.h | 26 +++++++++++++++++
>  2 files changed, 119 insertions(+)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 6bac4d1fae50..1313ef12ebc5 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -2195,6 +2195,99 @@ igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type)
>  	return &pipe->planes[plane_idx];
>  }
>  
> +static bool output_is_internal_panel(igt_output_t *output)
> +{
> +	switch (output->config.connector->connector_type) {
> +	case DRM_MODE_CONNECTOR_LVDS:
> +	case DRM_MODE_CONNECTOR_eDP:
> +	case DRM_MODE_CONNECTOR_DSI:
> +	case DRM_MODE_CONNECTOR_DPI:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +igt_output_t **__igt_pipe_populate_outputs(igt_display_t *display, igt_output_t **chosen_outputs)
> +{
> +	unsigned full_pipe_mask = (1 << (display->n_pipes)) - 1, assigned_pipes = 0;
> +	igt_output_t *output;
> +	int i, j;
> +
> +	memset(chosen_outputs, 0, sizeof(*chosen_outputs) * display->n_pipes);
> +
> +	/*
> +	 * Try to assign all outputs to the first available CRTC for
> +	 * it, start with the outputs restricted to 1 pipe, then increase
> +	 * number of pipes until we assign connectors to all pipes.
> +	 */
> +	for (i = 0; i <= display->n_pipes; i++) {
> +		for_each_connected_output(display, output) {
> +			uint32_t pipe_mask = output->config.valid_crtc_idx_mask & full_pipe_mask;
> +			bool found = false;
> +
> +			if (output_is_internal_panel(output)) {
> +				/*
> +				 * Internal panel should be assigned to pipe A
> +				 * if possible, so make sure they're enumerated
> +				 * first.
> +				 */
> +
> +				if (i)
> +					continue;
> +			} else if (__builtin_popcount(pipe_mask) != i)
> +				continue;
> +
> +			for (j = 0; j < display->n_pipes; j++) {
> +				bool pipe_assigned = assigned_pipes & (1 << j);
> +
> +				if (pipe_assigned || !(pipe_mask & (1 << j)))
> +					continue;
> +
> +				if (!found) {
> +					/* We found an unassigned pipe, use it! */
> +					found = true;
> +					assigned_pipes |= 1 << j;
> +					chosen_outputs[j] = output;
> +				} else if (!chosen_outputs[j] ||
> +					   /*
> +					    * Overwrite internal panel if not assigned,
> +					    * external outputs are faster to do modesets
> +					    */
> +					   output_is_internal_panel(chosen_outputs[j]))
> +					chosen_outputs[j] = output;
> +			}

I think this might not be find the optimal solution in some cases since
it always picks the first available pipe. So in theory we might not pick
as many outputs that are actually available. But I suppose such
configurations shouldn't be commoin so probably will do fine for most
cases.

> +
> +			if (!found)
> +				igt_warn("Output %s could not be assigned to a pipe\n",
> +					 igt_output_name(output));
> +		}
> +	}
> +
> +	return chosen_outputs;
> +}
> +
> +/**
> + * igt_get_single_output_for_pipe:
> + * @display: a pointer to an #igt_display_t structure
> + * @pipe: The pipe for which an #igt_output_t must be returned.
> + *
> + * Get a compatible output for a pipe.
> + *
> + * Returns: A compatible output for a given pipe, or NULL.
> + */
> +igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe pipe)
> +{
> +	igt_output_t *chosen_outputs[display->n_pipes];
> +
> +	igt_assert(pipe != PIPE_NONE);
> +	igt_require(pipe < display->n_pipes);
> +
> +	__igt_pipe_populate_outputs(display, chosen_outputs);
> +
> +	return chosen_outputs[pipe];
> +}
> +
>  static igt_output_t *igt_pipe_get_output(igt_pipe_t *pipe)
>  {
>  	igt_display_t *display = pipe->display;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 1c46186e8a9d..178b636c143e 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -380,7 +380,9 @@ igt_plane_t *igt_output_get_plane(igt_output_t *output, int plane_idx);
>  igt_plane_t *igt_output_get_plane_type(igt_output_t *output, int plane_type);
>  igt_output_t *igt_output_from_connector(igt_display_t *display,
>      drmModeConnector *connector);
> +
>  igt_plane_t *igt_pipe_get_plane_type(igt_pipe_t *pipe, int plane_type);
> +igt_output_t *igt_get_single_output_for_pipe(igt_display_t *display, enum pipe pipe);
>  
>  void igt_pipe_request_out_fence(igt_pipe_t *pipe);
>  
> @@ -468,6 +470,10 @@ static inline bool igt_output_is_connected(igt_output_t *output)
>   *
>   * This for loop is called over all connected outputs. This function
>   * will try every combination of @pipe and @output.
> + *
> + * If you only need to test a single output for each pipe, use
> + * for_each_pipe_with_single_output(), if you only need an
> + * output for a single pipe, use igt_get_single_output_for_pipe().
>   */
>  #define for_each_pipe_with_valid_output(display, pipe, output) \
>  	for (int con__ = (pipe) = 0; \
> @@ -476,6 +482,26 @@ static inline bool igt_output_is_connected(igt_output_t *output)
>  		for_each_if ((((output) = &(display)->outputs[con__]), \
>  			     igt_pipe_connector_valid((pipe), (output))))
>  
> +igt_output_t **__igt_pipe_populate_outputs(igt_display_t *display,
> +					   igt_output_t **chosen_outputs);
> +
> +/**
> + * for_each_pipe_with_single_output:
> + * @display: a pointer to an #igt_display_t structure
> + * @pipe: The pipe for which this @pipe / @output combination is valid.
> + * @output: The output for which this @pipe / @output combination is valid.
> + *
> + * This loop is called over all pipes, and will try to find a compatible output
> + * for each pipe. Unlike for_each_pipe_with_valid_output(), this function will
> + * be called at most once for each pipe.
> + */
> +#define for_each_pipe_with_single_output(display, pipe, output) \
> +	for (igt_output_t *__outputs[(display)->n_pipes], \
> +	     **__output = __igt_pipe_populate_outputs((display), __outputs); \
> +	     __output < &__outputs[(display)->n_pipes]; __output++) \
> +		for_each_if (*__output && \
> +			     ((pipe) = (__outputs - __output), (output) = *__output, 1))

That requires quite a sophisticated parser :) But it does look correct to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  /**
>   * for_each_valid_output_on_pipe:
>   * @display: a pointer to an #igt_display_t structure
> -- 
> 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