On Fri, Nov 12, 2021 at 05:31:46PM -0800, Matt Roper wrote:
On Fri, Nov 12, 2021 at 05:28:09PM -0800, Matt Roper wrote:
On Wed, Nov 10, 2021 at 04:45:47PM -0800, Michael Cheng wrote:
> Certain functions within i915 uses macros that are defined for
> specific architectures by the mmu, such as _PAGE_RW and _PAGE_PRESENT
> (Some architectures don't even have these macros defined, like ARM64).
>
> Instead of re-using bits defined for the CPU, we should use bits
> defined for i915. This patch introduces two new macros,
> I915_PAGE_PRESENT and I915_PAGE_RW, to check for bits 0 and 1 and, to
> replace all occurrences of _PAGE_RW and _PAGE_PRESENT within i915.
On older platforms we already had our own definition of GEN6_PTE_VALID
(the spec uses "present" and "valid" interchangeably) which we were
using to encode our ggtt ptes up through HSW; it might be better to go
back to using that rather than adding a new define.
It looks like BYT is when the writable bit showed up, and we did add a
new define there (BYT_PTE_WRITEABLE), but on the next platform (BDW) we
switched over to using the CPU page table flags instead and never used
it again. So we could probably replace _PAGE_RW with BYT_PTE_WRITEABLE
as well.
Okay, I should have looked at the rest of the series before reviewing
the first patch; it looks like your next two patches replace
GEN6_PTE_VALID and BYT_PTE_WRITEABLE with the new definitions here. I
still think it might be preferable to reuse the existing macros (which
also help clarify the platforms that those bits first showed up in the
PTE on) rather than replacing them with new macros, but I don't feel
super strongly about it if other reviewers feel differently.
I think it deserves a I915_ particularly because of the RW definition.
To me it's always suspicious when code spanning for several platforms
use a definition for BYT or CHV, because those are usually the one that
deviates from the norm, not the ones that dictate new behavior going
forward.
Lucas De Marchi
