[prev in list] [next in list] [prev in thread] [next in thread]
List: mesa3d-dev
Subject: Re: [Mesa-dev] [PATCH 2/2] mesa/st: add ARB_texture_view support
From: Jose Fonseca <jfonseca () vmware ! com>
Date: 2014-08-20 16:42:58
Message-ID: 53F4D012.6040304 () vmware ! com
[Download RAW message or body]
On 20/08/14 17:33, Ilia Mirkin wrote:
> On Wed, Aug 20, 2014 at 12:22 PM, Jose Fonseca <jfonseca@vmware.com> wrote:
> > On 20/08/14 17:14, Roland Scheidegger wrote:
> > >
> > > Am 20.08.2014 17:55, schrieb Ilia Mirkin:
> > > >
> > > > On Wed, Aug 20, 2014 at 11:47 AM, Jose Fonseca <jfonseca@vmware.com>
> > > > wrote:
> > > > >
> > > > > On 20/08/14 16:31, Ilia Mirkin wrote:
> > > > > >
> > > > > >
> > > > > > Hm, it's not tested. And you're right, that would (most likely) mess
> > > > > > up, since it would only have the pipe_resource's target. Any
> > > > > > suggestions on how to fix it? Should the target be added to
> > > > > > pipe_sampler_view?
> > > > > >
> > > > > > On Wed, Aug 20, 2014 at 11:25 AM, Roland Scheidegger
> > > > > > <sroland@vmware.com>
> > > > > > wrote:
> > > > > > >
> > > > > > >
> > > > > > > Didn't look at it that closely, but I'm pretty surprised this really
> > > > > > > works. One things ARB_texture_view can do is cast cube maps (and cube
> > > > > > > map arrays) to 2d arrays and vice versa (also 1d/2d to the respective
> > > > > > > array type), and we cannot express that in sampler views (yet) (we
> > > > > > > can't
> > > > > > > express it in surfaces neither but there it should not matter). Which
> > > > > > > means the type used in the shader for sampling will not match the
> > > > > > > sampler view, which sounds quite broken to me.
> > > > > > >
> > > > > > > Roland
> > > > > > >
> > > > >
> > > > > Probably the only sane thing to do eliminate the disctinction between
> > > > > PIPE_TEXTURE_FOO and PIPE_TEXTURE_FOO_ARRAY like in
> > > > >
> > > > > https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/en-us/l \
> > > > > ibrary/windows/desktop/ff476202.aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F \
> > > > > 4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=x03OmuVWAQgfFbsFB2SLM \
> > > > > LwSYavxkU8Zsypu9lEIkpg%3D%0A&s=4b0ca75d91e6d53d92658d9e334cf2a73a01efe5667464c969f5c085409052ff
> > > > > ,
> > > > > e.g.,:
> > > > >
> > > > > enum pipe_texture_target {
> > > > > PIPE_BUFFER = 0,
> > > > > PIPE_TEXTURE_1D = 1,
> > > > > PIPE_TEXTURE_2D = 2,
> > > > > PIPE_TEXTURE_3D = 3,
> > > > > PIPE_TEXTURE_CUBE = 4, // Must have same layout as
> > > > > PIPE_TEXTURE_2D
> > > > > PIPE_TEXTURE_RECT = 5,
> > > > > PIPE_TEXTURE_1D_ARRAY = PIPE_TEXTURE_1D,
> > > > > PIPE_TEXTURE_2D_ARRAY = PIPE_TEXTURE_2D,
> > > > > PIPE_TEXTURE_CUBE_ARRAY = PIPE_TEXTURE_CUBE,
> > > > > PIPE_MAX_TEXTURE_TYPES
> > > > > };
> > > > >
> > > > >
> > > > > We could also remove PIPE_TEXTURE_CUBE and have cube-maps be
> > > > > PIPE_TEXTURE_2D
> > > > > with a flag, but that's probably a lot of work. Instead, drivers that
> > > > > want
> > > > > to be able to support ARB_texture_view will need to ensure
> > > > > PIPE_TEXTURE_CUBE/PIPE_TEXTURE_2D layout match.
> > > >
> > > >
> > > > Another quick + cheap alternative (at least looking at nv50/nvc0 code)
> > > > would be to pass a separate target parameter to
> > > > ->create_sampler_view(). That would be enough for nouveau, but perhaps
> > > > not more generally? Take a look at nv50_tex.c:nv50_create_texture_view
> > > > -- it also needs to work out the depth of the texture (presumably to
> > > > deal with out-of-bounds accesses) and that is written to the texture
> > > > info structure.
> > >
> > > Well that should be enough, but I don't think it fits out design.
> >
> >
> >
> > > We've
> > >
> > > encapsulated other "override" information like the format in the view
> > > already, and I see no reason why the target "cast" should be treated any
> > > different.
> >
> >
> > In other words, you're arguing for:
> >
> > diff --git a/src/gallium/include/pipe/p_state.h
> > b/src/gallium/include/pipe/p_state.h
> > index a82686b..c87ac4e 100644
> > --- a/src/gallium/include/pipe/p_state.h
> > +++ b/src/gallium/include/pipe/p_state.h
> > @@ -333,6 +333,7 @@ struct pipe_surface
>
> On struct pipe_sampler_view, I thought... unless I'm misunderstanding.
Yep. My mistake.
> This was also my first thought about fixing this after Roland pointed
> out the issue.
>
> > struct pipe_reference reference;
> > struct pipe_resource *texture; /**< resource into which this is a view
> > */
> > struct pipe_context *context; /**< context this surface belongs to */
> > + enum pipe_texture target;
> > enum pipe_format format;
> >
> > /* XXX width/height should be removed */
> >
> >
> > It's a fair point. And I don't object that solution.
> >
> > Of course, for this to work, drivers will need to treat the _ARRAY and non
> > _ARRAY targets the same when determining the texture layout for this to
> > work.
> >
> >
> > I just felt this would be a good oportunity to slim down pipe_texture_target
> > too. I'm not sure the _ARRAY distinction still matters at this level, but I
> > suppose it doesn't hurt.
>
> Such a cleanup would probably have to be done by someone with a better
> understanding of gallium than me. OTOH if you guys feel like doing it
> the sampler_view way will accrue too much technical debt, that's fine
> too. Unless I hear otherwise, I'm going to try to do it the
> pipe_sampler_view way tonight.
>
> -ilia
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic