Re: [igt-dev] [PATCH i-g-t] tests/kms_rotation_crc: Add multi plane tests.

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

Hi Ankit, thanks for the comments. I'll soon post fixed patch. I had 
originally dropped idea for rectangular test figure as I didn't know if 
its useful, I now following your comments fixed it to work and take into 
account screen space.

/Juha-Pekka

On 16.11.2018 14:28, Nautiyal, Ankit K wrote:
> Hi Juha-Pekka,
> 
> The new test added do cover the gap of having multiple-plane rotation 
> together, along with the top and bottom cropping scenarios.
> 
> The test work well for square figures i.e. the width = height =512, but 
> for rectangular figure, the test needs some fix in get_multiplane_crc(), 
> as this function does not assume that the width and height are same.
> 
> Please find suggestions inline.
> 
> Other than that, I could only nitpick some of the styling issues given 
> inline.
> 
> On 10/26/2018 6:28 PM, Juha-Pekka Heikkila wrote:
>> Add three new tests which try primary and sprite planes
>> next to each other with different plane formats, rotations
>> and tiling modes.
>>
>> multiplane-rotation subtest run test through with both planes
>> fully visible.
>>
>> multiplane-rotation-cropping-top will crop primary plane to
>> left/top corner and sprite plane to right/top corner while running
>> rotation tests.
>>
>> multiplane-rotation-cropping-bottom will crop primary plane to
>> left/bottom corner and sprite plane to right/bottom corner while
>> running rotation tests.
>>
>> Signed-off-by: Juha-Pekka Heikkila<juhapekka.heikkila@gmail.com>
>> ---
>>   tests/kms_rotation_crc.c | 249 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 249 insertions(+)
>>
>> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
>> index 366c254..df9ac91 100644
>> --- a/tests/kms_rotation_crc.c
>> +++ b/tests/kms_rotation_crc.c
>> @@ -26,6 +26,25 @@
>>   #include <math.h>
>>   
>>   #define MAX_FENCES 32
>> +#define MAXMULTIPLANESAMOUNT 2
>> +
>> +struct p_struct {
>> +	igt_plane_t *plane;
>> +	struct igt_fb fb;
>> +};
>> +
>> +enum p_pointorigo {
>> +	p_top = 1 << 0,
>> +	p_bottom = 1 << 1,
>> +	p_left = 1 << 2,
>> +	p_right = 1 << 3
>> +};
>> +
>> +struct p_point{
>> +	enum p_pointorigo origo;
>> +	int32_t x;
>> +	int32_t y;
>> +};
>>   
>>   typedef struct {
>>   	int gfx_fd;
>> @@ -43,6 +62,9 @@ typedef struct {
>>   	uint32_t override_fmt;
>>   	uint64_t override_tiling;
>>   	int devid;
>> +
>> +	struct p_struct *multiplaneoldview;
>> +	struct p_point planepos[MAXMULTIPLANESAMOUNT];
>>   } data_t;
>>   
>>   typedef struct {
>> @@ -395,6 +417,197 @@ static void test_plane_rotation(data_t *data, int plane_type, bool test_bad_form
>>   	}
>>   }
>>   
>> +typedef struct {
>> +	int32_t x1, y1;
>> +	uint64_t width, height, tiling, planetype, format;
>> +	igt_rotation_t rotation_sw, rotation_hw;
>> +} planeinfos;
>> +
>> +static void get_multiplane_crc(data_t *data, igt_output_t *output,
>> +			       igt_crc_t *crc_output, planeinfos* planeinfo,
>> +			       int numplanes)
>> +{
>> +	uint32_t w, h;
>> +	igt_display_t *display = &data->display;
>> +	struct p_struct *planes, *oldplanes;
>> +	int c, ret;
>> +
>> +	oldplanes = data->multiplaneoldview;
>> +	planes = malloc(sizeof(*planes)*numplanes);
>> +
>> +	for (c = 0; c < numplanes; c++) {
>> +		planes[c].plane = igt_output_get_plane_type(output,
>> +							    planeinfo[c].planetype);
>> +
>> +		w = planeinfo[c].width;
>> +		h = planeinfo[c].height;
>> +
>> +		if ((planeinfo[c].rotation_hw|planeinfo[c].rotation_sw)
>> +		    &(IGT_ROTATION_90|IGT_ROTATION_270))
>> +			igt_swap(w, h);
>> +
> In case of an actual hardware rotation 90 and 270, the width and height 
> should not be swapped.
> The 'if condition' should have only check for rotation_sw, and not for 
> rotation_hw.
> 
>> +		igt_plane_set_size(planes[c].plane, w, h);
> For 90 and 270 degree rotation (i.e. rotation_hw) , igt_plane_set_size() 
> should have height in place of width and vice-versa.
> 
> if (rotation_hw & (IGT_ROTATION_90 | IGT_ROTATION_270)
>          igt_plane_set_size(plane[c].plane, h, w);
> 
> Further more this function igt_plane_set_size should be called after 
> igt_plane_set_fb(), because igt_plane_set_fb sets the plane size with 
> fb.width and fb.height, thereby overwriting our values.
> 
>> +
>> +		igt_create_fb(data->gfx_fd, w, h, planeinfo[c].format,
>> +			      planeinfo[c].tiling, &planes[c].fb);
>> +
>> +		paint_squares(data, planeinfo[c].rotation_sw, &planes[c].fb, 1.0f);
>> +		igt_plane_set_fb(planes[c].plane, &planes[c].fb);
>> +		igt_plane_set_position(planes[c].plane, planeinfo[c].x1, planeinfo[c].y1);
>> +		igt_plane_set_rotation(planes[c].plane, planeinfo[c].rotation_hw);
>> +	}
>> +
>> +	ret = igt_display_try_commit2(display, COMMIT_ATOMIC);
>> +	igt_assert_eq(ret, 0);
>> +
>> +	igt_pipe_crc_get_current(data->gfx_fd, data->pipe_crc, crc_output);
>> +
>> +	for (c = 0; c < numplanes && oldplanes; c++)
>> +		igt_remove_fb(data->gfx_fd, &oldplanes[c].fb);
>> +
>> +	free(oldplanes);
>> +	data->multiplaneoldview = (void*)planes;
>> +}
>> +
>> +static void pointlocation(data_t *data, planeinfos* p, drmModeModeInfo *mode,
>> +			  int c)
>> +{
>> +	p[c].x1 = data->planepos[c].x
>> +			+((data->planepos[c].origo&p_right)?mode->hdisplay:0);
>> +	p[c].y1 = data->planepos[c].y
>> +			+((data->planepos[c].origo&p_bottom)?mode->vdisplay:0);
>> +}
> 
> Space around '&' and '?'
> 
>> +
>> +/*
>> + * Here is pipe parameter which is now used only for first pipe.
>> + * It is left here if this test ever was wanted to be run on
>> + * different pipes.
>> + */
>> +static void test_multi_plane_rotation(data_t *data, enum pipe pipe)
>> +{
>> +	igt_display_t *display = &data->display;
>> +	igt_output_t *output;
>> +	igt_crc_t retcrc_sw, retcrc_hw;
>> +	planeinfos p[2];
>> +	int c;
>> +	struct p_struct *oldplanes;
>> +	drmModeModeInfo *mode;
>> +
>> +	const static struct {
>> +		igt_rotation_t	rotation;
>> +		uint64_t	width;
>> +		uint64_t	height;
>> +		uint64_t	tiling;
>> +	} planeconfigs[] = {
>> +	{IGT_ROTATION_0, 512, 512, LOCAL_DRM_FORMAT_MOD_NONE },
> 
> In my humble opinion, we can use a rectangle content, instead of a 
> square content. The function get_muliplane_crc already takes care of 
> different width and height.
> Only change will be required in function point_location.
> 
>> +	{IGT_ROTATION_0, 512, 512, LOCAL_I915_FORMAT_MOD_X_TILED },
>> +	{IGT_ROTATION_0, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +	{IGT_ROTATION_0, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +	{IGT_ROTATION_90, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +	{IGT_ROTATION_90, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +	{IGT_ROTATION_180, 512, 512, LOCAL_DRM_FORMAT_MOD_NONE },
>> +	{IGT_ROTATION_180, 512, 512, LOCAL_I915_FORMAT_MOD_X_TILED },
>> +	{IGT_ROTATION_180, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +	{IGT_ROTATION_180, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +	{IGT_ROTATION_270, 512, 512, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +	{IGT_ROTATION_270, 512, 512, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +	};
>> +
>> +	/*
>> +	* These are those modes which are tested. For testing feel interesting
>> +	* case with tiling are 2 byte wide and 4 byte wide.
>> +	*
>> +	* TODO:
>> +	* Built support for NV12 here.
>> +	*/
>> +	const static uint32_t  formatlist[] = {DRM_FORMAT_RGB565,
>> +					       DRM_FORMAT_XRGB8888};
>> +
>> +
>> +	for_each_valid_output_on_pipe(display, pipe, output) {
>> +		int i, j, k, l;
>> +		igt_output_set_pipe(output, pipe);
>> +
>> +		mode = igt_output_get_mode(output);
>> +
>> +		igt_display_require_output(display);
>> +		igt_display_commit2(display, COMMIT_ATOMIC);
>> +
>> +		data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe,
>> +						  INTEL_PIPE_CRC_SOURCE_AUTO);
>> +		igt_pipe_crc_start(data->pipe_crc);
>> +
>> +		for (i = 0; i < ARRAY_SIZE(planeconfigs); i++) {
>> +			p[0].planetype = DRM_PLANE_TYPE_PRIMARY;
>> +			p[0].width = planeconfigs[i].width;
>> +			p[0].height = planeconfigs[i].height;
>> +			p[0].tiling = planeconfigs[i].tiling;
>> +			pointlocation(data, (planeinfos*)&p, mode, 0);
>> +
>> +			for (k = 0; k < ARRAY_SIZE(formatlist); k++) {
>> +				p[0].format = formatlist[k];
>> +
>> +				for (j = 0; j < ARRAY_SIZE(planeconfigs); j++) {
>> +					p[1].planetype = DRM_PLANE_TYPE_OVERLAY;
>> +					p[1].width = planeconfigs[j].width;
>> +					p[1].height = planeconfigs[j].height;
>> +					p[1].tiling = planeconfigs[j].tiling;
>> +					pointlocation(data, (planeinfos*)&p,
>> +						      mode, 1);
>> +
>> +					for (l = 0; l < ARRAY_SIZE(formatlist); l++) {
>> +						p[1].format = formatlist[l];
>> +
>> +						/*
>> +						 * RGB565 90/270 degrees rotation is supported
>> +						 * from gen11 onwards.
>> +						 */
>> +						if (p[0].format == DRM_FORMAT_RGB565 &&
>> +						     (planeconfigs[i].rotation&(IGT_ROTATION_90|IGT_ROTATION_270))
>> +						     && intel_gen(data->devid) < 11)
>> +							continue;
>> +
>> +						if (p[1].format == DRM_FORMAT_RGB565 &&
>> +						     (planeconfigs[j].rotation&(IGT_ROTATION_90|IGT_ROTATION_270))
> Space around '&' and '|'
>> +						     && intel_gen(data->devid) < 11)
>> +							continue;
>> +
>> +
>> +						p[0].rotation_sw = planeconfigs[i].rotation;
>> +						p[0].rotation_hw = IGT_ROTATION_0;
>> +						p[1].rotation_sw = planeconfigs[j].rotation;
>> +						p[1].rotation_hw = IGT_ROTATION_0;
>> +						get_multiplane_crc(data, output, &retcrc_sw,
>> +								   (planeinfos*)&p, MAXMULTIPLANESAMOUNT);
>> +
>> +						igt_swap(p[0].rotation_sw, p[0].rotation_hw);
>> +						igt_swap(p[1].rotation_sw, p[1].rotation_hw);
>> +						get_multiplane_crc(data, output, &retcrc_hw,
>> +								   (planeinfos*)&p, MAXMULTIPLANESAMOUNT);
>> +
>> +						igt_assert_crc_equal(&retcrc_sw, &retcrc_hw);
>> +					}
>> +				}
>> +			}
>> +		}
>> +		igt_pipe_crc_stop(data->pipe_crc);
>> +		igt_pipe_crc_free(data->pipe_crc);
>> +		igt_output_set_pipe(output, PIPE_ANY);
>> +	}
>> +
>> +	/*
>> +	* Old fbs are deleted only after new ones are set on planes.
>> +	* This is done to speed up the test
>> +	*/
>> +	oldplanes = data->multiplaneoldview;
>> +	for (c = 0; c < MAXMULTIPLANESAMOUNT && oldplanes; c++)
>> +		igt_remove_fb(data->gfx_fd, &oldplanes[c].fb);
>> +
>> +	free(oldplanes);
>> +	data->multiplaneoldview = NULL;
>> +	data->pipe_crc = NULL;
>> +}
>> +
>>   static void test_plane_rotation_exhaust_fences(data_t *data,
>>   					       enum pipe pipe,
>>   					       igt_output_t *output,
>> @@ -595,6 +808,42 @@ igt_main
>>   		}
>>   	}
>>   
>> +	igt_subtest_f("multiplane-rotation") {
>> +		igt_require(gen >= 9);
>> +		cleanup_crtc(&data);
>> +		data.planepos[0].origo = p_top|p_left;
>> +		data.planepos[0].x = 128;
>> +		data.planepos[0].y = 128;
>> +		data.planepos[1].origo = p_top|p_right;
>> +		data.planepos[1].x = -128-512;
> Space around '|' and '-' operators. Valid for few more lines below.
> Not sure if we will ever hit a case where there is a lower resolution 
> e.g. 640x480, the figure will be cropped, which is not desired, the test 
> however would still pass as crc would match.
> Perhaps we can skip if resolution is less than some threshold.
> 
>> +		data.planepos[1].y = 128;
>> +		test_multi_plane_rotation(&data, 0);
>> +	}
>> +
>> +	igt_subtest_f("multiplane-rotation-cropping-top") {
>> +		igt_require(gen >= 9);
>> +		cleanup_crtc(&data);
>> +		data.planepos[0].origo = p_top|p_left;
>> +		data.planepos[0].x = -128;
>> +		data.planepos[0].y = -128;
>> +		data.planepos[1].origo = p_top|p_right;
>> +		data.planepos[1].x = -512+128;
>> +		data.planepos[1].y = -128;
>> +		test_multi_plane_rotation(&data, 0);
>> +	}
>> +
>> +	igt_subtest_f("multiplane-rotation-cropping-bottom") {
>> +		igt_require(gen >= 9);
>> +		cleanup_crtc(&data);
>> +		data.planepos[0].origo = p_bottom|p_left;
>> +		data.planepos[0].x = -128;
>> +		data.planepos[0].y = -512+128;
>> +		data.planepos[1].origo = p_bottom|p_right;
>> +		data.planepos[1].x = -512+128;
>> +		data.planepos[1].y = -512+128;
>> +		test_multi_plane_rotation(&data, 0);
>> +	}
>> +
>>   	/*
>>   	 * exhaust-fences should be last test, if it fails we may OOM in
>>   	 * the following subtests otherwise.
>>
>>    
>>

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