[Mesa-dev] [PATCH 8/8] nvc0: Add shader disk caching

[Emil Velikov]

Hi Mark,

Above all - welcome to nouveau. Glad to see fresh blood joining.

On Mon, 17 Feb 2020 at 17:41, Mark Menzynski <mmenzyns at redhat.com> wrote:
>
> Adds shader disk caching for nvc0 to reduce the need to every time compile
> shaders. Shaders are saved into disk_shader_cache from nvc0_screen structure.
>
> It serializes the input nv50_ir_prog_info to compute the hash key and
> also to do a byte compare between the original nv50_ir_prog_info and the one
> saved in the cache. If keys match and also the byte compare returns they
> are equal, shaders are same, and the compiled nv50_ir_prog_info_out from the
> cache can be used instead of compiling input info.
>
> Seems to be significantly improving loading times. Piglit tests seem
> to be OK.
>
Really happy to see this happening. Do you mind adding some rough
numbers in the commit message.

Out of curiosity: Is there any specific reason why nv50 isn't also wired up?
Is it a case of didn't test, or there's something more subtle?

> Signed-off-by: Mark Menzynski <mmenzyns at redhat.com>
> ---
>  .../drivers/nouveau/nvc0/nvc0_context.h       |  1 +
>  .../drivers/nouveau/nvc0/nvc0_program.c       | 49 ++++++++++++++++---
>  .../drivers/nouveau/nvc0/nvc0_shader_state.c  |  3 +-
>  src/gallium/drivers/nouveau/nvc0/nvc0_state.c |  2 +
>  4 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
> index 8a2a8f2797e..4b83d1afeb4 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_context.h
> @@ -321,6 +321,7 @@ extern struct draw_stage *nvc0_draw_render_stage(struct nvc0_context *);
>
>  /* nvc0_program.c */
>  bool nvc0_program_translate(struct nvc0_program *, uint16_t chipset,
> +                            struct disk_cache *,
>                              struct pipe_debug_callback *);
>  bool nvc0_program_upload(struct nvc0_context *, struct nvc0_program *);
>  void nvc0_program_destroy(struct nvc0_context *, struct nvc0_program *);
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_program.c b/src/gallium/drivers/nouveau/nvc0/nvc0_program.c
> index 1a5073292e8..06b6f7b4db5 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_program.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_program.c
> @@ -24,6 +24,7 @@
>
>  #include "compiler/nir/nir.h"
>  #include "tgsi/tgsi_ureg.h"
> +#include "util/blob.h"
>
>  #include "nvc0/nvc0_context.h"
>
> @@ -568,11 +569,19 @@ nvc0_program_dump(struct nvc0_program *prog)
>
>  bool
>  nvc0_program_translate(struct nvc0_program *prog, uint16_t chipset,
> +                       struct disk_cache *disk_shader_cache,
>                         struct pipe_debug_callback *debug)
>  {
> +   struct blob blob;
>     struct nv50_ir_prog_info *info;
>     struct nv50_ir_prog_info_out info_out = {};
> -   int ret;
> +
> +   void *cached_data = NULL;
> +   size_t cached_size;
> +   bool shader_found = false;
> +
> +   int ret = 0;
> +   cache_key key;
>
>     info = CALLOC_STRUCT(nv50_ir_prog_info);
>     if (!info)
> @@ -631,14 +640,38 @@ nvc0_program_translate(struct nvc0_program *prog, uint16_t chipset,
>     info->assignSlots = nvc0_program_assign_varying_slots;
>
>     /* these fields might be overwritten by the compiler */
> -   info_out.bin.smemSize = prog->cp.smem_size;
> -   info_out.io.genUserClip = prog->vp.num_ucps;
> -
> -   ret = nv50_ir_generate_code(info, &info_out);
> -   if (ret) {
> -      NOUVEAU_ERR("shader translation failed: %i\n", ret);
> -      goto out;
> +   info->bin.smemSize = prog->cp.smem_size;
> +   info->io.genUserClip = prog->vp.num_ucps;
> +
> +   blob_init(&blob);
> +   nv50_ir_prog_info_serialize(&blob, info);
> +
> +   if (disk_shader_cache) {

Will this work correctly, if the cache is explicitly disabled?

It's been years since I looked at this code so I wonder if the two
original "info_out... = prog->..." shouldn't stay as-is.
At the same time it will make sense to move the "info->... =
prog->..." + serialise() within the if (disk_shader_cache) hunk.

blob_init() seems safe, so it can stay as-is.

HTH
Emil