| Message ID | 20180611182604.30467-2-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 11, 2018 at 11:26 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > Now that the IOMMU is the master of it's own power we don't need to bring > up the GPU to do IOMMU operations. This is good because bringing up a6xx > requires the GMU so calling pm_runtime_get_sync() too early in the process > gets us into some nasty circular dependency situations. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> Reviewed-by: Kristian H. Kristensen <hoegsberg@google.com> > --- > drivers/gpu/drm/msm/msm_iommu.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > index b23d33622f37..ccd93ac6a4d8 100644 > --- a/drivers/gpu/drm/msm/msm_iommu.c > +++ b/drivers/gpu/drm/msm/msm_iommu.c > @@ -40,9 +40,7 @@ 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); > ret = iommu_attach_device(iommu->domain, mmu->dev); > - pm_runtime_put_sync(mmu->dev); > > return ret; > } > @@ -52,9 +50,7 @@ static void msm_iommu_detach(struct msm_mmu *mmu, const char * const *names, > { > struct msm_iommu *iommu = to_msm_iommu(mmu); > > - pm_runtime_get_sync(mmu->dev); > iommu_detach_device(iommu->domain, mmu->dev); > - pm_runtime_put_sync(mmu->dev); > } > > static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, > @@ -63,9 +59,7 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, > struct msm_iommu *iommu = to_msm_iommu(mmu); > size_t ret; > > -// pm_runtime_get_sync(mmu->dev); > ret = iommu_map_sg(iommu->domain, iova, sgt->sgl, sgt->nents, prot); > -// pm_runtime_put_sync(mmu->dev); > WARN_ON(ret < 0); > > return (ret == len) ? 0 : -EINVAL; > @@ -76,9 +70,7 @@ static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, > { > struct msm_iommu *iommu = to_msm_iommu(mmu); > > - pm_runtime_get_sync(mmu->dev); > iommu_unmap(iommu->domain, iova, len); > - pm_runtime_put_sync(mmu->dev); > > return 0; > } > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Jordan, On Mon, Jun 11, 2018 at 11:56 PM, Jordan Crouse <jcrouse@codeaurora.org> wrote: > Now that the IOMMU is the master of it's own power we don't need to bring > up the GPU to do IOMMU operations. This is good because bringing up a6xx > requires the GMU so calling pm_runtime_get_sync() too early in the process > gets us into some nasty circular dependency situations. Thanks for the patch. While you are removing these calls, we should add pm_runtime calls to qcom_iommu_map(). That should make qcom_iommu too master of its power control. Then we should be good to go with this patch. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > --- > drivers/gpu/drm/msm/msm_iommu.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > index b23d33622f37..ccd93ac6a4d8 100644 > --- a/drivers/gpu/drm/msm/msm_iommu.c > +++ b/drivers/gpu/drm/msm/msm_iommu.c > @@ -40,9 +40,7 @@ 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); > ret = iommu_attach_device(iommu->domain, mmu->dev); > - pm_runtime_put_sync(mmu->dev); may be just do the following here. return iommu_attach_device(iommu->domain, mmu->dev); Rest looks good. So after the change. Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org> Best regards Vivek > > return ret; > } > @@ -52,9 +50,7 @@ static void msm_iommu_detach(struct msm_mmu *mmu, const char * const *names, > { > struct msm_iommu *iommu = to_msm_iommu(mmu); > > - pm_runtime_get_sync(mmu->dev); > iommu_detach_device(iommu->domain, mmu->dev); > - pm_runtime_put_sync(mmu->dev); > } > > static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, > @@ -63,9 +59,7 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, > struct msm_iommu *iommu = to_msm_iommu(mmu); > size_t ret; > > -// pm_runtime_get_sync(mmu->dev); > ret = iommu_map_sg(iommu->domain, iova, sgt->sgl, sgt->nents, prot); > -// pm_runtime_put_sync(mmu->dev); > WARN_ON(ret < 0); > > return (ret == len) ? 0 : -EINVAL; > @@ -76,9 +70,7 @@ static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, > { > struct msm_iommu *iommu = to_msm_iommu(mmu); > > - pm_runtime_get_sync(mmu->dev); > iommu_unmap(iommu->domain, iova, len); > - pm_runtime_put_sync(mmu->dev); > > return 0; > } > -- > 2.17.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 14, 2018 at 12:30:35PM +0530, Vivek Gautam wrote: > Hi Jordan, > > On Mon, Jun 11, 2018 at 11:56 PM, Jordan Crouse <jcrouse@codeaurora.org> wrote: > > Now that the IOMMU is the master of it's own power we don't need to bring > > up the GPU to do IOMMU operations. This is good because bringing up a6xx > > requires the GMU so calling pm_runtime_get_sync() too early in the process > > gets us into some nasty circular dependency situations. > > Thanks for the patch. > While you are removing these calls, we should add pm_runtime calls > to qcom_iommu_map(). That should make qcom_iommu too master of > its power control. > Then we should be good to go with this patch. Right - I told Rob about that in IRC but I should have mentioned it here as well. In order to do this we need to be sure that all of of the possible MMU combinations are covered. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > --- > > drivers/gpu/drm/msm/msm_iommu.c | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c > > index b23d33622f37..ccd93ac6a4d8 100644 > > --- a/drivers/gpu/drm/msm/msm_iommu.c > > +++ b/drivers/gpu/drm/msm/msm_iommu.c > > @@ -40,9 +40,7 @@ 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); > > ret = iommu_attach_device(iommu->domain, mmu->dev); > > - pm_runtime_put_sync(mmu->dev); > > may be just do the following here. > return iommu_attach_device(iommu->domain, mmu->dev); I'll do that. Thanks. Jordan
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index b23d33622f37..ccd93ac6a4d8 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -40,9 +40,7 @@ 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); ret = iommu_attach_device(iommu->domain, mmu->dev); - pm_runtime_put_sync(mmu->dev); return ret; } @@ -52,9 +50,7 @@ static void msm_iommu_detach(struct msm_mmu *mmu, const char * const *names, { struct msm_iommu *iommu = to_msm_iommu(mmu); - pm_runtime_get_sync(mmu->dev); iommu_detach_device(iommu->domain, mmu->dev); - pm_runtime_put_sync(mmu->dev); } static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, @@ -63,9 +59,7 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, struct msm_iommu *iommu = to_msm_iommu(mmu); size_t ret; -// pm_runtime_get_sync(mmu->dev); ret = iommu_map_sg(iommu->domain, iova, sgt->sgl, sgt->nents, prot); -// pm_runtime_put_sync(mmu->dev); WARN_ON(ret < 0); return (ret == len) ? 0 : -EINVAL; @@ -76,9 +70,7 @@ static int msm_iommu_unmap(struct msm_mmu *mmu, uint64_t iova, { struct msm_iommu *iommu = to_msm_iommu(mmu); - pm_runtime_get_sync(mmu->dev); iommu_unmap(iommu->domain, iova, len); - pm_runtime_put_sync(mmu->dev); return 0; }
Now that the IOMMU is the master of it's own power we don't need to bring up the GPU to do IOMMU operations. This is good because bringing up a6xx requires the GMU so calling pm_runtime_get_sync() too early in the process gets us into some nasty circular dependency situations. Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> --- drivers/gpu/drm/msm/msm_iommu.c | 8 -------- 1 file changed, 8 deletions(-)