Re: [PATCH v3 23/27] drm/msm/dpu: rework dpu_plane_atomic_check()

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 07/02/2023 20:08, Abhinav Kumar wrote:
> On 2/7/2023 9:59 AM, Dmitry Baryshkov wrote:
> > On 07/02/2023 19:57, Abhinav Kumar wrote:
> > > On 2/7/2023 9:51 AM, Dmitry Baryshkov wrote:
> > > > On 07/02/2023 19:49, Abhinav Kumar wrote:
> > > > > On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> > > > > > Split pipe-dependent code from dpu_plane_atomic_check() into the
> > > > > > separate function dpu_plane_atomic_check_pipe(). This is one of
> > > > >
> > > > > Same comment as prev patch, dpu_plane_atomic_check_pipe() ---> 
> > > > > dpu_plane_atomic_check_sspp()
> > > >
> > > > No, it does what it does: it checks one software pipe configuration.
> > >
> > > The concept of software pipe is only to encapsulate the hw_sspp along 
> > > with its params
> > >
> > > +struct dpu_sw_pipe {
> > > +    struct dpu_hw_sspp *sspp;
> > > +    enum dpu_sspp_multirect_index multirect_index;
> > > +    enum dpu_sspp_multirect_mode multirect_mode;
> > > +};
> > >
> > > So its a very thin differentiation there.
> >
> > SSPP is a hardware thing, while the sw_pipe is a software instance. It 
> > can employ an SSPP or a half of SSPP. And if it employs a half of SSPP 
> > (rec1) than the name dpu_plane_atomic_check_sspp() doesn't make sense: 
> > we are not checking the SSPP itself (or its configuration). We are 
> > checking the parameters of SW pipe.
>
> Ok, I need to check in the next couple of patches how you are handling 
> the same dpu_sw_pipe for different rects of the same sspp.

rec0 and rec1 are two different instances of dpu_sw_pipe

