Re: [igt-dev] [PATCH i-g-t] tests/kms_available_modes_crc: Fix handling of test comparisons

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 30.1.2019 20.23, Dhinakaran Pandiyan wrote:
> On Wed, 2019-01-30 at 13:25 +0200, Juha-Pekka Heikkila wrote:
>> On 30.1.2019 8.40, Dhinakaran Pandiyan wrote:
>>> On Fri, 2019-01-25 at 14:30 +0200, Juha-Pekka Heikkila wrote:
>>>> Fixed handling of single plane modes so used bpp never get to
>>>> be 0.
>>>>
>>>> Reduce verboseness if there's no errors.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106701
>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>> ---
>>>>    tests/kms_available_modes_crc.c | 123 ++++++++++++++++++++-----
>>>> ---
>>>> ------------
>>>>    1 file changed, 61 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/tests/kms_available_modes_crc.c
>>>> b/tests/kms_available_modes_crc.c
>>>> index 7ff385f..432c084 100644
>>>> --- a/tests/kms_available_modes_crc.c
>>>> +++ b/tests/kms_available_modes_crc.c
>>>> @@ -54,6 +54,12 @@ typedef struct {
>>>>    
>>>>    	igt_crc_t cursor_crc;
>>>>    	igt_crc_t fullscreen_crc;
>>>> +
>>>> +	/*
>>>> +	 * If there's unknown format setting up mode can fail
>>>> +	 * without failing entire test.
>>>> +	 */
>>>> +	bool allow_fail;
>>>>    } data_t;
>>>>    
>>>>    
>>>> @@ -120,7 +126,6 @@ static void
>>>> generate_comparison_crc_list(data_t
>>>> *data, igt_output_t *output)
>>>>    
>>>>    static const struct {
>>>>    	uint32_t	fourcc;
>>>> -	char		zeropadding;
>>>>    	enum		{ BYTES_PP_1=1,
>>>>    				BYTES_PP_2=2,
>>>>    				BYTES_PP_4=4,
>>>> @@ -129,10 +134,10 @@ static const struct {
>>>>    				SKIP4 } bpp;
>>>>    	uint32_t	value;
>>>>    } fillers[] = {
>>>> -	{ DRM_FORMAT_C8, 0, BYTES_PP_1, 0xff},
>>>> -	{ DRM_FORMAT_RGB565, 0, BYTES_PP_2, 0xffff},
>>>> -	{ DRM_FORMAT_XRGB8888, 0, BYTES_PP_4, 0xffffffff},
>>>> -	{ DRM_FORMAT_XBGR8888, 0, BYTES_PP_4, 0xffffffff},
>>>> +	{ DRM_FORMAT_C8, BYTES_PP_1, 0xff},
>>>> +	{ DRM_FORMAT_RGB565, BYTES_PP_2, 0xffff},
>>>> +	{ DRM_FORMAT_XRGB8888, BYTES_PP_4, 0xffffffff},
>>>> +	{ DRM_FORMAT_XBGR8888, BYTES_PP_4, 0xffffffff},
>>>>    
>>>>    	/*
>>>>    	 * following two are skipped because blending seems to
>>>> work
>>>> @@ -140,31 +145,31 @@ static const struct {
>>>>    	 * Test still creates the planes, just filling plane
>>>>    	 * and getting crc is skipped.
>>>>    	 */
>>>> -	{ DRM_FORMAT_ARGB8888, 0, SKIP4, 0xffffffff},
>>>> -	{ DRM_FORMAT_ABGR8888, 0, SKIP4, 0x00ffffff},
>>>> +	{ DRM_FORMAT_ARGB8888, SKIP4, 0xffffffff},
>>>> +	{ DRM_FORMAT_ABGR8888, SKIP4, 0x00ffffff},
>>>>    
>>>> -	{ DRM_FORMAT_XRGB2101010, 0, BYTES_PP_4, 0xffffffff},
>>>> -	{ DRM_FORMAT_XBGR2101010, 0, BYTES_PP_4, 0xffffffff},
>>>> +	{ DRM_FORMAT_XRGB2101010, BYTES_PP_4, 0xffffffff},
>>>> +	{ DRM_FORMAT_XBGR2101010, BYTES_PP_4, 0xffffffff},
>>>>    
>>>> -	{ DRM_FORMAT_YUYV, 0, BYTES_PP_4, 0x80eb80eb},
>>>> -	{ DRM_FORMAT_YVYU, 0, BYTES_PP_4, 0x80eb80eb},
>>>> -	{ DRM_FORMAT_VYUY, 0, BYTES_PP_4, 0xeb80eb80},
>>>> -	{ DRM_FORMAT_UYVY, 0, BYTES_PP_4, 0xeb80eb80},
>>>> +	{ DRM_FORMAT_YUYV, BYTES_PP_4, 0x80eb80eb},
>>>> +	{ DRM_FORMAT_YVYU, BYTES_PP_4, 0x80eb80eb},
>>>> +	{ DRM_FORMAT_VYUY, BYTES_PP_4, 0xeb80eb80},
>>>> +	{ DRM_FORMAT_UYVY, BYTES_PP_4, 0xeb80eb80},
>>>>    
>>>>    	/*
>>>>    	 * (semi-)planar formats
>>>>    	 */
>>>> -	{ DRM_FORMAT_NV12, 0, NV12, 0x80eb},
>>>> +	{ DRM_FORMAT_NV12, NV12, 0x80eb},
>>>>    #ifdef DRM_FORMAT_P010
>>>> -	{ DRM_FORMAT_P010, 0, P010, 0x8000eb00},
>>>> +	{ DRM_FORMAT_P010, P010, 0x8000eb00},
>>>>    #endif
>>>>    #ifdef DRM_FORMAT_P012
>>>> -	{ DRM_FORMAT_P012, 0, P010, 0x8000eb00},
>>>> +	{ DRM_FORMAT_P012, P010, 0x8000eb00},
>>>>    #endif
>>>>    #ifdef DRM_FORMAT_P016
>>>> -	{ DRM_FORMAT_P016, 0, P010, 0x8000eb00},
>>>> +	{ DRM_FORMAT_P016, P010, 0x8000eb00},
>>>
>>> Doesn't the library implement this stuff already? I see similar
>>> code in
>>> create_bo_for_fb(). Even if new formats are needed, I think it's
>>> best
>>> to put that in the lib/igt_fb.
>>
>> These days igt_fb does implement most of these modes I agree. This
>> test
>> was written back in the day when igt support was limited to RGB888
>> and
>> RGB565. Idea of this test was just to try all fb formats kernel
>> advertised could actually initialize.
>>
>> If you feel this test is obsolete you could ask for votes to get it
>> removed, I tried that before xmas but nobody was interested in
>> removing
>> this so I started to fix problems seen in this test.
> 
> I had a quick glance at kms_plane, like you said it pretty much does
> the same things but better. My suggestion is to
> 1) add support in the library for the missing formats i.e.,
> DRM_FORMAT_C8, DRM_FORMAT_XBGR2101010
> 2) get rid of this test.
> 

Hi DK,

I was looking at those formats missing from list of IGT supported 
formats. DRM_FORMAT_XBGR2101010 shouldn't be much of a problem to add 
but it's yet to be seen how it will show in different tests.
DRM_FORMAT_C8 on the other hand is different story, I think it probably 
should not be added as supported format for IGT. Reason for not adding 
C8 would be the 8bit nature, I think it will never have enough colors to 
be passing most of CRC tests. I had idea for C8 there would be separate 
test which will only test C8 features.

I'd begin by removing available modes test as it seems to cause only 
useless noise now and is doubled by kms_plane in places where it really 
matter.

You have opinions or ideas on this? I'd add above on my 'to-do' list.

/Juha-Pekka

> 
>>
>> In any case these days one cannot include new fb formats into i915
>> without IGT support so kms_plane test does what this test does.
>>
>>>
>>>
>>>>    #endif
>>>> -	{ 0, 0, 0, 0 }
>>>> +	{ 0, SKIP4, 0 }
>>>>    };
>>>>    
>>>>    /*
>>>> @@ -233,13 +238,10 @@ static bool fill_in_fb(data_t *data,
>>>> igt_output_t *output, igt_plane_t *plane,
>>>>    			writesize = data->size;
>>>>    			break;
>>>>    		}
>>>> -		igt_info("Format %s CRC comparison skipped by
>>>> design.\n",
>>>> -			 (char*)&fillers[i].fourcc);
>>>> -
>>>> -		return false;
>>>> +	/* Fall through */
>>>>    	default:
>>>> -		igt_info("Unsupported mode for test %s\n",
>>>> -			 (char*)&fillers[i].fourcc);
>>>> +		igt_info("Unsupported mode for testing CRC %.4s\n",
>>>> +			 (char *)&format);
>>>>    		return false;
>>>>    	}
>>>>    
>>>> @@ -260,6 +262,8 @@ static bool setup_fb(data_t *data,
>>>> igt_output_t
>>>> *output, igt_plane_t *plane,
>>>>    	int bpp = 0;
>>>>    	int i;
>>>>    
>>>> +	data->allow_fail = false;
>>>> +
>>>>    	mode = igt_output_get_mode(output);
>>>>    	if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>>>>    		w = mode->hdisplay;
>>>> @@ -288,6 +292,8 @@ static bool setup_fb(data_t *data,
>>>> igt_output_t
>>>> *output, igt_plane_t *plane,
>>>>    		break;
>>>>    
>>>>    	case SKIP4:
>>>> +		data->allow_fail = true;
>>>> +		/* fall through */
>>>
>>> Wouldn't this prevent CRC testing for
>>> (fillers[i].fourcc == DRM_FORMAT_ARGB8888 && plane->type ==
>>> DRM_PLANE_TYPE_CURSOR) ?
>>
>> There was later patch which fixed this. This version didn't pass CI
>> test
>> because I had forgotten "igt_display_reset(&data->display)" call bit
>> below, it killed test on KBL.
>>
>>>
>>>
>>>>    	case BYTES_PP_4:
>>>>    		bpp = 32;
>>>>    		break;
>>>> @@ -324,7 +330,7 @@ static bool setup_fb(data_t *data,
>>>> igt_output_t
>>>> *output, igt_plane_t *plane,
>>>>    
>>>>    	if(ret < 0) {
>>>>    		igt_info("Creating fb for format %s failed,
>>>> return code
>>>> %d\n",
>>>> -			 (char*)&data->format.name, ret);
>>>> +			 (char *)&data->format.name, ret);
>>>>    
>>>>    		return false;
>>>>    	}
>>>> @@ -385,13 +391,14 @@ static bool prepare_crtc(data_t *data,
>>>> igt_output_t *output,
>>>>    
>>>>    
>>>>    static int
>>>> -test_one_mode(data_t* data, igt_output_t *output, igt_plane_t*
>>>
>>> Side note not related to the changes you are making, "mode" is
>>> quite
>>> misleading. Shouldn't the test be called kms_pixel_formats_crc or
>>> something? I thought this was about testing different connector
>>> modes.
>>>
>>>
>>>> plane,
>>>> -	      int mode)
>>>> +test_one_mode(data_t *data, igt_output_t *output, igt_plane_t
>>>> *plane,
>>>> +	      enum pipe pipe, int mode)
>>>>    {
>>>>    	igt_crc_t current_crc;
>>>>    	signed rVal = 0;
>>>>    	bool do_crc;
>>>> -	char* crccompare[2];
>>>> +	char *crccompare[2];
>>>> +	igt_crc_t *p_crc;
>>>>    
>>>>    	if (prepare_crtc(data, output, plane, mode)){
>>>>    		/*
>>>> @@ -409,32 +416,32 @@ test_one_mode(data_t* data, igt_output_t
>>>> *output, igt_plane_t* plane,
>>>>    		if (do_crc) {
>>>>    			igt_pipe_crc_get_current(data->gfx_fd,
>>>> data-
>>>>> pipe_crc, &current_crc);
>>>>
>>>>    
>>>> -			if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>>>> -				if (!igt_check_crc_equal(&current_crc,
>>>> -					&data->fullscreen_crc)) {
>>>> -					crccompare[0] =
>>>> igt_crc_to_string(&current_crc);
>>>> -					crccompare[1] =
>>>> igt_crc_to_string(&data->fullscreen_crc);
>>>> -					igt_warn("crc mismatch. target
>>>> %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
>>>> -					free(crccompare[0]);
>>>> -					free(crccompare[1]);
>>>> -					rVal++;
>>>> -				}
>>>> -			} else {
>>>> -				if (!igt_check_crc_equal(&current_crc,
>>>> -					&data->cursor_crc)) {
>>>> -					crccompare[0] =
>>>> igt_crc_to_string(&current_crc);
>>>> -					crccompare[1] =
>>>> igt_crc_to_string(&data->cursor_crc);
>>>> -					igt_warn("crc mismatch. target
>>>> %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
>>>> -					free(crccompare[0]);
>>>> -					free(crccompare[1]);
>>>> +			if (plane->type != DRM_PLANE_TYPE_CURSOR)
>>>> +				p_crc = &data->fullscreen_crc;
>>>> +			else
>>>> +				p_crc = &data->cursor_crc;
>>>> +
>>>> +			if (!igt_check_crc_equal(&current_crc, p_crc))
>>>> {
>>>> +				crccompare[0] =
>>>> igt_crc_to_string(&current_crc);
>>>> +				crccompare[1] =
>>>> igt_crc_to_string(p_crc);
>>>> +				igt_warn("crc mismatch on %s PIPE %s
>>>> plane %d format %.4s. target %.8s, result %.8s.\n",
>>>> +					 igt_output_name(output),
>>>> +					 kmstest_pipe_name(pipe),
>>>> +					 plane->index,
>>>> +					 (char *)&data->format.name,
>>>> +					 crccompare[0],
>>>> +					 crccompare[1]);
>>>
>>> This is the only hunk I was hoping you would be able to split into
>>> a
>>> separate patch.
>>>
>>>> +
>>>> +				free(crccompare[0]);
>>>> +				free(crccompare[1]);
>>>> +				if (!data->allow_fail)
>>>
>>> Is there a point of reading CRCs if failures are allowed?
>>
>> It is marked as mode which is not listed in this test but
>> initialization
>> worked so default paths are followed. It will produce warning which
>> I
>> think is useful for indicating this test need updating but will not
>> produce failure to mark newly added features caused problems.
>>
>>>
>>>>    					rVal++;
>>>> -				}
>>>>    			}
>>>>    		}
>>>>    		remove_fb(data, output, plane);
>>>>    		return rVal;
>>>>    	}
>>>> -	return 1;
>>>> +	return data->allow_fail ? 0 : 1;
>>>>    }
>>>>    
>>>>    
>>>> @@ -445,11 +452,11 @@ test_available_modes(data_t* data)
>>>>    	igt_plane_t *plane;
>>>>    	int modeindex;
>>>>    	enum pipe pipe;
>>>> -	int invalids = 0;
>>>> +	int failed_crcs = 0;
>>>>    	drmModePlane *modePlane;
>>>> -	char planetype[3][8] = {"OVERLAY\0", "PRIMARY\0", "CURSOR\0" };
>>>>    
>>>>    	for_each_pipe_with_valid_output(&data->display, pipe,
>>>> output) {
>>>> +		igt_display_reset(&data->display);
>>>>    		igt_output_set_pipe(output, pipe);
>>>>    		igt_display_commit2(&data->display, data-
>>>>> commit);
>>>>    
>>>> @@ -472,17 +479,9 @@ test_available_modes(data_t* data)
>>>>    			     modeindex++) {
>>>>    				data->format.dword = modePlane-
>>>>> formats[modeindex];
>>>>
>>>>    
>>>> -				igt_info("Testing connector %s using
>>>> pipe %s" \
>>>> -					 " plane index %d type %s mode
>>>> %s\n",
>>>> -					 igt_output_name(output),
>>>> -					 kmstest_pipe_name(pipe),
>>>> -					 plane->index,
>>>> -					 planetype[plane->type],
>>>> -					 (char*)&data->format.name);
>>>> -
>>>> -				invalids += test_one_mode(data, output,
>>>> -							  plane,
>>>> -							  modePlane-
>>>>> formats[modeindex]);
>>>>
>>>> +				failed_crcs += test_one_mode(data,
>>>> output,
>>>> +							     plane,
>>>> pipe,
>>>> +							     modePlane-
>>>>> formats[modeindex]);
>>>>
>>>>    			}
>>>>    			drmModeFreePlane(modePlane);
>>>>    		}
>>>> @@ -491,7 +490,7 @@ test_available_modes(data_t* data)
>>>>    		igt_display_commit2(&data->display, data-
>>>>> commit);
>>>>    		igt_output_set_pipe(output, PIPE_NONE);
>>>>    	}
>>>> -	igt_assert(invalids == 0);
>>>> +	igt_assert_f((failed_crcs == 0), "There was %d modes with
>>>> invalid crc\n", failed_crcs);
>>>>    }
>>>>    
>>>>    
>>
>>
> 

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