[RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Abhinav Kumar]

In preparation to register a iommu fault handler for display
related modules, register a fault handler for the backing
mmu object of msm_kms.

Currently, the fault handler only captures the display snapshot
but we can expand this later if more information needs to be
added to debug display mmu faults.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/msm_kms.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index af6a6fcb1173..62c8e6163e81 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
 	return aspace;
 }
 
+static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
+{
+	struct msm_kms *kms = arg;
+	struct msm_disp_state *state;
+	int ret;
+
+	ret = mutex_lock_interruptible(&kms->dump_mutex);
+	if (ret)
+		return ret;
+
+	state = msm_disp_snapshot_state_sync(kms);
+
+	mutex_unlock(&kms->dump_mutex);
+
+	if (IS_ERR(state)) {
+		DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
+		return PTR_ERR(state);
+	}
+
+	return 0;
+}
+
 void msm_drm_kms_uninit(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
 		goto err_msm_uninit;
 	}
 
+	if (kms->aspace)
+		msm_mmu_set_fault_handler(kms->aspace->mmu, kms, msm_kms_fault_handler);
+
 	drm_helper_move_panel_connectors_to_head(ddev);
 
 	drm_for_each_crtc(crtc, ddev) {
-- 
2.44.0

[RFC PATCH 2/4] drm/msm/iommu: rename msm_fault_handler to msm_gpu_fault_handler

[Abhinav Kumar]

In preparation of registering a separate fault handler for
display, lets rename the existing msm_fault_handler to
msm_gpu_fault_handler.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/msm_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index d5512037c38b..a79cd18bc4c9 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -243,7 +243,7 @@ static const struct iommu_flush_ops tlb_ops = {
 	.tlb_add_page = msm_iommu_tlb_add_page,
 };
 
-static int msm_fault_handler(struct iommu_domain *domain, struct device *dev,
+static int msm_gpu_fault_handler(struct iommu_domain *domain, struct device *dev,
 		unsigned long iova, int flags, void *arg);
 
 struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent)
@@ -319,7 +319,7 @@ struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent)
 	return &pagetable->base;
 }
 
-static int msm_fault_handler(struct iommu_domain *domain, struct device *dev,
+static int msm_gpu_fault_handler(struct iommu_domain *domain, struct device *dev,
 		unsigned long iova, int flags, void *arg)
 {
 	struct msm_iommu *iommu = arg;
@@ -445,7 +445,7 @@ struct msm_mmu *msm_iommu_gpu_new(struct device *dev, struct msm_gpu *gpu, unsig
 		return mmu;
 
 	iommu = to_msm_iommu(mmu);
-	iommu_set_fault_handler(iommu->domain, msm_fault_handler, iommu);
+	iommu_set_fault_handler(iommu->domain, msm_gpu_fault_handler, iommu);
 
 	/* Enable stall on iommu fault: */
 	if (adreno_smmu->set_stall)
-- 
2.44.0

[RFC PATCH 3/4] drm/msm/iommu: introduce msm_iommu_disp_new() for msm_kms

[Abhinav Kumar]

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

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

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index a79cd18bc4c9..3d5c1bb4c013 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -343,6 +343,19 @@ 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);
+
+	pr_warn_ratelimited("*** fault: iova=%16lx, flags=%d\n", iova, flags);
+
+	return 0;
+}
+
 static void msm_iommu_resume_translation(struct msm_mmu *mmu)
 {
 	struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(mmu->dev);
@@ -434,6 +447,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;
+}
+
 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

[RFC PATCH 4/4] drm/msm: switch msm_kms to use msm_iommu_disp_new()

[Abhinav Kumar]

Switch msm_kms to use msm_iommu_disp_new() so that the newly
registered fault handler will kick-in during any mmu faults.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/msm_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 62c8e6163e81..1859efbbff1d 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -181,7 +181,7 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
 	else
 		iommu_dev = mdss_dev;
 
-	mmu = msm_iommu_new(iommu_dev, 0);
+	mmu = msm_iommu_disp_new(iommu_dev, 0);
 	if (IS_ERR(mmu))
 		return ERR_CAST(mmu);
 
-- 
2.44.0

Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Dmitry Baryshkov]

On Fri, May 17, 2024 at 04:37:56PM -0700, Abhinav Kumar wrote:
> In preparation to register a iommu fault handler for display
> related modules, register a fault handler for the backing
> mmu object of msm_kms.
> 
> Currently, the fault handler only captures the display snapshot
> but we can expand this later if more information needs to be
> added to debug display mmu faults.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/msm_kms.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index af6a6fcb1173..62c8e6163e81 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>  	return aspace;
>  }
>  
> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
> +{
> +	struct msm_kms *kms = arg;
> +	struct msm_disp_state *state;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&kms->dump_mutex);
> +	if (ret)
> +		return ret;
> +
> +	state = msm_disp_snapshot_state_sync(kms);
> +
> +	mutex_unlock(&kms->dump_mutex);
> +
> +	if (IS_ERR(state)) {
> +		DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
> +		return PTR_ERR(state);
> +	}
> +
> +	return 0;
> +}
> +
>  void msm_drm_kms_uninit(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
>  		goto err_msm_uninit;
>  	}
>  
> +	if (kms->aspace)
> +		msm_mmu_set_fault_handler(kms->aspace->mmu, kms, msm_kms_fault_handler);
> +

Can we move this to msm_kms_init_aspace() instead of checking for
kms->aspace?

>  	drm_helper_move_panel_connectors_to_head(ddev);
>  
>  	drm_for_each_crtc(crtc, ddev) {
> -- 
> 2.44.0
> 

-- 
With best wishes
Dmitry

Re: [RFC PATCH 2/4] drm/msm/iommu: rename msm_fault_handler to msm_gpu_fault_handler

[Dmitry Baryshkov]

On Fri, May 17, 2024 at 04:37:57PM -0700, Abhinav Kumar wrote:
> In preparation of registering a separate fault handler for
> display, lets rename the existing msm_fault_handler to
> msm_gpu_fault_handler.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/msm_iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Fri, May 17, 2024 at 04:37:58PM -0700, Abhinav Kumar wrote:
> Introduce a new API msm_iommu_disp_new() for display use-cases.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/msm_iommu.c | 28 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/msm/msm_mmu.h   |  1 +
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index a79cd18bc4c9..3d5c1bb4c013 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -343,6 +343,19 @@ 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);
> +
> +	pr_warn_ratelimited("*** fault: iova=%16lx, flags=%d\n", iova, flags);
> +
> +	return 0;

I'd say, drop pr_warn and return -ENOSYS, letting the
arm_smmu_context_fault() report the error.

> +}
> +
>  static void msm_iommu_resume_translation(struct msm_mmu *mmu)
>  {
>  	struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(mmu->dev);
> @@ -434,6 +447,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;
> +}
> +
>  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
> 

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Dmitry Baryshkov]

On Fri, May 17, 2024 at 04:37:56PM -0700, Abhinav Kumar wrote:
> In preparation to register a iommu fault handler for display
> related modules, register a fault handler for the backing
> mmu object of msm_kms.
> 
> Currently, the fault handler only captures the display snapshot
> but we can expand this later if more information needs to be
> added to debug display mmu faults.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/msm_kms.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index af6a6fcb1173..62c8e6163e81 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>  	return aspace;
>  }
>  
> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
> +{
> +	struct msm_kms *kms = arg;
> +	struct msm_disp_state *state;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&kms->dump_mutex);
> +	if (ret)
> +		return ret;
> +
> +	state = msm_disp_snapshot_state_sync(kms);
> +
> +	mutex_unlock(&kms->dump_mutex);
> +
> +	if (IS_ERR(state)) {
> +		DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
> +		return PTR_ERR(state);
> +	}
> +
> +	return 0;

Hmm, after reading the rest of the code, this means that we won't get
the error on the console. Could you please change this to -ENOSYS?

> +}
> +
>  void msm_drm_kms_uninit(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
>  		goto err_msm_uninit;
>  	}
>  
> +	if (kms->aspace)
> +		msm_mmu_set_fault_handler(kms->aspace->mmu, kms, msm_kms_fault_handler);
> +
>  	drm_helper_move_panel_connectors_to_head(ddev);
>  
>  	drm_for_each_crtc(crtc, ddev) {
> -- 
> 2.44.0
> 

-- 
With best wishes
Dmitry

Re: [RFC PATCH 4/4] drm/msm: switch msm_kms to use msm_iommu_disp_new()

[Dmitry Baryshkov]

On Fri, May 17, 2024 at 04:37:59PM -0700, Abhinav Kumar wrote:
> Switch msm_kms to use msm_iommu_disp_new() so that the newly
> registered fault handler will kick-in during any mmu faults.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>  drivers/gpu/drm/msm/msm_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index 62c8e6163e81..1859efbbff1d 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -181,7 +181,7 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>  	else
>  		iommu_dev = mdss_dev;
>  
> -	mmu = msm_iommu_new(iommu_dev, 0);
> +	mmu = msm_iommu_disp_new(iommu_dev, 0);
>  	if (IS_ERR(mmu))
>  		return ERR_CAST(mmu);

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Note to myself: make mdp4 use msm_kms_init_aspace().

-- 
With best wishes
Dmitry

Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Stephen Boyd]

Quoting Abhinav Kumar (2024-05-17 16:37:56)
> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index af6a6fcb1173..62c8e6163e81 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>         return aspace;
>  }
>
> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
> +{
> +       struct msm_kms *kms = arg;
> +       struct msm_disp_state *state;
> +       int ret;
> +
> +       ret = mutex_lock_interruptible(&kms->dump_mutex);

From past experience I've seen the smmu fault handler called in hardirq
context, so it can't sleep. Is there some way to grab the register
contents without sleeping? Otherwise this will have to fork off
somewhere else that can take locks, runtime PM resume, etc.

> +       if (ret)
> +               return ret;
> +
> +       state = msm_disp_snapshot_state_sync(kms);
> +
> +       mutex_unlock(&kms->dump_mutex);
> +

Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Abhinav Kumar]

On 5/19/2024 1:31 AM, Dmitry Baryshkov wrote:
> On Fri, May 17, 2024 at 04:37:56PM -0700, Abhinav Kumar wrote:
>> In preparation to register a iommu fault handler for display
>> related modules, register a fault handler for the backing
>> mmu object of msm_kms.
>>
>> Currently, the fault handler only captures the display snapshot
>> but we can expand this later if more information needs to be
>> added to debug display mmu faults.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/msm_kms.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
>> index af6a6fcb1173..62c8e6163e81 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.c
>> +++ b/drivers/gpu/drm/msm/msm_kms.c
>> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>>   	return aspace;
>>   }
>>   
>> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
>> +{
>> +	struct msm_kms *kms = arg;
>> +	struct msm_disp_state *state;
>> +	int ret;
>> +
>> +	ret = mutex_lock_interruptible(&kms->dump_mutex);
>> +	if (ret)
>> +		return ret;
>> +
>> +	state = msm_disp_snapshot_state_sync(kms);
>> +
>> +	mutex_unlock(&kms->dump_mutex);
>> +
>> +	if (IS_ERR(state)) {
>> +		DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
>> +		return PTR_ERR(state);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   void msm_drm_kms_uninit(struct device *dev)
>>   {
>>   	struct platform_device *pdev = to_platform_device(dev);
>> @@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
>>   		goto err_msm_uninit;
>>   	}
>>   
>> +	if (kms->aspace)
>> +		msm_mmu_set_fault_handler(kms->aspace->mmu, kms, msm_kms_fault_handler);
>> +
> 
> Can we move this to msm_kms_init_aspace() instead of checking for
> kms->aspace?

Ack.
> 
>>   	drm_helper_move_panel_connectors_to_head(ddev);
>>   
>>   	drm_for_each_crtc(crtc, ddev) {
>> -- 
>> 2.44.0
>>
> 

Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Abhinav Kumar]

On 5/19/2024 1:38 AM, Dmitry Baryshkov wrote:
> On Fri, May 17, 2024 at 04:37:56PM -0700, Abhinav Kumar wrote:
>> In preparation to register a iommu fault handler for display
>> related modules, register a fault handler for the backing
>> mmu object of msm_kms.
>>
>> Currently, the fault handler only captures the display snapshot
>> but we can expand this later if more information needs to be
>> added to debug display mmu faults.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/msm_kms.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
>> index af6a6fcb1173..62c8e6163e81 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.c
>> +++ b/drivers/gpu/drm/msm/msm_kms.c
>> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>>   	return aspace;
>>   }
>>   
>> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
>> +{
>> +	struct msm_kms *kms = arg;
>> +	struct msm_disp_state *state;
>> +	int ret;
>> +
>> +	ret = mutex_lock_interruptible(&kms->dump_mutex);
>> +	if (ret)
>> +		return ret;
>> +
>> +	state = msm_disp_snapshot_state_sync(kms);
>> +
>> +	mutex_unlock(&kms->dump_mutex);
>> +
>> +	if (IS_ERR(state)) {
>> +		DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
>> +		return PTR_ERR(state);
>> +	}
>> +
>> +	return 0;
> 
> Hmm, after reading the rest of the code, this means that we won't get
> the error on the console. Could you please change this to -ENOSYS?
> 

I am perhaps missing something here. Are you referring to this 
documentation?

  * Specifically, -ENOSYS is returned if a fault handler isn't installed
  * (though fault handlers can also return -ENOSYS, in case they want to
  * elicit the default behavior of the IOMMU drivers).
  */
int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
                        unsigned long iova, int flags)

Doesnt this mean ENOSYS is when fault handler is not installed?

Because the console print will come if we return 0?

         if (!report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) {
                 dev_err_ratelimited(ctx->dev,
                                     "Unhandled context fault: fsr=0x%x, "
                                     "iova=0x%016llx, fsynr=0x%x, cb=%d\n",
                                     fsr, iova, fsynr, ctx->asid);
         }

>> +}
>> +
>>   void msm_drm_kms_uninit(struct device *dev)
>>   {
>>   	struct platform_device *pdev = to_platform_device(dev);
>> @@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
>>   		goto err_msm_uninit;
>>   	}
>>   
>> +	if (kms->aspace)
>> +		msm_mmu_set_fault_handler(kms->aspace->mmu, kms, msm_kms_fault_handler);
>> +
>>   	drm_helper_move_panel_connectors_to_head(ddev);
>>   
>>   	drm_for_each_crtc(crtc, ddev) {
>> -- 
>> 2.44.0
>>
> 

Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Abhinav Kumar]

On 5/20/2024 9:12 PM, Stephen Boyd wrote:
> Quoting Abhinav Kumar (2024-05-17 16:37:56)
>> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
>> index af6a6fcb1173..62c8e6163e81 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.c
>> +++ b/drivers/gpu/drm/msm/msm_kms.c
>> @@ -200,6 +200,28 @@ struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
>>          return aspace;
>>   }
>>
>> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int flags, void *data)
>> +{
>> +       struct msm_kms *kms = arg;
>> +       struct msm_disp_state *state;
>> +       int ret;
>> +
>> +       ret = mutex_lock_interruptible(&kms->dump_mutex);
> 
>  From past experience I've seen the smmu fault handler called in hardirq
> context, so it can't sleep. Is there some way to grab the register
> contents without sleeping? Otherwise this will have to fork off
> somewhere else that can take locks, runtime PM resume, etc.
>

Sorry for the delay in this series.

Yes, this is a valid point.

Let me see move to msm_disp_snapshot_state() as that one will spawn a 
kthread then dump the registers.

>> +       if (ret)
>> +               return ret;
>> +
>> +       state = msm_disp_snapshot_state_sync(kms);
>> +
>> +       mutex_unlock(&kms->dump_mutex);
>> +

Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults

[Abhinav Kumar]

On 6/21/2024 4:35 PM, Abhinav Kumar wrote:
> 
> 
> On 5/19/2024 1:38 AM, Dmitry Baryshkov wrote:
>> On Fri, May 17, 2024 at 04:37:56PM -0700, Abhinav Kumar wrote:
>>> In preparation to register a iommu fault handler for display
>>> related modules, register a fault handler for the backing
>>> mmu object of msm_kms.
>>>
>>> Currently, the fault handler only captures the display snapshot
>>> but we can expand this later if more information needs to be
>>> added to debug display mmu faults.
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/msm_kms.c | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_kms.c 
>>> b/drivers/gpu/drm/msm/msm_kms.c
>>> index af6a6fcb1173..62c8e6163e81 100644
>>> --- a/drivers/gpu/drm/msm/msm_kms.c
>>> +++ b/drivers/gpu/drm/msm/msm_kms.c
>>> @@ -200,6 +200,28 @@ struct msm_gem_address_space 
>>> *msm_kms_init_aspace(struct drm_device *dev)
>>>       return aspace;
>>>   }
>>> +static int msm_kms_fault_handler(void *arg, unsigned long iova, int 
>>> flags, void *data)
>>> +{
>>> +    struct msm_kms *kms = arg;
>>> +    struct msm_disp_state *state;
>>> +    int ret;
>>> +
>>> +    ret = mutex_lock_interruptible(&kms->dump_mutex);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    state = msm_disp_snapshot_state_sync(kms);
>>> +
>>> +    mutex_unlock(&kms->dump_mutex);
>>> +
>>> +    if (IS_ERR(state)) {
>>> +        DRM_DEV_ERROR(kms->dev->dev, "failed to capture snapshot\n");
>>> +        return PTR_ERR(state);
>>> +    }
>>> +
>>> +    return 0;
>>
>> Hmm, after reading the rest of the code, this means that we won't get
>> the error on the console. Could you please change this to -ENOSYS?
>>
> 
> I am perhaps missing something here. Are you referring to this 
> documentation?
> 
>   * Specifically, -ENOSYS is returned if a fault handler isn't installed
>   * (though fault handlers can also return -ENOSYS, in case they want to
>   * elicit the default behavior of the IOMMU drivers).
>   */
> int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
>                         unsigned long iova, int flags)
> 
> Doesnt this mean ENOSYS is when fault handler is not installed?
> 
> Because the console print will come if we return 0?
> 
>          if (!report_iommu_fault(ctx->domain, ctx->dev, iova, 0)) {
>                  dev_err_ratelimited(ctx->dev,
>                                      "Unhandled context fault: fsr=0x%x, "
>                                      "iova=0x%016llx, fsynr=0x%x, cb=%d\n",
>                                      fsr, iova, fsynr, ctx->asid);
>          }
> 

I was checking the wrong place, you are correct.

I will return ENOSYS here.

Thanks.

>>> +}
>>> +
>>>   void msm_drm_kms_uninit(struct device *dev)
>>>   {
>>>       struct platform_device *pdev = to_platform_device(dev);
>>> @@ -261,6 +283,9 @@ int msm_drm_kms_init(struct device *dev, const 
>>> struct drm_driver *drv)
>>>           goto err_msm_uninit;
>>>       }
>>> +    if (kms->aspace)
>>> +        msm_mmu_set_fault_handler(kms->aspace->mmu, kms, 
>>> msm_kms_fault_handler);
>>> +
>>>       drm_helper_move_panel_connectors_to_head(ddev);
>>>       drm_for_each_crtc(crtc, ddev) {
>>> -- 
>>> 2.44.0
>>>
>>