[RFC,1/2] dri: Let drivers reset the damage region

Submitted by Boris Brezillon on Aug. 29, 2019, 11:54 a.m.

Details

Message ID 20190829115416.15775-1-boris.brezillon@collabora.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon Aug. 29, 2019, 11:54 a.m.
dri2_set_damage_region() must call st_validate_state(UPDATE_FRAMEBUFFER)
to make sure the BACK_LEFT attachment is up-to-date.
Problem is, dri2_swap_buffers_xxx() functions are actually targeting
the old backbuffer when they call ->set_damage_region(0, NULL), and
more importantly, they are not expecting the caller to pull a new back
buffer at this point, which will happen if st_validate_state() is
called.

Let's delegate the damage region reset to drivers (they should take
care of that at flush/swap time) so we don't have to deal with this
extra complexity at the EGL level.

The only gallium driver implementing ->set_damage_region() is patched
accordingly.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 include/GL/internal/dri_interface.h         |  4 ++++
 src/egl/drivers/dri2/egl_dri2.c             | 24 ---------------------
 src/gallium/drivers/panfrost/pan_context.c  | 14 +++++++++++-
 src/gallium/drivers/panfrost/pan_resource.c |  2 +-
 src/gallium/drivers/panfrost/pan_resource.h |  2 ++
 5 files changed, 20 insertions(+), 26 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
