Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Wed, Nov 14, 2018, 10:36 AM Rob Clark On Wed, Nov 14, 2018 at 10:13 AM Marek Olšák wrote: > > > > > > > > On Wed, Nov 14, 2018, 7:54 AM Rob Clark >> > >> On Tue, Nov 13, 2018 at 9:54 PM Marek Olšák wrote: > >> > > >> > > >> > > >> > On Tue, Nov 13, 2018, 6:00 PM Rob Clark >> >> > >> >> On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: > >> >> > > >> >> > Rob Clark writes: > >> >> > > >> >> > > If we can't clear all the buffers with pctx->clear() (say, for > example, > >> >> > > because of ColorMask), push the buffers we *can* clear with > pctx->clear() > >> >> > > first. Tilers want to see clears coming before draws to enable > fast- > >> >> > > paths, and clearing one of the attachments with a quad-draw first > >> >> > > confuses that logic. > >> >> > > >> >> > Oh, nice! > >> >> > > >> >> > Reviewed-by: Eric Anholt > >> >> > > >> >> > Though it feels pretty silly that the ->clear() caller needs a > >> >> > clear_with_quad implementation when the ->clear() implementation > in the > >> >> > driver also needs a clear_with_quad implementation for > non-fast-cleared > >> >> > buffers. :/ > >> >> > >> >> hmm, so perhaps one easy option is to change pctx->clear() to return > a > >> >> boolean, so driver can return false to ask the state tracker to do a > >> > > >> > > >> > Ideally all pipe context functions should always return void to allow > gallium multithreading. > >> > >> Hmm, that is a bit awkward.. and a pipe cap is a bit restrictive, ie. > >> say you can fast-clear only certain color formats, or other weird > >> combination of restrictions. > >> > >> I suppose a pctx->can_do_clear(..) which isn't multithreaded would be > >> a bit more flexible than pipe caps. > > > > > > can_do_clear(..) should be in pipe_screen. I wouldn't like to have > context functions that return a value. > > that means we'd have to pass it framebuffer state, scissor state, and > colormask.. which is a bit annoying and less of a trivial change to > allow drivers to delete some code.. > A driver can also set a flag in pipe_resource to indicate it's clearable. Drivers still have to implement the quad clear for ignorant state trackers. I would say that at least all desktop GPUs would want to use u_threaded_context, so it doesn't make sense to add any queries into pipe_context. Marek > BR, > -R > > > Marek > > > >> > >> > A pipe cap for colormasked and scissored clears would be better. > >> > > >> > > >> >> clear_with_quad().. maybe that would be a first step towards > allowing > >> >> driver to handle clears w/ colormask and possibly scissor (although > >> >> for the later, plus > >> >> glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was > thinking > >> >> of pctx->invalidate_surface()/pctx->invalidate_sub_surface()) > >> > > >> > > >> > Pipe context already contains an invalidate function. > >> > >> yeah, but it is at the level of pipe_resource, which isn't really what > >> you want if only a certain level/layer of a resource is invalidated. > >> > >> I'm still unsure about how I want to track this in the driver, the > >> ideal case would be to track it in fd_surface, except that surfaces > >> tend to be created transiently. Still for glInvalidateFramebuffer() > >> and friends, having an pctx->invalidate_surface() seems like the > >> cleaner API. > >> > >> BR, > >> -R > >> > >> > > >> > Marek > >> > > >> >> > >> >> But either way, I guess this patch is a simple stop-gap solution. > >> >> > >> >> BR, > >> >> -R > >> >> ___ > >> >> mesa-dev mailing list > >> >> mesa-dev@lists.freedesktop.org > >> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Wed, Nov 14, 2018 at 10:13 AM Marek Olšák wrote: > > > > On Wed, Nov 14, 2018, 7:54 AM Rob Clark > >> On Tue, Nov 13, 2018 at 9:54 PM Marek Olšák wrote: >> > >> > >> > >> > On Tue, Nov 13, 2018, 6:00 PM Rob Clark > >> >> >> On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: >> >> > >> >> > Rob Clark writes: >> >> > >> >> > > If we can't clear all the buffers with pctx->clear() (say, for >> >> > > example, >> >> > > because of ColorMask), push the buffers we *can* clear with >> >> > > pctx->clear() >> >> > > first. Tilers want to see clears coming before draws to enable fast- >> >> > > paths, and clearing one of the attachments with a quad-draw first >> >> > > confuses that logic. >> >> > >> >> > Oh, nice! >> >> > >> >> > Reviewed-by: Eric Anholt >> >> > >> >> > Though it feels pretty silly that the ->clear() caller needs a >> >> > clear_with_quad implementation when the ->clear() implementation in the >> >> > driver also needs a clear_with_quad implementation for non-fast-cleared >> >> > buffers. :/ >> >> >> >> hmm, so perhaps one easy option is to change pctx->clear() to return a >> >> boolean, so driver can return false to ask the state tracker to do a >> > >> > >> > Ideally all pipe context functions should always return void to allow >> > gallium multithreading. >> >> Hmm, that is a bit awkward.. and a pipe cap is a bit restrictive, ie. >> say you can fast-clear only certain color formats, or other weird >> combination of restrictions. >> >> I suppose a pctx->can_do_clear(..) which isn't multithreaded would be >> a bit more flexible than pipe caps. > > > can_do_clear(..) should be in pipe_screen. I wouldn't like to have context > functions that return a value. that means we'd have to pass it framebuffer state, scissor state, and colormask.. which is a bit annoying and less of a trivial change to allow drivers to delete some code.. BR, -R > Marek > >> >> > A pipe cap for colormasked and scissored clears would be better. >> > >> > >> >> clear_with_quad().. maybe that would be a first step towards allowing >> >> driver to handle clears w/ colormask and possibly scissor (although >> >> for the later, plus >> >> glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking >> >> of pctx->invalidate_surface()/pctx->invalidate_sub_surface()) >> > >> > >> > Pipe context already contains an invalidate function. >> >> yeah, but it is at the level of pipe_resource, which isn't really what >> you want if only a certain level/layer of a resource is invalidated. >> >> I'm still unsure about how I want to track this in the driver, the >> ideal case would be to track it in fd_surface, except that surfaces >> tend to be created transiently. Still for glInvalidateFramebuffer() >> and friends, having an pctx->invalidate_surface() seems like the >> cleaner API. >> >> BR, >> -R >> >> > >> > Marek >> > >> >> >> >> But either way, I guess this patch is a simple stop-gap solution. >> >> >> >> BR, >> >> -R >> >> ___ >> >> mesa-dev mailing list >> >> mesa-dev@lists.freedesktop.org >> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Wed, Nov 14, 2018, 7:54 AM Rob Clark On Tue, Nov 13, 2018 at 9:54 PM Marek Olšák wrote: > > > > > > > > On Tue, Nov 13, 2018, 6:00 PM Rob Clark >> > >> On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: > >> > > >> > Rob Clark writes: > >> > > >> > > If we can't clear all the buffers with pctx->clear() (say, for > example, > >> > > because of ColorMask), push the buffers we *can* clear with > pctx->clear() > >> > > first. Tilers want to see clears coming before draws to enable > fast- > >> > > paths, and clearing one of the attachments with a quad-draw first > >> > > confuses that logic. > >> > > >> > Oh, nice! > >> > > >> > Reviewed-by: Eric Anholt > >> > > >> > Though it feels pretty silly that the ->clear() caller needs a > >> > clear_with_quad implementation when the ->clear() implementation in > the > >> > driver also needs a clear_with_quad implementation for > non-fast-cleared > >> > buffers. :/ > >> > >> hmm, so perhaps one easy option is to change pctx->clear() to return a > >> boolean, so driver can return false to ask the state tracker to do a > > > > > > Ideally all pipe context functions should always return void to allow > gallium multithreading. > > Hmm, that is a bit awkward.. and a pipe cap is a bit restrictive, ie. > say you can fast-clear only certain color formats, or other weird > combination of restrictions. > > I suppose a pctx->can_do_clear(..) which isn't multithreaded would be > a bit more flexible than pipe caps. > can_do_clear(..) should be in pipe_screen. I wouldn't like to have context functions that return a value. Marek > > A pipe cap for colormasked and scissored clears would be better. > > > > > >> clear_with_quad().. maybe that would be a first step towards allowing > >> driver to handle clears w/ colormask and possibly scissor (although > >> for the later, plus > >> glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking > >> of pctx->invalidate_surface()/pctx->invalidate_sub_surface()) > > > > > > Pipe context already contains an invalidate function. > > yeah, but it is at the level of pipe_resource, which isn't really what > you want if only a certain level/layer of a resource is invalidated. > > I'm still unsure about how I want to track this in the driver, the > ideal case would be to track it in fd_surface, except that surfaces > tend to be created transiently. Still for glInvalidateFramebuffer() > and friends, having an pctx->invalidate_surface() seems like the > cleaner API. > > BR, > -R > > > > > Marek > > > >> > >> But either way, I guess this patch is a simple stop-gap solution. > >> > >> BR, > >> -R > >> ___ > >> mesa-dev mailing list > >> mesa-dev@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Wed, Nov 14, 2018 at 6:32 AM Erik Faye-Lund wrote: > > On Tue, 2018-11-13 at 19:07 -0500, Ilia Mirkin wrote: > > On Tue, Nov 13, 2018 at 6:50 PM Rob Clark > > wrote: > > > On Tue, Nov 13, 2018 at 6:19 PM Eric Anholt > > > wrote: > > > > Rob Clark writes: > > > > > > > > > On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt > > > > > wrote: > > > > > > Rob Clark writes: > > > > > > > > > > > > > If we can't clear all the buffers with pctx->clear() (say, > > > > > > > for example, > > > > > > > because of ColorMask), push the buffers we *can* clear with > > > > > > > pctx->clear() > > > > > > > first. Tilers want to see clears coming before draws to > > > > > > > enable fast- > > > > > > > paths, and clearing one of the attachments with a quad-draw > > > > > > > first > > > > > > > confuses that logic. > > > > > > > > > > > > Oh, nice! > > > > > > > > > > > > Reviewed-by: Eric Anholt > > > > > > > > > > > > Though it feels pretty silly that the ->clear() caller needs > > > > > > a > > > > > > clear_with_quad implementation when the ->clear() > > > > > > implementation in the > > > > > > driver also needs a clear_with_quad implementation for non- > > > > > > fast-cleared > > > > > > buffers. :/ > > > > > > > > > > hmm, so perhaps one easy option is to change pctx->clear() to > > > > > return a > > > > > boolean, so driver can return false to ask the state tracker to > > > > > do a > > > > > clear_with_quad().. maybe that would be a first step towards > > > > > allowing > > > > > driver to handle clears w/ colormask and possibly scissor > > > > > (although > > > > > for the later, plus > > > > > glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was > > > > > thinking > > > > > of pctx->invalidate_surface()/pctx->invalidate_sub_surface()). > > > > > > > > I was thinking you'd return the mask of what buffers you couldn't > > > > (fast) > > > > clear. > > > > > > yeah, makes sense.. I kinda came to same conclusion when I started > > > thinking some drivers might not want us to split up the clear per > > > attachment.. still not quite sure about adding scissor/colormask, > > > might end up needing a pipe cap so st_Clear() would know to flush > > > the > > > corresponding state down to driver. I guess low hanging fruit is > > > to > > > not change the definition of pctx->clear() but just let driver ask > > > for > > > fallback path for some/all attachments. > > > > You could also create a pipe_clear_info which would take that data > > directly and let the driver worry about it. FWIW, nvidia command > > stream clears can take into account stencil, scissors, window > > rectangles, color masks - maybe everything that st_Clear needs to > > worry about. It never seemed important enough to address myself, but > > I'll happily go along for the ride. > > Just another suggestion in the bag: > > Vulkan supports three kinds of clears: > - Render pass clears; used at the start of a render pass, no > scissoring or color-masking. Just an aside, I've seen things (like CrOS compositor) which seem to do a scissored clear, followed by rendering. So the clear is still at the start of the render pass. And the driver wants to be aware of this to know what tiles it does not need to bring back into tile buffer from system memory. This is one clear related optimization that I am working on. (I believe it keeps the same scissor in place for rendering, so to a certain extent I think I can adjust the coordinates of the tiles to minimize the # of pixels that need to be brought back into the tile buffer.) BR, -R > - vkCmdClearAttachments: used inside render-passes, allows to specify > an arbitrary number of scissor-rectangles, no color-maksing. > - vkCmdClearColorImage / vkCmdClearDepthStencilImage: used outside > render-passes, no scissoring or color-masking. > > All of the Vulkan clears are "per-attachment" in a sense; > - Render pass clears / vkCmdClearAttachments can select any of the > attachments in the current framebuffer > - vkCmdClearColorImage / vkCmdClearDepthStencilImage can clear > individual images, regardless of framebuffers. > > The reason I'm bringing this up, is that I assume the Vulkan WG has had > long discussions about how to efficiently support common needs for > clears on all relevant hardware. > > It seems a bit logical (to me, at least) that a color-masked clear is > always a quad. Color masking makes it not really a clear-operation, but > more like a strange blend, and color-masking is a graphics-pipeline > operation. > > I doubt we'd ever care about stencil, as even GL doesn't. > > So: > - pipe_context::clear_render_target corresponds to > vkCmdClearColorImage (with the exception that clear_render_target can > specify a rectangle). > - pipe_context::clear_depth_stencil corresponds to > vkCmdClearDepthStencilImage (with the exception that > clear_depth_stencil can specify a rectangle). > - pipe_context::clear corresponds to render pass clears > > I think the only case we
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Tue, Nov 13, 2018 at 9:54 PM Marek Olšák wrote: > > > > On Tue, Nov 13, 2018, 6:00 PM Rob Clark > >> On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: >> > >> > Rob Clark writes: >> > >> > > If we can't clear all the buffers with pctx->clear() (say, for example, >> > > because of ColorMask), push the buffers we *can* clear with pctx->clear() >> > > first. Tilers want to see clears coming before draws to enable fast- >> > > paths, and clearing one of the attachments with a quad-draw first >> > > confuses that logic. >> > >> > Oh, nice! >> > >> > Reviewed-by: Eric Anholt >> > >> > Though it feels pretty silly that the ->clear() caller needs a >> > clear_with_quad implementation when the ->clear() implementation in the >> > driver also needs a clear_with_quad implementation for non-fast-cleared >> > buffers. :/ >> >> hmm, so perhaps one easy option is to change pctx->clear() to return a >> boolean, so driver can return false to ask the state tracker to do a > > > Ideally all pipe context functions should always return void to allow gallium > multithreading. Hmm, that is a bit awkward.. and a pipe cap is a bit restrictive, ie. say you can fast-clear only certain color formats, or other weird combination of restrictions. I suppose a pctx->can_do_clear(..) which isn't multithreaded would be a bit more flexible than pipe caps. > A pipe cap for colormasked and scissored clears would be better. > > >> clear_with_quad().. maybe that would be a first step towards allowing >> driver to handle clears w/ colormask and possibly scissor (although >> for the later, plus >> glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking >> of pctx->invalidate_surface()/pctx->invalidate_sub_surface()) > > > Pipe context already contains an invalidate function. yeah, but it is at the level of pipe_resource, which isn't really what you want if only a certain level/layer of a resource is invalidated. I'm still unsure about how I want to track this in the driver, the ideal case would be to track it in fd_surface, except that surfaces tend to be created transiently. Still for glInvalidateFramebuffer() and friends, having an pctx->invalidate_surface() seems like the cleaner API. BR, -R > > Marek > >> >> But either way, I guess this patch is a simple stop-gap solution. >> >> BR, >> -R >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Tue, 2018-11-13 at 21:53 -0500, Marek Olšák wrote: > > > On Tue, Nov 13, 2018, 6:00 PM Rob Clark > On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt > > wrote: > > > > > > Rob Clark writes: > > > > > > > If we can't clear all the buffers with pctx->clear() (say, for > > example, > > > > because of ColorMask), push the buffers we *can* clear with > > pctx->clear() > > > > first. Tilers want to see clears coming before draws to enable > > fast- > > > > paths, and clearing one of the attachments with a quad-draw > > first > > > > confuses that logic. > > > > > > Oh, nice! > > > > > > Reviewed-by: Eric Anholt > > > > > > Though it feels pretty silly that the ->clear() caller needs a > > > clear_with_quad implementation when the ->clear() implementation > > in the > > > driver also needs a clear_with_quad implementation for non-fast- > > cleared > > > buffers. :/ > > > > hmm, so perhaps one easy option is to change pctx->clear() to > > return a > > boolean, so driver can return false to ask the state tracker to do > > a > > Ideally all pipe context functions should always return void to allow > gallium multithreading. > > A pipe cap for colormasked and scissored clears would be better. > I have an ancient patch for the latter lying around: https://github.com/kusma/mesa/commit/d6be35297ffaa5eeb912e849e55a22ff4ea03868 > > clear_with_quad().. maybe that would be a first step towards > > allowing > > driver to handle clears w/ colormask and possibly scissor (although > > for the later, plus > > glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was > > thinking > > of pctx->invalidate_surface()/pctx->invalidate_sub_surface()) > > Pipe context already contains an invalidate function. > > Marek > > > But either way, I guess this patch is a simple stop-gap solution. > > > > BR, > > -R > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Tue, 2018-11-13 at 19:07 -0500, Ilia Mirkin wrote: > On Tue, Nov 13, 2018 at 6:50 PM Rob Clark > wrote: > > On Tue, Nov 13, 2018 at 6:19 PM Eric Anholt > > wrote: > > > Rob Clark writes: > > > > > > > On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt > > > > wrote: > > > > > Rob Clark writes: > > > > > > > > > > > If we can't clear all the buffers with pctx->clear() (say, > > > > > > for example, > > > > > > because of ColorMask), push the buffers we *can* clear with > > > > > > pctx->clear() > > > > > > first. Tilers want to see clears coming before draws to > > > > > > enable fast- > > > > > > paths, and clearing one of the attachments with a quad-draw > > > > > > first > > > > > > confuses that logic. > > > > > > > > > > Oh, nice! > > > > > > > > > > Reviewed-by: Eric Anholt > > > > > > > > > > Though it feels pretty silly that the ->clear() caller needs > > > > > a > > > > > clear_with_quad implementation when the ->clear() > > > > > implementation in the > > > > > driver also needs a clear_with_quad implementation for non- > > > > > fast-cleared > > > > > buffers. :/ > > > > > > > > hmm, so perhaps one easy option is to change pctx->clear() to > > > > return a > > > > boolean, so driver can return false to ask the state tracker to > > > > do a > > > > clear_with_quad().. maybe that would be a first step towards > > > > allowing > > > > driver to handle clears w/ colormask and possibly scissor > > > > (although > > > > for the later, plus > > > > glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was > > > > thinking > > > > of pctx->invalidate_surface()/pctx->invalidate_sub_surface()). > > > > > > I was thinking you'd return the mask of what buffers you couldn't > > > (fast) > > > clear. > > > > yeah, makes sense.. I kinda came to same conclusion when I started > > thinking some drivers might not want us to split up the clear per > > attachment.. still not quite sure about adding scissor/colormask, > > might end up needing a pipe cap so st_Clear() would know to flush > > the > > corresponding state down to driver. I guess low hanging fruit is > > to > > not change the definition of pctx->clear() but just let driver ask > > for > > fallback path for some/all attachments. > > You could also create a pipe_clear_info which would take that data > directly and let the driver worry about it. FWIW, nvidia command > stream clears can take into account stencil, scissors, window > rectangles, color masks - maybe everything that st_Clear needs to > worry about. It never seemed important enough to address myself, but > I'll happily go along for the ride. Just another suggestion in the bag: Vulkan supports three kinds of clears: - Render pass clears; used at the start of a render pass, no scissoring or color-masking. - vkCmdClearAttachments: used inside render-passes, allows to specify an arbitrary number of scissor-rectangles, no color-maksing. - vkCmdClearColorImage / vkCmdClearDepthStencilImage: used outside render-passes, no scissoring or color-masking. All of the Vulkan clears are "per-attachment" in a sense; - Render pass clears / vkCmdClearAttachments can select any of the attachments in the current framebuffer - vkCmdClearColorImage / vkCmdClearDepthStencilImage can clear individual images, regardless of framebuffers. The reason I'm bringing this up, is that I assume the Vulkan WG has had long discussions about how to efficiently support common needs for clears on all relevant hardware. It seems a bit logical (to me, at least) that a color-masked clear is always a quad. Color masking makes it not really a clear-operation, but more like a strange blend, and color-masking is a graphics-pipeline operation. I doubt we'd ever care about stencil, as even GL doesn't. So: - pipe_context::clear_render_target corresponds to vkCmdClearColorImage (with the exception that clear_render_target can specify a rectangle). - pipe_context::clear_depth_stencil corresponds to vkCmdClearDepthStencilImage (with the exception that clear_depth_stencil can specify a rectangle). - pipe_context::clear corresponds to render pass clears I think the only case we currently miss compared to what Vulkan provides is a non-quad version of scissored clear (vkCmdClearAttachments). So perhaps we should add support for that somehow? Apart from that, it sounds like the suggestion of returning a mask and falling back ti quads for harware that can't support this is a good suggestion to me. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Tue, Nov 13, 2018, 6:00 PM Rob Clark On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: > > > > Rob Clark writes: > > > > > If we can't clear all the buffers with pctx->clear() (say, for example, > > > because of ColorMask), push the buffers we *can* clear with > pctx->clear() > > > first. Tilers want to see clears coming before draws to enable fast- > > > paths, and clearing one of the attachments with a quad-draw first > > > confuses that logic. > > > > Oh, nice! > > > > Reviewed-by: Eric Anholt > > > > Though it feels pretty silly that the ->clear() caller needs a > > clear_with_quad implementation when the ->clear() implementation in the > > driver also needs a clear_with_quad implementation for non-fast-cleared > > buffers. :/ > > hmm, so perhaps one easy option is to change pctx->clear() to return a > boolean, so driver can return false to ask the state tracker to do a > Ideally all pipe context functions should always return void to allow gallium multithreading. A pipe cap for colormasked and scissored clears would be better. clear_with_quad().. maybe that would be a first step towards allowing > driver to handle clears w/ colormask and possibly scissor (although > for the later, plus > glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking > of pctx->invalidate_surface()/pctx->invalidate_sub_surface()) Pipe context already contains an invalidate function. Marek > But either way, I guess this patch is a simple stop-gap solution. > > BR, > -R > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Tue, Nov 13, 2018 at 6:50 PM Rob Clark wrote: > > On Tue, Nov 13, 2018 at 6:19 PM Eric Anholt wrote: > > > > Rob Clark writes: > > > > > On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: > > >> > > >> Rob Clark writes: > > >> > > >> > If we can't clear all the buffers with pctx->clear() (say, for example, > > >> > because of ColorMask), push the buffers we *can* clear with > > >> > pctx->clear() > > >> > first. Tilers want to see clears coming before draws to enable fast- > > >> > paths, and clearing one of the attachments with a quad-draw first > > >> > confuses that logic. > > >> > > >> Oh, nice! > > >> > > >> Reviewed-by: Eric Anholt > > >> > > >> Though it feels pretty silly that the ->clear() caller needs a > > >> clear_with_quad implementation when the ->clear() implementation in the > > >> driver also needs a clear_with_quad implementation for non-fast-cleared > > >> buffers. :/ > > > > > > hmm, so perhaps one easy option is to change pctx->clear() to return a > > > boolean, so driver can return false to ask the state tracker to do a > > > clear_with_quad().. maybe that would be a first step towards allowing > > > driver to handle clears w/ colormask and possibly scissor (although > > > for the later, plus > > > glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking > > > of pctx->invalidate_surface()/pctx->invalidate_sub_surface()). > > > > I was thinking you'd return the mask of what buffers you couldn't (fast) > > clear. > > yeah, makes sense.. I kinda came to same conclusion when I started > thinking some drivers might not want us to split up the clear per > attachment.. still not quite sure about adding scissor/colormask, > might end up needing a pipe cap so st_Clear() would know to flush the > corresponding state down to driver. I guess low hanging fruit is to > not change the definition of pctx->clear() but just let driver ask for > fallback path for some/all attachments. You could also create a pipe_clear_info which would take that data directly and let the driver worry about it. FWIW, nvidia command stream clears can take into account stencil, scissors, window rectangles, color masks - maybe everything that st_Clear needs to worry about. It never seemed important enough to address myself, but I'll happily go along for the ride. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Tue, Nov 13, 2018 at 6:19 PM Eric Anholt wrote: > > Rob Clark writes: > > > On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: > >> > >> Rob Clark writes: > >> > >> > If we can't clear all the buffers with pctx->clear() (say, for example, > >> > because of ColorMask), push the buffers we *can* clear with pctx->clear() > >> > first. Tilers want to see clears coming before draws to enable fast- > >> > paths, and clearing one of the attachments with a quad-draw first > >> > confuses that logic. > >> > >> Oh, nice! > >> > >> Reviewed-by: Eric Anholt > >> > >> Though it feels pretty silly that the ->clear() caller needs a > >> clear_with_quad implementation when the ->clear() implementation in the > >> driver also needs a clear_with_quad implementation for non-fast-cleared > >> buffers. :/ > > > > hmm, so perhaps one easy option is to change pctx->clear() to return a > > boolean, so driver can return false to ask the state tracker to do a > > clear_with_quad().. maybe that would be a first step towards allowing > > driver to handle clears w/ colormask and possibly scissor (although > > for the later, plus > > glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking > > of pctx->invalidate_surface()/pctx->invalidate_sub_surface()). > > I was thinking you'd return the mask of what buffers you couldn't (fast) > clear. yeah, makes sense.. I kinda came to same conclusion when I started thinking some drivers might not want us to split up the clear per attachment.. still not quite sure about adding scissor/colormask, might end up needing a pipe cap so st_Clear() would know to flush the corresponding state down to driver. I guess low hanging fruit is to not change the definition of pctx->clear() but just let driver ask for fallback path for some/all attachments. BR, -R ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
Rob Clark writes: > On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: >> >> Rob Clark writes: >> >> > If we can't clear all the buffers with pctx->clear() (say, for example, >> > because of ColorMask), push the buffers we *can* clear with pctx->clear() >> > first. Tilers want to see clears coming before draws to enable fast- >> > paths, and clearing one of the attachments with a quad-draw first >> > confuses that logic. >> >> Oh, nice! >> >> Reviewed-by: Eric Anholt >> >> Though it feels pretty silly that the ->clear() caller needs a >> clear_with_quad implementation when the ->clear() implementation in the >> driver also needs a clear_with_quad implementation for non-fast-cleared >> buffers. :/ > > hmm, so perhaps one easy option is to change pctx->clear() to return a > boolean, so driver can return false to ask the state tracker to do a > clear_with_quad().. maybe that would be a first step towards allowing > driver to handle clears w/ colormask and possibly scissor (although > for the later, plus > glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking > of pctx->invalidate_surface()/pctx->invalidate_sub_surface()). I was thinking you'd return the mask of what buffers you couldn't (fast) clear. signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt wrote: > > Rob Clark writes: > > > If we can't clear all the buffers with pctx->clear() (say, for example, > > because of ColorMask), push the buffers we *can* clear with pctx->clear() > > first. Tilers want to see clears coming before draws to enable fast- > > paths, and clearing one of the attachments with a quad-draw first > > confuses that logic. > > Oh, nice! > > Reviewed-by: Eric Anholt > > Though it feels pretty silly that the ->clear() caller needs a > clear_with_quad implementation when the ->clear() implementation in the > driver also needs a clear_with_quad implementation for non-fast-cleared > buffers. :/ hmm, so perhaps one easy option is to change pctx->clear() to return a boolean, so driver can return false to ask the state tracker to do a clear_with_quad().. maybe that would be a first step towards allowing driver to handle clears w/ colormask and possibly scissor (although for the later, plus glInvalidateFramebuffer()/glInvalidateSubFramebuffer(), I was thinking of pctx->invalidate_surface()/pctx->invalidate_sub_surface()). But either way, I guess this patch is a simple stop-gap solution. BR, -R ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
Rob Clark writes: > If we can't clear all the buffers with pctx->clear() (say, for example, > because of ColorMask), push the buffers we *can* clear with pctx->clear() > first. Tilers want to see clears coming before draws to enable fast- > paths, and clearing one of the attachments with a quad-draw first > confuses that logic. Oh, nice! Reviewed-by: Eric Anholt Though it feels pretty silly that the ->clear() caller needs a clear_with_quad implementation when the ->clear() implementation in the driver also needs a clear_with_quad implementation for non-fast-cleared buffers. :/ signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()
If we can't clear all the buffers with pctx->clear() (say, for example,
because of ColorMask), push the buffers we *can* clear with pctx->clear()
first. Tilers want to see clears coming before draws to enable fast-
paths, and clearing one of the attachments with a quad-draw first
confuses that logic.
Signed-off-by: Rob Clark
---
src/mesa/state_tracker/st_cb_clear.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/mesa/state_tracker/st_cb_clear.c
b/src/mesa/state_tracker/st_cb_clear.c
index 22e85019764..3b51bd2c8a7 100644
--- a/src/mesa/state_tracker/st_cb_clear.c
+++ b/src/mesa/state_tracker/st_cb_clear.c
@@ -442,9 +442,6 @@ st_Clear(struct gl_context *ctx, GLbitfield mask)
* use pipe->clear. We want to always use pipe->clear for the other
* renderbuffers, because it's likely to be faster.
*/
- if (quad_buffers) {
- clear_with_quad(ctx, quad_buffers);
- }
if (clear_buffers) {
/* We can't translate the clear color to the colorbuffer format,
* because different colorbuffers may have different formats.
@@ -453,6 +450,9 @@ st_Clear(struct gl_context *ctx, GLbitfield mask)
(union pipe_color_union*)>Color.ClearColor,
ctx->Depth.Clear, ctx->Stencil.Clear);
}
+ if (quad_buffers) {
+ clear_with_quad(ctx, quad_buffers);
+ }
if (mask & BUFFER_BIT_ACCUM)
_mesa_clear_accum_buffer(ctx);
}
--
2.19.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
