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

[Roland Scheidegger]

> On 07/25/2011 10:43 PM, Roland Scheidegger wrote:
> >>
> >> 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...).
> > 
> It's NEAREST. For the resolve too. That means you pick the value of a
> single sample, there's no averaging.
Where did you get that the filter applies to the resolve step too? I'm quite sure you're wrong there.
In contrast I think the ReadPixels language applies. So even with nearest filtering, you'll get proper resolve with color buffers (this is actually not in the ReadPixels section but follows from GLs notion that resolve step is implicit, though the resolve function isn't actually well-defined for GL for color buffers neither), and mostly implementation dependent crap with z/stencil buffers. And yes this looks like a mess to me.

> 
> Yes it's just like a blit without interpolation. Resolve is also just
> a
> blit with special filtering. But it's my fast,
> tailored-to-the-hardware
> blit without the cruft associated with going through the gallium
> interface.

We want to tailor it to all hw, however. If all other interested hw can do it fine, let's change it. And for the depth/stencil resolve at least I _really_ want to see a definition what this is supposed to do - if that's GL's "anything goes" like for multisampled depth ReadPixels then fine let's say that, though it probably isn't terribly useful if returning all zeros is considered compliant... There's a reason this crap was left out of the interface initially and it only contains what's REALLY needed - and that's a way to resolve the color buffers. I believe "resolving" depth buffers that way with glFrameBufferBlit is just not well defined enough to be useful to applications anyway.

Roland