Re: [Mesa-dev] State tracker pipe_surface woes...
Am 08.09.2011 09:08, schrieb Marek Olšák: > 2011/9/7 Roland Scheidegger : >> That said I'm not really sure why pipe_sampler_view and pipe_surface >> actually have a context pointer in them, since they are only supposed to >> be used with the context in which they were created I think those >> shouldn't actually exist - surface_destroy and sampler_view_destroy will >> have context as a parameter, and if they aren't shared between contexts >> it's pointless to store the context pointer. Might be a relic from when >> those structs were created/destroyed using screen functions... > > The ctx pointer is there because of pipe_surface_reference and > pipe_sampler_view_reference. When the refcount becomes 0, the > corresponding pipe...reference function uses the ctx pointer to do > ctx->...destroy(ctx, ... > > So that we can destroy objects just by: > pipe_surface_reference(&surface, NULL); > > This ctx pointer also ensures that the destroy function is called with > the right context. Yes. It's arguable though that these really need refcounting - the state tracker could probably create/destroy them like any other object (e.g. they could follow just the normal create/bind/destroy pattern). Only resources really need refcounting. Even with refcounting you could just pass in the context probably, since the context when creating/destroying them always has to be the same anyway. (pipe_surface though definitely needed refcounting in the past because it was a sharable screen entity once used for things like gl system framebuffers which were not backed by a resource.) It works for now though so I'm not really proposing to change it. Roland ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
2011/9/7 Roland Scheidegger : > That said I'm not really sure why pipe_sampler_view and pipe_surface > actually have a context pointer in them, since they are only supposed to > be used with the context in which they were created I think those > shouldn't actually exist - surface_destroy and sampler_view_destroy will > have context as a parameter, and if they aren't shared between contexts > it's pointless to store the context pointer. Might be a relic from when > those structs were created/destroyed using screen functions... The ctx pointer is there because of pipe_surface_reference and pipe_sampler_view_reference. When the refcount becomes 0, the corresponding pipe...reference function uses the ctx pointer to do ctx->...destroy(ctx, ... So that we can destroy objects just by: pipe_surface_reference(&surface, NULL); This ctx pointer also ensures that the destroy function is called with the right context. Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
- Original Message - > Am 07.09.2011 07:33, schrieb Stéphane Marchesin: > > 2011/9/2 Jose Fonseca : > >> - Original Message - > >>> Hi, > >>> > >>> While debugging some code I ran across the following situation: > >>> > >>> - pipe_context c1 is created > >>> - pipe_surface s1 is created > >>> - strb-> surface is set to s1 (s1's refcount goes up) > >>> - pipe_context c1 is destroyed > >>> - strb is destroyed > >>> - strb->surface is destroyed (so s1's refcount is now 0 and we > >>> want > >>> to > >>> destroy it) > >>> > >>> At that point s1 references c1 which is not available any more, > >>> so > >>> when we try to call ctx->surface_destroy to destroy s1 we crash. > >>> > >>> We discussed this a bit on IRC, and we agreed that the proper > >>> solution, since surfaces outlive their context, is to make > >>> surfaces > >>> screen-bound instead. I'm going to implement that unless someone > >>> objects. > >>> > >>> As a side note, the same issue will happen with sampler_views, so > >>> it'll get a similar fix. > >> > >> Sampler views and surfaces were previously objects bound to > >> screen, and we changed that because of poor multithreading > >> semantics. Per-context sampler views / render targets actually > >> matches the 3D APIs semantics better, so I don't think that > >> reverting is the solution. > >> > >> It looks to me that the issue here is that pipe_context should not > >> be destroyed before the surfaces. strb->surface should only be > >> used by one context, and should be destroyed before that context > >> is destroyed. > >> > >> IIUC, strb matches GL renderbuffer semantics and can be shared by > >> multiple context. If so, strb is treating pipe_surfaces as a > >> entity shareable by contexts when really shouldn't. > >> > >> The solution is: > >> - strb can only have pipe_resources, plus the key for the surface > >> (face, level, etc) > > > > Another bit I don't get: since pipe_resources don't have a pointer > > to > > the context, how do you go back to the corresponding > > context/surface > > when all you have is the pipe_resource? > > Short answer, you don't. Why do you need to? > A pipe_surface is conceptually really just a means to supply the > parameters to be used for rendering to a (color or depth) > rendertarget. > So you specify which level, layer(s) etc. of a resource will be used. > That's all, analogous to pipe_sampler_view which does the same for > texture sampling from a resource. Yes. The statetracker either stores the pipe_surface somewhere, or re-creates the pipe_surfaces. And when recreating the driver may or not give the same pipe_surface pointer. The semantics are such that everything should just work regargless. Is up to the pipe driver to ensure that when pipe->flush() is called, any pending drawing on a pipe_surface becomes visible in the parent pipe_resource, i.e, _all_ pipe_surfaces and pipe_sampler_views objects, created by all contexts will pick up the. For pipe drivers where a pipe_surface is just a stateless structure, this is trivial to achieve. For pipe drivers where a pipe_surface has its own VRAM storage (due to unsupported formats/workarounds), this is achieved by queing a blit from that surface private VRAM to the pipe_resource VRAM at flush time. Jose ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
- Original Message - > Am 07.09.2011 02:00, schrieb Stéphane Marchesin: > > 2011/9/6 Roland Scheidegger : > >> Am 07.09.2011 00:01, schrieb Stéphane Marchesin: > >>> 2011/9/3 Jose Fonseca : > > > - Original Message - > > 2011/9/2 Stéphane Marchesin : > >> 2011/9/2 Jose Fonseca : > >>> - Original Message - > Hi, > > While debugging some code I ran across the following > situation: > > - pipe_context c1 is created > - pipe_surface s1 is created > - strb-> surface is set to s1 (s1's refcount goes up) > - pipe_context c1 is destroyed > - strb is destroyed > - strb->surface is destroyed (so s1's refcount is now 0 and > we > want > to > destroy it) > > At that point s1 references c1 which is not available any > more, > so > when we try to call ctx->surface_destroy to destroy s1 we > crash. > > We discussed this a bit on IRC, and we agreed that the > proper > solution, since surfaces outlive their context, is to make > surfaces > screen-bound instead. I'm going to implement that unless > someone > objects. > > As a side note, the same issue will happen with > sampler_views, so > it'll get a similar fix. > >>> > >>> Sampler views and surfaces were previously objects bound to > >>> screen, and we changed that because of poor multithreading > >>> semantics. Per-context sampler views / render targets > >>> actually > >>> matches the 3D APIs semantics better, so I don't think that > >>> reverting is the solution. > >>> > >>> It looks to me that the issue here is that pipe_context > >>> should not > >>> be destroyed before the surfaces. strb->surface should only > >>> be > >>> used by one context, and should be destroyed before that > >>> context > >>> is destroyed. > >>> > >>> IIUC, strb matches GL renderbuffer semantics and can be > >>> shared by > >>> multiple context. If so, strb is treating pipe_surfaces as a > >>> entity shareable by contexts when really shouldn't. > >>> > >>> The solution is: > >>> - strb can only have pipe_resources, plus the key for the > >>> surface > >>> (face, level, etc) > >>> - the pipe_surfaces that are derived should be stored/cached > >>> in > >>> the GLcontext. > >>> - when the GLcontext / pipe_context is being destroy, the > >>> pipe > >>> surfaces can be destroyed before > >>> > >> > >> I don't understand some of it. From what I see, it should be > >> enough, > >> whenever strb binds a surface, to add a pointer to this strb > >> to a > >> list of strb's to the pipe_context. By doing that, we would be > >> able > >> to > >> unbind the surfaces from the strb before we destroy the > >> context. > >> However, pipe_context structures can't reference gl > >> structures, so > >> how > >> would you solve that? > >> > > > > Alright, maybe I'm too tired, I just have to put it in strb... > > My > > other questions still stand though :) > > > > Stéphane > > > > > >> Also, what difference does it make if strb's only have > >> pipe_resources? > > Pipe resources can be shared between contexts, therefore they > should not refer context or context data. > So it is always safe to destroy pipe_resources pipe_contexts on > any order. > > Using your example above, if you replace surface "s1" with > resource "r1", a reference from r1 to c1 would be violating the > semantics. > > >> And why do I need a key? > > "key" is a bad name perhaps. Unless the resource is always a > single level 2d texture, if we replace the strb->surface with a > strb->resource, we will need to specify separately which > face/level is sought. > > >> This all is definitely more complex than it should be. > > May be I'm not understanding what you were proposing. When you > said that > > "the proper solution, since surfaces outlive their context, is > to make > surfaces screen-bound instead" > > I interpreted this statement as moving the > pipe_context::create_surface method to > pipe_screen::create_surface. Was this really your plan or did > you meant something else? > > Because, my understanding is that it should be the other way > around: when we moved the create_surface method from > pipe_screen to pipe_context, we forgot to fix the > st_renderbuffer code to do this properly. > > > > The fix I proposed may seem a bit complex, but keeping > pipe_surfaces as conte
Re: [Mesa-dev] State tracker pipe_surface woes...
Am 07.09.2011 07:33, schrieb Stéphane Marchesin: > 2011/9/2 Jose Fonseca : >> - Original Message - >>> Hi, >>> >>> While debugging some code I ran across the following situation: >>> >>> - pipe_context c1 is created >>> - pipe_surface s1 is created >>> - strb-> surface is set to s1 (s1's refcount goes up) >>> - pipe_context c1 is destroyed >>> - strb is destroyed >>> - strb->surface is destroyed (so s1's refcount is now 0 and we want >>> to >>> destroy it) >>> >>> At that point s1 references c1 which is not available any more, so >>> when we try to call ctx->surface_destroy to destroy s1 we crash. >>> >>> We discussed this a bit on IRC, and we agreed that the proper >>> solution, since surfaces outlive their context, is to make surfaces >>> screen-bound instead. I'm going to implement that unless someone >>> objects. >>> >>> As a side note, the same issue will happen with sampler_views, so >>> it'll get a similar fix. >> >> Sampler views and surfaces were previously objects bound to screen, and we >> changed that because of poor multithreading semantics. Per-context sampler >> views / render targets actually matches the 3D APIs semantics better, so I >> don't think that reverting is the solution. >> >> It looks to me that the issue here is that pipe_context should not be >> destroyed before the surfaces. strb->surface should only be used by one >> context, and should be destroyed before that context is destroyed. >> >> IIUC, strb matches GL renderbuffer semantics and can be shared by multiple >> context. If so, strb is treating pipe_surfaces as a entity shareable by >> contexts when really shouldn't. >> >> The solution is: >> - strb can only have pipe_resources, plus the key for the surface (face, >> level, etc) > > Another bit I don't get: since pipe_resources don't have a pointer to > the context, how do you go back to the corresponding context/surface > when all you have is the pipe_resource? Short answer, you don't. Why do you need to? A pipe_surface is conceptually really just a means to supply the parameters to be used for rendering to a (color or depth) rendertarget. So you specify which level, layer(s) etc. of a resource will be used. That's all, analogous to pipe_sampler_view which does the same for texture sampling from a resource. Roland ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
Am 07.09.2011 02:00, schrieb Stéphane Marchesin: > 2011/9/6 Roland Scheidegger : >> Am 07.09.2011 00:01, schrieb Stéphane Marchesin: >>> 2011/9/3 Jose Fonseca : - Original Message - > 2011/9/2 Stéphane Marchesin : >> 2011/9/2 Jose Fonseca : >>> - Original Message - Hi, While debugging some code I ran across the following situation: - pipe_context c1 is created - pipe_surface s1 is created - strb-> surface is set to s1 (s1's refcount goes up) - pipe_context c1 is destroyed - strb is destroyed - strb->surface is destroyed (so s1's refcount is now 0 and we want to destroy it) At that point s1 references c1 which is not available any more, so when we try to call ctx->surface_destroy to destroy s1 we crash. We discussed this a bit on IRC, and we agreed that the proper solution, since surfaces outlive their context, is to make surfaces screen-bound instead. I'm going to implement that unless someone objects. As a side note, the same issue will happen with sampler_views, so it'll get a similar fix. >>> >>> Sampler views and surfaces were previously objects bound to >>> screen, and we changed that because of poor multithreading >>> semantics. Per-context sampler views / render targets actually >>> matches the 3D APIs semantics better, so I don't think that >>> reverting is the solution. >>> >>> It looks to me that the issue here is that pipe_context should not >>> be destroyed before the surfaces. strb->surface should only be >>> used by one context, and should be destroyed before that context >>> is destroyed. >>> >>> IIUC, strb matches GL renderbuffer semantics and can be shared by >>> multiple context. If so, strb is treating pipe_surfaces as a >>> entity shareable by contexts when really shouldn't. >>> >>> The solution is: >>> - strb can only have pipe_resources, plus the key for the surface >>> (face, level, etc) >>> - the pipe_surfaces that are derived should be stored/cached in >>> the GLcontext. >>> - when the GLcontext / pipe_context is being destroy, the pipe >>> surfaces can be destroyed before >>> >> >> I don't understand some of it. From what I see, it should be >> enough, >> whenever strb binds a surface, to add a pointer to this strb to a >> list of strb's to the pipe_context. By doing that, we would be able >> to >> unbind the surfaces from the strb before we destroy the context. >> However, pipe_context structures can't reference gl structures, so >> how >> would you solve that? >> > > Alright, maybe I'm too tired, I just have to put it in strb... My > other questions still stand though :) > > Stéphane > > >> Also, what difference does it make if strb's only have >> pipe_resources? Pipe resources can be shared between contexts, therefore they should not refer context or context data. So it is always safe to destroy pipe_resources pipe_contexts on any order. Using your example above, if you replace surface "s1" with resource "r1", a reference from r1 to c1 would be violating the semantics. >> And why do I need a key? "key" is a bad name perhaps. Unless the resource is always a single level 2d texture, if we replace the strb->surface with a strb->resource, we will need to specify separately which face/level is sought. >> This all is definitely more complex than it should be. May be I'm not understanding what you were proposing. When you said that "the proper solution, since surfaces outlive their context, is to make surfaces screen-bound instead" I interpreted this statement as moving the pipe_context::create_surface method to pipe_screen::create_surface. Was this really your plan or did you meant something else? Because, my understanding is that it should be the other way around: when we moved the create_surface method from pipe_screen to pipe_context, we forgot to fix the st_renderbuffer code to do this properly. The fix I proposed may seem a bit complex, but keeping pipe_surfaces as context objects actually helps pipe drivers that put derived data in pipe_surfaces to be much simpler, as they no longer need complicated locking to be thread safe -- the state tracker guarantees that pipe_surfaces belong to that context, and that context alone. That is, moving stuff all into the screen sounds simple at first, but then it becomes a serious nightmare to make it thread safe. Note that if st_renderbuffer can't be shared between GL c
Re: [Mesa-dev] State tracker pipe_surface woes...
2011/9/2 Jose Fonseca : > - Original Message - >> Hi, >> >> While debugging some code I ran across the following situation: >> >> - pipe_context c1 is created >> - pipe_surface s1 is created >> - strb-> surface is set to s1 (s1's refcount goes up) >> - pipe_context c1 is destroyed >> - strb is destroyed >> - strb->surface is destroyed (so s1's refcount is now 0 and we want >> to >> destroy it) >> >> At that point s1 references c1 which is not available any more, so >> when we try to call ctx->surface_destroy to destroy s1 we crash. >> >> We discussed this a bit on IRC, and we agreed that the proper >> solution, since surfaces outlive their context, is to make surfaces >> screen-bound instead. I'm going to implement that unless someone >> objects. >> >> As a side note, the same issue will happen with sampler_views, so >> it'll get a similar fix. > > Sampler views and surfaces were previously objects bound to screen, and we > changed that because of poor multithreading semantics. Per-context sampler > views / render targets actually matches the 3D APIs semantics better, so I > don't think that reverting is the solution. > > It looks to me that the issue here is that pipe_context should not be > destroyed before the surfaces. strb->surface should only be used by one > context, and should be destroyed before that context is destroyed. > > IIUC, strb matches GL renderbuffer semantics and can be shared by multiple > context. If so, strb is treating pipe_surfaces as a entity shareable by > contexts when really shouldn't. > > The solution is: > - strb can only have pipe_resources, plus the key for the surface (face, > level, etc) Another bit I don't get: since pipe_resources don't have a pointer to the context, how do you go back to the corresponding context/surface when all you have is the pipe_resource? Stéphane ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
2011/9/6 Roland Scheidegger : > Am 07.09.2011 00:01, schrieb Stéphane Marchesin: >> 2011/9/3 Jose Fonseca : >>> >>> >>> - Original Message - 2011/9/2 Stéphane Marchesin : > 2011/9/2 Jose Fonseca : >> - Original Message - >>> Hi, >>> >>> While debugging some code I ran across the following situation: >>> >>> - pipe_context c1 is created >>> - pipe_surface s1 is created >>> - strb-> surface is set to s1 (s1's refcount goes up) >>> - pipe_context c1 is destroyed >>> - strb is destroyed >>> - strb->surface is destroyed (so s1's refcount is now 0 and we >>> want >>> to >>> destroy it) >>> >>> At that point s1 references c1 which is not available any more, >>> so >>> when we try to call ctx->surface_destroy to destroy s1 we crash. >>> >>> We discussed this a bit on IRC, and we agreed that the proper >>> solution, since surfaces outlive their context, is to make >>> surfaces >>> screen-bound instead. I'm going to implement that unless someone >>> objects. >>> >>> As a side note, the same issue will happen with sampler_views, so >>> it'll get a similar fix. >> >> Sampler views and surfaces were previously objects bound to >> screen, and we changed that because of poor multithreading >> semantics. Per-context sampler views / render targets actually >> matches the 3D APIs semantics better, so I don't think that >> reverting is the solution. >> >> It looks to me that the issue here is that pipe_context should not >> be destroyed before the surfaces. strb->surface should only be >> used by one context, and should be destroyed before that context >> is destroyed. >> >> IIUC, strb matches GL renderbuffer semantics and can be shared by >> multiple context. If so, strb is treating pipe_surfaces as a >> entity shareable by contexts when really shouldn't. >> >> The solution is: >> - strb can only have pipe_resources, plus the key for the surface >> (face, level, etc) >> - the pipe_surfaces that are derived should be stored/cached in >> the GLcontext. >> - when the GLcontext / pipe_context is being destroy, the pipe >> surfaces can be destroyed before >> > > I don't understand some of it. From what I see, it should be > enough, > whenever strb binds a surface, to add a pointer to this strb to a > list of strb's to the pipe_context. By doing that, we would be able > to > unbind the surfaces from the strb before we destroy the context. > However, pipe_context structures can't reference gl structures, so > how > would you solve that? > Alright, maybe I'm too tired, I just have to put it in strb... My other questions still stand though :) Stéphane > Also, what difference does it make if strb's only have > pipe_resources? >>> >>> Pipe resources can be shared between contexts, therefore they should not >>> refer context or context data. >>> So it is always safe to destroy pipe_resources pipe_contexts on any order. >>> >>> Using your example above, if you replace surface "s1" with resource "r1", a >>> reference from r1 to c1 would be violating the semantics. >>> > And why do I need a key? >>> >>> "key" is a bad name perhaps. Unless the resource is always a single level >>> 2d texture, if we replace the strb->surface with a strb->resource, we will >>> need to specify separately which face/level is sought. >>> > This all is definitely more complex than it should be. >>> >>> May be I'm not understanding what you were proposing. When you said that >>> >>> "the proper solution, since surfaces outlive their context, is to make >>> surfaces screen-bound instead" >>> >>> I interpreted this statement as moving the pipe_context::create_surface >>> method to pipe_screen::create_surface. Was this really your plan or did you >>> meant something else? >>> >>> Because, my understanding is that it should be the other way around: when >>> we moved the create_surface method from pipe_screen to pipe_context, we >>> forgot to fix the st_renderbuffer code to do this properly. >>> >>> >>> >>> The fix I proposed may seem a bit complex, but keeping pipe_surfaces as >>> context objects actually helps pipe drivers that put derived data in >>> pipe_surfaces to be much simpler, as they no longer need complicated >>> locking to be thread safe -- the state tracker guarantees that >>> pipe_surfaces belong to that context, and that context alone. >>> >>> That is, moving stuff all into the screen sounds simple at first, but then >>> it becomes a serious nightmare to make it thread safe. >>> >>> >>> >>> Note that if st_renderbuffer can't be shared between GL contexts, then the >>> solution can be much simpler: all we need to do is ensure that the surfaces >>> are destroyed before the context. I haven't looked at the Mesa state
Re: [Mesa-dev] State tracker pipe_surface woes...
Am 07.09.2011 00:01, schrieb Stéphane Marchesin: > 2011/9/3 Jose Fonseca : >> >> >> - Original Message - >>> 2011/9/2 Stéphane Marchesin : 2011/9/2 Jose Fonseca : > - Original Message - >> Hi, >> >> While debugging some code I ran across the following situation: >> >> - pipe_context c1 is created >> - pipe_surface s1 is created >> - strb-> surface is set to s1 (s1's refcount goes up) >> - pipe_context c1 is destroyed >> - strb is destroyed >> - strb->surface is destroyed (so s1's refcount is now 0 and we >> want >> to >> destroy it) >> >> At that point s1 references c1 which is not available any more, >> so >> when we try to call ctx->surface_destroy to destroy s1 we crash. >> >> We discussed this a bit on IRC, and we agreed that the proper >> solution, since surfaces outlive their context, is to make >> surfaces >> screen-bound instead. I'm going to implement that unless someone >> objects. >> >> As a side note, the same issue will happen with sampler_views, so >> it'll get a similar fix. > > Sampler views and surfaces were previously objects bound to > screen, and we changed that because of poor multithreading > semantics. Per-context sampler views / render targets actually > matches the 3D APIs semantics better, so I don't think that > reverting is the solution. > > It looks to me that the issue here is that pipe_context should not > be destroyed before the surfaces. strb->surface should only be > used by one context, and should be destroyed before that context > is destroyed. > > IIUC, strb matches GL renderbuffer semantics and can be shared by > multiple context. If so, strb is treating pipe_surfaces as a > entity shareable by contexts when really shouldn't. > > The solution is: > - strb can only have pipe_resources, plus the key for the surface > (face, level, etc) > - the pipe_surfaces that are derived should be stored/cached in > the GLcontext. > - when the GLcontext / pipe_context is being destroy, the pipe > surfaces can be destroyed before > I don't understand some of it. From what I see, it should be enough, whenever strb binds a surface, to add a pointer to this strb to a list of strb's to the pipe_context. By doing that, we would be able to unbind the surfaces from the strb before we destroy the context. However, pipe_context structures can't reference gl structures, so how would you solve that? >>> >>> Alright, maybe I'm too tired, I just have to put it in strb... My >>> other questions still stand though :) >>> >>> Stéphane >>> >>> Also, what difference does it make if strb's only have pipe_resources? >> >> Pipe resources can be shared between contexts, therefore they should not >> refer context or context data. >> So it is always safe to destroy pipe_resources pipe_contexts on any order. >> >> Using your example above, if you replace surface "s1" with resource "r1", a >> reference from r1 to c1 would be violating the semantics. >> And why do I need a key? >> >> "key" is a bad name perhaps. Unless the resource is always a single level >> 2d texture, if we replace the strb->surface with a strb->resource, we will >> need to specify separately which face/level is sought. >> This all is definitely more complex than it should be. >> >> May be I'm not understanding what you were proposing. When you said that >> >> "the proper solution, since surfaces outlive their context, is to make >> surfaces screen-bound instead" >> >> I interpreted this statement as moving the pipe_context::create_surface >> method to pipe_screen::create_surface. Was this really your plan or did you >> meant something else? >> >> Because, my understanding is that it should be the other way around: when we >> moved the create_surface method from pipe_screen to pipe_context, we forgot >> to fix the st_renderbuffer code to do this properly. >> >> >> >> The fix I proposed may seem a bit complex, but keeping pipe_surfaces as >> context objects actually helps pipe drivers that put derived data in >> pipe_surfaces to be much simpler, as they no longer need complicated locking >> to be thread safe -- the state tracker guarantees that pipe_surfaces belong >> to that context, and that context alone. >> >> That is, moving stuff all into the screen sounds simple at first, but then >> it becomes a serious nightmare to make it thread safe. >> >> >> >> Note that if st_renderbuffer can't be shared between GL contexts, then the >> solution can be much simpler: all we need to do is ensure that the surfaces >> are destroyed before the context. I haven't looked at the Mesa state tracker >> code in a while, so I'm a bit rusty in this regard. >> >> ARB_framebuffer_object says that framebuffer objects are per-context, but >> rende
Re: [Mesa-dev] State tracker pipe_surface woes...
2011/9/3 Jose Fonseca : > > > - Original Message - >> 2011/9/2 Stéphane Marchesin : >> > 2011/9/2 Jose Fonseca : >> >> - Original Message - >> >>> Hi, >> >>> >> >>> While debugging some code I ran across the following situation: >> >>> >> >>> - pipe_context c1 is created >> >>> - pipe_surface s1 is created >> >>> - strb-> surface is set to s1 (s1's refcount goes up) >> >>> - pipe_context c1 is destroyed >> >>> - strb is destroyed >> >>> - strb->surface is destroyed (so s1's refcount is now 0 and we >> >>> want >> >>> to >> >>> destroy it) >> >>> >> >>> At that point s1 references c1 which is not available any more, >> >>> so >> >>> when we try to call ctx->surface_destroy to destroy s1 we crash. >> >>> >> >>> We discussed this a bit on IRC, and we agreed that the proper >> >>> solution, since surfaces outlive their context, is to make >> >>> surfaces >> >>> screen-bound instead. I'm going to implement that unless someone >> >>> objects. >> >>> >> >>> As a side note, the same issue will happen with sampler_views, so >> >>> it'll get a similar fix. >> >> >> >> Sampler views and surfaces were previously objects bound to >> >> screen, and we changed that because of poor multithreading >> >> semantics. Per-context sampler views / render targets actually >> >> matches the 3D APIs semantics better, so I don't think that >> >> reverting is the solution. >> >> >> >> It looks to me that the issue here is that pipe_context should not >> >> be destroyed before the surfaces. strb->surface should only be >> >> used by one context, and should be destroyed before that context >> >> is destroyed. >> >> >> >> IIUC, strb matches GL renderbuffer semantics and can be shared by >> >> multiple context. If so, strb is treating pipe_surfaces as a >> >> entity shareable by contexts when really shouldn't. >> >> >> >> The solution is: >> >> - strb can only have pipe_resources, plus the key for the surface >> >> (face, level, etc) >> >> - the pipe_surfaces that are derived should be stored/cached in >> >> the GLcontext. >> >> - when the GLcontext / pipe_context is being destroy, the pipe >> >> surfaces can be destroyed before >> >> >> > >> > I don't understand some of it. From what I see, it should be >> > enough, >> > whenever strb binds a surface, to add a pointer to this strb to a >> > list of strb's to the pipe_context. By doing that, we would be able >> > to >> > unbind the surfaces from the strb before we destroy the context. >> > However, pipe_context structures can't reference gl structures, so >> > how >> > would you solve that? >> > >> >> Alright, maybe I'm too tired, I just have to put it in strb... My >> other questions still stand though :) >> >> Stéphane >> >> >> > Also, what difference does it make if strb's only have >> > pipe_resources? > > Pipe resources can be shared between contexts, therefore they should not > refer context or context data. > So it is always safe to destroy pipe_resources pipe_contexts on any order. > > Using your example above, if you replace surface "s1" with resource "r1", a > reference from r1 to c1 would be violating the semantics. > >> > And why do I need a key? > > "key" is a bad name perhaps. Unless the resource is always a single level 2d > texture, if we replace the strb->surface with a strb->resource, we will need > to specify separately which face/level is sought. > >> > This all is definitely more complex than it should be. > > May be I'm not understanding what you were proposing. When you said that > > "the proper solution, since surfaces outlive their context, is to make > surfaces screen-bound instead" > > I interpreted this statement as moving the pipe_context::create_surface > method to pipe_screen::create_surface. Was this really your plan or did you > meant something else? > > Because, my understanding is that it should be the other way around: when we > moved the create_surface method from pipe_screen to pipe_context, we forgot > to fix the st_renderbuffer code to do this properly. > > > > The fix I proposed may seem a bit complex, but keeping pipe_surfaces as > context objects actually helps pipe drivers that put derived data in > pipe_surfaces to be much simpler, as they no longer need complicated locking > to be thread safe -- the state tracker guarantees that pipe_surfaces belong > to that context, and that context alone. > > That is, moving stuff all into the screen sounds simple at first, but then it > becomes a serious nightmare to make it thread safe. > > > > Note that if st_renderbuffer can't be shared between GL contexts, then the > solution can be much simpler: all we need to do is ensure that the surfaces > are destroyed before the context. I haven't looked at the Mesa state tracker > code in a while, so I'm a bit rusty in this regard. > > ARB_framebuffer_object says that framebuffer objects are per-context, but > renderbuffer objects like textures can be shared between contexts. So when > one looks at st_renderbuffer defini
Re: [Mesa-dev] State tracker pipe_surface woes...
- Original Message -
> 2011/9/2 Stéphane Marchesin :
> > 2011/9/2 Jose Fonseca :
> >> - Original Message -
> >>> Hi,
> >>>
> >>> While debugging some code I ran across the following situation:
> >>>
> >>> - pipe_context c1 is created
> >>> - pipe_surface s1 is created
> >>> - strb-> surface is set to s1 (s1's refcount goes up)
> >>> - pipe_context c1 is destroyed
> >>> - strb is destroyed
> >>> - strb->surface is destroyed (so s1's refcount is now 0 and we
> >>> want
> >>> to
> >>> destroy it)
> >>>
> >>> At that point s1 references c1 which is not available any more,
> >>> so
> >>> when we try to call ctx->surface_destroy to destroy s1 we crash.
> >>>
> >>> We discussed this a bit on IRC, and we agreed that the proper
> >>> solution, since surfaces outlive their context, is to make
> >>> surfaces
> >>> screen-bound instead. I'm going to implement that unless someone
> >>> objects.
> >>>
> >>> As a side note, the same issue will happen with sampler_views, so
> >>> it'll get a similar fix.
> >>
> >> Sampler views and surfaces were previously objects bound to
> >> screen, and we changed that because of poor multithreading
> >> semantics. Per-context sampler views / render targets actually
> >> matches the 3D APIs semantics better, so I don't think that
> >> reverting is the solution.
> >>
> >> It looks to me that the issue here is that pipe_context should not
> >> be destroyed before the surfaces. strb->surface should only be
> >> used by one context, and should be destroyed before that context
> >> is destroyed.
> >>
> >> IIUC, strb matches GL renderbuffer semantics and can be shared by
> >> multiple context. If so, strb is treating pipe_surfaces as a
> >> entity shareable by contexts when really shouldn't.
> >>
> >> The solution is:
> >> - strb can only have pipe_resources, plus the key for the surface
> >> (face, level, etc)
> >> - the pipe_surfaces that are derived should be stored/cached in
> >> the GLcontext.
> >> - when the GLcontext / pipe_context is being destroy, the pipe
> >> surfaces can be destroyed before
> >>
> >
> > I don't understand some of it. From what I see, it should be
> > enough,
> > whenever strb binds a surface, to add a pointer to this strb to a
> > list of strb's to the pipe_context. By doing that, we would be able
> > to
> > unbind the surfaces from the strb before we destroy the context.
> > However, pipe_context structures can't reference gl structures, so
> > how
> > would you solve that?
> >
>
> Alright, maybe I'm too tired, I just have to put it in strb... My
> other questions still stand though :)
>
> Stéphane
>
>
> > Also, what difference does it make if strb's only have
> > pipe_resources?
Pipe resources can be shared between contexts, therefore they should not refer
context or context data.
So it is always safe to destroy pipe_resources pipe_contexts on any order.
Using your example above, if you replace surface "s1" with resource "r1", a
reference from r1 to c1 would be violating the semantics.
> > And why do I need a key?
"key" is a bad name perhaps. Unless the resource is always a single level 2d
texture, if we replace the strb->surface with a strb->resource, we will need to
specify separately which face/level is sought.
> > This all is definitely more complex than it should be.
May be I'm not understanding what you were proposing. When you said that
"the proper solution, since surfaces outlive their context, is to make
surfaces screen-bound instead"
I interpreted this statement as moving the pipe_context::create_surface method
to pipe_screen::create_surface. Was this really your plan or did you meant
something else?
Because, my understanding is that it should be the other way around: when we
moved the create_surface method from pipe_screen to pipe_context, we forgot to
fix the st_renderbuffer code to do this properly.
The fix I proposed may seem a bit complex, but keeping pipe_surfaces as context
objects actually helps pipe drivers that put derived data in pipe_surfaces to
be much simpler, as they no longer need complicated locking to be thread safe
-- the state tracker guarantees that pipe_surfaces belong to that context, and
that context alone.
That is, moving stuff all into the screen sounds simple at first, but then it
becomes a serious nightmare to make it thread safe.
Note that if st_renderbuffer can't be shared between GL contexts, then the
solution can be much simpler: all we need to do is ensure that the surfaces are
destroyed before the context. I haven't looked at the Mesa state tracker code
in a while, so I'm a bit rusty in this regard.
ARB_framebuffer_object says that framebuffer objects are per-context, but
renderbuffer objects like textures can be shared between contexts. So when one
looks at st_renderbuffer definition:
/**
* Derived renderbuffer class. Just need to add a pointer to the
* pipe surface.
*/
struct st_renderbuffer
{
Re: [Mesa-dev] State tracker pipe_surface woes...
2011/9/2 Stéphane Marchesin : > 2011/9/2 Jose Fonseca : >> - Original Message - >>> Hi, >>> >>> While debugging some code I ran across the following situation: >>> >>> - pipe_context c1 is created >>> - pipe_surface s1 is created >>> - strb-> surface is set to s1 (s1's refcount goes up) >>> - pipe_context c1 is destroyed >>> - strb is destroyed >>> - strb->surface is destroyed (so s1's refcount is now 0 and we want >>> to >>> destroy it) >>> >>> At that point s1 references c1 which is not available any more, so >>> when we try to call ctx->surface_destroy to destroy s1 we crash. >>> >>> We discussed this a bit on IRC, and we agreed that the proper >>> solution, since surfaces outlive their context, is to make surfaces >>> screen-bound instead. I'm going to implement that unless someone >>> objects. >>> >>> As a side note, the same issue will happen with sampler_views, so >>> it'll get a similar fix. >> >> Sampler views and surfaces were previously objects bound to screen, and we >> changed that because of poor multithreading semantics. Per-context sampler >> views / render targets actually matches the 3D APIs semantics better, so I >> don't think that reverting is the solution. >> >> It looks to me that the issue here is that pipe_context should not be >> destroyed before the surfaces. strb->surface should only be used by one >> context, and should be destroyed before that context is destroyed. >> >> IIUC, strb matches GL renderbuffer semantics and can be shared by multiple >> context. If so, strb is treating pipe_surfaces as a entity shareable by >> contexts when really shouldn't. >> >> The solution is: >> - strb can only have pipe_resources, plus the key for the surface (face, >> level, etc) >> - the pipe_surfaces that are derived should be stored/cached in the >> GLcontext. >> - when the GLcontext / pipe_context is being destroy, the pipe surfaces can >> be destroyed before >> > > I don't understand some of it. From what I see, it should be enough, > whenever strb binds a surface, to add a pointer to this strb to a > list of strb's to the pipe_context. By doing that, we would be able to > unbind the surfaces from the strb before we destroy the context. > However, pipe_context structures can't reference gl structures, so how > would you solve that? > Alright, maybe I'm too tired, I just have to put it in strb... My other questions still stand though :) Stéphane > Also, what difference does it make if strb's only have pipe_resources? > And why do I need a key? > > This all is definitely more complex than it should be. > > Stéphane > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
2011/9/2 Stéphane Marchesin : > 2011/9/2 Jose Fonseca : >> - Original Message - >>> Hi, >>> >>> While debugging some code I ran across the following situation: >>> >>> - pipe_context c1 is created >>> - pipe_surface s1 is created >>> - strb-> surface is set to s1 (s1's refcount goes up) >>> - pipe_context c1 is destroyed >>> - strb is destroyed >>> - strb->surface is destroyed (so s1's refcount is now 0 and we want >>> to >>> destroy it) >>> >>> At that point s1 references c1 which is not available any more, so >>> when we try to call ctx->surface_destroy to destroy s1 we crash. >>> >>> We discussed this a bit on IRC, and we agreed that the proper >>> solution, since surfaces outlive their context, is to make surfaces >>> screen-bound instead. I'm going to implement that unless someone >>> objects. >>> >>> As a side note, the same issue will happen with sampler_views, so >>> it'll get a similar fix. >> >> Sampler views and surfaces were previously objects bound to screen, and we >> changed that because of poor multithreading semantics. Per-context sampler >> views / render targets actually matches the 3D APIs semantics better, so I >> don't think that reverting is the solution. >> >> It looks to me that the issue here is that pipe_context should not be >> destroyed before the surfaces. strb->surface should only be used by one >> context, and should be destroyed before that context is destroyed. >> >> IIUC, strb matches GL renderbuffer semantics and can be shared by multiple >> context. If so, strb is treating pipe_surfaces as a entity shareable by >> contexts when really shouldn't. >> >> The solution is: >> - strb can only have pipe_resources, plus the key for the surface (face, >> level, etc) >> - the pipe_surfaces that are derived should be stored/cached in the >> GLcontext. >> - when the GLcontext / pipe_context is being destroy, the pipe surfaces can >> be destroyed before >> > > I don't understand some of it. From what I see, it should be enough, > whenever strb binds a surface, to add a pointer to this strb to a > list of strb's to the pipe_context. By doing that, we would be able to > unbind the surfaces from the strb before we destroy the context. > However, pipe_context structures can't reference gl structures, so how > would you solve that? > > Also, what difference does it make if strb's only have pipe_resources? > And why do I need a key? > > This all is definitely more complex than it should be. > And, yeah, I have a much simpler solution in mind, which I might just end up doing for my own use: - remove the ctx arg to surface_destroy() - add a destroy function pointer to pipe_surface - call that instead of going through the ctx when we destroy the surface Stéphane ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
2011/9/2 Jose Fonseca : > - Original Message - >> Hi, >> >> While debugging some code I ran across the following situation: >> >> - pipe_context c1 is created >> - pipe_surface s1 is created >> - strb-> surface is set to s1 (s1's refcount goes up) >> - pipe_context c1 is destroyed >> - strb is destroyed >> - strb->surface is destroyed (so s1's refcount is now 0 and we want >> to >> destroy it) >> >> At that point s1 references c1 which is not available any more, so >> when we try to call ctx->surface_destroy to destroy s1 we crash. >> >> We discussed this a bit on IRC, and we agreed that the proper >> solution, since surfaces outlive their context, is to make surfaces >> screen-bound instead. I'm going to implement that unless someone >> objects. >> >> As a side note, the same issue will happen with sampler_views, so >> it'll get a similar fix. > > Sampler views and surfaces were previously objects bound to screen, and we > changed that because of poor multithreading semantics. Per-context sampler > views / render targets actually matches the 3D APIs semantics better, so I > don't think that reverting is the solution. > > It looks to me that the issue here is that pipe_context should not be > destroyed before the surfaces. strb->surface should only be used by one > context, and should be destroyed before that context is destroyed. > > IIUC, strb matches GL renderbuffer semantics and can be shared by multiple > context. If so, strb is treating pipe_surfaces as a entity shareable by > contexts when really shouldn't. > > The solution is: > - strb can only have pipe_resources, plus the key for the surface (face, > level, etc) > - the pipe_surfaces that are derived should be stored/cached in the GLcontext. > - when the GLcontext / pipe_context is being destroy, the pipe surfaces can > be destroyed before > I don't understand some of it. From what I see, it should be enough, whenever strb binds a surface, to add a pointer to this strb to a list of strb's to the pipe_context. By doing that, we would be able to unbind the surfaces from the strb before we destroy the context. However, pipe_context structures can't reference gl structures, so how would you solve that? Also, what difference does it make if strb's only have pipe_resources? And why do I need a key? This all is definitely more complex than it should be. Stéphane ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] State tracker pipe_surface woes...
- Original Message - > Hi, > > While debugging some code I ran across the following situation: > > - pipe_context c1 is created > - pipe_surface s1 is created > - strb-> surface is set to s1 (s1's refcount goes up) > - pipe_context c1 is destroyed > - strb is destroyed > - strb->surface is destroyed (so s1's refcount is now 0 and we want > to > destroy it) > > At that point s1 references c1 which is not available any more, so > when we try to call ctx->surface_destroy to destroy s1 we crash. > > We discussed this a bit on IRC, and we agreed that the proper > solution, since surfaces outlive their context, is to make surfaces > screen-bound instead. I'm going to implement that unless someone > objects. > > As a side note, the same issue will happen with sampler_views, so > it'll get a similar fix. Sampler views and surfaces were previously objects bound to screen, and we changed that because of poor multithreading semantics. Per-context sampler views / render targets actually matches the 3D APIs semantics better, so I don't think that reverting is the solution. It looks to me that the issue here is that pipe_context should not be destroyed before the surfaces. strb->surface should only be used by one context, and should be destroyed before that context is destroyed. IIUC, strb matches GL renderbuffer semantics and can be shared by multiple context. If so, strb is treating pipe_surfaces as a entity shareable by contexts when really shouldn't. The solution is: - strb can only have pipe_resources, plus the key for the surface (face, level, etc) - the pipe_surfaces that are derived should be stored/cached in the GLcontext. - when the GLcontext / pipe_context is being destroy, the pipe surfaces can be destroyed before This may seem a bit complex, but the benefit is that by having surfaces unshareable between contents, they need no mutex protection, and therefore are much more efficient. Jose ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] State tracker pipe_surface woes...
Hi, While debugging some code I ran across the following situation: - pipe_context c1 is created - pipe_surface s1 is created - strb-> surface is set to s1 (s1's refcount goes up) - pipe_context c1 is destroyed - strb is destroyed - strb->surface is destroyed (so s1's refcount is now 0 and we want to destroy it) At that point s1 references c1 which is not available any more, so when we try to call ctx->surface_destroy to destroy s1 we crash. We discussed this a bit on IRC, and we agreed that the proper solution, since surfaces outlive their context, is to make surfaces screen-bound instead. I'm going to implement that unless someone objects. As a side note, the same issue will happen with sampler_views, so it'll get a similar fix. Stéphane ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
