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.rosenzw...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
