| Message ID | 20211112171828.21770-1-siva.mullati@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Skip remap_io_mapping() for non-x86 platforms | expand |
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> > --- > 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. > struct remap_pfn r; > int err; > > @@ -108,6 +109,7 @@ int remap_io_mapping(struct vm_area_struct *vma, > zap_vma_ptes(vma, addr, (r.pfn - pfn) << PAGE_SHIFT); > return err; > } > +#endif > > return 0; > }
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? 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. thanks Lucas De Marchi
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
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) struct remap_pfn r; int err; @@ -108,6 +109,7 @@ int remap_io_mapping(struct vm_area_struct *vma, zap_vma_ptes(vma, addr, (r.pfn - pfn) << PAGE_SHIFT); return err; } +#endif return 0; }
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. Signed-off-by: Mullati Siva <siva.mullati@intel.com> --- drivers/gpu/drm/i915/i915_mm.c | 2 ++ 1 file changed, 2 insertions(+)