>   struct dpu_plane_state {
>       struct drm_plane_state base;
>       struct msm_gem_address_space *aspace;
> -    struct dpu_hw_sspp *hw_sspp;
> +    struct dpu_sw_pipe pipe;
>       enum dpu_stage stage;
>       bool needs_qos_remap;
> -    uint32_t multirect_index;
>
> So far what I saw was dpu_plane_state ---> dpu_sw_pipe ---> dpu_hw_sppp
>
> In this chain, I couldnt get how the two rects of the DMA SSPP are 
> differentiated, perhaps there is something in the next few changes.
>
> > > > > > preparational steps to add r_pipe support.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > > > ---
> > > > > >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 88 
> > > > > > ++++++++++++++---------
> > > > > >   1 file changed, 53 insertions(+), 35 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
> > > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > > > index f94e132733f3..e69499490d39 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > > > > > @@ -903,6 +903,51 @@ static int 
> > > > > > dpu_plane_check_inline_rotation(struct dpu_plane *pdpu,
> > > > > >       return 0;
> > > > > >   }
> > > > > > +static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
> > > > > > +        struct dpu_sw_pipe *pipe,
> > > > > > +        struct dpu_hw_sspp_cfg *pipe_cfg,
> > > > >
> > > > > pipe_cfg --> sspp_cfg
> > > > >
> > > > > Also, had a question. For function parameters spreading across 
> > > > > multiple lines do we want to align to one guideline? Is it going to 
> > > > > be two tabs more than the prev line or aligning it with the opening 
> > > > > brace of prev line?
> > > > >
> > > > > I am seeing a mix in the prev patch of the series and this one.
> > > >
> > > > I usually tend to keep the existing indent when adding new args and 
> > > > shifting to open brace when adding new functions. Maybe I failed a 
> > > > question in this patch, I'll doublecheck it.
> > > >
> > > > > > +        const struct dpu_format *fmt)
> > > > > > +{
> > > > > > +    uint32_t min_src_size;
> > > > > > +
> > > > > > +    min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> > > > > > +
> > > > > > +    if (DPU_FORMAT_IS_YUV(fmt) &&
> > > > > > +        (!(pipe->sspp->cap->features & DPU_SSPP_SCALER) ||
> > > > > > +         !(pipe->sspp->cap->features & DPU_SSPP_CSC_ANY))) {
> > > > > > +        DPU_DEBUG_PLANE(pdpu,
> > > > > > +                "plane doesn't have scaler/csc for yuv\n");
> > > > > > +        return -EINVAL;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* check src bounds */
> > > > > > +    if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size ||
> > > > > > +           drm_rect_height(&pipe_cfg->src_rect) < min_src_size) {
> > > > > > +        DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
> > > > > > +                DRM_RECT_ARG(&pipe_cfg->src_rect));
> > > > > > +        return -E2BIG;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* valid yuv image */
> > > > > > +    if (DPU_FORMAT_IS_YUV(fmt) &&
> > > > > > +           (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 
> > > > > > & 0x1 ||
> > > > > > +            drm_rect_width(&pipe_cfg->src_rect) & 0x1 ||
> > > > > > +            drm_rect_height(&pipe_cfg->src_rect) & 0x1)) {
> > > > > > +        DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT 
> > > > > > "\n",
> > > > > > +                DRM_RECT_ARG(&pipe_cfg->src_rect));
> > > > > > +        return -EINVAL;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* min dst support */
> > > > > > +    if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || 
> > > > > > drm_rect_height(&pipe_cfg->dst_rect) < 0x1) {
> > > > > > +        DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT 
> > > > > > "\n",
> > > > > > +                DRM_RECT_ARG(&pipe_cfg->dst_rect));
> > > > > > +        return -EINVAL;
> > > > > > +    }
> > > > > > +
> > > > > > +    return 0;
> > > > > > +}
> > > > > > +
> > > > > >   static int dpu_plane_atomic_check(struct drm_plane *plane,
> > > > > >                     struct drm_atomic_state *state)
> > > > > >   {
> > > > > > @@ -915,7 +960,7 @@ static int dpu_plane_atomic_check(struct 
> > > > > > drm_plane *plane,
> > > > > >       const struct dpu_format *fmt;
> > > > > >       struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> > > > > >       struct drm_rect fb_rect = { 0 };
> > > > > > -    uint32_t min_src_size, max_linewidth;
> > > > > > +    uint32_t max_linewidth;
> > > > > >       unsigned int rotation;
> > > > > >       uint32_t supported_rotations;
> > > > > >       const struct dpu_sspp_cfg *pipe_hw_caps = 
> > > > > > pstate->pipe.sspp->cap;
> > > > > > @@ -970,46 +1015,19 @@ static int dpu_plane_atomic_check(struct 
> > > > > > drm_plane *plane,
> > > > > >       max_linewidth = pdpu->catalog->caps->max_linewidth;
> > > > > > -    fmt = 
> > > > > > to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> > > > > > -
> > > > > > -    min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> > > > > > -
> > > > > > -    if (DPU_FORMAT_IS_YUV(fmt) &&
> > > > > > -        (!(pipe_hw_caps->features & DPU_SSPP_SCALER) ||
> > > > > > -         !(pipe_hw_caps->features & DPU_SSPP_CSC_ANY))) {
> > > > > > -        DPU_DEBUG_PLANE(pdpu,
> > > > > > -                "plane doesn't have scaler/csc for yuv\n");
> > > > > > -        return -EINVAL;
> > > > > > -
> > > > > > -    /* check src bounds */
> > > > > > -    } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size ||
> > > > > > -           drm_rect_height(&pipe_cfg->src_rect) < min_src_size) {
> > > > > > -        DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
> > > > > > -                DRM_RECT_ARG(&pipe_cfg->src_rect));
> > > > > > -        return -E2BIG;
> > > > > > -
> > > > > > -    /* valid yuv image */
> > > > > > -    } else if (DPU_FORMAT_IS_YUV(fmt) &&
> > > > > > -           (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 
> > > > > > & 0x1 ||
> > > > > > -            drm_rect_width(&pipe_cfg->src_rect) & 0x1 ||
> > > > > > -            drm_rect_height(&pipe_cfg->src_rect) & 0x1)) {
> > > > > > -        DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT 
> > > > > > "\n",
> > > > > > -                DRM_RECT_ARG(&pipe_cfg->src_rect));
> > > > > > -        return -EINVAL;
> > > > > > -
> > > > > > -    /* min dst support */
> > > > > > -    } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || 
> > > > > > drm_rect_height(&pipe_cfg->dst_rect) < 0x1) {
> > > > > > -        DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT 
> > > > > > "\n",
> > > > > > -                DRM_RECT_ARG(&pipe_cfg->dst_rect));
> > > > > > -        return -EINVAL;
> > > > > > -
> > > > > >       /* check decimated source width */
> > > > > > -    } else if (drm_rect_width(&pipe_cfg->src_rect) > 
> > > > > > max_linewidth) {
> > > > > > +    if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
> > > > > >           DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " 
> > > > > > line:%u\n",
> > > > > >                   DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> > > > > >           return -E2BIG;
> > > > > >       }
> > > > > > +    fmt = 
> > > > > > to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> > > > > > +
> > > > > > +    ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, 
> > > > > > pipe_cfg, fmt);
> > > > > > +    if (ret)
> > > > > > +        return ret;
> > > > > > +
> > > > > >       supported_rotations = DRM_MODE_REFLECT_MASK | 
> > > > > > DRM_MODE_ROTATE_0;
> > > > > >       if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION))

--
With best wishes
Dmitry