Re: [PATCH 05/14] drm/msm/dpu: enable master-slave encoders explicitly

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

On 2018-08-30 09:24, Sean Paul wrote:
> On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:
>> Identify slave-master encoders during initialization and enable
>> the encoders explicitly as the current logic has redundant and
>> ambiguous loops.
> 
> Please include a "Changes in vX" section so I don't need to figure it 
> out
> myself.
> 
Since I split these clean up changes into a new series, I was not sure
how to handle the versioning. With "changes in v4" log, Should I also
change the prefix labeling to [PATCH v4 05/14]? A couple of changes
are also introduced in this series.

Thanks,
Jeykumar S.

>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46
> ++++++++++-------------------
>>  1 file changed, 15 insertions(+), 31 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 991b22c..b223bd2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -180,6 +180,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;
>>  	struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
>> 
>>  	bool intfs_swapped;
>> @@ -1141,7 +1142,8 @@ 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;
>> -	int i, ret = 0;
>> +	struct dpu_encoder_phys *phys  = NULL;
>> +	int ret = 0;
>>  	struct drm_display_mode *cur_mode = NULL;
>> 
>>  	if (!drm_enc) {
>> @@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct
> drm_encoder *drm_enc)
>>  	trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay,
>>  			     cur_mode->vdisplay);
>> 
>> -	dpu_enc->cur_master = NULL;
>> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> -		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> +	/* always enable slave encoder before master */
>> +	phys = dpu_enc->cur_slave;
> 
> I think this variable makes things less readable. It made sense in the
> loop since
> you're indented an extra level and they're obfuscated anyways, but 
> since
> they're
> explicit now, could you please just use dpu_enc->cur_slave/master
> directly?
> 
> With this nit and the added changelog in the commit addressed,
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
> 
>> +	if (phys && phys->ops.enable)
>> +		phys->ops.enable(phys);
>> 
>> -		if (phys && phys->ops.is_master &&
> phys->ops.is_master(phys)) {
>> -			DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n",
> i);
>> -			dpu_enc->cur_master = phys;
>> -			break;
>> -		}
>> -	}
>> -
>> -	if (!dpu_enc->cur_master) {
>> -		DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
>> -		return;
>> -	}
>> +	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) {
>> @@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct
> drm_encoder *drm_enc)
>>  		return;
>>  	}
>> 
>> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> -		struct dpu_encoder_phys *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->cur_master->ops.enable)
>> -		dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
>> -
>>  	_dpu_encoder_virt_enable_helper(drm_enc);
>>  }
>> 
>> @@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs(
>>  		++dpu_enc->num_phys_encs;
>>  	}
>> 
>> +	if (params->split_role == ENC_ROLE_SLAVE)
>> +		dpu_enc->cur_slave = enc;
>> +	else
>> +		dpu_enc->cur_master = enc;
>> +
>>  	return 0;
>>  }
>> 
>> @@ -2228,7 +2213,6 @@ int dpu_encoder_setup(struct drm_device *dev,
> struct drm_encoder *enc,
>>  	if (ret)
>>  		goto fail;
>> 
>> -	dpu_enc->cur_master = NULL;
>>  	spin_lock_init(&dpu_enc->enc_spinlock);
>> 
>>  	atomic_set(&dpu_enc->frame_done_timeout, 0);
>> --
>> 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