[Mesa-dev] [PATCH] intel/blorp: Fix a couple asserts around image copy rectangles

[Jason Ekstrand]

On Tue, Oct 25, 2016 at 11:52 PM, Iago Toral <itoral at igalia.com> wrote:

> On Tue, 2016-10-25 at 22:53 -0700, Jason Ekstrand wrote:
> > With dealing with rectangles in compressed images, you can have a
> > width or
> > height that isn't a multiple of the corresponding compression block
> > dimension but only if that edge of your rectangle is on the edge of
> > the
> > image.  When we call convert_to_single_slice, it creates an 2-D image
> > and a
> > set of tile offsets into that image.  When detecting the right-edge
> > and
> > bottom-edge cases, we weren't including the tile offsets so the
> > assert
> > would misfire.  This caused crashes in a few UE4 demos
> >
> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> > Reported-by: "Eero Tamminen" <eero.t.tamminen at intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98431
> > Cc: "13.0" <mesa-stable at lists.freedesktop.org>
> > ---
> >  src/intel/blorp/blorp_blit.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/blorp/blorp_blit.c
> > b/src/intel/blorp/blorp_blit.c
> > index 0c3ee72..9357a72 100644
> > --- a/src/intel/blorp/blorp_blit.c
> > +++ b/src/intel/blorp/blorp_blit.c
> > @@ -1761,10 +1761,14 @@ surf_convert_to_uncompressed(const struct
> > isl_device *isl_dev,
> >     surf_convert_to_single_slice(isl_dev, info);
> >
> >     if (width || height) {
> > +#ifndef NDEBUG
> > +      uint32_t right_edge_px = info->tile_x_sa + *x + *width;
> > +      uint32_t bottom_edge_px = info->tile_y_sa + *y + *height;
> >        assert(*width % fmtl->bw == 0 ||
> > -             *x + *width == info->surf.logical_level0_px.width);
> > +             right_edge_px == info->surf.logical_level0_px.width);
> >        assert(*height % fmtl->bh == 0 ||
> > -             *y + *height == info->surf.logical_level0_px.height);
> > +             bottom_edge_px == info->surf.logical_level0_px.height);
> > +#endif
>
> Makes sense to me.
>
> >        *width = DIV_ROUND_UP(*width, fmtl->bw);
> >        *height = DIV_ROUND_UP(*height, fmtl->bh);
>
> In that case, we end up rounding up here. I suppose that is handled
> properly going forward from here, right? I ask because the previous
> asserts suggested that this might not have been considered during the
> implementation (although the DIV_ROUND_UP() here suggests that it was).
>

Right.  The DIV_ROUND_UP is here exactly to handle the unaligned
bottom-right edge case.  All of the other cases require alignment.


> Assuming this is all fine, this patch is:
> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
>
> >     }
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161026/fedc382e/attachment.html>