[2/2] tests/pm_rpm: add planes subtests

Commit Message

Paulo Zanoni July 28, 2014, 6:37 p.m. UTC

From: Paulo Zanoni <[e-mail]>

Just like the cursor subtests, these also trigger WARNs on the current
Kernel.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 211 insertions(+), 1 deletion(-)

Comments

Matt Roper July 28, 2014, 11:47 p.m. UTC | #1

On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> Just like the cursor subtests, these also trigger WARNs on the current
> Kernel.
> 
> Signed-off-by: Paulo Zanoni <[e-mail]>

I feel like a lot of the setup you have to do here is duplicating logic
we have in the igt_kms library.  Was there functionality lacking from
that library that prevented you from using it to write this test?  If
so, I can look into adding it.

Anyway, the test still looks good, just one or two minor comments inline
below.

> ---
>  tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 211 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> index f720f86..048d9ad 100644
> --- a/tests/pm_rpm.c
> +++ b/tests/pm_rpm.c
> @@ -51,6 +51,9 @@
>  #include "igt_kms.h"
>  #include "igt_debugfs.h"
>  
> +/* One day, this will be on your libdrm. */
> +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2

I think there was just an official release of libdrm today that finally
includes this and the plane type enum below...maybe we can just bump the
libdrm requirement for igt and stop including these by hand?

> +
>  #define MSR_PC8_RES	0x630
>  #define MSR_PC9_RES	0x631
>  #define MSR_PC10_RES	0x632
> @@ -72,6 +75,12 @@ enum screen_type {
>  	SCREEN_TYPE_ANY,
>  };
>  
> +enum plane_type {
> +	PLANE_OVERLAY,
> +	PLANE_PRIMARY,
> +	PLANE_CURSOR,
> +};
> +
>  /* Wait flags */
>  #define DONT_WAIT	0
>  #define WAIT_STATUS	1
> @@ -1504,6 +1513,199 @@ static void cursor_subtest(bool dpms)
>  	igt_assert(wait_for_suspended());
>  }
>  
> +static enum plane_type get_plane_type(uint32_t plane_id)
> +{
> +	drmModeObjectPropertiesPtr props;
> +	int i, j;
> +	enum plane_type type;
> +	bool found = false;
> +
> +	props = drmModeObjectGetProperties(drm_fd, plane_id,
> +					   DRM_MODE_OBJECT_PLANE);
> +	igt_assert(props);
> +
> +	for (i = 0; i < props->count_props && !found; i++) {
> +		drmModePropertyPtr prop;
> +		const char *enum_name = NULL;
> +
> +		prop = drmModeGetProperty(drm_fd, props->props[i]);
> +		igt_assert(prop);
> +
> +		if (strcmp(prop->name, "type") == 0) {
> +			igt_assert(prop->flags & DRM_MODE_PROP_ENUM);
> +			igt_assert(props->prop_values[i] < prop->count_enums);
> +
> +			for (j = 0; j < prop->count_enums; j++) {
> +				if (prop->enums[j].value ==
> +				    props->prop_values[i]) {
> +					enum_name = prop->enums[j].name;
> +					break;
> +				}
> +			}
> +			igt_assert(enum_name);
> +
> +			if (strcmp(enum_name, "Overlay") == 0)
> +				type = PLANE_OVERLAY;
> +			else if (strcmp(enum_name, "Primary") == 0)
> +				type = PLANE_PRIMARY;
> +			else if (strcmp(enum_name, "Cursor") == 0)
> +				type = PLANE_CURSOR;
> +			else
> +				igt_assert(0);
> +
> +			found = true;
> +		}
> +
> +		drmModeFreeProperty(prop);
> +	}
> +	igt_assert(found);
> +
> +	drmModeFreeObjectProperties(props);
> +	return type;
> +}
> +
> +static void test_one_plane(bool dpms, uint32_t plane_id,
> +			   enum plane_type plane_type)
> +{
> +	int rc;
> +	uint32_t plane_format, plane_w, plane_h;
> +	uint32_t crtc_id, connector_id;
> +	struct igt_fb scanout_fb, plane_fb1, plane_fb2;
> +	drmModeModeInfoPtr mode;
> +	int32_t crtc_x = 0, crtc_y = 0;
> +
> +	disable_all_screens(&ms_data);
> +	igt_assert(wait_for_suspended());
> +
> +	igt_require(find_connector_for_modeset(&ms_data, SCREEN_TYPE_ANY,
> +					       &connector_id, &mode));
> +
> +	crtc_id = ms_data.res->crtcs[0];
> +	igt_assert(crtc_id);
> +
> +	igt_create_fb(drm_fd, mode->hdisplay, mode->vdisplay,
> +		      DRM_FORMAT_XRGB8888, false, &scanout_fb);
> +
> +	fill_igt_fb(&scanout_fb, 0xFF);
> +
> +	switch (plane_type) {
> +	case PLANE_OVERLAY:
> +		plane_format = DRM_FORMAT_XRGB8888;
> +		plane_w = 64;
> +		plane_h = 64;
> +		break;
> +	case PLANE_PRIMARY:
> +		plane_format = DRM_FORMAT_XRGB8888;
> +		plane_w = mode->hdisplay;
> +		plane_h = mode->vdisplay;
> +		break;
> +	case PLANE_CURSOR:
> +		plane_format = DRM_FORMAT_ARGB8888;
> +		plane_w = 64;
> +		plane_h = 64;
> +		break;
> +	default:
> +		igt_assert(0);
> +		break;
> +	}
> +
> +	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
> +		      &plane_fb1);
> +	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
> +		      &plane_fb2);
> +	fill_igt_fb(&plane_fb1, 0xFF00FFFF);
> +	fill_igt_fb(&plane_fb2, 0xFF00FF00);
> +
> +	rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0,
> +			    &connector_id, 1, mode);
> +	igt_assert(rc == 0);
> +	igt_assert(wait_for_active());
> +
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
> +			     0, 0, plane_fb1.width, plane_fb1.height,
> +			     0 << 16, 0 << 16, plane_fb1.width << 16,
> +			     plane_fb1.height << 16);
> +	igt_assert(rc == 0);
> +
> +	if (dpms)
> +		disable_all_screens_dpms(&ms_data);
> +	else
> +		disable_all_screens(&ms_data);
> +	igt_assert(wait_for_suspended());
> +
> +	/* Just move the plane around. */
> +	if (plane_type != PLANE_PRIMARY) {
> +		crtc_x++;
> +		crtc_y++;
> +	}
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
> +			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
> +			     0 << 16, 0 << 16, plane_fb1.width << 16,
> +			     plane_fb1.height << 16);
> +	igt_assert(rc == 0);
> +
> +	/* Unset, then change the plane. */
> +	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
> +	igt_assert(rc == 0);
> +
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb2.fb_id, 0,
> +			     crtc_x, crtc_y, plane_fb2.width, plane_fb2.height,
> +			     0 << 16, 0 << 16, plane_fb2.width << 16,
> +			     plane_fb2.height << 16);
> +	igt_assert(rc == 0);
> +
> +	/* Now change the plane without unsetting first. */
> +	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
> +			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
> +			     0 << 16, 0 << 16, plane_fb1.width << 16,
> +			     plane_fb1.height << 16);
> +	igt_assert(rc == 0);
> +
> +	/* Make sure nothing remains for the other tests. */
> +	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
> +	igt_assert(rc == 0);
> +}
> +
> +/* This one also triggered WARNs on our driver at some point in time. */
> +static void planes_subtest(bool universal, bool dpms)
> +{
> +	int i, rc, planes_tested = 0;
> +	drmModePlaneResPtr planes;
> +
> +	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
> +			     universal);
> +	igt_require(rc == 0);

I think you want to make the setcap call conditional on universal.  If
we're running on an older kernel (pre-universal planes), the setcap
ioctl will return -EINVAL since it doesn't recognize
DRM_CLIENT_CAP_UNIVERSAL_PLANES and you'll never even get to try your
non-universal tests.

(I assume you still want to run on older kernels since otherwise I don't
think there'd be a need to test legacy and universal
separately...universal just adds extra stuff to the plane list, but you
still get all the same legacy planes in the list too.)


Matt

> +
> +	planes = drmModeGetPlaneResources(drm_fd);
> +	for (i = 0; i < planes->count_planes; i++) {
> +		drmModePlanePtr plane;
> +
> +		plane = drmModeGetPlane(drm_fd, planes->planes[i]);
> +		igt_assert(plane);
> +
> +		/* We just pick the first CRTC on the list, so we can test for
> +		 * 0x1 as the index. */
> +		if (plane->possible_crtcs & 0x1) {
> +			enum plane_type type;
> +
> +			type = universal ? get_plane_type(plane->plane_id) :
> +					   PLANE_OVERLAY;
> +			test_one_plane(dpms, plane->plane_id, type);
> +			planes_tested++;
> +		}
> +		drmModeFreePlane(plane);
> +	}
> +	drmModeFreePlaneResources(planes);
> +
> +	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> +	igt_assert(rc == 0);
> +
> +	if (universal)
> +		igt_assert(planes_tested >= 3);
> +	else
> +		igt_assert(planes_tested >= 1);
> +}
> +
>  int rounds = 50;
>  bool stay = false;
>  
> @@ -1577,11 +1779,19 @@ int main(int argc, char *argv[])
>  	igt_subtest("gem-idle")
>  		gem_idle_subtest();
>  
> -	/* Cursors */
> +	/* Planes and cursors */
>  	igt_subtest("cursor")
>  		cursor_subtest(false);
>  	igt_subtest("cursor-dpms")
>  		cursor_subtest(true);
> +	igt_subtest("legacy-planes")
> +		planes_subtest(false, false);
> +	igt_subtest("legacy-planes-dpms")
> +		planes_subtest(false, true);
> +	igt_subtest("universal-planes")
> +		planes_subtest(true, false);
> +	igt_subtest("universal-planes-dpms")
> +		planes_subtest(true, true);
>  
>  	/* Misc */
>  	igt_subtest("reg-read-ioctl")
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Paulo Zanoni Aug. 5, 2014, 9:34 p.m. UTC | #2

2014-07-28 20:47 GMT-03:00 Matt Roper <[e-mail]>:
> On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <[e-mail]>
>>
>> Just like the cursor subtests, these also trigger WARNs on the current
>> Kernel.
>>
>> Signed-off-by: Paulo Zanoni <[e-mail]>
>
> I feel like a lot of the setup you have to do here is duplicating logic
> we have in the igt_kms library.  Was there functionality lacking from
> that library that prevented you from using it to write this test?  If
> so, I can look into adding it.

Every single ioctl we call can result in runtime PM get/put calls
inside the driver, so for pm_rpm.c I would like to keep using the low
level interfaces, to make sure the suspends and resumes are
controlled.

Anyway, I never really looked at the library before. It seems the
biggest functionality missing from it is documentation. I just took a
look at the .c file and couldn't find anything that looked like would
reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
very simple. Any suggestions?

>
> Anyway, the test still looks good, just one or two minor comments inline
> below.
>
>> ---
>>  tests/pm_rpm.c | 212 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 211 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
>> index f720f86..048d9ad 100644
>> --- a/tests/pm_rpm.c
>> +++ b/tests/pm_rpm.c
>> @@ -51,6 +51,9 @@
>>  #include "igt_kms.h"
>>  #include "igt_debugfs.h"
>>
>> +/* One day, this will be on your libdrm. */
>> +#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
>
> I think there was just an official release of libdrm today that finally
> includes this and the plane type enum below...maybe we can just bump the
> libdrm requirement for igt and stop including these by hand?

I just noticed igt_kms.c also has this. I also have to be honest and
tell you that I get extremely annoyed when we bump the IGT
requirements to recently-released libdrm :)

>
>> +
>>  #define MSR_PC8_RES  0x630
>>  #define MSR_PC9_RES  0x631
>>  #define MSR_PC10_RES 0x632
>> @@ -72,6 +75,12 @@ enum screen_type {
>>       SCREEN_TYPE_ANY,
>>  };
>>
>> +enum plane_type {
>> +     PLANE_OVERLAY,
>> +     PLANE_PRIMARY,
>> +     PLANE_CURSOR,
>> +};
>> +
>>  /* Wait flags */
>>  #define DONT_WAIT    0
>>  #define WAIT_STATUS  1
>> @@ -1504,6 +1513,199 @@ static void cursor_subtest(bool dpms)
>>       igt_assert(wait_for_suspended());
>>  }
>>
>> +static enum plane_type get_plane_type(uint32_t plane_id)
>> +{
>> +     drmModeObjectPropertiesPtr props;
>> +     int i, j;
>> +     enum plane_type type;
>> +     bool found = false;
>> +
>> +     props = drmModeObjectGetProperties(drm_fd, plane_id,
>> +                                        DRM_MODE_OBJECT_PLANE);
>> +     igt_assert(props);
>> +
>> +     for (i = 0; i < props->count_props && !found; i++) {
>> +             drmModePropertyPtr prop;
>> +             const char *enum_name = NULL;
>> +
>> +             prop = drmModeGetProperty(drm_fd, props->props[i]);
>> +             igt_assert(prop);
>> +
>> +             if (strcmp(prop->name, "type") == 0) {
>> +                     igt_assert(prop->flags & DRM_MODE_PROP_ENUM);
>> +                     igt_assert(props->prop_values[i] < prop->count_enums);
>> +
>> +                     for (j = 0; j < prop->count_enums; j++) {
>> +                             if (prop->enums[j].value ==
>> +                                 props->prop_values[i]) {
>> +                                     enum_name = prop->enums[j].name;
>> +                                     break;
>> +                             }
>> +                     }
>> +                     igt_assert(enum_name);
>> +
>> +                     if (strcmp(enum_name, "Overlay") == 0)
>> +                             type = PLANE_OVERLAY;
>> +                     else if (strcmp(enum_name, "Primary") == 0)
>> +                             type = PLANE_PRIMARY;
>> +                     else if (strcmp(enum_name, "Cursor") == 0)
>> +                             type = PLANE_CURSOR;
>> +                     else
>> +                             igt_assert(0);
>> +
>> +                     found = true;
>> +             }
>> +
>> +             drmModeFreeProperty(prop);
>> +     }
>> +     igt_assert(found);
>> +
>> +     drmModeFreeObjectProperties(props);
>> +     return type;
>> +}
>> +
>> +static void test_one_plane(bool dpms, uint32_t plane_id,
>> +                        enum plane_type plane_type)
>> +{
>> +     int rc;
>> +     uint32_t plane_format, plane_w, plane_h;
>> +     uint32_t crtc_id, connector_id;
>> +     struct igt_fb scanout_fb, plane_fb1, plane_fb2;
>> +     drmModeModeInfoPtr mode;
>> +     int32_t crtc_x = 0, crtc_y = 0;
>> +
>> +     disable_all_screens(&ms_data);
>> +     igt_assert(wait_for_suspended());
>> +
>> +     igt_require(find_connector_for_modeset(&ms_data, SCREEN_TYPE_ANY,
>> +                                            &connector_id, &mode));
>> +
>> +     crtc_id = ms_data.res->crtcs[0];
>> +     igt_assert(crtc_id);
>> +
>> +     igt_create_fb(drm_fd, mode->hdisplay, mode->vdisplay,
>> +                   DRM_FORMAT_XRGB8888, false, &scanout_fb);
>> +
>> +     fill_igt_fb(&scanout_fb, 0xFF);
>> +
>> +     switch (plane_type) {
>> +     case PLANE_OVERLAY:
>> +             plane_format = DRM_FORMAT_XRGB8888;
>> +             plane_w = 64;
>> +             plane_h = 64;
>> +             break;
>> +     case PLANE_PRIMARY:
>> +             plane_format = DRM_FORMAT_XRGB8888;
>> +             plane_w = mode->hdisplay;
>> +             plane_h = mode->vdisplay;
>> +             break;
>> +     case PLANE_CURSOR:
>> +             plane_format = DRM_FORMAT_ARGB8888;
>> +             plane_w = 64;
>> +             plane_h = 64;
>> +             break;
>> +     default:
>> +             igt_assert(0);
>> +             break;
>> +     }
>> +
>> +     igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
>> +                   &plane_fb1);
>> +     igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
>> +                   &plane_fb2);
>> +     fill_igt_fb(&plane_fb1, 0xFF00FFFF);
>> +     fill_igt_fb(&plane_fb2, 0xFF00FF00);
>> +
>> +     rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0,
>> +                         &connector_id, 1, mode);
>> +     igt_assert(rc == 0);
>> +     igt_assert(wait_for_active());
>> +
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
>> +                          0, 0, plane_fb1.width, plane_fb1.height,
>> +                          0 << 16, 0 << 16, plane_fb1.width << 16,
>> +                          plane_fb1.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     if (dpms)
>> +             disable_all_screens_dpms(&ms_data);
>> +     else
>> +             disable_all_screens(&ms_data);
>> +     igt_assert(wait_for_suspended());
>> +
>> +     /* Just move the plane around. */
>> +     if (plane_type != PLANE_PRIMARY) {
>> +             crtc_x++;
>> +             crtc_y++;
>> +     }
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
>> +                          crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
>> +                          0 << 16, 0 << 16, plane_fb1.width << 16,
>> +                          plane_fb1.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     /* Unset, then change the plane. */
>> +     rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
>> +     igt_assert(rc == 0);
>> +
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb2.fb_id, 0,
>> +                          crtc_x, crtc_y, plane_fb2.width, plane_fb2.height,
>> +                          0 << 16, 0 << 16, plane_fb2.width << 16,
>> +                          plane_fb2.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     /* Now change the plane without unsetting first. */
>> +     rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
>> +                          crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
>> +                          0 << 16, 0 << 16, plane_fb1.width << 16,
>> +                          plane_fb1.height << 16);
>> +     igt_assert(rc == 0);
>> +
>> +     /* Make sure nothing remains for the other tests. */
>> +     rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
>> +     igt_assert(rc == 0);
>> +}
>> +
>> +/* This one also triggered WARNs on our driver at some point in time. */
>> +static void planes_subtest(bool universal, bool dpms)
>> +{
>> +     int i, rc, planes_tested = 0;
>> +     drmModePlaneResPtr planes;
>> +
>> +     rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
>> +                          universal);
>> +     igt_require(rc == 0);
>
> I think you want to make the setcap call conditional on universal.  If
> we're running on an older kernel (pre-universal planes), the setcap
> ioctl will return -EINVAL since it doesn't recognize
> DRM_CLIENT_CAP_UNIVERSAL_PLANES and you'll never even get to try your
> non-universal tests.
>
> (I assume you still want to run on older kernels since otherwise I don't
> think there'd be a need to test legacy and universal
> separately...universal just adds extra stuff to the plane list, but you
> still get all the same legacy planes in the list too.)

You're right. I'll fix this. Thanks!

>
>
> Matt
>
>> +
>> +     planes = drmModeGetPlaneResources(drm_fd);
>> +     for (i = 0; i < planes->count_planes; i++) {
>> +             drmModePlanePtr plane;
>> +
>> +             plane = drmModeGetPlane(drm_fd, planes->planes[i]);
>> +             igt_assert(plane);
>> +
>> +             /* We just pick the first CRTC on the list, so we can test for
>> +              * 0x1 as the index. */
>> +             if (plane->possible_crtcs & 0x1) {
>> +                     enum plane_type type;
>> +
>> +                     type = universal ? get_plane_type(plane->plane_id) :
>> +                                        PLANE_OVERLAY;
>> +                     test_one_plane(dpms, plane->plane_id, type);
>> +                     planes_tested++;
>> +             }
>> +             drmModeFreePlane(plane);
>> +     }
>> +     drmModeFreePlaneResources(planes);
>> +
>> +     rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
>> +     igt_assert(rc == 0);
>> +
>> +     if (universal)
>> +             igt_assert(planes_tested >= 3);
>> +     else
>> +             igt_assert(planes_tested >= 1);
>> +}
>> +
>>  int rounds = 50;
>>  bool stay = false;
>>
>> @@ -1577,11 +1779,19 @@ int main(int argc, char *argv[])
>>       igt_subtest("gem-idle")
>>               gem_idle_subtest();
>>
>> -     /* Cursors */
>> +     /* Planes and cursors */
>>       igt_subtest("cursor")
>>               cursor_subtest(false);
>>       igt_subtest("cursor-dpms")
>>               cursor_subtest(true);
>> +     igt_subtest("legacy-planes")
>> +             planes_subtest(false, false);
>> +     igt_subtest("legacy-planes-dpms")
>> +             planes_subtest(false, true);
>> +     igt_subtest("universal-planes")
>> +             planes_subtest(true, false);
>> +     igt_subtest("universal-planes-dpms")
>> +             planes_subtest(true, true);
>>
>>       /* Misc */
>>       igt_subtest("reg-read-ioctl")
>> --
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> [e-mail]
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]

Matt Roper Aug. 5, 2014, 9:51 p.m. UTC | #3

On Tue, Aug 05, 2014 at 06:34:38PM -0300, Paulo Zanoni wrote:
> 2014-07-28 20:47 GMT-03:00 Matt Roper <[e-mail]>:
> > On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <[e-mail]>
> >>
> >> Just like the cursor subtests, these also trigger WARNs on the current
> >> Kernel.
> >>
> >> Signed-off-by: Paulo Zanoni <[e-mail]>
> >
> > I feel like a lot of the setup you have to do here is duplicating logic
> > we have in the igt_kms library.  Was there functionality lacking from
> > that library that prevented you from using it to write this test?  If
> > so, I can look into adding it.
> 
> Every single ioctl we call can result in runtime PM get/put calls
> inside the driver, so for pm_rpm.c I would like to keep using the low
> level interfaces, to make sure the suspends and resumes are
> controlled.
> 
> Anyway, I never really looked at the library before. It seems the
> biggest functionality missing from it is documentation. I just took a
> look at the .c file and couldn't find anything that looked like would
> reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
> very simple. Any suggestions?

The main areas where I thought it might be possible to slim down a bit
by using igt_kms were all the setup code --- grabbing plane resources,
counting/looping over planes, grabbing properties to check plane types,
etc.  igt_kms will build up the plane list internally and hide a lot of
that long, boring code from your tests.  But since you've already
written the test without it, I don't feel there's any major need to
rewrite it with igt_kms; I was just curious if there was anything
specific you thought was lacking from the library so that we could
address it in the future.

I did add some kerneldoc comments when I added the new interfaces in
preparation for universal planes & atomic modeset, but you're right that
there's still a lot that could be better documented going forward.


Matt

Paulo Zanoni Aug. 6, 2014, 2:11 p.m. UTC | #4

2014-08-05 18:51 GMT-03:00 Matt Roper <[e-mail]>:
> On Tue, Aug 05, 2014 at 06:34:38PM -0300, Paulo Zanoni wrote:
>> 2014-07-28 20:47 GMT-03:00 Matt Roper <[e-mail]>:
>> > On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <[e-mail]>
>> >>
>> >> Just like the cursor subtests, these also trigger WARNs on the current
>> >> Kernel.
>> >>
>> >> Signed-off-by: Paulo Zanoni <[e-mail]>
>> >
>> > I feel like a lot of the setup you have to do here is duplicating logic
>> > we have in the igt_kms library.  Was there functionality lacking from
>> > that library that prevented you from using it to write this test?  If
>> > so, I can look into adding it.
>>
>> Every single ioctl we call can result in runtime PM get/put calls
>> inside the driver, so for pm_rpm.c I would like to keep using the low
>> level interfaces, to make sure the suspends and resumes are
>> controlled.
>>
>> Anyway, I never really looked at the library before. It seems the
>> biggest functionality missing from it is documentation. I just took a
>> look at the .c file and couldn't find anything that looked like would
>> reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
>> very simple. Any suggestions?
>
> The main areas where I thought it might be possible to slim down a bit
> by using igt_kms were all the setup code --- grabbing plane resources,
> counting/looping over planes, grabbing properties to check plane types,
> etc.  igt_kms will build up the plane list internally and hide a lot of
> that long, boring code from your tests.  But since you've already
> written the test without it, I don't feel there's any major need to
> rewrite it with igt_kms; I was just curious if there was anything
> specific you thought was lacking from the library so that we could
> address it in the future.

The big problem I see is that all/most functions take the
"igt_display_t" type as an argument instead of taking plain libdrm
types, so either you fully adopt the API, or you don't use it at all.
To be able to use igt_kms at all I'd have to call igt_display_init(),
which does way too much stuff, and is probably going to grow more, and
at some point do something that gets too many power refcounts and
breaks runtime PM.

I would really love if the igt_kms functions were little helpers that
accepted the plain libdrm types as arguments instead of its own types.
This way, I would, for example, probably be able to reuse
get_plane_property() or other functions. I see that on a lot of cases,
we pass igt_display_t just to use the FD, so why not just use the FD?

I'll try to write some patches to reuse the stuff I want to reuse,
then you can comment on them.

>
> I did add some kerneldoc comments when I added the new interfaces in
> preparation for universal planes & atomic modeset, but you're right that
> there's still a lot that could be better documented going forward.
>
>
> Matt
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> [phone]

Daniel Vetter Aug. 6, 2014, 2:23 p.m. UTC | #5

On Wed, Aug 06, 2014 at 11:11:41AM -0300, Paulo Zanoni wrote:
> 2014-08-05 18:51 GMT-03:00 Matt Roper <[e-mail]>:
> > On Tue, Aug 05, 2014 at 06:34:38PM -0300, Paulo Zanoni wrote:
> >> 2014-07-28 20:47 GMT-03:00 Matt Roper <[e-mail]>:
> >> > On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
> >> >> From: Paulo Zanoni <[e-mail]>
> >> >>
> >> >> Just like the cursor subtests, these also trigger WARNs on the current
> >> >> Kernel.
> >> >>
> >> >> Signed-off-by: Paulo Zanoni <[e-mail]>
> >> >
> >> > I feel like a lot of the setup you have to do here is duplicating logic
> >> > we have in the igt_kms library.  Was there functionality lacking from
> >> > that library that prevented you from using it to write this test?  If
> >> > so, I can look into adding it.
> >>
> >> Every single ioctl we call can result in runtime PM get/put calls
> >> inside the driver, so for pm_rpm.c I would like to keep using the low
> >> level interfaces, to make sure the suspends and resumes are
> >> controlled.
> >>
> >> Anyway, I never really looked at the library before. It seems the
> >> biggest functionality missing from it is documentation. I just took a
> >> look at the .c file and couldn't find anything that looked like would
> >> reduce my diffstat, since the libdrm functions we call on pm_rpm.c are
> >> very simple. Any suggestions?
> >
> > The main areas where I thought it might be possible to slim down a bit
> > by using igt_kms were all the setup code --- grabbing plane resources,
> > counting/looping over planes, grabbing properties to check plane types,
> > etc.  igt_kms will build up the plane list internally and hide a lot of
> > that long, boring code from your tests.  But since you've already
> > written the test without it, I don't feel there's any major need to
> > rewrite it with igt_kms; I was just curious if there was anything
> > specific you thought was lacking from the library so that we could
> > address it in the future.
> 
> The big problem I see is that all/most functions take the
> "igt_display_t" type as an argument instead of taking plain libdrm
> types, so either you fully adopt the API, or you don't use it at all.
> To be able to use igt_kms at all I'd have to call igt_display_init(),
> which does way too much stuff, and is probably going to grow more, and
> at some point do something that gets too many power refcounts and
> breaks runtime PM.
> 
> I would really love if the igt_kms functions were little helpers that
> accepted the plain libdrm types as arguments instead of its own types.
> This way, I would, for example, probably be able to reuse
> get_plane_property() or other functions. I see that on a lot of cases,
> we pass igt_display_t just to use the FD, so why not just use the FD?
> 
> I'll try to write some patches to reuse the stuff I want to reuse,
> then you can comment on them.

igt_kms is kinda a helper library in the larva stage and still looking for
an optimal design. Personally I'm also not really happy with it, which is
why I didn't write the documentation back when I've done that for all
other igt stuff - it just wasn't clear yet what the different parts should
do.
-Daniel

> 
> >
> > I did add some kerneldoc comments when I added the new interfaces in
> > preparation for universal planes & atomic modeset, but you're right that
> > there's still a lot that could be better documented going forward.
> >
> >
> > Matt
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > [phone]
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index f720f86..048d9ad 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -51,6 +51,9 @@ 
 #include "igt_kms.h"
 #include "igt_debugfs.h"
 
+/* One day, this will be on your libdrm. */
+#define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
+
 #define MSR_PC8_RES	0x630
 #define MSR_PC9_RES	0x631
 #define MSR_PC10_RES	0x632
@@ -72,6 +75,12 @@  enum screen_type {
 	SCREEN_TYPE_ANY,
 };
 
+enum plane_type {
+	PLANE_OVERLAY,
+	PLANE_PRIMARY,
+	PLANE_CURSOR,
+};
+
 /* Wait flags */
 #define DONT_WAIT	0
 #define WAIT_STATUS	1
@@ -1504,6 +1513,199 @@  static void cursor_subtest(bool dpms)
 	igt_assert(wait_for_suspended());
 }
 
+static enum plane_type get_plane_type(uint32_t plane_id)
+{
+	drmModeObjectPropertiesPtr props;
+	int i, j;
+	enum plane_type type;
+	bool found = false;
+
+	props = drmModeObjectGetProperties(drm_fd, plane_id,
+					   DRM_MODE_OBJECT_PLANE);
+	igt_assert(props);
+
+	for (i = 0; i < props->count_props && !found; i++) {
+		drmModePropertyPtr prop;
+		const char *enum_name = NULL;
+
+		prop = drmModeGetProperty(drm_fd, props->props[i]);
+		igt_assert(prop);
+
+		if (strcmp(prop->name, "type") == 0) {
+			igt_assert(prop->flags & DRM_MODE_PROP_ENUM);
+			igt_assert(props->prop_values[i] < prop->count_enums);
+
+			for (j = 0; j < prop->count_enums; j++) {
+				if (prop->enums[j].value ==
+				    props->prop_values[i]) {
+					enum_name = prop->enums[j].name;
+					break;
+				}
+			}
+			igt_assert(enum_name);
+
+			if (strcmp(enum_name, "Overlay") == 0)
+				type = PLANE_OVERLAY;
+			else if (strcmp(enum_name, "Primary") == 0)
+				type = PLANE_PRIMARY;
+			else if (strcmp(enum_name, "Cursor") == 0)
+				type = PLANE_CURSOR;
+			else
+				igt_assert(0);
+
+			found = true;
+		}
+
+		drmModeFreeProperty(prop);
+	}
+	igt_assert(found);
+
+	drmModeFreeObjectProperties(props);
+	return type;
+}
+
+static void test_one_plane(bool dpms, uint32_t plane_id,
+			   enum plane_type plane_type)
+{
+	int rc;
+	uint32_t plane_format, plane_w, plane_h;
+	uint32_t crtc_id, connector_id;
+	struct igt_fb scanout_fb, plane_fb1, plane_fb2;
+	drmModeModeInfoPtr mode;
+	int32_t crtc_x = 0, crtc_y = 0;
+
+	disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	igt_require(find_connector_for_modeset(&ms_data, SCREEN_TYPE_ANY,
+					       &connector_id, &mode));
+
+	crtc_id = ms_data.res->crtcs[0];
+	igt_assert(crtc_id);
+
+	igt_create_fb(drm_fd, mode->hdisplay, mode->vdisplay,
+		      DRM_FORMAT_XRGB8888, false, &scanout_fb);
+
+	fill_igt_fb(&scanout_fb, 0xFF);
+
+	switch (plane_type) {
+	case PLANE_OVERLAY:
+		plane_format = DRM_FORMAT_XRGB8888;
+		plane_w = 64;
+		plane_h = 64;
+		break;
+	case PLANE_PRIMARY:
+		plane_format = DRM_FORMAT_XRGB8888;
+		plane_w = mode->hdisplay;
+		plane_h = mode->vdisplay;
+		break;
+	case PLANE_CURSOR:
+		plane_format = DRM_FORMAT_ARGB8888;
+		plane_w = 64;
+		plane_h = 64;
+		break;
+	default:
+		igt_assert(0);
+		break;
+	}
+
+	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
+		      &plane_fb1);
+	igt_create_fb(drm_fd, plane_w, plane_h, plane_format, false,
+		      &plane_fb2);
+	fill_igt_fb(&plane_fb1, 0xFF00FFFF);
+	fill_igt_fb(&plane_fb2, 0xFF00FF00);
+
+	rc = drmModeSetCrtc(drm_fd, crtc_id, scanout_fb.fb_id, 0, 0,
+			    &connector_id, 1, mode);
+	igt_assert(rc == 0);
+	igt_assert(wait_for_active());
+
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
+			     0, 0, plane_fb1.width, plane_fb1.height,
+			     0 << 16, 0 << 16, plane_fb1.width << 16,
+			     plane_fb1.height << 16);
+	igt_assert(rc == 0);
+
+	if (dpms)
+		disable_all_screens_dpms(&ms_data);
+	else
+		disable_all_screens(&ms_data);
+	igt_assert(wait_for_suspended());
+
+	/* Just move the plane around. */
+	if (plane_type != PLANE_PRIMARY) {
+		crtc_x++;
+		crtc_y++;
+	}
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
+			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
+			     0 << 16, 0 << 16, plane_fb1.width << 16,
+			     plane_fb1.height << 16);
+	igt_assert(rc == 0);
+
+	/* Unset, then change the plane. */
+	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+	igt_assert(rc == 0);
+
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb2.fb_id, 0,
+			     crtc_x, crtc_y, plane_fb2.width, plane_fb2.height,
+			     0 << 16, 0 << 16, plane_fb2.width << 16,
+			     plane_fb2.height << 16);
+	igt_assert(rc == 0);
+
+	/* Now change the plane without unsetting first. */
+	rc = drmModeSetPlane(drm_fd, plane_id, crtc_id, plane_fb1.fb_id, 0,
+			     crtc_x, crtc_y, plane_fb1.width, plane_fb1.height,
+			     0 << 16, 0 << 16, plane_fb1.width << 16,
+			     plane_fb1.height << 16);
+	igt_assert(rc == 0);
+
+	/* Make sure nothing remains for the other tests. */
+	rc = drmModeSetPlane(drm_fd, plane_id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+	igt_assert(rc == 0);
+}
+
+/* This one also triggered WARNs on our driver at some point in time. */
+static void planes_subtest(bool universal, bool dpms)
+{
+	int i, rc, planes_tested = 0;
+	drmModePlaneResPtr planes;
+
+	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES,
+			     universal);
+	igt_require(rc == 0);
+
+	planes = drmModeGetPlaneResources(drm_fd);
+	for (i = 0; i < planes->count_planes; i++) {
+		drmModePlanePtr plane;
+
+		plane = drmModeGetPlane(drm_fd, planes->planes[i]);
+		igt_assert(plane);
+
+		/* We just pick the first CRTC on the list, so we can test for
+		 * 0x1 as the index. */
+		if (plane->possible_crtcs & 0x1) {
+			enum plane_type type;
+
+			type = universal ? get_plane_type(plane->plane_id) :
+					   PLANE_OVERLAY;
+			test_one_plane(dpms, plane->plane_id, type);
+			planes_tested++;
+		}
+		drmModeFreePlane(plane);
+	}
+	drmModeFreePlaneResources(planes);
+
+	rc = drmSetClientCap(drm_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
+	igt_assert(rc == 0);
+
+	if (universal)
+		igt_assert(planes_tested >= 3);
+	else
+		igt_assert(planes_tested >= 1);
+}
+
 int rounds = 50;
 bool stay = false;
 
@@ -1577,11 +1779,19 @@  int main(int argc, char *argv[])
 	igt_subtest("gem-idle")
 		gem_idle_subtest();
 
-	/* Cursors */
+	/* Planes and cursors */
 	igt_subtest("cursor")
 		cursor_subtest(false);
 	igt_subtest("cursor-dpms")
 		cursor_subtest(true);
+	igt_subtest("legacy-planes")
+		planes_subtest(false, false);
+	igt_subtest("legacy-planes-dpms")
+		planes_subtest(false, true);
+	igt_subtest("universal-planes")
+		planes_subtest(true, false);
+	igt_subtest("universal-planes-dpms")
+		planes_subtest(true, true);
 
 	/* Misc */
 	igt_subtest("reg-read-ioctl")