Re: [PATCH 10/14] drm/msm/dpu: move hw resource tracking to crtc state

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 2018-08-31 07:56, Sean Paul wrote:
> On Tue, Aug 28, 2018 at 05:39:59PM -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 v4...
> 
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 60
> ++++++++++++++++++--------------
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 22 ++++++------
>>  2 files changed, 44 insertions(+), 38 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index e061847..7ab320d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -163,9 +163,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))
>> @@ -246,7 +246,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, format);
>> 
>> @@ -270,7 +270,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;
>> @@ -281,17 +281,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) {
> 
> This is not possible.
> 
>> +		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;
>> @@ -308,7 +308,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;
>> 
>> @@ -530,7 +530,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;
>> @@ -542,8 +542,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;
>> @@ -568,7 +568,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",
>> @@ -579,11 +579,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));
> 
> Why is this necessary?
> 
>> 
>>  	mutex_lock(&dpu_crtc->crtc_lock);
>>  	/* Check for mixers on all encoders attached to this crtc */
>> @@ -618,7 +618,7 @@ static void _dpu_crtc_setup_lm_bounds(struct
> drm_crtc *crtc,
>>  	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;
>> @@ -635,6 +635,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;
>> @@ -654,10 +655,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);
>>  	}
>> @@ -684,7 +686,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);
>> @@ -748,7 +750,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;
>> 
>>  	/*
>> @@ -863,7 +865,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");
>> @@ -1097,12 +1099,14 @@ static void dpu_crtc_handle_power_event(u32
> event_type, void *arg)
>>  	struct drm_crtc *crtc = arg;
>>  	struct dpu_crtc *dpu_crtc;
>>  	struct drm_encoder *encoder;
>> +	struct dpu_crtc_state *cstate;
>> 
>>  	if (!crtc) {
>>  		DPU_ERROR("invalid crtc\n");
>>  		return;
>>  	}
>>  	dpu_crtc = to_dpu_crtc(crtc);
>> +	cstate = to_dpu_crtc_state(dpu_crtc->base.state);
>> 
>>  	mutex_lock(&dpu_crtc->crtc_lock);
>> 
>> @@ -1197,9 +1201,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));
> 
> Same question here, why is this necessary?
> 
>> +	cstate->num_mixers = 0;
>> 
>>  	/* disable clk & bw control until clk & bw properties are set */
>>  	cstate->bw_control = false;
>> @@ -1547,6 +1550,8 @@ static int _dpu_debugfs_status_show(struct
> seq_file *s, void *data)
>> 
>>  	dpu_crtc = s->private;
>>  	crtc = &dpu_crtc->base;
>> +
>> +	drm_modeset_lock(&crtc->mutex, NULL);
> 
> When I suggested this, I was hoping you'd put it in a separate patch.
> Additionally, you'll probably want modeset_lock_all instead.

crtc state retrieval is introduced in this patch. So I thought it would 
make
sense to add the locking as well a part of this patch.

> 
>>  	cstate = to_dpu_crtc_state(crtc->state);
>> 
>>  	mutex_lock(&dpu_crtc->crtc_lock);
>> @@ -1558,8 +1563,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];
>>  		if (!m->hw_lm)
>>  			seq_printf(s, "\tmixer[%d] has no lm\n", i);
>>  		else if (!m->hw_ctl)
>> @@ -1639,6 +1644,7 @@ static int _dpu_debugfs_status_show(struct
> seq_file *s, void *data)
>>  	seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested);
>> 
>>  	mutex_unlock(&dpu_crtc->crtc_lock);
>> +	drm_modeset_unlock(&crtc->mutex);
>> 
>>  	return 0;
>>  }
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 5e4dc5c..7aa772f 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
>> @@ -164,12 +159,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;
>> 
>> @@ -221,6 +210,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 active ctl paths
>>   */
>>  struct dpu_crtc_state {
>>  	struct drm_crtc_state base;
>> @@ -232,6 +225,13 @@ struct dpu_crtc_state {
>>  	uint64_t input_fence_timeout_ns;
>> 
>>  	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) \
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>> 

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno