Nouveau - Mar 2021 - [igt-dev] [PATCH i-g-t] tests/kms_plane: Don't unset primary_fb when testing other planes

On 18/03/2021 00:44, Lyude wrote:
> From: Lyude Paul <lyude at redhat.com>
> 
> Currently, nouveau doesn't support having a CRTC without a primary FB
set. We
> won't reject such configurations, but the behavior is undefined which
will cause
> CRCs to become undefined. So, avoid clearing the primary FB while we're
testing
> non-primary planes.
Looks good to me:

Reviewed-by: Martin Peres <martin.peres at mupuf.org>
> 
> Signed-off-by: Lyude Paul <lyude at redhat.com>
> Cc: Martin Peres <martin.peres at free.fr>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Jeremy Cline <jcline at redhat.com>
> ---
>   tests/kms_plane.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index 298a9375..c87983a4 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -817,9 +817,10 @@ enum crc_set { PACKED_CRC_SET,
>   	       MAX_CRC_SET };
>   
>   static bool test_format_plane(data_t *data, enum pipe pipe,
> -			      igt_output_t *output, igt_plane_t *plane)
> +			      igt_output_t *output, igt_plane_t *plane, igt_fb_t *primary_fb)
>   {
>   	struct igt_fb fb = {};
> +	struct igt_fb *clear_fb = plane->type == DRM_PLANE_TYPE_PRIMARY ?
primary_fb : NULL;
>   	drmModeModeInfo *mode;
>   	uint32_t format, ref_format;
>   	uint64_t modifier, ref_modifier;
> @@ -879,7 +880,7 @@ static bool test_format_plane(data_t *data, enum pipe
pipe,
>   			height = test_fb.height;
>   		}
>   
> -		igt_plane_set_fb(plane, NULL);
> +		igt_plane_set_fb(plane, clear_fb);
>   
>   		igt_remove_fb(data->drm_fd, &test_fb);
>   	}
> @@ -937,7 +938,7 @@ static bool test_format_plane(data_t *data, enum pipe
pipe,
>   
>   	igt_pipe_crc_stop(data->pipe_crc);
>   
> -	igt_plane_set_fb(plane, NULL);
> +	igt_plane_set_fb(plane, clear_fb);
>   	igt_remove_fb(data->drm_fd, &fb);
>   
>   	return result;
> @@ -1010,7 +1011,7 @@ test_pixel_formats(data_t *data, enum pipe pipe)
>   	for_each_plane_on_pipe(&data->display, pipe, plane) {
>   		if (skip_plane(data, plane))
>   			continue;
> -		result &= test_format_plane(data, pipe, output, plane);
> +		result &= test_format_plane(data, pipe, output, plane,
&primary_fb);
>   	}
>   
>   	test_fini(data);
>

On 18.3.2021 8.41, Martin Peres wrote:
> On 18/03/2021 00:44, Lyude wrote:
>> From: Lyude Paul <lyude at redhat.com>
>>
>> Currently, nouveau doesn't support having a CRTC without a primary
FB
>> set. We
>> won't reject such configurations, but the behavior is undefined
which
>> will cause
>> CRCs to become undefined. So, avoid clearing the primary FB while 
>> we're testing
>> non-primary planes.
> 
> Looks good to me:
> 
> Reviewed-by: Martin Peres <martin.peres at mupuf.org>
> 
This doesn't look good at all. Code changes were never run on ci, see this:
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5614/shards-all.html?testfilter=kms_plane
at pixel-format

This happen because of:
igt-gpu-tools$ git blame tests/intel-ci/blacklist-pre-merge.txt -L 173,+1
3e686098d9 (Martin Peres 2020-02-21 11:00:47 +0200 173) 
igt at kms_plane@pixel-format-pipe-[a-d]-planes(-source-clamping)?

You'll need to include something like this to test and review what did 
change on pixel format tests:
https://patchwork.freedesktop.org/patch/404706/?series=84370&rev=1

but as this patch seems to be already merged as untested we now have 
pixel format tests failing reliably ever since:
https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=kms_plane at
pixel-format&hosts=tgl%7Cicl

I have only intel icl at hand so I have no suggestions for fix. Though, 
looking at those failures Stan (CCd) may have some interest on them.

/Juha-Pekka
>>
>> Signed-off-by: Lyude Paul <lyude at redhat.com>
>> Cc: Martin Peres <martin.peres at free.fr>
>> Cc: Ben Skeggs <bskeggs at redhat.com>
>> Cc: Jeremy Cline <jcline at redhat.com>
>> ---
>> ? tests/kms_plane.c | 9 +++++----
>> ? 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
>> index 298a9375..c87983a4 100644
>> --- a/tests/kms_plane.c
>> +++ b/tests/kms_plane.c
>> @@ -817,9 +817,10 @@ enum crc_set { PACKED_CRC_SET,
>> ???????????? MAX_CRC_SET };
>> ? static bool test_format_plane(data_t *data, enum pipe pipe,
>> -????????????????? igt_output_t *output, igt_plane_t *plane)
>> +????????????????? igt_output_t *output, igt_plane_t *plane, igt_fb_t 
>> *primary_fb)
>> ? {
>> ????? struct igt_fb fb = {};
>> +??? struct igt_fb *clear_fb = plane->type == DRM_PLANE_TYPE_PRIMARY
?
>> primary_fb : NULL;
>> ????? drmModeModeInfo *mode;
>> ????? uint32_t format, ref_format;
>> ????? uint64_t modifier, ref_modifier;
>> @@ -879,7 +880,7 @@ static bool test_format_plane(data_t *data, enum 
>> pipe pipe,
>> ????????????? height = test_fb.height;
>> ????????? }
>> -??????? igt_plane_set_fb(plane, NULL);
>> +??????? igt_plane_set_fb(plane, clear_fb);
>> ????????? igt_remove_fb(data->drm_fd, &test_fb);
>> ????? }
>> @@ -937,7 +938,7 @@ static bool test_format_plane(data_t *data, enum 
>> pipe pipe,
>> ????? igt_pipe_crc_stop(data->pipe_crc);
>> -??? igt_plane_set_fb(plane, NULL);
>> +??? igt_plane_set_fb(plane, clear_fb);
>> ????? igt_remove_fb(data->drm_fd, &fb);
>> ????? return result;
>> @@ -1010,7 +1011,7 @@ test_pixel_formats(data_t *data, enum pipe pipe)
>> ????? for_each_plane_on_pipe(&data->display, pipe, plane) {
>> ????????? if (skip_plane(data, plane))
>> ????????????? continue;
>> -??????? result &= test_format_plane(data, pipe, output, plane);
>> +??????? result &= test_format_plane(data, pipe, output, plane, 
>> &primary_fb);
>> ????? }
>> ????? test_fini(data);
>>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev