2014-07-28 20:47 GMT-03:00 Matt Roper <matthew.d.roper@xxxxxxxxx>:
> On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> Just like the cursor subtests, these also trigger WARNs on the current
>> Kernel.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> 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
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
