Re: [PATCH 3/5] drm/msm/iommu: introduce msm_iommu_disp_new() for msm_kms

Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> · Tue, 16 Jul 2024 14:09:53 -0700

On 7/1/2024 1:41 PM, Rob Clark wrote:
On Fri, Jun 28, 2024 at 2:49 PM Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:

Introduce a new API msm_iommu_disp_new() for display use-cases.

Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
---
  drivers/gpu/drm/msm/msm_iommu.c | 26 ++++++++++++++++++++++++++
  drivers/gpu/drm/msm/msm_mmu.h   |  1 +
  2 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index a79cd18bc4c9..0420bdc4a224 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -343,6 +343,17 @@ static int msm_gpu_fault_handler(struct iommu_domain *domain, struct device *dev
         return 0;
  }

+static int msm_disp_fault_handler(struct iommu_domain *domain, struct device *dev,
+                                 unsigned long iova, int flags, void *arg)
+{
+       struct msm_iommu *iommu = arg;
+
+       if (iommu->base.handler)
+               return iommu->base.handler(iommu->base.arg, iova, flags, NULL);
+
+       return -ENOSYS;
+}
+
  static void msm_iommu_resume_translation(struct msm_mmu *mmu)
  {
         struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(mmu->dev);
@@ -434,6 +445,21 @@ struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks)
         return &iommu->base;
  }

+struct msm_mmu *msm_iommu_disp_new(struct device *dev, unsigned long quirks)
+{
+       struct msm_iommu *iommu;
+       struct msm_mmu *mmu;
+
+       mmu = msm_iommu_new(dev, quirks);
+       if (IS_ERR_OR_NULL(mmu))
+               return mmu;
+
+       iommu = to_msm_iommu(mmu);
+       iommu_set_fault_handler(iommu->domain, msm_disp_fault_handler, iommu);
+
+       return mmu;
+}

Hmm, are we using dev drvdata for the display pdev?  If
dev_get_drvdata() returns NULL for display pdev, we could get away
without having a different fault handler.

BR,
-R

It is being set to struct msm_drm_private* currently. So it shouldnt 
return NULL.

I also thought of re-using the same API as GPU but the drvdata along 
with its own fault handler and having below code in the gpu handler all 
made me conclude that its cleaner to let display have its own handler.

        if (adreno_smmu->set_stall)
                adreno_smmu->set_stall(adreno_smmu->cookie, true);


+
  struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsigned long quirks)
  {
         struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(dev);
diff --git a/drivers/gpu/drm/msm/msm_mmu.h b/drivers/gpu/drm/msm/msm_mmu.h
index 88af4f490881..730458d08d6b 100644
--- a/drivers/gpu/drm/msm/msm_mmu.h
+++ b/drivers/gpu/drm/msm/msm_mmu.h
@@ -42,6 +42,7 @@ static inline void msm_mmu_init(struct msm_mmu *mmu, struct device *dev,

  struct msm_mmu *msm_iommu_new(struct device *dev, unsigned long quirks);
  struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsigned long quirks);
+struct msm_mmu *msm_iommu_disp_new(struct device *dev, unsigned long quirks);

  static inline void msm_mmu_set_fault_handler(struct msm_mmu *mmu, void *arg,
                 int (*handler)(void *arg, unsigned long iova, int flags, void *data))
--
2.44.0







