[Intel-gfx] [PATCH 2/2] tests/pm_rpm: add planes subtests

[Paulo Zanoni]

2014-08-05 18:51 GMT-03:00 Matt Roper <matthew.d.roper at intel.com>:
> On Tue, Aug 05, 2014 at 06:34:38PM -0300, Paulo Zanoni wrote:
>> 2014-07-28 20:47 GMT-03:00 Matt Roper <matthew.d.roper at intel.com>:
>> > On Mon, Jul 28, 2014 at 03:37:15PM -0300, Paulo Zanoni wrote:
>> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >>
>> >> Just like the cursor subtests, these also trigger WARNs on the current
>> >> Kernel.
>> >>
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >
>> > 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
> (916) 356-2795



-- 
Paulo Zanoni