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

[Daniel Vetter]

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