[igt-dev] [PATCH i-g-t] lib/igt_kms: Fix commits for planes with multiple possible CRTCs
Daniel Vetter
daniel at ffwll.ch
Fri Feb 22 14:51:04 UTC 2019
On Thu, Feb 21, 2019 at 04:23:36PM -0500, Nicholas Kazlauskas wrote:
> An igt_plane_t is defined per igt_pipe_t. It is treated as its
> own independent resource but DRM planes can be exposed to being used on
> a number of possible CRTCs - making it a shared resource.
>
> In IGT planes with multiple possible CRTCs are added to the plane list
> for each pipe that the plane supports. The internal state remains
> independent in IGT so when the same plane is modified for multiple
> pipes in a single commit the last pipe to modify it is the one whose
> state gets fully applied.
>
> This situation happens fairly often in practice - resetting the display
> at the start of the test before a commit will reset the CRTC ID and FB
> ID for each plane.
>
> For an example, consider the
> igt at kms_plane_alpha_blend@pipe-a-constant-alpha-max test.
>
> This test will fail for any overlay plane exposed for multiple CRTCs.
>
> The test tries to set a framebuffer for pipe A but has all the other
> pipes reset the plane state in the same commit. If there are multiple
> pipes on the hardware then the last pipe will be the one to set all the
> plane properties. The driver will receive a commit with no overlay
> plane enabled since the last pipe set CRTC ID and FB ID to 0, disabling
> the plane. The reference CRC capture will be incorrect since not all
> the planes have been enabled and the subsequent CRC captures will
> not match, failing the test.
>
> The simplest (but hacky) fix to this problem is to only set the
> properties for the plane for the pipe that last had it bound.
>
> This patch introduces a global plane list on igt_display_t that keeps
> track of the pipe that pipe that last called igt_plane_set_fb. The
> properties for the plane will only be applied from that single pipe
> when commiting the state to DRM.
>
> No behavioral changes should be introduced by this patch for hardware
> whose planes are only ever exposed one CRTC.
>
> It would likely be best to eventually modify the igt_pipe_t plane list
> to be a list of pointers to planes instead (igt_plane_t**)
> instead of being the actual plane objects, but that can come later.
>
> Many areas of the code like to make use of the backpointer to the pipe
> on the plane which makes refactoring the code in that manner a little
> trickier.
Thanks a lot for tackling this intel-ism in igt, which indeed is totally
not how kms works (outside of recent i915 hw).
I think your plan to first build up a global plane list which aliases to
the current per-pipe planes as a first step is sound.
The next step we can probably do gradually, i.e. add a new
igt_pipe->plane_res or so which is the list of pointers for the global
plane_res array. And then step-by-step fade out usage of igt_pipe_planes.
Needs a bit of book-keeping for the transition, but should be doable.
>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> ---
> lib/igt_kms.c | 76 +++++++++++++++++++++++++++++++--------------------
> lib/igt_kms.h | 10 +++++++
> 2 files changed, 57 insertions(+), 29 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 080f90ae..f133a5cd 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1905,6 +1905,20 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> plane_resources = drmModeGetPlaneResources(display->drm_fd);
> igt_assert(plane_resources);
>
> + display->n_planes = plane_resources->count_planes;
> + display->planes = calloc(sizeof(igt_plane_res_t), display->n_planes);
> + igt_assert_f(display->planes, "Failed to allocate memory for %d planes\n", display->n_planes);
> +
> + for (i = 0; i < plane_resources->count_planes; ++i) {
> + igt_plane_res_t *plane = &display->planes[i];
> + uint32_t id = plane_resources->planes[i];
> +
> + plane->drm_plane = drmModeGetPlane(display->drm_fd, id);
> + igt_assert(plane->drm_plane);
> +
> + plane->type = get_drm_plane_type(display->drm_fd, id);
> + }
> +
> for_each_pipe(display, i) {
> igt_pipe_t *pipe = &display->pipes[i];
> igt_plane_t *plane;
> @@ -1922,17 +1936,12 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> igt_fill_pipe_props(display, pipe, IGT_NUM_CRTC_PROPS, igt_crtc_prop_names);
>
> /* count number of valid planes */
> - for (j = 0; j < plane_resources->count_planes; j++) {
> - drmModePlane *drm_plane;
> -
> - drm_plane = drmModeGetPlane(display->drm_fd,
> - plane_resources->planes[j]);
> + for (j = 0; j < display->n_planes; j++) {
> + drmModePlane *drm_plane = display->planes[j].drm_plane;
> igt_assert(drm_plane);
>
> if (drm_plane->possible_crtcs & (1 << i))
> n_planes++;
> -
> - drmModeFreePlane(drm_plane);
> }
>
> igt_assert_lt(0, n_planes);
> @@ -1941,20 +1950,14 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> last_plane = n_planes - 1;
>
> /* add the planes that can be used with that pipe */
> - for (j = 0; j < plane_resources->count_planes; j++) {
> - drmModePlane *drm_plane;
> -
> - drm_plane = drmModeGetPlane(display->drm_fd,
> - plane_resources->planes[j]);
> - igt_assert(drm_plane);
> + for (j = 0; j < display->n_planes; j++) {
> + igt_plane_res_t *res = &display->planes[j];
> + drmModePlane *drm_plane = res->drm_plane;
>
> - if (!(drm_plane->possible_crtcs & (1 << i))) {
> - drmModeFreePlane(drm_plane);
> + if (!(drm_plane->possible_crtcs & (1 << i)))
> continue;
> - }
>
> - type = get_drm_plane_type(display->drm_fd,
> - plane_resources->planes[j]);
> + type = res->type;
>
> if (type == DRM_PLANE_TYPE_PRIMARY && pipe->plane_primary == -1) {
> plane = &pipe->planes[0];
> @@ -1975,6 +1978,10 @@ void igt_display_require(igt_display_t *display, int drm_fd)
> plane->pipe = pipe;
> plane->drm_plane = drm_plane;
> plane->values[IGT_PLANE_IN_FENCE_FD] = ~0ULL;
> + plane->res = res;
> +
> + if (!res->state)
> + res->state = plane;
>
> igt_fill_plane_props(display, plane, IGT_NUM_PLANE_PROPS, igt_plane_prop_names);
>
> @@ -2127,17 +2134,6 @@ igt_output_t *igt_output_from_connector(igt_display_t *display,
>
> static void igt_pipe_fini(igt_pipe_t *pipe)
> {
> - int i;
> -
> - for (i = 0; i < pipe->n_planes; i++) {
> - igt_plane_t *plane = &pipe->planes[i];
> -
> - if (plane->drm_plane) {
> - drmModeFreePlane(plane->drm_plane);
> - plane->drm_plane = NULL;
> - }
> - }
> -
> free(pipe->planes);
> pipe->planes = NULL;
>
> @@ -2163,6 +2159,15 @@ void igt_display_fini(igt_display_t *display)
> {
> int i;
>
> + for (i = 0; i < display->n_planes; ++i) {
> + igt_plane_res_t *plane = &display->planes[i];
> +
> + if (plane->drm_plane) {
> + drmModeFreePlane(plane->drm_plane);
> + plane->drm_plane = NULL;
> + }
> + }
> +
> for (i = 0; i < display->n_pipes; i++)
> igt_pipe_fini(&display->pipes[i]);
>
> @@ -2172,6 +2177,8 @@ void igt_display_fini(igt_display_t *display)
> display->outputs = NULL;
> free(display->pipes);
> display->pipes = NULL;
> + free(display->planes);
> + display->planes = NULL;
> }
>
> static void igt_display_refresh(igt_display_t *display)
> @@ -2789,6 +2796,10 @@ static int igt_pipe_commit(igt_pipe_t *pipe,
> for (i = 0; i < pipe->n_planes; i++) {
> igt_plane_t *plane = &pipe->planes[i];
>
> + /* skip planes that are handled by another pipe */
> + if (plane->res->state != plane)
> + continue;
> +
> ret = igt_plane_commit(plane, pipe, s, fail_on_error);
> CHECK_RETURN(ret, fail_on_error);
> }
> @@ -3177,6 +3188,10 @@ static int igt_atomic_commit(igt_display_t *display, uint32_t flags, void *user_
> igt_atomic_prepare_crtc_commit(pipe_obj, req);
>
> for_each_plane_on_pipe(display, pipe, plane) {
> + /* skip planes that are handled by another pipe */
> + if (plane->res->state != plane)
> + continue;
> +
> if (plane->changed)
> igt_atomic_prepare_plane_commit(plane, pipe_obj, req);
> }
> @@ -3709,6 +3724,9 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
> if (igt_plane_has_prop(plane, IGT_PLANE_COLOR_RANGE))
> igt_plane_set_prop_enum(plane, IGT_PLANE_COLOR_RANGE,
> igt_color_range_to_str(fb->color_range));
> +
> + /* hack to prioritize plane on crtc it was last bound to */
> + plane->res->state = plane;
Hm, I'd creata a new igt_plane_set_pipe and put this in there. That would
give you a nice place to update all the other transition pointers we might
eventually need. Plus would allow us to gradually switch tests over to the
new world, but maybe better to keep that for the next step.
> } else {
> igt_plane_set_size(plane, 0, 0);
>
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 679d4e84..32d840ce 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -285,6 +285,7 @@ extern const char * const igt_plane_prop_names[];
>
> typedef struct igt_display igt_display_t;
> typedef struct igt_pipe igt_pipe_t;
> +typedef struct igt_plane_res igt_plane_res_t;
> typedef uint32_t igt_fixed_t; /* 16.16 fixed point */
>
> typedef enum {
> @@ -303,6 +304,7 @@ typedef enum {
> typedef struct {
> /*< private >*/
> igt_pipe_t *pipe;
> + igt_plane_res_t *res;
> int index;
> /* capabilities */
> int type;
> @@ -332,6 +334,12 @@ typedef struct {
> int format_mod_count;
> } igt_plane_t;
>
> +typedef struct igt_plane_res {
> + igt_plane_t *state;
> + drmModePlane *drm_plane;
> + int type;
> +} igt_plane_res_t;
Why not just reuse igt_plane_t for this? This means some duplication and
syncing (which you could do in the igt_plane_set_pipe for the transition),
but I think cleaner path forward. And easier to transition tests I think.
-Daniel
> +
> struct igt_pipe {
> igt_display_t *display;
> enum pipe pipe;
> @@ -372,8 +380,10 @@ struct igt_display {
> int drm_fd;
> int log_shift;
> int n_pipes;
> + int n_planes;
> int n_outputs;
> igt_output_t *outputs;
> + igt_plane_res_t *planes;
> igt_pipe_t *pipes;
> bool has_cursor_plane;
> bool is_atomic;
> --
> 2.17.1
>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the igt-dev
mailing list