Re: [PATCH v3 08/13] drm/msm/dpu: move hw resource tracking to crtc state

[Sean Paul <sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>]

On Tue, Aug 07, 2018 at 08:12:35PM -0700, Jeykumar Sankaran wrote:
> Prep changes for state based resource management.
> 
> Moves all the hw block tracking for the crtc to the state
> object.
> 
> changes in v2:
> 	- none
> changes in v3:
> 	- none
> 
> Change-Id: I2816e9e28b27f1126b477d62eb3858a30a652747
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77 +++++++++++++++++---------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 25 +++++------
>  2 files changed, 54 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 1f2d223..515b0e6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -136,9 +136,9 @@ static void _dpu_crtc_program_lm_output_roi(struct drm_crtc *crtc)
>  	crtc_state = to_dpu_crtc_state(crtc->state);
>  
>  	lm_horiz_position = 0;
> -	for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
> +	for (lm_idx = 0; lm_idx < crtc_state->num_mixers; lm_idx++) {
>  		const struct drm_rect *lm_roi = &crtc_state->lm_bounds[lm_idx];
> -		struct dpu_hw_mixer *hw_lm = dpu_crtc->mixers[lm_idx].hw_lm;
> +		struct dpu_hw_mixer *hw_lm = crtc_state->mixers[lm_idx].hw_lm;
>  		struct dpu_hw_mixer_cfg cfg;
>  
>  		if (!lm_roi || !drm_rect_visible(lm_roi))
> @@ -219,7 +219,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>  					   fb ? fb->modifier : 0);
>  
>  		/* blend config update */
> -		for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
> +		for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
>  			_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate);
>  
>  			mixer[lm_idx].flush_mask |= flush_mask;
> @@ -242,7 +242,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>  static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
>  {
>  	struct dpu_crtc *dpu_crtc;
> -	struct dpu_crtc_state *dpu_crtc_state;
> +	struct dpu_crtc_state *cstate;

Unrelated var renames are kind of noisy, but I suppose it's just one, so that's
ok.

>  	struct dpu_crtc_mixer *mixer;
>  	struct dpu_hw_ctl *ctl;
>  	struct dpu_hw_mixer *lm;
> @@ -253,17 +253,17 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
>  		return;
>  
>  	dpu_crtc = to_dpu_crtc(crtc);
> -	dpu_crtc_state = to_dpu_crtc_state(crtc->state);
> -	mixer = dpu_crtc->mixers;
> +	cstate = to_dpu_crtc_state(crtc->state);
> +	mixer = cstate->mixers;
>  
>  	DPU_DEBUG("%s\n", dpu_crtc->name);
>  
> -	if (dpu_crtc->num_mixers > CRTC_DUAL_MIXERS) {
> -		DPU_ERROR("invalid number mixers: %d\n", dpu_crtc->num_mixers);
> +	if (cstate->num_mixers > CRTC_DUAL_MIXERS) {
> +		DPU_ERROR("invalid number mixers: %d\n", cstate->num_mixers);
>  		return;
>  	}
>  
> -	for (i = 0; i < dpu_crtc->num_mixers; i++) {
> +	for (i = 0; i < cstate->num_mixers; i++) {
>  		if (!mixer[i].hw_lm || !mixer[i].hw_ctl) {
>  			DPU_ERROR("invalid lm or ctl assigned to mixer\n");
>  			return;
> @@ -280,7 +280,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
>  
>  	_dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer);
>  
> -	for (i = 0; i < dpu_crtc->num_mixers; i++) {
> +	for (i = 0; i < cstate->num_mixers; i++) {
>  		ctl = mixer[i].hw_ctl;
>  		lm = mixer[i].hw_lm;
>  
> @@ -502,7 +502,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
>  		struct drm_crtc *crtc,
>  		struct drm_encoder *enc)
>  {
> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>  	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>  	struct dpu_rm *rm = &dpu_kms->rm;
>  	struct dpu_crtc_mixer *mixer;
> @@ -514,8 +514,8 @@ static void _dpu_crtc_setup_mixer_for_encoder(
>  	dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL);
>  
>  	/* Set up all the mixers and ctls reserved by this encoder */
> -	for (i = dpu_crtc->num_mixers; i < ARRAY_SIZE(dpu_crtc->mixers); i++) {
> -		mixer = &dpu_crtc->mixers[i];
> +	for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) {
> +		mixer = &cstate->mixers[i];
>  
>  		if (!dpu_rm_get_hw(rm, &lm_iter))
>  			break;
> @@ -540,7 +540,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(
>  
>  		mixer->encoder = enc;
>  
> -		dpu_crtc->num_mixers++;
> +		cstate->num_mixers++;
>  		DPU_DEBUG("setup mixer %d: lm %d\n",
>  				i, mixer->hw_lm->idx - LM_0);
>  		DPU_DEBUG("setup mixer %d: ctl %d\n",
> @@ -551,11 +551,11 @@ static void _dpu_crtc_setup_mixer_for_encoder(
>  static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
>  {
>  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>  	struct drm_encoder *enc;
>  
> -	dpu_crtc->num_mixers = 0;
> -	dpu_crtc->mixers_swapped = false;
> -	memset(dpu_crtc->mixers, 0, sizeof(dpu_crtc->mixers));
> +	cstate->num_mixers = 0;
> +	memset(cstate->mixers, 0, sizeof(cstate->mixers));
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	/* Check for mixers on all encoders attached to this crtc */
> @@ -589,7 +589,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
>  	adj_mode = &state->adjusted_mode;
>  	crtc_split_width = dpu_crtc_get_mixer_width(dpu_crtc, cstate, adj_mode);
>  
> -	for (i = 0; i < dpu_crtc->num_mixers; i++) {
> +	for (i = 0; i < cstate->num_mixers; i++) {
>  		struct drm_rect *r = &cstate->lm_bounds[i];
>  		r->x1 = crtc_split_width * i;
>  		r->y1 = 0;
> @@ -606,6 +606,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc,
>  		struct drm_crtc_state *old_state)
>  {
>  	struct dpu_crtc *dpu_crtc;
> +	struct dpu_crtc_state *cstate;
>  	struct drm_encoder *encoder;
>  	struct drm_device *dev;
>  	unsigned long flags;
> @@ -625,10 +626,11 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc,
>  	DPU_DEBUG("crtc%d\n", crtc->base.id);
>  
>  	dpu_crtc = to_dpu_crtc(crtc);
> +	cstate = to_dpu_crtc_state(crtc->state);
>  	dev = crtc->dev;
>  	smmu_state = &dpu_crtc->smmu_state;
>  
> -	if (!dpu_crtc->num_mixers) {
> +	if (!cstate->num_mixers) {
>  		_dpu_crtc_setup_mixers(crtc);
>  		_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
>  	}
> @@ -655,7 +657,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc,
>  	 * it means we are trying to flush a CRTC whose state is disabled:
>  	 * nothing else needs to be done.
>  	 */
> -	if (unlikely(!dpu_crtc->num_mixers))
> +	if (unlikely(!cstate->num_mixers))
>  		return;
>  
>  	_dpu_crtc_blend_setup(crtc);
> @@ -719,7 +721,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
>  	 * it means we are trying to flush a CRTC whose state is disabled:
>  	 * nothing else needs to be done.
>  	 */
> -	if (unlikely(!dpu_crtc->num_mixers))
> +	if (unlikely(!cstate->num_mixers))
>  		return;
>  
>  	/*
> @@ -834,7 +836,7 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  	 * it means we are trying to start a CRTC whose state is disabled:
>  	 * nothing else needs to be done.
>  	 */
> -	if (unlikely(!dpu_crtc->num_mixers))
> +	if (unlikely(!cstate->num_mixers))
>  		return;
>  
>  	DPU_ATRACE_BEGIN("crtc_commit");
> @@ -1069,6 +1071,7 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg)
>  	struct dpu_crtc *dpu_crtc;
>  	struct drm_encoder *encoder;
>  	struct dpu_crtc_mixer *m;
> +	struct dpu_crtc_state *cstate;
>  	u32 i, misr_status;
>  
>  	if (!crtc) {
> @@ -1076,6 +1079,7 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg)
>  		return;
>  	}
>  	dpu_crtc = to_dpu_crtc(crtc);
> +	cstate = to_dpu_crtc_state(dpu_crtc->base.state);
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  
> @@ -1091,8 +1095,8 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg)
>  			dpu_encoder_virt_restore(encoder);
>  		}
>  
> -		for (i = 0; i < dpu_crtc->num_mixers; ++i) {
> -			m = &dpu_crtc->mixers[i];
> +		for (i = 0; i < cstate->num_mixers; ++i) {
> +			m = &cstate->mixers[i];
>  			if (!m->hw_lm || !m->hw_lm->ops.setup_misr ||
>  					!dpu_crtc->misr_enable)
>  				continue;
> @@ -1102,8 +1106,8 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg)
>  		}
>  		break;
>  	case DPU_POWER_EVENT_PRE_DISABLE:
> -		for (i = 0; i < dpu_crtc->num_mixers; ++i) {
> -			m = &dpu_crtc->mixers[i];
> +		for (i = 0; i < cstate->num_mixers; ++i) {
> +			m = &cstate->mixers[i];
>  			if (!m->hw_lm || !m->hw_lm->ops.collect_misr ||
>  					!dpu_crtc->misr_enable)
>  				continue;
> @@ -1191,9 +1195,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  		dpu_power_handle_unregister_event(dpu_crtc->phandle,
>  				dpu_crtc->power_event);
>  
> -	memset(dpu_crtc->mixers, 0, sizeof(dpu_crtc->mixers));
> -	dpu_crtc->num_mixers = 0;
> -	dpu_crtc->mixers_swapped = false;
> +	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;
> @@ -1552,8 +1555,8 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data)
>  
>  	seq_puts(s, "\n");
>  
> -	for (i = 0; i < dpu_crtc->num_mixers; ++i) {
> -		m = &dpu_crtc->mixers[i];
> +	for (i = 0; i < cstate->num_mixers; ++i) {
> +		m = &cstate->mixers[i];

Hmmm, the state accesses in this function aren't properly serialized. Could you
please whip up a patch to acquire the modeset locks?

>  		if (!m->hw_lm)
>  			seq_printf(s, "\tmixer[%d] has no lm\n", i);
>  		else if (!m->hw_ctl)
> @@ -1646,6 +1649,7 @@ static ssize_t _dpu_crtc_misr_setup(struct file *file,
>  		const char __user *user_buf, size_t count, loff_t *ppos)
>  {
>  	struct dpu_crtc *dpu_crtc;
> +	struct dpu_crtc_state *cstate;
>  	struct dpu_crtc_mixer *m;
>  	int i = 0, rc;
>  	char buf[MISR_BUFF_SIZE + 1];
> @@ -1656,6 +1660,7 @@ static ssize_t _dpu_crtc_misr_setup(struct file *file,
>  		return -EINVAL;
>  
>  	dpu_crtc = file->private_data;
> +	cstate = to_dpu_crtc_state(dpu_crtc->base.state);

Same here

>  	buff_copy = min_t(size_t, count, MISR_BUFF_SIZE);
>  	if (copy_from_user(buf, user_buf, buff_copy)) {
>  		DPU_ERROR("buffer copy failed\n");
> @@ -1674,9 +1679,9 @@ static ssize_t _dpu_crtc_misr_setup(struct file *file,
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	dpu_crtc->misr_enable = enable;
>  	dpu_crtc->misr_frame_count = frame_count;
> -	for (i = 0; i < dpu_crtc->num_mixers; ++i) {
> +	for (i = 0; i < cstate->num_mixers; ++i) {
>  		dpu_crtc->misr_data[i] = 0;
> -		m = &dpu_crtc->mixers[i];
> +		m = &cstate->mixers[i];
>  		if (!m->hw_lm || !m->hw_lm->ops.setup_misr)
>  			continue;
>  
> @@ -1692,6 +1697,7 @@ static ssize_t _dpu_crtc_misr_read(struct file *file,
>  		char __user *user_buff, size_t count, loff_t *ppos)
>  {
>  	struct dpu_crtc *dpu_crtc;
> +	struct dpu_crtc_state *cstate;
>  	struct dpu_crtc_mixer *m;
>  	int i = 0, rc;
>  	u32 misr_status;
> @@ -1705,6 +1711,7 @@ static ssize_t _dpu_crtc_misr_read(struct file *file,
>  		return -EINVAL;
>  
>  	dpu_crtc = file->private_data;
> +	cstate = to_dpu_crtc_state(dpu_crtc->base.state);

And here

>  	rc = _dpu_crtc_power_enable(dpu_crtc, true);
>  	if (rc)
>  		return rc;
> @@ -1716,8 +1723,8 @@ static ssize_t _dpu_crtc_misr_read(struct file *file,
>  		goto buff_check;
>  	}
>  
> -	for (i = 0; i < dpu_crtc->num_mixers; ++i) {
> -		m = &dpu_crtc->mixers[i];
> +	for (i = 0; i < cstate->num_mixers; ++i) {
> +		m = &cstate->mixers[i];
>  		if (!m->hw_lm || !m->hw_lm->ops.collect_misr)
>  			continue;
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index e632651..9177ee6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -121,11 +121,6 @@ struct dpu_crtc_frame_event {
>   * struct dpu_crtc - virtualized CRTC data structure
>   * @base          : Base drm crtc structure
>   * @name          : ASCII description of this crtc
> - * @num_ctls      : Number of ctl paths in use
> - * @num_mixers    : Number of mixers in use
> - * @mixers_swapped: Whether the mixers have been swapped for left/right update
> - *                  especially in the case of DSC Merge.
> - * @mixers        : List of active mixers
>   * @event         : Pointer to last received drm vblank event. If there is a
>   *                  pending vblank event, this will be non-null.
>   * @vsync_count   : Running count of received vsync events
> @@ -167,12 +162,6 @@ struct dpu_crtc {
>  	struct drm_crtc base;
>  	char name[DPU_CRTC_NAME_SIZE];
>  
> -	/* HW Resources reserved for the crtc */
> -	u32 num_ctls;
> -	u32 num_mixers;
> -	bool mixers_swapped;
> -	struct dpu_crtc_mixer mixers[CRTC_DUAL_MIXERS];
> -
>  	struct drm_pending_vblank_event *event;
>  	u32 vsync_count;
>  
> @@ -227,6 +216,10 @@ struct dpu_crtc {
>   * @property_values: Current crtc property values
>   * @input_fence_timeout_ns : Cached input fence timeout, in ns
>   * @new_perf: new performance state being requested
> + * @num_mixers    : Number of mixers in use
> + * @mixers        : List of active mixers
> + * @num_ctls      : Number of ctl paths in use
> + * @hw_ctls       : List of activel ctl paths

activel

>   */
>  struct dpu_crtc_state {
>  	struct drm_crtc_state base;
> @@ -236,8 +229,14 @@ struct dpu_crtc_state {
>  	struct drm_rect lm_bounds[CRTC_DUAL_MIXERS];
>  
>  	uint64_t input_fence_timeout_ns;
> -

unrelated whitespace

>  	struct dpu_core_perf_params new_perf;
> +
> +	/* HW Resources reserved for the crtc */
> +	u32 num_mixers;
> +	struct dpu_crtc_mixer mixers[CRTC_DUAL_MIXERS];
> +
> +	u32 num_ctls;
> +	struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];
>  };
>  
>  #define to_dpu_crtc_state(x) \
> @@ -255,7 +254,7 @@ static inline int dpu_crtc_get_mixer_width(struct dpu_crtc *dpu_crtc,
>  	if (!dpu_crtc || !cstate || !mode)
>  		return 0;
>  
> -	mixer_width = (dpu_crtc->num_mixers == CRTC_DUAL_MIXERS ?
> +	mixer_width = (cstate->num_mixers == CRTC_DUAL_MIXERS ?
>  			mode->hdisplay / CRTC_DUAL_MIXERS : mode->hdisplay);

Can you please add a new patch that precedes this to move this function into
dpu_crtc as a static function?

Sean

>  
>  	return mixer_width;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno