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