Re: [PATCH v3 04/13] drm/msm/dpu: program master-slave encoders explicitly

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

On 2018-08-14 12:19, Sean Paul wrote:
> On Tue, Aug 07, 2018 at 08:12:31PM -0700, Jeykumar Sankaran wrote:
>> Identify slave-master encoders and program them explicitly.
> 
> You've got the what, but could you please explain the why?
> 
>> 
>> changes in v2:
>> 	- none
>> changes in v3:
>> 	- none
>> 
>> Change-Id: I0ebfada05bd7f8437f842ad860490a678aa8f4cd
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 39
> ++++++++++++++++++-----------
>>  1 file changed, 24 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1b4de34..a0ced79 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -184,6 +184,7 @@ struct dpu_encoder_virt {
>>  	unsigned int num_phys_encs;
>>  	struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL];
>>  	struct dpu_encoder_phys *cur_master;
>> +	struct dpu_encoder_phys *cur_slave;
> 
> You only use this in one function, why not just make it a local?
> 
>>  	struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
>> 
>>  	bool intfs_swapped;
>> @@ -1153,6 +1154,7 @@ 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;
>> 
>> @@ -1160,6 +1162,7 @@ static void dpu_encoder_virt_enable(struct
> drm_encoder *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;
>> 
>> @@ -1167,21 +1170,36 @@ static void dpu_encoder_virt_enable(struct
> drm_encoder *drm_enc)
>>  			     cur_mode->vdisplay);
>> 
>>  	dpu_enc->cur_master = NULL;
>> +	dpu_enc->cur_slave = NULL;
> 
> There's no benefit to setting this NULL. cur_master is set to NULL so 
> it
> can be
> checked after the loop. Since you're not checking this, it's not
> necessary.
> 
Checking slave encoder below.
> I suppose you might also want to clear this on disable like master.
> 
>>  	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)
> 
> I don't think it's possible for phys to be NULL, is it?
> 
>> +			continue;
>> 
>> -		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->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;
>>  		}
> 
> You're making an assumption here that there can only be one master and
> there can
> only be one slave.
> 
Isn't that a fact? Do we have a topology in DPU where we have more than 
one master or slave?

> It seems like you could avoid all of this work if you just did the
> assignment in
> dpu_encoder_virt_add_phys_encs().
> 
That is true! Let me try doing that.
>>  	}
>> 
>>  	if (!dpu_enc->cur_master) {
>> -		DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
>> +		DPU_ERROR("virt encoder has no master identified\n");
>>  		return;
>>  	}
>> 
>> +	/* always enable slave encoder before master */
>> +	phys = dpu_enc->cur_slave;
>> +	if (phys && phys->ops.enable)
>> +		phys->ops.enable(phys);
>> +
We are checking for slave encoder being NULL here.
>> +	phys = dpu_enc->cur_master;
>> +	if (phys && phys->ops.enable)
>> +		phys->ops.enable(phys);
>> +
>>  	ret = dpu_encoder_resource_control(drm_enc,
> DPU_ENC_RC_EVENT_KICKOFF);
>>  	if (ret) {
>>  		DPU_ERROR_ENC(dpu_enc, "dpu resource control failed:
> %d\n",
>> @@ -1190,25 +1208,16 @@ static void dpu_encoder_virt_enable(struct
> drm_encoder *drm_enc)
>>  	}
>> 
>>  	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)
>>  			continue;
>> 
>> -		if (phys != dpu_enc->cur_master) {
>> -			if (phys->ops.enable)
>> -				phys->ops.enable(phys);
>> -		}
>> -
>>  		if (dpu_enc->misr_enable &&
> (dpu_enc->disp_info.capabilities &
>>  		     MSM_DISPLAY_CAP_VID_MODE) && phys->ops.setup_misr)
>>  			phys->ops.setup_misr(phys, true,
>> 
> dpu_enc->misr_frame_count);
>>  	}
>> 
>> -	if (dpu_enc->cur_master->ops.enable)
>> -		dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
>> -
> 
> There's a change in functionality here. Previously you could call
> setup_misr
> for slaves after they are enabled, but before master is enabled. Now
> you're
> calling it after all are enabled.
> 
> I'm guessing it doesn't matter since it was previously called on master
> before
> it was enabled, but I figure I'd point this out.
> 
> Sean
> 
Yes. It doesn't matter here.
>>  	_dpu_encoder_virt_enable_helper(drm_enc);
>>  }
>> 
>> --
>> 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