Re: [PATCH v3 22/27] drm/msm/dpu: rework dpu_plane_sspp_atomic_update()

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 07/02/2023 02:22, Abhinav Kumar wrote:
> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> > Split pipe-dependent code from dpu_plane_sspp_atomic_update() into the
>
> sspp-dependent?
>
> > separate function dpu_plane_sspp_update_pipe(). This is one of
> > preparational steps to add r_pipe support.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> Just a couple of minor comments below but otherwise this split up lgtm
>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 113 ++++++++++++----------
> >   1 file changed, 63 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 0986e740b978..f94e132733f3 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -404,12 +404,13 @@ static void _dpu_plane_set_qos_ctrl(struct 
> > drm_plane *plane,
> >    * _dpu_plane_set_ot_limit - set OT limit for the given plane
> >    * @plane:        Pointer to drm plane
> >    * @pipe:        Pointer to software pipe
> > - * @crtc:        Pointer to drm crtc
> >    * @pipe_cfg:        Pointer to pipe configuration
> > + * @frame_rate:        CRTC's frame rate
>
> Can you please check the spacing here. There seems to be an extra tab 
> before the CRTC's frame rate
>
> >    */
> >   static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
> >           struct dpu_sw_pipe *pipe,
> > -        struct drm_crtc *crtc, struct dpu_hw_sspp_cfg *pipe_cfg)
> > +        struct dpu_hw_sspp_cfg *pipe_cfg,
> > +        int frame_rate)
> >   {
> >       struct dpu_plane *pdpu = to_dpu_plane(plane);
> >       struct dpu_vbif_set_ot_params ot_params;
> > @@ -421,7 +422,7 @@ static void _dpu_plane_set_ot_limit(struct 
> > drm_plane *plane,
> >       ot_params.width = drm_rect_width(&pipe_cfg->src_rect);
> >       ot_params.height = drm_rect_height(&pipe_cfg->src_rect);
> >       ot_params.is_wfd = !pdpu->is_rt_pipe;
> > -    ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode);
> > +    ot_params.frame_rate = frame_rate;
> >       ot_params.vbif_idx = VBIF_RT;
> >       ot_params.clk_ctrl = pipe->sspp->cap->clk_ctrl;
> >       ot_params.rd = true;
> > @@ -457,26 +458,6 @@ static void _dpu_plane_set_qos_remap(struct 
> > drm_plane *plane,
> >       dpu_vbif_set_qos_remap(dpu_kms, &qos_params);
> >   }
> > -static void _dpu_plane_set_scanout(struct drm_plane *plane,
> > -        struct dpu_plane_state *pstate,
> > -        struct drm_framebuffer *fb)
> > -{
> > -    struct dpu_plane *pdpu = to_dpu_plane(plane);
> > -    struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
> > -    struct msm_gem_address_space *aspace = kms->base.aspace;
> > -    struct dpu_hw_fmt_layout layout;
> > -    int ret;
> > -
> > -    ret = dpu_format_populate_layout(aspace, fb, &layout);
> > -    if (ret)
> > -        DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
> > -    else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
> > -        trace_dpu_plane_set_scanout(&pstate->pipe,
> > -                        &layout);
> > -        pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, 
> > &layout);
> > -    }
> > -}
> > -
> >   static void _dpu_plane_setup_scaler3(struct dpu_hw_sspp *pipe_hw,
> >           uint32_t src_w, uint32_t src_h, uint32_t dst_w, uint32_t dst_h,
> >           struct dpu_hw_scaler3_cfg *scale_cfg,
> > @@ -1102,35 +1083,25 @@ void dpu_plane_set_error(struct drm_plane 
> > *plane, bool error)
> >       pdpu->is_error = error;
> >   }
> > -static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
> > +static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
> > +                       struct dpu_sw_pipe *pipe,
> > +                       struct dpu_hw_sspp_cfg *pipe_cfg,
>
> You can call this parameter sspp_cfg instead of pipe_cfg?

I think, I'll add a commit renaming dpu_hw_sspp_cfg to dpu_sw_pipe_cfg, 
it would be simmetrical to dpu_sw_pipe then.

--
With best wishes
Dmitry