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

[Boris Brezillon]

Hi Qiang,

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

> On Mon, Jul 15, 2019 at 8:50 PM 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>
> > ---
> > 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