All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak
@ 2017-02-20 11:02 Brian Starkey
  2017-02-20 11:02 ` [PATCH i-g-t v2 2/5] lib/igt_kms: Neaten up pipe->planes[] assignment Brian Starkey
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Brian Starkey @ 2017-02-20 11:02 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH i-g-t v2 2/5] lib/igt_kms: Neaten up pipe->planes[] assignment
  2017-02-20 11:02 [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
@ 2017-02-20 11:02 ` Brian Starkey
  2017-02-20 11:02 ` [PATCH i-g-t v2 3/5] lib/igt_kms: Fix possible out-of-bounds access Brian Starkey
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Brian Starkey @ 2017-02-20 11:02 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH i-g-t v2 3/5] lib/igt_kms: Fix possible out-of-bounds access
  2017-02-20 11:02 [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
  2017-02-20 11:02 ` [PATCH i-g-t v2 2/5] lib/igt_kms: Neaten up pipe->planes[] assignment Brian Starkey
@ 2017-02-20 11:02 ` Brian Starkey
  2017-02-20 11:02 ` [PATCH i-g-t v2 4/5] lib/igt_kms: Fix memory corruption when there's no cursor plane Brian Starkey
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Brian Starkey @ 2017-02-20 11:02 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH i-g-t v2 4/5] lib/igt_kms: Fix memory corruption when there's no cursor plane
  2017-02-20 11:02 [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
  2017-02-20 11:02 ` [PATCH i-g-t v2 2/5] lib/igt_kms: Neaten up pipe->planes[] assignment Brian Starkey
  2017-02-20 11:02 ` [PATCH i-g-t v2 3/5] lib/igt_kms: Fix possible out-of-bounds access Brian Starkey
@ 2017-02-20 11:02 ` Brian Starkey
  2017-02-20 11:02 ` [PATCH i-g-t v2 5/5] lib/igt_kms: Remove redundant cursor code Brian Starkey
  2017-02-21 10:12 ` [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
  4 siblings, 0 replies; 6+ messages in thread
From: Brian Starkey @ 2017-02-20 11:02 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH i-g-t v2 5/5] lib/igt_kms: Remove redundant cursor code
  2017-02-20 11:02 [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
                   ` (2 preceding siblings ...)
  2017-02-20 11:02 ` [PATCH i-g-t v2 4/5] lib/igt_kms: Fix memory corruption when there's no cursor plane Brian Starkey
@ 2017-02-20 11:02 ` Brian Starkey
  2017-02-21 10:12 ` [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
  4 siblings, 0 replies; 6+ messages in thread
From: Brian Starkey @ 2017-02-20 11:02 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak
  2017-02-20 11:02 [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
                   ` (3 preceding siblings ...)
  2017-02-20 11:02 ` [PATCH i-g-t v2 5/5] lib/igt_kms: Remove redundant cursor code Brian Starkey
@ 2017-02-21 10:12 ` Brian Starkey
  4 siblings, 0 replies; 6+ messages in thread
From: Brian Starkey @ 2017-02-21 10:12 UTC (permalink / raw)
  To: intel-gfx

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-21 10:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-20 11:02 [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey
2017-02-20 11:02 ` [PATCH i-g-t v2 2/5] lib/igt_kms: Neaten up pipe->planes[] assignment Brian Starkey
2017-02-20 11:02 ` [PATCH i-g-t v2 3/5] lib/igt_kms: Fix possible out-of-bounds access Brian Starkey
2017-02-20 11:02 ` [PATCH i-g-t v2 4/5] lib/igt_kms: Fix memory corruption when there's no cursor plane Brian Starkey
2017-02-20 11:02 ` [PATCH i-g-t v2 5/5] lib/igt_kms: Remove redundant cursor code Brian Starkey
2017-02-21 10:12 ` [PATCH i-g-t v2 1/5] lib/igt_kms: Fix drm_plane leak Brian Starkey

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.