Am 20.08.2014 18:33, schrieb Ilia Mirkin: > On Wed, Aug 20, 2014 at 12:22 PM, Jose Fonseca <jfons...@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 <jfons...@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 >>>>>> <srol...@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/library/windows/desktop/ff476202.aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=F4msKE2WxRzA%2BwN%2B25muztFm5TSPwE8HKJfWfR2NgfY%3D%0A&m=x03OmuVWAQgfFbsFB2SLMLwSYavxkU8Zsypu9lEIkpg%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. > This was also my first thought about fixing this after Roland pointed > out the issue. Yes definitely for pipe_sampler_view - d3d10 also has it on the render target / depth stencil views, though so far I'm not convinced there's any value in that (the addressing of cube maps / arrays, 1d / 1d arrays is entirely the same in all cases, what matters is really the first and last layer only).
> >> 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; Make it pipe_texture_target 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. > Yes I think it would be a nice cleanup to split it up into two enums. I was mostly proposing just reusing the same enum and keeping pipe_texture_target the same because it would require less code change. But maybe that could come back haunting us later, I agree it would be cleaner if they are separated. Roland _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev