Menu
▾
▴
mesa3d-dev
|
From: michal <mi...@vm...> - 2010-03-11 11:16:30
|
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. |
|
From: Keith W. <ke...@vm...> - 2010-03-11 13:22:00
|
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). Michal, I've got some issues with the way the sampler views are being generated and used inside the CSO module. The point of a sampler view is that it gives the driver an opportunity to do expensive operations required for special sampling modes (which may include copying surface data if hardware is deficient in some way). This approach works if a sampler view is created once, then used multiple times before being deleted. Unfortunately, it seems the changes to support this in the CSO module provide only a single-shot usage model. Sampler views are created in cso_set_XXX_sampler_textures, bound to the context, and then dereferenced/destroyed on the next bind. To make this change worthwhile, we'd want to somehow cache sampler views and reuse them on multiple draws. Currently drivers that implement views internally hang them off the relevant texture. The choices in this branch are to do it in the CSO module, or push it up to the state tracker. Keith |
|
From: Keith W. <ke...@vm...> - 2010-03-11 13:39:49
|
On Thu, 2010-03-11 at 05:21 -0800, Keith Whitwell wrote: > 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). > > Michal, > > I've got some issues with the way the sampler views are being generated > and used inside the CSO module. > > The point of a sampler view is that it gives the driver an opportunity > to do expensive operations required for special sampling modes (which > may include copying surface data if hardware is deficient in some way). > > This approach works if a sampler view is created once, then used > multiple times before being deleted. > > Unfortunately, it seems the changes to support this in the CSO module > provide only a single-shot usage model. Sampler views are created in > cso_set_XXX_sampler_textures, bound to the context, and then > dereferenced/destroyed on the next bind. > > To make this change worthwhile, we'd want to somehow cache sampler views > and reuse them on multiple draws. Currently drivers that implement > views internally hang them off the relevant texture. > > The choices in this branch are to do it in the CSO module, or push it up > to the state tracker. Looking at it a little, I suspect that doing it in the CSO module will be the least painful, though you'll need to be careful that caching sampler views doesn't cause texture references to never go to zero. Either way, it looks like there is a bit of work still to do on this branch. Keith |
|
From: José F. <jfo...@vm...> - 2010-03-11 13:53:27
|
On Thu, 2010-03-11 at 05:21 -0800, Keith Whitwell wrote: > 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). > > Michal, > > I've got some issues with the way the sampler views are being generated > and used inside the CSO module. > > The point of a sampler view is that it gives the driver an opportunity > to do expensive operations required for special sampling modes (which > may include copying surface data if hardware is deficient in some way). > > This approach works if a sampler view is created once, then used > multiple times before being deleted. > > Unfortunately, it seems the changes to support this in the CSO module > provide only a single-shot usage model. Sampler views are created in > cso_set_XXX_sampler_textures, bound to the context, and then > dereferenced/destroyed on the next bind. Although I endorse cso_set_XXX_sampler_textures for migration, I agree entirely with you. Perhaps we could make this clear in the comments. > To make this change worthwhile, we'd want to somehow cache sampler views > and reuse them on multiple draws. Currently drivers that implement > views internally hang them off the relevant texture. > > The choices in this branch are to do it in the CSO module, or push it up > to the state tracker. Caching in the cso module is certainly feasible, but regardless where it is done, care must be taken because the smapler view state includes texture pointers. Worse, it holds on to a texture reference. So at the very least the cso needs an entrypoint for flushing views for a given texture that will be called when the st is done with the texture. Jose |
|
From: michal <mi...@vm...> - 2010-03-11 14:05:18
|
Keith Whitwell wrote on 2010-03-11 14:21: > 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). >> > > Michal, > > I've got some issues with the way the sampler views are being generated > and used inside the CSO module. > > The point of a sampler view is that it gives the driver an opportunity > to do expensive operations required for special sampling modes (which > may include copying surface data if hardware is deficient in some way). > > This approach works if a sampler view is created once, then used > multiple times before being deleted. > > Unfortunately, it seems the changes to support this in the CSO module > provide only a single-shot usage model. Sampler views are created in > cso_set_XXX_sampler_textures, bound to the context, and then > dereferenced/destroyed on the next bind. > > The reason CSO code looks like this is because it was meant to be an itermediate step towards migration to sampler view model. Fully converting all existing state trackers is non-trivial and thus I chose this conservative approach. State trackers that do not care about extra features a sampler view provides will keep using this one-shot CSO interface with the hope that creation of sampler objects is lighweight (format matches texture format, swizzle matches native texel layout, etc.). Ideally, everybody moves on and we stop using CSO for sampler views. I prefer putting my effort into incremental migration of state trackers rather than caching something that by definition doesn't need to be cached. Thanks for having a look. > To make this change worthwhile, we'd want to somehow cache sampler views > and reuse them on multiple draws. Currently drivers that implement > views internally hang them off the relevant texture. > > The choices in this branch are to do it in the CSO module, or push it up > to the state tracker. > > |
|
From: Keith W. <ke...@vm...> - 2010-03-11 15:26:06
|
On Thu, 2010-03-11 at 06:05 -0800, michal wrote: > Keith Whitwell wrote on 2010-03-11 14:21: > > 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). > >> > > > > Michal, > > > > I've got some issues with the way the sampler views are being generated > > and used inside the CSO module. > > > > The point of a sampler view is that it gives the driver an opportunity > > to do expensive operations required for special sampling modes (which > > may include copying surface data if hardware is deficient in some way). > > > > This approach works if a sampler view is created once, then used > > multiple times before being deleted. > > > > Unfortunately, it seems the changes to support this in the CSO module > > provide only a single-shot usage model. Sampler views are created in > > cso_set_XXX_sampler_textures, bound to the context, and then > > dereferenced/destroyed on the next bind. > > > > > The reason CSO code looks like this is because it was meant to be an > itermediate step towards migration to sampler view model. Fully > converting all existing state trackers is non-trivial and thus I chose > this conservative approach. State trackers that do not care about extra > features a sampler view provides will keep using this one-shot CSO > interface with the hope that creation of sampler objects is lighweight > (format matches texture format, swizzle matches native texel layout, > etc.). On the surface, this hope isn't likely to be fulfilled - lots of hardware doesn't support non-zero first_level. Most cases of drivers implementing sampler views internally are to catch this issue. Of course, it seems like your branch so leaves the existing driver-specific sampler view code in place, so that there are potentially two implementations of sampler views in those drivers. I guess this means that you can get away with the current implementation for now, but it prevents drivers actually taking advantage of the fact that these entities exist in the interface -- they will continue to have to duplicate the concept internally until the state trackers and/or CSO module start caching views. > Ideally, everybody moves on and we stop using CSO for sampler > views. I prefer putting my effort into incremental migration of state > trackers rather than caching something that by definition doesn't need > to be cached. The CSO module exists to manage this type of caching on behalf of state trackers. I would have thought that this was a sensible extension of the existing purpose of the CSO module. Won't all state-trackers implementing APIs which don't expose sampler views to the application require essentially the same caching logic, as is the case with regular state? Wouldn't it be least effort to do that caching once only in the CSO module? Keith |
|
From: michal <mi...@vm...> - 2010-03-12 14:01:02
|
michal wrote on 2010-03-11 17:59: > Keith Whitwell wrote on 2010-03-11 16:16: > >> On Thu, 2010-03-11 at 06:05 -0800, michal wrote: >> >> >>> Keith Whitwell wrote on 2010-03-11 14:21: >>> >>> >>>> 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). >>>>> >>>>> >>>>> >>>> Michal, >>>> >>>> I've got some issues with the way the sampler views are being generated >>>> and used inside the CSO module. >>>> >>>> The point of a sampler view is that it gives the driver an opportunity >>>> to do expensive operations required for special sampling modes (which >>>> may include copying surface data if hardware is deficient in some way). >>>> >>>> This approach works if a sampler view is created once, then used >>>> multiple times before being deleted. >>>> >>>> Unfortunately, it seems the changes to support this in the CSO module >>>> provide only a single-shot usage model. Sampler views are created in >>>> cso_set_XXX_sampler_textures, bound to the context, and then >>>> dereferenced/destroyed on the next bind. >>>> >>>> >>>> >>>> >>> The reason CSO code looks like this is because it was meant to be an >>> itermediate step towards migration to sampler view model. Fully >>> converting all existing state trackers is non-trivial and thus I chose >>> this conservative approach. State trackers that do not care about extra >>> features a sampler view provides will keep using this one-shot CSO >>> interface with the hope that creation of sampler objects is lighweight >>> (format matches texture format, swizzle matches native texel layout, >>> etc.). >>> >>> >> On the surface, this hope isn't likely to be fulfilled - lots of >> hardware doesn't support non-zero first_level. Most cases of drivers >> implementing sampler views internally are to catch this issue. >> >> Of course, it seems like your branch so leaves the existing >> driver-specific sampler view code in place, so that there are >> potentially two implementations of sampler views in those drivers. >> >> I guess this means that you can get away with the current implementation >> for now, but it prevents drivers actually taking advantage of the fact >> that these entities exist in the interface -- they will continue to have >> to duplicate the concept internally until the state trackers and/or CSO >> module start caching views. >> >> >> >>> Ideally, everybody moves on and we stop using CSO for sampler >>> views. I prefer putting my effort into incremental migration of state >>> trackers rather than caching something that by definition doesn't need >>> to be cached. >>> >>> >> The CSO module exists to manage this type of caching on behalf of state >> trackers. I would have thought that this was a sensible extension of >> the existing purpose of the CSO module. >> >> Won't all state-trackers implementing APIs which don't expose sampler >> views to the application require essentially the same caching logic, as >> is the case with regular state? Wouldn't it be least effort to do that >> caching once only in the CSO module? >> >> > OK, I see your point. I will make the necessary changes and ping you > when that's done. > > Keith, I changed my mind, went ahead and implemented sampler view caching in mesa state tracker, rather than inside cso context. I strongly believe that doing caching on cso side would be slower and more complicated. A state tracker has a better understanding of the relationship between a texture and sampler view. In case of mesa, this is trivial 1-to-1 mapping. Later, when we'll need more sampler views per texture, we can have a per-texture cache for that, and yes, the code for that would be in cso. There are two other state trackers that need to be fixed: xorg and vega. The transition should be similar to mesa -- I can help with doing that, but I can't do it myself. Once that's done we can purge one-shot sampler view wrappers. What do you think? |
|
From: Jerome G. <gl...@fr...> - 2010-03-11 16:59:12
|
On Thu, Mar 11, 2010 at 03:16:32PM +0000, Keith Whitwell wrote: > On Thu, 2010-03-11 at 06:05 -0800, michal wrote: > > Keith Whitwell wrote on 2010-03-11 14:21: > > > 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). > > >> > > > > > > Michal, > > > > > > I've got some issues with the way the sampler views are being generated > > > and used inside the CSO module. > > > > > > The point of a sampler view is that it gives the driver an opportunity > > > to do expensive operations required for special sampling modes (which > > > may include copying surface data if hardware is deficient in some way). > > > > > > This approach works if a sampler view is created once, then used > > > multiple times before being deleted. > > > > > > Unfortunately, it seems the changes to support this in the CSO module > > > provide only a single-shot usage model. Sampler views are created in > > > cso_set_XXX_sampler_textures, bound to the context, and then > > > dereferenced/destroyed on the next bind. > > > > > > > > The reason CSO code looks like this is because it was meant to be an > > itermediate step towards migration to sampler view model. Fully > > converting all existing state trackers is non-trivial and thus I chose > > this conservative approach. State trackers that do not care about extra > > features a sampler view provides will keep using this one-shot CSO > > interface with the hope that creation of sampler objects is lighweight > > (format matches texture format, swizzle matches native texel layout, > > etc.). > > On the surface, this hope isn't likely to be fulfilled - lots of > hardware doesn't support non-zero first_level. Most cases of drivers > implementing sampler views internally are to catch this issue. > > Of course, it seems like your branch so leaves the existing > driver-specific sampler view code in place, so that there are > potentially two implementations of sampler views in those drivers. > > I guess this means that you can get away with the current implementation > for now, but it prevents drivers actually taking advantage of the fact > that these entities exist in the interface -- they will continue to have > to duplicate the concept internally until the state trackers and/or CSO > module start caching views. > > > Ideally, everybody moves on and we stop using CSO for sampler > > views. I prefer putting my effort into incremental migration of state > > trackers rather than caching something that by definition doesn't need > > to be cached. > > The CSO module exists to manage this type of caching on behalf of state > trackers. I would have thought that this was a sensible extension of > the existing purpose of the CSO module. > > Won't all state-trackers implementing APIs which don't expose sampler > views to the application require essentially the same caching logic, as > is the case with regular state? Wouldn't it be least effort to do that > caching once only in the CSO module? > > Keith > I didn't read anything about sampler view stuff, thus i can be completely wrong here, but from driver pov i prefer having most of the state going through create,bind,destroy pattern ie having the CSO doing the caching. Cheers, Jerome |
|
From: Keith W. <ke...@vm...> - 2010-03-11 17:02:34
|
On Thu, 2010-03-11 at 08:59 -0800, Jerome Glisse wrote: > On Thu, Mar 11, 2010 at 03:16:32PM +0000, Keith Whitwell wrote: > > On Thu, 2010-03-11 at 06:05 -0800, michal wrote: > > > Keith Whitwell wrote on 2010-03-11 14:21: > > > > 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). > > > >> > > > > > > > > Michal, > > > > > > > > I've got some issues with the way the sampler views are being generated > > > > and used inside the CSO module. > > > > > > > > The point of a sampler view is that it gives the driver an opportunity > > > > to do expensive operations required for special sampling modes (which > > > > may include copying surface data if hardware is deficient in some way). > > > > > > > > This approach works if a sampler view is created once, then used > > > > multiple times before being deleted. > > > > > > > > Unfortunately, it seems the changes to support this in the CSO module > > > > provide only a single-shot usage model. Sampler views are created in > > > > cso_set_XXX_sampler_textures, bound to the context, and then > > > > dereferenced/destroyed on the next bind. > > > > > > > > > > > The reason CSO code looks like this is because it was meant to be an > > > itermediate step towards migration to sampler view model. Fully > > > converting all existing state trackers is non-trivial and thus I chose > > > this conservative approach. State trackers that do not care about extra > > > features a sampler view provides will keep using this one-shot CSO > > > interface with the hope that creation of sampler objects is lighweight > > > (format matches texture format, swizzle matches native texel layout, > > > etc.). > > > > On the surface, this hope isn't likely to be fulfilled - lots of > > hardware doesn't support non-zero first_level. Most cases of drivers > > implementing sampler views internally are to catch this issue. > > > > Of course, it seems like your branch so leaves the existing > > driver-specific sampler view code in place, so that there are > > potentially two implementations of sampler views in those drivers. > > > > I guess this means that you can get away with the current implementation > > for now, but it prevents drivers actually taking advantage of the fact > > that these entities exist in the interface -- they will continue to have > > to duplicate the concept internally until the state trackers and/or CSO > > module start caching views. > > > > > Ideally, everybody moves on and we stop using CSO for sampler > > > views. I prefer putting my effort into incremental migration of state > > > trackers rather than caching something that by definition doesn't need > > > to be cached. > > > > The CSO module exists to manage this type of caching on behalf of state > > trackers. I would have thought that this was a sensible extension of > > the existing purpose of the CSO module. > > > > Won't all state-trackers implementing APIs which don't expose sampler > > views to the application require essentially the same caching logic, as > > is the case with regular state? Wouldn't it be least effort to do that > > caching once only in the CSO module? > > > > Keith > > > > I didn't read anything about sampler view stuff, thus i can be > completely wrong here, but from driver pov i prefer having most > of the state going through create,bind,destroy pattern ie having > the CSO doing the caching. Jerome, The driver view in both cases is as you describe - it's just a question of whether the state tracker does the create/bind/destroy itself, or if we can build a helper module to do that logic for it. Keith |
|
From: michal <mi...@vm...> - 2010-03-15 14:08:53
|
michal wrote on 2010-03-12 15:00: > michal wrote on 2010-03-11 17:59: > >> Keith Whitwell wrote on 2010-03-11 16:16: >> >> >>> On Thu, 2010-03-11 at 06:05 -0800, michal wrote: >>> >>> >>> >>>> Keith Whitwell wrote on 2010-03-11 14:21: >>>> >>>> >>>> >>>>> 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). >>>>>> >>>>>> >>>>>> >>>>>> >>>>> Michal, >>>>> >>>>> I've got some issues with the way the sampler views are being generated >>>>> and used inside the CSO module. >>>>> >>>>> The point of a sampler view is that it gives the driver an opportunity >>>>> to do expensive operations required for special sampling modes (which >>>>> may include copying surface data if hardware is deficient in some way). >>>>> >>>>> This approach works if a sampler view is created once, then used >>>>> multiple times before being deleted. >>>>> >>>>> Unfortunately, it seems the changes to support this in the CSO module >>>>> provide only a single-shot usage model. Sampler views are created in >>>>> cso_set_XXX_sampler_textures, bound to the context, and then >>>>> dereferenced/destroyed on the next bind. >>>>> >>>>> >>>>> >>>>> >>>>> >>>> The reason CSO code looks like this is because it was meant to be an >>>> itermediate step towards migration to sampler view model. Fully >>>> converting all existing state trackers is non-trivial and thus I chose >>>> this conservative approach. State trackers that do not care about extra >>>> features a sampler view provides will keep using this one-shot CSO >>>> interface with the hope that creation of sampler objects is lighweight >>>> (format matches texture format, swizzle matches native texel layout, >>>> etc.). >>>> >>>> >>>> >>> On the surface, this hope isn't likely to be fulfilled - lots of >>> hardware doesn't support non-zero first_level. Most cases of drivers >>> implementing sampler views internally are to catch this issue. >>> >>> Of course, it seems like your branch so leaves the existing >>> driver-specific sampler view code in place, so that there are >>> potentially two implementations of sampler views in those drivers. >>> >>> I guess this means that you can get away with the current implementation >>> for now, but it prevents drivers actually taking advantage of the fact >>> that these entities exist in the interface -- they will continue to have >>> to duplicate the concept internally until the state trackers and/or CSO >>> module start caching views. >>> >>> >>> >>> >>>> Ideally, everybody moves on and we stop using CSO for sampler >>>> views. I prefer putting my effort into incremental migration of state >>>> trackers rather than caching something that by definition doesn't need >>>> to be cached. >>>> >>>> >>>> >>> The CSO module exists to manage this type of caching on behalf of state >>> trackers. I would have thought that this was a sensible extension of >>> the existing purpose of the CSO module. >>> >>> Won't all state-trackers implementing APIs which don't expose sampler >>> views to the application require essentially the same caching logic, as >>> is the case with regular state? Wouldn't it be least effort to do that >>> caching once only in the CSO module? >>> >>> >>> >> OK, I see your point. I will make the necessary changes and ping you >> when that's done. >> >> >> > Keith, > > I changed my mind, went ahead and implemented sampler view caching in > mesa state tracker, rather than inside cso context. > > I strongly believe that doing caching on cso side would be slower and > more complicated. A state tracker has a better understanding of the > relationship between a texture and sampler view. In case of mesa, this > is trivial 1-to-1 mapping. Later, when we'll need more sampler views per > texture, we can have a per-texture cache for that, and yes, the code for > that would be in cso. > > There are two other state trackers that need to be fixed: xorg and vega. > The transition should be similar to mesa -- I can help with doing that, > but I can't do it myself. Once that's done we can purge one-shot sampler > view wrappers. > > What do you think? > > Keith, I just finished transforming mesa and auxiliary modules to new sampler view interfaces. The remaining bits are vega and xorg state trackers -- I will need help with them, but they could be fixed after the merge, as they are not broken, and just set sampler view in suboptimal fashion. Please review, thanks. |
|
From: Keith W. <ke...@vm...> - 2010-03-15 14:19:37
|
On Mon, 2010-03-15 at 07:08 -0700, michal wrote: > michal wrote on 2010-03-12 15:00: > > michal wrote on 2010-03-11 17:59: > > > >> Keith Whitwell wrote on 2010-03-11 16:16: > >> > >> > >>> On Thu, 2010-03-11 at 06:05 -0800, michal wrote: > >>> > >>> > >>> > >>>> Keith Whitwell wrote on 2010-03-11 14:21: > >>>> > >>>> > >>>> > >>>>> 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). > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>> Michal, > >>>>> > >>>>> I've got some issues with the way the sampler views are being generated > >>>>> and used inside the CSO module. > >>>>> > >>>>> The point of a sampler view is that it gives the driver an opportunity > >>>>> to do expensive operations required for special sampling modes (which > >>>>> may include copying surface data if hardware is deficient in some way). > >>>>> > >>>>> This approach works if a sampler view is created once, then used > >>>>> multiple times before being deleted. > >>>>> > >>>>> Unfortunately, it seems the changes to support this in the CSO module > >>>>> provide only a single-shot usage model. Sampler views are created in > >>>>> cso_set_XXX_sampler_textures, bound to the context, and then > >>>>> dereferenced/destroyed on the next bind. > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>> The reason CSO code looks like this is because it was meant to be an > >>>> itermediate step towards migration to sampler view model. Fully > >>>> converting all existing state trackers is non-trivial and thus I chose > >>>> this conservative approach. State trackers that do not care about extra > >>>> features a sampler view provides will keep using this one-shot CSO > >>>> interface with the hope that creation of sampler objects is lighweight > >>>> (format matches texture format, swizzle matches native texel layout, > >>>> etc.). > >>>> > >>>> > >>>> > >>> On the surface, this hope isn't likely to be fulfilled - lots of > >>> hardware doesn't support non-zero first_level. Most cases of drivers > >>> implementing sampler views internally are to catch this issue. > >>> > >>> Of course, it seems like your branch so leaves the existing > >>> driver-specific sampler view code in place, so that there are > >>> potentially two implementations of sampler views in those drivers. > >>> > >>> I guess this means that you can get away with the current implementation > >>> for now, but it prevents drivers actually taking advantage of the fact > >>> that these entities exist in the interface -- they will continue to have > >>> to duplicate the concept internally until the state trackers and/or CSO > >>> module start caching views. > >>> > >>> > >>> > >>> > >>>> Ideally, everybody moves on and we stop using CSO for sampler > >>>> views. I prefer putting my effort into incremental migration of state > >>>> trackers rather than caching something that by definition doesn't need > >>>> to be cached. > >>>> > >>>> > >>>> > >>> The CSO module exists to manage this type of caching on behalf of state > >>> trackers. I would have thought that this was a sensible extension of > >>> the existing purpose of the CSO module. > >>> > >>> Won't all state-trackers implementing APIs which don't expose sampler > >>> views to the application require essentially the same caching logic, as > >>> is the case with regular state? Wouldn't it be least effort to do that > >>> caching once only in the CSO module? > >>> > >>> > >>> > >> OK, I see your point. I will make the necessary changes and ping you > >> when that's done. > >> > >> > >> > > Keith, > > > > I changed my mind, went ahead and implemented sampler view caching in > > mesa state tracker, rather than inside cso context. > > > > I strongly believe that doing caching on cso side would be slower and > > more complicated. A state tracker has a better understanding of the > > relationship between a texture and sampler view. In case of mesa, this > > is trivial 1-to-1 mapping. Later, when we'll need more sampler views per > > texture, we can have a per-texture cache for that, and yes, the code for > > that would be in cso. > > > > There are two other state trackers that need to be fixed: xorg and vega. > > The transition should be similar to mesa -- I can help with doing that, > > but I can't do it myself. Once that's done we can purge one-shot sampler > > view wrappers. > > > > What do you think? > > > > > Keith, > > I just finished transforming mesa and auxiliary modules to new sampler > view interfaces. The remaining bits are vega and xorg state trackers -- > I will need help with them, but they could be fixed after the merge, as > they are not broken, and just set sampler view in suboptimal fashion. > > Please review, thanks. Michal, Did you get a chance to look at the double-refcounting (views + textures) in the cso module? Keith |
|
From: michal <mi...@vm...> - 2010-03-15 14:24:56
|
Keith Whitwell wrote on 2010-03-15 15:19: > On Mon, 2010-03-15 at 07:08 -0700, michal wrote: > >> michal wrote on 2010-03-12 15:00: >> >>> michal wrote on 2010-03-11 17:59: >>> >>> >>>> Keith Whitwell wrote on 2010-03-11 16:16: >>>> >>>> >>>> >>>>> On Thu, 2010-03-11 at 06:05 -0800, michal wrote: >>>>> >>>>> >>>>> >>>>> >>>>>> Keith Whitwell wrote on 2010-03-11 14:21: >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> 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). >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> Michal, >>>>>>> >>>>>>> I've got some issues with the way the sampler views are being generated >>>>>>> and used inside the CSO module. >>>>>>> >>>>>>> The point of a sampler view is that it gives the driver an opportunity >>>>>>> to do expensive operations required for special sampling modes (which >>>>>>> may include copying surface data if hardware is deficient in some way). >>>>>>> >>>>>>> This approach works if a sampler view is created once, then used >>>>>>> multiple times before being deleted. >>>>>>> >>>>>>> Unfortunately, it seems the changes to support this in the CSO module >>>>>>> provide only a single-shot usage model. Sampler views are created in >>>>>>> cso_set_XXX_sampler_textures, bound to the context, and then >>>>>>> dereferenced/destroyed on the next bind. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> The reason CSO code looks like this is because it was meant to be an >>>>>> itermediate step towards migration to sampler view model. Fully >>>>>> converting all existing state trackers is non-trivial and thus I chose >>>>>> this conservative approach. State trackers that do not care about extra >>>>>> features a sampler view provides will keep using this one-shot CSO >>>>>> interface with the hope that creation of sampler objects is lighweight >>>>>> (format matches texture format, swizzle matches native texel layout, >>>>>> etc.). >>>>>> >>>>>> >>>>>> >>>>>> >>>>> On the surface, this hope isn't likely to be fulfilled - lots of >>>>> hardware doesn't support non-zero first_level. Most cases of drivers >>>>> implementing sampler views internally are to catch this issue. >>>>> >>>>> Of course, it seems like your branch so leaves the existing >>>>> driver-specific sampler view code in place, so that there are >>>>> potentially two implementations of sampler views in those drivers. >>>>> >>>>> I guess this means that you can get away with the current implementation >>>>> for now, but it prevents drivers actually taking advantage of the fact >>>>> that these entities exist in the interface -- they will continue to have >>>>> to duplicate the concept internally until the state trackers and/or CSO >>>>> module start caching views. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>>> Ideally, everybody moves on and we stop using CSO for sampler >>>>>> views. I prefer putting my effort into incremental migration of state >>>>>> trackers rather than caching something that by definition doesn't need >>>>>> to be cached. >>>>>> >>>>>> >>>>>> >>>>>> >>>>> The CSO module exists to manage this type of caching on behalf of state >>>>> trackers. I would have thought that this was a sensible extension of >>>>> the existing purpose of the CSO module. >>>>> >>>>> Won't all state-trackers implementing APIs which don't expose sampler >>>>> views to the application require essentially the same caching logic, as >>>>> is the case with regular state? Wouldn't it be least effort to do that >>>>> caching once only in the CSO module? >>>>> >>>>> >>>>> >>>>> >>>> OK, I see your point. I will make the necessary changes and ping you >>>> when that's done. >>>> >>>> >>>> >>>> >>> Keith, >>> >>> I changed my mind, went ahead and implemented sampler view caching in >>> mesa state tracker, rather than inside cso context. >>> >>> I strongly believe that doing caching on cso side would be slower and >>> more complicated. A state tracker has a better understanding of the >>> relationship between a texture and sampler view. In case of mesa, this >>> is trivial 1-to-1 mapping. Later, when we'll need more sampler views per >>> texture, we can have a per-texture cache for that, and yes, the code for >>> that would be in cso. >>> >>> There are two other state trackers that need to be fixed: xorg and vega. >>> The transition should be similar to mesa -- I can help with doing that, >>> but I can't do it myself. Once that's done we can purge one-shot sampler >>> view wrappers. >>> >>> What do you think? >>> >>> >>> >> Keith, >> >> I just finished transforming mesa and auxiliary modules to new sampler >> view interfaces. The remaining bits are vega and xorg state trackers -- >> I will need help with them, but they could be fixed after the merge, as >> they are not broken, and just set sampler view in suboptimal fashion. >> >> Please review, thanks. >> > > > Michal, > > Did you get a chance to look at the double-refcounting (views + > textures) in the cso module? > > > They will go away once all the remaining consumers of cso_set/save/restore_sampler_textures() switch to cso_*_fragment_sampler_views(). |
|
From: Keith W. <ke...@vm...> - 2010-03-15 14:26:50
|
On Mon, 2010-03-15 at 07:24 -0700, michal wrote: > Keith Whitwell wrote on 2010-03-15 15:19: > > On Mon, 2010-03-15 at 07:08 -0700, michal wrote: > > > >> michal wrote on 2010-03-12 15:00: > >> > >>> michal wrote on 2010-03-11 17:59: > >>> > >>> > >>>> Keith Whitwell wrote on 2010-03-11 16:16: > >>>> > >>>> > >>>> > >>>>> On Thu, 2010-03-11 at 06:05 -0800, michal wrote: > >>>>> > >>>>> > >>>>> > >>>>> > >>>>>> Keith Whitwell wrote on 2010-03-11 14:21: > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>>> 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). > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>> Michal, > >>>>>>> > >>>>>>> I've got some issues with the way the sampler views are being generated > >>>>>>> and used inside the CSO module. > >>>>>>> > >>>>>>> The point of a sampler view is that it gives the driver an opportunity > >>>>>>> to do expensive operations required for special sampling modes (which > >>>>>>> may include copying surface data if hardware is deficient in some way). > >>>>>>> > >>>>>>> This approach works if a sampler view is created once, then used > >>>>>>> multiple times before being deleted. > >>>>>>> > >>>>>>> Unfortunately, it seems the changes to support this in the CSO module > >>>>>>> provide only a single-shot usage model. Sampler views are created in > >>>>>>> cso_set_XXX_sampler_textures, bound to the context, and then > >>>>>>> dereferenced/destroyed on the next bind. > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>> The reason CSO code looks like this is because it was meant to be an > >>>>>> itermediate step towards migration to sampler view model. Fully > >>>>>> converting all existing state trackers is non-trivial and thus I chose > >>>>>> this conservative approach. State trackers that do not care about extra > >>>>>> features a sampler view provides will keep using this one-shot CSO > >>>>>> interface with the hope that creation of sampler objects is lighweight > >>>>>> (format matches texture format, swizzle matches native texel layout, > >>>>>> etc.). > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>> On the surface, this hope isn't likely to be fulfilled - lots of > >>>>> hardware doesn't support non-zero first_level. Most cases of drivers > >>>>> implementing sampler views internally are to catch this issue. > >>>>> > >>>>> Of course, it seems like your branch so leaves the existing > >>>>> driver-specific sampler view code in place, so that there are > >>>>> potentially two implementations of sampler views in those drivers. > >>>>> > >>>>> I guess this means that you can get away with the current implementation > >>>>> for now, but it prevents drivers actually taking advantage of the fact > >>>>> that these entities exist in the interface -- they will continue to have > >>>>> to duplicate the concept internally until the state trackers and/or CSO > >>>>> module start caching views. > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>>> Ideally, everybody moves on and we stop using CSO for sampler > >>>>>> views. I prefer putting my effort into incremental migration of state > >>>>>> trackers rather than caching something that by definition doesn't need > >>>>>> to be cached. > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>> The CSO module exists to manage this type of caching on behalf of state > >>>>> trackers. I would have thought that this was a sensible extension of > >>>>> the existing purpose of the CSO module. > >>>>> > >>>>> Won't all state-trackers implementing APIs which don't expose sampler > >>>>> views to the application require essentially the same caching logic, as > >>>>> is the case with regular state? Wouldn't it be least effort to do that > >>>>> caching once only in the CSO module? > >>>>> > >>>>> > >>>>> > >>>>> > >>>> OK, I see your point. I will make the necessary changes and ping you > >>>> when that's done. > >>>> > >>>> > >>>> > >>>> > >>> Keith, > >>> > >>> I changed my mind, went ahead and implemented sampler view caching in > >>> mesa state tracker, rather than inside cso context. > >>> > >>> I strongly believe that doing caching on cso side would be slower and > >>> more complicated. A state tracker has a better understanding of the > >>> relationship between a texture and sampler view. In case of mesa, this > >>> is trivial 1-to-1 mapping. Later, when we'll need more sampler views per > >>> texture, we can have a per-texture cache for that, and yes, the code for > >>> that would be in cso. > >>> > >>> There are two other state trackers that need to be fixed: xorg and vega. > >>> The transition should be similar to mesa -- I can help with doing that, > >>> but I can't do it myself. Once that's done we can purge one-shot sampler > >>> view wrappers. > >>> > >>> What do you think? > >>> > >>> > >>> > >> Keith, > >> > >> I just finished transforming mesa and auxiliary modules to new sampler > >> view interfaces. The remaining bits are vega and xorg state trackers -- > >> I will need help with them, but they could be fixed after the merge, as > >> they are not broken, and just set sampler view in suboptimal fashion. > >> > >> Please review, thanks. > >> > > > > > > Michal, > > > > Did you get a chance to look at the double-refcounting (views + > > textures) in the cso module? > > > > > > > They will go away once all the remaining consumers of > cso_set/save/restore_sampler_textures() switch to > cso_*_fragment_sampler_views(). I think they're not needed even now - if you look at the patch I sent you, it seems like the only thing that happens to the texture pointers is they get refcounted - but never used for anything... Keith |
|
From: José F. <jfo...@vm...> - 2010-03-11 13:41:08
|
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.
+ 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.
+ 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.
+ 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.
> + *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.
Jose
|
|
From: michal <mi...@vm...> - 2010-03-11 14:21:18
|
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.
|
|
From: José F. <jfo...@vm...> - 2010-03-11 14:49:45
|
On Thu, 2010-03-11 at 06:21 -0800, michal wrote:
> 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)?
Yes, looking again, this does seem consistent with DX10's
DXGI_FORMAT_xxxxxx_TYPELESS formats.
What about GL's PBOs? I don't know much about them, but I heard that is
possible to use surfaces with a different format from what was original
created. Do you or anybody know how pipe sampler view could be used in
that case?
> > + 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().
If it makes life easier then sounds good.
> > + 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.
Thanks!
Jose
|
|
From: michal <mi...@vm...> - 2010-03-11 16:59:33
|
Keith Whitwell wrote on 2010-03-11 16:16: > On Thu, 2010-03-11 at 06:05 -0800, michal wrote: > >> Keith Whitwell wrote on 2010-03-11 14:21: >> >>> 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). >>>> >>>> >>> Michal, >>> >>> I've got some issues with the way the sampler views are being generated >>> and used inside the CSO module. >>> >>> The point of a sampler view is that it gives the driver an opportunity >>> to do expensive operations required for special sampling modes (which >>> may include copying surface data if hardware is deficient in some way). >>> >>> This approach works if a sampler view is created once, then used >>> multiple times before being deleted. >>> >>> Unfortunately, it seems the changes to support this in the CSO module >>> provide only a single-shot usage model. Sampler views are created in >>> cso_set_XXX_sampler_textures, bound to the context, and then >>> dereferenced/destroyed on the next bind. >>> >>> >>> >> The reason CSO code looks like this is because it was meant to be an >> itermediate step towards migration to sampler view model. Fully >> converting all existing state trackers is non-trivial and thus I chose >> this conservative approach. State trackers that do not care about extra >> features a sampler view provides will keep using this one-shot CSO >> interface with the hope that creation of sampler objects is lighweight >> (format matches texture format, swizzle matches native texel layout, >> etc.). >> > > On the surface, this hope isn't likely to be fulfilled - lots of > hardware doesn't support non-zero first_level. Most cases of drivers > implementing sampler views internally are to catch this issue. > > Of course, it seems like your branch so leaves the existing > driver-specific sampler view code in place, so that there are > potentially two implementations of sampler views in those drivers. > > I guess this means that you can get away with the current implementation > for now, but it prevents drivers actually taking advantage of the fact > that these entities exist in the interface -- they will continue to have > to duplicate the concept internally until the state trackers and/or CSO > module start caching views. > > >> Ideally, everybody moves on and we stop using CSO for sampler >> views. I prefer putting my effort into incremental migration of state >> trackers rather than caching something that by definition doesn't need >> to be cached. >> > > The CSO module exists to manage this type of caching on behalf of state > trackers. I would have thought that this was a sensible extension of > the existing purpose of the CSO module. > > Won't all state-trackers implementing APIs which don't expose sampler > views to the application require essentially the same caching logic, as > is the case with regular state? Wouldn't it be least effort to do that > caching once only in the CSO module? > OK, I see your point. I will make the necessary changes and ping you when that's done. Thanks. |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X