index 4b5583820ed0..4c60d349ddd5 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -521,6 +521,10 @@  struct __DRI2bufferDamageExtensionRec {
     * reset the region, followed by a second call with a populated region,
     * before a rendering call is made.
     *
+    * Drivers implementing this entrypoint should take care of resetting the
+    * damage region of resources being written to at swapbuffer/flush time.
+    * The DRI core will not automate that for you.
+    *
     * Used to implement EGL_KHR_partial_update.
     *
     * \param drawable affected drawable
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 526cb1969c18..ea8ae5d44c56 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1767,7 +1767,6 @@  static EGLBoolean
 dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
 {
    struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
-   __DRIdrawable *dri_drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
    _EGLContext *ctx = _eglGetCurrentContext();
    EGLBoolean ret;
 
@@ -1775,13 +1774,6 @@  dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
       dri2_surf_update_fence_fd(ctx, disp, surf);
    ret = dri2_dpy->vtbl->swap_buffers(drv, disp, surf);
 
-   /* SwapBuffers marks the end of the frame; reset the damage region for
-    * use again next time.
-    */
-   if (ret && dri2_dpy->buffer_damage &&
-       dri2_dpy->buffer_damage->set_damage_region)
-      dri2_dpy->buffer_damage->set_damage_region(dri_drawable, 0, NULL);
-
    return ret;
 }
 
@@ -1791,7 +1783,6 @@  dri2_swap_buffers_with_damage(_EGLDriver *drv, _EGLDisplay *disp,
                               const EGLint *rects, EGLint n_rects)
 {
    struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
-   __DRIdrawable *dri_drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
    _EGLContext *ctx = _eglGetCurrentContext();
    EGLBoolean ret;
 
@@ -1800,13 +1791,6 @@  dri2_swap_buffers_with_damage(_EGLDriver *drv, _EGLDisplay *disp,
    ret = dri2_dpy->vtbl->swap_buffers_with_damage(drv, disp, surf,
                                                   rects, n_rects);
 
-   /* SwapBuffers marks the end of the frame; reset the damage region for
-    * use again next time.
-    */
-   if (ret && dri2_dpy->buffer_damage &&
-       dri2_dpy->buffer_damage->set_damage_region)
-      dri2_dpy->buffer_damage->set_damage_region(dri_drawable, 0, NULL);
-
    return ret;
 }
 
@@ -1815,18 +1799,10 @@  dri2_swap_buffers_region(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf,
                          EGLint numRects, const EGLint *rects)
 {
    struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
-   __DRIdrawable *dri_drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
    EGLBoolean ret;
 
    ret = dri2_dpy->vtbl->swap_buffers_region(drv, disp, surf, numRects, rects);
 
-   /* SwapBuffers marks the end of the frame; reset the damage region for
-    * use again next time.
-    */
-   if (ret && dri2_dpy->buffer_damage &&
-       dri2_dpy->buffer_damage->set_damage_region)
-      dri2_dpy->buffer_damage->set_damage_region(dri_drawable, 0, NULL);
-
    return ret;
 }
 
diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
index fa9c92af9f6a..4069ec238fe4 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -1461,7 +1461,8 @@  panfrost_flush(
         struct panfrost_job *job = panfrost_get_job_for_fbo(ctx);
 
         /* Nothing to do! */
-        if (!job->last_job.gpu && !job->clear) return;
+        if (!job->last_job.gpu && !job->clear)
+                goto reset_damage;
 
         if (!job->clear && job->last_tiler.gpu)
                 panfrost_draw_wallpaper(&ctx->base);
@@ -1476,6 +1477,17 @@  panfrost_flush(
 
         /* Prepare for the next frame */
         panfrost_invalidate_frame(ctx);
+
+reset_damage:
+        /* Reset the damage region on all color bufs, so we can start from a
+         * fresh state next time those resources are used.
+         */
+        for (unsigned i = 0; i < ARRAY_SIZE(job->key.cbufs); i++) {
+                struct pipe_surface *surf = job->key.cbufs[i];
+
+                if (surf)
+                        panfrost_resource_reset_damage(pan_resource(surf->texture));
+        }
 }
 
 #define DEFINE_CASE(c) case PIPE_PRIM_##c: return MALI_##c;
diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
index 57df61cb71cc..91beebaf8ccd 100644
--- a/src/gallium/drivers/panfrost/pan_resource.c
+++ b/src/gallium/drivers/panfrost/pan_resource.c
@@ -47,7 +47,7 @@ 
 #include "pan_util.h"
 #include "pan_tiling.h"
 
-static void
+void
 panfrost_resource_reset_damage(struct panfrost_resource *pres)
 {
         /* We set the damage extent to the full resource size but keep the
diff --git a/src/gallium/drivers/panfrost/pan_resource.h b/src/gallium/drivers/panfrost/pan_resource.h
index 6ed3d1fd60e8..091fc36d17ce 100644
--- a/src/gallium/drivers/panfrost/pan_resource.h
+++ b/src/gallium/drivers/panfrost/pan_resource.h
@@ -141,6 +141,8 @@  void
 panfrost_blit_wallpaper(struct panfrost_context *ctx,
                         struct pipe_box *box);
 
+void
+panfrost_resource_reset_damage(struct panfrost_resource *pres);
 void
 panfrost_resource_set_damage_region(struct pipe_screen *screen,
                                     struct pipe_resource *res,

Comments

On Thu, 29 Aug 2019 13:54:15 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> dri2_set_damage_region() must call st_validate_state(UPDATE_FRAMEBUFFER)
> to make sure the BACK_LEFT attachment is up-to-date.
> Problem is, dri2_swap_buffers_xxx() functions are actually targeting
> the old backbuffer when they call ->set_damage_region(0, NULL), and
> more importantly, they are not expecting the caller to pull a new back
> buffer at this point, which will happen if st_validate_state() is
> called.
> 
> Let's delegate the damage region reset to drivers (they should take
> care of that at flush/swap time) so we don't have to deal with this
> extra complexity at the EGL level.
> 
> The only gallium driver implementing ->set_damage_region() is patched
> accordingly.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  include/GL/internal/dri_interface.h         |  4 ++++
>  src/egl/drivers/dri2/egl_dri2.c             | 24 ---------------------
>  src/gallium/drivers/panfrost/pan_context.c  | 14 +++++++++++-
>  src/gallium/drivers/panfrost/pan_resource.c |  2 +-
>  src/gallium/drivers/panfrost/pan_resource.h |  2 ++
>  5 files changed, 20 insertions(+), 26 deletions(-)
> 
> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> index 4b5583820ed0..4c60d349ddd5 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -521,6 +521,10 @@ struct __DRI2bufferDamageExtensionRec {
>      * reset the region, followed by a second call with a populated region,
>      * before a rendering call is made.
>      *
> +    * Drivers implementing this entrypoint should take care of resetting the
> +    * damage region of resources being written to at swapbuffer/flush time.
> +    * The DRI core will not automate that for you.
> +    *
>      * Used to implement EGL_KHR_partial_update.
>      *
>      * \param drawable affected drawable
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 526cb1969c18..ea8ae5d44c56 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1767,7 +1767,6 @@ static EGLBoolean
>  dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> -   __DRIdrawable *dri_drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
>     _EGLContext *ctx = _eglGetCurrentContext();
>     EGLBoolean ret;
>  
> @@ -1775,13 +1774,6 @@ dri2_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
>        dri2_surf_update_fence_fd(ctx, disp, surf);
>     ret = dri2_dpy->vtbl->swap_buffers(drv, disp, surf);
>  
> -   /* SwapBuffers marks the end of the frame; reset the damage region for
> -    * use again next time.
> -    */
> -   if (ret && dri2_dpy->buffer_damage &&
> -       dri2_dpy->buffer_damage->set_damage_region)
> -      dri2_dpy->buffer_damage->set_damage_region(dri_drawable, 0, NULL);
> -
>     return ret;
>  }
>  
> @@ -1791,7 +1783,6 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv, _EGLDisplay *disp,
>                                const EGLint *rects, EGLint n_rects)
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> -   __DRIdrawable *dri_drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
>     _EGLContext *ctx = _eglGetCurrentContext();
>     EGLBoolean ret;
>  
> @@ -1800,13 +1791,6 @@ dri2_swap_buffers_with_damage(_EGLDriver *drv, _EGLDisplay *disp,
>     ret = dri2_dpy->vtbl->swap_buffers_with_damage(drv, disp, surf,
>                                                    rects, n_rects);
>  
> -   /* SwapBuffers marks the end of the frame; reset the damage region for
> -    * use again next time.
> -    */
> -   if (ret && dri2_dpy->buffer_damage &&
> -       dri2_dpy->buffer_damage->set_damage_region)
> -      dri2_dpy->buffer_damage->set_damage_region(dri_drawable, 0, NULL);
> -
>     return ret;
>  }
>  
> @@ -1815,18 +1799,10 @@ dri2_swap_buffers_region(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf,
>                           EGLint numRects, const EGLint *rects)
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> -   __DRIdrawable *dri_drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
>     EGLBoolean ret;
>  
>     ret = dri2_dpy->vtbl->swap_buffers_region(drv, disp, surf, numRects, rects);
>  
> -   /* SwapBuffers marks the end of the frame; reset the damage region for
> -    * use again next time.
> -    */
> -   if (ret && dri2_dpy->buffer_damage &&
> -       dri2_dpy->buffer_damage->set_damage_region)
> -      dri2_dpy->buffer_damage->set_damage_region(dri_drawable, 0, NULL);
> -
>     return ret;
>  }
>  
> diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
> index fa9c92af9f6a..4069ec238fe4 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -1461,7 +1461,8 @@ panfrost_flush(
>          struct panfrost_job *job = panfrost_get_job_for_fbo(ctx);
>  
>          /* Nothing to do! */
> -        if (!job->last_job.gpu && !job->clear) return;
> +        if (!job->last_job.gpu && !job->clear)
> +                goto reset_damage;
>  
>          if (!job->clear && job->last_tiler.gpu)
>                  panfrost_draw_wallpaper(&ctx->base);
> @@ -1476,6 +1477,17 @@ panfrost_flush(
>  
>          /* Prepare for the next frame */
>          panfrost_invalidate_frame(ctx);
> +
> +reset_damage:
> +        /* Reset the damage region on all color bufs, so we can start from a
> +         * fresh state next time those resources are used.
> +         */
> +        for (unsigned i = 0; i < ARRAY_SIZE(job->key.cbufs); i++) {

There's a potential use after free issue here (job might have been
free-ed when we reach that point). Can be solved by getting refs to all
cbufs when entering the function.

I'll fix that in v2.

> +                struct pipe_surface *surf = job->key.cbufs[i];
> +
> +                if (surf)
> +                        panfrost_resource_reset_damage(pan_resource(surf->texture));
> +        }
>  }
>  
>  #define DEFINE_CASE(c) case PIPE_PRIM_##c: return MALI_##c;
> diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
> index 57df61cb71cc..91beebaf8ccd 100644
> --- a/src/gallium/drivers/panfrost/pan_resource.c
> +++ b/src/gallium/drivers/panfrost/pan_resource.c
> @@ -47,7 +47,7 @@
>  #include "pan_util.h"
>  #include "pan_tiling.h"
>  
> -static void
> +void
>  panfrost_resource_reset_damage(struct panfrost_resource *pres)
>  {
>          /* We set the damage extent to the full resource size but keep the
> diff --git a/src/gallium/drivers/panfrost/pan_resource.h b/src/gallium/drivers/panfrost/pan_resource.h
> index 6ed3d1fd60e8..091fc36d17ce 100644
> --- a/src/gallium/drivers/panfrost/pan_resource.h
> +++ b/src/gallium/drivers/panfrost/pan_resource.h
> @@ -141,6 +141,8 @@ void
>  panfrost_blit_wallpaper(struct panfrost_context *ctx,
>                          struct pipe_box *box);
>  
> +void
> +panfrost_resource_reset_damage(struct panfrost_resource *pres);
>  void
>  panfrost_resource_set_damage_region(struct pipe_screen *screen,
>                                      struct pipe_resource *res,