On Thu, Aug 1, 2019 at 8:15 PM Boris Brezillon
<boris.brezil...@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.brezil...@collabora.com> wrote:
>
> > Hi Qiang,
> >
> > On Sun, 21 Jul 2019 17:02:54 +0800
> > Qiang Yu <yuq...@gmail.com> wrote:
> >
> > > On Mon, Jul 15, 2019 at 8:50 PM 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>
> > > > ---
> > > > 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
