Re: [PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates

[Alistair Popple <alistair@popple.id.au>]

> > 
> > We also support 4K page sizes on PPC. If I am not mistaken this means every ATSD
> > would invalidate the entire GPU TLB for a the given PID on those systems. Could
> > we change the above check to `if (size <= PAGE_64K)` to avoid this?
> 
> PPC supports 4K pages but the GPU ATS implementation does not. For that
> reason I didn't bother handling invalidates smaller than 64K. I'll add a
> comment on that.

Interesting, I was not aware of that limitation. Do you know if it is a
SW/driver limitation or a HW limitation?

> I don't know that this requirement is enforced anywhere though. I could
> add a PAGE_SIZE == 64K check to pnv_npu2_init_context if you think it
> would be useful.

Given it's a static kernel build parameter perhaps it makes more sense to do the
check as part of the driver build in a conftest rather than a runtime failure?

> >
> > > +		atsd_start = start;
> > 
> > Which would also require:
> > 
> >       	    atsd_start = ALIGN_DOWN(start, PAGE_64K);
> > 
> > > +		atsd_psize = MMU_PAGE_64K;
> > > +	} else if (ALIGN_DOWN(start, PAGE_2M) == ALIGN_DOWN(end, PAGE_2M)) {
> > 
> > Wouldn't this lead to under invalidation in ranges which happen to cross a 2M
> > boundary? For example invalidating a 128K (ie. 2x64K pages) range with start ==
> > 0x1f0000 and end == 0x210000 would result in an invalidation of the range 0x0 -
> > 0x200000 incorrectly leaving 0x200000 - 0x210000 in the GPU TLB.
> 
> In this example:
> start                         0x1f0000
> size                          0x020000
> end (start + size - 1)        0x20ffff
> ALIGN_DOWN(start, PAGE_2M)    0x000000
> ALIGN_DOWN(end, PAGE_2M)      0x200000
> 
> Since ALIGN_DOWN(start, PAGE_2M) != ALIGN_DOWN(end, PAGE_2M), the
> condition fails and we move to the 1G clause. Then
> ALIGN_DOWN(start, PAGE_1G) == ALIGN_DOWN(end, PAGE_1G) == 0, so we
> invalidate the range [0, 1G).

Oh yeah, sorry that makes sense and looks good to me.

- Alistair