[PATCH i-g-t v4 2/5] lib/intel_blt: Change surface size calculation

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 02:44:56PM +0100, Karolina Stolarek wrote:
> 
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> > For tiled surfaces we need to ensure stride and height are valid
> > from the blit operation perspective. Calculate required surface
> > size according to tiling constraints.
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Karolina Drobnik <karolina.drobnik at intel.com>
> 
> I'm looking at blt_set_geom() function. We still pass normal height and
> width to it. Is this correct? Or should we change height to
> aligned_height? Because if we should, this patch should be updated to
> reflect that.

Function creates an object, which should be correct from stride/aligned
height perspective. I mean you need to allocate enough memory to handle
all pixels which will be touched during blit. So the best is to allocate
whole tile (x/y/4/64). Then you'll be sure hw won't touch addresses
outside your object and you don't hang the engine.

For example you want to blit 1x128x32bpp between two surfaces for
Tile64. Even you're using single pixel in x (one dword) you need to
provide stride 128 (in dwords). That's hardware requirement and even
if this looks like waste of memory (127 pixels in x won't be used at
all) hardware expects that. I learned this hard way during extending
block-copy from small to large buffers.

I wondered to refactor blt_copy_object to contain bpp (this way we
may warn if user passes invalid stride during object setup) but at
the moment I resisted as my primary goal is to fill the test gap
in blit/render-copy from small to large buffers.


> 
> If that's not a problem, then:
> 
> Reviewed-by: Karolina Stolarek <karolina.stolarek at intel.com>

Thanks for the review.

--
Zbigniew

> 
> > ---
> >   lib/intel_blt.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> > index 13b1dbba4f..00208fc243 100644
> > --- a/lib/intel_blt.c
> > +++ b/lib/intel_blt.c
> > @@ -1858,13 +1858,21 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region,
> >   		  bool create_mapping)
> >   {
> >   	struct blt_copy_object *obj;
> > -	uint64_t size = width * height * bpp / 8;
> > -	uint32_t stride = tiling == T_LINEAR ? width * 4 : width;
> > +	uint32_t stride, aligned_height;
> > +	uint64_t size;
> >   	uint32_t handle;
> >   	uint8_t pat_index = DEFAULT_PAT_INDEX;
> >   	igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n");
> > +	stride = blt_get_min_stride(width, bpp, tiling);
> > +	aligned_height = blt_get_aligned_height(height, bpp, tiling);
> > +	size = stride * aligned_height;
> > +
> > +	/* blitter command expects stride in dwords on tiled surfaces */
> > +	if (tiling)
> > +		stride /= 4;
> > +
> >   	obj = calloc(1, sizeof(*obj));
> >   	obj->size = size;