[v5,4/5] st/dri2: Implement DRI2bufferDamageExtension

Submitted by Boris Brezillon on July 2, 2019, 1:50 p.m.

Details

Message ID 20190702135002.23852-5-boris.brezillon@collabora.com
State New
Headers show
Series "EGL_KHR_partial_update support" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon July 2, 2019, 1:50 p.m.
From: Daniel Stone <daniels@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@collabora.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@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(+)

Patch hide | download patch | download mbox

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.
+    */
+   void (*set_damage_region)(struct pipe_screen *screen,
+                             struct pipe_resource *resource,
+                             unsigned int nrects, int *rects);
 };
 
 
diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
index 5a7ec878bab0..1a86cd244d21 100644
--- a/src/gallium/state_trackers/dri/dri2.c
+++ b/src/gallium/state_trackers/dri/dri2.c
@@ -1807,6 +1807,23 @@  static const __DRI2interopExtension dri2InteropExtension = {
    .export_object = dri2_interop_export_object
 };
 
+/**
+ * \brief the DRI2bufferDamageExtension set_damage_region method
+ */
+static void
+dri2_set_damage_region(__DRIdrawable *dPriv, unsigned int nrects, int *rects)
+{
+   struct dri_drawable *drawable = dri_drawable(dPriv);
+   struct pipe_resource *resource = drawable->textures[ST_ATTACHMENT_BACK_LEFT];
+   struct pipe_screen *screen = resource->screen;
+
+   screen->set_damage_region(screen, resource, nrects, rects);
+}
+
+static __DRI2bufferDamageExtension dri2BufferDamageExtension = {
+   .base = { __DRI2_BUFFER_DAMAGE, 1 },
+};
+
 /**
  * \brief the DRI2ConfigQueryExtension configQueryb method
  */
@@ -1908,6 +1925,7 @@  static const __DRIextension *dri_screen_extensions[] = {
    &dri2GalliumConfigQueryExtension.base,
    &dri2ThrottleExtension.base,
    &dri2FenceExtension.base,
+   &dri2BufferDamageExtension.base,
    &dri2InteropExtension.base,
    &dri2NoErrorExtension.base,
    &driBlobExtension.base,
@@ -1923,6 +1941,7 @@  static const __DRIextension *dri_robust_screen_extensions[] = {
    &dri2ThrottleExtension.base,
    &dri2FenceExtension.base,
    &dri2InteropExtension.base,
+   &dri2BufferDamageExtension.base,
    &dri2Robustness.base,
    &dri2NoErrorExtension.base,
    &driBlobExtension.base,
@@ -1983,6 +2002,9 @@  dri2_init_screen(__DRIscreen * sPriv)
       }
    }
 
+   if (pscreen->set_damage_region)
+      dri2BufferDamageExtension.set_damage_region = dri2_set_damage_region;
+
    if (pscreen->get_param(pscreen, PIPE_CAP_DEVICE_RESET_STATUS_QUERY)) {
       sPriv->extensions = dri_robust_screen_extensions;
       screen->has_reset_status_query = true;

Comments


On Tue, 2 Jul 2019 13:21:31 -0400
Marek Olšák <maraeo@gmail.com> wrote:

> On Tue., Jul. 2, 2019, 09:50 Boris Brezillon,
> <boris.brezillon@collabora.com> wrote:
> 
> > From: Daniel Stone <daniels@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@collabora.com>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@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.
Hello Marek,

On Tue, 2 Jul 2019 20:09:23 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

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

Regards,

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

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