* [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.