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

Submitted by Boris Brezillon on July 15, 2019, 12:50 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Boris Brezillon July 15, 2019, 12: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>
---
Hello Qiang,

I intentionally dropped your R-b/T-b on this patch since the
->set_damage_region() prototype has changed. Feel free to add it back.

Regards,

Boris

Changes in v6:
* Pass pipe_box objects instead ints
* Document the set_damage_region() hook

Changes in v5:
* Add Alyssa's R-b
---
 src/gallium/include/pipe/p_screen.h   | 17 ++++++++++++++
 src/gallium/state_trackers/dri/dri2.c | 34 +++++++++++++++++++++++++++
 2 files changed, 51 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..11a6aa939124 100644
--- a/src/gallium/include/pipe/p_screen.h
+++ b/src/gallium/include/pipe/p_screen.h
@@ -464,6 +464,23 @@  struct pipe_screen {
    bool (*is_parallel_shader_compilation_finished)(struct pipe_screen *screen,
                                                    void *shader,
                                                    unsigned shader_type);
+
+   /**
+    * Set the damage region (called when KHR_partial_update() is invoked).
+    * This function is passed an array of rectangles encoding the damage area.
+    * rects are using the bottom-left origin convention.
+    * nrects = 0 means 'reset the damage region'. What 'reset' implies is HW
+    * specific. For tile-based renderers, the damage extent is typically set
+    * to cover the whole resource with no damage rect (or a 0-size damage
+    * rect). This way, the existing resource content is reloaded into the
+    * local tile buffer for every tile thus making partial tile update
+    * possible. For HW operating in immediate mode, this reset operation is
+    * likely to be a NOOP.
+    */
+   void (*set_damage_region)(struct pipe_screen *screen,
+                             struct pipe_resource *resource,
+                             unsigned int nrects,
+                             const struct pipe_box *rects);
 };
 
 
diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
index 5a7ec878bab0..5273b95cd5fb 100644
--- a/src/gallium/state_trackers/dri/dri2.c
+++ b/src/gallium/state_trackers/dri/dri2.c
@@ -1807,6 +1807,35 @@  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;
+   struct pipe_box *boxes = NULL;
+
+   if (nrects) {
+      boxes = CALLOC(nrects, sizeof(*boxes));
+      assert(boxes);
+
+      for (unsigned int i = 0; i < nrects; i++) {
+         int *rect = &rects[i * 4];
+
+         u_box_2d(rect[0], rect[1], rect[2], rect[3], &boxes[i]);
+      }
+   }
+
+   screen->set_damage_region(screen, resource, nrects, boxes);
+}
+
+static __DRI2bufferDamageExtension dri2BufferDamageExtension = {
+   .base = { __DRI2_BUFFER_DAMAGE, 1 },
+};
+
 /**
  * \brief the DRI2ConfigQueryExtension configQueryb method
  */
@@ -1908,6 +1937,7 @@  static const __DRIextension *dri_screen_extensions[] = {
    &dri2GalliumConfigQueryExtension.base,
    &dri2ThrottleExtension.base,
    &dri2FenceExtension.base,
+   &dri2BufferDamageExtension.base,
    &dri2InteropExtension.base,
    &dri2NoErrorExtension.base,
    &driBlobExtension.base,
@@ -1923,6 +1953,7 @@  static const __DRIextension *dri_robust_screen_extensions[] = {
    &dri2ThrottleExtension.base,
    &dri2FenceExtension.base,
    &dri2InteropExtension.base,
+   &dri2BufferDamageExtension.base,
    &dri2Robustness.base,
    &dri2NoErrorExtension.base,
    &driBlobExtension.base,
@@ -1983,6 +2014,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 Mon, Jul 15, 2019 at 8:50 PM 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>
> ---
> Hello Qiang,
>
> I intentionally dropped your R-b/T-b on this patch since the
> ->set_damage_region() prototype has changed. Feel free to add it back.
>
> Regards,
>
> Boris
>
> Changes in v6:
> * Pass pipe_box objects instead ints
> * Document the set_damage_region() hook
>
> Changes in v5:
> * Add Alyssa's R-b
> ---
>  src/gallium/include/pipe/p_screen.h   | 17 ++++++++++++++
>  src/gallium/state_trackers/dri/dri2.c | 34 +++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
> index 3f9bad470950..11a6aa939124 100644
> --- a/src/gallium/include/pipe/p_screen.h
> +++ b/src/gallium/include/pipe/p_screen.h
> @@ -464,6 +464,23 @@ struct pipe_screen {
>     bool (*is_parallel_shader_compilation_finished)(struct pipe_screen *screen,
>                                                     void *shader,
>                                                     unsigned shader_type);
> +
> +   /**
> +    * Set the damage region (called when KHR_partial_update() is invoked).
> +    * This function is passed an array of rectangles encoding the damage area.
> +    * rects are using the bottom-left origin convention.
> +    * nrects = 0 means 'reset the damage region'. What 'reset' implies is HW
> +    * specific. For tile-based renderers, the damage extent is typically set
> +    * to cover the whole resource with no damage rect (or a 0-size damage
> +    * rect). This way, the existing resource content is reloaded into the
> +    * local tile buffer for every tile thus making partial tile update
> +    * possible. For HW operating in immediate mode, this reset operation is
> +    * likely to be a NOOP.
> +    */
> +   void (*set_damage_region)(struct pipe_screen *screen,
> +                             struct pipe_resource *resource,
> +                             unsigned int nrects,
> +                             const struct pipe_box *rects);
>  };
>
>
> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> index 5a7ec878bab0..5273b95cd5fb 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -1807,6 +1807,35 @@ 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;
> +   struct pipe_box *boxes = NULL;
> +
> +   if (nrects) {
> +      boxes = CALLOC(nrects, sizeof(*boxes));
> +      assert(boxes);

Where does this boxes array get freed? I can't find in your patch 6 either.
In fact I prefer the v5 way which just uses `int *rects` to avoid unnecessary
conversion.

Regards,
Qiang

> +
> +      for (unsigned int i = 0; i < nrects; i++) {
> +         int *rect = &rects[i * 4];
> +
> +         u_box_2d(rect[0], rect[1], rect[2], rect[3], &boxes[i]);
> +      }
> +   }
> +
> +   screen->set_damage_region(screen, resource, nrects, boxes);
> +}
> +
> +static __DRI2bufferDamageExtension dri2BufferDamageExtension = {
> +   .base = { __DRI2_BUFFER_DAMAGE, 1 },
> +};
> +
>  /**
>   * \brief the DRI2ConfigQueryExtension configQueryb method
>   */
> @@ -1908,6 +1937,7 @@ static const __DRIextension *dri_screen_extensions[] = {
>     &dri2GalliumConfigQueryExtension.base,
>     &dri2ThrottleExtension.base,
>     &dri2FenceExtension.base,
> +   &dri2BufferDamageExtension.base,
>     &dri2InteropExtension.base,
>     &dri2NoErrorExtension.base,
>     &driBlobExtension.base,
> @@ -1923,6 +1953,7 @@ static const __DRIextension *dri_robust_screen_extensions[] = {
>     &dri2ThrottleExtension.base,
>     &dri2FenceExtension.base,
>     &dri2InteropExtension.base,
> +   &dri2BufferDamageExtension.base,
>     &dri2Robustness.base,
>     &dri2NoErrorExtension.base,
>     &driBlobExtension.base,
> @@ -1983,6 +2014,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;
> --
> 2.21.0
>
Hi Qiang,

On Sun, 21 Jul 2019 17:02:54 +0800
Qiang Yu <yuq825@gmail.com> wrote:

> On Mon, Jul 15, 2019 at 8:50 PM 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>
> > ---
> > Hello Qiang,
> >
> > I intentionally dropped your R-b/T-b on this patch since the  
> > ->set_damage_region() prototype has changed. Feel free to add it back.  
> >
> > Regards,
> >
> > Boris
> >
> > Changes in v6:
> > * Pass pipe_box objects instead ints
> > * Document the set_damage_region() hook
> >
> > Changes in v5:
> > * Add Alyssa's R-b
> > ---
> >  src/gallium/include/pipe/p_screen.h   | 17 ++++++++++++++
> >  src/gallium/state_trackers/dri/dri2.c | 34 +++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
> > index 3f9bad470950..11a6aa939124 100644
> > --- a/src/gallium/include/pipe/p_screen.h
> > +++ b/src/gallium/include/pipe/p_screen.h
> > @@ -464,6 +464,23 @@ struct pipe_screen {
> >     bool (*is_parallel_shader_compilation_finished)(struct pipe_screen *screen,
> >                                                     void *shader,
> >                                                     unsigned shader_type);
> > +
> > +   /**
> > +    * Set the damage region (called when KHR_partial_update() is invoked).
> > +    * This function is passed an array of rectangles encoding the damage area.
> > +    * rects are using the bottom-left origin convention.
> > +    * nrects = 0 means 'reset the damage region'. What 'reset' implies is HW
> > +    * specific. For tile-based renderers, the damage extent is typically set
> > +    * to cover the whole resource with no damage rect (or a 0-size damage
> > +    * rect). This way, the existing resource content is reloaded into the
> > +    * local tile buffer for every tile thus making partial tile update
> > +    * possible. For HW operating in immediate mode, this reset operation is
> > +    * likely to be a NOOP.
> > +    */
> > +   void (*set_damage_region)(struct pipe_screen *screen,
> > +                             struct pipe_resource *resource,
> > +                             unsigned int nrects,
> > +                             const struct pipe_box *rects);
> >  };
> >
> >
> > diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> > index 5a7ec878bab0..5273b95cd5fb 100644
> > --- a/src/gallium/state_trackers/dri/dri2.c
> > +++ b/src/gallium/state_trackers/dri/dri2.c
> > @@ -1807,6 +1807,35 @@ 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;
> > +   struct pipe_box *boxes = NULL;
> > +
> > +   if (nrects) {
> > +      boxes = CALLOC(nrects, sizeof(*boxes));
> > +      assert(boxes);  
> 
> Where does this boxes array get freed? I can't find in your patch 6 either.

Indeed, the FREE() is missing.

> In fact I prefer the v5 way which just uses `int *rects` to avoid unnecessary
> conversion.

Well, Erik suggested to pass an array of pipe_boxe objects to make
things clearer, and I can of agree with him. Moreover, I'd expect the
extra allocation + pipe_box init overhead to be negligible.

Regards,

Boris
+Marek (looks like I forgot to Cc you on this v6 :-/).

On Mon, 22 Jul 2019 09:49:31 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hi Qiang,
> 
> On Sun, 21 Jul 2019 17:02:54 +0800
> Qiang Yu <yuq825@gmail.com> wrote:
> 
> > On Mon, Jul 15, 2019 at 8:50 PM 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>
> > > ---
> > > Hello Qiang,
> > >
> > > I intentionally dropped your R-b/T-b on this patch since the    
> > > ->set_damage_region() prototype has changed. Feel free to add it back.    
> > >
> > > Regards,
> > >
> > > Boris
> > >
> > > Changes in v6:
> > > * Pass pipe_box objects instead ints
> > > * Document the set_damage_region() hook
> > >
> > > Changes in v5:
> > > * Add Alyssa's R-b
> > > ---
> > >  src/gallium/include/pipe/p_screen.h   | 17 ++++++++++++++
> > >  src/gallium/state_trackers/dri/dri2.c | 34 +++++++++++++++++++++++++++
> > >  2 files changed, 51 insertions(+)
> > >
> > > diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
> > > index 3f9bad470950..11a6aa939124 100644
> > > --- a/src/gallium/include/pipe/p_screen.h
> > > +++ b/src/gallium/include/pipe/p_screen.h
> > > @@ -464,6 +464,23 @@ struct pipe_screen {
> > >     bool (*is_parallel_shader_compilation_finished)(struct pipe_screen *screen,
> > >                                                     void *shader,
> > >                                                     unsigned shader_type);
> > > +
> > > +   /**
> > > +    * Set the damage region (called when KHR_partial_update() is invoked).
> > > +    * This function is passed an array of rectangles encoding the damage area.
> > > +    * rects are using the bottom-left origin convention.
> > > +    * nrects = 0 means 'reset the damage region'. What 'reset' implies is HW
> > > +    * specific. For tile-based renderers, the damage extent is typically set
> > > +    * to cover the whole resource with no damage rect (or a 0-size damage
> > > +    * rect). This way, the existing resource content is reloaded into the
> > > +    * local tile buffer for every tile thus making partial tile update
> > > +    * possible. For HW operating in immediate mode, this reset operation is
> > > +    * likely to be a NOOP.
> > > +    */
> > > +   void (*set_damage_region)(struct pipe_screen *screen,
> > > +                             struct pipe_resource *resource,
> > > +                             unsigned int nrects,
> > > +                             const struct pipe_box *rects);
> > >  };
> > >
> > >
> > > diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> > > index 5a7ec878bab0..5273b95cd5fb 100644
> > > --- a/src/gallium/state_trackers/dri/dri2.c
> > > +++ b/src/gallium/state_trackers/dri/dri2.c
> > > @@ -1807,6 +1807,35 @@ 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;
> > > +   struct pipe_box *boxes = NULL;
> > > +
> > > +   if (nrects) {
> > > +      boxes = CALLOC(nrects, sizeof(*boxes));
> > > +      assert(boxes);    
> > 
> > Where does this boxes array get freed? I can't find in your patch 6 either.  
> 
> Indeed, the FREE() is missing.
> 
> > In fact I prefer the v5 way which just uses `int *rects` to avoid unnecessary
> > conversion.  
> 
> Well, Erik suggested to pass an array of pipe_boxe objects to make
> things clearer, and I can of agree with him. Moreover, I'd expect the

			*kind of

> extra allocation + pipe_box init overhead to be negligible.

Erik, Qiang, Marek,

Any comment on this v5. Should I send a v6 adding the missing FREE()
call. Anything else you'd like me to change?

Thanks,

Boris
On Thu, Aug 1, 2019 at 8:15 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> +Marek (looks like I forgot to Cc you on this v6 :-/).
>
> On Mon, 22 Jul 2019 09:49:31 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> > Hi Qiang,
> >
> > On Sun, 21 Jul 2019 17:02:54 +0800
> > Qiang Yu <yuq825@gmail.com> wrote:
> >
> > > On Mon, Jul 15, 2019 at 8:50 PM 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>
> > > > ---
> > > > Hello Qiang,
> > > >
> > > > I intentionally dropped your R-b/T-b on this patch since the
> > > > ->set_damage_region() prototype has changed. Feel free to add it back.
> > > >
> > > > Regards,
> > > >
> > > > Boris
> > > >
> > > > Changes in v6:
> > > > * Pass pipe_box objects instead ints
> > > > * Document the set_damage_region() hook
> > > >
> > > > Changes in v5:
> > > > * Add Alyssa's R-b
> > > > ---
> > > >  src/gallium/include/pipe/p_screen.h   | 17 ++++++++++++++
> > > >  src/gallium/state_trackers/dri/dri2.c | 34 +++++++++++++++++++++++++++
> > > >  2 files changed, 51 insertions(+)
> > > >
> > > > diff --git a/src/gallium/include/pipe/p_screen.h b/src/gallium/include/pipe/p_screen.h
> > > > index 3f9bad470950..11a6aa939124 100644
> > > > --- a/src/gallium/include/pipe/p_screen.h
> > > > +++ b/src/gallium/include/pipe/p_screen.h
> > > > @@ -464,6 +464,23 @@ struct pipe_screen {
> > > >     bool (*is_parallel_shader_compilation_finished)(struct pipe_screen *screen,
> > > >                                                     void *shader,
> > > >                                                     unsigned shader_type);
> > > > +
> > > > +   /**
> > > > +    * Set the damage region (called when KHR_partial_update() is invoked).
> > > > +    * This function is passed an array of rectangles encoding the damage area.
> > > > +    * rects are using the bottom-left origin convention.
> > > > +    * nrects = 0 means 'reset the damage region'. What 'reset' implies is HW
> > > > +    * specific. For tile-based renderers, the damage extent is typically set
> > > > +    * to cover the whole resource with no damage rect (or a 0-size damage
> > > > +    * rect). This way, the existing resource content is reloaded into the
> > > > +    * local tile buffer for every tile thus making partial tile update
> > > > +    * possible. For HW operating in immediate mode, this reset operation is
> > > > +    * likely to be a NOOP.
> > > > +    */
> > > > +   void (*set_damage_region)(struct pipe_screen *screen,
> > > > +                             struct pipe_resource *resource,
> > > > +                             unsigned int nrects,
> > > > +                             const struct pipe_box *rects);
> > > >  };
> > > >
> > > >
> > > > diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
> > > > index 5a7ec878bab0..5273b95cd5fb 100644
> > > > --- a/src/gallium/state_trackers/dri/dri2.c
> > > > +++ b/src/gallium/state_trackers/dri/dri2.c
> > > > @@ -1807,6 +1807,35 @@ 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;
> > > > +   struct pipe_box *boxes = NULL;
> > > > +
> > > > +   if (nrects) {
> > > > +      boxes = CALLOC(nrects, sizeof(*boxes));
> > > > +      assert(boxes);
> > >
> > > Where does this boxes array get freed? I can't find in your patch 6 either.
> >
> > Indeed, the FREE() is missing.
> >
> > > In fact I prefer the v5 way which just uses `int *rects` to avoid unnecessary
> > > conversion.
> >
> > Well, Erik suggested to pass an array of pipe_boxe objects to make
> > things clearer, and I can of agree with him. Moreover, I'd expect the
>
>                         *kind of
>
> > extra allocation + pipe_box init overhead to be negligible.
>
> Erik, Qiang, Marek,
>
> Any comment on this v5. Should I send a v6 adding the missing FREE()
> call. Anything else you'd like me to change?
>
Hi Boris,

There's no other change from my side. Use v5 way is just my suggestion,
you can get my RB anyway by either add FREE or go back to v5.

Thanks,
Qiang

> Thanks,
>
> Boris
On Tue, 6 Aug 2019 15:49:14 +0800
Qiang Yu <yuq825@gmail.com> wrote:

> >
> > Erik, Qiang, Marek,
> >
> > Any comment on this v5. Should I send a v6 adding the missing FREE()
> > call. Anything else you'd like me to change?
> >  
> Hi Boris,
> 
> There's no other change from my side. Use v5 way is just my suggestion,
> you can get my RB anyway by either add FREE or go back to v5.

Just sent a v7. Feel free to add your R-b if you're fine with this new
version.

Regards,

Boris