[PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak

[PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak

[Brian Starkey]

In the loop looking for planes on a pipe, we always want to free up
the drm_plane afterwards.

Fixes: 36656239ef96 lib/igt_kms: Implement dynamic plane count support
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index d6b7d677ed8d..9e59e35f2e2f 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1613,12 +1613,10 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 						    plane_resources->planes[j]);
 			igt_assert(drm_plane);
 
-			if (!(drm_plane->possible_crtcs & (1 << i))) {
-				drmModeFreePlane(drm_plane);
-				continue;
-			}
+			if (drm_plane->possible_crtcs & (1 << i))
+				n_planes++;
 
-			n_planes++;
+			drmModeFreePlane(drm_plane);
 		}
 
 		igt_assert_lte(0, n_planes);
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 2/5] lib/igt_kms: Neaten up pipe->planes[] assignment

[Brian Starkey]

Remove a bunch of branches, functionally equivalent.

Changes since v1:
 - Added back display->has_cursor_plane assignment

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c |   33 ++++++++++-----------------------
 1 file changed, 10 insertions(+), 23 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 9e59e35f2e2f..585aad7aafb3 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1639,32 +1639,19 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 
 			type = get_drm_plane_type(display->drm_fd,
 						  plane_resources->planes[j]);
-			switch (type) {
-			case DRM_PLANE_TYPE_PRIMARY:
-				if (pipe->plane_primary == -1) {
-					plane = &pipe->planes[0];
-					plane->index = 0;
-					pipe->plane_primary = 0;
-				} else {
-					plane = &pipe->planes[p];
-					plane->index = p++;
-				}
-				break;
-			case DRM_PLANE_TYPE_CURSOR:
-				if (pipe->plane_cursor == -1) {
-					plane = &pipe->planes[last_plane];
-					plane->index = last_plane;
-					pipe->plane_cursor = last_plane;
-				} else {
-					plane = &pipe->planes[p];
-					plane->index = p++;
-				}
+
+			if (type == DRM_PLANE_TYPE_PRIMARY && pipe->plane_primary == -1) {
+				plane = &pipe->planes[0];
+				plane->index = 0;
+				pipe->plane_primary = 0;
+			} 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;
-				break;
-			default:
+			} else {
 				plane = &pipe->planes[p];
 				plane->index = p++;
-				break;
 			}
 
 			igt_assert_f(plane->index < n_planes, "n_planes < plane->index failed\n");
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 3/5] lib/igt_kms: Fix possible out-of-bounds access

[Brian Starkey]

If there's no primary plane, pipe->plane_primary == -1, and the assert
meant to check that it's valid will access pipe->planes[-1].

Fix that to check that pipe->plane_primary has been set instead.

Fixes: 36656239ef96 lib/igt_kms: Implement dynamic plane count support
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 585aad7aafb3..2073a6acfa7b 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1675,9 +1675,9 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 
 		/*
 		 * At the bare minimum, we should expect to have a primary
-		 * plane
+		 * plane, and it must be in slot 0.
 		 */
-		igt_assert(pipe->planes[pipe->plane_primary].drm_plane);
+		igt_assert_eq(pipe->plane_primary, 0);
 
 		if (display->has_cursor_plane) {
 			/*
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 4/5] lib/igt_kms: Fix memory corruption when there's no cursor plane

[Brian Starkey]

The dynamic plane support means that if there's no cursor plane, then
there is no space in the pipe->planes array for it, and thus assigning
a "drm_plane-less" plane is out-of-bounds and leads to heap corruption
and later crashes.

The "drm_plane-less" cursor plane isn't included in n_planes anyway,
which means there's no way to ever access it/know that it's there - so
just remove it entirely.

Fixes: 36656239ef96 lib/igt_kms: Implement dynamic plane count support
Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 2073a6acfa7b..60c4c260bf2d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1692,12 +1692,6 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 				memset(&pipe->planes[last_plane], 0,
 				       sizeof *plane);
 			}
-		} else {
-			/* Add drm_plane-less cursor */
-			plane = &pipe->planes[p];
-			plane->pipe = pipe;
-			plane->index = p;
-			plane->type = DRM_PLANE_TYPE_CURSOR;
 		}
 
 		pipe->n_planes = n_planes;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v2 5/5] lib/igt_kms: Remove redundant cursor code

[Brian Starkey]

The dynamic plane support means that there should never be gaps in the
pipe->planes array. This means we should never need to move the cursor
plane from the last slot to another.

Remove the unnecessary code, and add an assert that makes sure nothing
strange happened that broke the assignment logic.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
Reviewed-by: Robert Foss <robert.foss@collabora.com>
---
 lib/igt_kms.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 60c4c260bf2d..8751c97f7a06 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1679,20 +1679,11 @@ void igt_display_init(igt_display_t *display, int drm_fd)
 		 */
 		igt_assert_eq(pipe->plane_primary, 0);
 
-		if (display->has_cursor_plane) {
-			/*
-			 * Cursor was put in the last slot.  If we have 0 or
-			 * only 1 sprite, that's the wrong slot and we need to
-			 * move it down.
-			 */
-			if (p != last_plane) {
-				pipe->planes[p] =
-					pipe->planes[last_plane];
-				pipe->planes[p].index = p;
-				memset(&pipe->planes[last_plane], 0,
-				       sizeof *plane);
-			}
-		}
+		/*
+		 * There should be no gaps. If there is, something happened
+		 * which we can't handle (e.g. all planes are cursors).
+		 */
+		igt_assert_eq(p, last_plane);
 
 		pipe->n_planes = n_planes;
 
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak

[Brian Starkey]

Hi,

tsa@freenode was kind enough to run some piglit testing on this.

I gave the following test list:

igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic
igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy
igt@kms_plane_multiple@legacy-pipe-A-tiling-none
igt@kms_plane_multiple@atomic-pipe-A-tiling-none
igt@kms_plane_multiple@legacy-pipe-A-tiling-x
igt@kms_plane_multiple@atomic-pipe-A-tiling-x
igt@kms_chv_cursor_fail@pipe-A-128x128-bottom-edge
igt@kms_chv_cursor_fail@pipe-A-128x128-left-edge
igt@kms_chv_cursor_fail@pipe-A-128x128-right-edge
igt@kms_chv_cursor_fail@pipe-A-128x128-top-edge
igt@kms_ccs@bad-rotation-90
igt@kms_busy@flip-blt-A
igt@kms_mmap_write_crc

There were no changes to any test on 11 hosts (ILK to KBL).
(3 fails on bad-rotation-90 stays just as they were)

Thanks,
Brian

On Mon, Feb 20, 2017 at 11:02:44AM +0000, Brian Starkey wrote:
>In the loop looking for planes on a pipe, we always want to free up
>the drm_plane afterwards.
>
>Fixes: 36656239ef96 lib/igt_kms: Implement dynamic plane count support
>Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>Reviewed-by: Robert Foss <robert.foss@collabora.com>
>---
> lib/igt_kms.c |    8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>index d6b7d677ed8d..9e59e35f2e2f 100644
>--- a/lib/igt_kms.c
>+++ b/lib/igt_kms.c
>@@ -1613,12 +1613,10 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> 						    plane_resources->planes[j]);
> 			igt_assert(drm_plane);
>
>-			if (!(drm_plane->possible_crtcs & (1 << i))) {
>-				drmModeFreePlane(drm_plane);
>-				continue;
>-			}
>+			if (drm_plane->possible_crtcs & (1 << i))
>+				n_planes++;
>
>-			n_planes++;
>+			drmModeFreePlane(drm_plane);
> 		}
>
> 		igt_assert_lte(0, n_planes);
>-- 
>1.7.9.5
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx