[Intel-gfx] [Patch] multiple ring buffer support(for libva H.264 decoding)

[Daniel Vetter]

Hi Zou,

I've scanned over your patch and found a few things:
- Please post patches inline. This makes commenting on specific issues
  much easier.

- Introducing multiple ringbuffers changes the gpu execution breadcrumb
  from seqno to (seqno, ring). You're spreading intel_ring_buffer *ring
  arguments all over the tree. I think this is ugly and guarantees funny
  inconsistencies in calling conventions. Introducing an
  intel_gpu_breadcrumb to pack these things together would fix this. We
  could even be clever and shave off a few bits of the seqno (32 bit are
  more than enough) and store the ring number without increasing space,
  like this

struct intel_gpu_breadcrumb {
	uint32_t seqno : 30;
	uint32_t ringno : 2;
};

- Related to the above: You add both
	uint32_t ring_flag;
	struct intel_ring_buffer *ring;
  to the intel bo struct. Using indexes (like we do for fences) would save
  space. I know, currently intel bo's are rather wasteful with memory, but
  I have patches lying around (some already posted) to fix this.
  Furthermore using the above intel_gpu_breadcrump would nicely unify the
  code.

- This patch is too big and therefore not easily reviewable. By a quick
  look I see at least three different patches in there (probably more):
  1) Introduce the intel_gpu_breadcrumb abstraction (and thread it through
  all relevant function calls).
  2a) Intruduce the ringbuffer abstraction (and change relevant
  callsites).
  2b) Move the render ring implementation to intel_ringbuffer (seperating
  pure code moves from functional changes makes patch reviewing _much_
  easier).
  3a) Implement the new bsd ring.
  3b) Wire the new bsd ring support up.

That's about it on a quick glance. Because this patch contains multiple
things moulded into one patch I can't do a deeper review.

Yours, Daniel
	
On Fri, Mar 26, 2010 at 03:41:53PM +0800, Zou, Nanhai wrote:
> Hi,
> This patch introduces multiple ring buffer support to the driver.
> 
> From G4X, GPU begin to support more than 1 kind of command buffer.
> On G4X, a separate command buffer, a BSD (bit-stream decoder) command buffer was introduced to support H.264 VLD decoding.
> 
> There will be more kinds of command buffer to come on newer chips.
> 
> The patch abstract ring buffer related methods.
> It separated sequential number and interrupt handling logic to per ring base.
> gem object active_list and request_list are also put per ring while flushing_list 
> And inactive_list keeps single.
> 
> The patch uses the flag in gem_execbuffer2 interface to indicate on which ring to execute.
> Then it will mark every referenced object in this execution to be on that ring. One object can belong only 1 ring at 1 time.
> 
> The multiple ring buffer capability will be first used by our H.264 VAAPI driver.
> We have tested the patch on pre Ironlake Systems(both 3D 2D)
> , and Ironlake Systems(HD H.264 video + 3D).
> 
> Signed-off-by: Xiang Hai hao <haihao.xiang at intel.com>
> Signed-off-by: Zou Nan hai <nanhai.zou at intel.com>


> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48