Re: [Mesa-dev] [PATCH 0/8] gallium: create a generic way to validate fb params
On Fri, Oct 4, 2013 at 6:39 PM, Ilia Mirkin imir...@alum.mit.edu wrote: On Fri, Oct 4, 2013 at 10:02 AM, Marek Olšák mar...@gmail.com wrote: Wouldn't it be more flexible for you to just change the depth buffer format internally by reallocating the depth buffer in the driver whenever it's incompatible with the colorbuffer? I think you should be able to do that easily without changing the pipe_resource structure. If a depth buffer cannot be used by set_framebuffer_state, just create a new one with an appropriate format, then blit from the old depth buffer to the new one, so that you won't lose its content, and delete the old one. For transfers, you just need to blit to convert the depth buffer to the format the state tracker expects. It would indeed be more flexible. There are two reasons why I didn't do it that way (a) I'm very new to gallium, I'm not comfortable with such trickery. I recommend reading p_state.h, p_context.h and p_screen.h. Make sure you understand most of it. Textures can be blitted using pipe_context::blit. (I don't even know how to blit between textures.) Also, wouldn't you have to blit the new texture back to the old texture? Or is there sufficient magic to take care of the format difference? What if the requested zsbuf was 24-bit but the cbufs were 16-bit? Would you suggest upgrading the cbufs or downgrading the zsbuf -- downgrading the zsbuf could also cause some application weirdness, no? No, the old texture would be completely replaced by the new texture. The old texture would be deleted. There is no magic to help you here, but sometimes we have to jump through hoops to support our hardware. r300g and r600g are full of workarounds and trickery, but at the end of the day, the drivers work better for everybody. If your driver supports only 1 render target, then feel free to upgrade the colorbuffer instead. However It wouldn't be a very good idea if the driver supported multiple render targets. (b) If that were the preferred approach, wouldn't the same approach have been taken with MIXED_COLORBUFFER_FORMATS? The preference seemed to be to reject things the driver didn't like rather than accepting every possible input and converting back and forth. Perhaps the cbufs are somehow different from depth in a way I don't understand though. MIXED_COLORBUFFER_FORMATS is a feature first introduced in R500 and DX9.0c, therefore I expect older games not to break if we disallow mixed formats on older ASICs. Also, apps using mixed formats are not very common, if they exist at all. If you don't want to implement my suggestion above, then I suggest adding a hw-specific CAP, e.g. PIPE_CAP_FRAMEBUFFER_RESTRICTIONS_NV30, and doing whatever you need in st/mesa if the CAP is advertised. That would a lot less invasive than your current patch series. Marek ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/8] gallium: create a generic way to validate fb params
Wouldn't it be more flexible for you to just change the depth buffer format internally by reallocating the depth buffer in the driver whenever it's incompatible with the colorbuffer? I think you should be able to do that easily without changing the pipe_resource structure. If a depth buffer cannot be used by set_framebuffer_state, just create a new one with an appropriate format, then blit from the old depth buffer to the new one, so that you won't lose its content, and delete the old one. For transfers, you just need to blit to convert the depth buffer to the format the state tracker expects. I'm quite sure exposing the weird restrictions of your hardware to applications won't end well. The apps won't like it. Marek On Fri, Oct 4, 2013 at 10:34 AM, Ilia Mirkin imir...@alum.mit.edu wrote: NV30 is blessed with all manners of hardware restrictions. One of them is that the render target format's color and depth outputs need to be the same bit-ness, i.e. either both 16 or both 32 bits. (In addition to all color attachments needing to be the same, and everything needing to be the same size.) There is a PIPE_CAP_MIXED_COLORBUFFER_FORMATS that currently handles the requirement of all the cbuf attachments having the same format. This series creates a more generic mechanism to perform such validation which allows nv30 to also check the sizes of the cbuf/zsbuf formats. At the end, I remove that PIPE_CAP as it is no longer necessary. I first tried to use a pipe_framebuffer_state as the parameter to this new call, however I couldn't quite figure out how to populate it in case that the depth output buffer was a texture (seems to map to a pipe_resource, not a pipe_surface). This series is a follow-up to the discussion started in http://lists.freedesktop.org/archives/mesa-dev/2013-September/044658.html Ilia Mirkin (8): mesa/st: create interface to verify fb format, reject bad fbs gallium: add helper to use as a replacement for the MIXED_COLOR cap i915: hook up is_fb_format_supported r300: hook up is_fb_format_supported softpipe: hook up is_fb_format_supported nv30: hook up is_fb_format_supported gallium: remove PIPE_CAP_MIXED_COLORBUFFER_FORMATS nv30: make sure that cbufs and zsbuf have the same depth src/gallium/auxiliary/util/u_format.c| 20 +++ src/gallium/auxiliary/util/u_format.h| 11 +++ src/gallium/docs/source/screen.rst | 2 -- src/gallium/drivers/freedreno/freedreno_screen.c | 1 - src/gallium/drivers/i915/i915_screen.c | 2 +- src/gallium/drivers/ilo/ilo_screen.c | 2 -- src/gallium/drivers/llvmpipe/lp_screen.c | 2 -- src/gallium/drivers/nouveau/nv30/nv30_screen.c | 20 ++- src/gallium/drivers/nouveau/nv50/nv50_screen.c | 1 - src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 1 - src/gallium/drivers/r300/r300_screen.c | 3 +- src/gallium/drivers/r600/r600_pipe.c | 1 - src/gallium/drivers/radeonsi/radeonsi_pipe.c | 1 - src/gallium/drivers/softpipe/sp_screen.c | 3 +- src/gallium/drivers/svga/svga_screen.c | 3 -- src/gallium/include/pipe/p_defines.h | 1 - src/gallium/include/pipe/p_screen.h | 18 +- src/mesa/state_tracker/st_cb_fbo.c | 42 ++-- 18 files changed, 95 insertions(+), 39 deletions(-) -- 1.8.1.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/8] gallium: create a generic way to validate fb params
On Fri, Oct 4, 2013 at 10:02 AM, Marek Olšák mar...@gmail.com wrote: Wouldn't it be more flexible for you to just change the depth buffer format internally by reallocating the depth buffer in the driver whenever it's incompatible with the colorbuffer? I think you should be able to do that easily without changing the pipe_resource structure. If a depth buffer cannot be used by set_framebuffer_state, just create a new one with an appropriate format, then blit from the old depth buffer to the new one, so that you won't lose its content, and delete the old one. For transfers, you just need to blit to convert the depth buffer to the format the state tracker expects. It would indeed be more flexible. There are two reasons why I didn't do it that way (a) I'm very new to gallium, I'm not comfortable with such trickery. (I don't even know how to blit between textures.) Also, wouldn't you have to blit the new texture back to the old texture? Or is there sufficient magic to take care of the format difference? What if the requested zsbuf was 24-bit but the cbufs were 16-bit? Would you suggest upgrading the cbufs or downgrading the zsbuf -- downgrading the zsbuf could also cause some application weirdness, no? (b) If that were the preferred approach, wouldn't the same approach have been taken with MIXED_COLORBUFFER_FORMATS? The preference seemed to be to reject things the driver didn't like rather than accepting every possible input and converting back and forth. Perhaps the cbufs are somehow different from depth in a way I don't understand though. I'm quite sure exposing the weird restrictions of your hardware to applications won't end well. The apps won't like it. Indeed. I observed that e.g. supertuxkart crashes (in its own code) due to it trying GL_RGBA8 with a 16-bit zsbuf. They've fixed the crash in SVN (before I even got to report it!), but not yet in the latest released version. -ilia Marek On Fri, Oct 4, 2013 at 10:34 AM, Ilia Mirkin imir...@alum.mit.edu wrote: NV30 is blessed with all manners of hardware restrictions. One of them is that the render target format's color and depth outputs need to be the same bit-ness, i.e. either both 16 or both 32 bits. (In addition to all color attachments needing to be the same, and everything needing to be the same size.) There is a PIPE_CAP_MIXED_COLORBUFFER_FORMATS that currently handles the requirement of all the cbuf attachments having the same format. This series creates a more generic mechanism to perform such validation which allows nv30 to also check the sizes of the cbuf/zsbuf formats. At the end, I remove that PIPE_CAP as it is no longer necessary. I first tried to use a pipe_framebuffer_state as the parameter to this new call, however I couldn't quite figure out how to populate it in case that the depth output buffer was a texture (seems to map to a pipe_resource, not a pipe_surface). This series is a follow-up to the discussion started in http://lists.freedesktop.org/archives/mesa-dev/2013-September/044658.html Ilia Mirkin (8): mesa/st: create interface to verify fb format, reject bad fbs gallium: add helper to use as a replacement for the MIXED_COLOR cap i915: hook up is_fb_format_supported r300: hook up is_fb_format_supported softpipe: hook up is_fb_format_supported nv30: hook up is_fb_format_supported gallium: remove PIPE_CAP_MIXED_COLORBUFFER_FORMATS nv30: make sure that cbufs and zsbuf have the same depth src/gallium/auxiliary/util/u_format.c| 20 +++ src/gallium/auxiliary/util/u_format.h| 11 +++ src/gallium/docs/source/screen.rst | 2 -- src/gallium/drivers/freedreno/freedreno_screen.c | 1 - src/gallium/drivers/i915/i915_screen.c | 2 +- src/gallium/drivers/ilo/ilo_screen.c | 2 -- src/gallium/drivers/llvmpipe/lp_screen.c | 2 -- src/gallium/drivers/nouveau/nv30/nv30_screen.c | 20 ++- src/gallium/drivers/nouveau/nv50/nv50_screen.c | 1 - src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 1 - src/gallium/drivers/r300/r300_screen.c | 3 +- src/gallium/drivers/r600/r600_pipe.c | 1 - src/gallium/drivers/radeonsi/radeonsi_pipe.c | 1 - src/gallium/drivers/softpipe/sp_screen.c | 3 +- src/gallium/drivers/svga/svga_screen.c | 3 -- src/gallium/include/pipe/p_defines.h | 1 - src/gallium/include/pipe/p_screen.h | 18 +- src/mesa/state_tracker/st_cb_fbo.c | 42 ++-- 18 files changed, 95 insertions(+), 39 deletions(-) -- 1.8.1.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
