[PATCH 1/2] radeon: simplify ZS buffer checking on r600

[PATCH 1/2] radeon: simplify ZS buffer checking on r600

[Marek Olšák]

Setting those flags has no effect anywhere else.
---
 radeon/radeon_surface.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
index c80f7f4..5800c33 100644
--- a/radeon/radeon_surface.c
+++ b/radeon/radeon_surface.c
@@ -385,14 +385,7 @@ static int r6_surface_init(struct radeon_surface_manager *surf_man,
     /* tiling mode */
     mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
 
-    /* always enable z & stencil together */
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
-        surf->flags |= RADEON_SURF_SBUFFER;
-    }
-    if (surf->flags & RADEON_SURF_SBUFFER) {
-        surf->flags |= RADEON_SURF_ZBUFFER;
-    }
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
+    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
         /* zbuffer only support 1D or 2D tiled surface */
         switch (mode) {
         case RADEON_SURF_MODE_1D:
-- 
1.7.9.5

[PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Marek Olšák]

If we don't need stencil, don't allocate it.
If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
---
 radeon/radeon_surface.c |   19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
index 5800c33..fcfefdc 100644
--- a/radeon/radeon_surface.c
+++ b/radeon/radeon_surface.c
@@ -604,7 +604,10 @@ static int eg_surface_init_1d(struct radeon_surface_manager *surf_man,
         }
     }
 
-    if (surf->flags & RADEON_SURF_SBUFFER) {
+    /* The depth and stencil buffers are in separate resources on evergreen.
+     * We allocate them in one buffer next to each other to simplify
+     * communication between the DDX and the Mesa driver. */
+    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
         surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
         surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
     }
@@ -752,14 +755,7 @@ static int eg_surface_init(struct radeon_surface_manager *surf_man,
     /* tiling mode */
     mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
 
-    /* for some reason eg need to have room for stencil right after depth */
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
-        surf->flags |= RADEON_SURF_SBUFFER;
-    }
-    if (surf->flags & RADEON_SURF_SBUFFER) {
-        surf->flags |= RADEON_SURF_ZBUFFER;
-    }
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
+    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
         /* zbuffer only support 1D or 2D tiled surface */
         switch (mode) {
         case RADEON_SURF_MODE_1D:
@@ -828,11 +824,6 @@ static int eg_surface_best(struct radeon_surface_manager *surf_man,
     /* tiling mode */
     mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
 
-    /* for some reason eg need to have room for stencil right after depth */
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
-        surf->flags |= RADEON_SURF_SBUFFER;
-    }
-
     /* set some default value to avoid sanity check choking on them */
     surf->tile_split = 1024;
     surf->bankw = 1;
-- 
1.7.9.5

[PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Marek Olšák]

If we don't need stencil, don't allocate it.
If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.

v2: actually do it correctly
---
 radeon/radeon_surface.c |   23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
index 5800c33..874a092 100644
--- a/radeon/radeon_surface.c
+++ b/radeon/radeon_surface.c
@@ -604,7 +604,11 @@ static int eg_surface_init_1d(struct radeon_surface_manager *surf_man,
         }
     }
 
-    if (surf->flags & RADEON_SURF_SBUFFER) {
+    /* The depth and stencil buffers are in separate resources on evergreen.
+     * We allocate them in one buffer next to each other to simplify
+     * communication between the DDX and the Mesa driver. */
+    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
+	(RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
         surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
         surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
     }
@@ -656,7 +660,8 @@ static int eg_surface_init_2d(struct radeon_surface_manager *surf_man,
         }
     }
 
-    if (surf->flags & RADEON_SURF_SBUFFER) {
+    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
+	(RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
         surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
         surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
     }
@@ -752,14 +757,7 @@ static int eg_surface_init(struct radeon_surface_manager *surf_man,
     /* tiling mode */
     mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
 
-    /* for some reason eg need to have room for stencil right after depth */
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
-        surf->flags |= RADEON_SURF_SBUFFER;
-    }
-    if (surf->flags & RADEON_SURF_SBUFFER) {
-        surf->flags |= RADEON_SURF_ZBUFFER;
-    }
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
+    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
         /* zbuffer only support 1D or 2D tiled surface */
         switch (mode) {
         case RADEON_SURF_MODE_1D:
@@ -828,11 +826,6 @@ static int eg_surface_best(struct radeon_surface_manager *surf_man,
     /* tiling mode */
     mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
 
-    /* for some reason eg need to have room for stencil right after depth */
-    if (surf->flags & RADEON_SURF_ZBUFFER) {
-        surf->flags |= RADEON_SURF_SBUFFER;
-    }
-
     /* set some default value to avoid sanity check choking on them */
     surf->tile_split = 1024;
     surf->bankw = 1;
-- 
1.7.9.5

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Jerome Glisse]

On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
> If we don't need stencil, don't allocate it.
> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>
> v2: actually do it correctly

Big NAK

We need to allocate stencil and depth no matter what. Otherwise it
will lockup. You can take a look by poisonning the surface and see
that when stencil is disabled or depth is disabled the hw still write
to it.

Cheers,
Jerome

> ---
>  radeon/radeon_surface.c |   23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
> index 5800c33..874a092 100644
> --- a/radeon/radeon_surface.c
> +++ b/radeon/radeon_surface.c
> @@ -604,7 +604,11 @@ static int eg_surface_init_1d(struct radeon_surface_manager *surf_man,
>          }
>      }
>
> -    if (surf->flags & RADEON_SURF_SBUFFER) {
> +    /* The depth and stencil buffers are in separate resources on evergreen.
> +     * We allocate them in one buffer next to each other to simplify
> +     * communication between the DDX and the Mesa driver. */
> +    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
> +       (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>          surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
>          surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
>      }
> @@ -656,7 +660,8 @@ static int eg_surface_init_2d(struct radeon_surface_manager *surf_man,
>          }
>      }
>
> -    if (surf->flags & RADEON_SURF_SBUFFER) {
> +    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
> +       (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>          surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
>          surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
>      }
> @@ -752,14 +757,7 @@ static int eg_surface_init(struct radeon_surface_manager *surf_man,
>      /* tiling mode */
>      mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
>
> -    /* for some reason eg need to have room for stencil right after depth */
> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
> -        surf->flags |= RADEON_SURF_SBUFFER;
> -    }
> -    if (surf->flags & RADEON_SURF_SBUFFER) {
> -        surf->flags |= RADEON_SURF_ZBUFFER;
> -    }
> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
> +    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>          /* zbuffer only support 1D or 2D tiled surface */
>          switch (mode) {
>          case RADEON_SURF_MODE_1D:
> @@ -828,11 +826,6 @@ static int eg_surface_best(struct radeon_surface_manager *surf_man,
>      /* tiling mode */
>      mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
>
> -    /* for some reason eg need to have room for stencil right after depth */
> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
> -        surf->flags |= RADEON_SURF_SBUFFER;
> -    }
> -
>      /* set some default value to avoid sanity check choking on them */
>      surf->tile_split = 1024;
>      surf->bankw = 1;
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Christian König]

On 30.07.2012 16:56, Jerome Glisse wrote:
> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> If we don't need stencil, don't allocate it.
>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>
>> v2: actually do it correctly
> Big NAK
>
> We need to allocate stencil and depth no matter what. Otherwise it
> will lockup. You can take a look by poisonning the surface and see
> that when stencil is disabled or depth is disabled the hw still write
> to it.

Really? That bug is new to me, at least on SI that works perfectly 
(currently working on it), so on which hardware do you see that behavior?

Anyway, when we have hardware bugs that enabling depth also enables 
stencil we should handle that in the hardware specific drivers, not in 
general purpose code.

Cheers,
Christian.

>
> Cheers,
> Jerome
>
>> ---
>>   radeon/radeon_surface.c |   23 ++++++++---------------
>>   1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
>> index 5800c33..874a092 100644
>> --- a/radeon/radeon_surface.c
>> +++ b/radeon/radeon_surface.c
>> @@ -604,7 +604,11 @@ static int eg_surface_init_1d(struct radeon_surface_manager *surf_man,
>>           }
>>       }
>>
>> -    if (surf->flags & RADEON_SURF_SBUFFER) {
>> +    /* The depth and stencil buffers are in separate resources on evergreen.
>> +     * We allocate them in one buffer next to each other to simplify
>> +     * communication between the DDX and the Mesa driver. */
>> +    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
>> +       (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>>           surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
>>           surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
>>       }
>> @@ -656,7 +660,8 @@ static int eg_surface_init_2d(struct radeon_surface_manager *surf_man,
>>           }
>>       }
>>
>> -    if (surf->flags & RADEON_SURF_SBUFFER) {
>> +    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
>> +       (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>>           surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
>>           surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
>>       }
>> @@ -752,14 +757,7 @@ static int eg_surface_init(struct radeon_surface_manager *surf_man,
>>       /* tiling mode */
>>       mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
>>
>> -    /* for some reason eg need to have room for stencil right after depth */
>> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
>> -        surf->flags |= RADEON_SURF_SBUFFER;
>> -    }
>> -    if (surf->flags & RADEON_SURF_SBUFFER) {
>> -        surf->flags |= RADEON_SURF_ZBUFFER;
>> -    }
>> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
>> +    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>>           /* zbuffer only support 1D or 2D tiled surface */
>>           switch (mode) {
>>           case RADEON_SURF_MODE_1D:
>> @@ -828,11 +826,6 @@ static int eg_surface_best(struct radeon_surface_manager *surf_man,
>>       /* tiling mode */
>>       mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
>>
>> -    /* for some reason eg need to have room for stencil right after depth */
>> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
>> -        surf->flags |= RADEON_SURF_SBUFFER;
>> -    }
>> -
>>       /* set some default value to avoid sanity check choking on them */
>>       surf->tile_split = 1024;
>>       surf->bankw = 1;
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Jerome Glisse]

On Mon, Jul 30, 2012 at 11:17 AM, Christian König
<deathsimple@vodafone.de> wrote:
> On 30.07.2012 16:56, Jerome Glisse wrote:
>>
>> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>
>>> If we don't need stencil, don't allocate it.
>>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>>
>>> v2: actually do it correctly
>>
>> Big NAK
>>
>> We need to allocate stencil and depth no matter what. Otherwise it
>> will lockup. You can take a look by poisonning the surface and see
>> that when stencil is disabled or depth is disabled the hw still write
>> to it.
>
>
> Really? That bug is new to me, at least on SI that works perfectly
> (currently working on it), so on which hardware do you see that behavior?

I must have put which GPU are affected in one commit either ddx, mesa
or libdrm. From memory all evergreen but not in all case and cayman in
all case.

Cheers,
Jerome

>
> Anyway, when we have hardware bugs that enabling depth also enables stencil
> we should handle that in the hardware specific drivers, not in general
> purpose code.
>
> Cheers,
> Christian.

At time it was the easiest solution to put it there.

Cheers,
Jerome
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Roland Scheidegger]

Am 30.07.2012 16:56, schrieb Jerome Glisse:
> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> If we don't need stencil, don't allocate it.
>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>
>> v2: actually do it correctly
> 
> Big NAK
> 
> We need to allocate stencil and depth no matter what. Otherwise it
> will lockup. You can take a look by poisonning the surface and see
> that when stencil is disabled or depth is disabled the hw still write
> to it.

That seems weird - bandwidth is a precious resource. Is that because of
some misconfiguration elsewhere? Or hiz buffers or the like?

Roland

> 
> Cheers,
> Jerome
> 
>> ---
>>  radeon/radeon_surface.c |   23 ++++++++---------------
>>  1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
>> index 5800c33..874a092 100644
>> --- a/radeon/radeon_surface.c
>> +++ b/radeon/radeon_surface.c
>> @@ -604,7 +604,11 @@ static int eg_surface_init_1d(struct radeon_surface_manager *surf_man,
>>          }
>>      }
>>
>> -    if (surf->flags & RADEON_SURF_SBUFFER) {
>> +    /* The depth and stencil buffers are in separate resources on evergreen.
>> +     * We allocate them in one buffer next to each other to simplify
>> +     * communication between the DDX and the Mesa driver. */
>> +    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
>> +       (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>>          surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
>>          surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
>>      }
>> @@ -656,7 +660,8 @@ static int eg_surface_init_2d(struct radeon_surface_manager *surf_man,
>>          }
>>      }
>>
>> -    if (surf->flags & RADEON_SURF_SBUFFER) {
>> +    if ((surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) ==
>> +       (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>>          surf->stencil_offset = ALIGN(surf->bo_size, surf->bo_alignment);
>>          surf->bo_size = surf->stencil_offset + surf->bo_size / 4;
>>      }
>> @@ -752,14 +757,7 @@ static int eg_surface_init(struct radeon_surface_manager *surf_man,
>>      /* tiling mode */
>>      mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
>>
>> -    /* for some reason eg need to have room for stencil right after depth */
>> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
>> -        surf->flags |= RADEON_SURF_SBUFFER;
>> -    }
>> -    if (surf->flags & RADEON_SURF_SBUFFER) {
>> -        surf->flags |= RADEON_SURF_ZBUFFER;
>> -    }
>> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
>> +    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>>          /* zbuffer only support 1D or 2D tiled surface */
>>          switch (mode) {
>>          case RADEON_SURF_MODE_1D:
>> @@ -828,11 +826,6 @@ static int eg_surface_best(struct radeon_surface_manager *surf_man,
>>      /* tiling mode */
>>      mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
>>
>> -    /* for some reason eg need to have room for stencil right after depth */
>> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
>> -        surf->flags |= RADEON_SURF_SBUFFER;
>> -    }
>> -
>>      /* set some default value to avoid sanity check choking on them */
>>      surf->tile_split = 1024;
>>      surf->bankw = 1;
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Marek Olšák]

On Mon, Jul 30, 2012 at 4:56 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>> If we don't need stencil, don't allocate it.
>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>
>> v2: actually do it correctly
>
> Big NAK
>
> We need to allocate stencil and depth no matter what. Otherwise it
> will lockup. You can take a look by poisonning the surface and see
> that when stencil is disabled or depth is disabled the hw still write
> to it.

:)

If what you say is true, then we're in a big trouble. Regardless of
this patch, we're in it right now, because we cannot fully disable
depth or stencil if the user binds a NULL zbuffer. That's clearly the
kind of issue that cannot be fixed in the allocator code and should be
fixed in r600g where the hardware is programmed.

I *think* that the correct way to disable Z or stencil is to set the
Z_INVALID or STENCIL_INVALID format, respectively, and not by
disabling reads and writes. (cc'ing Alex to confirm that if he can). I
don't think the hardware designers have added those "invalid" formats
just for the lulz. Please see my latest kernel patch "drm/radeon/kms:
allow "invalid" DB formats as a means to disable DB" that tries to
address this issue.

For r600g, I was thinking of allocating a dummy buffer that will be
always bound in case the depth or stencil buffer or both are missing.
Either way, we should find how to get around this issue without
wasting memory, especially when there are other options to try.

BTW, before we used this allocator, we allocated the depth and stencil
buffers in separate resources. We might need to get back to it in the
future if Gallium grows separate depth and stencil buffer bindings.

Marek
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Alex Deucher]

On Mon, Jul 30, 2012 at 11:48 AM, Marek Olšák <maraeo@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 4:56 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>> If we don't need stencil, don't allocate it.
>>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>>
>>> v2: actually do it correctly
>>
>> Big NAK
>>
>> We need to allocate stencil and depth no matter what. Otherwise it
>> will lockup. You can take a look by poisonning the surface and see
>> that when stencil is disabled or depth is disabled the hw still write
>> to it.
>
> :)
>
> If what you say is true, then we're in a big trouble. Regardless of
> this patch, we're in it right now, because we cannot fully disable
> depth or stencil if the user binds a NULL zbuffer. That's clearly the
> kind of issue that cannot be fixed in the allocator code and should be
> fixed in r600g where the hardware is programmed.
>
> I *think* that the correct way to disable Z or stencil is to set the
> Z_INVALID or STENCIL_INVALID format, respectively, and not by
> disabling reads and writes. (cc'ing Alex to confirm that if he can). I
> don't think the hardware designers have added those "invalid" formats
> just for the lulz. Please see my latest kernel patch "drm/radeon/kms:
> allow "invalid" DB formats as a means to disable DB" that tries to
> address this issue.

That is correct.  I just verified with the hw team.  If you allocate
both buffers there are some restrictions in that they share tiling
parameters, but you can set either buffer to _INVALID and allocate one
or the other independently.

Alex

>
> For r600g, I was thinking of allocating a dummy buffer that will be
> always bound in case the depth or stencil buffer or both are missing.
> Either way, we should find how to get around this issue without
> wasting memory, especially when there are other options to try.
>
> BTW, before we used this allocator, we allocated the depth and stencil
> buffers in separate resources. We might need to get back to it in the
> future if Gallium grows separate depth and stencil buffer bindings.
>
> Marek
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Alex Deucher]

On Mon, Jul 30, 2012 at 12:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 11:48 AM, Marek Olšák <maraeo@gmail.com> wrote:
>> On Mon, Jul 30, 2012 at 4:56 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>>> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>> If we don't need stencil, don't allocate it.
>>>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>>>
>>>> v2: actually do it correctly
>>>
>>> Big NAK
>>>
>>> We need to allocate stencil and depth no matter what. Otherwise it
>>> will lockup. You can take a look by poisonning the surface and see
>>> that when stencil is disabled or depth is disabled the hw still write
>>> to it.
>>
>> :)
>>
>> If what you say is true, then we're in a big trouble. Regardless of
>> this patch, we're in it right now, because we cannot fully disable
>> depth or stencil if the user binds a NULL zbuffer. That's clearly the
>> kind of issue that cannot be fixed in the allocator code and should be
>> fixed in r600g where the hardware is programmed.
>>
>> I *think* that the correct way to disable Z or stencil is to set the
>> Z_INVALID or STENCIL_INVALID format, respectively, and not by
>> disabling reads and writes. (cc'ing Alex to confirm that if he can). I
>> don't think the hardware designers have added those "invalid" formats
>> just for the lulz. Please see my latest kernel patch "drm/radeon/kms:
>> allow "invalid" DB formats as a means to disable DB" that tries to
>> address this issue.
>
> That is correct.  I just verified with the hw team.  If you allocate
> both buffers there are some restrictions in that they share tiling
> parameters, but you can set either buffer to _INVALID and allocate one
> or the other independently.

Some further clarifications from the hw team.  If you want to have
truly independent z and stencil buffers that allows for mixing and
matching, you need to make sure that any z and stencil buffer that can
be bound together must share the same addressing parameters (except
tile split), and you must disable the htile buffer on evergreen and
before (DB_Z_INFO.TILE_SURFACE_ENABLE=0) or disable the htile buffer
for stencil on cayman and newer
(DB_STENCIL_INFO.TILE_STENCIL_DISABLE=1).  If you are only interested
in unbinding z or stencil independently (but not mixing and matching)
then you don't need to the above restrictions on the htile buffer.
You can do so by setting the format to INVALID.

Alex
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Jerome Glisse]

On Mon, Jul 30, 2012 at 12:16 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 12:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Mon, Jul 30, 2012 at 11:48 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>> On Mon, Jul 30, 2012 at 4:56 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>>>> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> If we don't need stencil, don't allocate it.
>>>>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>>>>
>>>>> v2: actually do it correctly
>>>>
>>>> Big NAK
>>>>
>>>> We need to allocate stencil and depth no matter what. Otherwise it
>>>> will lockup. You can take a look by poisonning the surface and see
>>>> that when stencil is disabled or depth is disabled the hw still write
>>>> to it.
>>>
>>> :)
>>>
>>> If what you say is true, then we're in a big trouble. Regardless of
>>> this patch, we're in it right now, because we cannot fully disable
>>> depth or stencil if the user binds a NULL zbuffer. That's clearly the
>>> kind of issue that cannot be fixed in the allocator code and should be
>>> fixed in r600g where the hardware is programmed.
>>>
>>> I *think* that the correct way to disable Z or stencil is to set the
>>> Z_INVALID or STENCIL_INVALID format, respectively, and not by
>>> disabling reads and writes. (cc'ing Alex to confirm that if he can). I
>>> don't think the hardware designers have added those "invalid" formats
>>> just for the lulz. Please see my latest kernel patch "drm/radeon/kms:
>>> allow "invalid" DB formats as a means to disable DB" that tries to
>>> address this issue.
>>
>> That is correct.  I just verified with the hw team.  If you allocate
>> both buffers there are some restrictions in that they share tiling
>> parameters, but you can set either buffer to _INVALID and allocate one
>> or the other independently.
>
> Some further clarifications from the hw team.  If you want to have
> truly independent z and stencil buffers that allows for mixing and
> matching, you need to make sure that any z and stencil buffer that can
> be bound together must share the same addressing parameters (except
> tile split), and you must disable the htile buffer on evergreen and
> before (DB_Z_INFO.TILE_SURFACE_ENABLE=0) or disable the htile buffer
> for stencil on cayman and newer
> (DB_STENCIL_INFO.TILE_STENCIL_DISABLE=1).  If you are only interested
> in unbinding z or stencil independently (but not mixing and matching)
> then you don't need to the above restrictions on the htile buffer.
> You can do so by setting the format to INVALID.
>
> Alex

Well somehow i can't reproduce might have been fixed by something like
render backend fix. I should have write down how i saw this back in
the day.

Cheers,
Jerome
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Alex Deucher]

On Mon, Jul 30, 2012 at 12:52 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 12:16 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Mon, Jul 30, 2012 at 12:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>> On Mon, Jul 30, 2012 at 11:48 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>> On Mon, Jul 30, 2012 at 4:56 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>>>>> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>> If we don't need stencil, don't allocate it.
>>>>>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>>>>>
>>>>>> v2: actually do it correctly
>>>>>
>>>>> Big NAK
>>>>>
>>>>> We need to allocate stencil and depth no matter what. Otherwise it
>>>>> will lockup. You can take a look by poisonning the surface and see
>>>>> that when stencil is disabled or depth is disabled the hw still write
>>>>> to it.
>>>>
>>>> :)
>>>>
>>>> If what you say is true, then we're in a big trouble. Regardless of
>>>> this patch, we're in it right now, because we cannot fully disable
>>>> depth or stencil if the user binds a NULL zbuffer. That's clearly the
>>>> kind of issue that cannot be fixed in the allocator code and should be
>>>> fixed in r600g where the hardware is programmed.
>>>>
>>>> I *think* that the correct way to disable Z or stencil is to set the
>>>> Z_INVALID or STENCIL_INVALID format, respectively, and not by
>>>> disabling reads and writes. (cc'ing Alex to confirm that if he can). I
>>>> don't think the hardware designers have added those "invalid" formats
>>>> just for the lulz. Please see my latest kernel patch "drm/radeon/kms:
>>>> allow "invalid" DB formats as a means to disable DB" that tries to
>>>> address this issue.
>>>
>>> That is correct.  I just verified with the hw team.  If you allocate
>>> both buffers there are some restrictions in that they share tiling
>>> parameters, but you can set either buffer to _INVALID and allocate one
>>> or the other independently.
>>
>> Some further clarifications from the hw team.  If you want to have
>> truly independent z and stencil buffers that allows for mixing and
>> matching, you need to make sure that any z and stencil buffer that can
>> be bound together must share the same addressing parameters (except
>> tile split), and you must disable the htile buffer on evergreen and
>> before (DB_Z_INFO.TILE_SURFACE_ENABLE=0) or disable the htile buffer
>> for stencil on cayman and newer
>> (DB_STENCIL_INFO.TILE_STENCIL_DISABLE=1).  If you are only interested
>> in unbinding z or stencil independently (but not mixing and matching)
>> then you don't need to the above restrictions on the htile buffer.
>> You can do so by setting the format to INVALID.
>>
>> Alex
>
> Well somehow i can't reproduce might have been fixed by something like
> render backend fix. I should have write down how i saw this back in
> the day.

I think it was related to tiling.  I remember you mentioning something
about that at the time.

Alex

Re: [PATCH 2/2] radeon: optimize allocation for depth w/o stencil and stencil w/o depth on EG

[Jerome Glisse]

On Mon, Jul 30, 2012 at 1:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 12:52 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>> On Mon, Jul 30, 2012 at 12:16 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>> On Mon, Jul 30, 2012 at 12:03 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
>>>> On Mon, Jul 30, 2012 at 11:48 AM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>> On Mon, Jul 30, 2012 at 4:56 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>>>>>> On Sun, Jul 29, 2012 at 1:04 PM, Marek Olšák <maraeo@gmail.com> wrote:
>>>>>>> If we don't need stencil, don't allocate it.
>>>>>>> If we need only stencil (like PIPE_FORMAT_S8_UINT), don't allocate depth.
>>>>>>>
>>>>>>> v2: actually do it correctly
>>>>>>
>>>>>> Big NAK
>>>>>>
>>>>>> We need to allocate stencil and depth no matter what. Otherwise it
>>>>>> will lockup. You can take a look by poisonning the surface and see
>>>>>> that when stencil is disabled or depth is disabled the hw still write
>>>>>> to it.
>>>>>
>>>>> :)
>>>>>
>>>>> If what you say is true, then we're in a big trouble. Regardless of
>>>>> this patch, we're in it right now, because we cannot fully disable
>>>>> depth or stencil if the user binds a NULL zbuffer. That's clearly the
>>>>> kind of issue that cannot be fixed in the allocator code and should be
>>>>> fixed in r600g where the hardware is programmed.
>>>>>
>>>>> I *think* that the correct way to disable Z or stencil is to set the
>>>>> Z_INVALID or STENCIL_INVALID format, respectively, and not by
>>>>> disabling reads and writes. (cc'ing Alex to confirm that if he can). I
>>>>> don't think the hardware designers have added those "invalid" formats
>>>>> just for the lulz. Please see my latest kernel patch "drm/radeon/kms:
>>>>> allow "invalid" DB formats as a means to disable DB" that tries to
>>>>> address this issue.
>>>>
>>>> That is correct.  I just verified with the hw team.  If you allocate
>>>> both buffers there are some restrictions in that they share tiling
>>>> parameters, but you can set either buffer to _INVALID and allocate one
>>>> or the other independently.
>>>
>>> Some further clarifications from the hw team.  If you want to have
>>> truly independent z and stencil buffers that allows for mixing and
>>> matching, you need to make sure that any z and stencil buffer that can
>>> be bound together must share the same addressing parameters (except
>>> tile split), and you must disable the htile buffer on evergreen and
>>> before (DB_Z_INFO.TILE_SURFACE_ENABLE=0) or disable the htile buffer
>>> for stencil on cayman and newer
>>> (DB_STENCIL_INFO.TILE_STENCIL_DISABLE=1).  If you are only interested
>>> in unbinding z or stencil independently (but not mixing and matching)
>>> then you don't need to the above restrictions on the htile buffer.
>>> You can do so by setting the format to INVALID.
>>>
>>> Alex
>>
>> Well somehow i can't reproduce might have been fixed by something like
>> render backend fix. I should have write down how i saw this back in
>> the day.
>
> I think it was related to tiling.  I remember you mentioning something
> about that at the time.
>
> Alex

Yeah might be tiling related, surface allocator probably got the bo size right.

Cheers,
Jerime
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] radeon: simplify ZS buffer checking on r600

[Christian König]

I wanted to work on something similar this week, cause we need some 
updates for SI on this.

So thx, you saved me some work here. And both patches are:

Reviewed-by: Christian König <christian.koenig@amd.com>

On 29.07.2012 16:02, Marek Olšák wrote:
> Setting those flags has no effect anywhere else.
> ---
>   radeon/radeon_surface.c |    9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/radeon/radeon_surface.c b/radeon/radeon_surface.c
> index c80f7f4..5800c33 100644
> --- a/radeon/radeon_surface.c
> +++ b/radeon/radeon_surface.c
> @@ -385,14 +385,7 @@ static int r6_surface_init(struct radeon_surface_manager *surf_man,
>       /* tiling mode */
>       mode = (surf->flags >> RADEON_SURF_MODE_SHIFT) & RADEON_SURF_MODE_MASK;
>   
> -    /* always enable z & stencil together */
> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
> -        surf->flags |= RADEON_SURF_SBUFFER;
> -    }
> -    if (surf->flags & RADEON_SURF_SBUFFER) {
> -        surf->flags |= RADEON_SURF_ZBUFFER;
> -    }
> -    if (surf->flags & RADEON_SURF_ZBUFFER) {
> +    if (surf->flags & (RADEON_SURF_ZBUFFER | RADEON_SURF_SBUFFER)) {
>           /* zbuffer only support 1D or 2D tiled surface */
>           switch (mode) {
>           case RADEON_SURF_MODE_1D: