Re: [PATCH v7 13/15] drm/msm/dpu: support SSPP assignment for quad-pipe case

[Jessica Zhang <quic_jesszhan@quicinc.com>]

On 2/26/2025 6:28 PM, Jun Nie wrote:
> Jessica Zhang <quic_jesszhan@quicinc.com> 于2025年2月27日周四 02:10写道：
>>
>>
>>
>> On 2/26/2025 4:31 AM, Jun Nie wrote:
>>> Currently, SSPPs are assigned to a maximum of two pipes. However,
>>> quad-pipe usage scenarios require four pipes and involve configuring
>>> two stages. In quad-pipe case, the first two pipes share a set of
>>> mixer configurations and enable multi-rect mode when certain
>>> conditions are met. The same applies to the subsequent two pipes.
>>>
>>> Assign SSPPs to the pipes in each stage using a unified method and
>>> to loop the stages accordingly.
>>>
>>> Signed-off-by: Jun Nie <jun.nie@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 11 +++++
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h  |  2 +
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 71 ++++++++++++++++++++-----------
>>>    3 files changed, 58 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 0a053c5888262d863a1e549e14e3aa40a80c3f06..9405453cbf5d852e72a5f954cd8c6aed3a222723 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -1366,6 +1366,17 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
>>>        return 0;
>>>    }
>>>
>>> +/**
>>> + * dpu_crtc_get_num_lm - Get mixer number in this CRTC pipeline
>>> + * @state: Pointer to drm crtc state object
>>> + */
>>> +unsigned int dpu_crtc_get_num_lm(const struct drm_crtc_state *state)
>>> +{
>>> +     struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);
>>> +
>>> +     return cstate->num_mixers;
>>> +}
>>> +
>>>    #ifdef CONFIG_DEBUG_FS
>>>    static int _dpu_debugfs_status_show(struct seq_file *s, void *data)
>>>    {
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> index 0b148f3ce0d7af80ec4ffcd31d8632a5815b16f1..b14bab2754635953da402d09e11a43b9b4cf4153 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> @@ -264,4 +264,6 @@ static inline enum dpu_crtc_client_type dpu_crtc_get_client_type(
>>>
>>>    void dpu_crtc_frame_event_cb(struct drm_crtc *crtc, u32 event);
>>>
>>> +unsigned int dpu_crtc_get_num_lm(const struct drm_crtc_state *state);
>>> +
>>>    #endif /* _DPU_CRTC_H_ */
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> index d67f2ad20b4754ca4bcb759a65a39628b7236b0f..d1d6c91ed0f8e1c62b757ca42546fbc421609f72 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> @@ -1112,11 +1112,10 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc,
>>>        struct dpu_rm_sspp_requirements reqs;
>>>        struct dpu_plane_state *pstate;
>>>        struct dpu_sw_pipe *pipe;
>>> -     struct dpu_sw_pipe *r_pipe;
>>>        struct dpu_sw_pipe_cfg *pipe_cfg;
>>> -     struct dpu_sw_pipe_cfg *r_pipe_cfg;
>>> +     struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>        const struct msm_format *fmt;
>>> -     int i;
>>> +     int i, num_lm, stage_id, num_stages;
>>>
>>>        if (plane_state->crtc)
>>>                crtc_state = drm_atomic_get_new_crtc_state(state,
>>> @@ -1124,11 +1123,6 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc,
>>>
>>>        pstate = to_dpu_plane_state(plane_state);
>>>
>>> -     pipe = &pstate->pipe[0];
>>> -     r_pipe = &pstate->pipe[1];
>>> -     pipe_cfg = &pstate->pipe_cfg[0];
>>> -     r_pipe_cfg = &pstate->pipe_cfg[1];
>>> -
>>>        for (i = 0; i < PIPES_PER_PLANE; i++)
>>>                pstate->pipe[i].sspp = NULL;
>>>
>>> @@ -1142,24 +1136,49 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc,
>>>
>>>        reqs.rot90 = drm_rotation_90_or_270(plane_state->rotation);
>>>
>>> -     pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, &reqs);
>>> -     if (!pipe->sspp)
>>> -             return -ENODEV;
>>> -
>>> -     if (!dpu_plane_try_multirect_parallel(pipe, pipe_cfg, r_pipe, r_pipe_cfg,
>>> -                                           pipe->sspp,
>>> -                                           msm_framebuffer_format(plane_state->fb),
>>> -                                           dpu_kms->catalog->caps->max_linewidth)) {
>>> -             /* multirect is not possible, use two SSPP blocks */
>>> -             r_pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, &reqs);
>>> -             if (!r_pipe->sspp)
>>> -                     return -ENODEV;
>>> -
>>> -             pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>> -             pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> -
>>> -             r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>> -             r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> +     num_lm = dpu_crtc_get_num_lm(crtc_state);
>>
>> Hi Jun,
>>
>> Just wondering, why not use dpu_rm_get_assigned_resources() here instead
>> of adding a new *_get_num_lm() API?
>>
>> Thanks,
>>
>> Jessica Zhang
>>
> It is overkill to loop over allocated resource to just get the number.
> That's comments
> from Dmitry if I am not wrong.

Ah got it. I must have missed this when checking past comments.

In that case:

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Thanks,

Jessica Zhang

> 
> Regards,
> Jun