From: Rob Clark <[email protected]>
When we start getting these, we get a *lot*. So ratelimit it to not
flood dmesg.
Signed-off-by: Rob Clark <[email protected]>
---
dpu should probably stop rolling it's own trace macros, but that would
be a larger cleanup.
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 ++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 82538844614b..7c22235d0eba 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -39,6 +39,9 @@
#define DPU_ERROR_ENC(e, fmt, ...) DPU_ERROR("enc%d " fmt,\
(e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
+#define DPU_ERROR_ENC_RATELIMITED(e, fmt, ...) DPU_ERROR_RATELIMITED("enc%d " fmt,\
+ (e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
+
/*
* Two to anticipate panels that can do cmd/vid dynamic switching
* plan is to create all possible physical encoder types, and switch between
@@ -2339,7 +2342,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
return;
}
- DPU_ERROR_ENC(dpu_enc, "frame done timeout\n");
+ DPU_ERROR_ENC_RATELIMITED(dpu_enc, "frame done timeout\n");
event = DPU_ENCODER_FRAME_EVENT_ERROR;
trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index b6f53ca6e962..f5473d4dea92 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -51,6 +51,7 @@
} while (0)
#define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__)
+#define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" fmt, ##__VA_ARGS__)
/**
* ktime_compare_safe - compare two ktime structures
--
2.43.0
On 12/11/2023 10:19 AM, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> When we start getting these, we get a *lot*. So ratelimit it to not
> flood dmesg.
>
> Signed-off-by: Rob Clark <[email protected]>
> ---
>
> dpu should probably stop rolling it's own trace macros, but that would
> be a larger cleanup.
>
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 ++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
Reviewed-by: Abhinav Kumar <[email protected]>
On 2023-12-11 10:19:55, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> When we start getting these, we get a *lot*. So ratelimit it to not
> flood dmesg.
>
> Signed-off-by: Rob Clark <[email protected]>
> ---
>
> dpu should probably stop rolling it's own trace macros, but that would
> be a larger cleanup.
That would be lovely, use is currently all over the place.
Should this patch also ratelimit the corresponding:
[drm:dpu_encoder_phys_cmd_prepare_for_kickoff] *ERROR* failed wait_for_idle: id:31 ret:-110 pp:0
On CMD-mode panels?
Note that this is a prime example of using DRM_ERROR over DPU_ERROR*, resulting
in unnecessary divergence (and un-readability) between error messages and the
code (DPU_DEBUG_CMDENC, which has a corresponding DPU_ERROR variant, is also
used within that function...)
Reviewed-by: Marijn Suijten <[email protected]>
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 ++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 +
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 82538844614b..7c22235d0eba 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -39,6 +39,9 @@
> #define DPU_ERROR_ENC(e, fmt, ...) DPU_ERROR("enc%d " fmt,\
> (e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
>
> +#define DPU_ERROR_ENC_RATELIMITED(e, fmt, ...) DPU_ERROR_RATELIMITED("enc%d " fmt,\
> + (e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
> +
> /*
> * Two to anticipate panels that can do cmd/vid dynamic switching
> * plan is to create all possible physical encoder types, and switch between
> @@ -2339,7 +2342,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
> return;
> }
>
> - DPU_ERROR_ENC(dpu_enc, "frame done timeout\n");
> + DPU_ERROR_ENC_RATELIMITED(dpu_enc, "frame done timeout\n");
>
> event = DPU_ENCODER_FRAME_EVENT_ERROR;
> trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index b6f53ca6e962..f5473d4dea92 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -51,6 +51,7 @@
> } while (0)
>
> #define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__)
> +#define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" fmt, ##__VA_ARGS__)
>
> /**
> * ktime_compare_safe - compare two ktime structures
> --
> 2.43.0
>
On Mon, Dec 11, 2023 at 2:09 PM Marijn Suijten
<[email protected]> wrote:
>
> On 2023-12-11 10:19:55, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > When we start getting these, we get a *lot*. So ratelimit it to not
> > flood dmesg.
> >
> > Signed-off-by: Rob Clark <[email protected]>
> > ---
> >
> > dpu should probably stop rolling it's own trace macros, but that would
> > be a larger cleanup.
>
> That would be lovely, use is currently all over the place.
>
> Should this patch also ratelimit the corresponding:
>
> [drm:dpu_encoder_phys_cmd_prepare_for_kickoff] *ERROR* failed wait_for_idle: id:31 ret:-110 pp:0
>
> On CMD-mode panels?
Probably it should for consistency. But I think you normally wouldn't
get this error at 60Hz with a cmd mode panel, so probably ok to make
it ratelimited for cmd mode later.
BR,
-R
> Note that this is a prime example of using DRM_ERROR over DPU_ERROR*, resulting
> in unnecessary divergence (and un-readability) between error messages and the
> code (DPU_DEBUG_CMDENC, which has a corresponding DPU_ERROR variant, is also
> used within that function...)
>
> Reviewed-by: Marijn Suijten <[email protected]>
>
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 ++++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 +
> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 82538844614b..7c22235d0eba 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -39,6 +39,9 @@
> > #define DPU_ERROR_ENC(e, fmt, ...) DPU_ERROR("enc%d " fmt,\
> > (e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
> >
> > +#define DPU_ERROR_ENC_RATELIMITED(e, fmt, ...) DPU_ERROR_RATELIMITED("enc%d " fmt,\
> > + (e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
> > +
> > /*
> > * Two to anticipate panels that can do cmd/vid dynamic switching
> > * plan is to create all possible physical encoder types, and switch between
> > @@ -2339,7 +2342,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
> > return;
> > }
> >
> > - DPU_ERROR_ENC(dpu_enc, "frame done timeout\n");
> > + DPU_ERROR_ENC_RATELIMITED(dpu_enc, "frame done timeout\n");
> >
> > event = DPU_ENCODER_FRAME_EVENT_ERROR;
> > trace_dpu_enc_frame_done_timeout(DRMID(drm_enc), event);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > index b6f53ca6e962..f5473d4dea92 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> > @@ -51,6 +51,7 @@
> > } while (0)
> >
> > #define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__)
> > +#define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" fmt, ##__VA_ARGS__)
> >
> > /**
> > * ktime_compare_safe - compare two ktime structures
> > --
> > 2.43.0
> >
On Mon, 11 Dec 2023 10:19:55 -0800, Rob Clark wrote:
> When we start getting these, we get a *lot*. So ratelimit it to not
> flood dmesg.
>
>
Applied, thanks!
[1/1] drm/msm/dpu: Ratelimit framedone timeout msgs
https://gitlab.freedesktop.org/lumag/msm/-/commit/e37cb117b819
Best regards,
--
Dmitry Baryshkov <[email protected]>