diff mbox series

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

Message ID 1533697956-29686-5-git-send-email-jsanka@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Atomic resource management | expand

Commit Message

Jeykumar Sankaran Aug. 8, 2018, 3:12 a.m. UTC 
Identify slave-master encoders and program them explicitly.

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(-)

Comments

Sean Paul Aug. 14, 2018, 7:19 p.m. UTC | #1 
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.

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.

It seems like you could avoid all of this work if you just did the assignment in
dpu_encoder_virt_add_phys_encs().

>  	}
>  
>  	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);
> +
> +	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

>  	_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 Sankaran Aug. 15, 2018, 12:11 a.m. UTC | #2 
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
>>
diff mbox series

Patch

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;
 	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;
 	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)
+			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;
 		}
 	}
 
 	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);
+
+	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);
-
 	_dpu_encoder_virt_enable_helper(drm_enc);
 }