Re: [PATCH v5 03/16] drm/msm/dpu: move CRTC resource assignment to dpu_encoder_virt_atomic_mode_set

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

 



On Tue, 27 Aug 2024 at 23:35, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 7/16/2024 4:06 PM, Dmitry Baryshkov wrote:
> > On Wed, 17 Jul 2024 at 01:40, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 6/24/2024 2:13 PM, Dmitry Baryshkov wrote:
> >>> Historically CRTC resources (LMs and CTLs) were assigned in
> >>> dpu_crtc_atomic_begin(). The commit 9222cdd27e82 ("drm/msm/dpu: move hw
> >>> resource tracking to crtc state") simply moved resources to
> >>> struct dpu_crtc_state, without changing the code sequence. Later on the
> >>> commit b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder
> >>> modeset") rearanged the code, but still kept the cstate->num_mixers
> >>> assignment to happen during commit phase. This makes dpu_crtc_state
> >>> inconsistent between consequent atomic_check() calls.
> >>>
> >>> Move CRTC resource assignment to happen at the end of
> >>> dpu_encoder_virt_atomic_check().
> >>>
>
> I just realized, why is the subject saying
> dpu_encoder_virt_atomic_mode_set while the patch is moving the
> assignment to dpu_encoder_virt_atomic_check(). It should say
> dpu_encoder_virt_atomic_check().

Ack

>
> One more commment below.
>
> >>> Fixes: b107603b4ad0 ("drm/msm/dpu: map mixer/ctl hw blocks in encoder modeset")
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >>> ---
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  3 --
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 60 +++++++++++++++++++----------
> >>>    2 files changed, 39 insertions(+), 24 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> index 9f2164782844..7399794d75eb 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> @@ -1094,9 +1094,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >>>        drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> >>>                dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
> >>>
> >>> -     memset(cstate->mixers, 0, sizeof(cstate->mixers));
> >>> -     cstate->num_mixers = 0;
> >>> -
> >>
> >> Any reason this part was removed?
> >>
> >> I think we still need this part.
> >>
> >> In dpu_encoder_get_topology(), we do not assign topology.num_lm based on
> >> state parameters, its based on catalog and intf_count which are decided
> >> at init time itself.
> >>
> >> Which means cstate->num_mixers will remain at a non-zero value even if
> >> we have NO_MODE which is what happens when we are disabling the CRTC
> >> during suspend or hotplug.
> >
> > In the disable path the driver calls dpu_rm_release(), releasing all
> > resources. So there will be no assigned mixers, which matches num_lm =
> > 0 in dpu_encoder_assign_crtc_resources().
> >
>
> Ack.
>
> >>
> >>>        /* disable clk & bw control until clk & bw properties are set */
> >>>        cstate->bw_control = false;
> >>>        cstate->bw_split_vote = false;
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> index 7613005fbfea..98f3a8d84300 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> @@ -628,6 +628,41 @@ static struct msm_display_topology dpu_encoder_get_topology(
> >>>        return topology;
> >>>    }
> >>>
> >>> +static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms,
> >>> +                                           struct drm_encoder *drm_enc,
> >>> +                                           struct dpu_global_state *global_state,
> >>> +                                           struct drm_crtc_state *crtc_state)
> >>> +{
> >>> +     struct dpu_crtc_state *cstate;
> >>> +     struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
> >>> +     struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
> >>> +     struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC];
> >>> +     int num_lm, num_ctl, num_dspp, i;
> >>> +
> >>> +     cstate = to_dpu_crtc_state(crtc_state);
> >>> +
> >>> +     memset(cstate->mixers, 0, sizeof(cstate->mixers));
> >>> +
> >>> +     num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> >>> +             drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
> >>> +     num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> >>> +             drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
> >>> +     num_dspp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> >>> +             drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp,
> >>> +             ARRAY_SIZE(hw_dspp));
> >>> +
> >>> +     for (i = 0; i < num_lm; i++) {
> >>> +             int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
> >>> +
> >>> +             cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]);
> >>> +             cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]);
> >>> +             if (i < num_dspp)
> >>
> >> Will there be a case where num_lm != num_dspp?
> >
> > No, _dpu_rm_check_lm_and_get_connected_blks() makes sure that num_lm = num_dspp.
> > I will drop this condition and add a comment.
> >
> >>
> >>> +                     cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]);
> >>> +     }
> >>> +
> >>> +     cstate->num_mixers = num_lm;
> >>> +}
> >>> +
> >>>    static int dpu_encoder_virt_atomic_check(
> >>>                struct drm_encoder *drm_enc,
> >>>                struct drm_crtc_state *crtc_state,
> >>> @@ -698,6 +733,9 @@ static int dpu_encoder_virt_atomic_check(
> >>>                                        drm_enc, crtc_state, topology);
> >>>        }
> >>>
> >>> +     if (!ret)
> >>> +             dpu_encoder_assign_crtc_resources(dpu_kms, drm_enc, global_state, crtc_state);
> >>> +
>
> Do we need to protect this with a needs_modeset() condition? Like why
> should this get called every check?

Sounds like a good idea.

>
> >>>        trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> >>>
> >>>        return ret;
> >>> @@ -1097,14 +1135,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>>        struct dpu_encoder_virt *dpu_enc;
> >>>        struct msm_drm_private *priv;
> >>>        struct dpu_kms *dpu_kms;
> >>> -     struct dpu_crtc_state *cstate;
> >>>        struct dpu_global_state *global_state;
> >>>        struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
> >>>        struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC];
> >>> -     struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
> >>> -     struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
> >>>        struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> >>> -     int num_lm, num_ctl, num_pp, num_dsc;
> >>> +     int num_ctl, num_pp, num_dsc;
> >>>        unsigned int dsc_mask = 0;
> >>>        int i;
> >>>
> >>> @@ -1133,11 +1168,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>>                ARRAY_SIZE(hw_pp));
> >>>        num_ctl = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> >>>                drm_enc->base.id, DPU_HW_BLK_CTL, hw_ctl, ARRAY_SIZE(hw_ctl));
> >>> -     num_lm = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> >>> -             drm_enc->base.id, DPU_HW_BLK_LM, hw_lm, ARRAY_SIZE(hw_lm));
> >>> -     dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
> >>> -             drm_enc->base.id, DPU_HW_BLK_DSPP, hw_dspp,
> >>> -             ARRAY_SIZE(hw_dspp));
> >>>
> >>>        for (i = 0; i < MAX_CHANNELS_PER_ENC; i++)
> >>>                dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
> >>> @@ -1163,18 +1193,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>>                dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
> >>>        }
> >>>
> >>> -     cstate = to_dpu_crtc_state(crtc_state);
> >>> -
> >>> -     for (i = 0; i < num_lm; i++) {
> >>> -             int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
> >>> -
> >>> -             cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]);
> >>> -             cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]);
> >>> -             cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]);
> >>> -     }
> >>> -
> >>> -     cstate->num_mixers = num_lm;
> >>> -
> >>>        dpu_enc->connector = conn_state->connector;
> >>>
> >>>        for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux