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

[Roland Scheidegger]

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

Roland