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

[Cheng, Yao]

> > >
> > > 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