| Message ID | 1343581467-27881-1-git-send-email-maraeo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;