2014-08-05 18:51 GMT-03:00 Matt Roper <matthew.d.roper@xxxxxxxxx>: > 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@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? > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
