Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 7/3/2023 5:28 PM, Dmitry Baryshkov wrote:
> On Tue, 4 Jul 2023 at 02:16, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > On 7/3/2023 4:01 PM, Dmitry Baryshkov wrote:
> > > On Tue, 4 Jul 2023 at 01:55, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > > > On 7/3/2023 3:53 PM, Dmitry Baryshkov wrote:
> > > > > On Tue, 4 Jul 2023 at 01:37, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > > > > > On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> > > > > > > The stop_req is true only in the dpu_crtc_disable() case, when
> > > > > > > crtc->enable has already been set to false. This renders the stop_req
> > > > > > > argument useless. Remove it completely.
> > > > > >
> > > > > > What about the enable case?
> > > > > >
> > > > > > That time dpu_crtc->enabled  will be false but you need valid clock and
> > > > > > BW that time when you want to enable.
> > > > >
> > > > > Maybe I'm missing something here. The driver sets dpu_crtc->enabled in
> > > > > atomic_enable, while dpu_core_perf_crtc_update() is called further,
> > > > > during atomic_flush, or from the CRTC event handler. And both these
> > > > > cases have stop_req as false.
> > > >
> > > > But the first enable will happen with a commit too right?
> > >
> > > Which one? Could you please point to it?
> >
> > My question was that
> >
> > drm_atomic_helper_commit_planes which calls
> > drm_crtc_helper_funcs::atomic_flush (which calls
> > dpu_core_perf_crtc_update) is called before
> > drm_crtc_helper_funcs::atomic_enable which gets called only during
> > drm_atomic_helper_commit_modeset_enables in msm_atomic_commit_tail
> >
> > Wouldnt that get broken now?
>
> No, crtc->enabled is false, as you pointed out, so (crtc->enabled &&
> !stop_req) will be false too.

ah okay, no further concerns,


Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>

> > > > Thats the one I am referring to.
> > > >
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > > > ---
> > > > > > >      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
> > > > > > >      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
> > > > > > >      drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
> > > > > > >      3 files changed, 10 insertions(+), 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > > index f8d5c87d0915..773e641eab28 100644
> > > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > > > > > > @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> > > > > > >      }
> > > > > > >
> > > > > > >      int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > > > > > > -             int params_changed, bool stop_req)
> > > > > > > +                           int params_changed)
> > > > > > >      {
> > > > > > >          struct dpu_core_perf_params *new, *old;
> > > > > > >          bool update_bus = false, update_clk = false;
> > > > > > > @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > > > > > >          dpu_crtc = to_dpu_crtc(crtc);
> > > > > > >          dpu_cstate = to_dpu_crtc_state(crtc->state);
> > > > > > >
> > > > > > > -     DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
> > > > > > > -                     crtc->base.id, stop_req, kms->perf.core_clk_rate);
> > > > > > > +     DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
> > > > > > > +                     crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
> > > > > > >
> > > > > > >          old = &dpu_crtc->cur_perf;
> > > > > > >          new = &dpu_cstate->new_perf;
> > > > > > >
> > > > > > > -     if (crtc->enabled && !stop_req) {
> > > > > > > +     if (crtc->enabled) {
> > > > > > >                  /*
> > > > > > >                   * cases for bus bandwidth update.
> > > > > > >                   * 1. new bandwidth vote - "ab or ib vote" is higher
> > > > > > > @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > > > > > >          }
> > > > > > >
> > > > > > >          trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
> > > > > > > -             new->core_clk_rate, stop_req, update_bus, update_clk);
> > > > > > > +             new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
> > > > > > >
> > > > > > >          if (update_bus) {
> > > > > > >                  ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
> > > > > > > @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > > > > > >          if (update_clk) {
> > > > > > >                  clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
> > > > > > >
> > > > > > > -             trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
> > > > > > > +             trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
> > > > > > >
> > > > > > >                  clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
> > > > > > >                  ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
> > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > > > > index 2bf7836f79bb..c29ec72984b8 100644
> > > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > > > > > > @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> > > > > > >       * dpu_core_perf_crtc_update - update performance of the given crtc
> > > > > > >       * @crtc: Pointer to crtc
> > > > > > >       * @params_changed: true if crtc parameters are modified
> > > > > > > - * @stop_req: true if this is a stop request
> > > > > > >       * return: zero if success, or error code otherwise
> > > > > > >       */
> > > > > > >      int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > > > > > > -             int params_changed, bool stop_req);
> > > > > > > +                           int params_changed);
> > > > > > >
> > > > > > >      /**
> > > > > > >       * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
> > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > > index ff5d306b95ed..214229d11e3e 100644
> > > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > > > > > @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
> > > > > > >      void dpu_crtc_complete_commit(struct drm_crtc *crtc)
> > > > > > >      {
> > > > > > >          trace_dpu_crtc_complete_commit(DRMID(crtc));
> > > > > > > -     dpu_core_perf_crtc_update(crtc, 0, false);
> > > > > > > +     dpu_core_perf_crtc_update(crtc, 0);
> > > > > > >          _dpu_crtc_complete_flip(crtc);
> > > > > > >      }
> > > > > > >
> > > > > > > @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
> > > > > > >                  return;
> > > > > > >
> > > > > > >          /* update performance setting before crtc kickoff */
> > > > > > > -     dpu_core_perf_crtc_update(crtc, 1, false);
> > > > > > > +     dpu_core_perf_crtc_update(crtc, 1);
> > > > > > >
> > > > > > >          /*
> > > > > > >           * Final plane updates: Give each plane a chance to complete all
> > > > > > > @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> > > > > > >                  atomic_set(&dpu_crtc->frame_pending, 0);
> > > > > > >          }
> > > > > > >
> > > > > > > -     dpu_core_perf_crtc_update(crtc, 0, true);
> > > > > > > +     dpu_core_perf_crtc_update(crtc, 0);
> > > > > > >
> > > > > > >          drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> > > > > > >                  dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);