[Mesa-dev] [PATCH v5 4/5] st/dri2: Implement DRI2bufferDamageExtension

[Boris Brezillon]

On Mon, 15 Jul 2019 09:23:43 +0200
Boris Brezillon <boris.brezillon at collabora.com> wrote:

> Hello Marek,
> 
> On Tue, 2 Jul 2019 20:09:23 +0200
> Boris Brezillon <boris.brezillon at collabora.com> wrote:
> 
> > On Tue, 2 Jul 2019 13:21:31 -0400
> > Marek Olšák <maraeo at gmail.com> wrote:
> >   
> > > On Tue., Jul. 2, 2019, 09:50 Boris Brezillon,
> > > <boris.brezillon at collabora.com> wrote:
> > >     
> > > > From: Daniel Stone <daniels at 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 <daniels at collabora.com>
> > > > Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> > > > Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig at 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).