[RFC PATCH] drm/msm/dpu: Move TE setup to prepare_for_kickoff()

[RFC PATCH] drm/msm/dpu: Move TE setup to prepare_for_kickoff()

[Jessica Zhang]

Currently, DPU will enable TE during prepare_commit(). However, this
will cause issues when trying to read/write to register in
get_autorefresh_config(), because the core clock rates aren't set at
that time.

This used to work because phys_enc->hw_pp is only initialized in mode
set [1], so the first prepare_commit() will return before any register
read/write as hw_pp would be NULL.

However, when we try to implement support for INTF TE, we will run into
the clock issue described above as hw_intf will *not* be NULL on the
first prepare_commit(). This is because the initialization of
dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].

To avoid this issue, let's enable TE during prepare_for_kickoff()
instead as the core clock rates are guaranteed to be set then.

Depends on: "Implement tearcheck support on INTF block" [3]

[1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
[2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
[3] https://patchwork.freedesktop.org/series/112332/

Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 78 ++++++++++---------
 1 file changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 279a0b7015ce..746250bce3d1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -587,39 +587,6 @@ static void dpu_encoder_phys_cmd_destroy(struct dpu_encoder_phys *phys_enc)
 	kfree(cmd_enc);
 }
 
-static void dpu_encoder_phys_cmd_prepare_for_kickoff(
-		struct dpu_encoder_phys *phys_enc)
-{
-	struct dpu_encoder_phys_cmd *cmd_enc =
-			to_dpu_encoder_phys_cmd(phys_enc);
-	int ret;
-
-	if (!phys_enc->hw_pp) {
-		DPU_ERROR("invalid encoder\n");
-		return;
-	}
-	DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", DRMID(phys_enc->parent),
-		      phys_enc->hw_pp->idx - PINGPONG_0,
-		      atomic_read(&phys_enc->pending_kickoff_cnt));
-
-	/*
-	 * Mark kickoff request as outstanding. If there are more than one,
-	 * outstanding, then we have to wait for the previous one to complete
-	 */
-	ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
-	if (ret) {
-		/* force pending_kickoff_cnt 0 to discard failed kickoff */
-		atomic_set(&phys_enc->pending_kickoff_cnt, 0);
-		DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
-			  DRMID(phys_enc->parent), ret,
-			  phys_enc->hw_pp->idx - PINGPONG_0);
-	}
-
-	DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
-			phys_enc->hw_pp->idx - PINGPONG_0,
-			atomic_read(&phys_enc->pending_kickoff_cnt));
-}
-
 static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -645,8 +612,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
 	return false;
 }
 
-static void dpu_encoder_phys_cmd_prepare_commit(
-		struct dpu_encoder_phys *phys_enc)
+static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
 {
 	struct dpu_encoder_phys_cmd *cmd_enc =
 		to_dpu_encoder_phys_cmd(phys_enc);
@@ -704,6 +670,48 @@ static void dpu_encoder_phys_cmd_prepare_commit(
 			 "disabled autorefresh\n");
 }
 
+static void dpu_encoder_phys_cmd_prepare_for_kickoff(
+		struct dpu_encoder_phys *phys_enc)
+{
+	struct dpu_encoder_phys_cmd *cmd_enc =
+			to_dpu_encoder_phys_cmd(phys_enc);
+	int ret;
+
+	if (!phys_enc->hw_pp) {
+		DPU_ERROR("invalid encoder\n");
+		return;
+	}
+
+
+	DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", DRMID(phys_enc->parent),
+		      phys_enc->hw_pp->idx - PINGPONG_0,
+		      atomic_read(&phys_enc->pending_kickoff_cnt));
+
+	/*
+	 * Mark kickoff request as outstanding. If there are more than one,
+	 * outstanding, then we have to wait for the previous one to complete
+	 */
+	ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
+	if (ret) {
+		/* force pending_kickoff_cnt 0 to discard failed kickoff */
+		atomic_set(&phys_enc->pending_kickoff_cnt, 0);
+		DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
+			  DRMID(phys_enc->parent), ret,
+			  phys_enc->hw_pp->idx - PINGPONG_0);
+	}
+
+	dpu_encoder_phys_cmd_enable_te(phys_enc);
+
+	DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
+			phys_enc->hw_pp->idx - PINGPONG_0,
+			atomic_read(&phys_enc->pending_kickoff_cnt));
+}
+
+static void dpu_encoder_phys_cmd_prepare_commit(
+		struct dpu_encoder_phys *phys_enc)
+{
+}
+
 static int _dpu_encoder_phys_cmd_wait_for_ctl_start(
 		struct dpu_encoder_phys *phys_enc)
 {
-- 
2.39.1

Re: [RFC PATCH] drm/msm/dpu: Move TE setup to prepare_for_kickoff()

[Dmitry Baryshkov]

On 08/02/2023 23:37, Jessica Zhang wrote:
> Currently, DPU will enable TE during prepare_commit(). However, this
> will cause issues when trying to read/write to register in
> get_autorefresh_config(), because the core clock rates aren't set at
> that time.
> 
> This used to work because phys_enc->hw_pp is only initialized in mode
> set [1], so the first prepare_commit() will return before any register
> read/write as hw_pp would be NULL.
> 
> However, when we try to implement support for INTF TE, we will run into
> the clock issue described above as hw_intf will *not* be NULL on the
> first prepare_commit(). This is because the initialization of
> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
> 
> To avoid this issue, let's enable TE during prepare_for_kickoff()
> instead as the core clock rates are guaranteed to be set then.
> 
> Depends on: "Implement tearcheck support on INTF block" [3]
> 
> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
> [3] https://patchwork.freedesktop.org/series/112332/
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
> ---
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 78 ++++++++++---------
>   1 file changed, 43 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 279a0b7015ce..746250bce3d1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -587,39 +587,6 @@ static void dpu_encoder_phys_cmd_destroy(struct dpu_encoder_phys *phys_enc)
>   	kfree(cmd_enc);
>   }
>   
> -static void dpu_encoder_phys_cmd_prepare_for_kickoff(
> -		struct dpu_encoder_phys *phys_enc)
> -{
> -	struct dpu_encoder_phys_cmd *cmd_enc =
> -			to_dpu_encoder_phys_cmd(phys_enc);
> -	int ret;
> -
> -	if (!phys_enc->hw_pp) {
> -		DPU_ERROR("invalid encoder\n");
> -		return;
> -	}
> -	DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", DRMID(phys_enc->parent),
> -		      phys_enc->hw_pp->idx - PINGPONG_0,
> -		      atomic_read(&phys_enc->pending_kickoff_cnt));
> -
> -	/*
> -	 * Mark kickoff request as outstanding. If there are more than one,
> -	 * outstanding, then we have to wait for the previous one to complete
> -	 */
> -	ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
> -	if (ret) {
> -		/* force pending_kickoff_cnt 0 to discard failed kickoff */
> -		atomic_set(&phys_enc->pending_kickoff_cnt, 0);
> -		DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
> -			  DRMID(phys_enc->parent), ret,
> -			  phys_enc->hw_pp->idx - PINGPONG_0);
> -	}
> -
> -	DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
> -			phys_enc->hw_pp->idx - PINGPONG_0,
> -			atomic_read(&phys_enc->pending_kickoff_cnt));
> -}
> -
>   static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -645,8 +612,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>   	return false;
>   }
>   
> -static void dpu_encoder_phys_cmd_prepare_commit(
> -		struct dpu_encoder_phys *phys_enc)
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
>   {
>   	struct dpu_encoder_phys_cmd *cmd_enc =
>   		to_dpu_encoder_phys_cmd(phys_enc);
> @@ -704,6 +670,48 @@ static void dpu_encoder_phys_cmd_prepare_commit(
>   			 "disabled autorefresh\n");
>   }
>   
> +static void dpu_encoder_phys_cmd_prepare_for_kickoff(
> +		struct dpu_encoder_phys *phys_enc)

Could you please move the function back to the place, so that we can see 
the actual difference?

> +{
> +	struct dpu_encoder_phys_cmd *cmd_enc =
> +			to_dpu_encoder_phys_cmd(phys_enc);
> +	int ret;
> +
> +	if (!phys_enc->hw_pp) {
> +		DPU_ERROR("invalid encoder\n");
> +		return;
> +	}
> +
> +
> +	DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", DRMID(phys_enc->parent),
> +		      phys_enc->hw_pp->idx - PINGPONG_0,
> +		      atomic_read(&phys_enc->pending_kickoff_cnt));
> +
> +	/*
> +	 * Mark kickoff request as outstanding. If there are more than one,
> +	 * outstanding, then we have to wait for the previous one to complete
> +	 */
> +	ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
> +	if (ret) {
> +		/* force pending_kickoff_cnt 0 to discard failed kickoff */
> +		atomic_set(&phys_enc->pending_kickoff_cnt, 0);
> +		DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
> +			  DRMID(phys_enc->parent), ret,
> +			  phys_enc->hw_pp->idx - PINGPONG_0);
> +	}
> +
> +	dpu_encoder_phys_cmd_enable_te(phys_enc);
> +
> +	DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
> +			phys_enc->hw_pp->idx - PINGPONG_0,
> +			atomic_read(&phys_enc->pending_kickoff_cnt));
> +}
> +
> +static void dpu_encoder_phys_cmd_prepare_commit(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +}

There is no need to have the empty callback, you can skip it completely. 
Actually, if it is not needed anymore for the cmd encoders, you can drop 
the .prepare_commit from struct dpu_encoder_phys_ops. And then, by 
extension, dpu_encoder_prepare_commit(), dpu_kms_prepare_commit(). This 
sounds like a nice second patch for this rfc.

> +
>   static int _dpu_encoder_phys_cmd_wait_for_ctl_start(
>   		struct dpu_encoder_phys *phys_enc)
>   {

-- 
With best wishes
Dmitry

Re: [RFC PATCH] drm/msm/dpu: Move TE setup to prepare_for_kickoff()

[Jessica Zhang]

On 2/8/2023 2:18 PM, Dmitry Baryshkov wrote:
> On 08/02/2023 23:37, Jessica Zhang wrote:
>> Currently, DPU will enable TE during prepare_commit(). However, this
>> will cause issues when trying to read/write to register in
>> get_autorefresh_config(), because the core clock rates aren't set at
>> that time.
>>
>> This used to work because phys_enc->hw_pp is only initialized in mode
>> set [1], so the first prepare_commit() will return before any register
>> read/write as hw_pp would be NULL.
>>
>> However, when we try to implement support for INTF TE, we will run into
>> the clock issue described above as hw_intf will *not* be NULL on the
>> first prepare_commit(). This is because the initialization of
>> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>>
>> To avoid this issue, let's enable TE during prepare_for_kickoff()
>> instead as the core clock rates are guaranteed to be set then.
>>
>> Depends on: "Implement tearcheck support on INTF block" [3]
>>
>> [1] 
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
>> [2] 
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
>> [3] https://patchwork.freedesktop.org/series/112332/
>>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>> ---
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 78 ++++++++++---------
>>   1 file changed, 43 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index 279a0b7015ce..746250bce3d1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -587,39 +587,6 @@ static void dpu_encoder_phys_cmd_destroy(struct 
>> dpu_encoder_phys *phys_enc)
>>       kfree(cmd_enc);
>>   }
>> -static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>> -        struct dpu_encoder_phys *phys_enc)
>> -{
>> -    struct dpu_encoder_phys_cmd *cmd_enc =
>> -            to_dpu_encoder_phys_cmd(phys_enc);
>> -    int ret;
>> -
>> -    if (!phys_enc->hw_pp) {
>> -        DPU_ERROR("invalid encoder\n");
>> -        return;
>> -    }
>> -    DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", 
>> DRMID(phys_enc->parent),
>> -              phys_enc->hw_pp->idx - PINGPONG_0,
>> -              atomic_read(&phys_enc->pending_kickoff_cnt));
>> -
>> -    /*
>> -     * Mark kickoff request as outstanding. If there are more than one,
>> -     * outstanding, then we have to wait for the previous one to 
>> complete
>> -     */
>> -    ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
>> -    if (ret) {
>> -        /* force pending_kickoff_cnt 0 to discard failed kickoff */
>> -        atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>> -        DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
>> -              DRMID(phys_enc->parent), ret,
>> -              phys_enc->hw_pp->idx - PINGPONG_0);
>> -    }
>> -
>> -    DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
>> -            phys_enc->hw_pp->idx - PINGPONG_0,
>> -            atomic_read(&phys_enc->pending_kickoff_cnt));
>> -}
>> -
>>   static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -645,8 +612,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>>       return false;
>>   }
>> -static void dpu_encoder_phys_cmd_prepare_commit(
>> -        struct dpu_encoder_phys *phys_enc)
>> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys 
>> *phys_enc)
>>   {
>>       struct dpu_encoder_phys_cmd *cmd_enc =
>>           to_dpu_encoder_phys_cmd(phys_enc);
>> @@ -704,6 +670,48 @@ static void dpu_encoder_phys_cmd_prepare_commit(
>>                "disabled autorefresh\n");
>>   }
>> +static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>> +        struct dpu_encoder_phys *phys_enc)
> 
> Could you please move the function back to the place, so that we can see 
> the actual difference?

Hi Dmitry,

This function was moved because prepare_commit() and is_ongoing_pptx() 
(which is called in prepare_commit()) were originally defined later in 
the file.

> 
>> +{
>> +    struct dpu_encoder_phys_cmd *cmd_enc =
>> +            to_dpu_encoder_phys_cmd(phys_enc);
>> +    int ret;
>> +
>> +    if (!phys_enc->hw_pp) {
>> +        DPU_ERROR("invalid encoder\n");
>> +        return;
>> +    }
>> +
>> +
>> +    DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", 
>> DRMID(phys_enc->parent),
>> +              phys_enc->hw_pp->idx - PINGPONG_0,
>> +              atomic_read(&phys_enc->pending_kickoff_cnt));
>> +
>> +    /*
>> +     * Mark kickoff request as outstanding. If there are more than one,
>> +     * outstanding, then we have to wait for the previous one to 
>> complete
>> +     */
>> +    ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
>> +    if (ret) {
>> +        /* force pending_kickoff_cnt 0 to discard failed kickoff */
>> +        atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>> +        DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
>> +              DRMID(phys_enc->parent), ret,
>> +              phys_enc->hw_pp->idx - PINGPONG_0);
>> +    }
>> +
>> +    dpu_encoder_phys_cmd_enable_te(phys_enc);
>> +
>> +    DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
>> +            phys_enc->hw_pp->idx - PINGPONG_0,
>> +            atomic_read(&phys_enc->pending_kickoff_cnt));
>> +}
>> +
>> +static void dpu_encoder_phys_cmd_prepare_commit(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +}
> 
> There is no need to have the empty callback, you can skip it completely. 
> Actually, if it is not needed anymore for the cmd encoders, you can drop 
> the .prepare_commit from struct dpu_encoder_phys_ops. And then, by 
> extension, dpu_encoder_prepare_commit(), dpu_kms_prepare_commit(). This 
> sounds like a nice second patch for this rfc.

Got it.

FWIW I kept this as an empty method to match mdp4_prepare_commit() [1], 
but I can just add a NULL check in msm_atomic_commit_tail() and remove 
both instances of empty callbacks if that's preferable.

[1] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c#L87

Thanks,

Jessica Zhang

> 
>> +
>>   static int _dpu_encoder_phys_cmd_wait_for_ctl_start(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [RFC PATCH] drm/msm/dpu: Move TE setup to prepare_for_kickoff()

[Dmitry Baryshkov]

On 09/02/2023 01:24, Jessica Zhang wrote:
> 
> 
> On 2/8/2023 2:18 PM, Dmitry Baryshkov wrote:
>> On 08/02/2023 23:37, Jessica Zhang wrote:
>>> Currently, DPU will enable TE during prepare_commit(). However, this
>>> will cause issues when trying to read/write to register in
>>> get_autorefresh_config(), because the core clock rates aren't set at
>>> that time.
>>>
>>> This used to work because phys_enc->hw_pp is only initialized in mode
>>> set [1], so the first prepare_commit() will return before any register
>>> read/write as hw_pp would be NULL.
>>>
>>> However, when we try to implement support for INTF TE, we will run into
>>> the clock issue described above as hw_intf will *not* be NULL on the
>>> first prepare_commit(). This is because the initialization of
>>> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
>>>
>>> To avoid this issue, let's enable TE during prepare_for_kickoff()
>>> instead as the core clock rates are guaranteed to be set then.
>>>
>>> Depends on: "Implement tearcheck support on INTF block" [3]
>>>
>>> [1] 
>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
>>> [2] 
>>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
>>> [3] https://patchwork.freedesktop.org/series/112332/
>>>
>>> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> ---
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 78 ++++++++++---------
>>>   1 file changed, 43 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> index 279a0b7015ce..746250bce3d1 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>> @@ -587,39 +587,6 @@ static void dpu_encoder_phys_cmd_destroy(struct 
>>> dpu_encoder_phys *phys_enc)
>>>       kfree(cmd_enc);
>>>   }
>>> -static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>>> -        struct dpu_encoder_phys *phys_enc)
>>> -{
>>> -    struct dpu_encoder_phys_cmd *cmd_enc =
>>> -            to_dpu_encoder_phys_cmd(phys_enc);
>>> -    int ret;
>>> -
>>> -    if (!phys_enc->hw_pp) {
>>> -        DPU_ERROR("invalid encoder\n");
>>> -        return;
>>> -    }
>>> -    DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", 
>>> DRMID(phys_enc->parent),
>>> -              phys_enc->hw_pp->idx - PINGPONG_0,
>>> -              atomic_read(&phys_enc->pending_kickoff_cnt));
>>> -
>>> -    /*
>>> -     * Mark kickoff request as outstanding. If there are more than one,
>>> -     * outstanding, then we have to wait for the previous one to 
>>> complete
>>> -     */
>>> -    ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
>>> -    if (ret) {
>>> -        /* force pending_kickoff_cnt 0 to discard failed kickoff */
>>> -        atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>>> -        DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
>>> -              DRMID(phys_enc->parent), ret,
>>> -              phys_enc->hw_pp->idx - PINGPONG_0);
>>> -    }
>>> -
>>> -    DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
>>> -            phys_enc->hw_pp->idx - PINGPONG_0,
>>> -            atomic_read(&phys_enc->pending_kickoff_cnt));
>>> -}
>>> -
>>>   static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>>>           struct dpu_encoder_phys *phys_enc)
>>>   {
>>> @@ -645,8 +612,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>>>       return false;
>>>   }
>>> -static void dpu_encoder_phys_cmd_prepare_commit(
>>> -        struct dpu_encoder_phys *phys_enc)
>>> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys 
>>> *phys_enc)
>>>   {
>>>       struct dpu_encoder_phys_cmd *cmd_enc =
>>>           to_dpu_encoder_phys_cmd(phys_enc);
>>> @@ -704,6 +670,48 @@ static void dpu_encoder_phys_cmd_prepare_commit(
>>>                "disabled autorefresh\n");
>>>   }
>>> +static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>>> +        struct dpu_encoder_phys *phys_enc)
>>
>> Could you please move the function back to the place, so that we can 
>> see the actual difference?
> 
> Hi Dmitry,
> 
> This function was moved because prepare_commit() and is_ongoing_pptx() 
> (which is called in prepare_commit()) were originally defined later in 
> the file.
> 
>>
>>> +{
>>> +    struct dpu_encoder_phys_cmd *cmd_enc =
>>> +            to_dpu_encoder_phys_cmd(phys_enc);
>>> +    int ret;
>>> +
>>> +    if (!phys_enc->hw_pp) {
>>> +        DPU_ERROR("invalid encoder\n");
>>> +        return;
>>> +    }
>>> +
>>> +
>>> +    DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", 
>>> DRMID(phys_enc->parent),
>>> +              phys_enc->hw_pp->idx - PINGPONG_0,
>>> +              atomic_read(&phys_enc->pending_kickoff_cnt));
>>> +
>>> +    /*
>>> +     * Mark kickoff request as outstanding. If there are more than one,
>>> +     * outstanding, then we have to wait for the previous one to 
>>> complete
>>> +     */
>>> +    ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc);
>>> +    if (ret) {
>>> +        /* force pending_kickoff_cnt 0 to discard failed kickoff */
>>> +        atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>>> +        DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n",
>>> +              DRMID(phys_enc->parent), ret,
>>> +              phys_enc->hw_pp->idx - PINGPONG_0);
>>> +    }
>>> +
>>> +    dpu_encoder_phys_cmd_enable_te(phys_enc);
>>> +
>>> +    DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
>>> +            phys_enc->hw_pp->idx - PINGPONG_0,
>>> +            atomic_read(&phys_enc->pending_kickoff_cnt));
>>> +}
>>> +
>>> +static void dpu_encoder_phys_cmd_prepare_commit(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +}
>>
>> There is no need to have the empty callback, you can skip it 
>> completely. Actually, if it is not needed anymore for the cmd 
>> encoders, you can drop the .prepare_commit from struct 
>> dpu_encoder_phys_ops. And then, by extension, 
>> dpu_encoder_prepare_commit(), dpu_kms_prepare_commit(). This sounds 
>> like a nice second patch for this rfc.
> 
> Got it.
> 
> FWIW I kept this as an empty method to match mdp4_prepare_commit() [1], 
> but I can just add a NULL check in msm_atomic_commit_tail() and remove 
> both instances of empty callbacks if that's preferable.

yes, please.

> 
> [1] 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c#L87


-- 
With best wishes
Dmitry