[PATCH i-g-t v4 1/5] lib/intel_blt: Add helpers for calculating stride and aligned height

[Zbigniew Kempczyński]

On Thu, Feb 01, 2024 at 01:05:20PM +0100, Karolina Stolarek wrote:
> On 1.02.2024 11:07, Zbigniew Kempczyński wrote:
> > Tiled surfaces have stride / aligned height constraints. Currently
> > blt library has limitation and doesn't work properly when surface
> > stride is not valid for specific tiling.
> > 
> > As an example lets say we want to copy from linear to xmajor
> > 33 x 33 x 32bpp surface. Xmajor surface expects stride aligned to
> > 512 bytes and height to 8 rows so this surface will occupy 512B x 40
> > (128 x 40 x 32 bpp).
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Karolina Drobnik <karolina.drobnik at intel.com>
> 
> Just a bunch of nits, nothing major.
> 
> > ---
> > v2: Fix stride/aligned height calculation for Tile64
> > v3: Fix stride calculation for xmajor
> >      Make helpers public (fixes compilation warning due to lack
> >      or static function users)
> > ---
> >   lib/intel_blt.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   lib/intel_blt.h |  4 ++++
> >   2 files changed, 68 insertions(+)
> > 
> > diff --git a/lib/intel_blt.c b/lib/intel_blt.c
> > index 25d251c4f8..13b1dbba4f 100644
> > --- a/lib/intel_blt.c
> > +++ b/lib/intel_blt.c
> > @@ -521,6 +521,70 @@ static int __block_tiling(enum blt_tiling_type tiling)
> >   	return 0;
> >   }
> > +/**
> > + * blt_get_min_stride
> > + * @width: width in pixels
> > + * @bpp: bits per pixel
> > + * @tiling: tiling
> > + *
> > + * Function returns minimum posibble stride in bytes for width, bpp and tiling.
> 
> "return" is already described, we could say "calculates minimum possible
> stride..."

Agree, repeating same looks odd.

> 
> > + *
> > + * Returns:
> > + * minimum possible stride in bytes.
> > + */
> > +uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
> > +			    enum blt_tiling_type tiling)
> > +{
> > +	switch (tiling) {
> > +	case T_LINEAR:
> > +		return width * bpp / 8;
> > +	case T_XMAJOR:
> > +		return ALIGN(width * bpp / 8, 512);
> > +	case T_TILE64:
> > +		if (bpp == 8)
> > +			return ALIGN(width, 256);
> > +		else if (bpp == 16 || bpp == 32)
> > +			return ALIGN(width * bpp / 8, 512);
> > +		return ALIGN(width * bpp / 8, 1024);
> > +
> > +	default:
> > +		return ALIGN(width * bpp / 8, 128);
> > +	}
> > +}
> > +
> > +/**
> > + * blt_get_aligned_height
> > + * @height: height in pixels
> > + * @bpp: bits per pixel (used for Tile64 due to different tile organization
> > + * in pixels)
> > + * @tiling: tiling
> > + *
> > + * Function returns aligned height for specific tiling. Height returned is
> > + * important from memory allocation perspective, because each tiling has
> > + * specific memory constraints.
> 
> I'd move the comment on Tile64 to the description body and integrate it
> with the last sentence. You could say that each tiling has constrains
> dependent on bpp and mention a different pixel order for Tile64 as an
> example.

I've added some note about Tile64 to commit message to explain why
we need to pass bpp either.

Thanks for the review.

> 
> Apart from that, it's looking good to me:
> 
> Reviewed-by: Karolina Stolarek <karolina.stolarek at intel.com>

R-b taken.

--
Zbigniew
> 
> > + *
> > + * Returns:
> > + * height (rows) expected for specific tiling
> > + */
> > +uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
> > +				enum blt_tiling_type tiling)
> > +{
> > +	switch (tiling) {
> > +	case T_LINEAR:
> > +		return height;
> > +	case T_XMAJOR:
> > +		return ALIGN(height, 8);
> > +	case T_TILE64:
> > +		if (bpp == 8)
> > +			return ALIGN(height, 256);
> > +		else if (bpp == 16 || bpp == 32)
> > +			return ALIGN(height, 128);
> > +		return ALIGN(height, 64);
> > +	default:
> > +		return ALIGN(height, 32);
> > +	}
> > +}
> > +
> >   static int __special_mode(const struct blt_copy_data *blt)
> >   {
> >   	if (blt->src.handle == blt->dst.handle &&
> > diff --git a/lib/intel_blt.h b/lib/intel_blt.h
> > index d9be22fdf4..e3084dc0cd 100644
> > --- a/lib/intel_blt.h
> > +++ b/lib/intel_blt.h
> > @@ -212,6 +212,10 @@ bool blt_block_copy_supports_compression(int fd);
> >   bool blt_uses_extended_block_copy(int fd);
> >   const char *blt_tiling_name(enum blt_tiling_type tiling);
> > +uint32_t blt_get_min_stride(uint32_t width, uint32_t bpp,
> > +			    enum blt_tiling_type tiling);
> > +uint32_t blt_get_aligned_height(uint32_t height, uint32_t bpp,
> > +				enum blt_tiling_type tiling);
> >   void blt_copy_init(int fd, struct blt_copy_data *blt);