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@xxxxxxxx] On Behalf Of Daniel > Vetter > Sent: Wednesday, October 22, 2014 16:51 > To: Cheng, Yao > Cc: Daniel Vetter; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxx] On Behalf Of > > > Daniel Vetter > > > Sent: Tuesday, October 21, 2014 5:08 PM > > > To: Cheng, Yao > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > > > dri-devel@xxxxxxxxxxxxxxxxxxxxx; 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@xxxxxxxxx> > > > > > > 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 mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
