[Intel-gfx] [RFC PATCH v2 2/4] drm/ipvr: drm driver for VED

[Cheng, Yao]

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 at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, October 22, 2014 16:51
> To: Cheng, Yao
> Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org; dri-
> devel at 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 at ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Tuesday, October 21, 2014 5:08 PM
> > > To: Cheng, Yao
> > > Cc: intel-gfx at lists.freedesktop.org;
> > > dri-devel at 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 at 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