On Thu, 29 Aug 2019 13:54:15 +0200
Boris Brezillon <boris.brezil...@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.brezil...@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,
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
