* [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI @ 2013-09-19 12:33 Marek Olšák 2013-09-19 14:41 ` Michel Dänzer 0 siblings, 1 reply; 6+ messages in thread From: Marek Olšák @ 2013-09-19 12:33 UTC (permalink / raw) To: dri-devel 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); @@ -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); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI 2013-09-19 12:33 [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI Marek Olšák @ 2013-09-19 14:41 ` Michel Dänzer 2013-09-19 16:37 ` Marek Olšák 0 siblings, 1 reply; 6+ messages in thread From: Michel Dänzer @ 2013-09-19 14:41 UTC (permalink / raw) To: Marek Olšák; +Cc: dri-devel 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? -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Debian, X and DRI developer _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI 2013-09-19 14:41 ` Michel Dänzer @ 2013-09-19 16:37 ` Marek Olšák 2013-09-19 16:41 ` Marek Olšák 2013-09-20 14:14 ` Michel Dänzer 0 siblings, 2 replies; 6+ messages in thread From: Marek Olšák @ 2013-09-19 16:37 UTC (permalink / raw) To: Michel Dänzer; +Cc: dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI 2013-09-19 16:37 ` Marek Olšák @ 2013-09-19 16:41 ` Marek Olšák 2013-09-20 14:14 ` Michel Dänzer 1 sibling, 0 replies; 6+ messages in thread From: Marek Olšák @ 2013-09-19 16:41 UTC (permalink / raw) To: Michel Dänzer; +Cc: dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI 2013-09-19 16:37 ` Marek Olšák 2013-09-19 16:41 ` Marek Olšák @ 2013-09-20 14:14 ` Michel Dänzer 2013-09-20 14:43 ` Marek Olšák 1 sibling, 1 reply; 6+ messages in thread From: Michel Dänzer @ 2013-09-20 14:14 UTC (permalink / raw) To: Marek Olšák; +Cc: dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI 2013-09-20 14:14 ` Michel Dänzer @ 2013-09-20 14:43 ` Marek Olšák 0 siblings, 0 replies; 6+ messages in thread From: Marek Olšák @ 2013-09-20 14:43 UTC (permalink / raw) To: Michel Dänzer; +Cc: dri-devel 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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-20 14:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-19 12:33 [PATCH] radeon: fix pitch alignment for non-power-of-two mipmaps on SI Marek Olšák 2013-09-19 14:41 ` Michel Dänzer 2013-09-19 16:37 ` Marek Olšák 2013-09-19 16:41 ` Marek Olšák 2013-09-20 14:14 ` Michel Dänzer 2013-09-20 14:43 ` Marek Olšák
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.