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

[Jessica Zhang]

On 2/9/2023 10:51 AM, Dmitry Baryshkov wrote:
> On 09/02/2023 20:44, 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 at 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 cb05036f2916..561406d92a1a 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
>> @@ -583,39 +583,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)
>>   {
>> @@ -641,8 +608,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);
>> @@ -700,6 +666,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));
>> +}
> 
> Quoting v1:
> 
> Could you please move the function back to the place, so that we can see 
> the actual difference?

Hi Dmitry,

Sorry if I missed your reply to my reply in V1, but as stated in the V1 
patch: the reason the diff looks like this is because 
prepare_for_kickoff() is defined above where the prepare_commit() and 
is_ongoing_pptx() were originally defined. So I had to move both 
function definitions to above the prepare_for_kickoff() function for the 
patch to compile.

That being said, I'm open to any suggestions for making this patch more 
legible.

> 
>> +
>> +static void dpu_encoder_phys_cmd_prepare_commit(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +}
> 
> This is not necessary and can be dropped. There is a safety check in 
> dpu_encoder_prepare_commit().

Acked.

Thanks,

Jessica Zhang

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