[2/2] panfrost: Add madvise support to BO cache

Submitted by Rob Herring on Aug. 9, 2019, 7:53 p.m.

Details

Message ID 20190809195313.27778-2-robh@kernel.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rob Herring Aug. 9, 2019, 7:53 p.m.
The kernel now supports madvise ioctl to indicate which BOs can be freed
when there is memory pressure. Mark BOs purgeable when they are in the
BO cache. The BOs must also be munmapped when they are in the cache or
they cannot be purged.

We could optimize avoiding the madvise ioctl on older kernels once the
driver version bump lands, but probably not worth it given the other
driver features also being added.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 src/gallium/drivers/panfrost/pan_bo_cache.c | 21 +++++++++++++++++++++
 src/gallium/drivers/panfrost/pan_drm.c      |  4 ++--
 2 files changed, 23 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c b/src/gallium/drivers/panfrost/pan_bo_cache.c
index 7378d0a8abea..239ea0b46cb2 100644
--- a/src/gallium/drivers/panfrost/pan_bo_cache.c
+++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
@@ -23,6 +23,8 @@ 
  * Authors (Collabora):
  *   Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
  */
+#include <xf86drm.h>
+#include "drm-uapi/panfrost_drm.h"
 
 #include "pan_screen.h"
 #include "util/u_math.h"
@@ -88,9 +90,21 @@  panfrost_bo_cache_fetch(
         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;
+
                         /* This one works, splice it out of the cache */
                         list_del(&entry->link);
 
+                        madv.handle = entry->gem_handle;
+                        madv.madv = PANFROST_MADV_WILLNEED;
+			madv.retained = 0;
+
+                        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MADVISE, &madv);
+			if (!ret && !madv.retained) {
+				panfrost_drm_release_bo(screen, entry, false);
+				continue;
+			}
                         /* Let's go! */
                         return entry;
                 }
@@ -109,6 +123,13 @@  panfrost_bo_cache_put(
                 struct panfrost_bo *bo)
 {
         struct list_head *bucket = pan_bucket(screen, bo->size);
+        struct drm_panfrost_madvise madv;
+
+        madv.handle = bo->gem_handle;
+        madv.madv = PANFROST_MADV_DONTNEED;
+	madv.retained = 0;
+
+        drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MADVISE, &madv);
 
         /* Add us to the bucket */
         list_addtail(&bo->link, bucket);
diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
index 36a6b975680a..28a4287202bd 100644
--- a/src/gallium/drivers/panfrost/pan_drm.c
+++ b/src/gallium/drivers/panfrost/pan_drm.c
@@ -163,6 +163,8 @@  panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo,
         /* Rather than freeing the BO now, we'll cache the BO for later
          * allocations if we're allowed to */
 
+        panfrost_drm_munmap_bo(screen, bo);
+
         if (cacheable) {
                 bool cached = panfrost_bo_cache_put(screen, bo);
 
@@ -172,8 +174,6 @@  panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo,
 
         /* Otherwise, if the BO wasn't cached, we'll legitimately free the BO */
 
-        panfrost_drm_munmap_bo(screen, bo);
-
         ret = drmIoctl(screen->fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
         if (ret) {
                 fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed: %m\n");

Comments


On Fri, Aug 9, 2019 at 2:49 PM Alyssa Rosenzweig
<alyssa.rosenzweig@collabora.com> wrote:
>
> I'm not one to care, but fwiw, spacing is inconsistent..?

Context? I guess you mean the 'madv.retained = 0;' line.

>
> > +                     if (!ret && !madv.retained) {
>
> What's the logic here? (What does a 0/!0 return code mean here?) I'm
> wondering if this meant to be ||?
>
> Or is the idea that an older kernel will have ret!=0 (since it doesn't
> recognize the ioctl) and therefore BOs won't be released, whereas new
> kernels will have ret==0 and then retained=0 if it needs to be released?

Yes. Any error whether madvise is supported or not is just treated as
never having made the ioctl call. The only error we can get is handle
lookup failing.

I could perhaps simplify this by initializing madv.retained to 1 and
never checking the return.

Rob

Both patches look good to me.

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Thanks!

Tomeu

On Fri, 9 Aug 2019 at 21:53, Rob Herring <robh@kernel.org> wrote:
>
> The kernel now supports madvise ioctl to indicate which BOs can be freed
> when there is memory pressure. Mark BOs purgeable when they are in the
> BO cache. The BOs must also be munmapped when they are in the cache or
> they cannot be purged.
>
> We could optimize avoiding the madvise ioctl on older kernels once the
> driver version bump lands, but probably not worth it given the other
> driver features also being added.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  src/gallium/drivers/panfrost/pan_bo_cache.c | 21 +++++++++++++++++++++
>  src/gallium/drivers/panfrost/pan_drm.c      |  4 ++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_bo_cache.c b/src/gallium/drivers/panfrost/pan_bo_cache.c
> index 7378d0a8abea..239ea0b46cb2 100644
> --- a/src/gallium/drivers/panfrost/pan_bo_cache.c
> +++ b/src/gallium/drivers/panfrost/pan_bo_cache.c
> @@ -23,6 +23,8 @@
>   * Authors (Collabora):
>   *   Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
>   */
> +#include <xf86drm.h>
> +#include "drm-uapi/panfrost_drm.h"
>
>  #include "pan_screen.h"
>  #include "util/u_math.h"
> @@ -88,9 +90,21 @@ panfrost_bo_cache_fetch(
>          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;
> +
>                          /* This one works, splice it out of the cache */
>                          list_del(&entry->link);
>
> +                        madv.handle = entry->gem_handle;
> +                        madv.madv = PANFROST_MADV_WILLNEED;
> +                       madv.retained = 0;
> +
> +                        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MADVISE, &madv);
> +                       if (!ret && !madv.retained) {
> +                               panfrost_drm_release_bo(screen, entry, false);
> +                               continue;
> +                       }
>                          /* Let's go! */
>                          return entry;
>                  }
> @@ -109,6 +123,13 @@ panfrost_bo_cache_put(
>                  struct panfrost_bo *bo)
>  {
>          struct list_head *bucket = pan_bucket(screen, bo->size);
> +        struct drm_panfrost_madvise madv;
> +
> +        madv.handle = bo->gem_handle;
> +        madv.madv = PANFROST_MADV_DONTNEED;
> +       madv.retained = 0;
> +
> +        drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MADVISE, &madv);
>
>          /* Add us to the bucket */
>          list_addtail(&bo->link, bucket);
> diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
> index 36a6b975680a..28a4287202bd 100644
> --- a/src/gallium/drivers/panfrost/pan_drm.c
> +++ b/src/gallium/drivers/panfrost/pan_drm.c
> @@ -163,6 +163,8 @@ panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo,
>          /* Rather than freeing the BO now, we'll cache the BO for later
>           * allocations if we're allowed to */
>
> +        panfrost_drm_munmap_bo(screen, bo);
> +
>          if (cacheable) {
>                  bool cached = panfrost_bo_cache_put(screen, bo);
>
> @@ -172,8 +174,6 @@ panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo,
>
>          /* Otherwise, if the BO wasn't cached, we'll legitimately free the BO */
>
> -        panfrost_drm_munmap_bo(screen, bo);
> -
>          ret = drmIoctl(screen->fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
>          if (ret) {
>                  fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed: %m\n");
> --
> 2.20.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev