On Mon, 2011-02-14 at 10:18 -0800, Marek Olšák wrote: > On Mon, Feb 14, 2011 at 6:47 PM, José Fonseca > <jfons...@vmware.com<mailto:jfons...@vmware.com>> wrote: > On Sun, 2011-02-13 at 23:58 -0800, Dave Airlie wrote: > > >> if(buf->base.base.size < size) > > >> return 0; > > >> > > >> @@ -242,13 +240,10 @@ pb_cache_is_buffer_compat(struct pb_cache_buffer > > >> *buf, > > >> if(!pb_check_usage(desc->usage, buf->base.base.usage)) > > >> return 0; > > >> > > >> - map = pb_map(buf->buffer, PB_USAGE_DONTBLOCK, NULL); > > >> - if (!map) { > > >> - return -1; > > >> - } > > >> + if (buf->mgr->base.is_buffer_busy) > > >> + if (buf->mgr->base.is_buffer_busy(&buf->mgr->base, buf->buffer)) > > >> + return -1; > > > > > > Oops, this is wrong. I will locally replace any occurences of > > > "buf->mgr->base(.)" with "buf->mgr->provider(->)", which is how it was > > > meant > > > to be, but the idea remains the same. Please review. > > Marek, I don't understand what you want to do here: you removed the > pb_map, but you left the pb_unmap, and what will happen if > is_buffer_busy is not defined? > > I didn't leave the pb_unmap call, it was removed too, I just cut it off in my > second email, since it wasn't relevant to the typo. Sorry about that. So > there's only one way: is_buffer_busy. > > > > > > I actually suggested this originally, but Jose I think preferred using > > the dontblock to the buffer mapping. > > I'd prefer that there is one way of doing this, but I didn't/don't feel > strong about this. IMO, having two ways, PB_USAGE_DONTBLOCK and > is_buffer_busy, is not cleaner that just PB_USAGE_DONTBLOCK, even if > is_buffer_busy is conceptually cleaner. > > The thing is mapping a buffer just to know whether it's being used is > unnecessary, and the mapping itself may be slower than a simple is_busy query. > > > Marek, Would adding an inline function, pb_is_buffer_busy, that calls > pb_map(PB_USAGE_DONTBLOCK)+pb_unmap inside work for you? > > Another way, would be to add is_buffer_busy and have the default > implementation to do pb_map/pb_unmap. > > I can add a piece of code that uses pb_map/pb_unmap if the is_buffer_busy > hook is not set, so that the original behavior is preserved. Would that be ok > with you? Here's a new patch: > > > pb_bufmgr_cache: add is_buffer_busy hook and use it instead of > non-blocking map > > This is cleaner and implementing the hook is optional. > > diff --git a/src/gallium/auxiliary/pipebuffer/pb_bufmgr.h > b/src/gallium/auxiliary/pipebuffer/pb_bufm > index 2ef0216..960068c 100644 > --- a/src/gallium/auxiliary/pipebuffer/pb_bufmgr.h > +++ b/src/gallium/auxiliary/pipebuffer/pb_bufmgr.h > @@ -82,6 +82,10 @@ struct pb_manager > */ > void > (*flush)( struct pb_manager *mgr ); > + > + boolean > + (*is_buffer_busy)( struct pb_manager *mgr, > + struct pb_buffer *buf ); > }; > > > diff --git a/src/gallium/auxiliary/pipebuffer/pb_bufmgr_cache.c > b/src/gallium/auxiliary/pipebuffer/p > index a6eb403..25accef 100644 > --- a/src/gallium/auxiliary/pipebuffer/pb_bufmgr_cache.c > +++ b/src/gallium/auxiliary/pipebuffer/pb_bufmgr_cache.c > @@ -227,8 +227,6 @@ pb_cache_is_buffer_compat(struct pb_cache_buffer *buf, > pb_size size, > const struct pb_desc *desc) > { > - void *map; > - > if(buf->base.base.size < size) > return 0; > > @@ -242,13 +240,18 @@ pb_cache_is_buffer_compat(struct pb_cache_buffer *buf, > if(!pb_check_usage(desc->usage, buf->base.base.usage)) > return 0; > > - map = pb_map(buf->buffer, PB_USAGE_DONTBLOCK, NULL); > - if (!map) { > - return -1; > + if (buf->mgr->provider->is_buffer_busy) { > + if (buf->mgr->provider->is_buffer_busy(buf->mgr->provider, > buf->buffer)) > + return -1; > + } else { > + void *ptr = pb_map(buf->buffer, PB_USAGE_DONTBLOCK, NULL); > + > + if (!ptr) > + return -1; > + > + pb_unmap(buf->buffer); > } > > - pb_unmap(buf->buffer); > - > return 1; > }
This looks a better solution in the interim. We can ensure implement is_buffer_busy everywhere, and replace this fallback with an assert(buf->mgr->provider->is_buffer_busy) at a later stage. Thanks. Jose _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev