[Mesa-dev] [PATCH 2/2] pb_bufmgr_cache: add is_buffer_busy hook and use it instead of non-blocking map

[José Fonseca]

On Mon, 2011-02-14 at 10:18 -0800, Marek Olšák wrote:
> On Mon, Feb 14, 2011 at 6:47 PM, José Fonseca <jfonseca at vmware.com<mailto:jfonseca at 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