Re: [Mesa-dev] [PATCH] i965/gen5+: Fix incorrect miptree layout for non-power-of-two cubemaps.

2011-08-01 Thread Eric Anholt
On Mon, 01 Aug 2011 13:15:45 -0700, Kenneth Graunke  
wrote:
> On 07/31/2011 07:25 PM, Eric Anholt wrote:
> > On Sat, 30 Jul 2011 21:35:52 -0700, Kenneth Graunke  
> > wrote:
> >> For power-of-two sizes, h0 == mt->height0 since it's already a multiple
> >> of two.  However, for NPOT, they're different; h1 should be computed
> >> based on the original size.
> >>
> >> Fixes piglit test "cubemap npot" and oglconform_31 test "textureNPOT".
> >>
> >> NOTE: This is a candidate for stable release branches.
> >>
> >> Signed-off-by: Kenneth Graunke 
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_tex_layout.c |2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> Note that the piglit test referenced isn't committed yet; I sent it to the
> >> piglit mailing list.
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c 
> >> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> index f462f32..46a417a 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> @@ -60,7 +60,7 @@ GLboolean brw_miptree_layout(struct intel_context *intel,
> >>   * given in Volume 1 of the BSpec.
> >>   */
> >>  h0 = ALIGN(mt->height0, align_h);
> >> -h1 = ALIGN(minify(h0), align_h);
> >> +h1 = ALIGN(minify(mt->height0), align_h);
> >>  qpitch = (h0 + h1 + (intel->gen >= 7 ? 12 : 11) * align_h);
> >>if (mt->compressed)
> >> qpitch /= 4;
> > 
> > 
> > This looks wrong to me.  The height of a level L is ALIGN(height0 >> L,
> > j) according to SNB PRM vol1, 6.17.3.1 "Computing MIP level sizes".
> > Note that our calculation of j is wrong for a bunch of hardware/format
> > combos -- I wonder if that was the issue you were looking at?
> 
> Just for the record, I talked with Eric about this in person, and he
> agreed that my patch is correct.  mt->height0 is H_0 (actual height) in
> the PRM, while h0 is h_0 (aligned).  We were using the wrong one.  Also,
> for the case I was looking at, j == 2, so that wasn't the issue.

More to the point, I was reading the patch as exactly reversed of what
it did.  So, I think that's a Reviewed-by :)


pgp76RveSgnmE.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] i965/gen5+: Fix incorrect miptree layout for non-power-of-two cubemaps.

2011-08-01 Thread Kenneth Graunke
On 07/31/2011 07:25 PM, Eric Anholt wrote:
> On Sat, 30 Jul 2011 21:35:52 -0700, Kenneth Graunke  
> wrote:
>> For power-of-two sizes, h0 == mt->height0 since it's already a multiple
>> of two.  However, for NPOT, they're different; h1 should be computed
>> based on the original size.
>>
>> Fixes piglit test "cubemap npot" and oglconform_31 test "textureNPOT".
>>
>> NOTE: This is a candidate for stable release branches.
>>
>> Signed-off-by: Kenneth Graunke 
>> ---
>>  src/mesa/drivers/dri/i965/brw_tex_layout.c |2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> Note that the piglit test referenced isn't committed yet; I sent it to the
>> piglit mailing list.
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c 
>> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> index f462f32..46a417a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> @@ -60,7 +60,7 @@ GLboolean brw_miptree_layout(struct intel_context *intel,
>> * given in Volume 1 of the BSpec.
>> */
>>h0 = ALIGN(mt->height0, align_h);
>> -  h1 = ALIGN(minify(h0), align_h);
>> +  h1 = ALIGN(minify(mt->height0), align_h);
>>qpitch = (h0 + h1 + (intel->gen >= 7 ? 12 : 11) * align_h);
>>if (mt->compressed)
>>   qpitch /= 4;
> 
> 
> This looks wrong to me.  The height of a level L is ALIGN(height0 >> L,
> j) according to SNB PRM vol1, 6.17.3.1 "Computing MIP level sizes".
> Note that our calculation of j is wrong for a bunch of hardware/format
> combos -- I wonder if that was the issue you were looking at?

Just for the record, I talked with Eric about this in person, and he
agreed that my patch is correct.  mt->height0 is H_0 (actual height) in
the PRM, while h0 is h_0 (aligned).  We were using the wrong one.  Also,
for the case I was looking at, j == 2, so that wasn't the issue.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] i965/gen5+: Fix incorrect miptree layout for non-power-of-two cubemaps.

2011-07-31 Thread Eric Anholt
On Sat, 30 Jul 2011 21:35:52 -0700, Kenneth Graunke  
wrote:
> For power-of-two sizes, h0 == mt->height0 since it's already a multiple
> of two.  However, for NPOT, they're different; h1 should be computed
> based on the original size.
> 
> Fixes piglit test "cubemap npot" and oglconform_31 test "textureNPOT".
> 
> NOTE: This is a candidate for stable release branches.
> 
> Signed-off-by: Kenneth Graunke 
> ---
>  src/mesa/drivers/dri/i965/brw_tex_layout.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Note that the piglit test referenced isn't committed yet; I sent it to the
> piglit mailing list.
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c 
> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index f462f32..46a417a 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -60,7 +60,7 @@ GLboolean brw_miptree_layout(struct intel_context *intel,
>  * given in Volume 1 of the BSpec.
>  */
> h0 = ALIGN(mt->height0, align_h);
> -   h1 = ALIGN(minify(h0), align_h);
> +   h1 = ALIGN(minify(mt->height0), align_h);
> qpitch = (h0 + h1 + (intel->gen >= 7 ? 12 : 11) * align_h);
>if (mt->compressed)
>qpitch /= 4;


This looks wrong to me.  The height of a level L is ALIGN(height0 >> L,
j) according to SNB PRM vol1, 6.17.3.1 "Computing MIP level sizes".
Note that our calculation of j is wrong for a bunch of hardware/format
combos -- I wonder if that was the issue you were looking at?


pgpW1Mo6odvzG.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


[Mesa-dev] [PATCH] i965/gen5+: Fix incorrect miptree layout for non-power-of-two cubemaps.

2011-07-30 Thread Kenneth Graunke
For power-of-two sizes, h0 == mt->height0 since it's already a multiple
of two.  However, for NPOT, they're different; h1 should be computed
based on the original size.

Fixes piglit test "cubemap npot" and oglconform_31 test "textureNPOT".

NOTE: This is a candidate for stable release branches.

Signed-off-by: Kenneth Graunke 
---
 src/mesa/drivers/dri/i965/brw_tex_layout.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Note that the piglit test referenced isn't committed yet; I sent it to the
piglit mailing list.

diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c 
b/src/mesa/drivers/dri/i965/brw_tex_layout.c
index f462f32..46a417a 100644
--- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
+++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
@@ -60,7 +60,7 @@ GLboolean brw_miptree_layout(struct intel_context *intel,
   * given in Volume 1 of the BSpec.
   */
  h0 = ALIGN(mt->height0, align_h);
- h1 = ALIGN(minify(h0), align_h);
+ h1 = ALIGN(minify(mt->height0), align_h);
  qpitch = (h0 + h1 + (intel->gen >= 7 ? 12 : 11) * align_h);
   if (mt->compressed)
 qpitch /= 4;
-- 
1.7.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev