| Message ID | 20190906214409.26677-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iommu: handle drivers that manage iommu directly | expand |
On 06/09/2019 22:44, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > One of the challenges we have to enable the aarch64 laptops upstream > is dealing with the fact that the bootloader enables the display and > takes the corresponding SMMU context-bank out of BYPASS. Unfortunately, > currently, the IOMMU framework attaches a DMA (or potentially an > IDENTITY) domain before the driver is probed and has a chance to > intervene and shutdown scanout. Which makes things go horribly wrong. Nope, things already went horribly wrong in arm_smmu_device_reset() - sure, sometimes for some configurations it might *seem* like they didn't and that you can fudge the context bank state at arm's length from core code later, but the truth is that impl->cfg_probe is your last chance to guarantee that any necessary SMMU state is preserved. The remainder of the problem involves reworking default domain allocation such that we can converge on what iommu_request_dm_for_dev() currently does but without the momentary attachment to a translation domain to cause hiccups. That's starting here: https://lore.kernel.org/linux-iommu/cover.1566353521.git.sai.praneeth.prakhya@intel.com/ > But in this case, drm/msm is already directly managing it's IOMMUs > directly, the DMA API attached iommu_domain simply gets in the way. > This series adds a way that a driver can indicate to drivers/iommu > that it does not wish to have an DMA managed iommu_domain attached. > This way, drm/msm can shut down scanout cleanly before attaching it's > own iommu_domain. > > NOTE that to get things working with arm-smmu on the aarch64 laptops, > you also need a patchset[1] from Bjorn Andersson to inherit SMMU config > at boot, when it is already enabled. > > [1] https://www.spinics.net/lists/arm-kernel/msg732246.html > > NOTE that in discussion of previous revisions, RMRR came up. This is > not really a replacement for RMRR (nor does RMRR really provide any > more information than we already get from EFI GOP, or DT in the > simplefb case). I also don't see how RMRR could help w/ SMMU handover > of CB/SMR config (Bjorn's patchset[1]) without defining new tables. The point of RMRR-like-things is that they identify not just the memory region but also the specific device accessing them, which means the IOMMU driver knows up-front which IDs etc. it must be careful not to disrupt. Obviously for SMMU that *would* be some new table (designed to encompass everything relevant) since literal RMRRs are specifically an Intel VT-d thing. > This perhaps doesn't solve the more general case of bootloader enabled > display for drivers that actually want to use DMA API managed IOMMU. > But it does also happen to avoid a related problem with GPU, caused by > the DMA domain claiming the context bank that the GPU firmware expects > to use. Careful bringing that up again, or I really will rework the context bank allocator to avoid this default domain problem entirely... ;) Robin. > And it avoids spurious TLB invalidation coming from the unused > DMA domain. So IMHO this is a useful and necessary change. > > Rob Clark (2): > iommu: add support for drivers that manage iommu explicitly > drm/msm: mark devices where iommu is managed by driver > > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + > drivers/gpu/drm/msm/msm_drv.c | 1 + > drivers/iommu/iommu.c | 2 +- > drivers/iommu/of_iommu.c | 3 +++ > include/linux/device.h | 3 ++- > 7 files changed, 10 insertions(+), 2 deletions(-) >
On Tue, Sep 10, 2019 at 9:34 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 06/09/2019 22:44, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > One of the challenges we have to enable the aarch64 laptops upstream > > is dealing with the fact that the bootloader enables the display and > > takes the corresponding SMMU context-bank out of BYPASS. Unfortunately, > > currently, the IOMMU framework attaches a DMA (or potentially an > > IDENTITY) domain before the driver is probed and has a chance to > > intervene and shutdown scanout. Which makes things go horribly wrong. > > Nope, things already went horribly wrong in arm_smmu_device_reset() - > sure, sometimes for some configurations it might *seem* like they didn't > and that you can fudge the context bank state at arm's length from core > code later, but the truth is that impl->cfg_probe is your last chance to > guarantee that any necessary SMMU state is preserved. cfg_probe is where Bjorn's patch is preserving the SMMU state. So I think that should ensure device_reset() preserves the configuration.. or at least if something is missing, that seems fixable. > The remainder of the problem involves reworking default domain > allocation such that we can converge on what iommu_request_dm_for_dev() > currently does but without the momentary attachment to a translation > domain to cause hiccups. That's starting here: > > https://lore.kernel.org/linux-iommu/cover.1566353521.git.sai.praneeth.prakhya@intel.com/ I suppose if the stream-match state and bootloader chosen context bank is preserved, then keeping it direct-mapped would avoid things starting to fault before display driver is probed. That plus some solution for GPU default domain would narrow the scope of what I need to just avoiding getting iommu dma_ops installed. > > But in this case, drm/msm is already directly managing it's IOMMUs > > directly, the DMA API attached iommu_domain simply gets in the way. > > This series adds a way that a driver can indicate to drivers/iommu > > that it does not wish to have an DMA managed iommu_domain attached. > > This way, drm/msm can shut down scanout cleanly before attaching it's > > own iommu_domain. > > > > NOTE that to get things working with arm-smmu on the aarch64 laptops, > > you also need a patchset[1] from Bjorn Andersson to inherit SMMU config > > at boot, when it is already enabled. > > > > [1] https://www.spinics.net/lists/arm-kernel/msg732246.html > > > > NOTE that in discussion of previous revisions, RMRR came up. This is > > not really a replacement for RMRR (nor does RMRR really provide any > > more information than we already get from EFI GOP, or DT in the > > simplefb case). I also don't see how RMRR could help w/ SMMU handover > > of CB/SMR config (Bjorn's patchset[1]) without defining new tables. > > The point of RMRR-like-things is that they identify not just the memory > region but also the specific device accessing them, which means the > IOMMU driver knows up-front which IDs etc. it must be careful not to > disrupt. Obviously for SMMU that *would* be some new table (designed to > encompass everything relevant) since literal RMRRs are specifically an > Intel VT-d thing. Perhaps I'm not looking in the right place, but the extent of what I could find about RMRR tables was: https://github.com/tianocore/edk2/blob/master/MdePkg/Include/IndustryStandard/DmaRemappingReportingTable.h#L122 I couldn't really see how that specifies the device. But entirely possible that I'm not seeing the whole picture. I am a bit curious about how windows handles this, since they must have the same problem on these laptops. > > This perhaps doesn't solve the more general case of bootloader enabled > > display for drivers that actually want to use DMA API managed IOMMU. > > But it does also happen to avoid a related problem with GPU, caused by > > the DMA domain claiming the context bank that the GPU firmware expects > > to use. > > Careful bringing that up again, or I really will rework the context bank > allocator to avoid this default domain problem entirely... ;) That doesn't seem like a bad outcome ;-) BR, -R > Robin. > > > And it avoids spurious TLB invalidation coming from the unused > > DMA domain. So IMHO this is a useful and necessary change. > > > > Rob Clark (2): > > iommu: add support for drivers that manage iommu explicitly > > drm/msm: mark devices where iommu is managed by driver > > > > drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + > > drivers/gpu/drm/msm/msm_drv.c | 1 + > > drivers/iommu/iommu.c | 2 +- > > drivers/iommu/of_iommu.c | 3 +++ > > include/linux/device.h | 3 ++- > > 7 files changed, 10 insertions(+), 2 deletions(-) > >
On Tue, Sep 10, 2019 at 10:10:49AM -0700, Rob Clark wrote: > On Tue, Sep 10, 2019 at 9:34 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 06/09/2019 22:44, Rob Clark wrote: > > > NOTE that in discussion of previous revisions, RMRR came up. This is > > > not really a replacement for RMRR (nor does RMRR really provide any > > > more information than we already get from EFI GOP, or DT in the > > > simplefb case). I also don't see how RMRR could help w/ SMMU handover > > > of CB/SMR config (Bjorn's patchset[1]) without defining new tables. > > > > The point of RMRR-like-things is that they identify not just the memory > > region but also the specific device accessing them, which means the > > IOMMU driver knows up-front which IDs etc. it must be careful not to > > disrupt. Obviously for SMMU that *would* be some new table (designed to > > encompass everything relevant) since literal RMRRs are specifically an > > Intel VT-d thing. > > Perhaps I'm not looking in the right place, but the extent of what I > could find about RMRR tables was: > > https://github.com/tianocore/edk2/blob/master/MdePkg/Include/IndustryStandard/DmaRemappingReportingTable.h#L122 > > I couldn't really see how that specifies the device. But entirely > possible that I'm not seeing the whole picture. I don't think anybody was planning to implement RMRR "as-is" for arm64, so you might be better off looking at the proposal from Thierry, although it has some issues that are still to be resolved: http://lkml.kernel.org/r/20190829111407.17191-1-thierry.reding@gmail.com Will
From: Rob Clark <robdclark@chromium.org> One of the challenges we have to enable the aarch64 laptops upstream is dealing with the fact that the bootloader enables the display and takes the corresponding SMMU context-bank out of BYPASS. Unfortunately, currently, the IOMMU framework attaches a DMA (or potentially an IDENTITY) domain before the driver is probed and has a chance to intervene and shutdown scanout. Which makes things go horribly wrong. But in this case, drm/msm is already directly managing it's IOMMUs directly, the DMA API attached iommu_domain simply gets in the way. This series adds a way that a driver can indicate to drivers/iommu that it does not wish to have an DMA managed iommu_domain attached. This way, drm/msm can shut down scanout cleanly before attaching it's own iommu_domain. NOTE that to get things working with arm-smmu on the aarch64 laptops, you also need a patchset[1] from Bjorn Andersson to inherit SMMU config at boot, when it is already enabled. [1] https://www.spinics.net/lists/arm-kernel/msg732246.html NOTE that in discussion of previous revisions, RMRR came up. This is not really a replacement for RMRR (nor does RMRR really provide any more information than we already get from EFI GOP, or DT in the simplefb case). I also don't see how RMRR could help w/ SMMU handover of CB/SMR config (Bjorn's patchset[1]) without defining new tables. This perhaps doesn't solve the more general case of bootloader enabled display for drivers that actually want to use DMA API managed IOMMU. But it does also happen to avoid a related problem with GPU, caused by the DMA domain claiming the context bank that the GPU firmware expects to use. And it avoids spurious TLB invalidation coming from the unused DMA domain. So IMHO this is a useful and necessary change. Rob Clark (2): iommu: add support for drivers that manage iommu explicitly drm/msm: mark devices where iommu is managed by driver drivers/gpu/drm/msm/adreno/adreno_device.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + drivers/gpu/drm/msm/msm_drv.c | 1 + drivers/iommu/iommu.c | 2 +- drivers/iommu/of_iommu.c | 3 +++ include/linux/device.h | 3 ++- 7 files changed, 10 insertions(+), 2 deletions(-)