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

[Daniel Vetter]

On Wed, Aug 06, 2014 at 11:11:41AM -0300, Paulo Zanoni wrote:
> 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.

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
> > (916) 356-2795
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch