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

[Roland Scheidegger]

> 
> On 07/25/2011 09:03 PM, Roland Scheidegger wrote:
> >> 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.
> > 
> I had noticed that (and incorporated it into the nv50 patch), average
> depth doesn't make much sense indeed.
> That's why you can't use GL_LINEAR for depth resolve but only
> GL_NEAREST.
> 
> From the GL spec:
> "if mask includes DEPTH_BUFFER_BIT or STENCIL_BUFFER_BIT, and filter
> is
> not NEAREST, no copy is performed and an INVALID_OPERATION error is
> generated.

Yes, this is for different pixels (in case of stretch blit), not different samples. So it is acknowledged that averaging doesn't make sense between different depth pixel values, yet averaging is allowed (though not recommended) for depth sample values (it doesn't make one bit more sense there...).

Roland