On Wed, Oct 22, 2014 at 06:37:16AM +0000, Cheng, Yao wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, October 21, 2014 5:08 PM To: Cheng, Yao Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Kelley, Sean V; Vetter, Daniel; Abel, Michael J; Jiang, Fei; Rao, Ram R Subject: Re: [Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for VED
On Tue, Oct 21, 2014 at 02:36:42PM +0800, Yao Cheng wrote:
Probes VED and creates a new drm device for hardware accelerated video decoding. Currently support VP8 decoding on valleyview.
Signed-off-by: Yao Cheng yao.cheng@intel.com
The in-patch changelog here is missing, and there's also no indication in the cover letter for what changes you've made. On a quick look you've incorporated some of David's feedback, but not all of it. That's not good, since if you only partially apply review feedback then you essentially force reviewers to read the entire patch again, which is a good way to driver them away. Also you should Cc: (in the sob section of the patch) all the people who have commented on your patch already.
Oops, sorry for not following the upstreaming rules :( I might have overlooked some of David's comment......have to learn more about the rules. For this version, I'll add changelog by replying my patch with cc to those commenters, I assume this is not too late....
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.
The ioctl structs need to be cleaned up. You can't use uint32_t and similar typedefs since they can clash with userspace. You must use __u32 and friends. Also, some of the padding fields arent' really required - if you only have 4byte types then you don't need to align to 8 bytes.
Input validation on ioctls looks spotty at best. E.g. if you have any padding fields you need to check that they are 0, otherwise we can't ever reuse them as flags fields. And on principle _all_ input fields must be validated first.
For some good guidelines for ioctls see http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
Thanks for pointing me to the ioctl instruction... I'll read it carefully and update the ioctl interfaces...
- 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.
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.
- 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.
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. -Daniel
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
CC David for notifying the patch update:
Add the missing v2 changelog: Take David's comment: add mmap support, remove the MMAP_IOCTL and add MMAP_OFFSET_IOCTL Take David's comment: remove postclose() and move code to preclose() Take David's comment: set NULL to set_busid Forgot to take David's comment (Sorry, will add it in v3 patch): Replace drm_platform_init/drm_put_dev with drm_dev_alloc, drm_dev_register, drm_dev_unregister and drm_dev_unref
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, October 22, 2014 16:51 To: Cheng, Yao Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; dri- devel@lists.freedesktop.org; Kelley, Sean V; Vetter, Daniel; Abel, Michael J; Jiang, Fei; Rao, Ram R; David Herrmann Subject: Re: [Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for VED
On Wed, Oct 22, 2014 at 06:37:16AM +0000, Cheng, Yao wrote:
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Tuesday, October 21, 2014 5:08 PM To: Cheng, Yao Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Kelley, Sean V; Vetter, Daniel; Abel, Michael J; Jiang, Fei; Rao, Ram R Subject: Re: [Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for VED
On Tue, Oct 21, 2014 at 02:36:42PM +0800, Yao Cheng wrote:
Probes VED and creates a new drm device for hardware accelerated video decoding. Currently support VP8 decoding on valleyview.
Signed-off-by: Yao Cheng yao.cheng@intel.com
The in-patch changelog here is missing, and there's also no indication in the cover letter for what changes you've made. On a quick look you've incorporated some of David's feedback, but not all of it. That's not good, since if you only partially apply review feedback then you essentially force reviewers to read the entire patch again, which is a good way to driver them away. Also you should Cc: (in the sob section of the patch) all the people who have
commented on your patch already.
Oops, sorry for not following the upstreaming rules :( I might have overlooked some of David's comment......have to learn more about the rules. For this version, I'll add changelog by replying my patch with cc to those commenters, I assume this is not too late....
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.
- The ioctl structs need to be cleaned up. You can't use uint32_t and similar typedefs since they can clash with userspace. You must use
__u32
and friends. Also, some of the padding fields arent' really required - if you only have 4byte types then you don't need to align to 8 bytes.
Input validation on ioctls looks spotty at best. E.g. if you have any padding fields you need to check that they are 0, otherwise we can't ever reuse them as flags fields. And on principle _all_ input fields must be validated first.
For some good guidelines for ioctls see http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
Thanks for pointing me to the ioctl instruction... I'll read it carefully and update the ioctl interfaces...
- 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.
- 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.
- 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.
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.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Cheng, Yao -
Daniel Vetter