[Mesa-dev] [PATCH] mesa/st: swap order of clear() and clear_with_quad()

[Rob Clark]

On Wed, Nov 14, 2018 at 6:32 AM Erik Faye-Lund
<erik.faye-lund at collabora.com> wrote:
>
> On Tue, 2018-11-13 at 19:07 -0500, Ilia Mirkin wrote:
> > On Tue, Nov 13, 2018 at 6:50 PM Rob Clark <robdclark at gmail.com>
> > wrote:
> > > On Tue, Nov 13, 2018 at 6:19 PM Eric Anholt <eric at anholt.net>
> > > wrote:
> > > > Rob Clark <robdclark at gmail.com> writes:
> > > >
> > > > > On Tue, Nov 13, 2018 at 5:25 PM Eric Anholt <eric at anholt.net>
> > > > > wrote:
> > > > > > Rob Clark <robdclark at gmail.com> 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 <eric at anholt.net>
> > > > > >
> > > > > > 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 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.
>