[igt-dev] [PATCH] tests/kms_vblank: Turn on hardware before testing invalid vblank.

[Rodrigo Siqueira Jordao]

Hi Mark,

See my comments inline.

On 2021-05-20 3:34 p.m., Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub at google.com>
> 
> [Why]
> Before any hardware is on, the vblank is off and its ref counter is in
> an initialized state as each driver handles toggling it differently.
> Ioctl DRM_IOCTL_WAIT_VBLANK could return 0 or invalid depending on the
> hardware, without necessarily meaning anything.

Maybe you can add some hardware examples here.

> [How]
> For invalid_subtest(), active the CRTCs to turn the hardware on so
> DRM_IOCTL_WAIT_VBLANK returns something meaningful.

You forgot the `Signed-off-by`.

> ---
>   tests/kms_vblank.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/kms_vblank.c b/tests/kms_vblank.c
> index 93b01eba..cb68a2f8 100644
> --- a/tests/kms_vblank.c
> +++ b/tests/kms_vblank.c
> @@ -475,8 +475,14 @@ static void invalid_subtest(data_t *data, int fd)
>   {
>   	union drm_wait_vblank vbl;
>   	unsigned long valid_flags;
> +	igt_display_t* display = &data->display;

Why add this variable instead of just use `&data->display`?

> +	enum pipe p = 0;

Why did you add this variable? Improve the readability? If so, I'm not 
sure that p is better than have 0 here. Or did I miss something?

> +	igt_output_t* output;
>   
> -	igt_display_require_output_on_pipe(&data->display, 0);
> +	igt_display_require_output_on_pipe(display, p);
> +	data->pipe = p;
> +	for_each_valid_output_on_pipe(display, p, output)
> +		prepare_crtc(data, fd, output);
>   
>   	/* First check all is well with a simple query */
>   	memset(&vbl, 0, sizeof(vbl));
> @@ -511,6 +517,9 @@ static void invalid_subtest(data_t *data, int fd)
>   	vbl.request.type |= _DRM_VBLANK_SECONDARY;
>   	vbl.request.type |= _DRM_VBLANK_FLAGS_MASK;
>   	igt_assert_eq(wait_vblank(fd, &vbl), -EINVAL);
> +
> +	for_each_valid_output_on_pipe(display, p, output)
> +		cleanup_crtc(data, fd, output);
>   }

This patch makes sense to me. I tested it in a Raven system (DCN1.0), 
and everything looks fine to me; however, even without your patch, I see 
the same result.

Btw, thanks a lot for your patch.

>   igt_main
>