diff mbox series

[v3,4/7] drm/msm/dpu: add PINGPONG_NONE to disconnect DSC from PINGPONG

Message ID 1683061382-32651-5-git-send-email-quic_khsieh@quicinc.com (mailing list archive)
State Superseded
Headers show
Series add DSC 1.2 dpu supports | expand

Commit Message

Kuogee Hsieh May 2, 2023, 9:02 p.m. UTC
During DSC setup, the crossbar mux need to be programmed to engage
DSC to specified PINGPONG. Hence during tear down, the crossbar mux
need to be reset to disengage DSC from PINGPONG. This patch add
PINGPONG_NONE to serve as disable to reset crossbar mux.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c  | 7 +++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h  | 1 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++-
 4 files changed, 6 insertions(+), 7 deletions(-)

Comments

Dmitry Baryshkov May 2, 2023, 9:21 p.m. UTC | #1
On 03/05/2023 00:02, Kuogee Hsieh wrote:
> During DSC setup, the crossbar mux need to be programmed to engage
> DSC to specified PINGPONG. Hence during tear down, the crossbar mux
> need to be reset to disengage DSC from PINGPONG. This patch add
> PINGPONG_NONE to serve as disable to reset crossbar mux.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c  | 7 +++----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h  | 1 -
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++-
>   4 files changed, 6 insertions(+), 7 deletions(-)
Marijn Suijten May 3, 2023, 8:03 a.m. UTC | #2
On 2023-05-02 14:02:59, Kuogee Hsieh wrote:
> During DSC setup, the crossbar mux need to be programmed to engage
> DSC to specified PINGPONG. Hence during tear down, the crossbar mux
> need to be reset to disengage DSC from PINGPONG. This patch add
> PINGPONG_NONE to serve as disable to reset crossbar mux.

This patch doesn't *just add* PINGPONG_NONE to reset the crossbar; that
functionality was already available thanks to a `bool enable` function
parameter.  Instead it should explain why you think PINGPONG_NONE is
more convenient than passing a bool that warrants this replacement.
(Hint: I think because you don't have a hw_pp->idx available in the
 teardown path, and/or its value is not relevant for the disable case
 anyway.)

In addition I don't see this series use PINGPONG_NONE anywhere yet: will
that be added in the DSC 1.2 series for DP (to support hotplug)?

> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c  | 7 +++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h  | 1 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++-
>  4 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1dc5dbe..d9ad334 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1839,7 +1839,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
>  		hw_pp->ops.setup_dsc(hw_pp);
>  
>  	if (hw_dsc->ops.dsc_bind_pingpong_blk)
> -		hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx);
> +		hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, hw_pp->idx);
>  
>  	if (hw_pp->ops.enable_dsc)
>  		hw_pp->ops.enable_dsc(hw_pp);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index 4a6bbcc..3e68d47 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -157,7 +157,6 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc,
>  
>  static void dpu_hw_dsc_bind_pingpong_blk(
>  		struct dpu_hw_dsc *hw_dsc,
> -		bool enable,
>  		const enum dpu_pingpong pp)
>  {
>  	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> @@ -166,13 +165,13 @@ static void dpu_hw_dsc_bind_pingpong_blk(
>  
>  	dsc_ctl_offset = DSC_CTL(hw_dsc->idx);
>  
> -	if (enable)
> +	if (pp)
>  		mux_cfg = (pp - PINGPONG_0) & 0x7;
>  
>  	DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n",
> -			enable ? "Binding" : "Unbinding",
> +			pp ? "Binding" : "Unbinding",
>  			hw_dsc->idx - DSC_0,
> -			enable ? "to" : "from",
> +			pp ? "to" : "from",
>  			pp - PINGPONG_0);

PINGPONG_NONE - PINGPONG_0 = -1, so this whole debug log likely needs to
be rewritten for the disable case as we don't know what PINGPONG it is
being unbound from.  How about:

	if (pp)
		DRM_DEBUG_KMS("Binding dsc:%d to pp:%d\n",
				hw_dsc->idx - DSC_0,
				pp - PINGPONG_0);
	else
		DRM_DEBUG_KMS("Unbinding dsc:%d from any pp\n",
				hw_dsc->idx - DSC_0);

- Marijn

>  
>  	DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> index 287ec5f..138080a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
> @@ -44,7 +44,6 @@ struct dpu_hw_dsc_ops {
>  				  struct drm_dsc_config *dsc);
>  
>  	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
> -				  bool enable,
>  				  enum dpu_pingpong pp);
>  };
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> index 2d9192a..56826a9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
> @@ -191,7 +191,8 @@ enum dpu_dsc {
>  };
>  
>  enum dpu_pingpong {
> -	PINGPONG_0 = 1,
> +	PINGPONG_NONE,
> +	PINGPONG_0,
>  	PINGPONG_1,
>  	PINGPONG_2,
>  	PINGPONG_3,
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Kuogee Hsieh May 3, 2023, 5:51 p.m. UTC | #3
On 5/3/2023 1:03 AM, Marijn Suijten wrote:
> On 2023-05-02 14:02:59, Kuogee Hsieh wrote:
>> During DSC setup, the crossbar mux need to be programmed to engage
>> DSC to specified PINGPONG. Hence during tear down, the crossbar mux
>> need to be reset to disengage DSC from PINGPONG. This patch add
>> PINGPONG_NONE to serve as disable to reset crossbar mux.
> This patch doesn't *just add* PINGPONG_NONE to reset the crossbar; that
> functionality was already available thanks to a `bool enable` function
> parameter.  Instead it should explain why you think PINGPONG_NONE is
> more convenient than passing a bool that warrants this replacement.
> (Hint: I think because you don't have a hw_pp->idx available in the
>   teardown path, and/or its value is not relevant for the disable case
>   anyway.)
>
> In addition I don't see this series use PINGPONG_NONE anywhere yet: will
> that be added in the DSC 1.2 series for DP (to support hotplug)?

PINGPONG_NONE will be used to tear down DSC data path later at DP DSC 
patch series.

Current it is not used because DSI does not do tear down.

>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c  | 7 +++----
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h  | 1 -
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 3 ++-
>>   4 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1dc5dbe..d9ad334 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1839,7 +1839,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
>>   		hw_pp->ops.setup_dsc(hw_pp);
>>   
>>   	if (hw_dsc->ops.dsc_bind_pingpong_blk)
>> -		hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx);
>> +		hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, hw_pp->idx);
>>   
>>   	if (hw_pp->ops.enable_dsc)
>>   		hw_pp->ops.enable_dsc(hw_pp);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> index 4a6bbcc..3e68d47 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
>> @@ -157,7 +157,6 @@ static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc,
>>   
>>   static void dpu_hw_dsc_bind_pingpong_blk(
>>   		struct dpu_hw_dsc *hw_dsc,
>> -		bool enable,
>>   		const enum dpu_pingpong pp)
>>   {
>>   	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
>> @@ -166,13 +165,13 @@ static void dpu_hw_dsc_bind_pingpong_blk(
>>   
>>   	dsc_ctl_offset = DSC_CTL(hw_dsc->idx);
>>   
>> -	if (enable)
>> +	if (pp)
>>   		mux_cfg = (pp - PINGPONG_0) & 0x7;
>>   
>>   	DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n",
>> -			enable ? "Binding" : "Unbinding",
>> +			pp ? "Binding" : "Unbinding",
>>   			hw_dsc->idx - DSC_0,
>> -			enable ? "to" : "from",
>> +			pp ? "to" : "from",
>>   			pp - PINGPONG_0);
> PINGPONG_NONE - PINGPONG_0 = -1, so this whole debug log likely needs to
> be rewritten for the disable case as we don't know what PINGPONG it is
> being unbound from.  How about:
>
> 	if (pp)
> 		DRM_DEBUG_KMS("Binding dsc:%d to pp:%d\n",
> 				hw_dsc->idx - DSC_0,
> 				pp - PINGPONG_0);
> 	else
> 		DRM_DEBUG_KMS("Unbinding dsc:%d from any pp\n",
> 				hw_dsc->idx - DSC_0);
>
> - Marijn
>
>>   
>>   	DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> index 287ec5f..138080a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
>> @@ -44,7 +44,6 @@ struct dpu_hw_dsc_ops {
>>   				  struct drm_dsc_config *dsc);
>>   
>>   	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
>> -				  bool enable,
>>   				  enum dpu_pingpong pp);
>>   };
>>   
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> index 2d9192a..56826a9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
>> @@ -191,7 +191,8 @@ enum dpu_dsc {
>>   };
>>   
>>   enum dpu_pingpong {
>> -	PINGPONG_0 = 1,
>> +	PINGPONG_NONE,
>> +	PINGPONG_0,
>>   	PINGPONG_1,
>>   	PINGPONG_2,
>>   	PINGPONG_3,
>> -- 
>> 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 1dc5dbe..d9ad334 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1839,7 +1839,7 @@  static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc,
 		hw_pp->ops.setup_dsc(hw_pp);
 
 	if (hw_dsc->ops.dsc_bind_pingpong_blk)
-		hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, true, hw_pp->idx);
+		hw_dsc->ops.dsc_bind_pingpong_blk(hw_dsc, hw_pp->idx);
 
 	if (hw_pp->ops.enable_dsc)
 		hw_pp->ops.enable_dsc(hw_pp);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index 4a6bbcc..3e68d47 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -157,7 +157,6 @@  static void dpu_hw_dsc_config_thresh(struct dpu_hw_dsc *hw_dsc,
 
 static void dpu_hw_dsc_bind_pingpong_blk(
 		struct dpu_hw_dsc *hw_dsc,
-		bool enable,
 		const enum dpu_pingpong pp)
 {
 	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
@@ -166,13 +165,13 @@  static void dpu_hw_dsc_bind_pingpong_blk(
 
 	dsc_ctl_offset = DSC_CTL(hw_dsc->idx);
 
-	if (enable)
+	if (pp)
 		mux_cfg = (pp - PINGPONG_0) & 0x7;
 
 	DRM_DEBUG_KMS("%s dsc:%d %s pp:%d\n",
-			enable ? "Binding" : "Unbinding",
+			pp ? "Binding" : "Unbinding",
 			hw_dsc->idx - DSC_0,
-			enable ? "to" : "from",
+			pp ? "to" : "from",
 			pp - PINGPONG_0);
 
 	DPU_REG_WRITE(c, dsc_ctl_offset, mux_cfg);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
index 287ec5f..138080a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h
@@ -44,7 +44,6 @@  struct dpu_hw_dsc_ops {
 				  struct drm_dsc_config *dsc);
 
 	void (*dsc_bind_pingpong_blk)(struct dpu_hw_dsc *hw_dsc,
-				  bool enable,
 				  enum dpu_pingpong pp);
 };
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
index 2d9192a..56826a9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h
@@ -191,7 +191,8 @@  enum dpu_dsc {
 };
 
 enum dpu_pingpong {
-	PINGPONG_0 = 1,
+	PINGPONG_NONE,
+	PINGPONG_0,
 	PINGPONG_1,
 	PINGPONG_2,
 	PINGPONG_3,