i965/tex: ignore the diff between GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE

Submitted by andrey simiklit on July 10, 2018, 11:13 a.m.

Details

Message ID 1531221201-11360-1-git-send-email-andrii.simiklit@globallogic.com
State New
Headers show
Series "i965/tex: ignore the diff between GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit July 10, 2018, 11:13 a.m.
the difference between GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE
doesn't matter as far as the miptree is concerned;
genX(update_sampler_state) only looks at the
gl_texture_object and not the miptree when determining whether or
not to use normalized coordinates.

Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107117

---
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 7d1fa96..dc45a06 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -58,6 +58,12 @@  static void *intel_miptree_map_raw(struct brw_context *brw,
 
 static void intel_miptree_unmap_raw(struct intel_mipmap_tree *mt);
 
+static GLenum
+tex_rect_to_tex2d(GLenum val)
+{
+    return (GL_TEXTURE_RECTANGLE == val) ? GL_TEXTURE_2D : val;
+}
+
 static bool
 intel_miptree_supports_mcs(struct brw_context *brw,
                            const struct intel_mipmap_tree *mt)
@@ -1320,13 +1326,15 @@  intel_miptree_match_image(struct intel_mipmap_tree *mt,
 {
    struct intel_texture_image *intelImage = intel_texture_image(image);
    GLuint level = intelImage->base.Base.Level;
+   GLenum texObjTarget = tex_rect_to_tex2d(mt->target);
+   GLenum mipmapTreeTarget = tex_rect_to_tex2d(image->TexObject->Target);
    int width, height, depth;
 
    /* glTexImage* choose the texture object based on the target passed in, and
     * objects can't change targets over their lifetimes, so this should be
     * true.
     */
-   assert(image->TexObject->Target == mt->target);
+   assert(texObjTarget == mipmapTreeTarget);
 
    mesa_format mt_format = mt->format;
    if (mt->format == MESA_FORMAT_Z24_UNORM_X8_UINT && mt->stencil_mt)

Comments

On Tue, Jul 10, 2018 at 4:13 AM Andrii Simiklit <asimiklit.work@gmail.com>
wrote:

> the difference between GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE
> doesn't matter as far as the miptree is concerned;
> genX(update_sampler_state) only looks at the
> gl_texture_object and not the miptree when determining whether or
> not to use normalized coordinates.
>
> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107117
>
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 7d1fa96..dc45a06 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -58,6 +58,12 @@ static void *intel_miptree_map_raw(struct brw_context
> *brw,
>
>  static void intel_miptree_unmap_raw(struct intel_mipmap_tree *mt);
>
> +static GLenum
> +tex_rect_to_tex2d(GLenum val)
> +{
> +    return (GL_TEXTURE_RECTANGLE == val) ? GL_TEXTURE_2D : val;
> +}
> +
>  static bool
>  intel_miptree_supports_mcs(struct brw_context *brw,
>                             const struct intel_mipmap_tree *mt)
> @@ -1320,13 +1326,15 @@ intel_miptree_match_image(struct intel_mipmap_tree
> *mt,
>  {
>     struct intel_texture_image *intelImage = intel_texture_image(image);
>     GLuint level = intelImage->base.Base.Level;
> +   GLenum texObjTarget = tex_rect_to_tex2d(mt->target);
> +   GLenum mipmapTreeTarget = tex_rect_to_tex2d(image->TexObject->Target);
>     int width, height, depth;
>
>     /* glTexImage* choose the texture object based on the target passed
> in, and
>      * objects can't change targets over their lifetimes, so this should be
>      * true.
>      */
> -   assert(image->TexObject->Target == mt->target);
> +   assert(texObjTarget == mipmapTreeTarget);
>

If the only use of that helper is in an assert, just put the calls to the
helper inside the assert.  Otherwise, good job debugging. :-)

--Jason


>     mesa_format mt_format = mt->format;
>     if (mt->format == MESA_FORMAT_Z24_UNORM_X8_UINT && mt->stencil_mt)
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Ugh... not so good.  According to Oliver on the bug, this just make the
assert go away and doesn't actually fix anything.  Likely this is needed
but not sufficient.

--Jason

On Tue, Jul 10, 2018 at 8:17 AM Jason Ekstrand <jason@jlekstrand.net> wrote:

> On Tue, Jul 10, 2018 at 4:13 AM Andrii Simiklit <asimiklit.work@gmail.com>
> wrote:
>
>> the difference between GL_TEXTURE_2D and GL_TEXTURE_RECTANGLE
>> doesn't matter as far as the miptree is concerned;
>> genX(update_sampler_state) only looks at the
>> gl_texture_object and not the miptree when determining whether or
>> not to use normalized coordinates.
>>
>> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107117
>>
>> ---
>>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> index 7d1fa96..dc45a06 100644
>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> @@ -58,6 +58,12 @@ static void *intel_miptree_map_raw(struct brw_context
>> *brw,
>>
>>  static void intel_miptree_unmap_raw(struct intel_mipmap_tree *mt);
>>
>> +static GLenum
>> +tex_rect_to_tex2d(GLenum val)
>> +{
>> +    return (GL_TEXTURE_RECTANGLE == val) ? GL_TEXTURE_2D : val;
>> +}
>> +
>>  static bool
>>  intel_miptree_supports_mcs(struct brw_context *brw,
>>                             const struct intel_mipmap_tree *mt)
>> @@ -1320,13 +1326,15 @@ intel_miptree_match_image(struct
>> intel_mipmap_tree *mt,
>>  {
>>     struct intel_texture_image *intelImage = intel_texture_image(image);
>>     GLuint level = intelImage->base.Base.Level;
>> +   GLenum texObjTarget = tex_rect_to_tex2d(mt->target);
>> +   GLenum mipmapTreeTarget = tex_rect_to_tex2d(image->TexObject->Target);
>>     int width, height, depth;
>>
>>     /* glTexImage* choose the texture object based on the target passed
>> in, and
>>      * objects can't change targets over their lifetimes, so this should
>> be
>>      * true.
>>      */
>> -   assert(image->TexObject->Target == mt->target);
>> +   assert(texObjTarget == mipmapTreeTarget);
>>
>
> If the only use of that helper is in an assert, just put the calls to the
> helper inside the assert.  Otherwise, good job debugging. :-)
>
> --Jason
>
>
>>     mesa_format mt_format = mt->format;
>>     if (mt->format == MESA_FORMAT_Z24_UNORM_X8_UINT && mt->stencil_mt)
>> --
>> 2.7.4
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
Hi,

On Tue, 10 Jul 2018 at 18:56, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> Ugh... not so good.  According to Oliver on the bug, this just make the assert go away and doesn't actually fix anything.  Likely this is needed but not sufficient.

Well, maybe not even needed, at least in my case I don't hit that
assert() with the current Mesa code (from either master and 18.1),
I've just seen that happening with past commits while doing the
bisection in git... So adding this now wouldn't help with bisection
(again, in my case, not sure about others).

All I meant pointing at this assert() failure is that it breaks the
bisection in git as I am unable to tell if the rendering is correct or
not as it makes the test program abort.

Cheers,
Olivier
Hi,

> Ugh... not so good.  According to Oliver on the bug, this just make the
assert go away and doesn't actually fix anything.  Likely this is needed
but not sufficient.

So as far as I understand Oliver found the bad commit in xorg glamor:
https://bugs.freedesktop.org/show_bug.cgi?id=107287

So at the moment we should fix just this "assertion" issue for Intel
because "rendering" issue came from xorg/glamor and there is no "rendering"
issue in Intel part.
Please correct me if I incorrect.

Regards,
Andrii.

On Tue, Jul 10, 2018 at 8:04 PM, Olivier Fourdan <fourdan@gmail.com> wrote:

> Hi,
>
> On Tue, 10 Jul 2018 at 18:56, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > Ugh... not so good.  According to Oliver on the bug, this just make the
> assert go away and doesn't actually fix anything.  Likely this is needed
> but not sufficient.
>
> Well, maybe not even needed, at least in my case I don't hit that
> assert() with the current Mesa code (from either master and 18.1),
> I've just seen that happening with past commits while doing the
> bisection in git... So adding this now wouldn't help with bisection
> (again, in my case, not sure about others).
>
> All I meant pointing at this assert() failure is that it breaks the
> bisection in git as I am unable to tell if the rendering is correct or
> not as it makes the test program abort.
>
> Cheers,
> Olivier
>
Hi all,

On Thu, Jul 19, 2018 at 12:08 PM andrey simiklit
<asimiklit.work@gmail.com> wrote:
> > Ugh... not so good.  According to Oliver on the bug, this just make the assert go away and doesn't actually fix anything.  Likely this is needed but not sufficient.
>
> So as far as I understand Oliver found the bad commit in xorg glamor:
> https://bugs.freedesktop.org/show_bug.cgi?id=107287
>
> So at the moment we should fix just this "assertion" issue for Intel because "rendering" issue came from xorg/glamor and there is no "rendering" issue in Intel part.
> Please correct me if I incorrect.

Reviving an old thread/patch here.

Andrey, I reckon your patch here is still much needed as it fixes the
assert() issue:

intel_mipmap_tree.c:1301: intel_miptree_match_image: Assertion
`image->TexObject->Target == mt->target' failed.

Which is still occurring even with current master.

My patch was to fix the rendering issue (landed a while ago before
18.1 iirc), but yours was never merged and is still needed, I can
reproduce the assert() at will with the reproducer from
https://bugs.freedesktop.org/show_bug.cgi?id=107117

Jason, can we reconsider Andrii's patch? It still applies cleanly
(https://patchwork.freedesktop.org/patch/237490/)

Cheers,
Olivier

On 5/21/19 4:36 AM, Olivier Fourdan wrote:
> Hi all,
> 
> On Thu, Jul 19, 2018 at 12:08 PM andrey simiklit
> <asimiklit.work@gmail.com> wrote:
>>> Ugh... not so good.  According to Oliver on the bug, this just make the assert go away and doesn't actually fix anything.  Likely this is needed but not sufficient.
>>
>> So as far as I understand Oliver found the bad commit in xorg glamor:
>> https://bugs.freedesktop.org/show_bug.cgi?id=107287
>>
>> So at the moment we should fix just this "assertion" issue for Intel because "rendering" issue came from xorg/glamor and there is no "rendering" issue in Intel part.
>> Please correct me if I incorrect.
> 
> Reviving an old thread/patch here.
> 
> Andrey, I reckon your patch here is still much needed as it fixes the
> assert() issue:
> 
> intel_mipmap_tree.c:1301: intel_miptree_match_image: Assertion
> `image->TexObject->Target == mt->target' failed.
> 
> Which is still occurring even with current master.
> 
> My patch was to fix the rendering issue (landed a while ago before
> 18.1 iirc), but yours was never merged and is still needed, I can
> reproduce the assert() at will with the reproducer from
> https://bugs.freedesktop.org/show_bug.cgi?id=107117
> 
> Jason, can we reconsider Andrii's patch? It still applies cleanly
> (https://patchwork.freedesktop.org/patch/237490/)

Looking at the patch and the "Simple reproducer" in the bug, I think
this just papers over the issue.  It seems like the problem is somewhere
down inside the driver's handling of glXBindTexImageEXT.  My best guess
is that the texture is GL_TEXTURE_2D but the miptree backing it is
GL_TEXTURE_RECTANGLE.  It seems that the glXBindTexImageEXT handling
should mark the miptree as GL_TEXTURE_2D when binding the image to a
texture that is GL_TEXTURE_2D.  Or is that not possible for some
non-obvious reason?

> Cheers,
> Olivier
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Hi,

On Wed, May 22, 2019 at 4:01 AM Ian Romanick <idr@freedesktop.org> wrote:
> > On Thu, Jul 19, 2018 at 12:08 PM andrey simiklit
> >
> > Jason, can we reconsider Andrii's patch? It still applies cleanly
> > (https://patchwork.freedesktop.org/patch/237490/)
>
> Looking at the patch and the "Simple reproducer" in the bug, I think
> this just papers over the issue.  It seems like the problem is somewhere
> down inside the driver's handling of glXBindTexImageEXT.  My best guess
> is that the texture is GL_TEXTURE_2D but the miptree backing it is
> GL_TEXTURE_RECTANGLE.  It seems that the glXBindTexImageEXT handling
> should mark the miptree as GL_TEXTURE_2D when binding the image to a
> texture that is GL_TEXTURE_2D.  Or is that not possible for some
> non-obvious reason?

Sorry to be a pain, but I still get bug reports in xfce for this, for
everyone using a Mesa build with debug enabled (or a pre-release for
that matter) on Intel, the `assert()` are enabled and this bug occurs
with the xfce compositor.

Maybe Andrii's patch is just hiding the issue, but that's already the
case without the `assert()` enabled (i.e. all stable releases of
Mesa), so I guess it's not such a big deal anyway.

Can we agree on a fix on this?

Cheers,
Olivier