Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231211182000.218088-1-robdclark@gmail.com> <lcsqjcqwvmhys3tzrznyqhlmk3p4wbv2rcu77tecbsioscxbal@3s4qeztkktx5>
In-Reply-To: <lcsqjcqwvmhys3tzrznyqhlmk3p4wbv2rcu77tecbsioscxbal@3s4qeztkktx5>
From:   Rob Clark <robdclark@gmail.com>
Date:   Mon, 11 Dec 2023 15:04:46 -0800
Message-ID: <CAF6AEGtrA=ay_xT17HrLaecKUnMkUKrDn7DHS=_tibsDR8WRbA@mail.gmail.com>
Subject: Re: [PATCH] drm/msm/dpu: Ratelimit framedone timeout msgs
To:     Marijn Suijten <marijn.suijten@somainline.org>
Cc:     dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
        linux-arm-msm@vger.kernel.org, Rob Clark <robdclark@chromium.org>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Kuogee Hsieh <quic_khsieh@quicinc.com>,
        Jessica Zhang <quic_jesszhan@quicinc.com>,
        Vinod Polimera <quic_vpolimer@quicinc.com>,
        Kalyan Thota <quic_kalyant@quicinc.com>,
        Dan Carpenter <dan.carpenter@linaro.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Dec 11, 2023 at 2:09=E2=80=AFPM 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 wai=
t_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*, res=
ulting
> in unnecessary divergence (and un-readability) between error messages and=
 the
> code (DPU_DEBUG_CMDENC, which has a corresponding DPU_ERROR variant, is a=
lso
> 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 b=
etween
> > @@ -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 =3D 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
> >