[Intel-gfx] [PATCH v4 1/3] drm/i915: Introduce new macros for i915 PTE

[Lucas De Marchi]

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