[RFC PATCH] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
Jessica Zhang
quic_jesszhan at quicinc.com
Wed Feb 8 23:24:21 UTC 2023
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 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 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
>
More information about the dri-devel
mailing list