[PATCH i-g-t 0/2] minor improvements to the kms_cursor_crc doc and some comments cleanup

[Melissa Wen]

Hi,

I was studying the code of kms_cursor_crc test, and I just adjusted some comments
and added descriptions for subtests.

Melissa Wen (2):
lib/igt_fb: change comments with fd description
test/kms_cursor_crc: update subtests descriptions and some comments

lib/igt_fb.c | 6 +++---
tests/kms_cursor_crc.c | 38 ++++++++++++++++++++++++++++----------
2 files changed, 31 insertions(+), 13 deletions(-)

--
2.27.0

[Melissa Wen]

Generalize description of fd so as not restrict it to i915 driver

Signed-off-by: Melissa Wen <melis...@gmail.com>
---
lib/igt_fb.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index 5ed586e7..5a219c57 100644
--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -3548,7 +3548,7 @@ cairo_surface_t *igt_get_cairo_surface(int fd, struct igt_fb *fb)

/**
* igt_get_cairo_ctx:
- * @fd: open i915 drm file descriptor
+ * @fd: open drm file descriptor
* @fb: pointer to an #igt_fb structure
*
* This initializes a cairo surface for @fb and then allocates a drawing context
@@ -3578,7 +3578,7 @@ cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb)

/**
* igt_put_cairo_ctx:
- * @fd: open i915 drm file descriptor
+ * @fd: open drm file descriptor
* @fb: pointer to an #igt_fb structure
* @cr: the cairo context returned by igt_get_cairo_ctx.
*
@@ -3596,7 +3596,7 @@ void igt_put_cairo_ctx(int fd, struct igt_fb *fb, cairo_t *cr)

/**
* igt_remove_fb:
- * @fd: open i915 drm file descriptor
+ * @fd: open drm file descriptor
* @fb: pointer to an #igt_fb structure
*
* This function releases all resources allocated in igt_create_fb() for @fb.
--
2.27.0

[Melissa Wen]

Add descriptions for some subtests and detail a little more the comments in
test_cursor_alpha.

Signed-off-by: Melissa Wen <melis...@gmail.com>
---

tests/kms_cursor_crc.c | 38 ++++++++++++++++++++++++++++----------
1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index f105e295..4dba1471 100644
--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -31,12 +31,13 @@


IGT_TEST_DESCRIPTION(
- "Use the display CRC support to validate cursor plane functionality. "
- "The test will position the cursor plane either fully onscreen, "
- "partially onscreen, or fully offscreen, using either a fully opaque "
- "or fully transparent surface. In each case it then reads the PF CRC "
- "and compares it with the CRC value obtained when the cursor plane "
- "was disabled.");
+ "Use the display CRC support to validate cursor plane functionality. "\
+ "The test will position the cursor plane either fully onscreen, "\
+ "partially onscreen, or fully offscreen, using either a fully opaque "\
+ "or fully transparent surface. In each case, it enables the cursor plane "\
+ "and then reads the PF CRC (hardware test) and compares it with the CRC "\
+ "value obtained when the cursor plane was disabled and its drawing is "
+ "directly inserted on the PF by software.");

#ifndef DRM_CAP_CURSOR_WIDTH
#define DRM_CAP_CURSOR_WIDTH 0x8
@@ -485,7 +486,7 @@ static void test_cursor_alpha(data_t *data, double a)
int curw = data->curw;
int curh = data->curh;

- /*alpha cursor fb*/
+ /*Alpha cursor fb with white color*/
fb_id = igt_create_fb(data->drm_fd, curw, curh,
DRM_FORMAT_ARGB8888,
LOCAL_DRM_FORMAT_MOD_NONE,
@@ -495,22 +496,24 @@ static void test_cursor_alpha(data_t *data, double a)
igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
igt_put_cairo_ctx(data->drm_fd, &data->fb, cr);

- /*Hardware Test*/
+ /*Hardware Test - enable cursor and get PF CRC*/
cursor_enable(data);
igt_display_commit(display);
igt_wait_for_vblank(data->drm_fd, data->pipe);
igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
+
cursor_disable(data);
igt_remove_fb(data->drm_fd, &data->fb);

- /*Software Test*/
+ /*Software Test - render cursor in software, drawn it directly on PF*/
cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb[FRONTBUFFER]);
igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);
igt_put_cairo_ctx(data->drm_fd, &data->primary_fb[FRONTBUFFER], cr);
-
igt_display_commit(display);
igt_wait_for_vblank(data->drm_fd, data->pipe);
igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
+
+ /*Compare CRC from Hardware/Software tests*/
igt_assert_crc_equal(&crc, &ref_crc);

/*Clear Screen*/
@@ -688,13 +691,19 @@ static void run_tests_on_pipe(data_t *data, enum pipe pipe)
igt_require(data->output);
}

+ igt_describe("Create a maximum size cursor, then change the size in "\
+ "flight to smaller ones to see that the size is applied correctly.");
igt_subtest_f("pipe-%s-cursor-size-change", kmstest_pipe_name(pipe))
run_test(data, test_cursor_size,
data->cursor_max_w, data->cursor_max_h);

+ igt_describe("Validates the composition of a fully opaque cursor "\
+ "plane, i.e., alpha channel equal to 1.0.");
igt_subtest_f("pipe-%s-cursor-alpha-opaque", kmstest_pipe_name(pipe))
run_test(data, test_cursor_opaque, data->cursor_max_w, data->cursor_max_h);

+ igt_describe("Validates the composition of a fully transparent cursor "\
+ "plane, i.e., alpha channel equal to 0.0.");
igt_subtest_f("pipe-%s-cursor-alpha-transparent", kmstest_pipe_name(pipe))
run_test(data, test_cursor_transparent, data->cursor_max_w, data->cursor_max_h);

@@ -728,15 +737,24 @@ static void run_tests_on_pipe(data_t *data, enum pipe pipe)
}

/* Using created cursor FBs to test cursor support */
+ igt_describe("Check if a given-size cursor is well-positioned inside the screen.");
igt_subtest_f("pipe-%s-cursor-%dx%d-onscreen", kmstest_pipe_name(pipe), w, h)
run_test(data, test_crc_onscreen, w, h);
+
+ igt_describe("Check if a given-size cursor is well-positioned outside the screen.");
igt_subtest_f("pipe-%s-cursor-%dx%d-offscreen", kmstest_pipe_name(pipe), w, h)
run_test(data, test_crc_offscreen, w, h);
+
+ igt_describe("Check the smooth and pixel-by-pixel given-size cursor movements on"\
+ "horizontal, vertical and diagonal.");
igt_subtest_f("pipe-%s-cursor-%dx%d-sliding", kmstest_pipe_name(pipe), w, h)
run_test(data, test_crc_sliding, w, h);
+
+ igt_describe("Check random placement of a cursor with given size.");
igt_subtest_f("pipe-%s-cursor-%dx%d-random", kmstest_pipe_name(pipe), w, h)
run_test(data, test_crc_random, w, h);

+ igt_describe("Check the rapid update of given-size cursor movements.");
igt_subtest_f("pipe-%s-cursor-%dx%d-rapid-movement", kmstest_pipe_name(pipe), w, h) {
run_test(data, test_rapid_movement, w, h);
}
--
2.27.0

[Rodrigo Siqueira]

Hi,

LGTM

Reviewed-by: Rodrigo Siqueira <Rodrigo....@amd.com>

--
Rodrigo Siqueira
https://siqueira.tech

[Rodrigo Siqueira]

LGTM,

Reviewed-by: Rodrigo Siqueira <Rodrigo....@amd.com>

On 06/25, Melissa Wen wrote:

[Rodrigo Siqueira]

Hi Melissa,

First of all, thanks a lot for your effort to improve the documentation.

I noticed that for some weird reason the CI pointed out a failure in
this series. I looked at it, and I believe this is a false positive.

Hiler, Petri
Do you know if this failure is a false positive? Can I go ahead and
apply this change?

Thanks
Rodrigo Siqueira

On 06/25, Melissa Wen wrote:

> Hi,
>
> (I had sent this to the wrong list before)

[Arkadiusz Hiler]

On Wed, Jun 24, 2020 at 06:54:00AM -0300, Melissa Wen wrote:
> Hi,
>
> I was studying the code of kms_cursor_crc test, and I just adjusted some comments
> and added descriptions for subtests.
>
> Melissa Wen (2):
> lib/igt_fb: change comments with fd description
> test/kms_cursor_crc: update subtests descriptions and some comments

Seems like there's a conflict caused by your patch removing unused
parameters from igt_put_cairo_ctx().

Can you an send updated version and CC me on it?

In case of false positives please comment on the CI results with a short
explanation and CC Lakshmi <lakshminar...@intel.com>

Thanks for the cleanup!

--
Cheers,
Arek

[Melissa Wen]

Just some comments cleanup and adding descriptions for subtests.

v2:
- Rebase on master

Melissa Wen (2):
lib/igt_fb: change comments with fd description
test/kms_cursor_crc: update subtests descriptions and some comments

lib/igt_fb.c | 4 ++--
tests/kms_cursor_crc.c | 37 ++++++++++++++++++++++++++++---------
2 files changed, 30 insertions(+), 11 deletions(-)

--
2.27.0

[Melissa Wen]

Generalize description of fd so as not restrict it to i915 driver

Signed-off-by: Melissa Wen <melis...@gmail.com>
---

lib/igt_fb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_fb.c b/lib/igt_fb.c
index fd33907b..3864b7a1 100644

--- a/lib/igt_fb.c
+++ b/lib/igt_fb.c
@@ -3548,7 +3548,7 @@ cairo_surface_t *igt_get_cairo_surface(int fd, struct igt_fb *fb)

/**
* igt_get_cairo_ctx:
- * @fd: open i915 drm file descriptor
+ * @fd: open drm file descriptor
* @fb: pointer to an #igt_fb structure
*
* This initializes a cairo surface for @fb and then allocates a drawing context

@@ -3594,7 +3594,7 @@ void igt_put_cairo_ctx(cairo_t *cr)

[Melissa Wen]

Add descriptions for some subtests and detail a little more the comments in
test_cursor_alpha.

Signed-off-by: Melissa Wen <melis...@gmail.com>
---

tests/kms_cursor_crc.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
index 7e6b24cc..a6faa785 100644

--- a/tests/kms_cursor_crc.c
+++ b/tests/kms_cursor_crc.c
@@ -31,12 +31,13 @@


IGT_TEST_DESCRIPTION(
- "Use the display CRC support to validate cursor plane functionality. "
- "The test will position the cursor plane either fully onscreen, "
- "partially onscreen, or fully offscreen, using either a fully opaque "
- "or fully transparent surface. In each case it then reads the PF CRC "
- "and compares it with the CRC value obtained when the cursor plane "
- "was disabled.");
+ "Use the display CRC support to validate cursor plane functionality. "\
+ "The test will position the cursor plane either fully onscreen, "\
+ "partially onscreen, or fully offscreen, using either a fully opaque "\
+ "or fully transparent surface. In each case, it enables the cursor plane "\
+ "and then reads the PF CRC (hardware test) and compares it with the CRC "\
+ "value obtained when the cursor plane was disabled and its drawing is "
+ "directly inserted on the PF by software.");

#ifndef DRM_CAP_CURSOR_WIDTH
#define DRM_CAP_CURSOR_WIDTH 0x8

@@ -483,7 +484,7 @@ static void test_cursor_alpha(data_t *data, double a)

int curw = data->curw;
int curh = data->curh;

- /*alpha cursor fb*/
+ /*Alpha cursor fb with white color*/
fb_id = igt_create_fb(data->drm_fd, curw, curh,
DRM_FORMAT_ARGB8888,
LOCAL_DRM_FORMAT_MOD_NONE,

@@ -493,15 +494,16 @@ static void test_cursor_alpha(data_t *data, double a)

igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);

igt_put_cairo_ctx(cr);

- /*Hardware Test*/
+ /*Hardware Test - enable cursor and get PF CRC*/
cursor_enable(data);
igt_display_commit(display);
igt_wait_for_vblank(data->drm_fd, data->pipe);
igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &crc);
+
cursor_disable(data);
igt_remove_fb(data->drm_fd, &data->fb);

- /*Software Test*/
+ /*Software Test - render cursor in software, drawn it directly on PF*/
cr = igt_get_cairo_ctx(data->drm_fd, &data->primary_fb[FRONTBUFFER]);
igt_paint_color_alpha(cr, 0, 0, curw, curh, 1.0, 1.0, 1.0, a);

igt_put_cairo_ctx(cr);
@@ -509,6 +511,8 @@ static void test_cursor_alpha(data_t *data, double a)

igt_display_commit(display);
igt_wait_for_vblank(data->drm_fd, data->pipe);
igt_pipe_crc_get_current(data->drm_fd, pipe_crc, &ref_crc);
+
+ /*Compare CRC from Hardware/Software tests*/
igt_assert_crc_equal(&crc, &ref_crc);

/*Clear Screen*/

@@ -686,13 +690,19 @@ static void run_tests_on_pipe(data_t *data, enum pipe pipe)

igt_require(data->output);
}

+ igt_describe("Create a maximum size cursor, then change the size in "\
+ "flight to smaller ones to see that the size is applied correctly.");
igt_subtest_f("pipe-%s-cursor-size-change", kmstest_pipe_name(pipe))
run_test(data, test_cursor_size,
data->cursor_max_w, data->cursor_max_h);

+ igt_describe("Validates the composition of a fully opaque cursor "\
+ "plane, i.e., alpha channel equal to 1.0.");
igt_subtest_f("pipe-%s-cursor-alpha-opaque", kmstest_pipe_name(pipe))
run_test(data, test_cursor_opaque, data->cursor_max_w, data->cursor_max_h);

+ igt_describe("Validates the composition of a fully transparent cursor "\
+ "plane, i.e., alpha channel equal to 0.0.");
igt_subtest_f("pipe-%s-cursor-alpha-transparent", kmstest_pipe_name(pipe))
run_test(data, test_cursor_transparent, data->cursor_max_w, data->cursor_max_h);

@@ -726,15 +736,24 @@ static void run_tests_on_pipe(data_t *data, enum pipe pipe)

[Melissa Wen]

On 07/15, Arkadiusz Hiler wrote:
> On Wed, Jun 24, 2020 at 06:54:00AM -0300, Melissa Wen wrote:
> > Hi,
> >
> > I was studying the code of kms_cursor_crc test, and I just adjusted some comments
> > and added descriptions for subtests.
> >
> > Melissa Wen (2):
> > lib/igt_fb: change comments with fd description
> > test/kms_cursor_crc: update subtests descriptions and some comments
>
> Seems like there's a conflict caused by your patch removing unused
> parameters from igt_put_cairo_ctx().
>
> Can you an send updated version and CC me on it?

Hi,

I just sent a v2.

Thanks,

Melissa