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

[Erik Faye-Lund]

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.
- 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.