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

[Boris Brezillon]

On Wed, 26 Jun 2019 10:51:23 -0700
Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com> wrote:

> 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 ;)

Same here. I thought it would be more complicated than that, but it
turned out to be pretty simple (mainly because I didn't go into too much
optimization to discard as much of the wallpapering area as could
theoretically be).

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

No problem, I'll take care of that (not the first time I rebase the
patch series BTW).

> 
> 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?" :)

Daniel already replied to that one.

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

I'll add a comment to explain 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?

Yeah, reading the comment again I realize it's not clear at all. The
damage extent is the quad covering all damage rects (even if they don't
intersect or only partially intersect). The damage box is actually the
biggest damage rect (rect1 in the following example):

              _______________________
              |            |         |
              |__________  |  rect 2 |
              |         |  |_________|
              | rect 1  |______      |
              |         |rect3 |     |
              |_________|______|_____|

                    damage extent

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

I wish I could come with a better explanation, but I couldn't find
anything explaining why this alignment is requirement or spot any
obvious bugs in the code :-/.

> The tiles used in the fragment (the
> union/intersection_scissor) are 16x16,

Oops, I fear intersection of non-32x32-aligned regions is not safe, it's
just that I didn't test this case :-). Note that union would not be
a problem here, because the intersection is applied last (just before
drawing the wallpaper). That's only true if we assume the intersection
func aligns things on 32x32 pixels of course (which is not the case
right now).

> and the wallpapering blits are
> pixel-accurate.

That part is true, and I actually rely on it (only reload parts of the
tiles that are not dirty).

> What's really going on here?

If only I knew...

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

Will rename this argument.