[Mesa-dev] [PATCH] nvc0: implement multisampled images on Maxwell+

[Karol Herbst]

great work, some comments below though.

On Wed, Jul 4, 2018 at 12:37 AM, Rhys Perry <pendingchaos02 at gmail.com> wrote:
> Signed-off-by: Rhys Perry <pendingchaos02 at gmail.com>
> ---
>  .../drivers/nouveau/codegen/nv50_ir_lowering_gm107.cpp   | 16 ++++++++++++++++
>  .../drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp    |  3 +++
>  .../drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h      |  2 +-
>  src/gallium/drivers/nouveau/nv50/nv50_resource.h         |  1 +
>  src/gallium/drivers/nouveau/nvc0/nvc0_screen.c           |  7 -------
>  src/gallium/drivers/nouveau/nvc0/nvc0_tex.c              |  4 ++--
>  6 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_gm107.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_gm107.cpp
> index 209f5c67ab..66a16c236e 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_gm107.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_gm107.cpp
> @@ -262,6 +262,9 @@ GM107LoweringPass::handlePOPCNT(Instruction *i)
>     return true;
>  }
>
> +// Taken from nv50_ir_lowering_nvc0.cpp
> +#define NVC0_SU_INFO_MS(i)   (0x38 + (i) * 4)
> +
>  bool
>  GM107LoweringPass::handleSUQ(TexInstruction *suq)
>  {
> @@ -315,6 +318,19 @@ GM107LoweringPass::handleSUQ(TexInstruction *suq)
>        samples->tex.query = TXQ_TYPE;
>     }
>
> +   if (suq->tex.target.isMS()) {
> +      bld.setPosition(suq, true);
> +
> +      if (mask & 0x1)
> +         bld.mkOp2(OP_SHR, TYPE_U32, suq->getDef(0), suq->getDef(0),
> +                   loadSuInfo32(ind, slot, NVC0_SU_INFO_MS(0), suq->tex.bindless));
> +      if (mask & 0x2) {
> +         int d = util_bitcount(mask & 0x1);
> +         bld.mkOp2(OP_SHR, TYPE_U32, suq->getDef(d), suq->getDef(d),
> +                   loadSuInfo32(ind, slot, NVC0_SU_INFO_MS(1), suq->tex.bindless));
> +      }
> +   }
> +
>     return true;
>  }
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> index a2e9fcb612..b53e4bc536 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
> @@ -1775,6 +1775,7 @@ NVC0LoweringPass::loadMsInfo32(Value *ptr, uint32_t off)
>
>  #define NVC0_SU_INFO_DIM(i)  (0x08 + (i) * 8)
>  #define NVC0_SU_INFO_SIZE(i) (0x20 + (i) * 4)
> +// Duplicated in nv50_ir_lowering_gm107.cpp
>  #define NVC0_SU_INFO_MS(i)   (0x38 + (i) * 4)

I think it would make more sense to move those up into the
nv50_ir_lowering_nvc0.h header file instead of duplicating.

>
>  inline Value *
> @@ -2410,6 +2411,8 @@ NVC0LoweringPass::processSurfaceCoordsGM107(TexInstruction *su)
>
>     bld.setPosition(su, false);
>
> +   adjustCoordinatesMS(su);
> +
>     // add texture handle
>     switch (su->op) {
>     case OP_SUSTP:
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
> index b650bbd219..23b23f89e4 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
> @@ -123,7 +123,7 @@ protected:
>
>     virtual bool visit(Instruction *);
>
> -private:
> +protected:

mhh, I think it makes more sense to simply move the methods into the
protected section we actually need

>     virtual bool visit(Function *);
>     virtual bool visit(BasicBlock *);
>
> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_resource.h b/src/gallium/drivers/nouveau/nv50/nv50_resource.h
> index 5d03925b0d..c64b045364 100644
> --- a/src/gallium/drivers/nouveau/nv50/nv50_resource.h
> +++ b/src/gallium/drivers/nouveau/nv50/nv50_resource.h
> @@ -66,6 +66,7 @@ nv50_miptree(struct pipe_resource *pt)
>  #define NV50_TEXVIEW_SCALED_COORDS     (1 << 0)
>  #define NV50_TEXVIEW_FILTER_MSAA8      (1 << 1)
>  #define NV50_TEXVIEW_ACCESS_RESOLVE    (1 << 2)
> +#define NV50_TEXVIEW_IMAGE_GM107       (1 << 3)
>
>
>  /* Internal functions:
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> index 6a941c3cdb..3c01b2eaf0 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> @@ -85,13 +85,6 @@ nvc0_screen_is_format_supported(struct pipe_screen *pscreen,
>                   PIPE_BIND_SHARED);
>
>     if (bindings & PIPE_BIND_SHADER_IMAGE) {
> -      if (sample_count > 0 &&
> -          nouveau_screen(pscreen)->class_3d >= GM107_3D_CLASS) {
> -         /* MS images are currently unsupported on Maxwell because they have to
> -          * be handled explicitly. */
> -         return false;
> -      }
> -
>        if (format == PIPE_FORMAT_B8G8R8A8_UNORM &&
>            nouveau_screen(pscreen)->class_3d < NVE4_3D_CLASS) {
>           /* This should work on Fermi, but for currently unknown reasons it
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_tex.c b/src/gallium/drivers/nouveau/nvc0/nvc0_tex.c
> index e7cd60169e..f40600e48a 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_tex.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_tex.c
> @@ -208,7 +208,7 @@ gm107_create_texture_view(struct pipe_context *pipe,
>               GM107_TIC2_3_LOD_ANISO_QUALITY_HIGH |
>               GM107_TIC2_3_LOD_ISO_QUALITY_HIGH;
>
> -   if (flags & NV50_TEXVIEW_ACCESS_RESOLVE) {
> +   if (flags & (NV50_TEXVIEW_ACCESS_RESOLVE | NV50_TEXVIEW_IMAGE_GM107)) {
>        width = mt->base.base.width0 << mt->ms_x;
>        height = mt->base.base.height0 << mt->ms_y;
>     } else {
> @@ -268,7 +268,7 @@ gm107_create_texture_view_from_image(struct pipe_context *pipe,
>        templ.u.tex.first_level = templ.u.tex.last_level = view->u.tex.level;
>     }
>
> -   flags = NV50_TEXVIEW_SCALED_COORDS;
> +   flags = NV50_TEXVIEW_SCALED_COORDS | NV50_TEXVIEW_IMAGE_GM107;
>
>     return nvc0_create_texture_view(pipe, &res->base, &templ, flags, target);
>  }
> --
> 2.14.4
>