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:
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;
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;
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
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
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
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
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
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
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
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
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
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
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:
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Jerome Glisse -
Marek Olšák -
Roland Scheidegger