Re: [igt-dev] [PATCH i-g-t v4] lib/igt_kms: Set pipe->plane_primary to driver-assigned primary plane when there are multiple possible primary planes

["Saarinen, Jani" <jani.saarinen@intel.com>]

Jessica, Petri
This is now seems to cause regressions 

See:
https://gitlab.freedesktop.org/drm/intel/issues/6133
https://gitlab.freedesktop.org/drm/intel/-/issues/6134
https://gitlab.freedesktop.org/drm/intel/-/issues/6135
https://gitlab.freedesktop.org/drm/intel/-/issues/6136
https://gitlab.freedesktop.org/drm/intel/-/issues/6137

First seen at https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6496/git-log-oneline.txt
58a5a2d5b lib/igt_kms: Set pipe->plane_primary to driver-assigned primary plane when there are multiple possible primary planes
5b17cb8ea i915/gem_exec_gttfill: Added test description for test case.
ab8bfe267 i915/gem_pipe_control_store_loop: added description for test case
0e5b5c505 intel_gpu_top: Don't show client header if no kernel support

Please revert asap or fix accordingly...

Br,
Jani

> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Petri Latvala
> Sent: maanantai 23. toukokuuta 2022 15.36
> To: Jessica Zhang <quic_jesszhan@quicinc.com>
> Cc: igt-dev@lists.freedesktop.org; quic_aravindh@quicinc.com;
> robdclark@chromium.org
> Subject: Re: [igt-dev] [PATCH i-g-t v4] lib/igt_kms: Set pipe->plane_primary to
> driver-assigned primary plane when there are multiple possible primary planes
> 
> On Mon, May 16, 2022 at 06:37:12PM -0700, Jessica Zhang wrote:
> > Currently, IGT populates pipe->planes using possible_crtcs and assumes
> > that the first primary plane it encounters is the one being actively
> > used by the pipe.
> >
> > This is not always true in cases where there are multiple possible
> > primary planes. This will cause problems whenever IGT calls
> > drmModePageFlip since drmModePageFlip will use the primary plane
> > assigned to the pipe by the driver to perform the page flip [1]. So a
> > mismatch between the primary plane used by IGT and the driver can
> > cause the page flip to fail.
> >
> > To fix this, let's implement a helper method to get the primary plane
> > that's being assigned to the pipe by the driver. We can then call it
> > during igt_display_require() and, if there's a mismatch between
> > pipe->plane_primary and the assigned primary plane's index, we can
> > pipe->swap
> > the unused primary plane with the driver-assigned primary plane
> >
> > [1]
> > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm
> > /drm_plane.c#L1234
> >
> > Changes since v1:
> > - Instead of swapping the pointers of the planes within the array, we
> >   can just change the value of pipe->plane_primary.
> >
> > Changes since v2:
> > - Reverted `if (type == DRM_PLANE_TYPE_PRIMARY)` conditional then added
> >   a nested if statement to increment num_primary_planes if plane type is
> >   primary.
> >
> > Changes since v3:
> > - Created helper method igt_pipe_has_valid_output and added a check for
> >   if pipe has valid output before getting the output of a pipe to get
> >   the assigned primary plane
> > - Reverted back to using igt_swap to swap the order of the primary
> >   planes within the pipe->planes array. Some kms_* tests use
> >   igt_output_get_plane(output, 0) to get the primary plane, so this will
> >   avoid regressions in those tests.
> > - Updated the plane->index of the primary planes that are being swapped.
> >
> > Suggested-by: Rob Clark <robdclark@chromium.org>
> > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> > ---
> >  lib/igt_kms.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++----
> >  lib/igt_kms.h |   1 +
> >  2 files changed, 103 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index
> > 7838ff283b62..a97e0b72d6cf 100644
> > --- a/lib/igt_kms.c
> > +++ b/lib/igt_kms.c
> > @@ -751,6 +751,52 @@ igt_fill_pipe_props(igt_display_t *display, igt_pipe_t
> *pipe,
> >  	drmModeFreeObjectProperties(props);
> >  }
> >
> > +static igt_plane_t *igt_get_assigned_primary(igt_output_t *output,
> > +igt_pipe_t *pipe) {
> > +	int drm_fd = output->display->drm_fd;
> > +	drmModeModeInfo *mode;
> > +	struct igt_fb fb;
> > +	igt_plane_t *plane = NULL;
> > +	uint32_t crtc_id;
> > +	int i;
> > +
> > +	mode = igt_output_get_mode(output);
> > +
> > +	igt_create_color_fb(drm_fd, mode->hdisplay, mode->vdisplay,
> > +						DRM_FORMAT_XRGB8888,
> > +						DRM_FORMAT_MOD_LINEAR,
> > +						1.0, 1.0, 1.0, &fb);
> > +
> > +	crtc_id = pipe->crtc_id;
> > +
> > +	/*
> > +	 * Do a legacy SETCRTC to start things off, so that we know that
> > +	 * the kernel will pick the correct primary plane and attach it
> > +	 * to the CRTC. This lets us handle the case that there are
> > +	 * multiple primary planes (one per CRTC), but which can *also*
> > +	 * be attached to other CRTCs
> > +	 */
> > +	igt_assert(drmModeSetCrtc(output->display->drm_fd, crtc_id, fb.fb_id,
> > +							  0, 0, &output->id, 1,
> mode) == 0);
> > +
> > +	for(i = 0; i < pipe->n_planes; i++) {
> > +		if (pipe->planes[i].type != DRM_PLANE_TYPE_PRIMARY)
> > +			continue;
> > +
> > +		if (igt_plane_get_prop(&pipe->planes[i], IGT_PLANE_CRTC_ID) !=
> crtc_id)
> > +			continue;
> > +
> > +		plane = &pipe->planes[i];
> > +		break;
> > +	}
> > +
> > +	/* Removing the FB will also shut down the display for us: */
> > +	igt_remove_fb(drm_fd, &fb);
> > +	igt_assert_f(plane, "Valid assigned primary plane for CRTC_ID %d not
> > +found.\n", crtc_id);
> > +
> > +	return plane;
> > +}
> > +
> >  /**
> >   * kmstest_pipe_name:
> >   * @pipe: display pipe
> > @@ -2150,6 +2196,18 @@ __get_crtc_mask_for_pipe(drmModeRes *resources,
> igt_pipe_t *pipe)
> >  	return (1 << offset);
> >  }
> >
> > +static bool igt_pipe_has_valid_output(igt_display_t *display, enum
> > +pipe pipe) {
> > +	igt_output_t *output;
> > +
> > +	igt_require_pipe(display, pipe);
> > +
> > +	for_each_valid_output_on_pipe(display, pipe, output)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  /**
> >   * igt_display_require:
> >   * @display: a pointer to an #igt_display_t structure @@ -2272,6
> > +2330,7 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> >  		pipe->plane_cursor = -1;
> >  		pipe->plane_primary = -1;
> >  		pipe->planes = NULL;
> > +		pipe->num_primary_planes = 0;
> >
> >  		igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS,
> > igt_crtc_prop_names);
> >
> > @@ -2306,12 +2365,20 @@ void igt_display_require(igt_display_t *display, int
> drm_fd)
> >  				plane = &pipe->planes[0];
> >  				plane->index = 0;
> >  				pipe->plane_primary = 0;
> > +				pipe->num_primary_planes++;
> >  			} else if (type == DRM_PLANE_TYPE_CURSOR && pipe-
> >plane_cursor == -1) {
> >  				plane = &pipe->planes[last_plane];
> >  				plane->index = last_plane;
> >  				pipe->plane_cursor = last_plane;
> >  				display->has_cursor_plane = true;
> >  			} else {
> > +				/*
> > +				 * Increment num_primary_planes for any extra
> > +				 * primary plane found.
> > +				 */
> > +				if (type == DRM_PLANE_TYPE_PRIMARY)
> > +					pipe->num_primary_planes++;
> > +
> >  				plane = &pipe->planes[p];
> >  				plane->index = p++;
> >  			}
> > @@ -2392,6 +2459,39 @@ void igt_display_require(igt_display_t
> > *display, int drm_fd)
> >  out:
> >  	LOG_UNINDENT(display);
> >
> > +	for_each_pipe(display, i) {
> > +		igt_pipe_t *pipe = &display->pipes[i];
> > +
> > +		if (!igt_pipe_has_valid_output(display, i))
> > +			continue;
> > +
> > +		igt_output_t *output = igt_get_single_output_for_pipe(display, i);
> > +
> > +		if (pipe->num_primary_planes > 1) {
> > +			igt_plane_t *primary = &pipe->planes[pipe-
> >plane_primary];
> > +			igt_plane_t *assigned_primary =
> igt_get_assigned_primary(output, pipe);
> > +			int assigned_primary_index = assigned_primary->index;
> > +
> > +			/*
> > +			 * If the driver-assigned primary plane isn't at the
> > +			 * pipe->plane_primary index, swap it with the plane
> > +			 * that's currently at the plane_primary index and
> > +			 * update plane->index accordingly.
> > +			 *
> > +			 * This way, we can preserve pipe->plane_primary as 0
> > +			 * so that tests that assume pipe->plane_primary is always
> 0
> > +			 * won't break.
> > +			 */
> > +			if (assigned_primary_index != pipe->plane_primary) {
> > +				assigned_primary->index = pipe->plane_primary;
> > +				primary->index = assigned_primary_index;
> > +
> > +				igt_swap(pipe->planes[assigned_primary_index],
> > +						pipe->planes[pipe-
> >plane_primary]);
> > +			}
> > +		}
> > +	}
> > +
> >  	if (display->n_pipes && display->n_outputs)
> >  		igt_enable_connectors(drm_fd);
> >  	else
> > @@ -2438,14 +2538,8 @@ void igt_display_require_output(igt_display_t
> *display)
> >   */
> >  void igt_display_require_output_on_pipe(igt_display_t *display, enum
> > pipe pipe)  {
> > -	igt_output_t *output;
> > -
> > -	igt_require_pipe(display, pipe);
> > -
> > -	for_each_valid_output_on_pipe(display, pipe, output)
> > -		return;
> > -
> > -	igt_skip("No valid connector found on pipe %s\n",
> kmstest_pipe_name(pipe));
> > +	if (!igt_pipe_has_valid_output(display, pipe))
> > +		igt_skip("No valid connector found on pipe %s\n",
> > +kmstest_pipe_name(pipe));
> >  }
> >
> >  /**
> > diff --git a/lib/igt_kms.h b/lib/igt_kms.h index
> > e9ecd21e9824..4949b7b39e18 100644
> > --- a/lib/igt_kms.h
> > +++ b/lib/igt_kms.h
> > @@ -383,6 +383,7 @@ struct igt_pipe {
> >  	bool enabled;
> >
> >  	int n_planes;
> > +	int num_primary_planes;
> >  	int plane_cursor;
> >  	int plane_primary;
> >  	igt_plane_t *planes;
> > --
> > 2.31.0
> >
> 
> 
> The setcrtc dance is now only done when multiple primary planes are
> detected, good.
> 
> Acked-by: Petri Latvala <petri.latvala@intel.com>