On Mon, 15 Jul 2019 09:23:43 +0200 Boris Brezillon <boris.brezil...@collabora.com> wrote:
> Hello Marek, > > On Tue, 2 Jul 2019 20:09:23 +0200 > Boris Brezillon <boris.brezil...@collabora.com> wrote: > > > On Tue, 2 Jul 2019 13:21:31 -0400 > > Marek Olšák <mar...@gmail.com> wrote: > > > > > On Tue., Jul. 2, 2019, 09:50 Boris Brezillon, > > > <boris.brezil...@collabora.com> wrote: > > > > > > > From: Daniel Stone <dani...@collabora.com> > > > > > > > > Add a pipe_screen->set_damage_region() hook to propagate > > > > set-damage-region requests to the driver, it's then up to the > > > > driver to decide what to do with this piece of information. > > > > > > > > If the hook is left unassigned, the buffer-damage extension is > > > > considered unsupported. > > > > > > > > Signed-off-by: Daniel Stone <dani...@collabora.com> > > > > Signed-off-by: Boris Brezillon <boris.brezil...@collabora.com> > > > > Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzw...@collabora.com> > > > > --- > > > > Changes in v5: > > > > * Add Alyssa's R-b > > > > --- > > > > src/gallium/include/pipe/p_screen.h | 7 +++++++ > > > > src/gallium/state_trackers/dri/dri2.c | 22 ++++++++++++++++++++++ > > > > 2 files changed, 29 insertions(+) > > > > > > > > diff --git a/src/gallium/include/pipe/p_screen.h > > > > b/src/gallium/include/pipe/p_screen.h > > > > index 3f9bad470950..8df12ee4f865 100644 > > > > --- a/src/gallium/include/pipe/p_screen.h > > > > +++ b/src/gallium/include/pipe/p_screen.h > > > > @@ -464,6 +464,13 @@ struct pipe_screen { > > > > bool (*is_parallel_shader_compilation_finished)(struct > > > > pipe_screen *screen, > > > > void *shader, > > > > unsigned > > > > shader_type); + > > > > + /** > > > > + * Set damage region. > > > > > > > > > > Can you expand the comment to describe rects? The format of rects is > > > not obvious. > > > > Oops, will point to the KHR_partial_update() doc and explain what rects > > encode and how. > > This reminds me that we have a corner case (at least for tile-based > > GPUs): the dri implementation calls > > ->set_damage_region(screen, res, 0, NULL) to reset the damage region, > > but in KHR_partial_update() spec this means "damage all". If we follow > > the spec that would imply existing FB content is dropped which in turn > > means users relying on buffer_age() (without partial_update()) to only > > update the region that have changed will stop working properly. > > > > I see 2 options to solve this problem: > > > > 1/ add a new ->reset_damage_region() hook that would be called by the > > dri implementation after each swap_buf() in replacement of the > > current ->set_damage_region(screen, res, 0, NULL). Reset in that > > case means we consider the damage region as "unknown" and force > > a "reload FB content in the local-tile buffer" for the whole > > resource instead of restricting it to the !damage region. > > 2/ deviate from the KHR_partial_update() semantic and reserve > > ->set_damage_region(screen, res, 0, NULL) for the "reset damage > > region" op. That means we'll have to convert actual > > KHR_partial_update(0, NULL) calls into > > ->set_damage_region(screen, res, 1, full_res_rect) ones to reflect > > the behavior described in the spec. > > Any advice on how to solve this problem? Decided to go for a 3rd option in my v6 which is to keep things as they were and document that ->set_damage_region(0, NULL) should act as a 'reset damage region'. This is exactly how it's documented in the DRI2 extension, and I guess we can live the potential extra penalty when the application calls KHR_partial_update(0, NULL) instead of KHR_partial_update(1, full_res_rect). _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev