Hi Roland, Overall looks good. It's nice to finally have a way to export MSAA capabilities, and see the pipe_surfaces use being more constrained.
A few comments inline, but no strong feelings so feel free to do as you wish. Jose On Mon, 2010-04-26 at 11:05 -0700, Roland Scheidegger wrote: > Module: Mesa > Branch: gallium-msaa > Commit: aac2cccccfd701ae8d7ce0813c28c64498d4a076 > URL: > http://cgit.freedesktop.org/mesa/mesa/commit/?id=aac2cccccfd701ae8d7ce0813c28c64498d4a076 > > Author: Roland Scheidegger <srol...@vmware.com> > Date: Mon Apr 26 19:50:57 2010 +0200 > > gallium: interface changes for multisampling > > add function to set sample mask, and state for alpha-to-coverage and > alpha-to-one. Also make it possible to query for supported sample count > with is_msaa_supported(). > > Use explicit resource_resolve() to resolve a resource. Note that it is illegal > to bind a unresolved resource as a sampler view, must be resolved first (as > per > d3d10 and OGL APIs, binding unresolved resource would mean that special > texture > fetch functions need to be used which give explicit control over what samples > to fetch, which isn't supported yet). > > Also change surface_fill() and surface_copy() to operate directly on > resources. > Blits should operate directly on resources, most often state trackers just > used > get_tex_surface() then did a blit. Note this also means the blit bind flags > are > gone, if a driver implements this functionality it is expected to handle it > for > all resources having depth_stencil/render_target/sampler_view bind flags > (might > even require it for all bind flags?). > > Might want to introduce quality levels for MSAA later. > Might need to revisit this for hw which does instant resolve. > > --- > > src/gallium/auxiliary/cso_cache/cso_context.c | 11 +++++ > src/gallium/auxiliary/cso_cache/cso_context.h | 2 + > src/gallium/docs/source/context.rst | 16 +++++-- > src/gallium/docs/source/screen.rst | 13 +++++- > src/gallium/include/pipe/p_context.h | 51 +++++++++++++++++------- > src/gallium/include/pipe/p_defines.h | 2 - > src/gallium/include/pipe/p_screen.h | 11 +++++- > src/gallium/include/pipe/p_state.h | 2 + > 8 files changed, 82 insertions(+), 26 deletions(-) > > diff --git a/src/gallium/auxiliary/cso_cache/cso_context.c > b/src/gallium/auxiliary/cso_cache/cso_context.c > index 6d0b420..50736f0 100644 > --- a/src/gallium/auxiliary/cso_cache/cso_context.c > +++ b/src/gallium/auxiliary/cso_cache/cso_context.c > @@ -98,6 +98,7 @@ struct cso_context { > struct pipe_framebuffer_state fb, fb_saved; > struct pipe_viewport_state vp, vp_saved; > struct pipe_blend_color blend_color; > + unsigned sample_mask sample_mask; This doesn't look correct. sample_mask appears twice. > struct pipe_stencil_ref stencil_ref, stencil_ref_saved; > }; > > @@ -984,6 +985,16 @@ enum pipe_error cso_set_blend_color(struct cso_context > *ctx, > return PIPE_OK; > } > > +enum pipe_error cso_set_sample_mask(struct cso_context *ctx, > + unsigned sample_mask) > +{ > + if (ctx->sample_mask != sample_mask) { > + ctx->sample_mask = sample_mask; > + ctx->pipe->set_sample_mask(ctx->pipe, sample_mask); > + } > + return PIPE_OK; > +} > + > enum pipe_error cso_set_stencil_ref(struct cso_context *ctx, > const struct pipe_stencil_ref *sr) > { > diff --git a/src/gallium/auxiliary/cso_cache/cso_context.h > b/src/gallium/auxiliary/cso_cache/cso_context.h > index d6bcb1f..f0b07f7 100644 > --- a/src/gallium/auxiliary/cso_cache/cso_context.h > +++ b/src/gallium/auxiliary/cso_cache/cso_context.h > @@ -159,6 +159,8 @@ void cso_restore_viewport(struct cso_context *cso); > enum pipe_error cso_set_blend_color(struct cso_context *cso, > const struct pipe_blend_color *bc); > > +enum pipe_error cso_set_sample_mask(struct cso_context *cso, > + unsigned stencil_mask); > > enum pipe_error cso_set_stencil_ref(struct cso_context *cso, > const struct pipe_stencil_ref *sr); > diff --git a/src/gallium/docs/source/context.rst > b/src/gallium/docs/source/context.rst > index c82e681..374711b 100644 > --- a/src/gallium/docs/source/context.rst > +++ b/src/gallium/docs/source/context.rst > @@ -54,6 +54,7 @@ objects. They all follow simple, one-method binding calls, > e.g. > * ``set_stencil_ref`` sets the stencil front and back reference values > which are used as comparison values in stencil test. > * ``set_blend_color`` > +* ``set_sample_mask`` > * ``set_clip_state`` > * ``set_polygon_stipple`` > * ``set_scissor_state`` sets the bounds for the scissor test, which culls > @@ -255,15 +256,20 @@ Blitting > These methods emulate classic blitter controls. They are not guaranteed to be > available; if they are set to NULL, then they are not present. > > -These methods operate directly on ``pipe_surface`` objects, and stand > +These methods operate directly on ``pipe_resource`` objects, and stand > apart from any 3D state in the context. Blitting functionality may be > moved to a separate abstraction at some point in the future. > > -``surface_fill`` performs a fill operation on a section of a surface. > +``resource_fill_region`` performs a fill operation on a section of a > resource. > > -``surface_copy`` blits a region of a surface to a region of another surface, > -provided that both surfaces are the same format. The source and destination > -may be the same surface, and overlapping blits are permitted. > +``resource_copy_region`` blits a region of a subresource of a resource to a > +region of another subresource of a resource, provided that both resources > have the > +same format. The source and destination may be the same resource, and > overlapping > +blits are permitted. > + > +``resource_resolve`` resolves a multisampled resource into a non-multisampled > +one. Formats and dimensions must match. This function must be present if a > driver > +supports multisampling. > > The interfaces to these calls are likely to change to make it easier > for a driver to batch multiple blits with the same source and > diff --git a/src/gallium/docs/source/screen.rst > b/src/gallium/docs/source/screen.rst > index c5815f8..2a8f696 100644 > --- a/src/gallium/docs/source/screen.rst > +++ b/src/gallium/docs/source/screen.rst > @@ -128,9 +128,6 @@ resources might be created and handled quite differently. > * ``PIPE_BIND_VERTEX_BUFFER``: A vertex buffer. > * ``PIPE_BIND_INDEX_BUFFER``: An vertex index/element buffer. > * ``PIPE_BIND_CONSTANT_BUFFER``: A buffer of shader constants. > -* ``PIPE_BIND_BLIT_SOURCE``: A blit source, as given to surface_copy. > -* ``PIPE_BIND_BLIT_DESTINATION``: A blit destination, as given to > surface_copy > - and surface_fill. > * ``PIPE_BIND_TRANSFER_WRITE``: A transfer object which will be written to. > * ``PIPE_BIND_TRANSFER_READ``: A transfer object which will be read from. > * ``PIPE_BIND_CUSTOM``: > @@ -223,6 +220,16 @@ Determine if a resource in the given format can be used > in a specific manner. > > Returns TRUE if all usages can be satisfied. > > +is_msaa_supported > +^^^^^^^^^^^^^^^^^ > + > +Determine if a format supports multisampling with a given number of samples. > + > +**format** the resource format > + > +**sample_count** the number of samples. Valid query range is 2-32. > + > +Returns TRUE if sample_count number of samples is supported with this format. > > .. _resource_create: > > diff --git a/src/gallium/include/pipe/p_context.h > b/src/gallium/include/pipe/p_context.h > index 6f47845..c385481 100644 > --- a/src/gallium/include/pipe/p_context.h > +++ b/src/gallium/include/pipe/p_context.h > @@ -198,6 +198,9 @@ struct pipe_context { > void (*set_stencil_ref)( struct pipe_context *, > const struct pipe_stencil_ref * ); > > + void (*set_sample_mask)( struct pipe_context *, > + unsigned sample_mask ); > + > void (*set_clip_state)( struct pipe_context *, > const struct pipe_clip_state * ); > > @@ -233,32 +236,50 @@ struct pipe_context { > > > /** > - * Surface functions > + * Resource functions for blit-like functionality > * > * The pipe driver is allowed to set these functions to NULL, and in that > * case, they will not be available. > + * If a driver supports multisampling, resource_resolve must be available. > */ > /*...@{*/ > > /** > - * Copy a block of pixels from one surface to another. > - * The surfaces must be of the same format. > + * 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. > */ > - void (*surface_copy)(struct pipe_context *pipe, > - struct pipe_surface *dest, > - unsigned destx, unsigned desty, > - struct pipe_surface *src, > - unsigned srcx, unsigned srcy, > - unsigned width, unsigned height); > + void (*resource_copy_region)(struct pipe_context *pipe, > + struct pipe_resource *dst, > + struct pipe_subresource subdst, > + unsigned dstx, unsigned dsty, unsigned dstz, > + struct pipe_resource *src, > + struct pipe_subresource subsrc, > + unsigned srcx, unsigned srcy, unsigned srcz, > + unsigned width, unsigned height); > > /** > - * Fill a region of a surface with a constant value. > + * Fill a region of a resource with a constant value. > + * Resources with nr_samples > 1 are not allowed. > */ > - void (*surface_fill)(struct pipe_context *pipe, > - struct pipe_surface *dst, > - unsigned dstx, unsigned dsty, > - unsigned width, unsigned height, > - unsigned value); > + void (*resource_fill_region)(struct pipe_context *pipe, > + struct pipe_resource *dst, > + struct pipe_subresource subdst, > + struct pipe_box *dstbox, > + unsigned srcx, unsigned srcy, unsigned srcz, > + unsigned width, unsigned height, > + unsigned value); I think that once you're done with this change we should remove surface_fill/resource_fill_region(), as no state tracker uses it -- only drivers internally, and the 32bit value is hopeless for formats more than 32bits. > + > + /** > + * Resolve a multisampled resource into a non-multisampled one. > + * Source and destination must have the same size and same format. > + */ > + void (*resource_resolve)(struct pipe_context *pipe, > + struct pipe_resource *dst, > + struct pipe_subresource subdst, > + struct pipe_resource *src, > + struct pipe_subresource subsrc); > + > /*...@}*/ > > /** > diff --git a/src/gallium/include/pipe/p_defines.h > b/src/gallium/include/pipe/p_defines.h > index 48edfbf..3223e8d 100644 > --- a/src/gallium/include/pipe/p_defines.h > +++ b/src/gallium/include/pipe/p_defines.h > @@ -284,8 +284,6 @@ enum pipe_transfer_usage { > #define PIPE_BIND_VERTEX_BUFFER (1 << 3) /* set_vertex_buffers */ > #define PIPE_BIND_INDEX_BUFFER (1 << 4) /* draw_elements */ > #define PIPE_BIND_CONSTANT_BUFFER (1 << 5) /* set_constant_buffer */ > -#define PIPE_BIND_BLIT_SOURCE (1 << 6) /* surface_copy */ > -#define PIPE_BIND_BLIT_DESTINATION (1 << 7) /* surface_copy, fill */ > #define PIPE_BIND_DISPLAY_TARGET (1 << 8) /* flush_front_buffer */ > #define PIPE_BIND_TRANSFER_WRITE (1 << 9) /* get_transfer */ > #define PIPE_BIND_TRANSFER_READ (1 << 10) /* get_transfer */ > diff --git a/src/gallium/include/pipe/p_screen.h > b/src/gallium/include/pipe/p_screen.h > index beff1ae..1bad045 100644 > --- a/src/gallium/include/pipe/p_screen.h > +++ b/src/gallium/include/pipe/p_screen.h > @@ -99,10 +99,19 @@ struct pipe_screen { > boolean (*is_format_supported)( struct pipe_screen *, > enum pipe_format format, > enum pipe_texture_target target, > - unsigned bindings, > + unsigned bindings, > unsigned geom_flags ); > > /** > + * Check if the given pipe_format is supported with a requested > + * number of samples for msaa. > + * \param sample_count number of samples for multisampling > + */ > + boolean (*is_msaa_supported)( struct pipe_screen *, > + enum pipe_format format, > + unsigned sample_count ); Instead of a new is_msaa_support() I'd prefer see sample_count in is_format_supported or better, replace both with is_resource_supported which takes a resource template. But I understand that's a bit beyond the scope of this change. > + /** > * Create a new texture object, using the given template info. > */ > struct pipe_resource * (*resource_create)(struct pipe_screen *, > diff --git a/src/gallium/include/pipe/p_state.h > b/src/gallium/include/pipe/p_state.h > index a504757..f9ad07d 100644 > --- a/src/gallium/include/pipe/p_state.h > +++ b/src/gallium/include/pipe/p_state.h > @@ -218,6 +218,8 @@ struct pipe_blend_state > unsigned logicop_enable:1; > unsigned logicop_func:4; /**< PIPE_LOGICOP_x */ > unsigned dither:1; > + unsigned alpha_to_coverage:1; > + unsigned alpha_to_one:1; > struct pipe_rt_blend_state rt[PIPE_MAX_COLOR_BUFS]; > }; > > > _______________________________________________ > mesa-commit mailing list > mesa-com...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-commit ------------------------------------------------------------------------------ _______________________________________________ Mesa3d-dev mailing list Mesa3d-dev@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mesa3d-dev