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

[Christoph Bumiller]

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.

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

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

Christoph