[Mesa-dev] [PATCH 2/4] gallium: extend resource_resolve to accommodate BlitFramebuffer

[Roland Scheidegger]

> On 07/25/2011 07:54 PM, Roland Scheidegger wrote:
> >> Resolve via glBlitFramebuffer allows resolving a sub-region of a
> >> renderbuffer to a different location in any mipmap level of some
> >> other texture, therefore location and size parameters are needed.
> >>
> >> The mask parameter was added because resolving only depth or only
> >> stencil of a combined buffer is possible as well.
> >>
> >> Copying from FBO to a window system buffer requires a vertical
> >> so the yflip parameter was added.
> >>
> >> The y-flip parameter could be left out if pipe_box was changed to
> >> contain signed width/height or x0,y0,x1,y1 instead.
> >> This might benefit other methods, such as resource_copy_region,
> >> where
> >> some hw can easily do a backwards copy (yflip).
> > 
> > Hmm I'm not sure I like these interface changes too much.
> > Individual comments below.
> > 
> > 
> >> diff --git a/src/gallium/include/pipe/p_context.h
> >> b/src/gallium/include/pipe/p_context.h
> >> index 3f6d90d..9376cdd 100644
> >> --- a/src/gallium/include/pipe/p_context.h
> >> +++ b/src/gallium/include/pipe/p_context.h
> >> @@ -255,8 +255,7 @@ struct pipe_context {
> >>  
> >>     /**
> >>      * Copy a block of pixels from one resource to another.
> >> -    * The resource must be of the same format.
> >> -    * Resources with nr_samples > 1 are not allowed.
> >> +    * The resources must be of the same format and sample count.
> >>      */
> >>     void (*resource_copy_region)(struct pipe_context *pipe,
> >>                                  struct pipe_resource *dst,
> > I think the idea to restrict this to 1 sample was to keep it
> > simple. I'm not sure hw can always easily do this, depending on
> > how it is implemented (especially because there are regions
> > involved). Might be ok though.
> > 
> >> @@ -267,14 +266,17 @@ struct pipe_context {
> >>                                  const struct pipe_box *src_box);
> >>  
> >>     /**
> >> -    * Resolve a multisampled resource into a non-multisampled
> >> one.
> >> -    * Source and destination must have the same size and same
> >> format.
> >> +    * Resolve a multisampled resource into a non-multisampled
> >> one,
> >> +    * or vice versa (in the latter case, values are just
> >> replicated).
> >> +    * Source and destination must have the same format.
> >> +    * Mask can be either PIPE_MASK_RGBA, Z, S or ZS.
> >> +    * The mipmap level of the multisampled resource will be 0.
> >>      */
> >> -   void (*resource_resolve)(struct pipe_context *pipe,
> >> -                            struct pipe_resource *dst,
> >> -                            unsigned dst_layer,
> >> -                            struct pipe_resource *src,
> >> -                            unsigned src_layer);
> >> +   void (*resource_resolve)(struct pipe_context *pipe, unsigned
> >> mask,
> >> +                            struct pipe_resource *dst, unsigned
> >> dst_level,
> >> +                            unsigned dstx, unsigned dsty,
> >> unsigned dst_layer,
> >> +                            struct pipe_resource *src, unsigned
> >> src_level,
> >> +                            const struct pipe_box *src_box,
> >> boolean yflip);
> > 
> > This function was inspired by d3d10 and the wide variety of
> > possible implementations.
> > You cannot resolve depth and stencil buffers in d3d10/11 and I'm
> > not convinced it even makes a whole lot of sense (especially for
> > the stencil buffer).
> Just because you cannot do something in D3D10 doesn't mean you have
> to
> make it impossible in gallium if it can be done OpenGL.
> If you resolve because you don't need/want multisampling for the rest
> of
> the frame, but you still want the contents of the depth buffer, it
> does
> very much make sense.
Just think about it what this does: 2 out of 4 depth samples were from some near object, the rest from some object back. I haven't looked at the details of the spec too closely, what value are you even supposed to get there? A meaningless "average depth"? Or just one of the values at random?
Resolving color buffers is pretty well defined (for standard msaa at least). I have no idea how that's supposed to be defined for depth and stencil values. Frankly I'm not sure what glBlitFramebuffer is supposed to do here, maybe it's defined somewhere but even applying the term "resolve" to a depth buffer seems very iffy. At the very least it needs to be documented in the gallium docs what "resolving" a depth/stencil buffer really means.
I certainly agree just because it isn't in d3d doesn't mean it can't be in gallium. But just because it maps well to OpenGL isn't a good reason to include it neither.

> 
> > Some hw will potentially be unable to do this (I don't know how
> > deferred renderers would do that, for example).
> > Also, allowing regions might also be very difficult to do for some
> > hardware, and yes glBlitFramebuffer allows this but I'm not sure
> > it's really worth having this in the interface - I doubt anyone
> > ever will use this anyway, so it would be nice if we could keep
> > that out of drivers. Just doing another copy for the odd testcase
> > trying this seems ok to me.
> There will always be at least 1 application that does something you
> didn't expect, and it will be needlessly slow with our drivers.
That's always possible, apps do crazy things sometimes.

> 
> > For the same reason I don't like the y-flip very much. I see though
> > how that could be very useful. I guess drivers not able to do this
> > natively could just do the copy themselves (ok they could do that
> > with the regions too).
> > Frankly glBlitFrameBuffer can do a lot of things and I'm not sure
> > it makes a lot of sense to cram all that functionality into
> > resource_resolve. Though if other driver writers agree I could be
> > convinced.
> > 
> nv50+ can happily do all of the above in one go.
> 
> Every fallback you can do in a state tracker can be done equally well
> from within drivers through shared utility code, compare u_blit and
> u_blitter.
> The difference is, if a driver does *not* have to use the slow path,
> it
> is, for a change, *not* *forced* to do so anyway by a state tracker.
Yes, but there's some value in keeping the driver interface clean.

Roland