| Message ID | 20240903-dpu-mode-config-width-v6-2-617e1ecc4b7a@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm/dpu: be more friendly to X.org | expand |
On 9/2/2024 8:22 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(). > Mostly LGTM now, a couple of comments/questions 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 | 59 +++++++++++++++++++---------- > 2 files changed, 38 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 4c1be2f0555f..e81feb0d67f3 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1091,9 +1091,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > > dpu_core_perf_crtc_update(crtc, 0); > > - memset(cstate->mixers, 0, sizeof(cstate->mixers)); > - cstate->num_mixers = 0; > - Note to myself: This chunk will conflict with https://patchwork.freedesktop.org/patch/611385/, we will need to remove this part in the CWB series when its rebased. > /* 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 949ebda2fa82..bd3698bf0cf7 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -624,6 +624,40 @@ 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]); > + cstate->mixers[i].hw_dspp = i < num_dspp ? to_dpu_hw_dspp(hw_dspp[i]) : NULL; > + } > + > + cstate->num_mixers = num_lm; > +} > + > static int dpu_encoder_virt_atomic_check( > struct drm_encoder *drm_enc, > struct drm_crtc_state *crtc_state, > @@ -692,6 +726,9 @@ static int dpu_encoder_virt_atomic_check( > if (!crtc_state->active_changed || crtc_state->enable) > ret = dpu_rm_reserve(&dpu_kms->rm, global_state, > drm_enc, crtc_state, topology); > + if (!ret) > + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, > + global_state, crtc_state); > } This is now under the drm_atomic_crtc_needs_modeset() condition which is good, but shouldnt this also move under the same if condition as dpu_rm_reserve()? There cannot be any assignment without reservation right? > <snip>
On Mon, Sep 16, 2024 at 06:04:08PM GMT, Abhinav Kumar wrote: > > > On 9/2/2024 8:22 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(). > > > > Mostly LGTM now, a couple of comments/questions 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 | 59 +++++++++++++++++++---------- > > 2 files changed, 38 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 949ebda2fa82..bd3698bf0cf7 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -624,6 +624,40 @@ 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]); > > + cstate->mixers[i].hw_dspp = i < num_dspp ? to_dpu_hw_dspp(hw_dspp[i]) : NULL; > > + } > > + > > + cstate->num_mixers = num_lm; > > +} > > + > > static int dpu_encoder_virt_atomic_check( > > struct drm_encoder *drm_enc, > > struct drm_crtc_state *crtc_state, > > @@ -692,6 +726,9 @@ static int dpu_encoder_virt_atomic_check( > > if (!crtc_state->active_changed || crtc_state->enable) > > ret = dpu_rm_reserve(&dpu_kms->rm, global_state, > > drm_enc, crtc_state, topology); > > + if (!ret) > > + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, > > + global_state, crtc_state); > > } > > This is now under the drm_atomic_crtc_needs_modeset() condition which is > good, but shouldnt this also move under the same if condition as > dpu_rm_reserve()? There cannot be any assignment without reservation right? Maybe it's not that obvious from the function name, but it will also clear previously assigned resources. So, I think it is correct to be called even if the resources were released without further assignment. > > > <snip>
On 9/16/2024 11:13 PM, Dmitry Baryshkov wrote: > On Mon, Sep 16, 2024 at 06:04:08PM GMT, Abhinav Kumar wrote: >> >> >> On 9/2/2024 8:22 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(). >>> >> >> Mostly LGTM now, a couple of comments/questions 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 | 59 +++++++++++++++++++---------- >>> 2 files changed, 38 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 949ebda2fa82..bd3698bf0cf7 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -624,6 +624,40 @@ 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]); >>> + cstate->mixers[i].hw_dspp = i < num_dspp ? to_dpu_hw_dspp(hw_dspp[i]) : NULL; >>> + } >>> + >>> + cstate->num_mixers = num_lm; >>> +} >>> + >>> static int dpu_encoder_virt_atomic_check( >>> struct drm_encoder *drm_enc, >>> struct drm_crtc_state *crtc_state, >>> @@ -692,6 +726,9 @@ static int dpu_encoder_virt_atomic_check( >>> if (!crtc_state->active_changed || crtc_state->enable) >>> ret = dpu_rm_reserve(&dpu_kms->rm, global_state, >>> drm_enc, crtc_state, topology); >>> + if (!ret) >>> + dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, >>> + global_state, crtc_state); >>> } >> >> This is now under the drm_atomic_crtc_needs_modeset() condition which is >> good, but shouldnt this also move under the same if condition as >> dpu_rm_reserve()? There cannot be any assignment without reservation right? > > Maybe it's not that obvious from the function name, but it will also > clear previously assigned resources. So, I think it is correct to be > called even if the resources were released without further assignment. > Ack, yes I missed the dpu_rm_release() line just before it, hence Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >> >> <snip> >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 4c1be2f0555f..e81feb0d67f3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1091,9 +1091,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, dpu_core_perf_crtc_update(crtc, 0); - 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 949ebda2fa82..bd3698bf0cf7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -624,6 +624,40 @@ 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]); + cstate->mixers[i].hw_dspp = i < num_dspp ? to_dpu_hw_dspp(hw_dspp[i]) : NULL; + } + + cstate->num_mixers = num_lm; +} + static int dpu_encoder_virt_atomic_check( struct drm_encoder *drm_enc, struct drm_crtc_state *crtc_state, @@ -692,6 +726,9 @@ static int dpu_encoder_virt_atomic_check( if (!crtc_state->active_changed || crtc_state->enable) ret = dpu_rm_reserve(&dpu_kms->rm, global_state, 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); @@ -1093,14 +1130,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; @@ -1129,11 +1163,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]) @@ -1159,18 +1188,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; - for (i = 0; i < dpu_enc->num_phys_encs; i++) { struct dpu_encoder_phys *phys = dpu_enc->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 | 59 +++++++++++++++++++---------- 2 files changed, 38 insertions(+), 24 deletions(-)