Re: [PATCH v3 15/27] drm/msm/dpu: move the rest of plane checks to dpu_plane_atomic_check()

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 04/02/2023 00:44, Abhinav Kumar wrote:
> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> > Move plane state updates from dpu_crtc_atomic_check() to the function
> > where they belong: to dpu_plane_atomic_check().
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 18 +-----------------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 18 ++++++++++--------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |  6 ------
> >   3 files changed, 11 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index b485234eefb2..bd09bb319a58 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1129,7 +1129,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc 
> > *crtc,
> >                                         crtc);
> >       struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> >       struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
> > -    struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> >       const struct drm_plane_state *pstate;
> >       struct drm_plane *plane;
> > @@ -1161,11 +1160,10 @@ static int dpu_crtc_atomic_check(struct 
> > drm_crtc *crtc,
> >       crtc_rect.x2 = mode->hdisplay;
> >       crtc_rect.y2 = mode->vdisplay;
> > -     /* get plane state for all drm planes associated with crtc state */
> > +    /* FIXME: move this to dpu_plane_atomic_check? */
> >       drm_atomic_crtc_state_for_each_plane_state(plane, pstate, 
> > crtc_state) {
> >           struct dpu_plane_state *dpu_pstate = 
> > to_dpu_plane_state(pstate);
> >           struct drm_rect dst, clip = crtc_rect;
> > -        int stage;
> >           if (IS_ERR_OR_NULL(pstate)) {
> >               rc = PTR_ERR(pstate);
> > @@ -1179,8 +1177,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc 
> > *crtc,
> >           dpu_pstate->needs_dirtyfb = needs_dirtyfb;
> > -        dpu_plane_clear_multirect(pstate);
> > -
> >           dst = drm_plane_state_dest(pstate);
> >           if (!drm_rect_intersect(&clip, &dst)) {
> >               DPU_ERROR("invalid vertical/horizontal destination\n");
> > @@ -1189,18 +1185,6 @@ static int dpu_crtc_atomic_check(struct 
> > drm_crtc *crtc,
> >                     DRM_RECT_ARG(&dst));
> >               return -E2BIG;
> >           }
> > -
> > -        /* verify stage setting before using it */
> > -        stage = DPU_STAGE_0 + pstate->normalized_zpos;
> > -        if (stage >= dpu_kms->catalog->caps->max_mixer_blendstages) {
> > -            DPU_ERROR("> %d plane stages assigned\n",
> > -                    dpu_kms->catalog->caps->max_mixer_blendstages - 
> > DPU_STAGE_0);
> > -            return -EINVAL;
> > -        }
> > -
> > -        to_dpu_plane_state(pstate)->stage = stage;
> > -        DRM_DEBUG_ATOMIC("%s: stage %d\n", dpu_crtc->name, stage);
> > -
> >       }
> >       atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 1b3033b15bfa..5aabf9694a53 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -733,14 +733,6 @@ static int _dpu_plane_color_fill(struct dpu_plane 
> > *pdpu,
> >       return 0;
> >   }
> > -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state)
> > -{
> > -    struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
> > -
> > -    pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> > -    pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > -}
> > -
> >   int dpu_plane_validate_multirect_v2(struct 
> > dpu_multirect_plane_states *plane)
> >   {
> >       struct dpu_plane_state *pstate[R_MAX];
> > @@ -994,6 +986,16 @@ static int dpu_plane_atomic_check(struct 
> > drm_plane *plane,
> >       if (!new_plane_state->visible)
> >           return 0;
> > +    pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> > +    pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +
>
> But I am not sure if clearing the multirect belongs here and now I want 
> to clarify one thing about 
> https://patchwork.freedesktop.org/patch/521354/?series=99909&rev=4 which 
> was R-bed in the v1 and carried fwd since then.
>
> So prior to that change, we were only clearing the multirects of the 
> planes that were staged to the crtc and we were getting those from the 
> crtc state. But now we are clearing the multirect of all the planes.
>
> Dont we have to keep that in the crtc_atomic_check() since we do that on 
> all the planes attached to a certain CRTC.
>
> In that case shouldnt we keep this in the crtc_atomic_check() and bring 
> back pipe_staged[] without the multirect and source split cases ofcourse.

What for? In other words, what would be the difference?

> > +    pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
> > +    if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
> > +        DPU_ERROR("> %d plane stages assigned\n",
> > +                pdpu->catalog->caps->max_mixer_blendstages - 
> > DPU_STAGE_0);
> > +        return -EINVAL;
> > +    }
> > +
>
> I agree that this check belongs to the plane_atomic_check().
>
> >       src.x1 = new_plane_state->src_x >> 16;
> >       src.y1 = new_plane_state->src_y >> 16;
> >       src.x2 = src.x1 + (new_plane_state->src_w >> 16);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h 
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > index 228db401e905..a08b0539513b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > @@ -88,12 +88,6 @@ struct drm_plane *dpu_plane_init(struct drm_device 
> > *dev,
> >    */
> >   int dpu_plane_validate_multirect_v2(struct 
> > dpu_multirect_plane_states *plane);
> > -/**
> > - * dpu_plane_clear_multirect - clear multirect bits for the given pipe
> > - * @drm_state: Pointer to DRM plane state
> > - */
> > -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state);
> > -
> >   /**
> >    * dpu_plane_color_fill - enables color fill on plane
> >    * @plane:  Pointer to DRM plane object

--
With best wishes
Dmitry