José Fonseca wrote on 2010-03-11 14:40:
> On Thu, 2010-03-11 at 03:16 -0800, michal wrote:
>
>> Hi,
>>
>> I would like to merge the branch in subject this week. This feature
>> branch allows state trackers to bind sampler views instead of textures
>> to shader stages.
>>
>> A sampler view object holds a reference to a texture and also overrides
>> internal texture format (resource casting) and specifies RGBA swizzle
>> (needed for GL_EXT_texture_swizzle extension).
>>
>> Yesterday I had to merge master into gallium-sampler-view -- the nv50
>> and r300 drivers had lots of conflicts. Could the maintainers of said
>> drivers had a look at them and see if they are still functional, please?
>>
>> Thanks.
>>
>
> Michal,
>
> Looks good overall. Minor comments below. Sorry if I rehash things that
> have been already discussed. Feel free to ignore them.
>
>
> diff --git a/src/gallium/include/pipe/p_state.h
> b/src/gallium/include/pipe/p_state.h
> index 3a97d88..3c7c0a5 100644
> --- a/src/gallium/include/pipe/p_state.h
> +++ b/src/gallium/include/pipe/p_state.h
> @@ -300,6 +300,24 @@ struct pipe_surface
>
>
> /**
> + * A view into a texture that can be bound to a shader stage.
> + */
> +struct pipe_sampler_view
> +{
> + struct pipe_reference reference;
> + enum pipe_format format; /**< typed PIPE_FORMAT_x */
>
> I think it is worth to document that not all formats are valid here.
> pipe_sampler_view::format and pipe_texture::format must be compatible.
> The semantics of compatibility are a bit confusing though. Even DX10's.
>
> At very least format's util_format_block must match.
>
> RGB <=> SRGB variations should also be accepted. And sizzwle variations.
>
> The difficulty is whether formats like A4R4G4B4 <=> A1G5R5B5 or
> R8G8B8A8 <=> R32 should be accepted. I think hardware should have no
> troubles with typecasting. So I'm inclined towards make this acceptible.
>
>
How about all component sizes and order of components must match, and
the only difference can be in value type, that is one can convert
between SNORM, UNORM, SRGB, FLOAT, etc., as long as the base format is
the same (e.g. PIPE_FORMAT_R8G8B8A8)?
> + struct pipe_texture *texture; /**< texture into which this is a view */
>
> + struct pipe_context *context; /**< context this view belongs to */
>
> Is this for debugging? No other state object has a pointer to context so far.
>
>
Had this object been created by pipe_screen, it would have a reference
to a screen that created it. Sampler view is a first "resource" type
that is created by pipe_context. We want to migrate other types to
pipe_context as well. I suppose once pipe_texture has a reference to
pipe_context, we could remove it from here, but in the meantime we will
need it in e.g. pipe_sampler_view_reference().
> + unsigned first_level:8; /**< first mipmap level */
> + unsigned num_levels:8; /**< number of mipamp levels */
>
> I don't care much, but we've been using last_level instead of num_levels
> elsewhere. Is there a particular reason to use num_levels here?
>
> s/mipamp/mipmap/ in comment.
>
Makes sense, I will change it.
> + unsigned swizzle_r:3; /**< PIPE_SWIZZLE_x for red component */
> + unsigned swizzle_g:3; /**< PIPE_SWIZZLE_x for green component */
> + unsigned swizzle_b:3; /**< PIPE_SWIZZLE_x for blue component */
> + unsigned swizzle_a:3; /**< PIPE_SWIZZLE_x for alpha component */
> +};
> +
> +
> +/**
> * Transfer object. For data transfer to/from a texture.
> */
> struct pipe_transfer
>
>
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> b/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> index b30a075..2df86a0 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_sampler.c
>
> [...]
>
>
>> +struct pipe_sampler_view *
>> +llvmpipe_create_sampler_view(struct pipe_context *pipe,
>> + struct pipe_texture *texture,
>> + const struct pipe_sampler_view *templ)
>> +{
>> + struct pipe_sampler_view *view = CALLOC_STRUCT(pipe_sampler_view);
>>
>
> Need to handle out of memory here.
>
>
Will fix it, thanks.
>> + *view = *templ;
>> + view->reference.count = 1;
>> + view->texture = NULL;
>> + pipe_texture_reference(&view->texture, texture);
>> + view->context = pipe;
>> +
>> + return view;
>> +}
>>
>
> The rest looks great to me. It's a very useful gallium interface change.
> I particularly like how you eased migration with
> cso_set_sampler_textures().
>
> BTW, I noticed you commented out pipe video code. Everybody agreed on
> removing it from master and mature pipe video on a branch, but we never
> got around to do it. I'll remove this code in the next few days.
>
>
Thanks for having a look.
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev
