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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux