On 8.6.2021 10.01, Modem, Bhanuprakash wrote:

quoted

From: Intel-gfx <redacted> On Behalf Of Vidya
Srinivas
Sent: Friday, May 28, 2021 9:57 AM
To: intel-gfx@lists.freedesktop.org; igt-dev@lists.freedesktop.org
Cc: markyacoub@chromium.org; Lin, Charlton <redacted>
Subject: [Intel-gfx] [PATCH i-g-t] [RFC] tests/kms_big_fb: Wait for vblank
before collecting CRC

Without wait for vblank, CRC mismatch is seen
between big and small CRC on few Gen11 systems.

Signed-off-by: Vidya Srinivas <redacted>
---
  tests/kms_big_fb.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/kms_big_fb.c b/tests/kms_big_fb.c
index b35727a09bd0..f90363c3beb2 100644
--- a/tests/kms_big_fb.c
+++ b/tests/kms_big_fb.c

@@ -254,6 +254,7 @@ static void unset_lut(data_t *data)
  static bool test_plane(data_t *data)
  {
  	igt_plane_t *plane = data->plane;
+	igt_display_t *display = &data->display;

For code readability purpose, I think we need to update to use this variable
wherever we are using "&data->display" in this function.
s/"&data->display"/"display"/

With above change, this patch LGTM
Reviewed-by: Bhanuprakash Modem <redacted>

I still don't see benefit in this patch. For now all this look like is 
doing is slow down the test and if this actually helps there's a real 
bug somewhere which is not here. My earlier review comments were not 
addressed hence repeat here, see below.

quoted

  	struct igt_fb *small_fb = &data->small_fb;
  	struct igt_fb *big_fb = &data->big_fb;
  	int w = data->big_fb_width - small_fb->width;

@@ -337,16 +338,17 @@ static bool test_plane(data_t *data)
  		igt_display_commit2(&data->display, data->display.is_atomic ?
  				    COMMIT_ATOMIC : COMMIT_UNIVERSAL);

-
+		igt_wait_for_vblank(data->drm_fd, display->pipes[data->pipe].crtc_offset);

Above this line there's flip to different fb. Below this line crc 
calculation is restarted, get one crc and stop crc. There's several 
vblanks already spent here, if now adding one more somehow helps it 
sound like there's problems in crc calculation on some platform or 
kernel is saying too early framebuffer is ready to be shown. Am I 
missing something here?

/Juha-Pekka

quoted

  		igt_pipe_crc_collect_crc(data->pipe_crc, &small_crc);

  		igt_plane_set_fb(plane, big_fb);
  		igt_fb_set_position(big_fb, plane, x, y);
  		igt_fb_set_size(big_fb, plane, small_fb->width, small_fb->height);
+

spurious empty line need to be removed.

quoted

  		igt_plane_set_size(plane, data->width, data->height);
  		igt_display_commit2(&data->display, data->display.is_atomic ?
  				    COMMIT_ATOMIC : COMMIT_UNIVERSAL);
-
+		igt_wait_for_vblank(data->drm_fd, display->pipes[data->pipe].crtc_offset);
  		igt_pipe_crc_collect_crc(data->pipe_crc, &big_crc);

  		igt_plane_set_fb(plane, NULL);
--
2.7.4

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

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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