[Mesa-dev] [PATCH 12/34] i965: Handle Y-tile modifier

[Jason Ekstrand]

On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <ben at bwidawsk.net> wrote:

> This patch begins introducing how we'll actually handle the potentially
> many modifiers coming in from the API, how we'll store them, and the
> structure in the code to support it.
>
> Prior to this patch, the Y-tiled modifier would be entirely ignored. It
> shouldn't actually be used until this point because we've not bumped the
> DRIimage extension version (which is a requirement to use modifiers).
>
> With X-tiling:
> Writes:          6,583.58 MiB
> Reads:           6,540.93 MiB
>
> With Y-tiling:
> Writes:          5,361.78 MiB
> Reads            6,052.45 MiB
>
> Savings per frame
> Writes:  2 MiB
> Reads:  .8 MiB
>
> Similar functionality was introduced and then reverted here:
>
> commit 6a0d036483caf87d43ebe2edd1905873446c9589
> Author: Ben Widawsky <ben at bwidawsk.net>
> Date:   Thu Apr 21 20:14:58 2016 -0700
>
>     i965: Always use Y-tiled buffers on SKL+
>
> Signed-off-by: Ben Widawsky <benjamin.widawsky at intel.com>
> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
> Acked-by: Daniel Stone <daniels at collabora.com>
> ---
>  src/mesa/drivers/dri/i965/intel_screen.c | 55
> ++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index ebfa74a8ff..6eaf146181 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -23,6 +23,7 @@
>   * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>   */
>
> +#include <drm_fourcc.h>
>  #include <errno.h>
>  #include <time.h>
>  #include <unistd.h>
> @@ -559,16 +560,35 @@ select_best_modifier(struct gen_device_info *devinfo,
>                       const uint64_t *modifiers,
>                       const unsigned count)
>  {
> -   uint64_t modifier = DRM_FORMAT_MOD_INVALID;
> +#define YTILE  (1 << 1)
> +#define LINEAR (1 << 0)
> +
> +   const uint64_t prio_modifiers[] = { I915_FORMAT_MOD_Y_TILED,
> DRM_FORMAT_MOD_LINEAR };
> +   uint32_t modifier_bitmask = 0; /* API only allows 32 */
>

Is 32 an API limitation?  Only looking locally, it seems to just be a
limitation of the algorithm used here for picking modifiers.  Given that
the algorithm can be easily extended if needed, I see no reason why we need
to make it an API limitation.


>
>     for (int i = 0; i < count; i++) {
>        switch (modifiers[i]) {
>        case DRM_FORMAT_MOD_LINEAR:
> -         return modifiers[i];
> +         modifier_bitmask |= LINEAR;
> +         break;
> +      case I915_FORMAT_MOD_Y_TILED:
> +         if (devinfo->gen < 9) {
> +            _mesa_warning(NULL, "Invalid Y-tiling parameter\n");
>

Why?  We can support Y tiling on all our hardware, you just can't scan it
out prior to Sky Lake.  However, if you were doing mesa <-> mesa for
compositing, Y-tiling is perfectly fine all the way back.


> +            continue;
> +         }
> +
> +         modifier_bitmask |= YTILE;
> +         break;
>        }
>     }
>
> -   return modifier;
> +   if (modifier_bitmask)
> +      return prio_modifiers[ffsll(modifier_bitmask)-1];
> +
> +   return DRM_FORMAT_MOD_INVALID;
> +
> +#undef LINEAR
> +#undef YTILE
>  }
>
>  static __DRIimage *
> @@ -599,6 +619,9 @@ __intel_create_image(__DRIscreen *dri_screen,
>     case DRM_FORMAT_MOD_LINEAR:
>        tiling = I915_TILING_NONE;
>        break;
> +   case I915_FORMAT_MOD_Y_TILED:
> +      tiling = I915_TILING_Y;
> +      break;
>     case DRM_FORMAT_MOD_INVALID:
>     default:
>           break;
> @@ -650,8 +673,26 @@ intel_create_image_with_modifiers(__DRIscreen
> *dri_screen,
>                                    const unsigned count,
>                                    void *loaderPrivate)
>  {
> -   return __intel_create_image(dri_screen, width, height, format, 0,
> NULL, 0,
> -                               loaderPrivate);
> +   uint64_t local_mods[count];
>

Should we really be stack allocating this?  It comes in from the user and
could be arbitrarily large.  It's probably not a problem, but this patch
got me thinking about modifier counts.


> +   int local_count = 0;
> +
> +   /* This compacts the actual modifiers to the ones we know how to
> handle */
> +   for (int i = 0; i < count; i++) {
> +      switch (modifiers[i]) {
> +      case I915_FORMAT_MOD_Y_TILED:
> +      case DRM_FORMAT_MOD_LINEAR:
> +         local_mods[local_count++] = modifiers[i];
> +         break;
> +      case DRM_FORMAT_MOD_INVALID:
> +         unreachable("Invalid modifiers specified\n");
> +      default:
> +         /* Modifiers from other vendors would land here. */
> +         break;
> +      }
> +   }
> +
> +   return __intel_create_image(dri_screen, width, height, format, 0,
> +                               local_mods, local_count, loaderPrivate);
>  }
>
>  static GLboolean
> @@ -1912,7 +1953,9 @@ intelAllocateBuffer(__DRIscreen *dri_screen,
>     if (intelBuffer == NULL)
>        return NULL;
>
> -   /* The front and back buffers are color buffers, which are X tiled. */
> +   /* The front and back buffers are color buffers, which are X tiled.
> GEN9+
> +    * supports Y tiled and compressed buffers, but there is no way to
> plumb that
> +    * through to here. */
>     uint32_t tiling = I915_TILING_X;
>     unsigned long pitch;
>     int cpp = format / 8;
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170131/298e8430/attachment.html>