Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Wed, Feb 14, 2018 at 11:09 PM, Tomasz Figawrote: > On Thu, Feb 15, 2018 at 1:12 AM, Rob Clark wrote: >> On Wed, Feb 14, 2018 at 10:48 AM, Jordan Crouse >> wrote: >>> On Wed, Feb 14, 2018 at 12:31:29PM +0900, Tomasz Figa wrote: - When submitting commands to the GPU, the GPU driver will pm_runtime_get_sync() on the GPU device, which will automatically do the same on all the linked suppliers, which would also include the SMMU itself. The role of device links here is exactly that the GPU driver doesn't have to care which other devices need to be brought up. >>> >>> This is true. Assuming that the device link works correctly we would not >>> need >>> to explicitly power the SMMU which makes my point entirely moot. >> >> Just to point out what motivated this patchset, the biggest problem is >> iommu_unmap() because that can happen when GPU is not powered on (or >> in the v4l2 case, because some other device dropped it's reference to >> the dma-buf allowing it to be free'd). Currently we pm get/put the >> GPU device around unmap, but it is kinda silly to boot up the GPU just >> to unmap a buffer. > > Note that in V4L2 both mapping and unmapping can happen completely > without involving the driver. So AFAICT the approach being implemented > by this patchset will not work, because there will be no one to power > up the IOMMU before the operation. Moreover, there are platforms for > which there is no reason to power up the IOMMU just for map/unmap, > because the hardware state is lost anyway and the only real work > needed is updating the page tables in memory. (I feel like this is > actually true for most of the platforms in the wild, but this is based > purely on the not so small number of platforms I worked with, haven't > bothered looking for more general evidence.) > At least as far as drm/msm/adreno, I'm not terribly concerned about other platforms that don't need to power up iommu. It's not really the same situation as a IP block that shows up in all different vendor's SoCs. But if you can convince Robin to go for get/put_sync() calls inside the iommu driver, I'm fine with that approach too. That is what I do in qcom_iommu already. But if not I'd like to at least solve this for some platforms if we can't solve for all. BR, -R >> >> (Semi-related, I would also like to batch map/unmap's, I just haven't >> gotten around to implementing it yet.. but that would be another case >> where a single get_supplier()/put_supplier() outside of the iommu >> would make sense instead of pm_get/put() inside the iommu driver's >> ->unmap().) >> >> If you really dislike the get/put_supplier() approach, then perhaps we >> need iommu_pm_get()/iommu_pm_put() operations that the iommu user >> could use to accomplish the same thing? > > I'm afraid this wouldn't work for V4L2 either. And I still haven't > been given any evidence that the approach I'm suggesting, which relies > only on existing pieces of infrastructure and which worked for both > Exynos and Rockchip, including V4L2, wouldn't work for SMMU and/or QC > SoCs. > > Best regards, > Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Thu, Feb 15, 2018 at 12:17 PM, Tomasz Figawrote: > On Thu, Feb 15, 2018 at 1:03 AM, Robin Murphy wrote: >> On 14/02/18 10:33, Vivek Gautam wrote: >>> >>> On Wed, Feb 14, 2018 at 2:46 PM, Tomasz Figa wrote: >>> >>> Adding Jordan to this thread as well. >>> On Wed, Feb 14, 2018 at 6:13 PM, Vivek Gautam wrote: > > Hi Tomasz, > > On Wed, Feb 14, 2018 at 11:08 AM, Tomasz Figa > wrote: >> >> On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautam >> wrote: >>> >>> Hi Tomasz, >>> >>> On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa >>> wrote: On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark wrote: > > On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa > wrote: >> >> On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark >> wrote: >>> >>> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa >>> wrote: Hi Vivek, Thanks for the patch. Please see my comments inline. On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam wrote: > > While handling the concerned iommu, there should not be a > need to power control the drm devices from iommu interface. > If these drm devices need to be powered around this time, > the respective drivers should take care of this. > > Replace the pm_runtime_get/put_sync() with > pm_runtime_get/put_suppliers() calls, to power-up > the connected iommu through the device link interface. > In case the device link is not setup these get/put_suppliers() > calls will be a no-op, and the iommu driver should take care of > powering on its devices accordingly. > > Signed-off-by: Vivek Gautam > --- > drivers/gpu/drm/msm/msm_iommu.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c > b/drivers/gpu/drm/msm/msm_iommu.c > index b23d33622f37..1ab629bbee69 100644 > --- a/drivers/gpu/drm/msm/msm_iommu.c > +++ b/drivers/gpu/drm/msm/msm_iommu.c > @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu > *mmu, const char * const *names, > struct msm_iommu *iommu = to_msm_iommu(mmu); > int ret; > > - pm_runtime_get_sync(mmu->dev); > + pm_runtime_get_suppliers(mmu->dev); > ret = iommu_attach_device(iommu->domain, mmu->dev); > - pm_runtime_put_sync(mmu->dev); > + pm_runtime_put_suppliers(mmu->dev); For me, it looks like a wrong place to handle runtime PM of IOMMU here. iommu_attach_device() calls into IOMMU driver's attach_device() callback and that's where necessary runtime PM gets should happen, if any. In other words, driver A (MSM DRM driver) shouldn't be dealing with power state of device controlled by driver B (ARM SMMU). >>> >>> >>> Note that we end up having to do the same, because of >>> iommu_unmap() >>> while DRM driver is powered off.. it might be cleaner if it was >>> all >>> self contained in the iommu driver, but that would make it so >>> other >>> drivers couldn't call iommu_unmap() from an irq handler, which is >>> apparently something that some of them want to do.. >> >> >> I'd assume that runtime PM status is already guaranteed to be >> active >> when the IRQ handler is running, by some other means (e.g. >> pm_runtime_get_sync() called earlier, when queuing some work to the >> hardware). Otherwise, I'm not sure how a powered down device could >> trigger an IRQ. >> >> So, if the master device power is already on, suppliers should be >> powered on as well, thanks to device links. >> > > umm, that is kindof the inverse of the problem.. the problem is > things like gpu driver (and v4l2 drivers that import dma-buf's, > afaict).. they will potentially call iommu->unmap() when device is > not > active (due to userspace or things beyond the control of the > driver).. > so *they* would want iommu to do pm get/put
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Thu, Feb 15, 2018 at 1:12 AM, Rob Clarkwrote: > On Wed, Feb 14, 2018 at 10:48 AM, Jordan Crouse > wrote: >> On Wed, Feb 14, 2018 at 12:31:29PM +0900, Tomasz Figa wrote: >>> >>> - When submitting commands to the GPU, the GPU driver will >>> pm_runtime_get_sync() on the GPU device, which will automatically do >>> the same on all the linked suppliers, which would also include the >>> SMMU itself. The role of device links here is exactly that the GPU >>> driver doesn't have to care which other devices need to be brought up. >> >> This is true. Assuming that the device link works correctly we would not >> need >> to explicitly power the SMMU which makes my point entirely moot. > > Just to point out what motivated this patchset, the biggest problem is > iommu_unmap() because that can happen when GPU is not powered on (or > in the v4l2 case, because some other device dropped it's reference to > the dma-buf allowing it to be free'd). Currently we pm get/put the > GPU device around unmap, but it is kinda silly to boot up the GPU just > to unmap a buffer. Note that in V4L2 both mapping and unmapping can happen completely without involving the driver. So AFAICT the approach being implemented by this patchset will not work, because there will be no one to power up the IOMMU before the operation. Moreover, there are platforms for which there is no reason to power up the IOMMU just for map/unmap, because the hardware state is lost anyway and the only real work needed is updating the page tables in memory. (I feel like this is actually true for most of the platforms in the wild, but this is based purely on the not so small number of platforms I worked with, haven't bothered looking for more general evidence.) > > (Semi-related, I would also like to batch map/unmap's, I just haven't > gotten around to implementing it yet.. but that would be another case > where a single get_supplier()/put_supplier() outside of the iommu > would make sense instead of pm_get/put() inside the iommu driver's > ->unmap().) > > If you really dislike the get/put_supplier() approach, then perhaps we > need iommu_pm_get()/iommu_pm_put() operations that the iommu user > could use to accomplish the same thing? I'm afraid this wouldn't work for V4L2 either. And I still haven't been given any evidence that the approach I'm suggesting, which relies only on existing pieces of infrastructure and which worked for both Exynos and Rockchip, including V4L2, wouldn't work for SMMU and/or QC SoCs. Best regards, Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Thu, Feb 15, 2018 at 1:03 AM, Robin Murphywrote: > On 14/02/18 10:33, Vivek Gautam wrote: >> >> On Wed, Feb 14, 2018 at 2:46 PM, Tomasz Figa wrote: >> >> Adding Jordan to this thread as well. >> >>> On Wed, Feb 14, 2018 at 6:13 PM, Vivek Gautam >>> wrote: Hi Tomasz, On Wed, Feb 14, 2018 at 11:08 AM, Tomasz Figa wrote: > > On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautam > wrote: >> >> Hi Tomasz, >> >> On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa >> wrote: >>> >>> On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark >>> wrote: On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa wrote: > > On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark > wrote: >> >> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa >> wrote: >>> >>> Hi Vivek, >>> >>> Thanks for the patch. Please see my comments inline. >>> >>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >>> wrote: While handling the concerned iommu, there should not be a need to power control the drm devices from iommu interface. If these drm devices need to be powered around this time, the respective drivers should take care of this. Replace the pm_runtime_get/put_sync() with pm_runtime_get/put_suppliers() calls, to power-up the connected iommu through the device link interface. In case the device link is not setup these get/put_suppliers() calls will be a no-op, and the iommu driver should take care of powering on its devices accordingly. Signed-off-by: Vivek Gautam --- drivers/gpu/drm/msm/msm_iommu.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index b23d33622f37..1ab629bbee69 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char * const *names, struct msm_iommu *iommu = to_msm_iommu(mmu); int ret; - pm_runtime_get_sync(mmu->dev); + pm_runtime_get_suppliers(mmu->dev); ret = iommu_attach_device(iommu->domain, mmu->dev); - pm_runtime_put_sync(mmu->dev); + pm_runtime_put_suppliers(mmu->dev); >>> >>> >>> For me, it looks like a wrong place to handle runtime PM of IOMMU >>> here. iommu_attach_device() calls into IOMMU driver's >>> attach_device() >>> callback and that's where necessary runtime PM gets should >>> happen, if >>> any. In other words, driver A (MSM DRM driver) shouldn't be >>> dealing >>> with power state of device controlled by driver B (ARM SMMU). >> >> >> Note that we end up having to do the same, because of >> iommu_unmap() >> while DRM driver is powered off.. it might be cleaner if it was >> all >> self contained in the iommu driver, but that would make it so >> other >> drivers couldn't call iommu_unmap() from an irq handler, which is >> apparently something that some of them want to do.. > > > I'd assume that runtime PM status is already guaranteed to be > active > when the IRQ handler is running, by some other means (e.g. > pm_runtime_get_sync() called earlier, when queuing some work to the > hardware). Otherwise, I'm not sure how a powered down device could > trigger an IRQ. > > So, if the master device power is already on, suppliers should be > powered on as well, thanks to device links. > umm, that is kindof the inverse of the problem.. the problem is things like gpu driver (and v4l2 drivers that import dma-buf's, afaict).. they will potentially call iommu->unmap() when device is not active (due to userspace or things beyond the control of the driver).. so *they* would want iommu to do pm get/put calls. >>> >>> >>> Which is fine and which is actually already done by one of the >>> patches >>> in this series, not for map/unmap, but probe, add_device, >>>
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On 14/02/18 10:33, Vivek Gautam wrote: On Wed, Feb 14, 2018 at 2:46 PM, Tomasz Figawrote: Adding Jordan to this thread as well. On Wed, Feb 14, 2018 at 6:13 PM, Vivek Gautam wrote: Hi Tomasz, On Wed, Feb 14, 2018 at 11:08 AM, Tomasz Figa wrote: On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautam wrote: Hi Tomasz, On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa wrote: On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark wrote: On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa wrote: On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark wrote: On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa wrote: Hi Vivek, Thanks for the patch. Please see my comments inline. On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam wrote: While handling the concerned iommu, there should not be a need to power control the drm devices from iommu interface. If these drm devices need to be powered around this time, the respective drivers should take care of this. Replace the pm_runtime_get/put_sync() with pm_runtime_get/put_suppliers() calls, to power-up the connected iommu through the device link interface. In case the device link is not setup these get/put_suppliers() calls will be a no-op, and the iommu driver should take care of powering on its devices accordingly. Signed-off-by: Vivek Gautam --- drivers/gpu/drm/msm/msm_iommu.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index b23d33622f37..1ab629bbee69 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char * const *names, struct msm_iommu *iommu = to_msm_iommu(mmu); int ret; - pm_runtime_get_sync(mmu->dev); + pm_runtime_get_suppliers(mmu->dev); ret = iommu_attach_device(iommu->domain, mmu->dev); - pm_runtime_put_sync(mmu->dev); + pm_runtime_put_suppliers(mmu->dev); For me, it looks like a wrong place to handle runtime PM of IOMMU here. iommu_attach_device() calls into IOMMU driver's attach_device() callback and that's where necessary runtime PM gets should happen, if any. In other words, driver A (MSM DRM driver) shouldn't be dealing with power state of device controlled by driver B (ARM SMMU). Note that we end up having to do the same, because of iommu_unmap() while DRM driver is powered off.. it might be cleaner if it was all self contained in the iommu driver, but that would make it so other drivers couldn't call iommu_unmap() from an irq handler, which is apparently something that some of them want to do.. I'd assume that runtime PM status is already guaranteed to be active when the IRQ handler is running, by some other means (e.g. pm_runtime_get_sync() called earlier, when queuing some work to the hardware). Otherwise, I'm not sure how a powered down device could trigger an IRQ. So, if the master device power is already on, suppliers should be powered on as well, thanks to device links. umm, that is kindof the inverse of the problem.. the problem is things like gpu driver (and v4l2 drivers that import dma-buf's, afaict).. they will potentially call iommu->unmap() when device is not active (due to userspace or things beyond the control of the driver).. so *they* would want iommu to do pm get/put calls. Which is fine and which is actually already done by one of the patches in this series, not for map/unmap, but probe, add_device, remove_device. Having parts of the API doing it inside the callback and other parts outside sounds at least inconsistent. But other drivers trying to unmap from irq ctx would not. Which is the contradictory requirement that lead to the idea of iommu user powering up iommu for unmap. Sorry, maybe I wasn't clear. My last message was supposed to show that it's not contradictory at all, because "other drivers trying to unmap from irq ctx" would already have called pm_runtime_get_*() earlier from a non-irq ctx, which would have also done the same on all the linked suppliers, including the IOMMU. The ultimate result would be that the map/unmap() of the IOMMU driver calling pm_runtime_get_sync() would do nothing besides incrementing the reference count. The entire point was to avoid the slowpath that pm_runtime_get/put_sync() would add in map/unmap. It would not be correct to add a slowpath in irq_ctx for taking care of non-irq_ctx and for the situations where master is already powered-off. Correct me if I'm wrong, but I believe that with what I'm proposing there wouldn't be any slow path. Yea, but only when the power domain is irq-safe? And not all platforms enable irq-safe power domains. For instance, msm doesn't enable its
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Wed, Feb 14, 2018 at 12:31:29PM +0900, Tomasz Figa wrote: > Hi Jordan, > > On Wed, Feb 14, 2018 at 1:42 AM, Jordan Crousewrote: > > On Tue, Feb 13, 2018 at 06:10:38PM +0900, Tomasz Figa wrote: > >> Hi Vivek, > >> > >> Thanks for the patch. Please see my comments inline. > >> > >> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam > >> wrote: > >> > While handling the concerned iommu, there should not be a > >> > need to power control the drm devices from iommu interface. > >> > If these drm devices need to be powered around this time, > >> > the respective drivers should take care of this. > >> > > >> > Replace the pm_runtime_get/put_sync() with > >> > pm_runtime_get/put_suppliers() calls, to power-up > >> > the connected iommu through the device link interface. > >> > In case the device link is not setup these get/put_suppliers() > >> > calls will be a no-op, and the iommu driver should take care of > >> > powering on its devices accordingly. > >> > > >> > Signed-off-by: Vivek Gautam > >> > --- > >> > drivers/gpu/drm/msm/msm_iommu.c | 16 > >> > 1 file changed, 8 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/msm/msm_iommu.c > >> > b/drivers/gpu/drm/msm/msm_iommu.c > >> > index b23d33622f37..1ab629bbee69 100644 > >> > --- a/drivers/gpu/drm/msm/msm_iommu.c > >> > +++ b/drivers/gpu/drm/msm/msm_iommu.c > >> > @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const > >> > char * const *names, > >> > struct msm_iommu *iommu = to_msm_iommu(mmu); > >> > int ret; > >> > > >> > - pm_runtime_get_sync(mmu->dev); > >> > + pm_runtime_get_suppliers(mmu->dev); > >> > ret = iommu_attach_device(iommu->domain, mmu->dev); > >> > - pm_runtime_put_sync(mmu->dev); > >> > + pm_runtime_put_suppliers(mmu->dev); > >> > >> For me, it looks like a wrong place to handle runtime PM of IOMMU > >> here. iommu_attach_device() calls into IOMMU driver's attach_device() > >> callback and that's where necessary runtime PM gets should happen, if > >> any. In other words, driver A (MSM DRM driver) shouldn't be dealing > >> with power state of device controlled by driver B (ARM SMMU). > > > > This whole thing is confused by the fact that on MSM the GPU and the GPU > > IOMMU > > share some of the same clocks and power rail so turning on the GPU also > > turned on the IOMMU register banks by extension. > > This is surprisingly not a very surprising case. Exactly the same can > be seen on Rockchip SoCs and we're solving the problem using the > solution I suggested. In fact, my suggestions to this thread are based > on the design we chose for Rockchip, due to the high level of > similarity (+/- the GPU directly programming IOMMU registers, which is > not present there, but AFAICT it doesn't pose a problem here). > > > > > But if we put that aside the question is who should be responsible for > > controlling the power in this relationship and there are several good > > reasons to > > leave it up to the client device. The most important reason is when we move > > to > > the per-instance model where the GPU self-programmings the SMMU registers. > > In > > that case, the driver will need to make sure that the SMMU is powered up > > before > > submitting the command and then removing the power vote when the commands > > are done to save energy. > > I might need more insight on what's going on in your hardware, but > with my current understanding I'd argue that that is not right, > because: > > - When submitting commands to the GPU, the GPU driver will > pm_runtime_get_sync() on the GPU device, which will automatically do > the same on all the linked suppliers, which would also include the > SMMU itself. The role of device links here is exactly that the GPU > driver doesn't have to care which other devices need to be brought up. This is true. Assuming that the device link works correctly we would not need to explicitly power the SMMU which makes my point entirely moot. > - When the GPU is operating, the SMMU power must be supplied anyway, > because it needs to be doing the translations, right? Note that by > "power" I really mean the physical power supply in the SoC, e.g. as > for a power domain. The runtime PM API in its current form (e.g. > binary off or on operation) is unsuitable for managing other things, > such as clocks (and there is ongoing work on improving it, e.g. by > adding support for multiple power states). As others have pointed out, the register banks and the translation unit are powered separately (or at least, clocked separately). >^^ The above would be actually guaranteed by your hardware design, > where SMMU and GPU share the power domain and clocks. (We used to rely > on this in old downstream implementation of Rockchip IOMMU and master > drivers in Chromium OS kernel, before we moved to handling the clocks > explicitly in
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Wed, Feb 14, 2018 at 2:46 PM, Tomasz Figawrote: Adding Jordan to this thread as well. > On Wed, Feb 14, 2018 at 6:13 PM, Vivek Gautam > wrote: >> Hi Tomasz, >> >> On Wed, Feb 14, 2018 at 11:08 AM, Tomasz Figa wrote: >>> On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautam >>> wrote: Hi Tomasz, On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa wrote: > On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark wrote: >> On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa wrote: >>> On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark wrote: On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa wrote: > Hi Vivek, > > Thanks for the patch. Please see my comments inline. > > On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam > wrote: >> While handling the concerned iommu, there should not be a >> need to power control the drm devices from iommu interface. >> If these drm devices need to be powered around this time, >> the respective drivers should take care of this. >> >> Replace the pm_runtime_get/put_sync() with >> pm_runtime_get/put_suppliers() calls, to power-up >> the connected iommu through the device link interface. >> In case the device link is not setup these get/put_suppliers() >> calls will be a no-op, and the iommu driver should take care of >> powering on its devices accordingly. >> >> Signed-off-by: Vivek Gautam >> --- >> drivers/gpu/drm/msm/msm_iommu.c | 16 >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_iommu.c >> b/drivers/gpu/drm/msm/msm_iommu.c >> index b23d33622f37..1ab629bbee69 100644 >> --- a/drivers/gpu/drm/msm/msm_iommu.c >> +++ b/drivers/gpu/drm/msm/msm_iommu.c >> @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, >> const char * const *names, >> struct msm_iommu *iommu = to_msm_iommu(mmu); >> int ret; >> >> - pm_runtime_get_sync(mmu->dev); >> + pm_runtime_get_suppliers(mmu->dev); >> ret = iommu_attach_device(iommu->domain, mmu->dev); >> - pm_runtime_put_sync(mmu->dev); >> + pm_runtime_put_suppliers(mmu->dev); > > For me, it looks like a wrong place to handle runtime PM of IOMMU > here. iommu_attach_device() calls into IOMMU driver's attach_device() > callback and that's where necessary runtime PM gets should happen, if > any. In other words, driver A (MSM DRM driver) shouldn't be dealing > with power state of device controlled by driver B (ARM SMMU). Note that we end up having to do the same, because of iommu_unmap() while DRM driver is powered off.. it might be cleaner if it was all self contained in the iommu driver, but that would make it so other drivers couldn't call iommu_unmap() from an irq handler, which is apparently something that some of them want to do.. >>> >>> I'd assume that runtime PM status is already guaranteed to be active >>> when the IRQ handler is running, by some other means (e.g. >>> pm_runtime_get_sync() called earlier, when queuing some work to the >>> hardware). Otherwise, I'm not sure how a powered down device could >>> trigger an IRQ. >>> >>> So, if the master device power is already on, suppliers should be >>> powered on as well, thanks to device links. >>> >> >> umm, that is kindof the inverse of the problem.. the problem is >> things like gpu driver (and v4l2 drivers that import dma-buf's, >> afaict).. they will potentially call iommu->unmap() when device is not >> active (due to userspace or things beyond the control of the driver).. >> so *they* would want iommu to do pm get/put calls. > > Which is fine and which is actually already done by one of the patches > in this series, not for map/unmap, but probe, add_device, > remove_device. Having parts of the API doing it inside the callback > and other parts outside sounds at least inconsistent. > >> But other drivers >> trying to unmap from irq ctx would not. Which is the contradictory >> requirement that lead to the idea of iommu user powering up iommu for >> unmap. > > Sorry, maybe I wasn't clear. My last message was supposed to show that > it's not contradictory at all, because "other drivers trying to unmap > from irq ctx" would already have called pm_runtime_get_*() earlier > from a non-irq
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Wed, Feb 14, 2018 at 6:13 PM, Vivek Gautamwrote: > Hi Tomasz, > > On Wed, Feb 14, 2018 at 11:08 AM, Tomasz Figa wrote: >> On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautam >> wrote: >>> Hi Tomasz, >>> >>> On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa wrote: On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark wrote: > On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa wrote: >> On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark wrote: >>> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa wrote: Hi Vivek, Thanks for the patch. Please see my comments inline. On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam wrote: > While handling the concerned iommu, there should not be a > need to power control the drm devices from iommu interface. > If these drm devices need to be powered around this time, > the respective drivers should take care of this. > > Replace the pm_runtime_get/put_sync() with > pm_runtime_get/put_suppliers() calls, to power-up > the connected iommu through the device link interface. > In case the device link is not setup these get/put_suppliers() > calls will be a no-op, and the iommu driver should take care of > powering on its devices accordingly. > > Signed-off-by: Vivek Gautam > --- > drivers/gpu/drm/msm/msm_iommu.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c > b/drivers/gpu/drm/msm/msm_iommu.c > index b23d33622f37..1ab629bbee69 100644 > --- a/drivers/gpu/drm/msm/msm_iommu.c > +++ b/drivers/gpu/drm/msm/msm_iommu.c > @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, > const char * const *names, > struct msm_iommu *iommu = to_msm_iommu(mmu); > int ret; > > - pm_runtime_get_sync(mmu->dev); > + pm_runtime_get_suppliers(mmu->dev); > ret = iommu_attach_device(iommu->domain, mmu->dev); > - pm_runtime_put_sync(mmu->dev); > + pm_runtime_put_suppliers(mmu->dev); For me, it looks like a wrong place to handle runtime PM of IOMMU here. iommu_attach_device() calls into IOMMU driver's attach_device() callback and that's where necessary runtime PM gets should happen, if any. In other words, driver A (MSM DRM driver) shouldn't be dealing with power state of device controlled by driver B (ARM SMMU). >>> >>> Note that we end up having to do the same, because of iommu_unmap() >>> while DRM driver is powered off.. it might be cleaner if it was all >>> self contained in the iommu driver, but that would make it so other >>> drivers couldn't call iommu_unmap() from an irq handler, which is >>> apparently something that some of them want to do.. >> >> I'd assume that runtime PM status is already guaranteed to be active >> when the IRQ handler is running, by some other means (e.g. >> pm_runtime_get_sync() called earlier, when queuing some work to the >> hardware). Otherwise, I'm not sure how a powered down device could >> trigger an IRQ. >> >> So, if the master device power is already on, suppliers should be >> powered on as well, thanks to device links. >> > > umm, that is kindof the inverse of the problem.. the problem is > things like gpu driver (and v4l2 drivers that import dma-buf's, > afaict).. they will potentially call iommu->unmap() when device is not > active (due to userspace or things beyond the control of the driver).. > so *they* would want iommu to do pm get/put calls. Which is fine and which is actually already done by one of the patches in this series, not for map/unmap, but probe, add_device, remove_device. Having parts of the API doing it inside the callback and other parts outside sounds at least inconsistent. > But other drivers > trying to unmap from irq ctx would not. Which is the contradictory > requirement that lead to the idea of iommu user powering up iommu for > unmap. Sorry, maybe I wasn't clear. My last message was supposed to show that it's not contradictory at all, because "other drivers trying to unmap from irq ctx" would already have called pm_runtime_get_*() earlier from a non-irq ctx, which would have also done the same on all the linked suppliers, including the IOMMU. The ultimate result would be that the map/unmap() of the IOMMU driver calling pm_runtime_get_sync() would do
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautamwrote: > Hi Tomasz, > > On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa wrote: >> On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark wrote: >>> On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa wrote: On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark wrote: > On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa wrote: >> Hi Vivek, >> >> Thanks for the patch. Please see my comments inline. >> >> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >> wrote: >>> While handling the concerned iommu, there should not be a >>> need to power control the drm devices from iommu interface. >>> If these drm devices need to be powered around this time, >>> the respective drivers should take care of this. >>> >>> Replace the pm_runtime_get/put_sync() with >>> pm_runtime_get/put_suppliers() calls, to power-up >>> the connected iommu through the device link interface. >>> In case the device link is not setup these get/put_suppliers() >>> calls will be a no-op, and the iommu driver should take care of >>> powering on its devices accordingly. >>> >>> Signed-off-by: Vivek Gautam >>> --- >>> drivers/gpu/drm/msm/msm_iommu.c | 16 >>> 1 file changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_iommu.c >>> b/drivers/gpu/drm/msm/msm_iommu.c >>> index b23d33622f37..1ab629bbee69 100644 >>> --- a/drivers/gpu/drm/msm/msm_iommu.c >>> +++ b/drivers/gpu/drm/msm/msm_iommu.c >>> @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, >>> const char * const *names, >>> struct msm_iommu *iommu = to_msm_iommu(mmu); >>> int ret; >>> >>> - pm_runtime_get_sync(mmu->dev); >>> + pm_runtime_get_suppliers(mmu->dev); >>> ret = iommu_attach_device(iommu->domain, mmu->dev); >>> - pm_runtime_put_sync(mmu->dev); >>> + pm_runtime_put_suppliers(mmu->dev); >> >> For me, it looks like a wrong place to handle runtime PM of IOMMU >> here. iommu_attach_device() calls into IOMMU driver's attach_device() >> callback and that's where necessary runtime PM gets should happen, if >> any. In other words, driver A (MSM DRM driver) shouldn't be dealing >> with power state of device controlled by driver B (ARM SMMU). > > Note that we end up having to do the same, because of iommu_unmap() > while DRM driver is powered off.. it might be cleaner if it was all > self contained in the iommu driver, but that would make it so other > drivers couldn't call iommu_unmap() from an irq handler, which is > apparently something that some of them want to do.. I'd assume that runtime PM status is already guaranteed to be active when the IRQ handler is running, by some other means (e.g. pm_runtime_get_sync() called earlier, when queuing some work to the hardware). Otherwise, I'm not sure how a powered down device could trigger an IRQ. So, if the master device power is already on, suppliers should be powered on as well, thanks to device links. >>> >>> umm, that is kindof the inverse of the problem.. the problem is >>> things like gpu driver (and v4l2 drivers that import dma-buf's, >>> afaict).. they will potentially call iommu->unmap() when device is not >>> active (due to userspace or things beyond the control of the driver).. >>> so *they* would want iommu to do pm get/put calls. >> >> Which is fine and which is actually already done by one of the patches >> in this series, not for map/unmap, but probe, add_device, >> remove_device. Having parts of the API doing it inside the callback >> and other parts outside sounds at least inconsistent. >> >>> But other drivers >>> trying to unmap from irq ctx would not. Which is the contradictory >>> requirement that lead to the idea of iommu user powering up iommu for >>> unmap. >> >> Sorry, maybe I wasn't clear. My last message was supposed to show that >> it's not contradictory at all, because "other drivers trying to unmap >> from irq ctx" would already have called pm_runtime_get_*() earlier >> from a non-irq ctx, which would have also done the same on all the >> linked suppliers, including the IOMMU. The ultimate result would be >> that the map/unmap() of the IOMMU driver calling pm_runtime_get_sync() >> would do nothing besides incrementing the reference count. > > The entire point was to avoid the slowpath that pm_runtime_get/put_sync() > would add in map/unmap. It would not be correct to add a slowpath in irq_ctx > for taking care of non-irq_ctx and for the situations where master is already > powered-off. Correct me if I'm wrong, but I believe that with what I'm
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
Hi Jordan, On Wed, Feb 14, 2018 at 1:42 AM, Jordan Crousewrote: > On Tue, Feb 13, 2018 at 06:10:38PM +0900, Tomasz Figa wrote: >> Hi Vivek, >> >> Thanks for the patch. Please see my comments inline. >> >> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >> wrote: >> > While handling the concerned iommu, there should not be a >> > need to power control the drm devices from iommu interface. >> > If these drm devices need to be powered around this time, >> > the respective drivers should take care of this. >> > >> > Replace the pm_runtime_get/put_sync() with >> > pm_runtime_get/put_suppliers() calls, to power-up >> > the connected iommu through the device link interface. >> > In case the device link is not setup these get/put_suppliers() >> > calls will be a no-op, and the iommu driver should take care of >> > powering on its devices accordingly. >> > >> > Signed-off-by: Vivek Gautam >> > --- >> > drivers/gpu/drm/msm/msm_iommu.c | 16 >> > 1 file changed, 8 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/msm/msm_iommu.c >> > b/drivers/gpu/drm/msm/msm_iommu.c >> > index b23d33622f37..1ab629bbee69 100644 >> > --- a/drivers/gpu/drm/msm/msm_iommu.c >> > +++ b/drivers/gpu/drm/msm/msm_iommu.c >> > @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const >> > char * const *names, >> > struct msm_iommu *iommu = to_msm_iommu(mmu); >> > int ret; >> > >> > - pm_runtime_get_sync(mmu->dev); >> > + pm_runtime_get_suppliers(mmu->dev); >> > ret = iommu_attach_device(iommu->domain, mmu->dev); >> > - pm_runtime_put_sync(mmu->dev); >> > + pm_runtime_put_suppliers(mmu->dev); >> >> For me, it looks like a wrong place to handle runtime PM of IOMMU >> here. iommu_attach_device() calls into IOMMU driver's attach_device() >> callback and that's where necessary runtime PM gets should happen, if >> any. In other words, driver A (MSM DRM driver) shouldn't be dealing >> with power state of device controlled by driver B (ARM SMMU). > > This whole thing is confused by the fact that on MSM the GPU and the GPU IOMMU > share some of the same clocks and power rail so turning on the GPU also > turned on the IOMMU register banks by extension. This is surprisingly not a very surprising case. Exactly the same can be seen on Rockchip SoCs and we're solving the problem using the solution I suggested. In fact, my suggestions to this thread are based on the design we chose for Rockchip, due to the high level of similarity (+/- the GPU directly programming IOMMU registers, which is not present there, but AFAICT it doesn't pose a problem here). > > But if we put that aside the question is who should be responsible for > controlling the power in this relationship and there are several good reasons > to > leave it up to the client device. The most important reason is when we move to > the per-instance model where the GPU self-programmings the SMMU registers. In > that case, the driver will need to make sure that the SMMU is powered up > before > submitting the command and then removing the power vote when the commands > are done to save energy. I might need more insight on what's going on in your hardware, but with my current understanding I'd argue that that is not right, because: - When submitting commands to the GPU, the GPU driver will pm_runtime_get_sync() on the GPU device, which will automatically do the same on all the linked suppliers, which would also include the SMMU itself. The role of device links here is exactly that the GPU driver doesn't have to care which other devices need to be brought up. - When the GPU is operating, the SMMU power must be supplied anyway, because it needs to be doing the translations, right? Note that by "power" I really mean the physical power supply in the SoC, e.g. as for a power domain. The runtime PM API in its current form (e.g. binary off or on operation) is unsuitable for managing other things, such as clocks (and there is ongoing work on improving it, e.g. by adding support for multiple power states). ^^ The above would be actually guaranteed by your hardware design, where SMMU and GPU share the power domain and clocks. (We used to rely on this in old downstream implementation of Rockchip IOMMU and master drivers in Chromium OS kernel, before we moved to handling the clocks explicitly in the IOMMU driver and properly using device links to manage the power domain and state restoration.) > > Additionally, there might be legitimate reasons in the driver to batch > operations - you may wish to attach the device and then map several global > buffers immediately - having driver side control prevents several unneeded > power > transitions. As I mentioned before, these operations wouldn't normally need any power transitions, since mapping with the TLB powered down boils down to just updating the page tables in memory.
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figawrote: > On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark wrote: >> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa wrote: >>> Hi Vivek, >>> >>> Thanks for the patch. Please see my comments inline. >>> >>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >>> wrote: While handling the concerned iommu, there should not be a need to power control the drm devices from iommu interface. If these drm devices need to be powered around this time, the respective drivers should take care of this. Replace the pm_runtime_get/put_sync() with pm_runtime_get/put_suppliers() calls, to power-up the connected iommu through the device link interface. In case the device link is not setup these get/put_suppliers() calls will be a no-op, and the iommu driver should take care of powering on its devices accordingly. Signed-off-by: Vivek Gautam --- drivers/gpu/drm/msm/msm_iommu.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index b23d33622f37..1ab629bbee69 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char * const *names, struct msm_iommu *iommu = to_msm_iommu(mmu); int ret; - pm_runtime_get_sync(mmu->dev); + pm_runtime_get_suppliers(mmu->dev); ret = iommu_attach_device(iommu->domain, mmu->dev); - pm_runtime_put_sync(mmu->dev); + pm_runtime_put_suppliers(mmu->dev); >>> >>> For me, it looks like a wrong place to handle runtime PM of IOMMU >>> here. iommu_attach_device() calls into IOMMU driver's attach_device() >>> callback and that's where necessary runtime PM gets should happen, if >>> any. In other words, driver A (MSM DRM driver) shouldn't be dealing >>> with power state of device controlled by driver B (ARM SMMU). >> >> Note that we end up having to do the same, because of iommu_unmap() >> while DRM driver is powered off.. it might be cleaner if it was all >> self contained in the iommu driver, but that would make it so other >> drivers couldn't call iommu_unmap() from an irq handler, which is >> apparently something that some of them want to do.. > > I'd assume that runtime PM status is already guaranteed to be active > when the IRQ handler is running, by some other means (e.g. > pm_runtime_get_sync() called earlier, when queuing some work to the > hardware). Otherwise, I'm not sure how a powered down device could > trigger an IRQ. > > So, if the master device power is already on, suppliers should be > powered on as well, thanks to device links. > umm, that is kindof the inverse of the problem.. the problem is things like gpu driver (and v4l2 drivers that import dma-buf's, afaict).. they will potentially call iommu->unmap() when device is not active (due to userspace or things beyond the control of the driver).. so *they* would want iommu to do pm get/put calls. But other drivers trying to unmap from irq ctx would not. Which is the contradictory requirement that lead to the idea of iommu user powering up iommu for unmap. There has already been some discussion about this on various earlier permutations of this patchset. I think we have exhausted all other options. BR, -R ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Wed, Feb 14, 2018 at 3:03 AM, Rob Clarkwrote: > On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa wrote: >> Hi Vivek, >> >> Thanks for the patch. Please see my comments inline. >> >> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam >> wrote: >>> While handling the concerned iommu, there should not be a >>> need to power control the drm devices from iommu interface. >>> If these drm devices need to be powered around this time, >>> the respective drivers should take care of this. >>> >>> Replace the pm_runtime_get/put_sync() with >>> pm_runtime_get/put_suppliers() calls, to power-up >>> the connected iommu through the device link interface. >>> In case the device link is not setup these get/put_suppliers() >>> calls will be a no-op, and the iommu driver should take care of >>> powering on its devices accordingly. >>> >>> Signed-off-by: Vivek Gautam >>> --- >>> drivers/gpu/drm/msm/msm_iommu.c | 16 >>> 1 file changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/msm_iommu.c >>> b/drivers/gpu/drm/msm/msm_iommu.c >>> index b23d33622f37..1ab629bbee69 100644 >>> --- a/drivers/gpu/drm/msm/msm_iommu.c >>> +++ b/drivers/gpu/drm/msm/msm_iommu.c >>> @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const >>> char * const *names, >>> struct msm_iommu *iommu = to_msm_iommu(mmu); >>> int ret; >>> >>> - pm_runtime_get_sync(mmu->dev); >>> + pm_runtime_get_suppliers(mmu->dev); >>> ret = iommu_attach_device(iommu->domain, mmu->dev); >>> - pm_runtime_put_sync(mmu->dev); >>> + pm_runtime_put_suppliers(mmu->dev); >> >> For me, it looks like a wrong place to handle runtime PM of IOMMU >> here. iommu_attach_device() calls into IOMMU driver's attach_device() >> callback and that's where necessary runtime PM gets should happen, if >> any. In other words, driver A (MSM DRM driver) shouldn't be dealing >> with power state of device controlled by driver B (ARM SMMU). > > Note that we end up having to do the same, because of iommu_unmap() > while DRM driver is powered off.. it might be cleaner if it was all > self contained in the iommu driver, but that would make it so other > drivers couldn't call iommu_unmap() from an irq handler, which is > apparently something that some of them want to do.. I'd assume that runtime PM status is already guaranteed to be active when the IRQ handler is running, by some other means (e.g. pm_runtime_get_sync() called earlier, when queuing some work to the hardware). Otherwise, I'm not sure how a powered down device could trigger an IRQ. So, if the master device power is already on, suppliers should be powered on as well, thanks to device links. Best regards, Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figawrote: > Hi Vivek, > > Thanks for the patch. Please see my comments inline. > > On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam > wrote: >> While handling the concerned iommu, there should not be a >> need to power control the drm devices from iommu interface. >> If these drm devices need to be powered around this time, >> the respective drivers should take care of this. >> >> Replace the pm_runtime_get/put_sync() with >> pm_runtime_get/put_suppliers() calls, to power-up >> the connected iommu through the device link interface. >> In case the device link is not setup these get/put_suppliers() >> calls will be a no-op, and the iommu driver should take care of >> powering on its devices accordingly. >> >> Signed-off-by: Vivek Gautam >> --- >> drivers/gpu/drm/msm/msm_iommu.c | 16 >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_iommu.c >> b/drivers/gpu/drm/msm/msm_iommu.c >> index b23d33622f37..1ab629bbee69 100644 >> --- a/drivers/gpu/drm/msm/msm_iommu.c >> +++ b/drivers/gpu/drm/msm/msm_iommu.c >> @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const >> char * const *names, >> struct msm_iommu *iommu = to_msm_iommu(mmu); >> int ret; >> >> - pm_runtime_get_sync(mmu->dev); >> + pm_runtime_get_suppliers(mmu->dev); >> ret = iommu_attach_device(iommu->domain, mmu->dev); >> - pm_runtime_put_sync(mmu->dev); >> + pm_runtime_put_suppliers(mmu->dev); > > For me, it looks like a wrong place to handle runtime PM of IOMMU > here. iommu_attach_device() calls into IOMMU driver's attach_device() > callback and that's where necessary runtime PM gets should happen, if > any. In other words, driver A (MSM DRM driver) shouldn't be dealing > with power state of device controlled by driver B (ARM SMMU). Note that we end up having to do the same, because of iommu_unmap() while DRM driver is powered off.. it might be cleaner if it was all self contained in the iommu driver, but that would make it so other drivers couldn't call iommu_unmap() from an irq handler, which is apparently something that some of them want to do.. So I'm happy with the pm_runtime_get/put_suppliers() approach as a reasonable compromise. (Perhaps specifically, attach/detach this could move inside the iommu driver, but we still need to get/put_suppliers() for unmap(), so meh) BR, -R > This is also important for the reasons I stated in my comments to > "[PATCH v7 1/6] base: power: runtime: Export > pm_runtime_get/put_suppliers". Quoting for everyone's convenience: > >>> There are however cases in which the consumer wants to power-on >>> the supplier, but not itself. >>> E.g., A Graphics or multimedia driver wants to power-on the SMMU >>> to unmap a buffer and finish the TLB operations without powering >>> on itself. >> >>This sounds strange to me. If the SMMU is powered down, wouldn't the >>TLB lose its contents as well (and so no flushing needed)? >> >>Other than that, what kind of hardware operations would be needed >>besides just updating the page tables from the CPU? >> > > In other words, the SMMU driver can deal with hardware state based on > return value of pm_runtime_get_sync() or pm_runtime_get_if_in_use() > and decide whether some operations are necessary or not, e.g. > - a state restore is necessary if the domain was powered off, but we > are bringing the master on, > - a flush may not be required when (un)mapping with the domain powered off, > - etc. > > Best regards, > Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers
Hi Vivek, Thanks for the patch. Please see my comments inline. On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautamwrote: > While handling the concerned iommu, there should not be a > need to power control the drm devices from iommu interface. > If these drm devices need to be powered around this time, > the respective drivers should take care of this. > > Replace the pm_runtime_get/put_sync() with > pm_runtime_get/put_suppliers() calls, to power-up > the connected iommu through the device link interface. > In case the device link is not setup these get/put_suppliers() > calls will be a no-op, and the iommu driver should take care of > powering on its devices accordingly. > > Signed-off-by: Vivek Gautam > --- > drivers/gpu/drm/msm/msm_iommu.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > index b23d33622f37..1ab629bbee69 100644 > --- a/drivers/gpu/drm/msm/msm_iommu.c > +++ b/drivers/gpu/drm/msm/msm_iommu.c > @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char > * const *names, > struct msm_iommu *iommu = to_msm_iommu(mmu); > int ret; > > - pm_runtime_get_sync(mmu->dev); > + pm_runtime_get_suppliers(mmu->dev); > ret = iommu_attach_device(iommu->domain, mmu->dev); > - pm_runtime_put_sync(mmu->dev); > + pm_runtime_put_suppliers(mmu->dev); For me, it looks like a wrong place to handle runtime PM of IOMMU here. iommu_attach_device() calls into IOMMU driver's attach_device() callback and that's where necessary runtime PM gets should happen, if any. In other words, driver A (MSM DRM driver) shouldn't be dealing with power state of device controlled by driver B (ARM SMMU). This is also important for the reasons I stated in my comments to "[PATCH v7 1/6] base: power: runtime: Export pm_runtime_get/put_suppliers". Quoting for everyone's convenience: >> There are however cases in which the consumer wants to power-on >> the supplier, but not itself. >> E.g., A Graphics or multimedia driver wants to power-on the SMMU >> to unmap a buffer and finish the TLB operations without powering >> on itself. > >This sounds strange to me. If the SMMU is powered down, wouldn't the >TLB lose its contents as well (and so no flushing needed)? > >Other than that, what kind of hardware operations would be needed >besides just updating the page tables from the CPU? > In other words, the SMMU driver can deal with hardware state based on return value of pm_runtime_get_sync() or pm_runtime_get_if_in_use() and decide whether some operations are necessary or not, e.g. - a state restore is necessary if the domain was powered off, but we are bringing the master on, - a flush may not be required when (un)mapping with the domain powered off, - etc. Best regards, Tomasz ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno