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

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Tue, 2019-02-05 at 17:18 +0200, Juha-Pekka Heikkila wrote:
> 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 see. Would it not pass even if the color that we use is base color
like R, G  or B? Can we conditionally limit the colors that are tested
for DRM_FORMAT_C8 in kms_plane?

>  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.

How about first removing only the formats that are tested elsewhere -
reduces noise and test duplication? That should leave
DRM_FORMAT_XBGR2101010 and DRM_FORMAT_C8. When you've added support for
DRM_FORMAT_XBGR2101010 in kms_plane, then that can be removed too.
Finally, test a limited set of colors for DRM_FORMAT_C8 in kms_plane
and kill kms_available_modes_crc.

-DK

> 
> /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
> > > > > ,
> > > > > -							  modeP
> > > > > lane-
> > > > > > formats[modeindex]);
> > > > > 
> > > > > +				failed_crcs +=
> > > > > test_one_mode(data,
> > > > > output,
> > > > > +							     pl
> > > > > ane,
> > > > > pipe,
> > > > > +							     mo
> > > > > dePlane-
> > > > > > 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