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. > > > >> > >> 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); > >>> > >>> > >>> > > > > > > -- With best wishes Dmitry