[Lima] [PATCH v2 6/8] gallium: add lima driver

[Qiang Yu]

On Tue, Mar 26, 2019 at 11:33 AM Alyssa Rosenzweig <alyssa at rosenzweig.io> wrote:
>
> > +   [PPIR_INSTR_SLOT_ALU_VEC_MUL] = ppir_codegen_encode_vec_mul,
> > +   [PPIR_INSTR_SLOT_ALU_SCL_MUL] = ppir_codegen_encode_scl_mul,
> > +   [PPIR_INSTR_SLOT_ALU_VEC_ADD] = ppir_codegen_encode_vec_add,
> > +   [PPIR_INSTR_SLOT_ALU_SCL_ADD] = ppir_codegen_encode_scl_add,
>
> vmul/smul/vadd/sadd are the (official?) names for this on Midgard, fwiw.
>
> > +static const int ppir_codegen_field_size[] = {
> > +   34, 62, 41, 43, 30, 44, 31, 30, 41, 73
> > +};
>
> What is this?
PP instruction field (i.e. vadd part length) length in bit.

>
> > +static inline int align_to_word(int size)
> > +{
> > +   return ((size + 0x1f) >> 5);
> > +}
>
> Maybe use the align() macro (it's in mesa/src/util/) for clarity?
>
> > +   ppir_codegen_combine_scalar_op_atan  = 8, /* Arc Tangent Part 1 */
> > +   ppir_codegen_combine_scalar_op_atan2 = 9, /* Arc Tangent 2 Part 1 */
>
> Any plan to use these ops? Midgard has them too, but inverse trig gets
> lowered away anyway...
>
Not yet.

> > +            if (node->op == ppir_op_neg)
> > +               src->negate = !src->negate;
> > +            else if (node->op == ppir_op_abs)
> > +               src->absolute = true;
>
> I'm confused. NIR has neg/abs modifiers natively. You can get all
> fneg/fabs instructions lowered in NIR to these modifiers via
> the lower_source_mods pass. If you don't want them, you'll never have
> neg/abs ops at all; they'll all be modifiers. Why duplicate this in
> ppir?

Didn't know this pass, I can use it, thanks.

>
> > +   uint32_t va;
>
> I don't know the Utgard MMU, but will this work on 64-bit?
>
It's fine as Utgard MMU only support 32bit address.

>
> > +   uint32_t dubya;
>
> ???
Don't know for me either, just copy from lima-ng.

>
> > +   uint32_t mrt_bits;
> > +   uint32_t mrt_pitch;
>
> Utgard supports MRT?!
>
Utgard can render to 3 FB (WB0~2) in one pass.

>
> > +   uint32_t width = fui(fb->width);
> > +   uint32_t height = fui(fb->height);
> > +   uint32_t reload_gl_pos[] = {
> > +      width,      0x00000000, 0x00000000, 0x3f800000, /* 0x00000000 */
> > +      0x00000000, 0x00000000, 0x00000000, 0x3f800000, /* 0x00000010 */
> > +      0x00000000, height,     0x00000000, 0x3f800000, /* 0x00000020 */
> > +   };
> > +   memcpy(cpu + lima_reload_gl_pos_offset, reload_gl_pos,
> > +          sizeof(reload_gl_pos));
> > +
> > +   uint32_t reload_varying[] = {
> > +      width,      0x00000000, 0x00000000, 0x00000000, /* 0x00000000 */
> > +      0x00000000, height,     0x00000000, 0x00000000, /* 0x00000010 */
> > +   };
> > +   memcpy(cpu + lima_reload_varying_offset, reload_varying,
> > +          sizeof(reload_varying));
> > +
>
> Why not actual float arrays?
>
Right.

>
> > +   uint32_t clear_shader[] = {
> > +      0x00021025, 0x0000000c,
> > +      (ctx->clear.color_16pc << 12) | 0x000007cf,
> > +      ctx->clear.color_16pc >> 12,
> > +      ctx->clear.color_16pc >> 44,
> > +   };
>
> Please don't include shader blobs. This should maybe be generated
> from NIR, but at minimum, please include the corresponding annotated
> assembly.
>
I think no need to compile this kind of util shader, will add some
comments about it.

> > +      clear->color_16pc =
> > +         ((uint64_t)float_to_ushort(color->f[3]) << 48) |
> > +         ((uint64_t)float_to_ushort(color->f[2]) << 32) |
> > +         ((uint64_t)float_to_ushort(color->f[1]) << 16) |
> > +         float_to_ubyte(color->f[0]);
>
> Should that last line be float_to_ushort?
>
Right, will fix it.

>
> > +enum lima_attrib_type {
> > +   LIMA_ATTRIB_FLOAT = 0x000,
> > +   /* todo: find out what lives here. */
>
> LIMA_ATTRIB_FP16 maybe.
> LIMA_ATTRIB_I32/LIMA_ATTRIB_U32?.
>
>
> > +static bool
> > +is_modifier_ok(const uint64_t *modifiers, int count, uint64_t mod)
>
> drm_find_modifier in util/u_drm.h
>
> > +   /* fs program for clear buffer? */
> > +   static const uint32_t pp_clear_program[] = {
> > +      0x00020425, 0x0000000c, 0x01e007cf, 0xb0000000, /* 0x00000000 */
> > +      0x000005f5, 0x00000000, 0x00000000, 0x00000000, /* 0x00000010 */
> > +   };
> > +   memcpy(lima_bo_map(screen->pp_buffer) + pp_clear_program_offset,
> > +          pp_clear_program, sizeof(pp_clear_program));
> > +
> > +   /* copy texture to framebuffer, used to reload gpu tile buffer */
> > +   static const uint32_t pp_reload_program[] = {
> > +      0x000005e6, 0xf1003c20, 0x00000000, 0x39001000, /* 0x00000000 */
> > +      0x00000e4e, 0x000007cf, 0x00000000, 0x00000000, /* 0x00000010 */
> > +   };
> > +   memcpy(lima_bo_map(screen->pp_buffer) + pp_reload_program_offset,
> > +          pp_reload_program, sizeof(pp_reload_program));
> > +
> > +   /* 0/1/2 vertex index for reload/clear draw */
> > +   static const uint32_t pp_shared_index[] = {
> > +      0x00020100, 0x00000000, 0x00000000, 0x00000000, /* 0x00000000 */
> > +   };
> > +   memcpy(lima_bo_map(screen->pp_buffer) + pp_shared_index_offset,
> > +          pp_shared_index, sizeof(pp_shared_index));
> > +
> > +   /* 4096x4096 gl pos used for partial clear */
> > +   static const uint32_t pp_clear_gl_pos[] = {
> > +      0x45800000, 0x00000000, 0x3f800000, 0x3f800000, /* 0x00000000 */
> > +      0x00000000, 0x00000000, 0x3f800000, 0x3f800000, /* 0x00000010 */
> > +      0x00000000, 0x45800000, 0x3f800000, 0x3f800000, /* 0x00000020 */
> > +   };
> > +   memcpy(lima_bo_map(screen->pp_buffer) + pp_clear_gl_pos_offset,
> > +          pp_clear_gl_pos, sizeof(pp_clear_gl_pos));
>
> Again, please no shader blobs.
>
> > +
> > +   /* reverse calculate the parameter of glViewport */
> > +   ctx->viewport.x = viewport->translate[0] - viewport->scale[0];
> > +   ctx->viewport.y = fabsf(viewport->translate[1] - fabsf(viewport->scale[1]));
> > +   ctx->viewport.width = viewport->scale[0] * 2;
> > +   ctx->viewport.height = fabsf(viewport->scale[1] * 2);
>
> Careful about flipped-y viewports...?
>
Right.

> > +++ b/src/gallium/drivers/lima/lima_tiling.c
>
> We'll maybe want to share this routine somewhere... Also, what's your
> plan for autogenerating mipmaps?

Arno did the mipmap implementation, so far no further work from him.