Re: [DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 12, 2018 at 06:17:47PM -0700, Jeykumar Sankaran wrote:
> Switch to state based resource management. This patch
> overhauls the resource manager and HW allocation methods by
> maintaining the global resource pool and allocated hw
> blocks in respective drm component states.
> 
> Global resource manager(RM) is tracked in private object.
> Allocation strategy is switched from single point allocation
> of HW resources for the display pipeline to per component
> based allocation, where each drm component allocates HW
> blocks mapped to it's domain and tracks them in their respective
> state objects.
> 
> Fixes resource contention due to race conditions between
> user space and display thread by reserving resources
> only in atomic check.
> 
> Signed-off-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           | 210 +++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h           |  59 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 223 ++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |   4 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |   9 +-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   |  32 +-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  86 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c            |  19 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h            |   8 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 805 ++++++---------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             | 149 ++--
>  11 files changed, 534 insertions(+), 1070 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 426e2ad..a484c06 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -47,6 +47,8 @@
>  #define RIGHT_MIXER 1
>  
>  #define MISR_BUFF_SIZE			256
> +#define MAX_VDISPLAY_SPLIT		1080
> +
>  
>  static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
>  {
> @@ -142,9 +144,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 dpu_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 (dpu_kms_rect_is_null(lm_roi))
> @@ -182,7 +184,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>  		return;
>  	}
>  
> -	ctl = mixer->hw_ctl;
> +	ctl = mixer->lm_ctl;
>  	lm = mixer->hw_lm;
>  	stage_cfg = &dpu_crtc->stage_cfg;
>  	cstate = to_dpu_crtc_state(crtc->state);
> @@ -237,7 +239,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>  			format->base.pixel_format, 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;
> @@ -260,7 +262,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;
>  	struct dpu_crtc_mixer *mixer;
>  	struct dpu_hw_ctl *ctl;
>  	struct dpu_hw_mixer *lm;
> @@ -271,26 +273,26 @@ 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);

Nit - this could be worded a bit better - "too many mixers" would be better, but
I have to ask - under what circumstances would the number of mixers be larger
than CRTC_DUAL_MIXERS and/or why don't we support a dynamic number of mixers?

>  		return;
>  	}

<ship>

> +static void _dpu_crtc_setup_mixers(struct drm_crtc_state *crtc_state)
>  {
> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> -	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> -	struct dpu_rm *rm = &dpu_kms->rm;
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
>  	struct dpu_crtc_mixer *mixer;
> -	struct dpu_hw_ctl *last_valid_ctl = NULL;
>  	int i;
> -	struct dpu_rm_hw_iter lm_iter, ctl_iter;
>  
> -	dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM);
> -	dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL);
> +	if (!cstate->num_mixers || !cstate->num_ctls) {
> +		DPU_ERROR("invalid hw count-lm's:%d ctl's:%d\n",
> +			cstate->num_mixers, cstate->num_ctls);

This can also be reworded - I don't know what lm is, and you shouldn't use an
apostrophe - it would just be 'ctls' or 'mixers'.  Instead of invalid, perhaps
using "no mixers defined" and "no controls defined".


> +		return;
> +	}

<snip>

> @@ -1584,6 +1579,23 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  		}
>  	}
>  
> +	/* Resource allocation */
> +	dpu_priv_state = dpu_get_private_state(state->state);
> +

Ah, here is a use of dpu_get_private_state() that assumes dpu_priv_state is
valid - this could use a ERR check but I think it also implies that
dpu_get_private_state() doesn't have a legitimate reason to return NULL.

> +	topology = dpu_crtc_get_topology(cstate, &state->adjusted_mode);
> +	if (!topology.needs_realloc)
> +		goto end;
> +
> +	dpu_rm_release_crtc_res(&dpu_priv_state->rm, cstate);
> +	rc = dpu_rm_reserve_crtc_res(&dpu_priv_state->rm, cstate, &topology);
> +	if (rc) {
> +		DPU_ERROR("failed to allocate resources for crtc: %d\n",
> +				crtc->base.id);
> +		goto end;
> +	}
> +
> +	_dpu_crtc_setup_mixers(state);
> +
>  end:
>  	return rc;
>  }

<snip>

> @@ -657,27 +666,31 @@ static int dpu_encoder_virt_atomic_check(
>  		}
>  	}
>  
> -	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> +	if (ret)
> +		goto end;
>  
> -	/* Reserve dynamic resources now. Indicating AtomicTest phase */
> -	if (!ret) {
> -		/*
> -		 * Avoid reserving resources when mode set is pending. Topology
> -		 * info may not be available to complete reservation.
> -		 */
> -		if (drm_atomic_crtc_needs_modeset(crtc_state)
> -				&& dpu_enc->mode_set_complete) {
> -			ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state,
> -				conn_state, topology, true);
> -			dpu_enc->mode_set_complete = false;
> -		}
> +	/* hw resource allocation */
> +	dpu_encoder_get_hw_resources(drm_enc, &enc_hw_res, conn_state);
> +
> +	dpu_priv_state = dpu_get_private_state(crtc_state->state);

Here is another use of dpu_get_private_state() that assumes a valid pointer.

> +	topology = dpu_encoder_get_topology(dpu_enc, dpu_cstate);
> +	if (!topology.needs_realloc)
> +		goto end;
> +
> +	dpu_rm_release_encoder_res(&dpu_priv_state->rm, dpu_cstate);
> +	ret = dpu_rm_reserve_encoder_res(&dpu_priv_state->rm,
> +						     dpu_cstate, &enc_hw_res);
> +	if (ret) {
> +		DPU_ERROR_ENC(dpu_enc,
> +				"failed to allocate hw resources\n");
> +		goto end;
>  	}
>  
> -	if (!ret)
> -		drm_mode_set_crtcinfo(adj_mode, 0);
> +	drm_mode_set_crtcinfo(adj_mode, 0);
>  
>  	DPU_EVT32(DRMID(drm_enc), adj_mode->flags, adj_mode->private_flags);
> -
> +end:
>  	return ret;
>  }

<snip>

> @@ -1170,35 +1159,48 @@ void dpu_encoder_virt_restore(struct drm_encoder *drm_enc)
>  static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
>  {
>  	struct dpu_encoder_virt *dpu_enc = NULL;
> +	struct dpu_encoder_phys *phys  = NULL;
>  	int i, ret = 0;
> -	struct drm_display_mode *cur_mode = NULL;
>  
>  	if (!drm_enc) {
>  		DPU_ERROR("invalid encoder\n");
>  		return;
>  	}
>  	dpu_enc = to_dpu_encoder_virt(drm_enc);
> -	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>  
>  	DPU_DEBUG_ENC(dpu_enc, "\n");
> -	DPU_EVT32(DRMID(drm_enc), cur_mode->hdisplay, cur_mode->vdisplay);
>  
>  	dpu_enc->cur_master = NULL;
> +	dpu_enc->cur_slave = NULL;
>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> +		phys = dpu_enc->phys_encs[i];
>  
> -		if (phys && phys->ops.is_master && phys->ops.is_master(phys)) {
> -			DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n", i);
> +		if (!phys || !phys->ops.is_master)
> +			continue;
> +
> +		if (phys->ops.is_master(phys)) {
> +			DPU_DEBUG_ENC(dpu_enc, "master is at idx %d\n", i);
>  			dpu_enc->cur_master = phys;
> -			break;
> +		} else {
> +			DPU_DEBUG_ENC(dpu_enc, "slave is at idx %d\n", i);
> +			dpu_enc->cur_slave = phys;
>  		}
>  	}
>  
>  	if (!dpu_enc->cur_master) {
> -		DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
> +		DPU_ERROR("virt encoder has no master!\n");

I don't think this rises to the occasion of needing a exclamation point.

<snip>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux