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

[Abhinav Kumar]

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 at linaro.org>
> ---
>   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.

> +	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