Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
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
---
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(>dump_mutex);
+ if (ret)
+ return ret;
+
+ state = msm_disp_snapshot_state_sync(kms);
+
+ mutex_unlock(>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
Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
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(>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(>dump_mutex);
+
Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
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
---
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(>dump_mutex);
+ if (ret)
+ return ret;
+
+ state = msm_disp_snapshot_state_sync(kms);
+
+ mutex_unlock(>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
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
---
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(>dump_mutex);
+ if (ret)
+ return ret;
+
+ state = msm_disp_snapshot_state_sync(kms);
+
+ mutex_unlock(>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
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(>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(>dump_mutex);
> +
Re: [RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
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
> ---
> 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(>dump_mutex);
> + if (ret)
> + return ret;
> +
> + state = msm_disp_snapshot_state_sync(kms);
> +
> + mutex_unlock(>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 1/4] drm/msm: register a fault handler for display mmu faults
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
> ---
> 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(>dump_mutex);
> + if (ret)
> + return ret;
> +
> + state = msm_disp_snapshot_state_sync(kms);
> +
> + mutex_unlock(>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
[RFC PATCH 1/4] drm/msm: register a fault handler for display mmu faults
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
---
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(>dump_mutex);
+ if (ret)
+ return ret;
+
+ state = msm_disp_snapshot_state_sync(kms);
+
+ mutex_unlock(>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
