[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 at intel.com
Sat Jun 4 08:59:03 UTC 2022
Hi,
> -----Original Message-----
> From: Jessica Zhang <quic_jesszhan at quicinc.com>
> Sent: perjantai 3. kesäkuuta 2022 21.38
> To: Saarinen, Jani <jani.saarinen at intel.com>; Latvala, Petri
> <petri.latvala at intel.com>
> Cc: igt-dev at lists.freedesktop.org; quic_aravindh at quicinc.com;
> robdclark at 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 6/3/2022 9:30 AM, Saarinen, Jani wrote:
> > 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...
>
> Hey Jani,
>
> Thanks for the heads up -- seems that my patch is hitting a NULL dereference
> case because the display resources aren't guaranteed to be initialized in the `out`
> tag.
>
> This patch should fix that issue:
> https://patchwork.freedesktop.org/series/104739/
Thanks. I see Ashutosh already merged and fixed at: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_6506/git-log-oneline.txt
>
> Thanks,
>
> Jessica Zhang
>
> >
> > Br,
> > Jani
> >
> >> -----Original Message-----
> >> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of
> >> Petri Latvala
> >> Sent: maanantai 23. toukokuuta 2022 15.36
> >> To: Jessica Zhang <quic_jesszhan at quicinc.com>
> >> Cc: igt-dev at lists.freedesktop.org; quic_aravindh at quicinc.com;
> >> robdclark at 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/d
> >>> rm
> >>> /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 at chromium.org>
> >>> Signed-off-by: Jessica Zhang <quic_jesszhan at 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 at intel.com>
More information about the igt-dev
mailing list