Re: [igt-dev] [PATCH i-g-t v4 2/3] lib/igt_fb.c: Update tile sizes for GEN12 CCS

[Imre Deak <imre.deak@intel.com>]

On Tue, Nov 12, 2019 at 09:40:10AM +0200, Kahola, Mika wrote:
> On Mon, 2019-11-11 at 17:51 +0200, Imre Deak wrote:
> > On Mon, Nov 11, 2019 at 05:22:32PM +0200, Imre Deak wrote:
> > > Hi,
> > > 
> > > On Mon, Nov 11, 2019 at 01:00:48PM +0200, Mika Kahola wrote:
> > > > Update tile sizes for GEN12 CCS.
> > > > 
> > > > BSpec: 47709
> > > > 
> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > > Reviewed-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com
> > > > >
> > > > ---
> > > >  lib/igt_fb.c | 34 ++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 32 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > > > index 4adca967..77030d49 100644
> > > > --- a/lib/igt_fb.c
> > > > +++ b/lib/igt_fb.c
> > > > @@ -413,6 +413,33 @@ void igt_get_fb_tile_size(int fd, uint64_t
> > > > modifier, int fb_bpp,
> > > >  			*height_ret = 32;
> > > >  		}
> > > >  		break;
> > > > +	case LOCAL_I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > > +		igt_require_intel(fd);
> > > > +		switch (fb_bpp) {
> > > > +		case 8:
> > > > +			*width_ret = 512;
> > > > +			*height_ret = 32;
> > > > +			break;
> > > > +		case 16:
> > > > +			*width_ret = 256;
> > > > +			*height_ret = 32;
> > > > +			break;
> > > > +		case 32:
> > > > +			*width_ret = 128;
> > > > +			*height_ret = 32;
> > > > +			break;
> > > > +		case 64:
> > > > +			*width_ret = 64;
> > > > +			*height_ret = 32;
> > > > +			break;
> > > > +		case 128:
> > > > +			*width_ret = 32;
> > > > +			*height_ret = 32;
> > > > +			break;
> > > > +		default:
> > > > +			igt_assert(false);
> > > > +		}
> > > > +		break;
> > > 
> > > The above look wrong to me, tiles are always 4kB. So the GEN12 y-
> > > tiled tile width, height (CCS or not) are the same as on other
> > > platforms.
>
> > > OTOH, we should align both the main surface and AUX surface width
> > > and height for CCS. Those are setup in
> > > igt_init_fb()->fb_plane_height() and ->fb_plane_width(). The main
> > > surface (plane 0) should be aligned to 4x4 tiles (128x128 pixels).
> > 
> > and 128x128 is the case for 32bpp. For all bpps:
> > 
> > bpp	width	height
> > 8	512	128
> > 16	256	128
> > 32	128	128
> > 64	64	128
> > 128	32	128
>
> Ok, the numbers that I wrote originally above was found from BSpec.
> Since the tiles are always 4kB, I could update the patch with the
> numbers you provided.

The BSpec list is correct, but it's about the plane size alignment not
about tile sizes and it specifies width in pixels not in bytes as
igt_get_fb_tile_size(). I think we should keep the tile size what it
actually is (so 4kB or 128 bytes x 32 rows) and enforce the plane size
alignment in calc_plane_stride() and calc_plane_size().

> > > The AUX surface (plane 1) width/height should match this, so for
> > > that
> > > in fb_plane_width():
> > > 
> > > 	return DIV_ROUND_UP(fb->width, 128) * 64;
> > 
> > and here the divisor is based on the bpp as above.
> > 
> > > 
> > > and in fb_plane_height():
> > > 
> > > 	return DIV_ROUND_UP(fb->height, 128) * 4;
> > > 
> > > 
> > > >  	case LOCAL_I915_FORMAT_MOD_Yf_TILED:
> > > >  	case LOCAL_I915_FORMAT_MOD_Yf_TILED_CCS:
> > > >  		igt_require_intel(fd);
> > > > @@ -467,8 +494,10 @@ void igt_get_fb_tile_size(int fd, uint64_t
> > > > modifier, int fb_bpp,
> > > >  
> > > >  static bool is_ccs_modifier(uint64_t modifier)
> > > >  {
> > > > -	return modifier == LOCAL_I915_FORMAT_MOD_Y_TILED_CCS ||
> > > > -		modifier == LOCAL_I915_FORMAT_MOD_Yf_TILED_CCS;
> > > > +
> > > > +	return modifier == I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS ||
> > > > +		modifier == I915_FORMAT_MOD_Y_TILED_CCS ||
> > > > +		modifier == I915_FORMAT_MOD_Yf_TILED_CCS;
> > > >  }
> > > >  
> > > >  static unsigned fb_plane_width(const struct igt_fb *fb, int
> > > > plane)
> > > > @@ -687,6 +716,7 @@ uint64_t igt_fb_mod_to_tiling(uint64_t
> > > > modifier)
> > > >  		return I915_TILING_X;
> > > >  	case LOCAL_I915_FORMAT_MOD_Y_TILED:
> > > >  	case LOCAL_I915_FORMAT_MOD_Y_TILED_CCS:
> > > > +	case LOCAL_I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> > > >  		return I915_TILING_Y;
> > > >  	case LOCAL_I915_FORMAT_MOD_Yf_TILED:
> > > >  	case LOCAL_I915_FORMAT_MOD_Yf_TILED_CCS:
> > > > -- 
> > > > 2.17.1
> > > > 
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev