Re: [igt-dev] [PATCH i-g-t 3/3] tests: Add new PSR2 selective fetch test

["Laxminarayan Bharadiya, Pankaj" <pankaj.laxminarayan.bharadiya@intel.com>]

> -----Original Message-----
> From: Souza, Jose <jose.souza@intel.com>
> Sent: 19 November 2020 01:50
> To: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>; Mun, Gwan-gyeong <gwan-
> gyeong.mun@intel.com>; igt-dev@lists.freedesktop.org
> Subject: Re: [PATCH i-g-t 3/3] tests: Add new PSR2 selective fetch test
> 
> On Wed, 2020-11-18 at 13:44 +0530, Pankaj Bharadiya wrote:
> > Selective fetch reduces display engine use of memory bandwidth by only
> > fetching (reading from memory) the updated regions of the frame buffer
> > and sending those updated regions to a panel with a PSR2 capability.
> >
> > The FB_DAMAGE_CLIPS plane property provides user-space a way inform
> > kernel about the updated regions.
> >
> > Add new test to verify selective fetch by using FB_DAMAGE_CLIPS
> > property to send  updated regions.
> >
> > Signed-off-by: Pankaj Bharadiya
> > <pankaj.laxminarayan.bharadiya@intel.com>
> > ---
> >  tests/Makefile.sources |   1 +
> >  tests/kms_psr2_sf.c    | 717
> +++++++++++++++++++++++++++++++++++++++++
> >  tests/meson.build      |   1 +
> >  3 files changed, 719 insertions(+)
> >  create mode 100644 tests/kms_psr2_sf.c
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources index
> > 96d835820..4e653c8b2 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -79,6 +79,7 @@ TESTS_progs = \
> >  	kms_properties \
> >  	kms_psr \
> >  	kms_psr2_su \
> > +	kms_psr2_sf \
> >  	kms_pwrite_crc \
> >  	kms_rmfb \
> >  	kms_rotation_crc \
> > diff --git a/tests/kms_psr2_sf.c b/tests/kms_psr2_sf.c new file mode
> > 100644 index 000000000..baa640851
> > --- /dev/null
> > +++ b/tests/kms_psr2_sf.c
> > @@ -0,0 +1,717 @@
> > +/*
> > + * Copyright © 2020 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > +obtaining a
> > + * copy of this software and associated documentation files (the
> > +"Software"),
> > + * to deal in the Software without restriction, including without
> > +limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > +sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > +the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > +the next
> > + * paragraph) shall be included in all copies or substantial portions
> > +of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > +EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > +MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT
> > +SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES
> > +OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > +ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > +OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + */
> > +
> > +#include "igt.h"
> > +#include "igt_sysfs.h"
> > +#include "igt_psr.h"
> > +#include <errno.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include "intel_bufmgr.h"
> > +
> > +IGT_TEST_DESCRIPTION("Test PSR2 selective fetch");
> > +
> > +#define SQUARE_SIZE 100
> > +
> > +#define CUR_SIZE 64
> > +
> > +enum operations {
> > +	PLANE_UPDATE,
> > +	PLANE_MOVE,
> > +	OVERLAY_PRIM_UPDATE
> > +};
> > +
> > +enum plane_move_postion {
> > +	POS_TOP_LEFT,
> > +	POS_TOP_RIGHT,
> > +	POS_BOTTOM_LEFT,
> > +	POS_BOTTOM_RIGHT
> > +};
> > +
> > +typedef struct {
> > +	int drm_fd;
> > +	int debugfs_fd;
> > +	igt_display_t display;
> > +	drm_intel_bufmgr *bufmgr;
> > +	drmModeModeInfo *mode;
> > +	igt_output_t *output;
> > +	struct igt_fb fb[3];
> > +	int damage_area_count;
> > +	enum operations op;
> > +	enum plane_move_postion pos;
> > +	int test_plane_id;
> > +	igt_plane_t *test_plane;
> > +	cairo_t *cr;
> > +} data_t;
> > +
> > +static const char *op_str(enum operations op) {
> > +	static const char * const name[] = {
> > +		[PLANE_UPDATE] = "plane-update",
> > +		[PLANE_MOVE] = "plane-move",
> > +		[OVERLAY_PRIM_UPDATE] = "overlay-primary-update",
> > +	};
> > +
> > +	return name[op];
> > +}
> > +
> > +static void setup_output(data_t *data) {
> > +	igt_display_t *display = &data->display;
> > +	igt_output_t *output;
> > +	enum pipe pipe;
> > +
> > +	for_each_pipe_with_valid_output(display, pipe, output) {
> > +		drmModeConnectorPtr c = output->config.connector;
> > +
> > +		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
> > +			continue;
> > +
> > +		igt_output_set_pipe(output, pipe);
> > +		data->output = output;
> > +		data->mode = igt_output_get_mode(output);
> > +
> > +		return;
> > +	}
> > +}
> > +
> > +static void display_init(data_t *data) {
> > +	igt_display_require(&data->display, data->drm_fd);
> > +	setup_output(data);
> > +}
> > +
> > +static void display_fini(data_t *data) {
> > +	igt_display_fini(&data->display);
> > +}
> > +
> > +static void draw_rect(data_t *data, igt_fb_t *fb, int x, int y, int w, int h,
> > +			double r, double g, double b, double a) {
> > +	cairo_t *cr;
> > +
> > +	cr = igt_get_cairo_ctx(data->drm_fd, fb);
> > +	igt_paint_color_alpha(cr, x, y, w, h, r, g, b, a);
> > +	igt_put_cairo_ctx(cr);
> > +}
> > +
> > +static void plane_update_paint_squares(data_t *data, igt_fb_t *fb, uint32_t
> h,
> > +				       uint32_t v)
> > +{
> > +
> > +	switch (data->damage_area_count) {
> > +	case 5:
> > +		/*Bottom right corner*/
> > +		draw_rect(data, fb, h - SQUARE_SIZE, v - SQUARE_SIZE,
> > +			  SQUARE_SIZE, SQUARE_SIZE, 1.0, 1.0, 1.0, 1.0);
> > +	case 4:
> > +		/*Bottom left corner*/
> > +		draw_rect(data, fb, h - SQUARE_SIZE, 0,
> > +			  SQUARE_SIZE, SQUARE_SIZE,
> > +			  1.0, 1.0, 1.0, 1.0);
> > +	case 3:
> > +		/*Top right corner*/
> > +		draw_rect(data, fb, 0, v - SQUARE_SIZE,
> > +			  SQUARE_SIZE, SQUARE_SIZE, 1.0, 1.0, 1.0, 1.0);
> > +	case 2:
> > +		/*Top left corner*/
> > +		draw_rect(data, fb, 0, 0, SQUARE_SIZE, SQUARE_SIZE,
> > +			  1.0, 1.0, 1.0, 1.0);
> > +
> > +	case 1:
> > +		/*Center*/
> > +		draw_rect(data, fb, h/2 - SQUARE_SIZE/2, v/2 -
> SQUARE_SIZE/2,
> > +			  SQUARE_SIZE, SQUARE_SIZE, 1.0, 1.0, 1.0, 1.0);
> > +	}
> 
> Want to see a damage clip being set every time you draw in the fb, together
> calls. Even better if you set int x, y, w and h variables and reuse to it to call
> draw_rect() and set clips(clip[x].x2 = clip[x].x1 + w);
> 
> Also you are only setting the damage area of the places you are drawing to, you
> need also to mark the regions of the previous fb that was draw and in the new fb
> have the background color, example:
> 
> FB0
> |------|
> |O     |
> |      |
> --------
> 
> FB1
> |------|
> |      |
> |      |
> --------
> 
> When flipping to FB1 you need to mark as damaged the region that had a 0 but
> now is only background.

This scenario will not occur as new areas are drawn along with the previous ones.
Previous areas are also part of new damaged FB update.

Did I miss anything?

> 
> > +}
> > +
> > +static void plane_move_paint_square(data_t *data, igt_fb_t *fb, uint32_t h,
> > +				       uint32_t v)
> > +{
> > +
> > +	switch (data->pos) {
> > +	case POS_TOP_LEFT:
> > +		/*Bottom right corner*/
> > +		draw_rect(data, fb, h - SQUARE_SIZE, v - SQUARE_SIZE,
> > +			  SQUARE_SIZE, SQUARE_SIZE, 1.0, 1.0, 1.0, 1.0);
> > +		break;
> > +	case POS_TOP_RIGHT:
> > +		/*Bottom left corner*/
> > +		draw_rect(data, fb, 0, v - SQUARE_SIZE,
> > +			  SQUARE_SIZE, SQUARE_SIZE, 1.0, 1.0, 1.0, 1.0);
> > +		break;
> > +	case POS_BOTTOM_LEFT:
> > +		/*Top right corner*/
> > +		draw_rect(data, fb, h - SQUARE_SIZE, 0,
> > +			  SQUARE_SIZE, SQUARE_SIZE, 1.0, 1.0, 1.0, 1.0);
> > +		break;
> > +	case POS_BOTTOM_RIGHT:
> > +		/*Top left corner*/
> > +		draw_rect(data, fb, 0, 0, SQUARE_SIZE, SQUARE_SIZE,
> > +			  1.0, 1.0, 1.0, 1.0);
> > +		break;
> > +	}
> > +}
> > +
> > +static struct drm_mode_rect *plane_update_setup_damage_areas(data_t
> *data,
> > +							     uint32_t h,
> > +							     uint32_t v)
> > +{
> > +	struct drm_mode_rect *clip;
> > +
> > +	if (data->test_plane_id == DRM_PLANE_TYPE_CURSOR) {
> > +		clip = (struct drm_mode_rect *) malloc(sizeof(struct
> > +							      drm_mode_rect));
> > +		igt_assert(clip);
> > +
> > +		clip->x1 = clip->y1 = 0;
> > +		clip->x2 = clip->y2 = CUR_SIZE;
> > +
> > +	} else {
> > +		clip = (struct drm_mode_rect *) malloc(sizeof(struct
> > +							      drm_mode_rect) *
> > +						       data-
> >damage_area_count);
> 
> alloc enough clips for all tests in stack and reuse in every update.
> 
> 
> > +		igt_assert(clip);
> > +
> > +		switch (data->damage_area_count) {
> > +		case 5:
> > +			/*Bottom right corner*/
> > +			clip[4].x1 = h - SQUARE_SIZE;
> > +			clip[4].y1 = v - SQUARE_SIZE;
> > +			clip[4].x2 = clip[4].x1 + SQUARE_SIZE;
> > +			clip[4].y2 = clip[4].y1 + SQUARE_SIZE;
> > +		case 4:
> > +			/*Bottom left corner*/
> > +			clip[3].x1 = h - SQUARE_SIZE;
> > +			clip[3].y1 = 0;
> > +			clip[3].x2 = clip[3].x1 + SQUARE_SIZE;
> > +			clip[3].y2 = clip[3].y1 + SQUARE_SIZE;
> > +		case 3:
> > +			/*Top right corner*/
> > +			clip[2].x1 = 0;
> > +			clip[2].y1 = v - SQUARE_SIZE;
> > +			clip[2].x2 = clip[2].x1 + SQUARE_SIZE;
> > +			clip[2].y2 = clip[2].y1 + SQUARE_SIZE;
> > +		case 2:
> > +			/*Top left corner*/
> > +			clip[1].x1 = 0;
> > +			clip[1].y1 = 0;
> > +			clip[1].x2 = clip[1].x1 + SQUARE_SIZE;
> > +			clip[1].y2 = clip[1].y1 + SQUARE_SIZE;
> > +
> > +		case 1:
> > +			/*Center*/
> > +			clip[0].x1 = h/2 - SQUARE_SIZE/2;
> > +			clip[0].y1 = v/2 - SQUARE_SIZE/2;
> > +			clip[0].x2 = clip[0].x1 + SQUARE_SIZE;
> > +			clip[0].y2 = clip[0].y1 + SQUARE_SIZE;
> > +		}
> > +	}
> > +
> > +	return clip;
> > +}
> > +
> > +static struct drm_mode_rect *plane_move_setup_damage_area(data_t
> *data,
> > +							  uint32_t h,
> > +							  uint32_t v)
> > +{
> > +	struct drm_mode_rect *clip;
> > +
> > +	clip = (struct drm_mode_rect *) malloc(sizeof(struct drm_mode_rect));
> > +	igt_assert(clip);
> > +
> > +	switch (data->pos) {
> > +	case POS_TOP_LEFT:
> > +		/*Bottom right corner*/
> > +		clip->x1 = h - SQUARE_SIZE;
> > +		clip->y1 = v - SQUARE_SIZE;
> > +		clip->x2 = clip->x1 + SQUARE_SIZE;
> > +		clip->y2 = clip->y1 + SQUARE_SIZE;
> > +
> > +		break;
> > +	case POS_TOP_RIGHT:
> > +		/*Bottom left corner*/
> > +		clip->x1 = h - SQUARE_SIZE;
> > +		clip->y1 = 0;
> > +		clip->x2 = clip->x1 + SQUARE_SIZE;
> > +		clip->y2 = clip->y1 + SQUARE_SIZE;
> > +
> > +		break;
> > +	case POS_BOTTOM_LEFT:
> > +		/*Top right corner*/
> > +		clip->x1 = 0;
> > +		clip->y1 = v - SQUARE_SIZE;
> > +		clip->x2 = clip->x1 + SQUARE_SIZE;
> > +		clip->y2 = clip->y1 + SQUARE_SIZE;
> > +
> > +		break;
> > +	case POS_BOTTOM_RIGHT:
> > +		/*Top left corner*/
> > +		clip->x1 = 0;
> > +		clip->y1 = 0;
> > +		clip->x2 = clip->x1 + SQUARE_SIZE;
> > +		clip->y2 = clip->y1 + SQUARE_SIZE;
> > +	}
> > +
> > +	return clip;
> > +}
> > +
> > +static void free_damage_areas(struct drm_mode_rect *clip) {
> > +	if (!clip)
> > +		return;
> > +
> > +	free(clip);
> > +}
> > +
> > +static void prepare(data_t *data)
> > +{
> > +	igt_plane_t *primary, *sprite = NULL, *cursor = NULL;
> > +
> > +	/* all green frame */
> > +	igt_create_color_fb(data->drm_fd,
> > +			    data->mode->hdisplay, data->mode->vdisplay,
> > +			    DRM_FORMAT_XRGB8888,
> > +			    LOCAL_DRM_FORMAT_MOD_NONE,
> > +			    0.0, 1.0, 0.0,
> > +			    &data->fb[0]);
> > +
> > +	primary = igt_output_get_plane_type(data->output,
> > +			DRM_PLANE_TYPE_PRIMARY);
> > +
> > +	switch (data->test_plane_id) {
> > +	case DRM_PLANE_TYPE_OVERLAY:
> > +		sprite = igt_output_get_plane_type(data->output,
> > +
> DRM_PLANE_TYPE_OVERLAY);
> > +		/*All blue plane*/
> > +		igt_create_color_fb(data->drm_fd,
> > +				    data->mode->hdisplay/2,
> > +				    data->mode->vdisplay/2,
> > +				    DRM_FORMAT_XRGB8888,
> > +				    LOCAL_DRM_FORMAT_MOD_NONE,
> > +				    0.0, 0.0, 1.0,
> > +				    &data->fb[1]);
> 
> Kinda of confusing data->fb, add data->fb_primary[], data->fb_overlay[] and
> data->fb_cursor[].
> 
> > +
> > +		igt_create_color_fb(data->drm_fd,
> > +				    data->mode->hdisplay/2,
> > +				    data->mode->vdisplay/2,
> > +				    DRM_FORMAT_XRGB8888,
> > +				    LOCAL_DRM_FORMAT_MOD_NONE,
> > +				    0.0, 0.0, 1.0,
> > +				    &data->fb[2]);
> > +
> > +		if (data->op == PLANE_MOVE) {
> > +			plane_move_paint_square(data, &data->fb[2],
> > +					   data->mode->hdisplay/2,
> > +					   data->mode->vdisplay/2);
> > +
> > +		} else {
> > +			plane_update_paint_squares(data, &data->fb[2],
> > +					   data->mode->hdisplay/2,
> > +					   data->mode->vdisplay/2);
> > +		}
> > +
> > +		igt_plane_set_fb(sprite, &data->fb[1]);
> > +		data->test_plane = sprite;
> > +		break;
> > +
> > +	case DRM_PLANE_TYPE_PRIMARY:
> > +		igt_create_color_fb(data->drm_fd,
> > +			    data->mode->hdisplay, data->mode->vdisplay,
> > +			    DRM_FORMAT_XRGB8888,
> > +			    LOCAL_DRM_FORMAT_MOD_NONE,
> > +			    0.0, 1.0, 0.0,
> > +			    &data->fb[2]);
> > +
> > +		plane_update_paint_squares(data, &data->fb[2],
> > +					   data->mode->hdisplay,
> > +					   data->mode->vdisplay);
> > +		data->test_plane = primary;
> > +
> > +		if (data->op == OVERLAY_PRIM_UPDATE) {
> > +			sprite = igt_output_get_plane_type(data->output,
> > +
> DRM_PLANE_TYPE_OVERLAY);
> > +
> > +			igt_create_color_fb(data->drm_fd,
> > +					    data->mode->hdisplay,
> > +					    data->mode->vdisplay,
> > +					    DRM_FORMAT_XRGB8888,
> > +					    LOCAL_DRM_FORMAT_MOD_NONE,
> > +					    0.0, 0.0, 1.0,
> > +					    &data->fb[1]);
> > +
> > +			igt_plane_set_fb(sprite, &data->fb[1]);
> > +			igt_plane_set_prop_value(sprite, IGT_PLANE_ALPHA,
> > +						 0x6060);
> > +		}
> > +		break;
> > +
> > +	case DRM_PLANE_TYPE_CURSOR:
> > +		cursor = igt_output_get_plane_type(data->output,
> > +
> DRM_PLANE_TYPE_CURSOR);
> > +		igt_plane_set_position(cursor, 0, 0);
> > +
> > +		igt_create_fb(data->drm_fd, CUR_SIZE, CUR_SIZE,
> > +			      DRM_FORMAT_ARGB8888,
> LOCAL_DRM_FORMAT_MOD_NONE,
> > +			      &data->fb[1]);
> > +
> > +		draw_rect(data, &data->fb[1], 0, 0, CUR_SIZE, CUR_SIZE,
> > +			    0.0, 0.0, 1.0, 1.0);
> > +
> > +		igt_create_fb(data->drm_fd, CUR_SIZE, CUR_SIZE,
> > +			      DRM_FORMAT_ARGB8888,
> LOCAL_DRM_FORMAT_MOD_NONE,
> > +			      &data->fb[2]);
> > +
> > +		draw_rect(data, &data->fb[2], 0, 0, CUR_SIZE, CUR_SIZE,
> > +			    1.0, 1.0, 1.0, 1.0);
> > +
> > +		igt_plane_set_fb(cursor, &data->fb[1]);
> > +		data->test_plane = cursor;
> > +		break;
> > +	}
> > +
> > +	igt_plane_set_fb(primary, &data->fb[0]);
> > +
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC); }
> > +
> > +static inline void manual(const char *expected) {
> > +	igt_debug_manual_check("all", expected); }
> > +
> > +static void plane_update_expected_output(int plane_type, int
> > +box_count) {
> > +	char expected[64] = {};
> > +
> > +	switch (plane_type) {
> > +	case DRM_PLANE_TYPE_PRIMARY:
> > +		sprintf(expected, "screen Green with %d White box(es)",
> > +			box_count);
> > +		break;
> > +	case DRM_PLANE_TYPE_OVERLAY:
> > +		sprintf(expected,
> > +			"screen Green with Blue box and %d White box(es)",
> > +			box_count);
> > +		break;
> > +	case DRM_PLANE_TYPE_CURSOR:
> > +		sprintf(expected, "screen Green with %d White box(es)",
> > +			box_count);
> > +		break;
> > +	}
> > +
> > +	manual(expected);
> > +}
> > +
> > +static void plane_move_expected_output(enum plane_move_postion pos) {
> > +	char expected[64] = {};
> > +
> > +	switch (pos) {
> > +	case POS_TOP_LEFT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on top left corner and
> White box");
> > +		break;
> > +	case POS_TOP_RIGHT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on top right corner and
> White box");
> > +		break;
> > +	case POS_BOTTOM_LEFT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on bottom left corner and
> White box");
> > +		break;
> > +	case POS_BOTTOM_RIGHT:
> > +		sprintf(expected,
> > +			"screen Green with Blue box on bottom right corner
> and White box");
> > +		break;
> > +	}
> > +
> > +	manual(expected);
> > +}
> > +
> > +static void overlay_prim_update_expected_output(int box_count) {
> > +	char expected[64] = {};
> > +
> > +	sprintf(expected,
> > +		"screen Green with Blue overlay, %d light Blue box(es)",
> > +		box_count);
> > +
> > +	manual(expected);
> > +
> > +}
> > +
> > +static void expected_output(data_t *data) {
> > +	switch (data->op) {
> > +	case PLANE_MOVE:
> > +		plane_move_expected_output(data->pos);
> > +		break;
> > +	case PLANE_UPDATE:
> > +		plane_update_expected_output(data->test_plane_id,
> > +					     data->damage_area_count);
> > +		break;
> > +	case OVERLAY_PRIM_UPDATE:
> > +		overlay_prim_update_expected_output(data-
> >damage_area_count);
> > +		break;
> > +	}
> > +}
> > +
> > +static void damaged_plane_move(data_t *data) {
> > +	igt_plane_t *test_plane = data->test_plane;
> > +	struct drm_mode_rect *clip;
> > +	uint32_t h = data->mode->hdisplay;
> > +	uint32_t v = data->mode->vdisplay;
> > +
> > +	igt_plane_set_fb(test_plane, &data->fb[2]);
> > +
> > +	if (data->test_plane_id == DRM_PLANE_TYPE_OVERLAY) {
> > +		h = h/2;
> > +		v = v/2;
> > +	}
> > +
> > +	clip = plane_move_setup_damage_area(data, h, v);
> 
> This property is in relation to the plane, this damage areas you are setting in the
> function makes no sense.

If I understood correctly after discussion with GG,  we can setup the damaged 
areas in plane w.r.t plane co-ordinates irrespective  set the plane position

Is this correct?

> When moving a plane or set the whole visible plane area as damaged or
> nothing(it should be one of the cases handled by driver).

I did not quite get it. Are you saying to setup damage area for the entire
overplay plane where it is being moved?  Will you please share example.

> 
> > +	igt_plane_replace_prop_blob(test_plane,
> IGT_PLANE_FB_DAMAGE_CLIPS, clip,
> > +				    sizeof(struct drm_mode_rect));
> > +
> > +	switch (data->pos) {
> > +	case POS_TOP_LEFT:
> > +		igt_plane_set_position(data->test_plane, 0, 0);
> > +		break;
> > +	case POS_TOP_RIGHT:
> > +		igt_plane_set_position(data->test_plane,
> > +				       data->mode->hdisplay/2, 0);
> > +		break;
> > +	case POS_BOTTOM_LEFT:
> > +		igt_plane_set_position(data->test_plane, 0,
> > +				       data->mode->vdisplay/2);
> > +		break;
> > +	case POS_BOTTOM_RIGHT:
> > +		igt_plane_set_position(data->test_plane,
> > +				       data->mode->hdisplay/2,
> > +				       data->mode->vdisplay/2);
> > +		break;
> > +	}
> > +
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +
> > +	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > +
> > +	expected_output(data);
> > +
> > +	free_damage_areas(clip);
> > +}
> > +
> > +static void damaged_plane_update(data_t *data) {
> > +	igt_plane_t *test_plane = data->test_plane;
> > +	struct drm_mode_rect *clip;
> > +	uint32_t h = data->mode->hdisplay;
> > +	uint32_t v = data->mode->vdisplay;
> > +
> > +	igt_plane_set_fb(test_plane, &data->fb[2]);
> > +
> > +	if (data->test_plane_id == DRM_PLANE_TYPE_OVERLAY) {
> > +		h = h/2;
> > +		v = v/2;
> > +	}
> > +
> > +	clip = plane_update_setup_damage_areas(data, h, v);
> > +	igt_plane_replace_prop_blob(test_plane,
> IGT_PLANE_FB_DAMAGE_CLIPS, clip,
> > +				    sizeof(struct drm_mode_rect)*
> > +				    data->damage_area_count);
> > +	igt_plane_set_position(data->test_plane, 0, 0);
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +
> > +	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > +
> > +	expected_output(data);
> > +
> > +	free_damage_areas(clip);
> > +}
> > +
> > +static void update_screen_and_test(data_t *data) {
> > +	switch (data->op) {
> > +	case PLANE_UPDATE:
> > +	case OVERLAY_PRIM_UPDATE:
> > +		damaged_plane_update(data);
> > +		break;
> > +	case PLANE_MOVE:
> > +		damaged_plane_move(data);
> > +		break;
> > +	}
> > +}
> > +
> > +static void screen_reset(data_t  *data) {
> > +	igt_plane_t *test_plane = data->test_plane;
> > +
> > +	if (data->test_plane_id == DRM_PLANE_TYPE_PRIMARY)
> > +		igt_plane_set_fb(test_plane, &data->fb[0]);
> > +	else
> > +		igt_plane_set_fb(test_plane, &data->fb[1]);
> > +
> > +	igt_plane_set_position(data->test_plane, 0, 0);
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC); }
> > +
> > +static void run(data_t *data)
> > +{
> > +	igt_assert(psr_wait_entry(data->debugfs_fd, PSR_MODE_2));
> > +
> > +	screen_reset(data);
> 
> why you need this if you have committed already in the prepare()?

Yeah will remove it.
Was experimenting with loop for with multiple screen changes.

> 
> > +	update_screen_and_test(data);
> 
> Content of this function could be moved to here.

Yes.

> 
> 
> > +}
> > +
> > +static void cleanup(data_t *data)
> > +{
> > +	igt_plane_t *primary;
> > +	igt_plane_t *sprite;
> > +
> > +	primary = igt_output_get_plane_type(data->output,
> > +					    DRM_PLANE_TYPE_PRIMARY);
> > +
> > +	igt_plane_set_fb(primary, NULL);
> > +
> > +	if (data->test_plane_id != DRM_PLANE_TYPE_PRIMARY) {
> > +		igt_plane_set_position(data->test_plane, 0, 0);
> > +		igt_plane_set_fb(data->test_plane, NULL);
> > +	}
> > +
> > +	if (data->op == OVERLAY_PRIM_UPDATE) {
> > +		sprite = igt_output_get_plane_type(data->output,
> > +				DRM_PLANE_TYPE_OVERLAY);
> > +		igt_plane_set_position(sprite, 0, 0);
> > +		igt_plane_set_fb(sprite, NULL);
> > +	}
> > +
> > +	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > +
> > +	igt_remove_fb(data->drm_fd, &data->fb[0]);
> > +	igt_remove_fb(data->drm_fd, &data->fb[1]);
> > +	igt_remove_fb(data->drm_fd, &data->fb[2]); }
> > +
> > +igt_main
> > +{
> > +	data_t data = {};
> > +	int i;
> > +
> > +	igt_fixture {
> > +		int r;
> > +
> > +		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
> > +		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
> > +		kmstest_set_vt_graphics_mode();
> > +
> > +		igt_require_f(psr_sink_support(data.drm_fd,
> > +					       data.debugfs_fd, PSR_MODE_2),
> > +			      "Sink does not support PSR2\n");
> > +
> > +		igt_require_f(psr2_selective_fetch_check(data.debugfs_fd),
> > +			      "PSR2 selective fetch not enabled\n");
> > +
> > +		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
> > +		igt_assert(data.bufmgr);
> > +		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > +
> > +		display_init(&data);
> > +
> > +		/* Test if PSR2 can be enabled */
> > +		igt_require_f(psr_enable(data.drm_fd,
> > +					 data.debugfs_fd, PSR_MODE_2),
> > +			      "Error enabling PSR2\n");
> > +
> > +		data.damage_area_count = 5;
> > +		data.op = PLANE_UPDATE;
> > +		data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > +		prepare(&data);
> > +		r = psr_wait_entry(data.debugfs_fd, PSR_MODE_2);
> > +		cleanup(&data);
> > +		if (!r)
> > +			psr_print_debugfs(data.debugfs_fd);
> > +		igt_require_f(r, "PSR2 can not be enabled\n");
> > +	}
> > +
> 
> Missing some description of what each subtest will, also a better name.
> What we will see in CI would be: primary-plane-update-sf-5, 5 what?

Any suggestions here?

Thanks,
Pankaj

> 
> > +	for (i = 1; i <= 5; i++) {
> > +		igt_subtest_f("primary-%s-sf-%d", op_str(data.op), i) {
> > +			data.damage_area_count = i;
> > +			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > +			prepare(&data);
> > +			run(&data);
> > +			cleanup(&data);
> > +		}
> > +	}
> > +
> > +	for (i = 1; i <= 5; i++) {
> > +		igt_subtest_f("overlay-%s-sf-%d", op_str(data.op), i) {
> > +			data.damage_area_count = i;
> > +			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > +			prepare(&data);
> > +			run(&data);
> > +			cleanup(&data);
> > +		}
> > +	}
> > +
> > +	igt_subtest_f("cursor-%s-sf", op_str(data.op)) {
> > +		data.damage_area_count = 1;
> > +		data.test_plane_id = DRM_PLANE_TYPE_CURSOR;
> > +		prepare(&data);
> > +		run(&data);
> > +		cleanup(&data);
> > +	}
> > +
> > +	/* Only for overlay plane */
> > +	data.op = PLANE_MOVE;
> > +	for (i = POS_TOP_LEFT; i <= POS_BOTTOM_RIGHT ; i++) {
> > +		igt_subtest_f("%s-sf-%d", op_str(data.op), i) {
> > +			data.pos = i;
> > +			data.test_plane_id = DRM_PLANE_TYPE_OVERLAY;
> > +			prepare(&data);
> > +			run(&data);
> > +			cleanup(&data);
> > +		}
> > +	}
> > +
> > +	data.op = OVERLAY_PRIM_UPDATE;
> > +	for (i = 1; i <= 5; i++) {
> > +		igt_subtest_f("%s-sf-%d", op_str(data.op), i) {
> > +			data.damage_area_count = i;
> > +			data.test_plane_id = DRM_PLANE_TYPE_PRIMARY;
> > +			prepare(&data);
> > +			run(&data);
> > +			cleanup(&data);
> > +		}
> > +	}
> > +
> > +	igt_fixture {
> > +		close(data.debugfs_fd);
> > +		drm_intel_bufmgr_destroy(data.bufmgr);
> > +		display_fini(&data);
> > +	}
> > +}
> > diff --git a/tests/meson.build b/tests/meson.build index
> > cc79ac228..b33cfb63f 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -63,6 +63,7 @@ test_progs = [
> >  	'kms_properties',
> >  	'kms_psr',
> >  	'kms_psr2_su',
> > +	'kms_psr2_sf',
> >  	'kms_pwrite_crc',
> >  	'kms_rmfb',
> >  	'kms_rotation_crc',

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