[Mesa-dev] [PATCH v4 5/5] panfrost: Add support for KHR_partial_update()

[Daniel Stone]

Hi,
To be honest, I haven't been able to look too closely at this one. I
wasn't able to easily reason about the twists and turns, so had to run
away to reviews elsewhere. But as long as we reload every single
region passed in - be it individually or just lazily pulling in the
extents, it's correct.

There's no need to intersect the partial_update region with the
scissor, since rendering outside of the partial_update area is
explicitly undefined.

On Wed, 26 Jun 2019 at 18:51, Alyssa Rosenzweig
<alyssa.rosenzweig at collabora.com> wrote:
> > +        binfo.src.resource = binfo.dst.resource = ctx->pipe_framebuffer.cbufs[0]->texture;
> > +        binfo.src.level = binfo.dst.level = 0;
> > +        binfo.src.box.x = binfo.dst.box.x = rect->x;
> > +        binfo.src.box.y = binfo.dst.box.y = rect->y;
> > +        binfo.src.box.width = binfo.dst.box.width = rect->width;
> > +        binfo.src.box.height = binfo.dst.box.height = rect->height;
> >
> >       /* This avoids an assert due to missing nir_texop_txb support */
> >       //binfo.src.box.depth = binfo.dst.box.depth = 1;
>
> This will need to be rebased in a slightly messy way, since
> panfrost_blit_wallpaper was edited pretty heavily in the mipmap series
> that just landed. Sorry for the conflicts, although conceptually this
> looks good.
>
> Have you considered if this interacts with mipmapping, by the way? I
> suppose surfaces that get partial updates *by definition* are not
> mipmapped, so that's an easy "who cares?" :)

Yeah, partial_update only applies to winsys surfaces.

> > +        u_box_2d(batch->minx, batch->miny, damage.minx - batch->minx,
> > +                 batch->maxy - batch->miny, &rects[0]);
> > +        u_box_2d(damage.maxx, batch->miny, batch->maxx - damage.maxx,
> > +                 batch->maxy - batch->miny, &rects[1]);
> > +        u_box_2d(damage.minx, batch->miny, damage.maxx - damage.minx,
> > +                 damage.miny - batch->miny, &rects[2]);
> > +        u_box_2d(damage.minx, damage.maxy, damage.maxx - damage.minx,
> > +                 batch->maxy - damage.maxy, &rects[3]);
> > +
> > +        for (unsigned i = 0; i < 4; i++) {
> > +                if (!rects[i].width || !rects[i].height)
> > +                        continue;
>
> This 'if' statement seems a little magic. Does u_box_2d clamp
> width/height positive automatically? Is it possible to get negative
> width/height? If the answer is "yes; no" respectively, which seems to be
> how the code works, maybe add a quick comment explaining that.

Negative width or height isn't explicitly allowed in the
partial_update spec; by that omission I would say it's not defined
behaviour.

Cheers,
Daniel