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

[PATCH i-g-t 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>
---

Hi,

This series cleans up igt_display_init a bit.

 - Fixes a memory leak.
 - Fixes out-of-bounds array access on cards with no primary plane.
 - Fixes memory corruption/crashes on cards with no cursor plane.
 - Cleans up some left-over stuff which wasn't really relevant after
   the dynamic planes change.

There's one detail (patch 4) I'm not really sure about - the dynamic
planes stuff means that the "drm_plane-less" cursor plane doesn't
work/make sense anymore, as it will never be found by
igt_pipe_get_plane_type(). I couldn't find any tests which seemed to
rely on having that cursor plane present, but I don't have any
hardware with a cursor to test on.

igt_display_init() could be simplified a bit further by not putting
the primary/cursor planes at the start/end respectively, but at least
kms_cursor_legacy appears to rely on a non-cursor plane being index 0,
so I've left it as-is for now.

I've given this a cursory test on Mali-DP, but if anyone is able to
test it more thoroughly on Intel HW that might be a good idea.

-Brian

 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 f59af6e75348..4ca9145726e2 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1759,12 +1759,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 2/5] lib/igt_kms: Neaten up pipe->planes[] assignment

[Brian Starkey]

Remove a bunch of branches, functionally equivalent.

Signed-off-by: Brian Starkey <brian.starkey@arm.com>
---
 lib/igt_kms.c |   34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 4ca9145726e2..783c891aebf1 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1785,32 +1785,18 @@ 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++;
-				}
-				display->has_cursor_plane = true;
-				break;
-			default:
+
+			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;
+			} 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 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>
---
 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 783c891aebf1..45c90c71f301 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1820,9 +1820,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 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>
---
 lib/igt_kms.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 45c90c71f301..ef7bfd1a8108 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1837,12 +1837,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 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>
---
 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 ef7bfd1a8108..6fbe67139d98 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -1824,20 +1824,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 1/5] lib/igt_kms: Fix drm_plane leak

[Robert Foss]

On 2017-02-17 12:54 PM, 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>
> ---
>
> Hi,
>
> This series cleans up igt_display_init a bit.
>
>  - Fixes a memory leak.
>  - Fixes out-of-bounds array access on cards with no primary plane.
>  - Fixes memory corruption/crashes on cards with no cursor plane.
>  - Cleans up some left-over stuff which wasn't really relevant after
>    the dynamic planes change.
>
> There's one detail (patch 4) I'm not really sure about - the dynamic
> planes stuff means that the "drm_plane-less" cursor plane doesn't
> work/make sense anymore, as it will never be found by
> igt_pipe_get_plane_type(). I couldn't find any tests which seemed to
> rely on having that cursor plane present, but I don't have any
> hardware with a cursor to test on.
>
> igt_display_init() could be simplified a bit further by not putting
> the primary/cursor planes at the start/end respectively, but at least
> kms_cursor_legacy appears to rely on a non-cursor plane being index 0,
> so I've left it as-is for now.
>
> I've given this a cursory test on Mali-DP, but if anyone is able to
> test it more thoroughly on Intel HW that might be a good idea.

Regarding testing, I pestered tsa@freenode about running some tests on 
intel-ci for me while writing dyn_n_planes, and he was very helpful.

I used a severely shrunk tests/intel-ci/fast-feedback.testlist
that you can find here: https://hastebin.com/tajoxawewi.pl


On a separate note, this patch looks good to me:
Reviewed-by: Robert Foss <robert.foss@collabora.com>


Rob.

>
> -Brian
>
>  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 f59af6e75348..4ca9145726e2 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1759,12 +1759,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);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Robert Foss]

On 2017-02-17 12:54 PM, Brian Starkey wrote:
> Remove a bunch of branches, functionally equivalent.
>
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  lib/igt_kms.c |   34 ++++++++++------------------------
>  1 file changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 4ca9145726e2..783c891aebf1 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1785,32 +1785,18 @@ 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++;
> -				}
> -				display->has_cursor_plane = true;

The display->has_cursor_plane assignment does not seem to appear in the 
refactored code.

With that fixed this patch is:
Reviewed-by: Robert Foss <robert.foss@collabora.com>


Rob.

> -				break;
> -			default:
> +
> +			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;
> +			} else {
>  				plane = &pipe->planes[p];
>  				plane->index = p++;
> -				break;
>  			}
>
>  			igt_assert_f(plane->index < n_planes, "n_planes < plane->index failed\n");
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Robert Foss]

On 2017-02-17 12:54 PM, Brian Starkey wrote:
> 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.

Reviewed-by: Robert Foss <robert.foss@collabora.com>


Rob.

>
> Fixes: 36656239ef96 lib/igt_kms: Implement dynamic plane count support
> Signed-off-by: Brian Starkey <brian.starkey@arm.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 783c891aebf1..45c90c71f301 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1820,9 +1820,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) {
>  			/*
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Robert Foss]

On 2017-02-17 12:54 PM, Brian Starkey wrote:
> 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.

Nice catch!

Reviewed-by: Robert Foss <robert.foss@collabora.com>


Rob.

>
> Fixes: 36656239ef96 lib/igt_kms: Implement dynamic plane count support
> Signed-off-by: Brian Starkey <brian.starkey@arm.com>
> ---
>  lib/igt_kms.c |    6 ------
>  1 file changed, 6 deletions(-)
>
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 45c90c71f301..ef7bfd1a8108 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1837,12 +1837,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;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Robert Foss]

On 2017-02-17 12:54 PM, Brian Starkey wrote:
> 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.

I was tempted to make this change too, but didn't quite dare risking 
introducing unexpected behavior. But I think this is the desired behavior.


Reviewed-by: Robert Foss <robert.foss@collabora.com>

>
> Signed-off-by: Brian Starkey <brian.starkey@arm.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 ef7bfd1a8108..6fbe67139d98 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -1824,20 +1824,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;
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Brian Starkey]

On Sun, Feb 19, 2017 at 03:39:48PM -0500, Robert Foss wrote:
>
>
>On 2017-02-17 12:54 PM, 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>
>>---
>>
>>Hi,
>>
>>This series cleans up igt_display_init a bit.
>>
>> - Fixes a memory leak.
>> - Fixes out-of-bounds array access on cards with no primary plane.
>> - Fixes memory corruption/crashes on cards with no cursor plane.
>> - Cleans up some left-over stuff which wasn't really relevant after
>>   the dynamic planes change.
>>
>>There's one detail (patch 4) I'm not really sure about - the dynamic
>>planes stuff means that the "drm_plane-less" cursor plane doesn't
>>work/make sense anymore, as it will never be found by
>>igt_pipe_get_plane_type(). I couldn't find any tests which seemed to
>>rely on having that cursor plane present, but I don't have any
>>hardware with a cursor to test on.
>>
>>igt_display_init() could be simplified a bit further by not putting
>>the primary/cursor planes at the start/end respectively, but at least
>>kms_cursor_legacy appears to rely on a non-cursor plane being index 0,
>>so I've left it as-is for now.
>>
>>I've given this a cursory test on Mali-DP, but if anyone is able to
>>test it more thoroughly on Intel HW that might be a good idea.
>
>Regarding testing, I pestered tsa@freenode about running some tests 
>on intel-ci for me while writing dyn_n_planes, and he was very 
>helpful.
>
>I used a severely shrunk tests/intel-ci/fast-feedback.testlist
>that you can find here: https://hastebin.com/tajoxawewi.pl
>
>
>On a separate note, this patch looks good to me:
>Reviewed-by: Robert Foss <robert.foss@collabora.com>
>

Great, thanks for the review. I'll see about that testing and send a
v2.

-Brian

>
>Rob.
>
>>
>>-Brian
>>
>> 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 f59af6e75348..4ca9145726e2 100644
>>--- a/lib/igt_kms.c
>>+++ b/lib/igt_kms.c
>>@@ -1759,12 +1759,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);
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Brian Starkey]

On Sun, Feb 19, 2017 at 03:41:45PM -0500, Robert Foss wrote:
>
>
>On 2017-02-17 12:54 PM, Brian Starkey wrote:
>>Remove a bunch of branches, functionally equivalent.
>>
>>Signed-off-by: Brian Starkey <brian.starkey@arm.com>
>>---
>> lib/igt_kms.c |   34 ++++++++++------------------------
>> 1 file changed, 10 insertions(+), 24 deletions(-)
>>
>>diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>index 4ca9145726e2..783c891aebf1 100644
>>--- a/lib/igt_kms.c
>>+++ b/lib/igt_kms.c
>>@@ -1785,32 +1785,18 @@ 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++;
>>-				}
>>-				display->has_cursor_plane = true;
>
>The display->has_cursor_plane assignment does not seem to appear in 
>the refactored code.
>

doh! Thanks for spotting that. I'll fix in v2.

-Brian

>With that fixed this patch is:
>Reviewed-by: Robert Foss <robert.foss@collabora.com>
>
>
>Rob.
>
>>-				break;
>>-			default:
>>+
>>+			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;
>>+			} else {
>> 				plane = &pipe->planes[p];
>> 				plane->index = p++;
>>-				break;
>> 			}
>>
>> 			igt_assert_f(plane->index < n_planes, "n_planes < plane->index failed\n");
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx