| Message ID | 1379594038-17106-1-git-send-email-marek.olsak@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
On Thu, Sep 19, 2013 at 4:41 PM, Michel Dänzer <michel@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
BTW, the intuitive explanation of the fix is that we have to minify the pitch (or POT width) instead of the NPOT width. Marek On Thu, Sep 19, 2013 at 6:37 PM, Marek Olšák <maraeo@gmail.com> wrote: > On Thu, Sep 19, 2013 at 4:41 PM, Michel Dänzer <michel@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
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@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?
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@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@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 >
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); @@ -1434,7 +1438,7 @@ static void si_surf_minify(struct radeon_surface *surf, if (level == 0 && surf->last_level == 0) /* Non-mipmap pitch padded to slice alignment */ xalign = MAX2(xalign, slice_align / surf->bpe); - else + else if (surflevel->mode == RADEON_SURF_MODE_LINEAR_ALIGNED) /* Small rows evenly distributed across slice */ xalign = MAX2(xalign, slice_align / surf->bpe / surflevel->nblk_y); @@ -1456,7 +1460,11 @@ static void si_surf_minify_2d(struct radeon_surface *surf, { unsigned mtile_pr, mtile_ps; - 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);