[igt-dev] [[RFC] i-g-t 1/1] lib/igt_kms: Add support for display with non-contiguous pipes

[Khajapasha, Mohammed]

> -----Original Message-----
> From: Latvala, Petri <petri.latvala at intel.com>
> Sent: Tuesday, June 16, 2020 7:14 PM
> To: Khajapasha, Mohammed <mohammed.khajapasha at intel.com>
> Cc: igt-dev at lists.freedesktop.org
> Subject: Re: [igt-dev] [[RFC] i-g-t 1/1] lib/igt_kms: Add support for display
> with non-contiguous pipes
> 
> On Thu, Jun 11, 2020 at 01:07:24AM +0530, Mohammed Khajapasha wrote:
> > Add support for non-contiguous pipe display by allocating upper bound
> > pipes array for display. Set the pipe enum name to igt pipe for
> > enabled pipes in drm.
> >
> > Signed-off-by: Mohammed Khajapasha
> <mohammed.khajapasha at intel.com>
> > ---
> >  lib/igt_kms.c | 30 +++++++++++++++++++++++++-----  lib/igt_kms.h | 13
> > ++++++++-----
> >  2 files changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index afef5939..2b1cf878
> > 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -1921,10 +1921,26 @@ void igt_display_require(igt_display_t
> *display, int drm_fd)
> >  	 * We cache the number of pipes, that number is a physical limit of
> the
> >  	 * hardware and cannot change of time (for now, at least).
> >  	 */
> > -	display->n_pipes = resources->count_crtcs;
> > -	display->pipes = calloc(sizeof(igt_pipe_t), display->n_pipes);
> > +	display->n_pipes = IGT_MAX_PIPES;
> > +	display->pipes = calloc(sizeof(igt_pipe_t), IGT_MAX_PIPES);
> >  	igt_assert_f(display->pipes, "Failed to allocate memory for %d
> > pipes\n", display->n_pipes);
> >
> > +	for(i = 0; i < resources->count_crtcs; i++) {
> > +		igt_pipe_t *pipe;
> > +		struct drm_i915_get_pipe_from_crtc_id get_pipe;
> > +
> > +		/* Get right pipe enum from kernel for a pipe */
> > +		get_pipe.pipe = 0;
> > +		get_pipe.crtc_id =  resources->crtcs[i];
> > +		do_ioctl(display->drm_fd,
> > +				DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID,
> &get_pipe);
> > +
> > +		pipe = &display->pipes[get_pipe.pipe];
> > +		pipe->pipe = get_pipe.pipe;
> > +		pipe->enabled = true;
> > +		pipe->crtc_id = resources->crtcs[i];
> > +	}
> > +
> 
> You need to eventually make sure to only do this dancing for i915 devices.
> Might as well get that done early. For non-i915 devices you need to make
> pipe->pipe equal to i directly, and so on.
> 
> You also need to store i in the pipe objects. possible_crtcs from a drm
> plane refers to the value of i, not the pipe enum. That also needs to be
> handled correctly futher down.
> 
> 
> >  	drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
> 1);
> >  	if (drmSetClientCap(drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) == 0)
> >  		display->is_atomic = 1;
> > @@ -1959,9 +1975,7 @@ void igt_display_require(igt_display_t *display,
> int drm_fd)
> >  		int j, type;
> >  		uint8_t last_plane = 0, n_planes = 0;
> >
> > -		pipe->crtc_id = resources->crtcs[i];
> >  		pipe->display = display;
> > -		pipe->pipe = i;
> >  		pipe->plane_cursor = -1;
> >  		pipe->plane_primary = -1;
> >  		pipe->planes = NULL;
> > @@ -2409,12 +2423,18 @@ static bool
> > output_is_internal_panel(igt_output_t *output)
> >
> >  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;
> > +	unsigned full_pipe_mask, assigned_pipes = 0;
> >  	igt_output_t *output;
> >  	int i, j;
> >
> >  	memset(chosen_outputs, 0, sizeof(*chosen_outputs) *
> > display->n_pipes);
> >
> > +	for( i = 0; i < display->n_pipes; i++) {
> > +		igt_pipe_t *pipe = &display->pipes[i];
> > +		if (pipe->enabled)
> > +			full_pipe_mask |= (1 << pipe->pipe);
> 
> full_pipe_mask operates on config->valid_crtc_idx_mask later in this
> function, so it's one of those that needs to operate on the index (i), not the
> pipe->pipe value.

With PIPE_A & PIPE_C configuration, if we use index(i) rather than pipe->pipe the full_pipe_mask will be 0x11
But from kernel the encoder->possible_crtcs can be set from individual encoder's pipe_mask
Snippet
/*
for_each_intel_crtc(dev, crtc) 
{
	if (encoder->pipe_mask & 
		BIT(crtc->pipe)) possible_crtcs |= drm_crtc_mask(&crtc->base);
}

and pipe_mask for invidual encoder can be set by 

intel_encoder->pipe_mask = BIT(PIPE_A) | BIT(PIPE_C) --> which is 0x0101

*/

So with PIPE_A & PIPE_C, the output connected to PIPE_C can be miss while populating outputs for pipes.
--
Khajapasha
> 
> 
> --
> Petri Latvala