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