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@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. 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
