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

[Alyssa Rosenzweig]

Overall, I'm quite happy with how this turns out; I was fearful it would
be a lot more complicated, though there's always time for that ;)

Some specific comments follow: (mostly minor):
-----------------------------------------------

> +panfrost_blit_wallpaper(struct panfrost_context *ctx, struct pipe_box *rect)
>  {
>          struct pipe_blit_info binfo = { };
>  
>          panfrost_blitter_save(ctx);
>  
> -	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 = 0;
> -	binfo.src.box.y = binfo.dst.box.y = 0;
> -	binfo.src.box.width = binfo.dst.box.width = ctx->pipe_framebuffer.width;
> -	binfo.src.box.height = binfo.dst.box.height = ctx->pipe_framebuffer.height;
> +        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?" :)

> +        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.

> +        /* We set the damage extent to the full resource size but keep the
> +         * damage box empty so that the FB content is reloaded by default.
> +         */

....English, please? Francais, s'il te plait? I'm not too familiar with
winsys or the extension -- what's the difference between damage extent
and damage box?

> +                /* Looks like aligning on a tile is not enough, but aligning on
> +                 * twice the tile size works.
> +                 */
> +                ss.minx = rect[0] & ~((MALI_TILE_LENGTH * 2) - 1);
> +                ss.miny = y & ~((MALI_TILE_LENGTH * 2) - 1);
> +                ss.maxx = MIN2(ALIGN(rect[0] + rect[2], MALI_TILE_LENGTH * 2),
> +                               res->width0);
> +                ss.maxy = MIN2(ALIGN(y + rect[3], MALI_TILE_LENGTH * 2),
> +                               res->height0);

If aligning to 32x32 but not 16x16 works, that's probably masking over a
bug somewhere else in the code. The tiles used in the fragment (the
union/intersection_scissor) are 16x16, and the wallpapering blits are
pixel-accurate. What's really going on here?

> +panfrost_blit_wallpaper(struct panfrost_context *ctx,
> +                        struct pipe_box *damage);

Bikeshedding, but is it appropriate to name the field `damage` rather
than just generically `box`? Conceptually, blit_wallpaper just cares
about where to blit, it doesn't care *why* you're blitting there
(separation of concerns); it's not really that function's business that
we're blitting because of damage (and not some other reason we might
need to blit. Hypothetical example: we're doing a blit-based transfer
from a compressed format and we need to blit partial tiles on the edges.
That's not really 'damage').
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190626/f5d8d493/attachment.sig>