Re: [PATCH i-g-t 01/37] lib/intel_aux_pgtable: Library to add support for RGB16161616_64B format

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Aug 27, 2024 at 06:16:20PM +0300, Juha-Pekka Heikkila wrote:
> On 3.7.2024 2.27, Ville Syrjala wrote:
> > From: Melanie Lobo <melanie.lobo@intel.com>
> > 
> > TGL+ supports RGB16161616_64B FP16 format which is a binary
> > floating-point computer number format that occupies 16 bits
> > in computer memory.
> > 
> > This was tested with kernel patch,
> > https://patchwork.freedesktop.org/series/124957/
> > https://lore.kernel.org/all/20231201091133.23508-1-melanie.lobo@intel.com/
> > 
> > cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > cc: Swati Sharma <swati2.sharma@intel.com>
> > Signed-off-by: Melanie Lobo <melanie.lobo@intel.com>
> > [vsyrjala: s/0x1/0x10/ to make the format correct, note that it's for TGL+]
> 
> I'm feeling bit challenged on this s/0x1/0x10/ change. When looking at 
> bpsec 43868 Melanie's original '1' look correct. Rest of those 
> AUX_FORMAT_* match to what's in 43868.

That's for media compression it seems. We need render compression.

> Then with bspec 43869 this '10' 

That page seems to be just for ATS/etc. For TGL we need 
want 53911 and for DG2+ 53726. Which give us 0x10 and 0x5
respectively.

> look correct but what will we get on MTL with this? > I think Melanie's 
> original kernel change was targeting for MTL hence this was not looked at.

For DG2+ 44930 says:
"Compression format from AUX page walk is ignored. Instead compression
 format from Surface State is used."
which presumably was changed for flat CCS, but bspec seems to be
telling us that MTL already includes this change despite not having
flat CCS yet.

Though the tags on that page are somehow screwed up because if I filter
for MTL the whole page gets disabled. I filed an issue for that.

Anyways, should be fairly easy to confirm by putting garbage into
the AUX page table compression format on MTL vs. TGL and seeing what
happens.

And if everything checks out we should perhaps explicitly not populate
the AUX page table compression format on MTL, just to avoid confusing
people (bspec itself is already confusing enough when it comes to
compression).

> 
> /Juha-Pekka
> 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   lib/intel_aux_pgtable.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/lib/intel_aux_pgtable.c b/lib/intel_aux_pgtable.c
> > index 3cbb0e26f35c..8ff48641619e 100644
> > --- a/lib/intel_aux_pgtable.c
> > +++ b/lib/intel_aux_pgtable.c
> > @@ -22,6 +22,7 @@
> >   #define AUX_FORMAT_AYUV		0x09
> >   #define AUX_FORMAT_ARGB_8B	0x0A
> >   #define AUX_FORMAT_NV12_21	0x0F
> > +#define AUX_FORMAT_RGBA16_FLOAT	0x10
> >   
> >   struct pgtable_level_desc {
> >   	int idx_shift;
> > @@ -306,6 +307,10 @@ static uint64_t pgt_get_l1_flags(const struct intel_buf *buf, int surface_idx)
> >   			entry.e.format = AUX_FORMAT_ARGB_8B;
> >   			entry.e.depth = bpp_to_depth_val(32);
> >   			break;
> > +		case 64:
> > +			entry.e.format = AUX_FORMAT_RGBA16_FLOAT;
> > +			entry.e.depth = bpp_to_depth_val(64);
> > +			break;
> >   		default:
> >   			igt_assert(0);
> >   		}

-- 
Ville Syrjälä
Intel