[PATCH 1/1] drm/i915: Drop user contexts on device remove

[Janusz Krzysztofik]

On Fri, 2020-04-24 at 15:46 +0100, Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2020-04-24 15:40:44)
> > On Fri, 2020-04-24 at 15:23 +0100, Chris Wilson wrote:
> > > Quoting Janusz Krzysztofik (2020-04-24 15:20:39)
> > > > Hi Chris,
> > > > 
> > > > On Fri, 2020-04-24 at 15:15 +0100, Chris Wilson wrote:
> > > > > Quoting Chris Wilson (2020-04-24 15:09:27)
> > > > > > Quoting Janusz Krzysztofik (2020-04-24 14:47:42)
> > > > > > > Contexts associated with open file descriptors are now closed on file
> > > > > > > close.  If a device is removed while being open, DMA API warns about
> > > > > > > device associated mappings still active.  Moreover, subsequent removal
> > > > > > > of contexts on device close may trigger a bug in intel-iommu code on
> > > > > > > dma_unmap attempt.
> > > > > > 
> > > > > > The contexts are not the primary association with dma-mappings. The
> > > > > > objects are dma-mapped independently, what you are doing here is
> > > > > > indirectly releasing the dma-mapping of the ppGTT page directory.
> > > > > > 
> > > > > > While it seems to be only solving half the problem, automatically
> > > > > > marking the vm as closed on driver_unregister sounds viable. But it
> > > > > > feels like we should just be marking the device as going away and just
> > > > > > ignore all the dma unmapping.
> > > > > > 
> > > > > > We could do something like set up a dummy dma_map_ops in unregister,
> > > > > > and so discard all the unmaps, report error for further maps, and leave
> > > > > > in place until the next device replaces it. It appears that can be done
> > > > > > with just a blank dma_map_ops. Seems cheeky, but it just might work...
> > > > > 
> > > > > I wonder if
> > > > > 
> > > > > void i915_driver_remove(struct drm_i915_private *i915)
> > > > > 
> > > > >         i915_driver_hw_remove(i915);
> > > > > 
> > > > > +       set_dma_ops(&i915->drm.pdev->dev, &dma_dummy_ops);
> > > > > +
> > > > >         enable_rpm_wakeref_asserts(&i915->runtime_pm);
> > > > >  }
> > > > > 
> > > > > is all we need.
> > > > 
> > > > Thanks for sharing that idea.  I can try to implement it if you haven't
> > > > started yet.
> > > 
> > > One also needs to clear (set_dma_ops(dev, NULL)) in late_release.
> > > 
> > > It may work to prevent us doing unmaps while the iommu is not registered
> > > to us, but it doesn't help intel-iommu work across rebinds :(
> > 
> > Save original dma_ops before setting up dummy ones, then restore them
> > in late_release?
> 
> If we aren't using the direct dma (NULL ops) we are going to be terribly
> broken :)

Yes, I've already realized there is nothing to restore.  Do I
understand correctly that there will be a problem with not revoked
mappings if the device is still the same?

Thanks,
Janusz