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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 10 Feb 2023 at 00:31, Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:
>
>
>
> 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@xxxxxxxxxxx>
> >> ---
> >>   .../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.

You can add a function prototype in front of
dpu_encoder_phys_cmd_prepare_for_kickoff().

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



-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux