Re: [Intel-gfx] [PATCH] drm/i915: Skip remap_io_mapping() for non-x86 platforms

[Matthew Auld <matthew.william.auld@gmail.com>]

On Sat, 13 Nov 2021 at 17:33, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>
> On Sat, Nov 13, 2021 at 12:18:10AM +0200, Jani Nikula wrote:
> >On Fri, 12 Nov 2021, Mullati Siva <siva.mullati@intel.com> wrote:
> >> The _PAGE_CACHE_MASK macro is not defined in non-x86
> >> architectures and it's been used in remap_io_mapping().
> >> Only hw that supports mappable aperture would hit this path
> >> remap_io_mapping(), So skip this code for non-x86 architectures.
> >
> >Patch changelog goes here.
> >
> >> Signed-off-by: Mullati Siva <siva.mullati@intel.com>
>
> Is your git config correct?
>
>
> >> ---
> >>  drivers/gpu/drm/i915/i915_mm.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_mm.c b/drivers/gpu/drm/i915/i915_mm.c
> >> index 666808cb3a32..d76feeaf3fd1 100644
> >> --- a/drivers/gpu/drm/i915/i915_mm.c
> >> +++ b/drivers/gpu/drm/i915/i915_mm.c
> >> @@ -91,6 +91,7 @@ int remap_io_mapping(struct vm_area_struct *vma,
> >>                   unsigned long addr, unsigned long pfn, unsigned long size,
> >>                   struct io_mapping *iomap)
> >>  {
> >> +#if IS_ENABLED(CONFIG_X86)
> >
> >My feedback to the previous version was:
> >
> >Please don't add conditional compilation within functions.
> >
> >I mean it.
>
> if it's not clear, that means we should have a stub remap_io_mapping()
> in the header rather than doing the conditional compilation here.
>
> However, I'm still not confident about the approach since in DG1 for
> example we have mappable aperture. And even for the cases we don't need
> it, are you sure we never call remap_io_mapping()?  Did you add a trace
> or printk() in this function to confirm that while loading the module?

We don't probe the gmadr on discrete, and so i915_ggtt_has_aperture()
should always return false, which should make hitting this path
impossible. Also for discrete the fault handler stuff all goes though
vm_fault_ttm only, which doesn't use any of this stuff.

>
> Lastly, see commit b87482dfe800 ("Revert "i915: use io_mapping_map_user"")
> We had a function added in mm/io-mapping.c - io_mapping_map_user() that
> is now unused.
>
> We still want to use that api (see https://lore.kernel.org/all/20211021061839.GA27953@lst.de/so).
> So since you are touching this area, it would be good if you can help
> figure out why it was failing.

Apparently some IGTs were able to reproduce it in case that helps:
https://gitlab.freedesktop.org/drm/intel/-/issues/3468

There was also an issue with allocation vs mapping cache type
mismatch, but I guess the BUG_ON(!pte_none(*pte)) is something else.

>
> thanks
> Lucas De Marchi