> > > > > > With that out of the way some high-level review: > > > - I think we need the full libva implementation to review the interfaces > > > properly. At least the little libdrm test program doesn't seem to fully > > > exercise it all. > > > > The libva driver need some time to be fully open sourced, but I can > > upload the code to Sean's private github repo for your access. I'll > > sync with Sean and you internally. > > It doesn't need to be the final libva driver of course, just something so that > people can look at the userspace side. So upload to some github account is > perfectly ok. > > Or do you mean we still have legal review pending on those patches? In that > case I think we need to wait for that to complete first. I see....yes you're right, it's still under legal review. We'll put it to internet as soon as being approved. > > > - Locking seems to be inexistent in places, at least some of the idr > > > manipulation very much looks like it's done lock-free. That doesn't work > > > well. > > > > Yes, probably we haven't considered all the scenarios carefully, is it > > possible to review them in an internal discussion? > > Imo no need for private review since I didn't spot anything fundamentally > wrong. It's just a lot of small details, and for those I think m-l review is a good > tool. But someone needs to do that, and I don't really have the time for it. I see, thanks. > > > > - You implement file-descriptor based fences, but then also have the > more > > > gem-traditional wait ioctl working on buffer objects. That's a bit a > > > funky mix of implicit and explicit fencing. Furthermore adding new > > > private fence objects isn't a good idea now that everyon is talking > > > about de-staging android syncpts as the official userspace interface. > > > > > > Also, your userspace patches don't use this, so maybe we can just rip it > > > all out? > > > > Currently the libdrm_ipvr.so uses both the WAIT IOCTL and FD style > > fence... At beginning, both drm_ipvr_gem_bo_alloc() and > > drm_ipvr_gem_bo_wait() use the WAIT IOCTL. > > In drm_ipvr_gem_bo_alloc(), libdrm_ipvr tries to return an existing > > free BO instead of requesting kernel via IOCTL, like libdrm_intel does. > > Eventually we think the status query on multiple BOs is inefficient, > > so we added the FD style fence to let libdrm_ipvr call select() to do > > a batch query. I'm fine to drop one and keep the other. Which one is > > preferred by GEM? The WAIT_IOCTL or the FD fence? Or do you suggest > > directly use the Android syncpts? > > The wait ioctl is the usual approach with gem drivers. Explicit fencing is still in > flux like I've said, so charging ahead and locking down an interface doesn't > seem like a good idea. And I'd be _really_ surprised if you can benchmark the > benefits of explicit fencing, so I don't think you can even justify the added > complexity. Understood...We didn't do real benchmark, the "inefficient" just means the logic in code. Will double-check the perf, and rip out the FD-based fence in v3 patch if no real benefit. > > > > - I'm a bit unclear on your usage of vxd_/pvr_ prefixes. > > > > > > > Thanks for pointing out this, shall I add some description about this in next > patch (in git commit message)? > > We use different prefixes to distinguish the function scope, like we used to > do on GMA series (Android product): > > ved: decoding function only > > vec: encoding function only (for future extension) > > vsp: post-processing runction only (for future extension) > > ipvr: common for all encoding/decoding/postproc > > Yeah, explaining this kind of stuff in the commit message would be great. > Or just go ahead and add a new vxd section in the drm docbook (like we > already have for i915) and add such high-level information there. Thanks, will add this in v3 patch. > > > > The driver is fairly big and I don't really have the time to do a > > > full blown review of even just the interfaces. I think we need to > > > have some internal discussions about how to do this, but meanwhile > > > we can cover some of the high-level bits. > > > > > > > This is great, I'll talk with Sean on how to run this. > > Yeah, we need to internally figure out how to do the review. Thx I asked Sean to co-ordinate this :) > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
