Re: [Intel-gfx] [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 
---

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 



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: [Intel-gfx] [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 
---

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 


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

[Intel-gfx] [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 
---

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