[Mesa-dev] [PATCH v2 17/37] panfrost: Make sure the BO is 'ready' when picked from the cache

[Alyssa Rosenzweig]

R-b, I suppose. I wish we could do away with this kernel traffic as
discussed, though.

On Mon, Sep 16, 2019 at 11:36:55AM +0200, Boris Brezillon wrote:
> This is needed if we want to free the panfrost_batch object at submit
> time in order to not have to GC the batch on the next job submission.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> ---
>  src/gallium/drivers/panfrost/pan_bo.c | 78 +++++++++++++++++++--------
>  src/gallium/drivers/panfrost/pan_bo.h |  2 +
>  2 files changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/src/gallium/drivers/panfrost/pan_bo.c b/src/gallium/drivers/panfrost/pan_bo.c
> index 209d1e0d71e5..3f05226f96f4 100644
> --- a/src/gallium/drivers/panfrost/pan_bo.c
> +++ b/src/gallium/drivers/panfrost/pan_bo.c
> @@ -23,6 +23,7 @@
>   * Authors (Collabora):
>   *   Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com>
>   */
> +#include <errno.h>
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <xf86drm.h>
> @@ -101,6 +102,30 @@ panfrost_bo_free(struct panfrost_bo *bo)
>          ralloc_free(bo);
>  }
>  
> +/* Returns true if the BO is ready, false otherwise. */
> +bool
> +panfrost_bo_wait(struct panfrost_bo *bo, int64_t timeout_ns)
> +{
> +        struct drm_panfrost_wait_bo req = {
> +                .handle = bo->gem_handle,
> +		.timeout_ns = timeout_ns,
> +        };
> +        int ret;
> +
> +        /* The ioctl returns >= 0 value when the BO we are waiting for is ready
> +         * -1 otherwise.
> +         */
> +        ret = drmIoctl(bo->screen->fd, DRM_IOCTL_PANFROST_WAIT_BO, &req);
> +        if (ret != -1)
> +                return true;
> +
> +        /* If errno is not ETIMEDOUT or EBUSY that means the handle we passed
> +         * is invalid, which shouldn't happen here.
> +         */
> +        assert(errno == ETIMEDOUT || errno == EBUSY);
> +        return false;
> +}
> +
>  /* Helper to calculate the bucket index of a BO */
>  
>  static unsigned
> @@ -137,9 +162,8 @@ pan_bucket(struct panfrost_screen *screen, unsigned size)
>   * BO. */
>  
>  static struct panfrost_bo *
> -panfrost_bo_cache_fetch(
> -                struct panfrost_screen *screen,
> -                size_t size, uint32_t flags)
> +panfrost_bo_cache_fetch(struct panfrost_screen *screen,
> +                        size_t size, uint32_t flags, bool dontwait)
>  {
>          pthread_mutex_lock(&screen->bo_cache_lock);
>          struct list_head *bucket = pan_bucket(screen, size);
> @@ -147,27 +171,29 @@ panfrost_bo_cache_fetch(
>  
>          /* Iterate the bucket looking for something suitable */
>          list_for_each_entry_safe(struct panfrost_bo, entry, bucket, link) {
> -                if (entry->size >= size &&
> -                    entry->flags == flags) {
> -                        int ret;
> -                        struct drm_panfrost_madvise madv;
> +                if (entry->size < size || entry->flags != flags)
> +                        continue;
>  
> -                        /* This one works, splice it out of the cache */
> -                        list_del(&entry->link);
> +                if (!panfrost_bo_wait(entry, dontwait ? 0 : INT64_MAX))
> +                        continue;
>  
> -                        madv.handle = entry->gem_handle;
> -                        madv.madv = PANFROST_MADV_WILLNEED;
> -                        madv.retained = 0;
> +                struct drm_panfrost_madvise madv = {
> +                        .handle = entry->gem_handle,
> +                        .madv = PANFROST_MADV_WILLNEED,
> +                };
> +                int ret;
>  
> -                        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MADVISE, &madv);
> -                        if (!ret && !madv.retained) {
> -                                panfrost_bo_free(entry);
> -                                continue;
> -                        }
> -                        /* Let's go! */
> -                        bo = entry;
> -                        break;
> +                /* This one works, splice it out of the cache */
> +                list_del(&entry->link);
> +
> +                ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MADVISE, &madv);
> +                if (!ret && !madv.retained) {
> +                        panfrost_bo_free(entry);
> +                        continue;
>                  }
> +                /* Let's go! */
> +                bo = entry;
> +                break;
>          }
>          pthread_mutex_unlock(&screen->bo_cache_lock);
>  
> @@ -281,12 +307,18 @@ panfrost_bo_create(struct panfrost_screen *screen, size_t size,
>          if (flags & PAN_BO_GROWABLE)
>                  assert(flags & PAN_BO_INVISIBLE);
>  
> -        /* Before creating a BO, we first want to check the cache, otherwise,
> -         * the cache misses and we need to allocate a BO fresh from the kernel
> +        /* Before creating a BO, we first want to check the cache but without
> +         * waiting for BO readiness (BOs in the cache can still be referenced
> +         * by jobs that are not finished yet).
> +         * If the cached allocation fails we fall back on fresh BO allocation,
> +         * and if that fails too, we try one more time to allocate from the
> +         * cache, but this time we accept to wait.
>           */
> -        bo = panfrost_bo_cache_fetch(screen, size, flags);
> +        bo = panfrost_bo_cache_fetch(screen, size, flags, true);
>          if (!bo)
>                  bo = panfrost_bo_alloc(screen, size, flags);
> +        if (!bo)
> +                bo = panfrost_bo_cache_fetch(screen, size, flags, false);
>  
>          if (!bo)
>                  fprintf(stderr, "BO creation failed\n");
> diff --git a/src/gallium/drivers/panfrost/pan_bo.h b/src/gallium/drivers/panfrost/pan_bo.h
> index 33fbddff3369..49b392f7bd76 100644
> --- a/src/gallium/drivers/panfrost/pan_bo.h
> +++ b/src/gallium/drivers/panfrost/pan_bo.h
> @@ -78,6 +78,8 @@ struct panfrost_bo {
>          uint32_t flags;
>  };
>  
> +bool
> +panfrost_bo_wait(struct panfrost_bo *bo, int64_t timeout_ns);
>  void
>  panfrost_bo_reference(struct panfrost_bo *bo);
>  void
> -- 
> 2.21.0