[Mesa-dev] [PATCH] panfrost: Refactor blend descriptors

Connor Abbott cwabbott0 at gmail.com
Sat May 4 23:20:49 UTC 2019


On Sun, May 5, 2019 at 12:14 AM Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
>
> This commit does a fairly large cleanup of blend descriptors, although
> there should not be any functional changes. In particular, we split
> apart the Midgard and Bifrost blend descriptors, since they are
> radically different. From there, we can identify that the Midgard
> descriptor as previously written was really two render targets'
> descriptors stuck together. From this observation, we split the Midgard
> descriptor into what a single RT actually needs. This enables us to
> correctly dump blending configuration for MRT samples on Midgard. It
> also allows the Midgard and Bifrost blend code to peacefully coexist,
> with runtime selection rather than a #ifdef. So, as a bonus, this will
> help the future Bifrost effort, eliminating one major source of
> compile-time architectural divergence.
>
> Signed-off-by: Alyssa Rosenzweig <alyssa at rosenzweig.io>
> ---
>  .../drivers/panfrost/include/panfrost-job.h   |  56 ++++---
>  src/gallium/drivers/panfrost/pan_context.c    |  31 ++--
>  .../drivers/panfrost/pandecode/decode.c       | 155 +++++++++---------
>  3 files changed, 122 insertions(+), 120 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/include/panfrost-job.h b/src/gallium/drivers/panfrost/include/panfrost-job.h
> index c2d922678b8..71ac054f7c3 100644
> --- a/src/gallium/drivers/panfrost/include/panfrost-job.h
> +++ b/src/gallium/drivers/panfrost/include/panfrost-job.h
> @@ -415,25 +415,37 @@ enum mali_format {
>  #define MALI_READS_ZS (1 << 12)
>  #define MALI_READS_TILEBUFFER (1 << 16)
>
> -struct mali_blend_meta {
> -#ifndef BIFROST
> -        /* Base value of 0x200.
> +/* The raw Midgard blend payload can either be an equation or a shader
> + * address, depending on the context */
> +
> +union midgard_blend {
> +        mali_ptr shader;
> +        struct mali_blend_equation equation;
> +};
> +
> +/* On MRT Midgard systems (using an MFBD), each render target gets its own
> + * blend descriptor */
> +
> +struct midgard_blend_rt {
> +        /* Flags base value of 0x200 to enable the render target.
>           * OR with 0x1 for blending (anything other than REPLACE).
> -         * OR with 0x2 for programmable blending
> +         * OR with 0x2 for programmable blending with 0-2 registers
> +         * OR with 0x3 for programmable blending with 2+ registers
>           */
>
> -        u64 unk1;
> +        u64 flags;
> +        union midgard_blend blend;
> +} __attribute__((packed));
>
> -        union {
> -                struct mali_blend_equation blend_equation_1;
> -                mali_ptr blend_shader;
> -        };
> +/* On Bifrost systems (all MRT), each render target gets one of these
> + * descriptors */
> +
> +struct bifrost_blend_rt {
> +        /* This is likely an analogue of the flags on
> +         * midgard_blend_rt */
>
> -        u64 zero2;
> -        struct mali_blend_equation blend_equation_2;
> -#else
>          u32 unk1; // = 0x200
> -        struct mali_blend_equation blend_equation;
> +        struct mali_blend_equation equation;
>          /*
>           * - 0x19 normally
>           * - 0x3 when this slot is unused (everything else is 0 except the index)
> @@ -479,11 +491,13 @@ struct mali_blend_meta {
>                  * in the same pool as the original shader. The kernel will
>                  * make sure this allocation is aligned to 2^24 bytes.
>                  */
> -               u32 blend_shader;
> +               u32 shader;
>         };
> -#endif
>  } __attribute__((packed));
>
> +/* Descriptor for the shader. Following this is at least one, up to four blend
> + * descriptors for each active render target */
> +
>  struct mali_shader_meta {
>          mali_ptr shader;
>          u16 texture_count;
> @@ -584,17 +598,7 @@ struct mali_shader_meta {
>           * MALI_HAS_BLEND_SHADER to decide how to interpret.
>           */
>
> -        union {
> -                mali_ptr blend_shader;
> -                struct mali_blend_equation blend_equation;
> -        };
> -
> -        /* There can be up to 4 blend_meta's. None of them are required for
> -         * vertex shaders or the non-MRT case for Midgard (so the blob doesn't
> -         * allocate any space).
> -         */
> -        struct mali_blend_meta blend_meta[];
> -
> +        union midgard_blend blend;
>  } __attribute__((packed));
>
>  /* This only concerns hardware jobs */
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index 17b5b75db92..cab7c89ac8b 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -1000,7 +1000,7 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
>                   * maybe both are read...?) */
>
>                  if (ctx->blend->has_blend_shader) {
> -                        ctx->fragment_shader_core.blend_shader = ctx->blend->blend_shader;
> +                        ctx->fragment_shader_core.blend.shader = ctx->blend->blend_shader;
>                  }
>
>                  if (ctx->require_sfbd) {
> @@ -1010,7 +1010,7 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
>                           * modes (so we're able to read back the destination buffer) */
>
>                          if (!ctx->blend->has_blend_shader) {
> -                                memcpy(&ctx->fragment_shader_core.blend_equation, &ctx->blend->equation, sizeof(ctx->blend->equation));
> +                                ctx->fragment_shader_core.blend.equation = ctx->blend->equation;
>                          }
>
>                          if (!no_blending) {
> @@ -1018,7 +1018,7 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
>                          }
>                  }
>
> -                size_t size = sizeof(struct mali_shader_meta) + sizeof(struct mali_blend_meta);
> +                size_t size = sizeof(struct mali_shader_meta) + sizeof(struct midgard_blend_rt);
>                  struct panfrost_transfer transfer = panfrost_allocate_transient(ctx, size);
>                  memcpy(transfer.cpu, &ctx->fragment_shader_core, sizeof(struct mali_shader_meta));
>
> @@ -1027,7 +1027,7 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
>                  if (!ctx->require_sfbd) {
>                          /* Additional blend descriptor tacked on for jobs using MFBD */
>
> -                        unsigned blend_count = 0;
> +                        unsigned blend_count = 0x200;
>
>                          if (ctx->blend->has_blend_shader) {
>                                  /* For a blend shader, the bottom nibble corresponds to
> @@ -1045,25 +1045,20 @@ panfrost_emit_for_draw(struct panfrost_context *ctx, bool with_vertex_data)
>                                          blend_count |= 0x1;
>                          }
>
> -                        /* Second blend equation is always a simple replace */
> +                        struct midgard_blend_rt rts[4];
>
> -                        uint64_t replace_magic = 0xf0122122;
> -                        struct mali_blend_equation replace_mode;
> -                        memcpy(&replace_mode, &replace_magic, sizeof(replace_mode));
> +                        /* TODO: MRT */
>
> -                        struct mali_blend_meta blend_meta[] = {
> -                                {
> -                                        .unk1 = 0x200 | blend_count,
> -                                        .blend_equation_1 = ctx->blend->equation,
> -                                        .blend_equation_2 = replace_mode
> -                                },
> -                        };
> +                        for (unsigned i = 0; i < 1; ++i) {
> +                                rts[i].flags = blend_count;
>
> -                        if (ctx->blend->has_blend_shader) {
> -                                blend_meta[0].blend_shader = ctx->blend->blend_shader;
> +                                if (ctx->blend->has_blend_shader)
> +                                        rts[i].blend.shader = ctx->blend->blend_shader;
> +                                else
> +                                        rts[i].blend.equation = ctx->blend->equation;
>                          }
>
> -                        memcpy(transfer.cpu + sizeof(struct mali_shader_meta), blend_meta, sizeof(blend_meta));
> +                        memcpy(transfer.cpu + sizeof(struct mali_shader_meta), rts, sizeof(rts[0]) * 1);
>                  }
>          }
>
> diff --git a/src/gallium/drivers/panfrost/pandecode/decode.c b/src/gallium/drivers/panfrost/pandecode/decode.c
> index 9936249e524..9d9b9b31bcd 100644
> --- a/src/gallium/drivers/panfrost/pandecode/decode.c
> +++ b/src/gallium/drivers/panfrost/pandecode/decode.c
> @@ -857,12 +857,12 @@ pandecode_replay_stencil(const char *name, const struct mali_stencil_test *stenc
>  }
>
>  static void
> -pandecode_replay_blend_equation(const struct mali_blend_equation *blend, const char *suffix)
> +pandecode_replay_blend_equation(const struct mali_blend_equation *blend)
>  {
>          if (blend->zero1)
>                  pandecode_msg("Blend zero tripped: %X\n", blend->zero1);
>
> -        pandecode_log(".blend_equation%s = {\n", suffix);
> +        pandecode_log(".equation = {\n");
>          pandecode_indent++;
>
>          pandecode_prop("rgb_mode = 0x%X", blend->rgb_mode);
> @@ -876,6 +876,69 @@ pandecode_replay_blend_equation(const struct mali_blend_equation *blend, const c
>          pandecode_log("},\n");
>  }
>
> +static mali_ptr
> +pandecode_bifrost_blend(void *descs, int job_no, int rt_no)
> +{
> +        struct bifrost_blend_rt *b =
> +                ((struct bifrost_blend_rt *) descs) + rt_no;
> +
> +        pandecode_log("struct bifrost_blend_rt blend_rt_%d_%d = {\n", job_no, rt_no);
> +        pandecode_indent++;
> +
> +        pandecode_prop("unk1 = 0x%" PRIx32, b->unk1);
> +        /* TODO figure out blend shader enable bit */

The blend shader enable bit is already described in the comments in
the header; the blend shader is enabled when unk2 == 0. You have to
fish out the high 32 bits from the main shader (the blend shader has
to be allocated within the same 2^24 byte range as the main shader for
it to work properly anyways, even on Midgard, which is probably not
implemented properly on mainline). Maybe it would be better if these
functions got passed the mali_shader_descriptor itself?

> +        pandecode_replay_blend_equation(&b->equation);
> +        pandecode_prop("unk2 = 0x%" PRIx16, b->unk2);
> +        pandecode_prop("index = 0x%" PRIx16, b->index);
> +        pandecode_prop("shader = 0x%" PRIx32, b->shader);
> +
> +        pandecode_indent--;
> +        pandecode_log("},\n");
> +
> +        return 0;
> +}
> +
> +static mali_ptr
> +pandecode_midgard_blend(union midgard_blend *blend, bool is_shader)
> +{
> +        pandecode_log(".blend = {\n");
> +        pandecode_indent++;
> +
> +        if (is_shader) {
> +                pandecode_replay_shader_address("shader", blend->shader);
> +        } else {
> +                pandecode_replay_blend_equation(&blend->equation);
> +        }
> +
> +        pandecode_indent--;
> +        pandecode_log("},\n");
> +
> +        /* Return blend shader to disassemble if present */
> +        return is_shader ? (blend->shader & ~0xF) : 0;
> +}
> +
> +static mali_ptr
> +pandecode_midgard_blend_mrt(void *descs, int job_no, int rt_no)
> +{
> +        struct midgard_blend_rt *b =
> +                ((struct midgard_blend_rt *) descs) + rt_no;
> +
> +        /* Flags determine presence of blend shader */
> +        bool is_shader = (b->flags & 0xF) >= 0x2;
> +
> +        pandecode_log("struct midgard_blend_rt blend_rt_%d_%d = {\n", job_no, rt_no);
> +        pandecode_indent++;
> +
> +        pandecode_prop("flags = 0x%" PRIx64, b->flags);
> +
> +        mali_ptr shader = pandecode_midgard_blend(&b->blend, is_shader);
> +
> +        pandecode_indent--;
> +        pandecode_log("};\n");
> +
> +        return shader;
> +}
> +
>  static int
>  pandecode_replay_attribute_meta(int job_no, int count, const struct mali_vertex_tiler_postfix *v, bool varying, char *suffix)
>  {
> @@ -1248,95 +1311,35 @@ pandecode_replay_vertex_tiler_postfix_pre(const struct mali_vertex_tiler_postfix
>
>                  pandecode_prop("unknown2_8 = 0x%" PRIx32, s->unknown2_8);
>
> -                bool blend_shader = false;
> -
>                  if (!is_bifrost) {
> -                        if (s->unknown2_3 & MALI_HAS_BLEND_SHADER) {
> -                                blend_shader = true;
> -                                pandecode_replay_shader_address("blend_shader", s->blend_shader);
> -                        } else {
> -                                pandecode_replay_blend_equation(&s->blend_equation, "");
> -                        }
> +                        /* TODO: Blend shaders routing/disasm */
> +
> +                        pandecode_midgard_blend(&s->blend, false);
>                  }
>
>                  pandecode_indent--;
>                  pandecode_log("};\n");
>
> -                /* MRT blend fields are used whenever MFBD is used */
> +                /* MRT blend fields are used whenever MFBD is used, with
> +                 * per-RT descriptors */
>
>                  if (job_type == JOB_TYPE_TILER) {
> -                        pandecode_log("struct mali_blend_meta blend_meta_%d[] = {\n",
> -                                    job_no);
> -                        pandecode_indent++;
> -
> -                        int i;
> -
> -                        for (i = 0; i < 4; i++) {
> -                                const struct mali_blend_meta *b = &s->blend_meta[i];
> -                                pandecode_log("{\n");
> -                                pandecode_indent++;
> -
> -#ifndef BIFROST
> -                                pandecode_prop("unk1 = 0x%" PRIx64, b->unk1);
> -
> -                                /* Depending on unk1, we determine if there's a
> -                                 * blend shader */
> -
> -                                if ((b->unk1 & 0xF) >= 0x2) {
> -                                        blend_shader = true;
> -                                        pandecode_replay_shader_address("blend_shader", b->blend_shader);
> -                                } else {
> -                                        pandecode_replay_blend_equation(&b->blend_equation_1, "_1");
> -                                }
> -
> -                                /* This is always an equation, I think. If
> -                                 * there's a shader, it just defaults to
> -                                 * REPLACE (0x122) */
> -                                pandecode_replay_blend_equation(&b->blend_equation_2, "_2");
> -
> -                                if (b->zero2) {
> -                                        pandecode_msg("blend zero tripped\n");
> -                                        pandecode_prop("zero2 = 0x%x", b->zero2);
> -                                }
> +                        void* blend_base = (void *) (s + 1);
>
> -#else
> +                        for (unsigned i = 0; i < 4; i++) {
> +                                mali_ptr shader = 0;
>
> -                                pandecode_prop("unk1 = 0x%" PRIx32, b->unk1);
> -                                /* TODO figure out blend shader enable bit */
> -                                pandecode_replay_blend_equation(&b->blend_equation);
> -                                pandecode_prop("unk2 = 0x%" PRIx16, b->unk2);
> -                                pandecode_prop("index = 0x%" PRIx16, b->index);
> -                                pandecode_prop("unk3 = 0x%" PRIx32, b->unk3);
> -#endif
> -
> -                                pandecode_indent--;
> -                                pandecode_log("},\n");
> -
> -#ifdef BIFROST
> -                                if (b->unk2 == 3)
> -                                        break;
> -#else
> -                                /* TODO: What's this supposed to be? */
> -                                if (b->unk1 & 0x200)
> -                                        break;
> -#endif
> +                                if (is_bifrost)
> +                                        shader = pandecode_bifrost_blend(blend_base, job_no, i);
> +                                else
> +                                        shader = pandecode_midgard_blend_mrt(blend_base, job_no, i);
>
> +                                if (shader)
> +                                        pandecode_shader_disassemble(shader, job_no, job_type, false);
>                          }
> -
> -                        pandecode_indent--;
> -                        pandecode_log("};\n");
> -
> -                        /* This needs to be uploaded right after the
> -                         * shader_meta since it's technically part of the same
> -                         * (variable-size) structure.
> -                         */
>                  }
>
>                  pandecode_shader_disassemble(shader_ptr, job_no, job_type, is_bifrost);
> -
> -                if (!is_bifrost && blend_shader)
> -                        pandecode_shader_disassemble(s->blend_shader & ~0xF, job_no, job_type, false);
> -
>          } else
>                  pandecode_msg("<no shader>\n");
>
> --
> 2.20.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list