| Message ID | 20231211182000.218088-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: Ratelimit framedone timeout msgs | expand |
On 12/11/2023 10:19 AM, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > When we start getting these, we get a *lot*. So ratelimit it to not > flood dmesg. > > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > > 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 <quic_abhinavk@quicinc.com>
On 2023-12-11 10:19:55, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > When we start getting these, we get a *lot*. So ratelimit it to not > flood dmesg. > > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > > 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 <marijn.suijten@somainline.org> > 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 <marijn.suijten@somainline.org> wrote: > > On 2023-12-11 10:19:55, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > When we start getting these, we get a *lot*. So ratelimit it to not > > flood dmesg. > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > --- > > > > 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 <marijn.suijten@somainline.org> > > > 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,
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