On 26.04.2010 20:26, José Fonseca wrote:
> 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.
>> 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.
Of course you're right. Since things don't compile yet I didn't notice :-).
>> + 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.
I wondered about the unsigned value too. One of the non-public state
trackers seems to use it though. It is a bit odd though, maybe we really
should find a way to express that in the clear function.
>> + * 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.
Yes, that's what it first looked like. I don't feel strongly either way.
For reference, d3d10 has a CheckFormatSupport() query which will return
if a format supports multisampling (for rendertarget or for sampling) -
but no indication of sample count. Sample count (along with quality
levels) can be queried with CheckMultisampleQualityLevels(). I realize
our is_format_supported() query looks quite a bit different, so maybe
the approach you suggest with giving it a template is the right thing to do.
Roland
------------------------------------------------------------------------------
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
