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

[Marek Olšák]

I agree we should make npix_x/y/z more useful, but at the same time
it's a public interface and current and old Mesa depends on the
seemingly incorrect values we set there. This wouldn't be an issue if
we merged the allocator to Mesa, along with xf86-video-ati. Who's with
me? :)

In particular, I think changing the values of npix_x/y/z would break
resource_copy_region for NPOT compressed textures in radeonsi, which
is kind of a mess at the moment.

Marek

On Fri, Sep 20, 2013 at 4:14 PM, Michel Dänzer <michel at daenzer.net> wrote:
> On Don, 2013-09-19 at 18:37 +0200, Marek Olšák wrote:
>> 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.
>
> But it seems like it would be possible to make npix_x/y/z contain the
> values their names suggest, by making mip_minify() do the same thing as
> u_minify(), and only rounding up to the next power of two for
> nblk_x/y/z, wouldn't it?
>
>
> --
> Earthling Michel Dänzer           |                   http://www.amd.com
> Libre software enthusiast         |          Debian, X and DRI developer
>