| Message ID | 20240625-dpu-mode-config-width-v5-3-501d984d634f@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3ae133b0192b9b0c9f560bbc096887053150195f |
| Headers | show |
| Series | None | expand |
On Wed, 17 Jul 2024 at 01:40, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote: > > Historically CRTC resources (LMs and CTLs) were assigned in > > dpu_crtc_atomic_begin(). The commit 9222cdd27e82 ("drm/msm/dpu: move hw > > resource tracking to crtc state") simply moved resources to > > struct dpu_crtc_state, without changing the code sequence. Later on the > > commit b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder > > modeset") rearanged the code, but still kept the cstate->num_mixers > > assignment to happen during commit phase. This makes dpu_crtc_state > > inconsistent between consequent atomic_check() calls. > > > > Move CRTC resource assignment to happen at the end of > > dpu_encoder_virt_atomic_check(). > > > > Fixes: b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder modeset") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 -- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 60 +++++++++++++++++++---------- > > 2 files changed, 39 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 9f2164782844..7399794d75eb 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -1094,9 +1094,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) > > dpu_encoder_register_frame_event_callback(encoder, NULL, NULL); > > > > - memset(cstate->mixers, 0, sizeof(cstate->mixers)); > > - cstate->num_mixers = 0; > > - > > Any reason this part was removed? > > I think we still need this part. > > In dpu_encoder_get_topology(), we do not assign topology.num_lm based on > state parameters, its based on catalog and intf_count which are decided > at init time itself. > > Which means cstate->num_mixers will remain at a non-zero value even if > we have NO_MODE which is what happens when we are disabling the CRTC > during suspend or hotplug. In the disable path the driver calls dpu_rm_release(), releasing all resources. So there will be no assigned mixers, which matches num_lm = 0 in dpu_encoder_assign_crtc_resources(). > > > /* disable clk & bw control until clk & bw properties are set */ > > cstate->bw_control = false; > > cstate->bw_split_vote = false; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 7613005fbfea..98f3a8d84300 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -628,6 +628,41 @@ static struct msm_display_topology dpu_encoder_get_topology( > > return topology; > > } > > > > +static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms, > > + struct drm_encoder *drm_enc, > > + struct dpu_global_state *global_state, > > + struct drm_crtc_state *crtc_state) > > +{ > > + struct dpu_crtc_state *cstate; > > + struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > > + struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > > + struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC]; > > + int num_lm, num_ctl, num_dspp, i; > > + > > + cstate = to_dpu_crtc_state(crtc_state); > > + > > + memset(cstate->mixers, 0, sizeof(cstate->mixers)); > > + > > + num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > + drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); > > + num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > + drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); > > + num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > + drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > > + ARRAY_SIZE(hw_dspp)); > > + > > + for (i = 0; i < num_lm; i++) { > > + int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); > > + > > + cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); > > + cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); > > + if (i < num_dspp) > > Will there be a case where num_lm != num_dspp? No, _dpu_rm_check_lm_and_get_connected_blks() makes sure that num_lm = num_dspp. I will drop this condition and add a comment. > > > + cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); > > + } > > + > > + cstate->num_mixers = num_lm; > > +} > > + > > static int dpu_encoder_virt_atomic_check( > > struct drm_encoder *drm_enc, > > struct drm_crtc_state *crtc_state, > > @@ -698,6 +733,9 @@ static int dpu_encoder_virt_atomic_check( > > drm_enc, crtc_state, topology); > > } > > > > + if (!ret) > > + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, global_state, crtc_state); > > + > > trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags); > > > > return ret; > > @@ -1097,14 +1135,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > struct dpu_encoder_virt *dpu_enc; > > struct msm_drm_private *priv; > > struct dpu_kms *dpu_kms; > > - struct dpu_crtc_state *cstate; > > struct dpu_global_state *global_state; > > struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC]; > > struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > > - struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > > - struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; > > struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; > > - int num_lm, num_ctl, num_pp, num_dsc; > > + int num_ctl, num_pp, num_dsc; > > unsigned int dsc_mask = 0; > > int i; > > > > @@ -1133,11 +1168,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > ARRAY_SIZE(hw_pp)); > > num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); > > - num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > - drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); > > - dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > - drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > > - ARRAY_SIZE(hw_dspp)); > > > > for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > > dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) > > @@ -1163,18 +1193,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL; > > } > > > > - cstate = to_dpu_crtc_state(crtc_state); > > - > > - for (i = 0; i < num_lm; i++) { > > - int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); > > - > > - cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); > > - cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); > > - cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); > > - } > > - > > - cstate->num_mixers = num_lm; > > - > > dpu_enc->connector = conn_state->connector; > > > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > >
On 7/16/2024 4:06 PM, Dmitry Baryshkov wrote: > On Wed, 17 Jul 2024 at 01:40, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote: >>> Historically CRTC resources (LMs and CTLs) were assigned in >>> dpu_crtc_atomic_begin(). The commit 9222cdd27e82 ("drm/msm/dpu: move hw >>> resource tracking to crtc state") simply moved resources to >>> struct dpu_crtc_state, without changing the code sequence. Later on the >>> commit b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder >>> modeset") rearanged the code, but still kept the cstate->num_mixers >>> assignment to happen during commit phase. This makes dpu_crtc_state >>> inconsistent between consequent atomic_check() calls. >>> >>> Move CRTC resource assignment to happen at the end of >>> dpu_encoder_virt_atomic_check(). >>> I just realized, why is the subject saying dpu_encoder_virt_atomic_mode_set while the patch is moving the assignment to dpu_encoder_virt_atomic_check(). It should say dpu_encoder_virt_atomic_check(). One more commment below. >>> Fixes: b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder modeset") >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 -- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 60 +++++++++++++++++++---------- >>> 2 files changed, 39 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> index 9f2164782844..7399794d75eb 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >>> @@ -1094,9 +1094,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, >>> drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) >>> dpu_encoder_register_frame_event_callback(encoder, NULL, NULL); >>> >>> - memset(cstate->mixers, 0, sizeof(cstate->mixers)); >>> - cstate->num_mixers = 0; >>> - >> >> Any reason this part was removed? >> >> I think we still need this part. >> >> In dpu_encoder_get_topology(), we do not assign topology.num_lm based on >> state parameters, its based on catalog and intf_count which are decided >> at init time itself. >> >> Which means cstate->num_mixers will remain at a non-zero value even if >> we have NO_MODE which is what happens when we are disabling the CRTC >> during suspend or hotplug. > > In the disable path the driver calls dpu_rm_release(), releasing all > resources. So there will be no assigned mixers, which matches num_lm = > 0 in dpu_encoder_assign_crtc_resources(). > Ack. >> >>> /* disable clk & bw control until clk & bw properties are set */ >>> cstate->bw_control = false; >>> cstate->bw_split_vote = false; >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index 7613005fbfea..98f3a8d84300 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -628,6 +628,41 @@ static struct msm_display_topology dpu_encoder_get_topology( >>> return topology; >>> } >>> >>> +static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms, >>> + struct drm_encoder *drm_enc, >>> + struct dpu_global_state *global_state, >>> + struct drm_crtc_state *crtc_state) >>> +{ >>> + struct dpu_crtc_state *cstate; >>> + struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; >>> + struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; >>> + struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC]; >>> + int num_lm, num_ctl, num_dspp, i; >>> + >>> + cstate = to_dpu_crtc_state(crtc_state); >>> + >>> + memset(cstate->mixers, 0, sizeof(cstate->mixers)); >>> + >>> + num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, >>> + drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); >>> + num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, >>> + drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); >>> + num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, >>> + drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, >>> + ARRAY_SIZE(hw_dspp)); >>> + >>> + for (i = 0; i < num_lm; i++) { >>> + int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); >>> + >>> + cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); >>> + cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); >>> + if (i < num_dspp) >> >> Will there be a case where num_lm != num_dspp? > > No, _dpu_rm_check_lm_and_get_connected_blks() makes sure that num_lm = num_dspp. > I will drop this condition and add a comment. > >> >>> + cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); >>> + } >>> + >>> + cstate->num_mixers = num_lm; >>> +} >>> + >>> static int dpu_encoder_virt_atomic_check( >>> struct drm_encoder *drm_enc, >>> struct drm_crtc_state *crtc_state, >>> @@ -698,6 +733,9 @@ static int dpu_encoder_virt_atomic_check( >>> drm_enc, crtc_state, topology); >>> } >>> >>> + if (!ret) >>> + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, global_state, crtc_state); >>> + Do we need to protect this with a needs_modeset() condition? Like why should this get called every check? >>> trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags); >>> >>> return ret; >>> @@ -1097,14 +1135,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >>> struct dpu_encoder_virt *dpu_enc; >>> struct msm_drm_private *priv; >>> struct dpu_kms *dpu_kms; >>> - struct dpu_crtc_state *cstate; >>> struct dpu_global_state *global_state; >>> struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC]; >>> struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; >>> - struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; >>> - struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; >>> struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; >>> - int num_lm, num_ctl, num_pp, num_dsc; >>> + int num_ctl, num_pp, num_dsc; >>> unsigned int dsc_mask = 0; >>> int i; >>> >>> @@ -1133,11 +1168,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >>> ARRAY_SIZE(hw_pp)); >>> num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, >>> drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); >>> - num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, >>> - drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); >>> - dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, >>> - drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, >>> - ARRAY_SIZE(hw_dspp)); >>> >>> for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) >>> dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) >>> @@ -1163,18 +1193,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, >>> dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL; >>> } >>> >>> - cstate = to_dpu_crtc_state(crtc_state); >>> - >>> - for (i = 0; i < num_lm; i++) { >>> - int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); >>> - >>> - cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); >>> - cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); >>> - cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); >>> - } >>> - >>> - cstate->num_mixers = num_lm; >>> - >>> dpu_enc->connector = conn_state->connector; >>> >>> for (i = 0; i < dpu_enc->num_phys_encs; i++) { >>> > > >
On Tue, 27 Aug 2024 at 23:35, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 7/16/2024 4:06 PM, Dmitry Baryshkov wrote: > > On Wed, 17 Jul 2024 at 01:40, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote: > >>> Historically CRTC resources (LMs and CTLs) were assigned in > >>> dpu_crtc_atomic_begin(). The commit 9222cdd27e82 ("drm/msm/dpu: move hw > >>> resource tracking to crtc state") simply moved resources to > >>> struct dpu_crtc_state, without changing the code sequence. Later on the > >>> commit b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder > >>> modeset") rearanged the code, but still kept the cstate->num_mixers > >>> assignment to happen during commit phase. This makes dpu_crtc_state > >>> inconsistent between consequent atomic_check() calls. > >>> > >>> Move CRTC resource assignment to happen at the end of > >>> dpu_encoder_virt_atomic_check(). > >>> > > I just realized, why is the subject saying > dpu_encoder_virt_atomic_mode_set while the patch is moving the > assignment to dpu_encoder_virt_atomic_check(). It should say > dpu_encoder_virt_atomic_check(). Ack > > One more commment below. > > >>> Fixes: b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder modeset") > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> --- > >>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 -- > >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 60 +++++++++++++++++++---------- > >>> 2 files changed, 39 insertions(+), 24 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >>> index 9f2164782844..7399794d75eb 100644 > >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > >>> @@ -1094,9 +1094,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > >>> drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) > >>> dpu_encoder_register_frame_event_callback(encoder, NULL, NULL); > >>> > >>> - memset(cstate->mixers, 0, sizeof(cstate->mixers)); > >>> - cstate->num_mixers = 0; > >>> - > >> > >> Any reason this part was removed? > >> > >> I think we still need this part. > >> > >> In dpu_encoder_get_topology(), we do not assign topology.num_lm based on > >> state parameters, its based on catalog and intf_count which are decided > >> at init time itself. > >> > >> Which means cstate->num_mixers will remain at a non-zero value even if > >> we have NO_MODE which is what happens when we are disabling the CRTC > >> during suspend or hotplug. > > > > In the disable path the driver calls dpu_rm_release(), releasing all > > resources. So there will be no assigned mixers, which matches num_lm = > > 0 in dpu_encoder_assign_crtc_resources(). > > > > Ack. > > >> > >>> /* disable clk & bw control until clk & bw properties are set */ > >>> cstate->bw_control = false; > >>> cstate->bw_split_vote = false; > >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>> index 7613005fbfea..98f3a8d84300 100644 > >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>> @@ -628,6 +628,41 @@ static struct msm_display_topology dpu_encoder_get_topology( > >>> return topology; > >>> } > >>> > >>> +static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms, > >>> + struct drm_encoder *drm_enc, > >>> + struct dpu_global_state *global_state, > >>> + struct drm_crtc_state *crtc_state) > >>> +{ > >>> + struct dpu_crtc_state *cstate; > >>> + struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > >>> + struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > >>> + struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC]; > >>> + int num_lm, num_ctl, num_dspp, i; > >>> + > >>> + cstate = to_dpu_crtc_state(crtc_state); > >>> + > >>> + memset(cstate->mixers, 0, sizeof(cstate->mixers)); > >>> + > >>> + num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > >>> + drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); > >>> + num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > >>> + drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); > >>> + num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > >>> + drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > >>> + ARRAY_SIZE(hw_dspp)); > >>> + > >>> + for (i = 0; i < num_lm; i++) { > >>> + int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); > >>> + > >>> + cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); > >>> + cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); > >>> + if (i < num_dspp) > >> > >> Will there be a case where num_lm != num_dspp? > > > > No, _dpu_rm_check_lm_and_get_connected_blks() makes sure that num_lm = num_dspp. > > I will drop this condition and add a comment. > > > >> > >>> + cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); > >>> + } > >>> + > >>> + cstate->num_mixers = num_lm; > >>> +} > >>> + > >>> static int dpu_encoder_virt_atomic_check( > >>> struct drm_encoder *drm_enc, > >>> struct drm_crtc_state *crtc_state, > >>> @@ -698,6 +733,9 @@ static int dpu_encoder_virt_atomic_check( > >>> drm_enc, crtc_state, topology); > >>> } > >>> > >>> + if (!ret) > >>> + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, global_state, crtc_state); > >>> + > > Do we need to protect this with a needs_modeset() condition? Like why > should this get called every check? Sounds like a good idea. > > >>> trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags); > >>> > >>> return ret; > >>> @@ -1097,14 +1135,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > >>> struct dpu_encoder_virt *dpu_enc; > >>> struct msm_drm_private *priv; > >>> struct dpu_kms *dpu_kms; > >>> - struct dpu_crtc_state *cstate; > >>> struct dpu_global_state *global_state; > >>> struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC]; > >>> struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > >>> - struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > >>> - struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; > >>> struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; > >>> - int num_lm, num_ctl, num_pp, num_dsc; > >>> + int num_ctl, num_pp, num_dsc; > >>> unsigned int dsc_mask = 0; > >>> int i; > >>> > >>> @@ -1133,11 +1168,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > >>> ARRAY_SIZE(hw_pp)); > >>> num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > >>> drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); > >>> - num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > >>> - drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); > >>> - dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > >>> - drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > >>> - ARRAY_SIZE(hw_dspp)); > >>> > >>> for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > >>> dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) > >>> @@ -1163,18 +1193,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > >>> dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL; > >>> } > >>> > >>> - cstate = to_dpu_crtc_state(crtc_state); > >>> - > >>> - for (i = 0; i < num_lm; i++) { > >>> - int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); > >>> - > >>> - cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); > >>> - cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); > >>> - cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); > >>> - } > >>> - > >>> - cstate->num_mixers = num_lm; > >>> - > >>> dpu_enc->connector = conn_state->connector; > >>> > >>> for (i = 0; i < dpu_enc->num_phys_encs; i++) { > >>> > > > > > >
On Wed, 17 Jul 2024 at 01:40, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote: > > Historically CRTC resources (LMs and CTLs) were assigned in > > dpu_crtc_atomic_begin(). The commit 9222cdd27e82 ("drm/msm/dpu: move hw > > resource tracking to crtc state") simply moved resources to > > struct dpu_crtc_state, without changing the code sequence. Later on the > > commit b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder > > modeset") rearanged the code, but still kept the cstate->num_mixers > > assignment to happen during commit phase. This makes dpu_crtc_state > > inconsistent between consequent atomic_check() calls. > > > > Move CRTC resource assignment to happen at the end of > > dpu_encoder_virt_atomic_check(). > > > > Fixes: b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder modeset") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 -- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 60 +++++++++++++++++++---------- > > 2 files changed, 39 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > index 7613005fbfea..98f3a8d84300 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -628,6 +628,41 @@ static struct msm_display_topology dpu_encoder_get_topology( > > return topology; > > } > > > > +static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms, > > + struct drm_encoder *drm_enc, > > + struct dpu_global_state *global_state, > > + struct drm_crtc_state *crtc_state) > > +{ > > + struct dpu_crtc_state *cstate; > > + struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > > + struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > > + struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC]; > > + int num_lm, num_ctl, num_dspp, i; > > + > > + cstate = to_dpu_crtc_state(crtc_state); > > + > > + memset(cstate->mixers, 0, sizeof(cstate->mixers)); > > + > > + num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > + drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); > > + num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > + drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); > > + num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > + drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > > + ARRAY_SIZE(hw_dspp)); > > + > > + for (i = 0; i < num_lm; i++) { > > + int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); > > + > > + cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); > > + cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); > > + if (i < num_dspp) > > Will there be a case where num_lm != num_dspp? And there is one. If DSPPs were not requested at all. I'll handle this by setting hw_dspp to NULL instead. > > > + cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); > > + } > > + > > + cstate->num_mixers = num_lm; > > +} > > + > > static int dpu_encoder_virt_atomic_check( > > struct drm_encoder *drm_enc, > > struct drm_crtc_state *crtc_state, > > @@ -698,6 +733,9 @@ static int dpu_encoder_virt_atomic_check( > > drm_enc, crtc_state, topology); > > } > > > > + if (!ret) > > + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, global_state, crtc_state); > > + > > trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags); > > > > return ret; > > @@ -1097,14 +1135,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > struct dpu_encoder_virt *dpu_enc; > > struct msm_drm_private *priv; > > struct dpu_kms *dpu_kms; > > - struct dpu_crtc_state *cstate; > > struct dpu_global_state *global_state; > > struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC]; > > struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > > - struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > > - struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; > > struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; > > - int num_lm, num_ctl, num_pp, num_dsc; > > + int num_ctl, num_pp, num_dsc; > > unsigned int dsc_mask = 0; > > int i; > > > > @@ -1133,11 +1168,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > ARRAY_SIZE(hw_pp)); > > num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); > > - num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > - drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); > > - dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, > > - drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, > > - ARRAY_SIZE(hw_dspp)); > > > > for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) > > dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) > > @@ -1163,18 +1193,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > > dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL; > > } > > > > - cstate = to_dpu_crtc_state(crtc_state); > > - > > - for (i = 0; i < num_lm; i++) { > > - int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); > > - > > - cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); > > - cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); > > - cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); > > - } > > - > > - cstate->num_mixers = num_lm; > > - > > dpu_enc->connector = conn_state->connector; > > > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 9f2164782844..7399794d75eb 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1094,9 +1094,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) dpu_encoder_register_frame_event_callback(encoder, NULL, NULL); - memset(cstate->mixers, 0, sizeof(cstate->mixers)); - cstate->num_mixers = 0; - /* disable clk & bw control until clk & bw properties are set */ cstate->bw_control = false; cstate->bw_split_vote = false; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 7613005fbfea..98f3a8d84300 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -628,6 +628,41 @@ static struct msm_display_topology dpu_encoder_get_topology( return topology; } +static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms, + struct drm_encoder *drm_enc, + struct dpu_global_state *global_state, + struct drm_crtc_state *crtc_state) +{ + struct dpu_crtc_state *cstate; + struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; + struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; + struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC]; + int num_lm, num_ctl, num_dspp, i; + + cstate = to_dpu_crtc_state(crtc_state); + + memset(cstate->mixers, 0, sizeof(cstate->mixers)); + + num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, + drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); + num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, + drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); + num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, + drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, + ARRAY_SIZE(hw_dspp)); + + for (i = 0; i < num_lm; i++) { + int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); + + cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); + cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); + if (i < num_dspp) + cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); + } + + cstate->num_mixers = num_lm; +} + static int dpu_encoder_virt_atomic_check( struct drm_encoder *drm_enc, struct drm_crtc_state *crtc_state, @@ -698,6 +733,9 @@ static int dpu_encoder_virt_atomic_check( drm_enc, crtc_state, topology); } + if (!ret) + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, global_state, crtc_state); + trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags); return ret; @@ -1097,14 +1135,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, struct dpu_encoder_virt *dpu_enc; struct msm_drm_private *priv; struct dpu_kms *dpu_kms; - struct dpu_crtc_state *cstate; struct dpu_global_state *global_state; struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC]; struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; - struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; - struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; - int num_lm, num_ctl, num_pp, num_dsc; + int num_ctl, num_pp, num_dsc; unsigned int dsc_mask = 0; int i; @@ -1133,11 +1168,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, ARRAY_SIZE(hw_pp)); num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl)); - num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, - drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm)); - dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state, - drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp, - ARRAY_SIZE(hw_dspp)); for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i]) @@ -1163,18 +1193,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL; } - cstate = to_dpu_crtc_state(crtc_state); - - for (i = 0; i < num_lm; i++) { - int ctl_idx = (i < num_ctl) ? i : (num_ctl-1); - - cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]); - cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]); - cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]); - } - - cstate->num_mixers = num_lm; - dpu_enc->connector = conn_state->connector; for (i = 0; i < dpu_enc->num_phys_encs; i++) {
Historically CRTC resources (LMs and CTLs) were assigned in dpu_crtc_atomic_begin(). The commit 9222cdd27e82 ("drm/msm/dpu: move hw resource tracking to crtc state") simply moved resources to struct dpu_crtc_state, without changing the code sequence. Later on the commit b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder modeset") rearanged the code, but still kept the cstate->num_mixers assignment to happen during commit phase. This makes dpu_crtc_state inconsistent between consequent atomic_check() calls. Move CRTC resource assignment to happen at the end of dpu_encoder_virt_atomic_check(). Fixes: b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder modeset") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 -- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 60 +++++++++++++++++++---------- 2 files changed, 39 insertions(+), 24 deletions(-)