[1/2] blorp: Don't try to use R32_UNORM for R24_UNORM_X8_TYPELESS rendering.

Submitted by Kenneth Graunke on Aug. 10, 2018, 9:18 a.m.

Details

Message ID 20180810091841.15164-1-kenneth@whitecape.org
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Kenneth Graunke Aug. 10, 2018, 9:18 a.m.
The hardware doesn't support rendering to R24_UNORM_X8_TYPELESS, so
Jason decided to fake it with a bit of shader math and R32_UNORM RTs.

The only problem is that R32_UNORM isn't renderable either...so we've
just traded one bad format for another.

This patch makes us use R32_UINT instead.
---
 src/intel/blorp/blorp_blit.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
index c85ec8543a9..7b49f9afa35 100644
--- a/src/intel/blorp/blorp_blit.c
+++ b/src/intel/blorp/blorp_blit.c
@@ -984,14 +984,15 @@  convert_color(struct nir_builder *b, nir_ssa_def *color,
    nir_ssa_def *value;
 
    if (key->dst_format == ISL_FORMAT_R24_UNORM_X8_TYPELESS) {
-      /* The destination image is bound as R32_UNORM but the data needs to be
+      /* The destination image is bound as R32_UINT but the data needs to be
        * in R24_UNORM_X8_TYPELESS.  The bottom 24 are the actual data and the
        * top 8 need to be zero.  We can accomplish this by simply multiplying
        * by a factor to scale things down.
        */
-      float factor = (float)((1 << 24) - 1) / (float)UINT32_MAX;
-      value = nir_fmul(b, nir_fsat(b, nir_channel(b, color, 0)),
-                          nir_imm_float(b, factor));
+      unsigned factor = (1 << 24) - 1;
+      value = nir_fsat(b, nir_channel(b, color, 0));
+      value = nir_fmul(b, value, nir_imm_float(b, factor));
+      value = nir_iand(b, nir_f2i32(b, value), nir_imm_int(b, factor));
    } else if (key->dst_format == ISL_FORMAT_L8_UNORM_SRGB) {
       value = nir_format_linear_to_srgb(b, nir_channel(b, color, 0));
    } else if (key->dst_format == ISL_FORMAT_R8G8B8_UNORM_SRGB) {
@@ -1976,7 +1977,7 @@  try_blorp_blit(struct blorp_batch *batch,
          isl_format_rgbx_to_rgba(params->dst.view.format);
    } else if (params->dst.view.format == ISL_FORMAT_R24_UNORM_X8_TYPELESS) {
       wm_prog_key->dst_format = params->dst.view.format;
-      params->dst.view.format = ISL_FORMAT_R32_UNORM;
+      params->dst.view.format = ISL_FORMAT_R32_UINT;
    } else if (params->dst.view.format == ISL_FORMAT_A4B4G4R4_UNORM) {
       params->dst.view.swizzle =
          isl_swizzle_compose(params->dst.view.swizzle,

Comments

On August 10, 2018 03:19:18 Kenneth Graunke <kenneth@whitecape.org> wrote:

> The hardware doesn't support rendering to R24_UNORM_X8_TYPELESS, so
> Jason decided to fake it with a bit of shader math and R32_UNORM RTs.
>
> The only problem is that R32_UNORM isn't renderable either...so we've
> just traded one bad format for another.
>
> This patch makes us use R32_UINT instead.
> ---
> src/intel/blorp/blorp_blit.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
> index c85ec8543a9..7b49f9afa35 100644
> --- a/src/intel/blorp/blorp_blit.c
> +++ b/src/intel/blorp/blorp_blit.c
> @@ -984,14 +984,15 @@ convert_color(struct nir_builder *b, nir_ssa_def *color,
>    nir_ssa_def *value;
>
>    if (key->dst_format == ISL_FORMAT_R24_UNORM_X8_TYPELESS) {
> -      /* The destination image is bound as R32_UNORM but the data needs to be
> +      /* The destination image is bound as R32_UINT but the data needs to be
>        * in R24_UNORM_X8_TYPELESS.  The bottom 24 are the actual data and the
>        * top 8 need to be zero.  We can accomplish this by simply multiplying
>        * by a factor to scale things down.
>        */
> -      float factor = (float)((1 << 24) - 1) / (float)UINT32_MAX;
> -      value = nir_fmul(b, nir_fsat(b, nir_channel(b, color, 0)),
> -                          nir_imm_float(b, factor));
> +      unsigned factor = (1 << 24) - 1;
> +      value = nir_fsat(b, nir_channel(b, color, 0));
> +      value = nir_fmul(b, value, nir_imm_float(b, factor));
> +      value = nir_iand(b, nir_f2i32(b, value), nir_imm_int(b, factor));

Do we really need the and?

>    } else if (key->dst_format == ISL_FORMAT_L8_UNORM_SRGB) {
>       value = nir_format_linear_to_srgb(b, nir_channel(b, color, 0));
>    } else if (key->dst_format == ISL_FORMAT_R8G8B8_UNORM_SRGB) {
> @@ -1976,7 +1977,7 @@ try_blorp_blit(struct blorp_batch *batch,
>          isl_format_rgbx_to_rgba(params->dst.view.format);
>    } else if (params->dst.view.format == ISL_FORMAT_R24_UNORM_X8_TYPELESS) {
>       wm_prog_key->dst_format = params->dst.view.format;
> -      params->dst.view.format = ISL_FORMAT_R32_UNORM;
> +      params->dst.view.format = ISL_FORMAT_R32_UINT;
>    } else if (params->dst.view.format == ISL_FORMAT_A4B4G4R4_UNORM) {
>       params->dst.view.swizzle =
>          isl_swizzle_compose(params->dst.view.swizzle,
> --
> 2.18.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Friday, August 10, 2018 8:34:12 AM PDT Jason Ekstrand wrote:
> On August 10, 2018 03:19:18 Kenneth Graunke <kenneth@whitecape.org> wrote:
> 
> > The hardware doesn't support rendering to R24_UNORM_X8_TYPELESS, so
> > Jason decided to fake it with a bit of shader math and R32_UNORM RTs.
> >
> > The only problem is that R32_UNORM isn't renderable either...so we've
> > just traded one bad format for another.
> >
> > This patch makes us use R32_UINT instead.
> > ---
> > src/intel/blorp/blorp_blit.c | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
> > index c85ec8543a9..7b49f9afa35 100644
> > --- a/src/intel/blorp/blorp_blit.c
> > +++ b/src/intel/blorp/blorp_blit.c
> > @@ -984,14 +984,15 @@ convert_color(struct nir_builder *b, nir_ssa_def *color,
> >    nir_ssa_def *value;
> >
> >    if (key->dst_format == ISL_FORMAT_R24_UNORM_X8_TYPELESS) {
> > -      /* The destination image is bound as R32_UNORM but the data needs to be
> > +      /* The destination image is bound as R32_UINT but the data needs to be
> >        * in R24_UNORM_X8_TYPELESS.  The bottom 24 are the actual data and the
> >        * top 8 need to be zero.  We can accomplish this by simply multiplying
> >        * by a factor to scale things down.
> >        */
> > -      float factor = (float)((1 << 24) - 1) / (float)UINT32_MAX;
> > -      value = nir_fmul(b, nir_fsat(b, nir_channel(b, color, 0)),
> > -                          nir_imm_float(b, factor));
> > +      unsigned factor = (1 << 24) - 1;
> > +      value = nir_fsat(b, nir_channel(b, color, 0));
> > +      value = nir_fmul(b, value, nir_imm_float(b, factor));
> > +      value = nir_iand(b, nir_f2i32(b, value), nir_imm_int(b, factor));
> 
> Do we really need the and?

Probably not - without it, we'll just leave garbage in the X8 bits
instead of zeroing them.  But that's probably fine, unless it affects
HiZ compression somehow?  I was mostly borrowing from u_format_zs.c
code to do this without thinking too hard.
On August 10, 2018 13:25:04 Kenneth Graunke <kenneth@whitecape.org> wrote:

> On Friday, August 10, 2018 8:34:12 AM PDT Jason Ekstrand wrote:
>> On August 10, 2018 03:19:18 Kenneth Graunke <kenneth@whitecape.org> wrote:
>>
>>> The hardware doesn't support rendering to R24_UNORM_X8_TYPELESS, so
>>> Jason decided to fake it with a bit of shader math and R32_UNORM RTs.
>>>
>>> The only problem is that R32_UNORM isn't renderable either...so we've
>>> just traded one bad format for another.
>>>
>>> This patch makes us use R32_UINT instead.
>>> ---
>>> src/intel/blorp/blorp_blit.c | 11 ++++++-----
>>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
>>> index c85ec8543a9..7b49f9afa35 100644
>>> --- a/src/intel/blorp/blorp_blit.c
>>> +++ b/src/intel/blorp/blorp_blit.c
>>> @@ -984,14 +984,15 @@ convert_color(struct nir_builder *b, nir_ssa_def *color,
>>> nir_ssa_def *value;
>>>
>>> if (key->dst_format == ISL_FORMAT_R24_UNORM_X8_TYPELESS) {
>>> -      /* The destination image is bound as R32_UNORM but the data needs to be
>>> +      /* The destination image is bound as R32_UINT but the data needs to be
>>> * in R24_UNORM_X8_TYPELESS.  The bottom 24 are the actual data and the
>>> * top 8 need to be zero.  We can accomplish this by simply multiplying
>>> * by a factor to scale things down.
>>> */
>>> -      float factor = (float)((1 << 24) - 1) / (float)UINT32_MAX;
>>> -      value = nir_fmul(b, nir_fsat(b, nir_channel(b, color, 0)),
>>> -                          nir_imm_float(b, factor));
>>> +      unsigned factor = (1 << 24) - 1;
>>> +      value = nir_fsat(b, nir_channel(b, color, 0));
>>> +      value = nir_fmul(b, value, nir_imm_float(b, factor));
>>> +      value = nir_iand(b, nir_f2i32(b, value), nir_imm_int(b, factor));
>>
>> Do we really need the and?
>
> Probably not - without it, we'll just leave garbage in the X8 bits
> instead of zeroing them.  But that's probably fine, unless it affects
> HiZ compression somehow?  I was mostly borrowing from u_format_zs.c
> code to do this without thinking too hard.

That would only matter of mul(sat(x), factor) produces something outside 
the range [0, factor+1). I really don't think it's doing anything useful.

--Jason