mesa: fix texStore for FORMAT_Z32_FLOAT_S8X24_UINT

Submitted by Marek Olšák on Aug. 15, 2019, 11:56 p.m.

Details

Message ID 20190815235634.4649-1-maraeo@gmail.com
State New
Headers show
Series "mesa: fix texStore for FORMAT_Z32_FLOAT_S8X24_UINT" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Marek Olšák Aug. 15, 2019, 11:56 p.m.
From: Jiadong Zhu <Jiadong.Zhu@amd.com>

_mesa_texstore_z32f_x24s8 calculates source rowStride at a
pace of 64-bit, this will make inaccuracy offset if the width
of src image is an odd number. Modify src pointer to int_32* as
source image format is gl_float which is 32-bit per pixel.

Signed-off-by: Jiadong Zhu <Jiadong.Zhu@amd.com>
Signed-off-by: Marek Olšák <marek.olsak@amd.com>
---
 src/mesa/main/texstore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
 mode change 100644 => 100755 src/mesa/main/texstore.c

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
old mode 100644
new mode 100755
index 2913d4bc067..207695041a7
--- a/src/mesa/main/texstore.c
+++ b/src/mesa/main/texstore.c
@@ -531,35 +531,35 @@  _mesa_texstore_s8(TEXSTORE_PARAMS)
    return GL_TRUE;
 }
 
 
 static GLboolean
 _mesa_texstore_z32f_x24s8(TEXSTORE_PARAMS)
 {
    GLint img, row;
    const GLint srcRowStride
       = _mesa_image_row_stride(srcPacking, srcWidth, srcFormat, srcType)
-      / sizeof(uint64_t);
+         / sizeof(int32_t);
 
    assert(dstFormat == MESA_FORMAT_Z32_FLOAT_S8X24_UINT);
    assert(srcFormat == GL_DEPTH_STENCIL ||
           srcFormat == GL_DEPTH_COMPONENT ||
           srcFormat == GL_STENCIL_INDEX);
    assert(srcFormat != GL_DEPTH_STENCIL ||
           srcType == GL_UNSIGNED_INT_24_8 ||
           srcType == GL_FLOAT_32_UNSIGNED_INT_24_8_REV);
 
    /* In case we only upload depth we need to preserve the stencil */
    for (img = 0; img < srcDepth; img++) {
       uint64_t *dstRow = (uint64_t *) dstSlices[img];
-      const uint64_t *src
-         = (const uint64_t *) _mesa_image_address(dims, srcPacking, srcAddr,
+      const int32_t *src
+         = (const int32_t *) _mesa_image_address(dims, srcPacking, srcAddr,
                srcWidth, srcHeight,
                srcFormat, srcType,
                img, 0, 0);
       for (row = 0; row < srcHeight; row++) {
          /* The unpack functions with:
           *    dstType = GL_FLOAT_32_UNSIGNED_INT_24_8_REV
           * only write their own dword, so the other dword (stencil
           * or depth) is preserved. */
          if (srcFormat != GL_STENCIL_INDEX)
             _mesa_unpack_depth_span(ctx, srcWidth,

Comments

Subtle. The source format *can* be 64-bit, by the way, but if it's
GL_DEPTH_COMPONENT it may well be 32-bit.

But what if it's GL_STENCIL_INDEX -- could it not be 1-byte? IOW,
should this just be a char *, and use byte addressing and be done with
it?

On Thu, Aug 15, 2019 at 7:56 PM Marek Olšák <maraeo@gmail.com> wrote:
>
> From: Jiadong Zhu <Jiadong.Zhu@amd.com>
>
> _mesa_texstore_z32f_x24s8 calculates source rowStride at a
> pace of 64-bit, this will make inaccuracy offset if the width
> of src image is an odd number. Modify src pointer to int_32* as
> source image format is gl_float which is 32-bit per pixel.
>
> Signed-off-by: Jiadong Zhu <Jiadong.Zhu@amd.com>
> Signed-off-by: Marek Olšák <marek.olsak@amd.com>
> ---
>  src/mesa/main/texstore.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>  mode change 100644 => 100755 src/mesa/main/texstore.c
>
> diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
> old mode 100644
> new mode 100755
> index 2913d4bc067..207695041a7
> --- a/src/mesa/main/texstore.c
> +++ b/src/mesa/main/texstore.c
> @@ -531,35 +531,35 @@ _mesa_texstore_s8(TEXSTORE_PARAMS)
>     return GL_TRUE;
>  }
>
>
>  static GLboolean
>  _mesa_texstore_z32f_x24s8(TEXSTORE_PARAMS)
>  {
>     GLint img, row;
>     const GLint srcRowStride
>        = _mesa_image_row_stride(srcPacking, srcWidth, srcFormat, srcType)
> -      / sizeof(uint64_t);
> +         / sizeof(int32_t);
>
>     assert(dstFormat == MESA_FORMAT_Z32_FLOAT_S8X24_UINT);
>     assert(srcFormat == GL_DEPTH_STENCIL ||
>            srcFormat == GL_DEPTH_COMPONENT ||
>            srcFormat == GL_STENCIL_INDEX);
>     assert(srcFormat != GL_DEPTH_STENCIL ||
>            srcType == GL_UNSIGNED_INT_24_8 ||
>            srcType == GL_FLOAT_32_UNSIGNED_INT_24_8_REV);
>
>     /* In case we only upload depth we need to preserve the stencil */
>     for (img = 0; img < srcDepth; img++) {
>        uint64_t *dstRow = (uint64_t *) dstSlices[img];
> -      const uint64_t *src
> -         = (const uint64_t *) _mesa_image_address(dims, srcPacking, srcAddr,
> +      const int32_t *src
> +         = (const int32_t *) _mesa_image_address(dims, srcPacking, srcAddr,
>                 srcWidth, srcHeight,
>                 srcFormat, srcType,
>                 img, 0, 0);
>        for (row = 0; row < srcHeight; row++) {
>           /* The unpack functions with:
>            *    dstType = GL_FLOAT_32_UNSIGNED_INT_24_8_REV
>            * only write their own dword, so the other dword (stencil
>            * or depth) is preserved. */
>           if (srcFormat != GL_STENCIL_INDEX)
>              _mesa_unpack_depth_span(ctx, srcWidth,
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

I don't think the original author was included -- CC was probably
stripped by the ML. Adding here.

On Tue, Aug 27, 2019 at 6:49 PM Marek Olšák <maraeo@gmail.com> wrote:
>
> Yes, but if the original author doesn't reply, I'd like to push this.
>
> Marek
>
> On Thu, Aug 15, 2019 at 8:01 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>
>> Subtle. The source format *can* be 64-bit, by the way, but if it's
>> GL_DEPTH_COMPONENT it may well be 32-bit.
>>
>> But what if it's GL_STENCIL_INDEX -- could it not be 1-byte? IOW,
>> should this just be a char *, and use byte addressing and be done with
>> it?
>>
>> On Thu, Aug 15, 2019 at 7:56 PM Marek Olšák <maraeo@gmail.com> wrote:
>> >
>> > From: Jiadong Zhu <Jiadong.Zhu@amd.com>
>> >
>> > _mesa_texstore_z32f_x24s8 calculates source rowStride at a
>> > pace of 64-bit, this will make inaccuracy offset if the width
>> > of src image is an odd number. Modify src pointer to int_32* as
>> > source image format is gl_float which is 32-bit per pixel.
>> >
>> > Signed-off-by: Jiadong Zhu <Jiadong.Zhu@amd.com>
>> > Signed-off-by: Marek Olšák <marek.olsak@amd.com>
>> > ---
>> >  src/mesa/main/texstore.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >  mode change 100644 => 100755 src/mesa/main/texstore.c
>> >
>> > diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
>> > old mode 100644
>> > new mode 100755
>> > index 2913d4bc067..207695041a7
>> > --- a/src/mesa/main/texstore.c
>> > +++ b/src/mesa/main/texstore.c
>> > @@ -531,35 +531,35 @@ _mesa_texstore_s8(TEXSTORE_PARAMS)
>> >     return GL_TRUE;
>> >  }
>> >
>> >
>> >  static GLboolean
>> >  _mesa_texstore_z32f_x24s8(TEXSTORE_PARAMS)
>> >  {
>> >     GLint img, row;
>> >     const GLint srcRowStride
>> >        = _mesa_image_row_stride(srcPacking, srcWidth, srcFormat, srcType)
>> > -      / sizeof(uint64_t);
>> > +         / sizeof(int32_t);
>> >
>> >     assert(dstFormat == MESA_FORMAT_Z32_FLOAT_S8X24_UINT);
>> >     assert(srcFormat == GL_DEPTH_STENCIL ||
>> >            srcFormat == GL_DEPTH_COMPONENT ||
>> >            srcFormat == GL_STENCIL_INDEX);
>> >     assert(srcFormat != GL_DEPTH_STENCIL ||
>> >            srcType == GL_UNSIGNED_INT_24_8 ||
>> >            srcType == GL_FLOAT_32_UNSIGNED_INT_24_8_REV);
>> >
>> >     /* In case we only upload depth we need to preserve the stencil */
>> >     for (img = 0; img < srcDepth; img++) {
>> >        uint64_t *dstRow = (uint64_t *) dstSlices[img];
>> > -      const uint64_t *src
>> > -         = (const uint64_t *) _mesa_image_address(dims, srcPacking, srcAddr,
>> > +      const int32_t *src
>> > +         = (const int32_t *) _mesa_image_address(dims, srcPacking, srcAddr,
>> >                 srcWidth, srcHeight,
>> >                 srcFormat, srcType,
>> >                 img, 0, 0);
>> >        for (row = 0; row < srcHeight; row++) {
>> >           /* The unpack functions with:
>> >            *    dstType = GL_FLOAT_32_UNSIGNED_INT_24_8_REV
>> >            * only write their own dword, so the other dword (stencil
>> >            * or depth) is preserved. */
>> >           if (srcFormat != GL_STENCIL_INDEX)
>> >              _mesa_unpack_depth_span(ctx, srcWidth,
>> > --
>> > 2.17.1
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
By the way, I took the liberty of composing a test which fails with
current mesa, and I'm pretty sure it continues to fail with this
patch. It does pass if I just make it a GLubyte like all the other
instances of the code already do.

Piglit test: https://patchwork.freedesktop.org/patch/327460/

On Tue, Aug 27, 2019 at 7:37 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
> I don't think the original author was included -- CC was probably
> stripped by the ML. Adding here.
>
> On Tue, Aug 27, 2019 at 6:49 PM Marek Olšák <maraeo@gmail.com> wrote:
> >
> > Yes, but if the original author doesn't reply, I'd like to push this.
> >
> > Marek
> >
> > On Thu, Aug 15, 2019 at 8:01 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> >>
> >> Subtle. The source format *can* be 64-bit, by the way, but if it's
> >> GL_DEPTH_COMPONENT it may well be 32-bit.
> >>
> >> But what if it's GL_STENCIL_INDEX -- could it not be 1-byte? IOW,
> >> should this just be a char *, and use byte addressing and be done with
> >> it?
> >>
> >> On Thu, Aug 15, 2019 at 7:56 PM Marek Olšák <maraeo@gmail.com> wrote:
> >> >
> >> > From: Jiadong Zhu <Jiadong.Zhu@amd.com>
> >> >
> >> > _mesa_texstore_z32f_x24s8 calculates source rowStride at a
> >> > pace of 64-bit, this will make inaccuracy offset if the width
> >> > of src image is an odd number. Modify src pointer to int_32* as
> >> > source image format is gl_float which is 32-bit per pixel.
> >> >
> >> > Signed-off-by: Jiadong Zhu <Jiadong.Zhu@amd.com>
> >> > Signed-off-by: Marek Olšák <marek.olsak@amd.com>
> >> > ---
> >> >  src/mesa/main/texstore.c | 6 +++---
> >> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >> >  mode change 100644 => 100755 src/mesa/main/texstore.c
> >> >
> >> > diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
> >> > old mode 100644
> >> > new mode 100755
> >> > index 2913d4bc067..207695041a7
> >> > --- a/src/mesa/main/texstore.c
> >> > +++ b/src/mesa/main/texstore.c
> >> > @@ -531,35 +531,35 @@ _mesa_texstore_s8(TEXSTORE_PARAMS)
> >> >     return GL_TRUE;
> >> >  }
> >> >
> >> >
> >> >  static GLboolean
> >> >  _mesa_texstore_z32f_x24s8(TEXSTORE_PARAMS)
> >> >  {
> >> >     GLint img, row;
> >> >     const GLint srcRowStride
> >> >        = _mesa_image_row_stride(srcPacking, srcWidth, srcFormat, srcType)
> >> > -      / sizeof(uint64_t);
> >> > +         / sizeof(int32_t);
> >> >
> >> >     assert(dstFormat == MESA_FORMAT_Z32_FLOAT_S8X24_UINT);
> >> >     assert(srcFormat == GL_DEPTH_STENCIL ||
> >> >            srcFormat == GL_DEPTH_COMPONENT ||
> >> >            srcFormat == GL_STENCIL_INDEX);
> >> >     assert(srcFormat != GL_DEPTH_STENCIL ||
> >> >            srcType == GL_UNSIGNED_INT_24_8 ||
> >> >            srcType == GL_FLOAT_32_UNSIGNED_INT_24_8_REV);
> >> >
> >> >     /* In case we only upload depth we need to preserve the stencil */
> >> >     for (img = 0; img < srcDepth; img++) {
> >> >        uint64_t *dstRow = (uint64_t *) dstSlices[img];
> >> > -      const uint64_t *src
> >> > -         = (const uint64_t *) _mesa_image_address(dims, srcPacking, srcAddr,
> >> > +      const int32_t *src
> >> > +         = (const int32_t *) _mesa_image_address(dims, srcPacking, srcAddr,
> >> >                 srcWidth, srcHeight,
> >> >                 srcFormat, srcType,
> >> >                 img, 0, 0);
> >> >        for (row = 0; row < srcHeight; row++) {
> >> >           /* The unpack functions with:
> >> >            *    dstType = GL_FLOAT_32_UNSIGNED_INT_24_8_REV
> >> >            * only write their own dword, so the other dword (stencil
> >> >            * or depth) is preserved. */
> >> >           if (srcFormat != GL_STENCIL_INDEX)
> >> >              _mesa_unpack_depth_span(ctx, srcWidth,
> >> > --
> >> > 2.17.1
> >> >
> >> > _______________________________________________
> >> > mesa-dev mailing list
> >> > mesa-dev@lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Yep, it does work out ... you get lucky :) It does seem like it'd be
really easy to just switch to GLubyte like all the other functions do,
but whatever.

On Thu, Sep 12, 2019 at 10:22 PM Marek Olšák <maraeo@gmail.com> wrote:
>
> Ilia, the patch is OK if GL_STENCIL_INDEX is not allowed, right?
>
> Marek
>
> On Tue, Aug 27, 2019 at 9:24 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>
>> By the way, I took the liberty of composing a test which fails with
>> current mesa, and I'm pretty sure it continues to fail with this
>> patch. It does pass if I just make it a GLubyte like all the other
>> instances of the code already do.
>>
>> Piglit test: https://patchwork.freedesktop.org/patch/327460/
>>
>> On Tue, Aug 27, 2019 at 7:37 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> >
>> > I don't think the original author was included -- CC was probably
>> > stripped by the ML. Adding here.
>> >
>> > On Tue, Aug 27, 2019 at 6:49 PM Marek Olšák <maraeo@gmail.com> wrote:
>> > >
>> > > Yes, but if the original author doesn't reply, I'd like to push this.
>> > >
>> > > Marek
>> > >
>> > > On Thu, Aug 15, 2019 at 8:01 PM Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>> > >>
>> > >> Subtle. The source format *can* be 64-bit, by the way, but if it's
>> > >> GL_DEPTH_COMPONENT it may well be 32-bit.
>> > >>
>> > >> But what if it's GL_STENCIL_INDEX -- could it not be 1-byte? IOW,
>> > >> should this just be a char *, and use byte addressing and be done with
>> > >> it?
>> > >>
>> > >> On Thu, Aug 15, 2019 at 7:56 PM Marek Olšák <maraeo@gmail.com> wrote:
>> > >> >
>> > >> > From: Jiadong Zhu <Jiadong.Zhu@amd.com>
>> > >> >
>> > >> > _mesa_texstore_z32f_x24s8 calculates source rowStride at a
>> > >> > pace of 64-bit, this will make inaccuracy offset if the width
>> > >> > of src image is an odd number. Modify src pointer to int_32* as
>> > >> > source image format is gl_float which is 32-bit per pixel.
>> > >> >
>> > >> > Signed-off-by: Jiadong Zhu <Jiadong.Zhu@amd.com>
>> > >> > Signed-off-by: Marek Olšák <marek.olsak@amd.com>
>> > >> > ---
>> > >> >  src/mesa/main/texstore.c | 6 +++---
>> > >> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > >> >  mode change 100644 => 100755 src/mesa/main/texstore.c
>> > >> >
>> > >> > diff --git a/src/mesa/main/texstore.c b/src/mesa/main/texstore.c
>> > >> > old mode 100644
>> > >> > new mode 100755
>> > >> > index 2913d4bc067..207695041a7
>> > >> > --- a/src/mesa/main/texstore.c
>> > >> > +++ b/src/mesa/main/texstore.c
>> > >> > @@ -531,35 +531,35 @@ _mesa_texstore_s8(TEXSTORE_PARAMS)
>> > >> >     return GL_TRUE;
>> > >> >  }
>> > >> >
>> > >> >
>> > >> >  static GLboolean
>> > >> >  _mesa_texstore_z32f_x24s8(TEXSTORE_PARAMS)
>> > >> >  {
>> > >> >     GLint img, row;
>> > >> >     const GLint srcRowStride
>> > >> >        = _mesa_image_row_stride(srcPacking, srcWidth, srcFormat, srcType)
>> > >> > -      / sizeof(uint64_t);
>> > >> > +         / sizeof(int32_t);
>> > >> >
>> > >> >     assert(dstFormat == MESA_FORMAT_Z32_FLOAT_S8X24_UINT);
>> > >> >     assert(srcFormat == GL_DEPTH_STENCIL ||
>> > >> >            srcFormat == GL_DEPTH_COMPONENT ||
>> > >> >            srcFormat == GL_STENCIL_INDEX);
>> > >> >     assert(srcFormat != GL_DEPTH_STENCIL ||
>> > >> >            srcType == GL_UNSIGNED_INT_24_8 ||
>> > >> >            srcType == GL_FLOAT_32_UNSIGNED_INT_24_8_REV);
>> > >> >
>> > >> >     /* In case we only upload depth we need to preserve the stencil */
>> > >> >     for (img = 0; img < srcDepth; img++) {
>> > >> >        uint64_t *dstRow = (uint64_t *) dstSlices[img];
>> > >> > -      const uint64_t *src
>> > >> > -         = (const uint64_t *) _mesa_image_address(dims, srcPacking, srcAddr,
>> > >> > +      const int32_t *src
>> > >> > +         = (const int32_t *) _mesa_image_address(dims, srcPacking, srcAddr,
>> > >> >                 srcWidth, srcHeight,
>> > >> >                 srcFormat, srcType,
>> > >> >                 img, 0, 0);
>> > >> >        for (row = 0; row < srcHeight; row++) {
>> > >> >           /* The unpack functions with:
>> > >> >            *    dstType = GL_FLOAT_32_UNSIGNED_INT_24_8_REV
>> > >> >            * only write their own dword, so the other dword (stencil
>> > >> >            * or depth) is preserved. */
>> > >> >           if (srcFormat != GL_STENCIL_INDEX)
>> > >> >              _mesa_unpack_depth_span(ctx, srcWidth,
>> > >> > --
>> > >> > 2.17.1
>> > >> >
>> > >> > _______________________________________________
>> > >> > mesa-dev mailing list
>> > >> > mesa-dev@lists.freedesktop.org
>> > >> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev