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?

Analysing more on the need for this memset. After a crtc is disabled,
it can also be used with a mode where the needed no. of layer mixers
can be different than the current one. So isn't it always safe to
clear the stale pointers to hw blocks?

Thanks and Regards,
Jeykumar S.

> 
>> +	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.
> 
>>  	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