[PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI

[Marek Olšák]

On Thu, Sep 19, 2013 at 4:41 PM, Michel Dänzer <michel at daenzer.net> wrote:
> On Don, 2013-09-19 at 14:33 +0200, Marek Olšák wrote:
>> This fixes VM protection faults.
>>
>> I have a new piglit test which can iterate over all possible widths, heights,
>> and depths (including NPOT) and tests mipmapping with various texture targets.
>>
>> After this is committed, I'll make a new release of libdrm and bump
>> the libdrm version requirement in Mesa.
>> ---
>>  radeon/radeon_surface.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
>> index 1710e34..d5c45c4 100644
>> --- a/radeon/radeon_surface.c
>> +++ b/radeon/radeon_surface.c
>> @@ -1412,7 +1412,11 @@ static void si_surf_minify(struct radeon_surface *surf,
>>                             uint32_t xalign, uint32_t yalign, uint32_t zalign,
>>                             uint32_t slice_align, unsigned offset)
>>  {
>> -    surflevel->npix_x = mip_minify(surf->npix_x, level);
>> +    if (level == 0) {
>> +        surflevel->npix_x = surf->npix_x;
>> +    } else {
>> +        surflevel->npix_x = mip_minify(next_power_of_two(surf->npix_x), level);
>> +    }
>>      surflevel->npix_y = mip_minify(surf->npix_y, level);
>>      surflevel->npix_z = mip_minify(surf->npix_z, level);
>>
>
> Shouldn't this be done (only) for nblk_x instead of npix_x?

First, level[i].npix_x/y/z have misleading names, because they are
always aligned to a power of two for non-zero mipmap levels, therefore
Mesa shouldn't use them in place of u_minify, because it's not the
same thing. In fact, r600g doesn't really use them and even though
radeonsi does, they are incorrectly used in place of u_minify. It's on
my TODO list.

mip_minify is defined as: level ? MAX2(1, next_power_of_two(x >> level)) : x.
u_minify is defined as: level ? MAX2(1, x >> level) : x.

Considering that probably nothing in Mesa uses level[i].npix_x/y/z
correctly, it's not so important what the variables contain.

The problem this patch fixes is that next_power_of_two should be
applied before the minification, like this: next_power_of_two(x) >>
level. I had to guess it and test it thoroughly. The memory addressing
documentation is pretty useless here.

Marek