[3/3] panfrost: Add support for modifiers

Submitted by Tomeu Vizoso on July 4, 2019, 8:04 a.m.

Details

Message ID 20190704080443.4153-3-tomeu.vizoso@collabora.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Tomeu Vizoso July 4, 2019, 8:04 a.m.
Implement query_dmabuf_modifiers and resource_create_with_modifiers so
Wayland clients can share AFBC buffers with the compositor.

For now this is guarded behind the PAN_MESA_DEBUG=modifiers env var
because implementing those callbacks causes Weston to try to pass
modifiers to the Rockchip KMS driver, which currently doesn't support
modifiers, thus failing the modeset.

This has been fixed in Weston 6.0, so we can enable unconditionally once
we are confident that most people testing panfrost have upgraded.

This lays the ground for scanning out AFBC buffers, if the KMS driver
supports it.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 src/gallium/drivers/panfrost/pan_drm.c      |  1 +
 src/gallium/drivers/panfrost/pan_resource.c | 74 ++++++++++++++++++---
 src/gallium/drivers/panfrost/pan_screen.c   | 37 +++++++++++
 src/gallium/drivers/panfrost/pan_util.h     |  7 ++
 4 files changed, 108 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
index 9648ac1d452d..623793a84411 100644
--- a/src/gallium/drivers/panfrost/pan_drm.c
+++ b/src/gallium/drivers/panfrost/pan_drm.c
@@ -26,6 +26,7 @@ 
 #include <fcntl.h>
 #include <xf86drm.h>
 
+#include "drm-uapi/drm_fourcc.h"
 #include "drm-uapi/panfrost_drm.h"
 
 #include "util/u_memory.h"
diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
index 54497d3de2bb..d901d43168fb 100644
--- a/src/gallium/drivers/panfrost/pan_resource.c
+++ b/src/gallium/drivers/panfrost/pan_resource.c
@@ -34,6 +34,7 @@ 
 #include "drm-uapi/drm_fourcc.h"
 
 #include "state_tracker/winsys_handle.h"
+#include "util/u_drm.h"
 #include "util/u_format.h"
 #include "util/u_memory.h"
 #include "util/u_surface.h"
@@ -91,6 +92,18 @@  panfrost_resource_from_handle(struct pipe_screen *pscreen,
 
 	rsc->bo = panfrost_drm_import_bo(screen, whandle->handle);
 
+	switch(whandle->modifier) {
+	case DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP):
+		rsc->layout = PAN_AFBC;
+		break;
+	case DRM_FORMAT_MOD_LINEAR:
+		rsc->layout = PAN_LINEAR;
+		break;
+	default:
+                fprintf(stderr, "unknown modifier: 0x%"PRIx64"\n", whandle->modifier);
+		assert(0);
+	}
+
         size_t bo_size;
         panfrost_setup_slices(rsc, &bo_size);
         assert(bo_size == rsc->bo->size);
@@ -106,6 +119,21 @@  panfrost_resource_from_handle(struct pipe_screen *pscreen,
         return prsc;
 }
 
+static uint64_t
+panfrost_resource_modifier(struct panfrost_resource *rsrc)
+{
+        switch (rsrc->layout) {
+        case PAN_AFBC:
+                return DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP);
+        case PAN_TILED:
+                return DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED;
+        case PAN_LINEAR:
+                return DRM_FORMAT_MOD_LINEAR;
+        default:
+                return DRM_FORMAT_MOD_INVALID;
+        }
+}
+
 static boolean
 panfrost_resource_get_handle(struct pipe_screen *pscreen,
                              struct pipe_context *ctx,
@@ -117,7 +145,7 @@  panfrost_resource_get_handle(struct pipe_screen *pscreen,
         struct panfrost_resource *rsrc = (struct panfrost_resource *) pt;
         struct renderonly_scanout *scanout = rsrc->scanout;
 
-        handle->modifier = DRM_FORMAT_MOD_INVALID;
+        handle->modifier = panfrost_resource_modifier(rsrc);
 
 	if (handle->type == WINSYS_HANDLE_TYPE_SHARED) {
 		return FALSE;
@@ -341,7 +369,9 @@  panfrost_setup_slices(struct panfrost_resource *pres, size_t *bo_size)
 }
 
 static void
-panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_resource *pres)
+panfrost_resource_create_bo(struct panfrost_screen *screen,
+                            struct panfrost_resource *pres,
+                            const uint64_t *modifiers, int count)
 {
 	struct pipe_resource *res = &pres->base;
 
@@ -362,14 +392,19 @@  panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_reso
 
         /* Tiling textures is almost always faster, unless we only use it once */
 
+        bool can_afbc = panfrost_format_supports_afbc(res->format);
+        bool want_afbc = drm_find_modifier(DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP), modifiers, count);
+        bool want_linear = drm_find_modifier(DRM_FORMAT_MOD_LINEAR, modifiers, count);
+
 #define SCANOUT (PIPE_BIND_SCANOUT | PIPE_BIND_SHARED | PIPE_BIND_DISPLAY_TARGET)
 
         bool is_scanout = (res->bind & SCANOUT);
         bool is_texture = (res->bind & PIPE_BIND_SAMPLER_VIEW);
+        bool is_renderable = (res->bind & PIPE_BIND_RENDER_TARGET);
         bool is_2d = res->depth0 == 1 && res->array_size == 1;
         bool is_streaming = (res->usage == PIPE_USAGE_STREAM);
 
-        bool should_tile = !is_streaming && is_texture && is_2d && !is_scanout;
+        bool should_tile = !is_streaming && is_texture && is_2d && !want_linear && !is_scanout;
 
         /* Depth/stencil can't be tiled, only linear or AFBC */
         should_tile &= !(res->bind & PIPE_BIND_DEPTH_STENCIL);
@@ -381,17 +416,20 @@  panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_reso
         pres->checksummed = can_checksum && should_checksum;
 
         /* Set the layout appropriately */
-        pres->layout = should_tile ? PAN_TILED : PAN_LINEAR;
+	if (want_afbc || (is_renderable && can_afbc && !is_texture))
+                pres->layout = PAN_AFBC;
+        else
+                pres->layout = should_tile ? PAN_TILED : PAN_LINEAR;
 
         size_t bo_size;
-
         panfrost_setup_slices(pres, &bo_size);
         pres->bo = panfrost_drm_create_bo(screen, bo_size, 0);
 }
 
 static struct pipe_resource *
-panfrost_resource_create(struct pipe_screen *screen,
-                         const struct pipe_resource *template)
+panfrost_resource_create_with_modifiers(struct pipe_screen *screen,
+                                        const struct pipe_resource *template,
+                                        const uint64_t *modifiers, int count)
 {
         /* Make sure we're familiar */
         switch (template->target) {
@@ -418,13 +456,16 @@  panfrost_resource_create(struct pipe_screen *screen,
 
         util_range_init(&so->valid_buffer_range);
 
-        panfrost_resource_create_bo(pscreen, so);
+        panfrost_resource_create_bo(pscreen, so, modifiers, count);
 
         /* Set up the "scanout resource" (the dmabuf export of our buffer to
          * the KMS handle) if the buffer might ever have
          * resource_get_handle(WINSYS_HANDLE_TYPE_KMS) called on it.
+         * create_with_modifiers() doesn't give us usage flags, so we have to
+         * assume that all calls with modifiers are scanout-possible.
          */
-        if (template->bind & PIPE_BIND_SCANOUT) {
+        if (((template->bind & PIPE_BIND_SCANOUT) ||
+             !(count == 1 && modifiers[0] == DRM_FORMAT_MOD_INVALID))) {
                 so->scanout =
                         renderonly_scanout_for_resource(&so->base, pscreen->ro, NULL);
                 if (!so->scanout) {
@@ -436,6 +477,14 @@  panfrost_resource_create(struct pipe_screen *screen,
         return (struct pipe_resource *)so;
 }
 
+static struct pipe_resource *
+panfrost_resource_create(struct pipe_screen *screen,
+                         const struct pipe_resource *template)
+{
+	const uint64_t mod = DRM_FORMAT_MOD_INVALID;
+	return panfrost_resource_create_with_modifiers(screen, template, &mod, 1);
+}
+
 void
 panfrost_bo_reference(struct panfrost_bo *bo)
 {
@@ -779,8 +828,11 @@  static const struct u_transfer_vtbl transfer_vtbl = {
 void
 panfrost_resource_screen_init(struct panfrost_screen *pscreen)
 {
-        //pscreen->base.resource_create_with_modifiers =
-        //        panfrost_resource_create_with_modifiers;
+        if (pan_debug & PAN_DBG_MODIFIERS) {
+                pscreen->base.resource_create_with_modifiers =
+                        panfrost_resource_create_with_modifiers;
+        }
+
         pscreen->base.resource_create = u_transfer_helper_resource_create;
         pscreen->base.resource_destroy = u_transfer_helper_resource_destroy;
         pscreen->base.resource_from_handle = panfrost_resource_from_handle;
diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
index d53a906838eb..02b65e068c30 100644
--- a/src/gallium/drivers/panfrost/pan_screen.c
+++ b/src/gallium/drivers/panfrost/pan_screen.c
@@ -57,6 +57,7 @@ 
 static const struct debug_named_value debug_options[] = {
 	{"msgs",      PAN_DBG_MSGS,	"Print debug messages"},
 	{"trace",     PAN_DBG_TRACE,    "Trace the command stream"},
+	{"modifiers", PAN_DBG_MODIFIERS,"Enable modifiers support"},
 	DEBUG_NAMED_VALUE_END
 };
 
@@ -559,6 +560,38 @@  panfrost_screen_get_compiler_options(struct pipe_screen *pscreen,
         return &midgard_nir_options;
 }
 
+const uint64_t supported_modifiers[] = {
+   DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP),
+   DRM_FORMAT_MOD_LINEAR,
+};
+
+static void
+panfrost_query_dmabuf_modifiers(struct pipe_screen *pscreen,
+                                enum pipe_format format, int max,
+                                uint64_t *modifiers,
+                                unsigned int *external_only, int *count)
+{
+   int i, num_modifiers = 0;
+
+   if (max > ARRAY_SIZE(supported_modifiers))
+      max = ARRAY_SIZE(supported_modifiers);
+
+   if (!max) {
+      modifiers = NULL;
+      max = ARRAY_SIZE(supported_modifiers);
+   }
+
+   for (i = 0; num_modifiers < max; i++) {
+      if (modifiers)
+         modifiers[num_modifiers] = supported_modifiers[i];
+      if (external_only)
+         external_only[num_modifiers] = util_format_is_yuv(format) ? 1 : 0;
+      num_modifiers++;
+   }
+
+   *count = num_modifiers;
+}
+
 struct pipe_screen *
 panfrost_create_screen(int fd, struct renderonly *ro)
 {
@@ -599,6 +632,10 @@  panfrost_create_screen(int fd, struct renderonly *ro)
         screen->base.fence_reference = panfrost_fence_reference;
         screen->base.fence_finish = panfrost_fence_finish;
 
+        if (pan_debug & PAN_DBG_MODIFIERS) {
+                screen->base.query_dmabuf_modifiers = panfrost_query_dmabuf_modifiers;
+        }
+
 	screen->last_fragment_flushed = true;
         screen->last_job = NULL;
 
diff --git a/src/gallium/drivers/panfrost/pan_util.h b/src/gallium/drivers/panfrost/pan_util.h
index 8fd41420a486..a3a7ec3067e6 100644
--- a/src/gallium/drivers/panfrost/pan_util.h
+++ b/src/gallium/drivers/panfrost/pan_util.h
@@ -30,6 +30,7 @@ 
 
 #define PAN_DBG_MSGS		0x0001
 #define PAN_DBG_TRACE           0x0002
+#define PAN_DBG_MODIFIERS       0x0004
 
 extern int pan_debug;
 
@@ -38,4 +39,10 @@  extern int pan_debug;
 			fprintf(stderr, "%s:%d: "fmt, \
 				__FUNCTION__, __LINE__, ##__VA_ARGS__); } while (0)
 
+/* TODO: Pick these two up from kernel header */
+#define AFBC_FORMAT_MOD_ROCKCHIP (1ULL <<  20)
+
+#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
+       fourcc_mod_code(ARM, ((1ULL << 55) | 1))
+
 #endif /* PAN_UTIL_H */

Comments

On Thu, 4 Jul 2019 at 10:05, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>
> Implement query_dmabuf_modifiers and resource_create_with_modifiers so
> Wayland clients can share AFBC buffers with the compositor.
>
> For now this is guarded behind the PAN_MESA_DEBUG=modifiers env var
> because implementing those callbacks causes Weston to try to pass
> modifiers to the Rockchip KMS driver, which currently doesn't support
> modifiers, thus failing the modeset.
>
> This has been fixed in Weston 6.0, so we can enable unconditionally once
> we are confident that most people testing panfrost have upgraded.
>
> This lays the ground for scanning out AFBC buffers, if the KMS driver
> supports it.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>  src/gallium/drivers/panfrost/pan_drm.c      |  1 +
>  src/gallium/drivers/panfrost/pan_resource.c | 74 ++++++++++++++++++---
>  src/gallium/drivers/panfrost/pan_screen.c   | 37 +++++++++++
>  src/gallium/drivers/panfrost/pan_util.h     |  7 ++
>  4 files changed, 108 insertions(+), 11 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
> index 9648ac1d452d..623793a84411 100644
> --- a/src/gallium/drivers/panfrost/pan_drm.c
> +++ b/src/gallium/drivers/panfrost/pan_drm.c
> @@ -26,6 +26,7 @@
>  #include <fcntl.h>
>  #include <xf86drm.h>
>
> +#include "drm-uapi/drm_fourcc.h"
>  #include "drm-uapi/panfrost_drm.h"
>
>  #include "util/u_memory.h"
> diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
> index 54497d3de2bb..d901d43168fb 100644
> --- a/src/gallium/drivers/panfrost/pan_resource.c
> +++ b/src/gallium/drivers/panfrost/pan_resource.c
> @@ -34,6 +34,7 @@
>  #include "drm-uapi/drm_fourcc.h"
>
>  #include "state_tracker/winsys_handle.h"
> +#include "util/u_drm.h"
>  #include "util/u_format.h"
>  #include "util/u_memory.h"
>  #include "util/u_surface.h"
> @@ -91,6 +92,18 @@ panfrost_resource_from_handle(struct pipe_screen *pscreen,
>
>         rsc->bo = panfrost_drm_import_bo(screen, whandle->handle);
>
> +       switch(whandle->modifier) {
> +       case DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP):
> +               rsc->layout = PAN_AFBC;
> +               break;
> +       case DRM_FORMAT_MOD_LINEAR:
> +               rsc->layout = PAN_LINEAR;
> +               break;
> +       default:
> +                fprintf(stderr, "unknown modifier: 0x%"PRIx64"\n", whandle->modifier);
> +               assert(0);
> +       }
> +
>          size_t bo_size;
>          panfrost_setup_slices(rsc, &bo_size);
>          assert(bo_size == rsc->bo->size);
> @@ -106,6 +119,21 @@ panfrost_resource_from_handle(struct pipe_screen *pscreen,
>          return prsc;
>  }
>
> +static uint64_t
> +panfrost_resource_modifier(struct panfrost_resource *rsrc)
> +{
> +        switch (rsrc->layout) {
> +        case PAN_AFBC:
> +                return DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP);
> +        case PAN_TILED:
> +                return DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED;
> +        case PAN_LINEAR:
> +                return DRM_FORMAT_MOD_LINEAR;
> +        default:
> +                return DRM_FORMAT_MOD_INVALID;
> +        }
> +}
> +
>  static boolean
>  panfrost_resource_get_handle(struct pipe_screen *pscreen,
>                               struct pipe_context *ctx,
> @@ -117,7 +145,7 @@ panfrost_resource_get_handle(struct pipe_screen *pscreen,
>          struct panfrost_resource *rsrc = (struct panfrost_resource *) pt;
>          struct renderonly_scanout *scanout = rsrc->scanout;
>
> -        handle->modifier = DRM_FORMAT_MOD_INVALID;
> +        handle->modifier = panfrost_resource_modifier(rsrc);
>
>         if (handle->type == WINSYS_HANDLE_TYPE_SHARED) {
>                 return FALSE;
> @@ -341,7 +369,9 @@ panfrost_setup_slices(struct panfrost_resource *pres, size_t *bo_size)
>  }
>
>  static void
> -panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_resource *pres)
> +panfrost_resource_create_bo(struct panfrost_screen *screen,
> +                            struct panfrost_resource *pres,
> +                            const uint64_t *modifiers, int count)
>  {
>         struct pipe_resource *res = &pres->base;
>
> @@ -362,14 +392,19 @@ panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_reso
>
>          /* Tiling textures is almost always faster, unless we only use it once */
>
> +        bool can_afbc = panfrost_format_supports_afbc(res->format);
> +        bool want_afbc = drm_find_modifier(DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP), modifiers, count);
> +        bool want_linear = drm_find_modifier(DRM_FORMAT_MOD_LINEAR, modifiers, count);
> +
>  #define SCANOUT (PIPE_BIND_SCANOUT | PIPE_BIND_SHARED | PIPE_BIND_DISPLAY_TARGET)
>
>          bool is_scanout = (res->bind & SCANOUT);
>          bool is_texture = (res->bind & PIPE_BIND_SAMPLER_VIEW);
> +        bool is_renderable = (res->bind & PIPE_BIND_RENDER_TARGET);
>          bool is_2d = res->depth0 == 1 && res->array_size == 1;
>          bool is_streaming = (res->usage == PIPE_USAGE_STREAM);
>
> -        bool should_tile = !is_streaming && is_texture && is_2d && !is_scanout;
> +        bool should_tile = !is_streaming && is_texture && is_2d && !want_linear && !is_scanout;
>
>          /* Depth/stencil can't be tiled, only linear or AFBC */
>          should_tile &= !(res->bind & PIPE_BIND_DEPTH_STENCIL);
> @@ -381,17 +416,20 @@ panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_reso
>          pres->checksummed = can_checksum && should_checksum;
>
>          /* Set the layout appropriately */
> -        pres->layout = should_tile ? PAN_TILED : PAN_LINEAR;
> +       if (want_afbc || (is_renderable && can_afbc && !is_texture))
> +                pres->layout = PAN_AFBC;

We regress here because we seem to have some bugs regarding AFBC usage
with some formats such as rgba4 and rgb5_a1:

https://gitlab.freedesktop.org/tomeu/mesa/-/jobs/399237

I'm looking at only enabling AFBC when explicitly asked to by the winsys.

Cheers,

Tomeu

> +        else
> +                pres->layout = should_tile ? PAN_TILED : PAN_LINEAR;
>
>          size_t bo_size;
> -
>          panfrost_setup_slices(pres, &bo_size);
>          pres->bo = panfrost_drm_create_bo(screen, bo_size, 0);
>  }
>
>  static struct pipe_resource *
> -panfrost_resource_create(struct pipe_screen *screen,
> -                         const struct pipe_resource *template)
> +panfrost_resource_create_with_modifiers(struct pipe_screen *screen,
> +                                        const struct pipe_resource *template,
> +                                        const uint64_t *modifiers, int count)
>  {
>          /* Make sure we're familiar */
>          switch (template->target) {
> @@ -418,13 +456,16 @@ panfrost_resource_create(struct pipe_screen *screen,
>
>          util_range_init(&so->valid_buffer_range);
>
> -        panfrost_resource_create_bo(pscreen, so);
> +        panfrost_resource_create_bo(pscreen, so, modifiers, count);
>
>          /* Set up the "scanout resource" (the dmabuf export of our buffer to
>           * the KMS handle) if the buffer might ever have
>           * resource_get_handle(WINSYS_HANDLE_TYPE_KMS) called on it.
> +         * create_with_modifiers() doesn't give us usage flags, so we have to
> +         * assume that all calls with modifiers are scanout-possible.
>           */
> -        if (template->bind & PIPE_BIND_SCANOUT) {
> +        if (((template->bind & PIPE_BIND_SCANOUT) ||
> +             !(count == 1 && modifiers[0] == DRM_FORMAT_MOD_INVALID))) {
>                  so->scanout =
>                          renderonly_scanout_for_resource(&so->base, pscreen->ro, NULL);
>                  if (!so->scanout) {
> @@ -436,6 +477,14 @@ panfrost_resource_create(struct pipe_screen *screen,
>          return (struct pipe_resource *)so;
>  }
>
> +static struct pipe_resource *
> +panfrost_resource_create(struct pipe_screen *screen,
> +                         const struct pipe_resource *template)
> +{
> +       const uint64_t mod = DRM_FORMAT_MOD_INVALID;
> +       return panfrost_resource_create_with_modifiers(screen, template, &mod, 1);
> +}
> +
>  void
>  panfrost_bo_reference(struct panfrost_bo *bo)
>  {
> @@ -779,8 +828,11 @@ static const struct u_transfer_vtbl transfer_vtbl = {
>  void
>  panfrost_resource_screen_init(struct panfrost_screen *pscreen)
>  {
> -        //pscreen->base.resource_create_with_modifiers =
> -        //        panfrost_resource_create_with_modifiers;
> +        if (pan_debug & PAN_DBG_MODIFIERS) {
> +                pscreen->base.resource_create_with_modifiers =
> +                        panfrost_resource_create_with_modifiers;
> +        }
> +
>          pscreen->base.resource_create = u_transfer_helper_resource_create;
>          pscreen->base.resource_destroy = u_transfer_helper_resource_destroy;
>          pscreen->base.resource_from_handle = panfrost_resource_from_handle;
> diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
> index d53a906838eb..02b65e068c30 100644
> --- a/src/gallium/drivers/panfrost/pan_screen.c
> +++ b/src/gallium/drivers/panfrost/pan_screen.c
> @@ -57,6 +57,7 @@
>  static const struct debug_named_value debug_options[] = {
>         {"msgs",      PAN_DBG_MSGS,     "Print debug messages"},
>         {"trace",     PAN_DBG_TRACE,    "Trace the command stream"},
> +       {"modifiers", PAN_DBG_MODIFIERS,"Enable modifiers support"},
>         DEBUG_NAMED_VALUE_END
>  };
>
> @@ -559,6 +560,38 @@ panfrost_screen_get_compiler_options(struct pipe_screen *pscreen,
>          return &midgard_nir_options;
>  }
>
> +const uint64_t supported_modifiers[] = {
> +   DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP),
> +   DRM_FORMAT_MOD_LINEAR,
> +};
> +
> +static void
> +panfrost_query_dmabuf_modifiers(struct pipe_screen *pscreen,
> +                                enum pipe_format format, int max,
> +                                uint64_t *modifiers,
> +                                unsigned int *external_only, int *count)
> +{
> +   int i, num_modifiers = 0;
> +
> +   if (max > ARRAY_SIZE(supported_modifiers))
> +      max = ARRAY_SIZE(supported_modifiers);
> +
> +   if (!max) {
> +      modifiers = NULL;
> +      max = ARRAY_SIZE(supported_modifiers);
> +   }
> +
> +   for (i = 0; num_modifiers < max; i++) {
> +      if (modifiers)
> +         modifiers[num_modifiers] = supported_modifiers[i];
> +      if (external_only)
> +         external_only[num_modifiers] = util_format_is_yuv(format) ? 1 : 0;
> +      num_modifiers++;
> +   }
> +
> +   *count = num_modifiers;
> +}
> +
>  struct pipe_screen *
>  panfrost_create_screen(int fd, struct renderonly *ro)
>  {
> @@ -599,6 +632,10 @@ panfrost_create_screen(int fd, struct renderonly *ro)
>          screen->base.fence_reference = panfrost_fence_reference;
>          screen->base.fence_finish = panfrost_fence_finish;
>
> +        if (pan_debug & PAN_DBG_MODIFIERS) {
> +                screen->base.query_dmabuf_modifiers = panfrost_query_dmabuf_modifiers;
> +        }
> +
>         screen->last_fragment_flushed = true;
>          screen->last_job = NULL;
>
> diff --git a/src/gallium/drivers/panfrost/pan_util.h b/src/gallium/drivers/panfrost/pan_util.h
> index 8fd41420a486..a3a7ec3067e6 100644
> --- a/src/gallium/drivers/panfrost/pan_util.h
> +++ b/src/gallium/drivers/panfrost/pan_util.h
> @@ -30,6 +30,7 @@
>
>  #define PAN_DBG_MSGS           0x0001
>  #define PAN_DBG_TRACE           0x0002
> +#define PAN_DBG_MODIFIERS       0x0004
>
>  extern int pan_debug;
>
> @@ -38,4 +39,10 @@ extern int pan_debug;
>                         fprintf(stderr, "%s:%d: "fmt, \
>                                 __FUNCTION__, __LINE__, ##__VA_ARGS__); } while (0)
>
> +/* TODO: Pick these two up from kernel header */
> +#define AFBC_FORMAT_MOD_ROCKCHIP (1ULL <<  20)
> +
> +#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
> +       fourcc_mod_code(ARM, ((1ULL << 55) | 1))
> +
>  #endif /* PAN_UTIL_H */
> --
> 2.20.1
>
Hi Tomeu,

On Thu, 4 Jul 2019 at 09:05, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> @@ -362,14 +392,19 @@ panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_reso
>
>          /* Tiling textures is almost always faster, unless we only use it once */
>
> +        bool can_afbc = panfrost_format_supports_afbc(res->format);
> +        bool want_afbc = drm_find_modifier(DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP), modifiers, count);
> +        bool want_linear = drm_find_modifier(DRM_FORMAT_MOD_LINEAR, modifiers, count);
>
> [...]
> -        bool should_tile = !is_streaming && is_texture && is_2d && !is_scanout;
> +        bool should_tile = !is_streaming && is_texture && is_2d && !want_linear && !is_scanout;

I don't think this is right.

The list of modifiers we pass in is the total list of acceptable
modifiers. Clients don't make any judgement or discernment about them,
but just pass in the full list. For example, in Weston we use EGL to
query the list of all modifiers supported by the driver, and then give
those to the client as the list of acceptable modifiers; the EGL
Wayland winsys will then pass that list into DRIImage create.

This means that 'want_linear' will be true as long as Panfrost (or, on
GBM, the KMS driver) supports linear, which realistically is all the
time.

I think ultimately we want should_tile to be (in order of evaluation):
  * definitively false if we can't support it - GPU doesn't support
it, isn't a 2D texture/RT, etc
  * if the above passes, definitively true if the above conditions are
met and we have passed in a modifier set which includes AFBC
  * if we can do it (first test) but we have no modifiers,
definitively false if it's a shared/scanout buffer (i.e. we need to
export the buffer somewhere else but the client isn't modifier-aware),
or definitively true if it's just an internal buffer

That code seems pretty familiar from VC4 (which is in turn probably
from freedreno? everything else is). It could be semi-useful to look
at how VC4 has structured its resource creation decision tree for
modifiers, but on the other hand they also have an implicit fallback
mode; when modifiers aren't available, it will pass the tiling
information through a magic kernel side-channel, which we really don't
want to do here.

Cheers,
Daniel
Hi,

On Thu, 4 Jul 2019 at 09:47, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On Thu, 4 Jul 2019 at 10:05, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> > -        pres->layout = should_tile ? PAN_TILED : PAN_LINEAR;
> > +       if (want_afbc || (is_renderable && can_afbc && !is_texture))
> > +                pres->layout = PAN_AFBC;
>
> We regress here because we seem to have some bugs regarding AFBC usage
> with some formats such as rgba4 and rgb5_a1:
>
> https://gitlab.freedesktop.org/tomeu/mesa/-/jobs/399237
>
> I'm looking at only enabling AFBC when explicitly asked to by the winsys.

Again, the winsys just pulls the list of acceptable format/modifier
pairs from the EGL format query. If Panfrost declares 1555+AFBC to be
a supported combination, then the winsys will pass that on for clients
to use. If it isn't working, my suggestion is to just never advertise
it in the first place.

Cheers,
Daniel


Hi,

On Fri, 5 Jul 2019 at 14:51, Alyssa Rosenzweig
<alyssa.rosenzweig@collabora.com> wrote:
> > +     switch(whandle->modifier) {
> > +     case DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_ROCKCHIP):
> > +             rsc->layout = PAN_AFBC;
> > +             break;
>
> Why ROCKCHIP in particular? AFBC fourcc codes are based on modifiers,
> and there are a *lot* of them. We need to figure out which upstream
> modifier PAN_AFBC actually enables and use that:
>
> [...]
>
> On other AFBC flags, I don't know. I'm also not sure if Arm would chime
> in one way or the other.
>
> Mainly the issue is that whandle->modifier could contain some other,
> non-Rockchip modifier, and then the whole stack collapses; AFBC
> modifiers are meant to be parsedd, not =='d. See the komeda driver in
> the kernel for reference.

Yes and no. See below.

> > +     if (want_afbc || (is_renderable && can_afbc && !is_texture))
>
> This doesn't seem quite correct. We preselect AFBC if:
>
>         - AFBC is explicitly requested, OR
>         - We're rendering to something that's not bound as a texture
>           that could be AFBC
>
> In case one, what happens if AFBC is explicitly requested but we don't
> support AFBC on that format? It should fallback to linear, or error out.
>
> In case two, what if the modifier for linear or tiled is explicitly set?
> Now we've given them AFBC when they've asked for something else;
> switching it up is only safe for internal BOs.
>
> The logic in this code block has only ever been for internal BOs, where
> it's up to *us* to pick a format. Imports are logically distinct, since
> it's now up to the modifiers. Two different problems, unless the
> modifier is a "don't care".

It's still up to you-the-driver to pick a modifier.

EGL/Wayland/KMS all advertise sets of modifiers to clients. In
general, clients don't try to reason about the contents of those lists
- if they have multiple lists, they might intersect them, or they
might create their own list-of-one to be LINEAR, or whatever.

Modifier lists are explicitly unsorted. The list provided is the
modifiers which are acceptable to use. The driver is expected to sort
(on whatever metric) for 'optimality'. If none of the modifiers
provided are usable, then resource creation is free to fail. If the
provided modifiers are not optimal, then the driver just has to select
that less-optimal modifier. The only no-no is to select a modifier
which is not in the provided list.

Cheers,
Daniel