[igt-dev] [PATCH i-g-t v2] tests/kms_plane_multiple: Drain pipe before reading CRC

[Maarten Lankhorst]

Op 09-03-18 om 12:25 schreef Mika Kahola:
> On Fri, 2018-03-09 at 09:40 +0100, Maarten Lankhorst wrote:
>> Op 28-02-18 om 14:44 schreef Mika Kahola:
>>> In CI runs we every now and then fail to read correct CRC yielding
>>> an error
>>> when comparing reference and grabbed CRC's. Let's first fix the
>>> test so that
>>> we drain the pipe first and then read the correct CRC. While at it,
>>> let's
>>> simplify the test by combining legacy and atomic tests into a one
>>> common
>>> function.
>>>
>>> v2: We don't need to drain pipe when we grab first CRC
>>>
>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=103166
>>> Signed-off-by: Mika Kahola <mika.kahola at intel.com>
>>> ---
>>>  tests/kms_plane_multiple.c | 142 ++++++++++++---------------------
>>> ------------
>>>  1 file changed, 38 insertions(+), 104 deletions(-)
>>>
>>> diff --git a/tests/kms_plane_multiple.c
>>> b/tests/kms_plane_multiple.c
>>> index 95b7138..ff8ede3 100644
>>> --- a/tests/kms_plane_multiple.c
>>> +++ b/tests/kms_plane_multiple.c
>>> @@ -32,7 +32,6 @@
>>>  
>>>  IGT_TEST_DESCRIPTION("Test atomic mode setting with multiple
>>> planes ");
>>>  
>>> -#define MAX_CRCS          2
>>>  #define SIZE_PLANE      256
>>>  #define SIZE_CURSOR     128
>>>  #define LOOP_FOREVER     -1
>>> @@ -46,6 +45,7 @@ typedef struct {
>>>  typedef struct {
>>>  	int drm_fd;
>>>  	igt_display_t display;
>>> +	igt_crc_t ref_crc;
>>>  	igt_pipe_crc_t *pipe_crc;
>>>  	igt_plane_t **plane;
>>>  	struct igt_fb *fb;
>>> @@ -92,20 +92,23 @@ static void test_fini(data_t *data,
>>> igt_output_t *output, int n_planes)
>>>  	igt_output_set_pipe(output, PIPE_ANY);
>>>  
>>>  	igt_pipe_crc_free(data->pipe_crc);
>>> +	data->pipe_crc = NULL;
>>>  
>>>  	free(data->plane);
>>>  	data->plane = NULL;
>>> -	free(data->fb);
>>> -	data->fb = NULL;
>>> +
>>> +	igt_remove_fb(data->drm_fd, data->fb);
>>> +
>>> +	igt_display_reset(&data->display);
>>>  }
>>>  
>>>  static void
>>> -test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
>>> bool atomic,
>>> -	      color_t *color, uint64_t tiling, igt_crc_t **crc /*
>>> out */)
>>> +test_grab_crc(data_t *data, igt_output_t *output, enum pipe pipe,
>>> int commit,
>>> +	      color_t *color, uint64_t tiling)
>>>  {
>>>  	drmModeModeInfo *mode;
>>>  	igt_plane_t *primary;
>>> -	int ret, n;
>>> +	int ret;
>>>  
>>>  	igt_output_set_pipe(output, pipe);
>>>  
>>> @@ -122,13 +125,16 @@ test_grab_crc(data_t *data, igt_output_t
>>> *output, enum pipe pipe, bool atomic,
>>>  
>>>  	igt_plane_set_fb(data->plane[primary->index], &data-
>>>> fb[primary->index]);
>>>  
>>> -	ret = igt_display_try_commit2(&data->display,
>>> -				      atomic ? COMMIT_ATOMIC :
>>> COMMIT_LEGACY);
>>> +	ret = igt_display_try_commit2(&data->display, commit);
>>>  	igt_skip_on(ret != 0);
>>>  
>>> -	igt_pipe_crc_start(data->pipe_crc);
>>> -	n = igt_pipe_crc_get_crcs(data->pipe_crc, 1, crc);
>>> -	igt_assert_eq(n, 1);
>>> +	if (commit == COMMIT_LEGACY) {
>>> +		igt_pipe_crc_collect_crc(data->pipe_crc, &data-
>>>> ref_crc);
>>> +	} else {
>>> +		igt_pipe_crc_start(data->pipe_crc);
>>> +		igt_pipe_crc_drain(data->pipe_crc);
>>> +		igt_pipe_crc_get_single(data->pipe_crc, &data-
>>>> ref_crc);
>>> +	}
>> Why the 2 different paths?
> The *_start *_get_single method didn't work for COMMIT_ATOMIC so I
> decided to use legacy way for COMMIT_LEGACY. Maybe we could after all
> drop the legacy commits from this test. Originally, this test was all
> about atomic tests. 
>> And again, after pipe_crc_start no _drain() is needed, we already
>> have a good CRC.
> Yeah, this thing is an echo from the past. I remember we already
> discussed this when previous patch was under review.
>
> Thanks for the review!
Just nuke the legacy paths then. :)