[Mesa-dev] [PATCH v2 3/6] gallium: add pipe->invalidate_surface()

[Marek Olšák]

The way I see it, pipe_surface is a render target view for
pipe_framebuffer_state. The fact that other functions also accept
pipe_surface is a historic artifact. resource_copy_region used to accept 2
pipe_surfaces before it was changed to 2 pipe_resources + parameters.
Somebody might decide to change invalidate_subresource to take
pipe_resource in the future.

It seems to me that you really want e.g. invalidate_framebuffer(ctx,
PIPE_FRAMEBUFFER_COLOR0 | PIPE_FRAMEBUFFER_COLOR1), not pipe_surface.

I'm not too crazy about this, just pointing out the way I understand the
conventions.

Marek

On Fri, Dec 14, 2018 at 3:51 PM Rob Clark <robdclark at gmail.com> wrote:

> I don't mind renaming it to invalidate_subsurface() if you prefer
> that, but I really prefer that it takes a surface rather than a
> resource... pipe_resource is the wrong level to do this.
>
> Even though surfaces are transient, tracking the invalidate state in
> the surface works out much easier in the driver.  And I don't want to
> have to map resource+params back to a surface.  If another driver
> wants to track this at the resource level, it is easier for them to go
> from surface to resource+params, compared to the opposite direction.
>
> BR,
> -R
>
>
> On Fri, Dec 14, 2018 at 3:38 PM Marek Olšák <maraeo at gmail.com> wrote:
> >
> > Can you please call it invalidate_subresource and inline relevant
> pipe_surface variables inside the parameters?
> >
> > Thanks,
> > Marek
> >
> > On Wed, Dec 12, 2018 at 10:48 AM Rob Clark <robdclark at gmail.com> wrote:
> >>
> >> A new API to implement glInvalidateFramebuffer() and friends.  It is
> >> similar to invalidate_resource() but can be used to invalidate a
> >> specific layer/level, so it is suitable to use for user FBOs in addition
> >> to window system framebuffer.
> >>
> >> Signed-off-by: Rob Clark <robdclark at gmail.com>
> >> ---
> >>  src/gallium/docs/source/context.rst  | 18 ++++++++++++++++++
> >>  src/gallium/include/pipe/p_context.h | 18 ++++++++++++++++++
> >>  2 files changed, 36 insertions(+)
> >>
> >> diff --git a/src/gallium/docs/source/context.rst
> b/src/gallium/docs/source/context.rst
> >> index 20d0df79312..9f3cd49ee31 100644
> >> --- a/src/gallium/docs/source/context.rst
> >> +++ b/src/gallium/docs/source/context.rst
> >> @@ -740,7 +740,25 @@ of the buffer is not a multiple of the page size,
> changing the commit state of
> >>  the last (partial) page requires a box that ends at the end of the
> buffer
> >>  (i.e., box->x + box->width == buffer->width0).
> >>
> >> +.. _invalidate_surface:
> >>
> >> +invalidate_surface
> >> +%%%%%%%%%%%%%%%%%%
> >> +
> >> +This optional function marks a surface, or a portion of a surface, as
> invalid,
> >> +for example to implement glInvalidateFramebuffer() (and friends).  It
> is
> >> +useful in particular for tiler GPUs, as a way to avoid unnecessary
> transfers
> >> +between system memory and tile buffer.
> >> +
> >> +For example, invalidating a surface after rendering, but before flush,
> can
> >> +be used to avoid tile to system memory transfer (for example, if zs
> can be
> >> +discarded).  And an invalidate after flush before rendering can be
> used to
> >> +avoid a system memory to tile buffer transfer.
> >> +
> >> +Invalidating a partial surface can also be used to avoid unnecessary
> transfer
> >> +from system memory to tile buffer in the case of a scissored clear
> (which is
> >> +implemented via ->draw_vbo() by the state tracker) by invalidating the
> >> +scissored region.
> >>
> >>  .. _pipe_transfer:
> >>
> >> diff --git a/src/gallium/include/pipe/p_context.h
> b/src/gallium/include/pipe/p_context.h
> >> index e07b76d4f03..4dfc87faff6 100644
> >> --- a/src/gallium/include/pipe/p_context.h
> >> +++ b/src/gallium/include/pipe/p_context.h
> >> @@ -803,6 +803,24 @@ struct pipe_context {
> >>     void (*invalidate_resource)(struct pipe_context *ctx,
> >>                                 struct pipe_resource *resource);
> >>
> >> +   /**
> >> +    * Like ->invalidate_resource, but can invalidate a specific layer
> and level
> >> +    * of a resource, and optionally a specific sub-region of the
> resource (if
> >> +    * region is not NULL).
> >> +    *
> >> +    * If the backing surf->texture has just a single layer and level
> (like
> >> +    * window system buffers), and region is NULL, it is equivalent to
> >> +    * ->invalidate_resource().
> >> +    *
> >> +    * \param ctx    pipe context
> >> +    * \param surf   surface to invalidate
> >> +    * \param region NULL to invalidate whole surface, otherwise
> specifies which
> >> +    *               portion of the surface is invalidated
> >> +    */
> >> +   void (*invalidate_surface)(struct pipe_context *ctx,
> >> +                              struct pipe_surface *surf,
> >> +                              const struct pipe_box *region);
> >> +
> >>     /**
> >>      * Return information about unexpected device resets.
> >>      */
> >> --
> >> 2.19.2
> >>
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181214/f1a956a6/attachment.html>