[Mesa-dev] [PATCH v2 2/3] nir: Add a lowering pass for UYVY textures

[Lin, Johnson]

Thanks for the comments. I don't have a swap in other places in the patches. But I think your point is good, from the code that is a swap of U/V channels. I am suspecting there might be some bug somewhere else. Need to continue to investigate.

-----Original Message-----
From: hoegsberg at gmail.com [mailto:hoegsberg at gmail.com] On Behalf Of Kristian Høgsberg
Sent: Wednesday, June 21, 2017 12:50 AM
To: Lin, Johnson <johnson.lin at intel.com>
Cc: mesa-dev at lists.freedesktop.org
Subject: Re: [PATCH v2 2/3] nir: Add a lowering pass for UYVY textures

On Mon, Jun 19, 2017 at 7:07 PM, Lin, Johnson <johnson.lin at intel.com> wrote:
> Kristian,
>
> Thanks for the review comments. By my tests, UYVY buffer can be sampled and rendered correctly. So there is no swap of U/V channel here.

Just saying "the test pass, all is fine" isn't good enough. See below.

>
> -----Original Message-----
> From: Lin, Johnson
> Sent: Tuesday, June 20, 2017 9:43 AM
> To: mesa-dev at lists.freedesktop.org
> Cc: Lin, Johnson <johnson.lin at intel.com>
> Subject: [PATCH v2 2/3] nir: Add a lowering pass for UYVY textures
>
> Similar with support for YUYV but with byte order difference in 
> sampler
> ---
>  src/compiler/nir/nir.h           |  1 +
>  src/compiler/nir/nir_lower_tex.c | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index 
> ab7ba14303b7..1b4e47058d4d 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2449,6 +2449,7 @@ typedef struct nir_lower_tex_options {
>     unsigned lower_y_uv_external;
>     unsigned lower_y_u_v_external;
>     unsigned lower_yx_xuxv_external;
> +   unsigned lower_xy_uxvx_external;
>
>     /**
>      * To emulate certain texture wrap modes, this can be used diff 
> --git a/src/compiler/nir/nir_lower_tex.c 
> b/src/compiler/nir/nir_lower_tex.c
> index 4ef81955513e..5593f9890b28 100644
> --- a/src/compiler/nir/nir_lower_tex.c
> +++ b/src/compiler/nir/nir_lower_tex.c
> @@ -301,6 +301,18 @@ lower_yx_xuxv_external(nir_builder *b, nir_tex_instr *tex)
>                        nir_channel(b, xuxv, 3));  }
>
> +static void lower_xy_uxvx_external(nir_builder *b, nir_tex_instr 
> +*tex) {
> +  b->cursor = nir_after_instr(&tex->instr);
> +
> +  nir_ssa_def *y = sample_plane(b, tex, 0);  nir_ssa_def *uxvx = 
> + sample_plane(b, tex, 1);
> +
> +  convert_yuv_to_rgb(b, tex,
> +                     nir_channel(b, y, 1),
> +                     nir_channel(b, uxvx, 2),
> +                     nir_channel(b, uxvx, 0)); }

Let's look at convert_yuv_to_rgb:

static void
convert_yuv_to_rgb(nir_builder *b, nir_tex_instr *tex,
                   nir_ssa_def *y, nir_ssa_def *u, nir_ssa_def *v)

Notice how it takes the channels in y, u and v order. But you're passing in y, then nir_channel(b, uxvx, 2), which extracts the second channels, which is v and then nir_channel(b, uxvx, 0) which is u. If the tests pass you probably have another swap somewhere else in the patch that cancels out this problem. You need to fix this.

Kristian

> +
>  /*
>   * Emits a textureLod operation used to replace an existing
>   * textureGrad instruction.
> @@ -760,6 +772,10 @@ nir_lower_tex_block(nir_block *block, nir_builder *b,
>           progress = true;
>        }
>
> +      if ((1 << tex->texture_index) & options->lower_xy_uxvx_external) {
> +         lower_xy_uxvx_external(b, tex);
> +         progress = true;
> +      }
>
>        if (sat_mask) {
>           saturate_src(b, tex, sat_mask);
> --
> 1.9.1
>