[PATCH v6 02/15] drm/msm/dpu: move CRTC resource assignment to dpu_encoder_virt_atomic_check

[Dmitry Baryshkov]

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 at 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>

-- 
With best wishes
Dmitry