[10/25] drm/msm/dpu: introduce struct dpu_sw_pipe

Message ID	20220209172520.3719906-11-dmitry.baryshkov@linaro.org
State	New
Headers	show Return-Path: <linux-arm-msm-owner@kernel.org> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Bjorn Andersson <bjorn.andersson@linaro.org>, Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>, Abhinav Kumar <quic_abhinavk@quicinc.com> Cc: Stephen Boyd <swboyd@chromium.org>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org Subject: [PATCH 10/25] drm/msm/dpu: introduce struct dpu_sw_pipe Date: Wed, 9 Feb 2022 20:25:05 +0300 Message-Id: <20220209172520.3719906-11-dmitry.baryshkov@linaro.org> In-Reply-To: <20220209172520.3719906-1-dmitry.baryshkov@linaro.org> References: <20220209172520.3719906-1-dmitry.baryshkov@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	drm/msm/dpu: wide planes support \| expand [00/25] drm/msm/dpu: wide planes support [01/25] drm/msm/dpu: rip out master planes support [02/25] drm/msm/dpu: do not limit the zpos property [03/25] drm/msm/dpu: add support for SSPP allocation to RM [04/25] drm/msm/dpu: move SSPP debugfs creation to dpu_kms.c [05/25] drm/msm/dpu: move pipe_hw to dpu_plane_state [06/25] drm/msm/dpu: inline dpu_plane_get_ctl_flush [07/25] drm/msm/dpu: drop dpu_plane_pipe function [08/25] drm/msm/dpu: get rid of cached flush_mask [09/25] drm/msm/dpu: dpu_crtc_blend_setup: split mixer and ctl logic [10/25] drm/msm/dpu: introduce struct dpu_sw_pipe [11/25] drm/msm/dpu: use dpu_sw_pipe for dpu_hw_sspp callbacks [12/25] drm/msm/dpu: inline _dpu_plane_set_scanout [13/25] drm/msm/dpu: pass dpu_format to _dpu_hw_sspp_setup_scaler3() [14/25] drm/msm/dpu: move stride programming to dpu_hw_sspp_setup_sourceaddress [15/25] drm/msm/dpu: remove dpu_hw_fmt_layout from struct dpu_hw_pipe_cfg [16/25] drm/msm/dpu: drop EAGAIN check from dpu_format_populate_layout [17/25] drm/msm/dpu: drop src_split and multirect check from dpu_crtc_atomic_check [18/25] drm/msm/dpu: move the rest of plane checks to dpu_plane_atomic_check() [19/25] drm/msm/dpu: don't use unsupported blend stages [20/25] drm/msm/dpu: add dpu_hw_pipe_cfg to dpu_plane_state [21/25] drm/msm/dpu: simplify dpu_plane_validate_src() [22/25] drm/msm/dpu: rewrite plane's QoS-related functions to take dpu_sw_pipe and dpu_format [23/25] drm/msm/dpu: rework dpu_plane_atomic_check() and dpu_plane_sspp_atomic_update() [24/25] drm/msm/dpu: populate SmartDMA features in hw catalog [25/25] drm/msm/dpu: add support for wide planes

Dmitry Baryshkov Feb. 9, 2022, 5:25 p.m. UTC

Wrap SSPP and multirect index/mode into a single structure that
represents software view on the pipe used.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |   8 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  16 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 136 ++++++++++----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |   4 +-
 4 files changed, 86 insertions(+), 78 deletions(-)

Abhinav Kumar May 6, 2022, 9:30 p.m. UTC | #1

On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
> Wrap SSPP and multirect index/mode into a single structure that
> represents software view on the pipe used.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |   8 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  16 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 136 ++++++++++----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |   4 +-
>   4 files changed, 86 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index ada7d5750536..751c64012058 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -441,14 +441,14 @@ static void _dpu_crtc_blend_setup_ctl(struct drm_crtc *crtc)
>   
>   		pstate = to_dpu_plane_state(state);
>   
> -		sspp_idx = pstate->pipe_hw->idx;
> +		sspp_idx = pstate->pipe.sspp->idx;
>   		set_bit(sspp_idx, fetch_active);
>   
>   		stage_idx = zpos_cnt[pstate->stage]++;
>   		stage_cfg.stage[pstate->stage][stage_idx] =
>   					sspp_idx;
>   		stage_cfg.multirect_index[pstate->stage][stage_idx] =
> -					pstate->multirect_index;
> +					pstate->pipe.multirect_index;
>   
>   		/* blend config update */
>   		for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++)
> @@ -1118,7 +1118,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>   		pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate);
>   		pstates[cnt].drm_pstate = pstate;
>   		pstates[cnt].stage = pstate->normalized_zpos;
> -		pstates[cnt].pipe_id = to_dpu_plane_state(pstate)->pipe_hw->idx;
> +		pstates[cnt].pipe_id = to_dpu_plane_state(pstate)->pipe.sspp->idx;
>   
>   		if (pipe_staged[pstates[cnt].pipe_id]) {
>   			multirect_plane[multirect_count].r0 =
> @@ -1389,7 +1389,7 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data)
>   			state->crtc_x, state->crtc_y, state->crtc_w,
>   			state->crtc_h);
>   		seq_printf(s, "\tmultirect: mode: %d index: %d\n",
> -			pstate->multirect_mode, pstate->multirect_index);
> +			pstate->pipe.multirect_mode, pstate->pipe.multirect_index);
>   
>   		seq_puts(s, "\n");
>   	}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> index 674f311f99b4..0af2bc6e5df8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> @@ -159,15 +159,11 @@ struct dpu_hw_pixel_ext {
>    * @src_rect:  src ROI, caller takes into account the different operations
>    *             such as decimation, flip etc to program this field
>    * @dest_rect: destination ROI.
> - * @index:     index of the rectangle of SSPP
> - * @mode:      parallel or time multiplex multirect mode
>    */
>   struct dpu_hw_pipe_cfg {
>   	struct dpu_hw_fmt_layout layout;
>   	struct drm_rect src_rect;
>   	struct drm_rect dst_rect;
> -	enum dpu_sspp_multirect_index index;
> -	enum dpu_sspp_multirect_mode mode;
>   };
>   
>   /**
> @@ -218,6 +214,18 @@ struct dpu_hw_pipe_ts_cfg {
>   	u64 time;
>   };
>   
> +/**
> + * struct dpu_sw_pipe - software pipe description
> + * @sspp:      backing SSPP pipe
> + * @index:     index of the rectangle of SSPP
> + * @mode:      parallel or time multiplex multirect mode
> + */
> +struct dpu_sw_pipe {
> +	struct dpu_hw_pipe *sspp;
> +	enum dpu_sspp_multirect_index multirect_index;
> +	enum dpu_sspp_multirect_mode multirect_mode;
> +};
> +
>   /**
>    * struct dpu_hw_sspp_ops - interface to the SSPP Hw driver functions
>    * Caller must call the init function to get the pipe context for each pipe
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index d1f9b4bc10ac..51b5e8a3182b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -247,7 +247,7 @@ static int _dpu_plane_calc_fill_level(struct drm_plane *plane,
>   				((src_width + 32) * fmt->bpp);
>   		}
>   	} else {
> -		if (pstate->multirect_mode == DPU_SSPP_MULTIRECT_PARALLEL) {
> +		if (pstate->pipe.multirect_mode == DPU_SSPP_MULTIRECT_PARALLEL) {
>   			total_fl = (fixed_buff_size / 2) * 2 /
>   				((src_width + 32) * fmt->bpp);
>   		} else {
> @@ -331,7 +331,7 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
>   			fmt ? (char *)&fmt->base.pixel_format : NULL,
>   			pdpu->is_rt_pipe, total_fl, qos_lut);
>   
> -	pstate->pipe_hw->ops.setup_creq_lut(pstate->pipe_hw, qos_lut);
> +	pstate->pipe.sspp->ops.setup_creq_lut(pstate->pipe.sspp, qos_lut);
>   }
>   
>   /**
> @@ -383,7 +383,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
>   		danger_lut,
>   		safe_lut);
>   
> -	pstate->pipe_hw->ops.setup_danger_safe_lut(pstate->pipe_hw,
> +	pstate->pipe.sspp->ops.setup_danger_safe_lut(pstate->pipe.sspp,
>   			danger_lut, safe_lut);
>   }
>   
> @@ -403,9 +403,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>   	memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>   
>   	if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
> -		pipe_qos_cfg.creq_vblank = pstate->pipe_hw->cap->sblk->creq_vblank;
> +		pipe_qos_cfg.creq_vblank = pstate->pipe.sspp->cap->sblk->creq_vblank;
>   		pipe_qos_cfg.danger_vblank =
> -				pstate->pipe_hw->cap->sblk->danger_vblank;
> +				pstate->pipe.sspp->cap->sblk->danger_vblank;
>   		pipe_qos_cfg.vblank_en = enable;
>   	}
>   
> @@ -431,7 +431,7 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>   		pipe_qos_cfg.danger_vblank,
>   		pdpu->is_rt_pipe);
>   
> -	pstate->pipe_hw->ops.setup_qos_ctrl(pstate->pipe_hw,
> +	pstate->pipe.sspp->ops.setup_qos_ctrl(pstate->pipe.sspp,
>   			&pipe_qos_cfg);
>   }
>   
> @@ -450,14 +450,14 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
>   	struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
>   
>   	memset(&ot_params, 0, sizeof(ot_params));
> -	ot_params.xin_id = pstate->pipe_hw->cap->xin_id;
> -	ot_params.num = pstate->pipe_hw->idx - SSPP_NONE;
> +	ot_params.xin_id = pstate->pipe.sspp->cap->xin_id;
> +	ot_params.num = pstate->pipe.sspp->idx - SSPP_NONE;
>   	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.vbif_idx = VBIF_RT;
> -	ot_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
> +	ot_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>   	ot_params.rd = true;
>   
>   	dpu_vbif_set_ot_limit(dpu_kms, &ot_params);
> @@ -476,9 +476,9 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
>   
>   	memset(&qos_params, 0, sizeof(qos_params));
>   	qos_params.vbif_idx = VBIF_RT;
> -	qos_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
> -	qos_params.xin_id = pstate->pipe_hw->cap->xin_id;
> -	qos_params.num = pstate->pipe_hw->idx - SSPP_VIG0;
> +	qos_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
> +	qos_params.xin_id = pstate->pipe.sspp->cap->xin_id;
> +	qos_params.num = pstate->pipe.sspp->idx - SSPP_VIG0;
>   	qos_params.is_rt = pdpu->is_rt_pipe;
>   
>   	DPU_DEBUG_PLANE(pdpu, "pipe:%d vbif:%d xin:%d rt:%d, clk_ctrl:%d\n",
> @@ -505,12 +505,12 @@ static void _dpu_plane_set_scanout(struct drm_plane *plane,
>   		DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
>   	else if (ret)
>   		DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
> -	else if (pstate->pipe_hw->ops.setup_sourceaddress) {
> -		trace_dpu_plane_set_scanout(pstate->pipe_hw->idx,
> +	else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
> +		trace_dpu_plane_set_scanout(pstate->pipe.sspp->idx,
>   					    &pipe_cfg->layout,
> -					    pstate->multirect_index);
> -		pstate->pipe_hw->ops.setup_sourceaddress(pstate->pipe_hw, pipe_cfg,
> -						pstate->multirect_index);
> +					    pstate->pipe.multirect_index);
> +		pstate->pipe.sspp->ops.setup_sourceaddress(pstate->pipe.sspp, pipe_cfg,
> +						pstate->pipe.multirect_index);
>   	}
>   }
>   
> @@ -553,7 +553,7 @@ static void _dpu_plane_setup_scaler3(struct dpu_plane *pdpu,
>   			scale_cfg->src_height[i] /= chroma_subsmpl_v;
>   		}
>   
> -		if (pstate->pipe_hw->cap->features &
> +		if (pstate->pipe.sspp->cap->features &
>   			BIT(DPU_SSPP_SCALER_QSEED4)) {
>   			scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
>   			scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
> @@ -624,7 +624,7 @@ static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_plane *pdpu, cons
>   	if (!DPU_FORMAT_IS_YUV(fmt))
>   		return NULL;
>   
> -	if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe_hw->cap->features)
> +	if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe.sspp->cap->features)
>   		csc_ptr = &dpu_csc10_YUV2RGB_601L;
>   	else
>   		csc_ptr = &dpu_csc_YUV2RGB_601L;
> @@ -659,8 +659,8 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,
>   			&scaler3_cfg, &pixel_ext, fmt,
>   			info->hsub, info->vsub);
>   
> -	if (pstate->pipe_hw->ops.setup_pe)
> -		pstate->pipe_hw->ops.setup_pe(pstate->pipe_hw,
> +	if (pstate->pipe.sspp->ops.setup_pe)
> +		pstate->pipe.sspp->ops.setup_pe(pstate->pipe.sspp,
>   				&pixel_ext);
>   
>   	/**
> @@ -668,9 +668,9 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,
>   	 * bypassed. Still we need to update alpha and bitwidth
>   	 * ONLY for RECT0
>   	 */
> -	if (pstate->pipe_hw->ops.setup_scaler &&
> -			pstate->multirect_index != DPU_SSPP_RECT_1)
> -		pstate->pipe_hw->ops.setup_scaler(pstate->pipe_hw,
> +	if (pstate->pipe.sspp->ops.setup_scaler &&
> +			pstate->pipe.multirect_index != DPU_SSPP_RECT_1)
> +		pstate->pipe.sspp->ops.setup_scaler(pstate->pipe.sspp,
>   				pipe_cfg,
>   				&scaler3_cfg);
>   }
> @@ -699,10 +699,10 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
>   	fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>   
>   	/* update sspp */
> -	if (fmt && pstate->pipe_hw->ops.setup_solidfill) {
> -		pstate->pipe_hw->ops.setup_solidfill(pstate->pipe_hw,
> +	if (fmt && pstate->pipe.sspp->ops.setup_solidfill) {
> +		pstate->pipe.sspp->ops.setup_solidfill(pstate->pipe.sspp,
>   				(color & 0xFFFFFF) | ((alpha & 0xFF) << 24),
> -				pstate->multirect_index);
> +				pstate->pipe.multirect_index);
>   
>   		/* override scaler/decimation if solid fill */
>   		pipe_cfg.dst_rect = pstate->base.dst;
> @@ -714,15 +714,15 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
>   		pipe_cfg.src_rect.y2 =
>   			drm_rect_height(&pipe_cfg.dst_rect);
>   
> -		if (pstate->pipe_hw->ops.setup_format)
> -			pstate->pipe_hw->ops.setup_format(pstate->pipe_hw,
> +		if (pstate->pipe.sspp->ops.setup_format)
> +			pstate->pipe.sspp->ops.setup_format(pstate->pipe.sspp,
>   					fmt, DPU_SSPP_SOLID_FILL,
> -					pstate->multirect_index);
> +					pstate->pipe.multirect_index);
>   
> -		if (pstate->pipe_hw->ops.setup_rects)
> -			pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
> +		if (pstate->pipe.sspp->ops.setup_rects)
> +			pstate->pipe.sspp->ops.setup_rects(pstate->pipe.sspp,
>   					&pipe_cfg,
> -					pstate->multirect_index);
> +					pstate->pipe.multirect_index);
>   
>   		_dpu_plane_setup_scaler(pdpu, pstate, fmt, true, &pipe_cfg);
>   	}
> @@ -734,8 +734,8 @@ void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state)
>   {
>   	struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>   
> -	pstate->multirect_index = DPU_SSPP_RECT_SOLO;
> -	pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> +	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)
> @@ -817,8 +817,8 @@ int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>   
>   	/* Prefer PARALLEL FETCH Mode over TIME_MX Mode */
>   	if (parallel_fetch_qualified) {
> -		pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
> -		pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
> +		pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
> +		pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>   
>   		goto done;
>   	}
> @@ -828,8 +828,8 @@ int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>   
>   	if (dst[R1].y1 >= dst[R0].y2 + buffer_lines ||
>   	    dst[R0].y1 >= dst[R1].y2 + buffer_lines) {
> -		pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
> -		pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
> +		pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
> +		pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>   	} else {
>   		DPU_ERROR(
>   			"No multirect mode possible for the planes (%d - %d)\n",
> @@ -839,13 +839,13 @@ int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>   	}
>   
>   done:
> -	pstate[R0]->multirect_index = DPU_SSPP_RECT_0;
> -	pstate[R1]->multirect_index = DPU_SSPP_RECT_1;
> +	pstate[R0]->pipe.multirect_index = DPU_SSPP_RECT_0;
> +	pstate[R1]->pipe.multirect_index = DPU_SSPP_RECT_1;
>   
>   	DPU_DEBUG_PLANE(dpu_plane[R0], "R0: %d - %d\n",
> -		pstate[R0]->multirect_mode, pstate[R0]->multirect_index);
> +		pstate[R0]->pipe.multirect_mode, pstate[R0]->pipe.multirect_index);
>   	DPU_DEBUG_PLANE(dpu_plane[R1], "R1: %d - %d\n",
> -		pstate[R1]->multirect_mode, pstate[R1]->multirect_index);
> +		pstate[R1]->pipe.multirect_mode, pstate[R1]->pipe.multirect_index);
>   	return 0;
>   }
>   
> @@ -937,6 +937,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   	int ret = 0, min_scale;
>   	struct dpu_plane *pdpu = to_dpu_plane(plane);
>   	struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state);
> +	struct dpu_sw_pipe *pipe = &pstate->pipe;
>   	const struct drm_crtc_state *crtc_state = NULL;
>   	const struct dpu_format *fmt;
>   	struct drm_rect src, dst, fb_rect = { 0 };
> @@ -946,10 +947,10 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   		crtc_state = drm_atomic_get_new_crtc_state(state,
>   							   new_plane_state->crtc);
>   
> -	min_scale = FRAC_16_16(1, pstate->pipe_hw->cap->sblk->maxupscale);
> +	min_scale = FRAC_16_16(1, pipe->sspp->cap->sblk->maxupscale);
>   	ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
>   						  min_scale,
> -						  pstate->pipe_hw->cap->sblk->maxdwnscale << 16,
> +						  pipe->sspp->cap->sblk->maxdwnscale << 16,
>   						  true, true);
>   	if (ret) {
>   		DPU_DEBUG_PLANE(pdpu, "Check plane state failed (%d)\n", ret);
> @@ -975,8 +976,8 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>   
>   	if (DPU_FORMAT_IS_YUV(fmt) &&
> -		(!(pstate->pipe_hw->cap->features & DPU_SSPP_SCALER) ||
> -		 !(pstate->pipe_hw->cap->features & DPU_SSPP_CSC_ANY))) {
> +		(!(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;
> @@ -1037,12 +1038,12 @@ void dpu_plane_flush(struct drm_plane *plane)
>   	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>   		/* force 100% alpha */
>   		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> -	else if (pstate->pipe_hw && pstate->pipe_hw->ops.setup_csc) {
> +	else if (pstate->pipe.sspp && pstate->pipe.sspp->ops.setup_csc) {
>   		const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(plane->state->fb));
>   		const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, fmt);
>   
>   		if (csc_ptr)
> -			pstate->pipe_hw->ops.setup_csc(pstate->pipe_hw, csc_ptr);
> +			pstate->pipe.sspp->ops.setup_csc(pstate->pipe.sspp, csc_ptr);
>   	}
>   
>   	/* flag h/w flush complete */
> @@ -1072,6 +1073,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   	struct dpu_plane *pdpu = to_dpu_plane(plane);
>   	struct drm_plane_state *state = plane->state;
>   	struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> +	struct dpu_sw_pipe *pipe = &pstate->pipe;
>   	struct drm_crtc *crtc = state->crtc;
>   	struct drm_framebuffer *fb = state->fb;
>   	bool is_rt_pipe, update_qos_remap;
> @@ -1109,21 +1111,21 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   		return;
>   	}
>   
> -	if (pstate->pipe_hw->ops.setup_rects) {
> -		pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
> +	if (pipe->sspp->ops.setup_rects) {
> +		pipe->sspp->ops.setup_rects(pipe->sspp,
>   				&pipe_cfg,
> -				pstate->multirect_index);
> +				pipe->multirect_index);
>   	}
>   
>   	_dpu_plane_setup_scaler(pdpu, pstate, fmt, false, &pipe_cfg);
>   
> -	if (pstate->pipe_hw->ops.setup_multirect)
> -		pstate->pipe_hw->ops.setup_multirect(
> -				pstate->pipe_hw,
> -				pstate->multirect_index,
> -				pstate->multirect_mode);
> +	if (pipe->sspp->ops.setup_multirect)
> +		pipe->sspp->ops.setup_multirect(
> +				pipe->sspp,
> +				pipe->multirect_index,
> +				pipe->multirect_mode);
>   
> -	if (pstate->pipe_hw->ops.setup_format) {
> +	if (pipe->sspp->ops.setup_format) {
>   		unsigned int rotation;
>   
>   		src_flags = 0x0;
> @@ -1140,10 +1142,10 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   			src_flags |= DPU_SSPP_FLIP_UD;
>   
>   		/* update format */
> -		pstate->pipe_hw->ops.setup_format(pstate->pipe_hw, fmt, src_flags,
> -				pstate->multirect_index);
> +		pipe->sspp->ops.setup_format(pipe->sspp, fmt, src_flags,
> +				pipe->multirect_index);
>   
> -		if (pstate->pipe_hw->ops.setup_cdp) {
> +		if (pipe->sspp->ops.setup_cdp) {
>   			struct dpu_hw_pipe_cdp_cfg cdp_cfg;
>   
>   			memset(&cdp_cfg, 0, sizeof(struct dpu_hw_pipe_cdp_cfg));
> @@ -1157,7 +1159,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   					DPU_FORMAT_IS_TILE(fmt);
>   			cdp_cfg.preload_ahead = DPU_SSPP_CDP_PRELOAD_AHEAD_64;
>   
> -			pstate->pipe_hw->ops.setup_cdp(pstate->pipe_hw, &cdp_cfg, pstate->multirect_index);
> +			pipe->sspp->ops.setup_cdp(pipe->sspp, &cdp_cfg, pipe->multirect_index);
>   		}
>   	}
>   
> @@ -1191,7 +1193,7 @@ static void _dpu_plane_atomic_disable(struct drm_plane *plane)
>   	struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>   
>   	trace_dpu_plane_disable(DRMID(plane), false,
> -				pstate->multirect_mode);
> +				pstate->pipe.multirect_mode);
>   
>   	pstate->pending = true;
>   }
> @@ -1305,9 +1307,9 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p,
>   	const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>   
>   	drm_printf(p, "\tstage=%d\n", pstate->stage);
> -	drm_printf(p, "\tsspp=%s\n", pstate->pipe_hw->cap->name);
> -	drm_printf(p, "\tmultirect_mode=%s\n", dpu_get_multirect_mode(pstate->multirect_mode));
> -	drm_printf(p, "\tmultirect_index=%s\n", dpu_get_multirect_index(pstate->multirect_index));
> +	drm_printf(p, "\tsspp=%s\n", pstate->pipe.sspp->cap->name);
> +	drm_printf(p, "\tmultirect_mode=%s\n", dpu_get_multirect_mode(pstate->pipe.multirect_mode));
> +	drm_printf(p, "\tmultirect_index=%s\n", dpu_get_multirect_index(pstate->pipe.multirect_index));
>   }
>   
>   static void dpu_plane_reset(struct drm_plane *plane)
> @@ -1336,7 +1338,7 @@ static void dpu_plane_reset(struct drm_plane *plane)
>   		return;
>   	}
>   
> -	pstate->pipe_hw = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe - SSPP_NONE]);
> +	pstate->pipe.sspp = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe - SSPP_NONE]);
>   
>   	__drm_atomic_helper_plane_reset(plane, &pstate->base);
>   }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> index d745cde4ea77..b6fd6f856d6a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> @@ -31,11 +31,9 @@ struct dpu_plane_state {
>   	struct msm_gem_address_space *aspace;
>   	enum dpu_stage stage;
>   	bool needs_qos_remap;
> -	uint32_t multirect_index;
> -	uint32_t multirect_mode;
>   	bool pending;
>   
> -	struct dpu_hw_pipe *pipe_hw;
> +	struct dpu_sw_pipe pipe;

Are you sure we dont need this to be a pointer?

Since this is now part of plane_state, its possible that across commits, 
the hw pipe can change for this plane's state. So might be better if 
this is pointer in that case ....

>   
>   	u64 plane_fetch_bw;
>   	u64 plane_clk;

Dmitry Baryshkov May 6, 2022, 9:39 p.m. UTC | #2

On 07/05/2022 00:30, Abhinav Kumar wrote:
> 
> 
> On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
>> Wrap SSPP and multirect index/mode into a single structure that
>> represents software view on the pipe used.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |   8 +-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  16 ++-
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 136 ++++++++++----------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |   4 +-
>>   4 files changed, 86 insertions(+), 78 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index ada7d5750536..751c64012058 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -441,14 +441,14 @@ static void _dpu_crtc_blend_setup_ctl(struct 
>> drm_crtc *crtc)
>>           pstate = to_dpu_plane_state(state);
>> -        sspp_idx = pstate->pipe_hw->idx;
>> +        sspp_idx = pstate->pipe.sspp->idx;
>>           set_bit(sspp_idx, fetch_active);
>>           stage_idx = zpos_cnt[pstate->stage]++;
>>           stage_cfg.stage[pstate->stage][stage_idx] =
>>                       sspp_idx;
>>           stage_cfg.multirect_index[pstate->stage][stage_idx] =
>> -                    pstate->multirect_index;
>> +                    pstate->pipe.multirect_index;
>>           /* blend config update */
>>           for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++)
>> @@ -1118,7 +1118,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc 
>> *crtc,
>>           pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate);
>>           pstates[cnt].drm_pstate = pstate;
>>           pstates[cnt].stage = pstate->normalized_zpos;
>> -        pstates[cnt].pipe_id = to_dpu_plane_state(pstate)->pipe_hw->idx;
>> +        pstates[cnt].pipe_id = 
>> to_dpu_plane_state(pstate)->pipe.sspp->idx;
>>           if (pipe_staged[pstates[cnt].pipe_id]) {
>>               multirect_plane[multirect_count].r0 =
>> @@ -1389,7 +1389,7 @@ static int _dpu_debugfs_status_show(struct 
>> seq_file *s, void *data)
>>               state->crtc_x, state->crtc_y, state->crtc_w,
>>               state->crtc_h);
>>           seq_printf(s, "\tmultirect: mode: %d index: %d\n",
>> -            pstate->multirect_mode, pstate->multirect_index);
>> +            pstate->pipe.multirect_mode, pstate->pipe.multirect_index);
>>           seq_puts(s, "\n");
>>       }
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>> index 674f311f99b4..0af2bc6e5df8 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>> @@ -159,15 +159,11 @@ struct dpu_hw_pixel_ext {
>>    * @src_rect:  src ROI, caller takes into account the different 
>> operations
>>    *             such as decimation, flip etc to program this field
>>    * @dest_rect: destination ROI.
>> - * @index:     index of the rectangle of SSPP
>> - * @mode:      parallel or time multiplex multirect mode
>>    */
>>   struct dpu_hw_pipe_cfg {
>>       struct dpu_hw_fmt_layout layout;
>>       struct drm_rect src_rect;
>>       struct drm_rect dst_rect;
>> -    enum dpu_sspp_multirect_index index;
>> -    enum dpu_sspp_multirect_mode mode;
>>   };
>>   /**
>> @@ -218,6 +214,18 @@ struct dpu_hw_pipe_ts_cfg {
>>       u64 time;
>>   };
>> +/**
>> + * struct dpu_sw_pipe - software pipe description
>> + * @sspp:      backing SSPP pipe
>> + * @index:     index of the rectangle of SSPP
>> + * @mode:      parallel or time multiplex multirect mode
>> + */
>> +struct dpu_sw_pipe {
>> +    struct dpu_hw_pipe *sspp;
>> +    enum dpu_sspp_multirect_index multirect_index;
>> +    enum dpu_sspp_multirect_mode multirect_mode;
>> +};
>> +
>>   /**
>>    * struct dpu_hw_sspp_ops - interface to the SSPP Hw driver functions
>>    * Caller must call the init function to get the pipe context for 
>> each pipe
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> index d1f9b4bc10ac..51b5e8a3182b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>> @@ -247,7 +247,7 @@ static int _dpu_plane_calc_fill_level(struct 
>> drm_plane *plane,
>>                   ((src_width + 32) * fmt->bpp);
>>           }
>>       } else {
>> -        if (pstate->multirect_mode == DPU_SSPP_MULTIRECT_PARALLEL) {
>> +        if (pstate->pipe.multirect_mode == 
>> DPU_SSPP_MULTIRECT_PARALLEL) {
>>               total_fl = (fixed_buff_size / 2) * 2 /
>>                   ((src_width + 32) * fmt->bpp);
>>           } else {
>> @@ -331,7 +331,7 @@ static void _dpu_plane_set_qos_lut(struct 
>> drm_plane *plane,
>>               fmt ? (char *)&fmt->base.pixel_format : NULL,
>>               pdpu->is_rt_pipe, total_fl, qos_lut);
>> -    pstate->pipe_hw->ops.setup_creq_lut(pstate->pipe_hw, qos_lut);
>> +    pstate->pipe.sspp->ops.setup_creq_lut(pstate->pipe.sspp, qos_lut);
>>   }
>>   /**
>> @@ -383,7 +383,7 @@ static void _dpu_plane_set_danger_lut(struct 
>> drm_plane *plane,
>>           danger_lut,
>>           safe_lut);
>> -    pstate->pipe_hw->ops.setup_danger_safe_lut(pstate->pipe_hw,
>> +    pstate->pipe.sspp->ops.setup_danger_safe_lut(pstate->pipe.sspp,
>>               danger_lut, safe_lut);
>>   }
>> @@ -403,9 +403,9 @@ static void _dpu_plane_set_qos_ctrl(struct 
>> drm_plane *plane,
>>       memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>       if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>> -        pipe_qos_cfg.creq_vblank = 
>> pstate->pipe_hw->cap->sblk->creq_vblank;
>> +        pipe_qos_cfg.creq_vblank = 
>> pstate->pipe.sspp->cap->sblk->creq_vblank;
>>           pipe_qos_cfg.danger_vblank =
>> -                pstate->pipe_hw->cap->sblk->danger_vblank;
>> +                pstate->pipe.sspp->cap->sblk->danger_vblank;
>>           pipe_qos_cfg.vblank_en = enable;
>>       }
>> @@ -431,7 +431,7 @@ static void _dpu_plane_set_qos_ctrl(struct 
>> drm_plane *plane,
>>           pipe_qos_cfg.danger_vblank,
>>           pdpu->is_rt_pipe);
>> -    pstate->pipe_hw->ops.setup_qos_ctrl(pstate->pipe_hw,
>> +    pstate->pipe.sspp->ops.setup_qos_ctrl(pstate->pipe.sspp,
>>               &pipe_qos_cfg);
>>   }
>> @@ -450,14 +450,14 @@ static void _dpu_plane_set_ot_limit(struct 
>> drm_plane *plane,
>>       struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
>>       memset(&ot_params, 0, sizeof(ot_params));
>> -    ot_params.xin_id = pstate->pipe_hw->cap->xin_id;
>> -    ot_params.num = pstate->pipe_hw->idx - SSPP_NONE;
>> +    ot_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>> +    ot_params.num = pstate->pipe.sspp->idx - SSPP_NONE;
>>       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.vbif_idx = VBIF_RT;
>> -    ot_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>> +    ot_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>>       ot_params.rd = true;
>>       dpu_vbif_set_ot_limit(dpu_kms, &ot_params);
>> @@ -476,9 +476,9 @@ static void _dpu_plane_set_qos_remap(struct 
>> drm_plane *plane)
>>       memset(&qos_params, 0, sizeof(qos_params));
>>       qos_params.vbif_idx = VBIF_RT;
>> -    qos_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>> -    qos_params.xin_id = pstate->pipe_hw->cap->xin_id;
>> -    qos_params.num = pstate->pipe_hw->idx - SSPP_VIG0;
>> +    qos_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>> +    qos_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>> +    qos_params.num = pstate->pipe.sspp->idx - SSPP_VIG0;
>>       qos_params.is_rt = pdpu->is_rt_pipe;
>>       DPU_DEBUG_PLANE(pdpu, "pipe:%d vbif:%d xin:%d rt:%d, 
>> clk_ctrl:%d\n",
>> @@ -505,12 +505,12 @@ static void _dpu_plane_set_scanout(struct 
>> drm_plane *plane,
>>           DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
>>       else if (ret)
>>           DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", 
>> ret);
>> -    else if (pstate->pipe_hw->ops.setup_sourceaddress) {
>> -        trace_dpu_plane_set_scanout(pstate->pipe_hw->idx,
>> +    else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
>> +        trace_dpu_plane_set_scanout(pstate->pipe.sspp->idx,
>>                           &pipe_cfg->layout,
>> -                        pstate->multirect_index);
>> -        pstate->pipe_hw->ops.setup_sourceaddress(pstate->pipe_hw, 
>> pipe_cfg,
>> -                        pstate->multirect_index);
>> +                        pstate->pipe.multirect_index);
>> +        pstate->pipe.sspp->ops.setup_sourceaddress(pstate->pipe.sspp, 
>> pipe_cfg,
>> +                        pstate->pipe.multirect_index);
>>       }
>>   }
>> @@ -553,7 +553,7 @@ static void _dpu_plane_setup_scaler3(struct 
>> dpu_plane *pdpu,
>>               scale_cfg->src_height[i] /= chroma_subsmpl_v;
>>           }
>> -        if (pstate->pipe_hw->cap->features &
>> +        if (pstate->pipe.sspp->cap->features &
>>               BIT(DPU_SSPP_SCALER_QSEED4)) {
>>               scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
>>               scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
>> @@ -624,7 +624,7 @@ static const struct dpu_csc_cfg 
>> *_dpu_plane_get_csc(struct dpu_plane *pdpu, cons
>>       if (!DPU_FORMAT_IS_YUV(fmt))
>>           return NULL;
>> -    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe_hw->cap->features)
>> +    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe.sspp->cap->features)
>>           csc_ptr = &dpu_csc10_YUV2RGB_601L;
>>       else
>>           csc_ptr = &dpu_csc_YUV2RGB_601L;
>> @@ -659,8 +659,8 @@ static void _dpu_plane_setup_scaler(struct 
>> dpu_plane *pdpu,
>>               &scaler3_cfg, &pixel_ext, fmt,
>>               info->hsub, info->vsub);
>> -    if (pstate->pipe_hw->ops.setup_pe)
>> -        pstate->pipe_hw->ops.setup_pe(pstate->pipe_hw,
>> +    if (pstate->pipe.sspp->ops.setup_pe)
>> +        pstate->pipe.sspp->ops.setup_pe(pstate->pipe.sspp,
>>                   &pixel_ext);
>>       /**
>> @@ -668,9 +668,9 @@ static void _dpu_plane_setup_scaler(struct 
>> dpu_plane *pdpu,
>>        * bypassed. Still we need to update alpha and bitwidth
>>        * ONLY for RECT0
>>        */
>> -    if (pstate->pipe_hw->ops.setup_scaler &&
>> -            pstate->multirect_index != DPU_SSPP_RECT_1)
>> -        pstate->pipe_hw->ops.setup_scaler(pstate->pipe_hw,
>> +    if (pstate->pipe.sspp->ops.setup_scaler &&
>> +            pstate->pipe.multirect_index != DPU_SSPP_RECT_1)
>> +        pstate->pipe.sspp->ops.setup_scaler(pstate->pipe.sspp,
>>                   pipe_cfg,
>>                   &scaler3_cfg);
>>   }
>> @@ -699,10 +699,10 @@ static int _dpu_plane_color_fill(struct 
>> dpu_plane *pdpu,
>>       fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>>       /* update sspp */
>> -    if (fmt && pstate->pipe_hw->ops.setup_solidfill) {
>> -        pstate->pipe_hw->ops.setup_solidfill(pstate->pipe_hw,
>> +    if (fmt && pstate->pipe.sspp->ops.setup_solidfill) {
>> +        pstate->pipe.sspp->ops.setup_solidfill(pstate->pipe.sspp,
>>                   (color & 0xFFFFFF) | ((alpha & 0xFF) << 24),
>> -                pstate->multirect_index);
>> +                pstate->pipe.multirect_index);
>>           /* override scaler/decimation if solid fill */
>>           pipe_cfg.dst_rect = pstate->base.dst;
>> @@ -714,15 +714,15 @@ static int _dpu_plane_color_fill(struct 
>> dpu_plane *pdpu,
>>           pipe_cfg.src_rect.y2 =
>>               drm_rect_height(&pipe_cfg.dst_rect);
>> -        if (pstate->pipe_hw->ops.setup_format)
>> -            pstate->pipe_hw->ops.setup_format(pstate->pipe_hw,
>> +        if (pstate->pipe.sspp->ops.setup_format)
>> +            pstate->pipe.sspp->ops.setup_format(pstate->pipe.sspp,
>>                       fmt, DPU_SSPP_SOLID_FILL,
>> -                    pstate->multirect_index);
>> +                    pstate->pipe.multirect_index);
>> -        if (pstate->pipe_hw->ops.setup_rects)
>> -            pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>> +        if (pstate->pipe.sspp->ops.setup_rects)
>> +            pstate->pipe.sspp->ops.setup_rects(pstate->pipe.sspp,
>>                       &pipe_cfg,
>> -                    pstate->multirect_index);
>> +                    pstate->pipe.multirect_index);
>>           _dpu_plane_setup_scaler(pdpu, pstate, fmt, true, &pipe_cfg);
>>       }
>> @@ -734,8 +734,8 @@ void dpu_plane_clear_multirect(const struct 
>> drm_plane_state *drm_state)
>>   {
>>       struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>> -    pstate->multirect_index = DPU_SSPP_RECT_SOLO;
>> -    pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>> +    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)
>> @@ -817,8 +817,8 @@ int dpu_plane_validate_multirect_v2(struct 
>> dpu_multirect_plane_states *plane)
>>       /* Prefer PARALLEL FETCH Mode over TIME_MX Mode */
>>       if (parallel_fetch_qualified) {
>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>> +        pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>> +        pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>           goto done;
>>       }
>> @@ -828,8 +828,8 @@ int dpu_plane_validate_multirect_v2(struct 
>> dpu_multirect_plane_states *plane)
>>       if (dst[R1].y1 >= dst[R0].y2 + buffer_lines ||
>>           dst[R0].y1 >= dst[R1].y2 + buffer_lines) {
>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>> +        pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>> +        pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>       } else {
>>           DPU_ERROR(
>>               "No multirect mode possible for the planes (%d - %d)\n",
>> @@ -839,13 +839,13 @@ int dpu_plane_validate_multirect_v2(struct 
>> dpu_multirect_plane_states *plane)
>>       }
>>   done:
>> -    pstate[R0]->multirect_index = DPU_SSPP_RECT_0;
>> -    pstate[R1]->multirect_index = DPU_SSPP_RECT_1;
>> +    pstate[R0]->pipe.multirect_index = DPU_SSPP_RECT_0;
>> +    pstate[R1]->pipe.multirect_index = DPU_SSPP_RECT_1;
>>       DPU_DEBUG_PLANE(dpu_plane[R0], "R0: %d - %d\n",
>> -        pstate[R0]->multirect_mode, pstate[R0]->multirect_index);
>> +        pstate[R0]->pipe.multirect_mode, 
>> pstate[R0]->pipe.multirect_index);
>>       DPU_DEBUG_PLANE(dpu_plane[R1], "R1: %d - %d\n",
>> -        pstate[R1]->multirect_mode, pstate[R1]->multirect_index);
>> +        pstate[R1]->pipe.multirect_mode, 
>> pstate[R1]->pipe.multirect_index);
>>       return 0;
>>   }
>> @@ -937,6 +937,7 @@ static int dpu_plane_atomic_check(struct drm_plane 
>> *plane,
>>       int ret = 0, min_scale;
>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>       struct dpu_plane_state *pstate = 
>> to_dpu_plane_state(new_plane_state);
>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>       const struct drm_crtc_state *crtc_state = NULL;
>>       const struct dpu_format *fmt;
>>       struct drm_rect src, dst, fb_rect = { 0 };
>> @@ -946,10 +947,10 @@ static int dpu_plane_atomic_check(struct 
>> drm_plane *plane,
>>           crtc_state = drm_atomic_get_new_crtc_state(state,
>>                                  new_plane_state->crtc);
>> -    min_scale = FRAC_16_16(1, pstate->pipe_hw->cap->sblk->maxupscale);
>> +    min_scale = FRAC_16_16(1, pipe->sspp->cap->sblk->maxupscale);
>>       ret = drm_atomic_helper_check_plane_state(new_plane_state, 
>> crtc_state,
>>                             min_scale,
>> -                          pstate->pipe_hw->cap->sblk->maxdwnscale << 16,
>> +                          pipe->sspp->cap->sblk->maxdwnscale << 16,
>>                             true, true);
>>       if (ret) {
>>           DPU_DEBUG_PLANE(pdpu, "Check plane state failed (%d)\n", ret);
>> @@ -975,8 +976,8 @@ static int dpu_plane_atomic_check(struct drm_plane 
>> *plane,
>>       min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>       if (DPU_FORMAT_IS_YUV(fmt) &&
>> -        (!(pstate->pipe_hw->cap->features & DPU_SSPP_SCALER) ||
>> -         !(pstate->pipe_hw->cap->features & DPU_SSPP_CSC_ANY))) {
>> +        (!(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;
>> @@ -1037,12 +1038,12 @@ void dpu_plane_flush(struct drm_plane *plane)
>>       else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>>           /* force 100% alpha */
>>           _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
>> -    else if (pstate->pipe_hw && pstate->pipe_hw->ops.setup_csc) {
>> +    else if (pstate->pipe.sspp && pstate->pipe.sspp->ops.setup_csc) {
>>           const struct dpu_format *fmt = 
>> to_dpu_format(msm_framebuffer_format(plane->state->fb));
>>           const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, 
>> fmt);
>>           if (csc_ptr)
>> -            pstate->pipe_hw->ops.setup_csc(pstate->pipe_hw, csc_ptr);
>> +            pstate->pipe.sspp->ops.setup_csc(pstate->pipe.sspp, 
>> csc_ptr);
>>       }
>>       /* flag h/w flush complete */
>> @@ -1072,6 +1073,7 @@ static void dpu_plane_sspp_atomic_update(struct 
>> drm_plane *plane)
>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>       struct drm_plane_state *state = plane->state;
>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>       struct drm_crtc *crtc = state->crtc;
>>       struct drm_framebuffer *fb = state->fb;
>>       bool is_rt_pipe, update_qos_remap;
>> @@ -1109,21 +1111,21 @@ static void 
>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>           return;
>>       }
>> -    if (pstate->pipe_hw->ops.setup_rects) {
>> -        pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>> +    if (pipe->sspp->ops.setup_rects) {
>> +        pipe->sspp->ops.setup_rects(pipe->sspp,
>>                   &pipe_cfg,
>> -                pstate->multirect_index);
>> +                pipe->multirect_index);
>>       }
>>       _dpu_plane_setup_scaler(pdpu, pstate, fmt, false, &pipe_cfg);
>> -    if (pstate->pipe_hw->ops.setup_multirect)
>> -        pstate->pipe_hw->ops.setup_multirect(
>> -                pstate->pipe_hw,
>> -                pstate->multirect_index,
>> -                pstate->multirect_mode);
>> +    if (pipe->sspp->ops.setup_multirect)
>> +        pipe->sspp->ops.setup_multirect(
>> +                pipe->sspp,
>> +                pipe->multirect_index,
>> +                pipe->multirect_mode);
>> -    if (pstate->pipe_hw->ops.setup_format) {
>> +    if (pipe->sspp->ops.setup_format) {
>>           unsigned int rotation;
>>           src_flags = 0x0;
>> @@ -1140,10 +1142,10 @@ static void 
>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>               src_flags |= DPU_SSPP_FLIP_UD;
>>           /* update format */
>> -        pstate->pipe_hw->ops.setup_format(pstate->pipe_hw, fmt, 
>> src_flags,
>> -                pstate->multirect_index);
>> +        pipe->sspp->ops.setup_format(pipe->sspp, fmt, src_flags,
>> +                pipe->multirect_index);
>> -        if (pstate->pipe_hw->ops.setup_cdp) {
>> +        if (pipe->sspp->ops.setup_cdp) {
>>               struct dpu_hw_pipe_cdp_cfg cdp_cfg;
>>               memset(&cdp_cfg, 0, sizeof(struct dpu_hw_pipe_cdp_cfg));
>> @@ -1157,7 +1159,7 @@ static void dpu_plane_sspp_atomic_update(struct 
>> drm_plane *plane)
>>                       DPU_FORMAT_IS_TILE(fmt);
>>               cdp_cfg.preload_ahead = DPU_SSPP_CDP_PRELOAD_AHEAD_64;
>> -            pstate->pipe_hw->ops.setup_cdp(pstate->pipe_hw, &cdp_cfg, 
>> pstate->multirect_index);
>> +            pipe->sspp->ops.setup_cdp(pipe->sspp, &cdp_cfg, 
>> pipe->multirect_index);
>>           }
>>       }
>> @@ -1191,7 +1193,7 @@ static void _dpu_plane_atomic_disable(struct 
>> drm_plane *plane)
>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>       trace_dpu_plane_disable(DRMID(plane), false,
>> -                pstate->multirect_mode);
>> +                pstate->pipe.multirect_mode);
>>       pstate->pending = true;
>>   }
>> @@ -1305,9 +1307,9 @@ static void dpu_plane_atomic_print_state(struct 
>> drm_printer *p,
>>       const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>       drm_printf(p, "\tstage=%d\n", pstate->stage);
>> -    drm_printf(p, "\tsspp=%s\n", pstate->pipe_hw->cap->name);
>> -    drm_printf(p, "\tmultirect_mode=%s\n", 
>> dpu_get_multirect_mode(pstate->multirect_mode));
>> -    drm_printf(p, "\tmultirect_index=%s\n", 
>> dpu_get_multirect_index(pstate->multirect_index));
>> +    drm_printf(p, "\tsspp=%s\n", pstate->pipe.sspp->cap->name);
>> +    drm_printf(p, "\tmultirect_mode=%s\n", 
>> dpu_get_multirect_mode(pstate->pipe.multirect_mode));
>> +    drm_printf(p, "\tmultirect_index=%s\n", 
>> dpu_get_multirect_index(pstate->pipe.multirect_index));
>>   }
>>   static void dpu_plane_reset(struct drm_plane *plane)
>> @@ -1336,7 +1338,7 @@ static void dpu_plane_reset(struct drm_plane 
>> *plane)
>>           return;
>>       }
>> -    pstate->pipe_hw = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe - 
>> SSPP_NONE]);
>> +    pstate->pipe.sspp = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe - 
>> SSPP_NONE]);
>>       __drm_atomic_helper_plane_reset(plane, &pstate->base);
>>   }
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>> index d745cde4ea77..b6fd6f856d6a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>> @@ -31,11 +31,9 @@ struct dpu_plane_state {
>>       struct msm_gem_address_space *aspace;
>>       enum dpu_stage stage;
>>       bool needs_qos_remap;
>> -    uint32_t multirect_index;
>> -    uint32_t multirect_mode;
>>       bool pending;
>> -    struct dpu_hw_pipe *pipe_hw;
>> +    struct dpu_sw_pipe pipe;
> 
> Are you sure we dont need this to be a pointer?
> 
> Since this is now part of plane_state, its possible that across commits, 
> the hw pipe can change for this plane's state. So might be better if 
> this is pointer in that case ....

A pointer means that it is allocated and deallocated by somebody. 
Probably you meant that it is a pointer into the dpu_hw_pipe member 
(with dpu_hw_pipe having sw_pipe's for rect0 and rect1).

I thought about this. And ended up with the dpu_sw_pipe having a 
dpu_hw_pipe pointer inside, but itself being a member of plane state. 
This way the plane state contains such data as multirect mode and rectangle.


> 
>>       u64 plane_fetch_bw;
>>       u64 plane_clk;

Abhinav Kumar May 6, 2022, 9:48 p.m. UTC | #3

On 5/6/2022 2:39 PM, Dmitry Baryshkov wrote:
> On 07/05/2022 00:30, Abhinav Kumar wrote:
>>
>>
>> On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
>>> Wrap SSPP and multirect index/mode into a single structure that
>>> represents software view on the pipe used.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |   8 +-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  16 ++-
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 136 ++++++++++----------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |   4 +-
>>>   4 files changed, 86 insertions(+), 78 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index ada7d5750536..751c64012058 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -441,14 +441,14 @@ static void _dpu_crtc_blend_setup_ctl(struct 
>>> drm_crtc *crtc)
>>>           pstate = to_dpu_plane_state(state);
>>> -        sspp_idx = pstate->pipe_hw->idx;
>>> +        sspp_idx = pstate->pipe.sspp->idx;
>>>           set_bit(sspp_idx, fetch_active);
>>>           stage_idx = zpos_cnt[pstate->stage]++;
>>>           stage_cfg.stage[pstate->stage][stage_idx] =
>>>                       sspp_idx;
>>>           stage_cfg.multirect_index[pstate->stage][stage_idx] =
>>> -                    pstate->multirect_index;
>>> +                    pstate->pipe.multirect_index;
>>>           /* blend config update */
>>>           for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++)
>>> @@ -1118,7 +1118,7 @@ static int dpu_crtc_atomic_check(struct 
>>> drm_crtc *crtc,
>>>           pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate);
>>>           pstates[cnt].drm_pstate = pstate;
>>>           pstates[cnt].stage = pstate->normalized_zpos;
>>> -        pstates[cnt].pipe_id = 
>>> to_dpu_plane_state(pstate)->pipe_hw->idx;
>>> +        pstates[cnt].pipe_id = 
>>> to_dpu_plane_state(pstate)->pipe.sspp->idx;
>>>           if (pipe_staged[pstates[cnt].pipe_id]) {
>>>               multirect_plane[multirect_count].r0 =
>>> @@ -1389,7 +1389,7 @@ static int _dpu_debugfs_status_show(struct 
>>> seq_file *s, void *data)
>>>               state->crtc_x, state->crtc_y, state->crtc_w,
>>>               state->crtc_h);
>>>           seq_printf(s, "\tmultirect: mode: %d index: %d\n",
>>> -            pstate->multirect_mode, pstate->multirect_index);
>>> +            pstate->pipe.multirect_mode, pstate->pipe.multirect_index);
>>>           seq_puts(s, "\n");
>>>       }
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> index 674f311f99b4..0af2bc6e5df8 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>> @@ -159,15 +159,11 @@ struct dpu_hw_pixel_ext {
>>>    * @src_rect:  src ROI, caller takes into account the different 
>>> operations
>>>    *             such as decimation, flip etc to program this field
>>>    * @dest_rect: destination ROI.
>>> - * @index:     index of the rectangle of SSPP
>>> - * @mode:      parallel or time multiplex multirect mode
>>>    */
>>>   struct dpu_hw_pipe_cfg {
>>>       struct dpu_hw_fmt_layout layout;
>>>       struct drm_rect src_rect;
>>>       struct drm_rect dst_rect;
>>> -    enum dpu_sspp_multirect_index index;
>>> -    enum dpu_sspp_multirect_mode mode;
>>>   };
>>>   /**
>>> @@ -218,6 +214,18 @@ struct dpu_hw_pipe_ts_cfg {
>>>       u64 time;
>>>   };
>>> +/**
>>> + * struct dpu_sw_pipe - software pipe description
>>> + * @sspp:      backing SSPP pipe
>>> + * @index:     index of the rectangle of SSPP
>>> + * @mode:      parallel or time multiplex multirect mode
>>> + */
>>> +struct dpu_sw_pipe {
>>> +    struct dpu_hw_pipe *sspp;
>>> +    enum dpu_sspp_multirect_index multirect_index;
>>> +    enum dpu_sspp_multirect_mode multirect_mode;
>>> +};
>>> +
>>>   /**
>>>    * struct dpu_hw_sspp_ops - interface to the SSPP Hw driver functions
>>>    * Caller must call the init function to get the pipe context for 
>>> each pipe
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> index d1f9b4bc10ac..51b5e8a3182b 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> @@ -247,7 +247,7 @@ static int _dpu_plane_calc_fill_level(struct 
>>> drm_plane *plane,
>>>                   ((src_width + 32) * fmt->bpp);
>>>           }
>>>       } else {
>>> -        if (pstate->multirect_mode == DPU_SSPP_MULTIRECT_PARALLEL) {
>>> +        if (pstate->pipe.multirect_mode == 
>>> DPU_SSPP_MULTIRECT_PARALLEL) {
>>>               total_fl = (fixed_buff_size / 2) * 2 /
>>>                   ((src_width + 32) * fmt->bpp);
>>>           } else {
>>> @@ -331,7 +331,7 @@ static void _dpu_plane_set_qos_lut(struct 
>>> drm_plane *plane,
>>>               fmt ? (char *)&fmt->base.pixel_format : NULL,
>>>               pdpu->is_rt_pipe, total_fl, qos_lut);
>>> -    pstate->pipe_hw->ops.setup_creq_lut(pstate->pipe_hw, qos_lut);
>>> +    pstate->pipe.sspp->ops.setup_creq_lut(pstate->pipe.sspp, qos_lut);
>>>   }
>>>   /**
>>> @@ -383,7 +383,7 @@ static void _dpu_plane_set_danger_lut(struct 
>>> drm_plane *plane,
>>>           danger_lut,
>>>           safe_lut);
>>> -    pstate->pipe_hw->ops.setup_danger_safe_lut(pstate->pipe_hw,
>>> +    pstate->pipe.sspp->ops.setup_danger_safe_lut(pstate->pipe.sspp,
>>>               danger_lut, safe_lut);
>>>   }
>>> @@ -403,9 +403,9 @@ static void _dpu_plane_set_qos_ctrl(struct 
>>> drm_plane *plane,
>>>       memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>>       if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>>> -        pipe_qos_cfg.creq_vblank = 
>>> pstate->pipe_hw->cap->sblk->creq_vblank;
>>> +        pipe_qos_cfg.creq_vblank = 
>>> pstate->pipe.sspp->cap->sblk->creq_vblank;
>>>           pipe_qos_cfg.danger_vblank =
>>> -                pstate->pipe_hw->cap->sblk->danger_vblank;
>>> +                pstate->pipe.sspp->cap->sblk->danger_vblank;
>>>           pipe_qos_cfg.vblank_en = enable;
>>>       }
>>> @@ -431,7 +431,7 @@ static void _dpu_plane_set_qos_ctrl(struct 
>>> drm_plane *plane,
>>>           pipe_qos_cfg.danger_vblank,
>>>           pdpu->is_rt_pipe);
>>> -    pstate->pipe_hw->ops.setup_qos_ctrl(pstate->pipe_hw,
>>> +    pstate->pipe.sspp->ops.setup_qos_ctrl(pstate->pipe.sspp,
>>>               &pipe_qos_cfg);
>>>   }
>>> @@ -450,14 +450,14 @@ static void _dpu_plane_set_ot_limit(struct 
>>> drm_plane *plane,
>>>       struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
>>>       memset(&ot_params, 0, sizeof(ot_params));
>>> -    ot_params.xin_id = pstate->pipe_hw->cap->xin_id;
>>> -    ot_params.num = pstate->pipe_hw->idx - SSPP_NONE;
>>> +    ot_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>>> +    ot_params.num = pstate->pipe.sspp->idx - SSPP_NONE;
>>>       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.vbif_idx = VBIF_RT;
>>> -    ot_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>>> +    ot_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>>>       ot_params.rd = true;
>>>       dpu_vbif_set_ot_limit(dpu_kms, &ot_params);
>>> @@ -476,9 +476,9 @@ static void _dpu_plane_set_qos_remap(struct 
>>> drm_plane *plane)
>>>       memset(&qos_params, 0, sizeof(qos_params));
>>>       qos_params.vbif_idx = VBIF_RT;
>>> -    qos_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>>> -    qos_params.xin_id = pstate->pipe_hw->cap->xin_id;
>>> -    qos_params.num = pstate->pipe_hw->idx - SSPP_VIG0;
>>> +    qos_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>>> +    qos_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>>> +    qos_params.num = pstate->pipe.sspp->idx - SSPP_VIG0;
>>>       qos_params.is_rt = pdpu->is_rt_pipe;
>>>       DPU_DEBUG_PLANE(pdpu, "pipe:%d vbif:%d xin:%d rt:%d, 
>>> clk_ctrl:%d\n",
>>> @@ -505,12 +505,12 @@ static void _dpu_plane_set_scanout(struct 
>>> drm_plane *plane,
>>>           DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
>>>       else if (ret)
>>>           DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", 
>>> ret);
>>> -    else if (pstate->pipe_hw->ops.setup_sourceaddress) {
>>> -        trace_dpu_plane_set_scanout(pstate->pipe_hw->idx,
>>> +    else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
>>> +        trace_dpu_plane_set_scanout(pstate->pipe.sspp->idx,
>>>                           &pipe_cfg->layout,
>>> -                        pstate->multirect_index);
>>> -        pstate->pipe_hw->ops.setup_sourceaddress(pstate->pipe_hw, 
>>> pipe_cfg,
>>> -                        pstate->multirect_index);
>>> +                        pstate->pipe.multirect_index);
>>> +        
>>> pstate->pipe.sspp->ops.setup_sourceaddress(pstate->pipe.sspp, pipe_cfg,
>>> +                        pstate->pipe.multirect_index);
>>>       }
>>>   }
>>> @@ -553,7 +553,7 @@ static void _dpu_plane_setup_scaler3(struct 
>>> dpu_plane *pdpu,
>>>               scale_cfg->src_height[i] /= chroma_subsmpl_v;
>>>           }
>>> -        if (pstate->pipe_hw->cap->features &
>>> +        if (pstate->pipe.sspp->cap->features &
>>>               BIT(DPU_SSPP_SCALER_QSEED4)) {
>>>               scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
>>>               scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
>>> @@ -624,7 +624,7 @@ static const struct dpu_csc_cfg 
>>> *_dpu_plane_get_csc(struct dpu_plane *pdpu, cons
>>>       if (!DPU_FORMAT_IS_YUV(fmt))
>>>           return NULL;
>>> -    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe_hw->cap->features)
>>> +    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe.sspp->cap->features)
>>>           csc_ptr = &dpu_csc10_YUV2RGB_601L;
>>>       else
>>>           csc_ptr = &dpu_csc_YUV2RGB_601L;
>>> @@ -659,8 +659,8 @@ static void _dpu_plane_setup_scaler(struct 
>>> dpu_plane *pdpu,
>>>               &scaler3_cfg, &pixel_ext, fmt,
>>>               info->hsub, info->vsub);
>>> -    if (pstate->pipe_hw->ops.setup_pe)
>>> -        pstate->pipe_hw->ops.setup_pe(pstate->pipe_hw,
>>> +    if (pstate->pipe.sspp->ops.setup_pe)
>>> +        pstate->pipe.sspp->ops.setup_pe(pstate->pipe.sspp,
>>>                   &pixel_ext);
>>>       /**
>>> @@ -668,9 +668,9 @@ static void _dpu_plane_setup_scaler(struct 
>>> dpu_plane *pdpu,
>>>        * bypassed. Still we need to update alpha and bitwidth
>>>        * ONLY for RECT0
>>>        */
>>> -    if (pstate->pipe_hw->ops.setup_scaler &&
>>> -            pstate->multirect_index != DPU_SSPP_RECT_1)
>>> -        pstate->pipe_hw->ops.setup_scaler(pstate->pipe_hw,
>>> +    if (pstate->pipe.sspp->ops.setup_scaler &&
>>> +            pstate->pipe.multirect_index != DPU_SSPP_RECT_1)
>>> +        pstate->pipe.sspp->ops.setup_scaler(pstate->pipe.sspp,
>>>                   pipe_cfg,
>>>                   &scaler3_cfg);
>>>   }
>>> @@ -699,10 +699,10 @@ static int _dpu_plane_color_fill(struct 
>>> dpu_plane *pdpu,
>>>       fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>>>       /* update sspp */
>>> -    if (fmt && pstate->pipe_hw->ops.setup_solidfill) {
>>> -        pstate->pipe_hw->ops.setup_solidfill(pstate->pipe_hw,
>>> +    if (fmt && pstate->pipe.sspp->ops.setup_solidfill) {
>>> +        pstate->pipe.sspp->ops.setup_solidfill(pstate->pipe.sspp,
>>>                   (color & 0xFFFFFF) | ((alpha & 0xFF) << 24),
>>> -                pstate->multirect_index);
>>> +                pstate->pipe.multirect_index);
>>>           /* override scaler/decimation if solid fill */
>>>           pipe_cfg.dst_rect = pstate->base.dst;
>>> @@ -714,15 +714,15 @@ static int _dpu_plane_color_fill(struct 
>>> dpu_plane *pdpu,
>>>           pipe_cfg.src_rect.y2 =
>>>               drm_rect_height(&pipe_cfg.dst_rect);
>>> -        if (pstate->pipe_hw->ops.setup_format)
>>> -            pstate->pipe_hw->ops.setup_format(pstate->pipe_hw,
>>> +        if (pstate->pipe.sspp->ops.setup_format)
>>> +            pstate->pipe.sspp->ops.setup_format(pstate->pipe.sspp,
>>>                       fmt, DPU_SSPP_SOLID_FILL,
>>> -                    pstate->multirect_index);
>>> +                    pstate->pipe.multirect_index);
>>> -        if (pstate->pipe_hw->ops.setup_rects)
>>> -            pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>>> +        if (pstate->pipe.sspp->ops.setup_rects)
>>> +            pstate->pipe.sspp->ops.setup_rects(pstate->pipe.sspp,
>>>                       &pipe_cfg,
>>> -                    pstate->multirect_index);
>>> +                    pstate->pipe.multirect_index);
>>>           _dpu_plane_setup_scaler(pdpu, pstate, fmt, true, &pipe_cfg);
>>>       }
>>> @@ -734,8 +734,8 @@ void dpu_plane_clear_multirect(const struct 
>>> drm_plane_state *drm_state)
>>>   {
>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>>> -    pstate->multirect_index = DPU_SSPP_RECT_SOLO;
>>> -    pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> +    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)
>>> @@ -817,8 +817,8 @@ int dpu_plane_validate_multirect_v2(struct 
>>> dpu_multirect_plane_states *plane)
>>>       /* Prefer PARALLEL FETCH Mode over TIME_MX Mode */
>>>       if (parallel_fetch_qualified) {
>>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>> +        pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>> +        pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>           goto done;
>>>       }
>>> @@ -828,8 +828,8 @@ int dpu_plane_validate_multirect_v2(struct 
>>> dpu_multirect_plane_states *plane)
>>>       if (dst[R1].y1 >= dst[R0].y2 + buffer_lines ||
>>>           dst[R0].y1 >= dst[R1].y2 + buffer_lines) {
>>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>> +        pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>> +        pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>       } else {
>>>           DPU_ERROR(
>>>               "No multirect mode possible for the planes (%d - %d)\n",
>>> @@ -839,13 +839,13 @@ int dpu_plane_validate_multirect_v2(struct 
>>> dpu_multirect_plane_states *plane)
>>>       }
>>>   done:
>>> -    pstate[R0]->multirect_index = DPU_SSPP_RECT_0;
>>> -    pstate[R1]->multirect_index = DPU_SSPP_RECT_1;
>>> +    pstate[R0]->pipe.multirect_index = DPU_SSPP_RECT_0;
>>> +    pstate[R1]->pipe.multirect_index = DPU_SSPP_RECT_1;
>>>       DPU_DEBUG_PLANE(dpu_plane[R0], "R0: %d - %d\n",
>>> -        pstate[R0]->multirect_mode, pstate[R0]->multirect_index);
>>> +        pstate[R0]->pipe.multirect_mode, 
>>> pstate[R0]->pipe.multirect_index);
>>>       DPU_DEBUG_PLANE(dpu_plane[R1], "R1: %d - %d\n",
>>> -        pstate[R1]->multirect_mode, pstate[R1]->multirect_index);
>>> +        pstate[R1]->pipe.multirect_mode, 
>>> pstate[R1]->pipe.multirect_index);
>>>       return 0;
>>>   }
>>> @@ -937,6 +937,7 @@ static int dpu_plane_atomic_check(struct 
>>> drm_plane *plane,
>>>       int ret = 0, min_scale;
>>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>       struct dpu_plane_state *pstate = 
>>> to_dpu_plane_state(new_plane_state);
>>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>>       const struct drm_crtc_state *crtc_state = NULL;
>>>       const struct dpu_format *fmt;
>>>       struct drm_rect src, dst, fb_rect = { 0 };
>>> @@ -946,10 +947,10 @@ static int dpu_plane_atomic_check(struct 
>>> drm_plane *plane,
>>>           crtc_state = drm_atomic_get_new_crtc_state(state,
>>>                                  new_plane_state->crtc);
>>> -    min_scale = FRAC_16_16(1, pstate->pipe_hw->cap->sblk->maxupscale);
>>> +    min_scale = FRAC_16_16(1, pipe->sspp->cap->sblk->maxupscale);
>>>       ret = drm_atomic_helper_check_plane_state(new_plane_state, 
>>> crtc_state,
>>>                             min_scale,
>>> -                          pstate->pipe_hw->cap->sblk->maxdwnscale << 
>>> 16,
>>> +                          pipe->sspp->cap->sblk->maxdwnscale << 16,
>>>                             true, true);
>>>       if (ret) {
>>>           DPU_DEBUG_PLANE(pdpu, "Check plane state failed (%d)\n", ret);
>>> @@ -975,8 +976,8 @@ static int dpu_plane_atomic_check(struct 
>>> drm_plane *plane,
>>>       min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>       if (DPU_FORMAT_IS_YUV(fmt) &&
>>> -        (!(pstate->pipe_hw->cap->features & DPU_SSPP_SCALER) ||
>>> -         !(pstate->pipe_hw->cap->features & DPU_SSPP_CSC_ANY))) {
>>> +        (!(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;
>>> @@ -1037,12 +1038,12 @@ void dpu_plane_flush(struct drm_plane *plane)
>>>       else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>>>           /* force 100% alpha */
>>>           _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
>>> -    else if (pstate->pipe_hw && pstate->pipe_hw->ops.setup_csc) {
>>> +    else if (pstate->pipe.sspp && pstate->pipe.sspp->ops.setup_csc) {
>>>           const struct dpu_format *fmt = 
>>> to_dpu_format(msm_framebuffer_format(plane->state->fb));
>>>           const struct dpu_csc_cfg *csc_ptr = 
>>> _dpu_plane_get_csc(pdpu, fmt);
>>>           if (csc_ptr)
>>> -            pstate->pipe_hw->ops.setup_csc(pstate->pipe_hw, csc_ptr);
>>> +            pstate->pipe.sspp->ops.setup_csc(pstate->pipe.sspp, 
>>> csc_ptr);
>>>       }
>>>       /* flag h/w flush complete */
>>> @@ -1072,6 +1073,7 @@ static void dpu_plane_sspp_atomic_update(struct 
>>> drm_plane *plane)
>>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>       struct drm_plane_state *state = plane->state;
>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>>       struct drm_crtc *crtc = state->crtc;
>>>       struct drm_framebuffer *fb = state->fb;
>>>       bool is_rt_pipe, update_qos_remap;
>>> @@ -1109,21 +1111,21 @@ static void 
>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>           return;
>>>       }
>>> -    if (pstate->pipe_hw->ops.setup_rects) {
>>> -        pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>>> +    if (pipe->sspp->ops.setup_rects) {
>>> +        pipe->sspp->ops.setup_rects(pipe->sspp,
>>>                   &pipe_cfg,
>>> -                pstate->multirect_index);
>>> +                pipe->multirect_index);
>>>       }
>>>       _dpu_plane_setup_scaler(pdpu, pstate, fmt, false, &pipe_cfg);
>>> -    if (pstate->pipe_hw->ops.setup_multirect)
>>> -        pstate->pipe_hw->ops.setup_multirect(
>>> -                pstate->pipe_hw,
>>> -                pstate->multirect_index,
>>> -                pstate->multirect_mode);
>>> +    if (pipe->sspp->ops.setup_multirect)
>>> +        pipe->sspp->ops.setup_multirect(
>>> +                pipe->sspp,
>>> +                pipe->multirect_index,
>>> +                pipe->multirect_mode);
>>> -    if (pstate->pipe_hw->ops.setup_format) {
>>> +    if (pipe->sspp->ops.setup_format) {
>>>           unsigned int rotation;
>>>           src_flags = 0x0;
>>> @@ -1140,10 +1142,10 @@ static void 
>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>               src_flags |= DPU_SSPP_FLIP_UD;
>>>           /* update format */
>>> -        pstate->pipe_hw->ops.setup_format(pstate->pipe_hw, fmt, 
>>> src_flags,
>>> -                pstate->multirect_index);
>>> +        pipe->sspp->ops.setup_format(pipe->sspp, fmt, src_flags,
>>> +                pipe->multirect_index);
>>> -        if (pstate->pipe_hw->ops.setup_cdp) {
>>> +        if (pipe->sspp->ops.setup_cdp) {
>>>               struct dpu_hw_pipe_cdp_cfg cdp_cfg;
>>>               memset(&cdp_cfg, 0, sizeof(struct dpu_hw_pipe_cdp_cfg));
>>> @@ -1157,7 +1159,7 @@ static void dpu_plane_sspp_atomic_update(struct 
>>> drm_plane *plane)
>>>                       DPU_FORMAT_IS_TILE(fmt);
>>>               cdp_cfg.preload_ahead = DPU_SSPP_CDP_PRELOAD_AHEAD_64;
>>> -            pstate->pipe_hw->ops.setup_cdp(pstate->pipe_hw, 
>>> &cdp_cfg, pstate->multirect_index);
>>> +            pipe->sspp->ops.setup_cdp(pipe->sspp, &cdp_cfg, 
>>> pipe->multirect_index);
>>>           }
>>>       }
>>> @@ -1191,7 +1193,7 @@ static void _dpu_plane_atomic_disable(struct 
>>> drm_plane *plane)
>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>       trace_dpu_plane_disable(DRMID(plane), false,
>>> -                pstate->multirect_mode);
>>> +                pstate->pipe.multirect_mode);
>>>       pstate->pending = true;
>>>   }
>>> @@ -1305,9 +1307,9 @@ static void dpu_plane_atomic_print_state(struct 
>>> drm_printer *p,
>>>       const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>       drm_printf(p, "\tstage=%d\n", pstate->stage);
>>> -    drm_printf(p, "\tsspp=%s\n", pstate->pipe_hw->cap->name);
>>> -    drm_printf(p, "\tmultirect_mode=%s\n", 
>>> dpu_get_multirect_mode(pstate->multirect_mode));
>>> -    drm_printf(p, "\tmultirect_index=%s\n", 
>>> dpu_get_multirect_index(pstate->multirect_index));
>>> +    drm_printf(p, "\tsspp=%s\n", pstate->pipe.sspp->cap->name);
>>> +    drm_printf(p, "\tmultirect_mode=%s\n", 
>>> dpu_get_multirect_mode(pstate->pipe.multirect_mode));
>>> +    drm_printf(p, "\tmultirect_index=%s\n", 
>>> dpu_get_multirect_index(pstate->pipe.multirect_index));
>>>   }
>>>   static void dpu_plane_reset(struct drm_plane *plane)
>>> @@ -1336,7 +1338,7 @@ static void dpu_plane_reset(struct drm_plane 
>>> *plane)
>>>           return;
>>>       }
>>> -    pstate->pipe_hw = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe - 
>>> SSPP_NONE]);
>>> +    pstate->pipe.sspp = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe 
>>> - SSPP_NONE]);
>>>       __drm_atomic_helper_plane_reset(plane, &pstate->base);
>>>   }
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>> index d745cde4ea77..b6fd6f856d6a 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>> @@ -31,11 +31,9 @@ struct dpu_plane_state {
>>>       struct msm_gem_address_space *aspace;
>>>       enum dpu_stage stage;
>>>       bool needs_qos_remap;
>>> -    uint32_t multirect_index;
>>> -    uint32_t multirect_mode;
>>>       bool pending;
>>> -    struct dpu_hw_pipe *pipe_hw;
>>> +    struct dpu_sw_pipe pipe;
>>
>> Are you sure we dont need this to be a pointer?
>>
>> Since this is now part of plane_state, its possible that across 
>> commits, the hw pipe can change for this plane's state. So might be 
>> better if this is pointer in that case ....
> 
> A pointer means that it is allocated and deallocated by somebody. 
> Probably you meant that it is a pointer into the dpu_hw_pipe member 
> (with dpu_hw_pipe having sw_pipe's for rect0 and rect1).
> 
> I thought about this. And ended up with the dpu_sw_pipe having a 
> dpu_hw_pipe pointer inside, but itself being a member of plane state. 
> This way the plane state contains such data as multirect mode and 
> rectangle.

Right, so what I meant was, previously pipe_hw was a pointer but it was 
not changed dynamically because it was assigned once in dpu_plane_init() 
but after that was not updated to point to a different pipe_hw.

Now, pipe_hw is part of the plane state which means the actual hardware 
which the plane state is using can change across commits with virtual 
plane support.

So it made more sense to me that this was "struct dpu_sw_pipe *pipe" 
because looking at it from a top level it tells me that the dpu_sw_pipe 
can point to a different pipe. Just little more intuitive.

With it being just a member, sure it still means the same thing because 
internal to it, pipe_hw is still a pointer.

Just felt it was more logical to have the whole dpu_sw_pipe as a pointer.

But its just a thought not a strong inclination against current patch.

> 
> 
>>
>>>       u64 plane_fetch_bw;
>>>       u64 plane_clk;
> 
>

Dmitry Baryshkov May 6, 2022, 10:29 p.m. UTC | #4

On 07/05/2022 00:48, Abhinav Kumar wrote:
> 
> 
> On 5/6/2022 2:39 PM, Dmitry Baryshkov wrote:
>> On 07/05/2022 00:30, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
>>>> Wrap SSPP and multirect index/mode into a single structure that
>>>> represents software view on the pipe used.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |   8 +-
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  16 ++-
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 136 
>>>> ++++++++++----------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |   4 +-
>>>>   4 files changed, 86 insertions(+), 78 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> index ada7d5750536..751c64012058 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> @@ -441,14 +441,14 @@ static void _dpu_crtc_blend_setup_ctl(struct 
>>>> drm_crtc *crtc)
>>>>           pstate = to_dpu_plane_state(state);
>>>> -        sspp_idx = pstate->pipe_hw->idx;
>>>> +        sspp_idx = pstate->pipe.sspp->idx;
>>>>           set_bit(sspp_idx, fetch_active);
>>>>           stage_idx = zpos_cnt[pstate->stage]++;
>>>>           stage_cfg.stage[pstate->stage][stage_idx] =
>>>>                       sspp_idx;
>>>>           stage_cfg.multirect_index[pstate->stage][stage_idx] =
>>>> -                    pstate->multirect_index;
>>>> +                    pstate->pipe.multirect_index;
>>>>           /* blend config update */
>>>>           for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++)
>>>> @@ -1118,7 +1118,7 @@ static int dpu_crtc_atomic_check(struct 
>>>> drm_crtc *crtc,
>>>>           pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate);
>>>>           pstates[cnt].drm_pstate = pstate;
>>>>           pstates[cnt].stage = pstate->normalized_zpos;
>>>> -        pstates[cnt].pipe_id = 
>>>> to_dpu_plane_state(pstate)->pipe_hw->idx;
>>>> +        pstates[cnt].pipe_id = 
>>>> to_dpu_plane_state(pstate)->pipe.sspp->idx;
>>>>           if (pipe_staged[pstates[cnt].pipe_id]) {
>>>>               multirect_plane[multirect_count].r0 =
>>>> @@ -1389,7 +1389,7 @@ static int _dpu_debugfs_status_show(struct 
>>>> seq_file *s, void *data)
>>>>               state->crtc_x, state->crtc_y, state->crtc_w,
>>>>               state->crtc_h);
>>>>           seq_printf(s, "\tmultirect: mode: %d index: %d\n",
>>>> -            pstate->multirect_mode, pstate->multirect_index);
>>>> +            pstate->pipe.multirect_mode, 
>>>> pstate->pipe.multirect_index);
>>>>           seq_puts(s, "\n");
>>>>       }
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>> index 674f311f99b4..0af2bc6e5df8 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>> @@ -159,15 +159,11 @@ struct dpu_hw_pixel_ext {
>>>>    * @src_rect:  src ROI, caller takes into account the different 
>>>> operations
>>>>    *             such as decimation, flip etc to program this field
>>>>    * @dest_rect: destination ROI.
>>>> - * @index:     index of the rectangle of SSPP
>>>> - * @mode:      parallel or time multiplex multirect mode
>>>>    */
>>>>   struct dpu_hw_pipe_cfg {
>>>>       struct dpu_hw_fmt_layout layout;
>>>>       struct drm_rect src_rect;
>>>>       struct drm_rect dst_rect;
>>>> -    enum dpu_sspp_multirect_index index;
>>>> -    enum dpu_sspp_multirect_mode mode;
>>>>   };
>>>>   /**
>>>> @@ -218,6 +214,18 @@ struct dpu_hw_pipe_ts_cfg {
>>>>       u64 time;
>>>>   };
>>>> +/**
>>>> + * struct dpu_sw_pipe - software pipe description
>>>> + * @sspp:      backing SSPP pipe
>>>> + * @index:     index of the rectangle of SSPP
>>>> + * @mode:      parallel or time multiplex multirect mode
>>>> + */
>>>> +struct dpu_sw_pipe {
>>>> +    struct dpu_hw_pipe *sspp;
>>>> +    enum dpu_sspp_multirect_index multirect_index;
>>>> +    enum dpu_sspp_multirect_mode multirect_mode;
>>>> +};
>>>> +
>>>>   /**
>>>>    * struct dpu_hw_sspp_ops - interface to the SSPP Hw driver functions
>>>>    * Caller must call the init function to get the pipe context for 
>>>> each pipe
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> index d1f9b4bc10ac..51b5e8a3182b 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>> @@ -247,7 +247,7 @@ static int _dpu_plane_calc_fill_level(struct 
>>>> drm_plane *plane,
>>>>                   ((src_width + 32) * fmt->bpp);
>>>>           }
>>>>       } else {
>>>> -        if (pstate->multirect_mode == DPU_SSPP_MULTIRECT_PARALLEL) {
>>>> +        if (pstate->pipe.multirect_mode == 
>>>> DPU_SSPP_MULTIRECT_PARALLEL) {
>>>>               total_fl = (fixed_buff_size / 2) * 2 /
>>>>                   ((src_width + 32) * fmt->bpp);
>>>>           } else {
>>>> @@ -331,7 +331,7 @@ static void _dpu_plane_set_qos_lut(struct 
>>>> drm_plane *plane,
>>>>               fmt ? (char *)&fmt->base.pixel_format : NULL,
>>>>               pdpu->is_rt_pipe, total_fl, qos_lut);
>>>> -    pstate->pipe_hw->ops.setup_creq_lut(pstate->pipe_hw, qos_lut);
>>>> +    pstate->pipe.sspp->ops.setup_creq_lut(pstate->pipe.sspp, qos_lut);
>>>>   }
>>>>   /**
>>>> @@ -383,7 +383,7 @@ static void _dpu_plane_set_danger_lut(struct 
>>>> drm_plane *plane,
>>>>           danger_lut,
>>>>           safe_lut);
>>>> -    pstate->pipe_hw->ops.setup_danger_safe_lut(pstate->pipe_hw,
>>>> +    pstate->pipe.sspp->ops.setup_danger_safe_lut(pstate->pipe.sspp,
>>>>               danger_lut, safe_lut);
>>>>   }
>>>> @@ -403,9 +403,9 @@ static void _dpu_plane_set_qos_ctrl(struct 
>>>> drm_plane *plane,
>>>>       memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>>>       if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>>>> -        pipe_qos_cfg.creq_vblank = 
>>>> pstate->pipe_hw->cap->sblk->creq_vblank;
>>>> +        pipe_qos_cfg.creq_vblank = 
>>>> pstate->pipe.sspp->cap->sblk->creq_vblank;
>>>>           pipe_qos_cfg.danger_vblank =
>>>> -                pstate->pipe_hw->cap->sblk->danger_vblank;
>>>> +                pstate->pipe.sspp->cap->sblk->danger_vblank;
>>>>           pipe_qos_cfg.vblank_en = enable;
>>>>       }
>>>> @@ -431,7 +431,7 @@ static void _dpu_plane_set_qos_ctrl(struct 
>>>> drm_plane *plane,
>>>>           pipe_qos_cfg.danger_vblank,
>>>>           pdpu->is_rt_pipe);
>>>> -    pstate->pipe_hw->ops.setup_qos_ctrl(pstate->pipe_hw,
>>>> +    pstate->pipe.sspp->ops.setup_qos_ctrl(pstate->pipe.sspp,
>>>>               &pipe_qos_cfg);
>>>>   }
>>>> @@ -450,14 +450,14 @@ static void _dpu_plane_set_ot_limit(struct 
>>>> drm_plane *plane,
>>>>       struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
>>>>       memset(&ot_params, 0, sizeof(ot_params));
>>>> -    ot_params.xin_id = pstate->pipe_hw->cap->xin_id;
>>>> -    ot_params.num = pstate->pipe_hw->idx - SSPP_NONE;
>>>> +    ot_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>>>> +    ot_params.num = pstate->pipe.sspp->idx - SSPP_NONE;
>>>>       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.vbif_idx = VBIF_RT;
>>>> -    ot_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>>>> +    ot_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>>>>       ot_params.rd = true;
>>>>       dpu_vbif_set_ot_limit(dpu_kms, &ot_params);
>>>> @@ -476,9 +476,9 @@ static void _dpu_plane_set_qos_remap(struct 
>>>> drm_plane *plane)
>>>>       memset(&qos_params, 0, sizeof(qos_params));
>>>>       qos_params.vbif_idx = VBIF_RT;
>>>> -    qos_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>>>> -    qos_params.xin_id = pstate->pipe_hw->cap->xin_id;
>>>> -    qos_params.num = pstate->pipe_hw->idx - SSPP_VIG0;
>>>> +    qos_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>>>> +    qos_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>>>> +    qos_params.num = pstate->pipe.sspp->idx - SSPP_VIG0;
>>>>       qos_params.is_rt = pdpu->is_rt_pipe;
>>>>       DPU_DEBUG_PLANE(pdpu, "pipe:%d vbif:%d xin:%d rt:%d, 
>>>> clk_ctrl:%d\n",
>>>> @@ -505,12 +505,12 @@ static void _dpu_plane_set_scanout(struct 
>>>> drm_plane *plane,
>>>>           DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
>>>>       else if (ret)
>>>>           DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", 
>>>> ret);
>>>> -    else if (pstate->pipe_hw->ops.setup_sourceaddress) {
>>>> -        trace_dpu_plane_set_scanout(pstate->pipe_hw->idx,
>>>> +    else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
>>>> +        trace_dpu_plane_set_scanout(pstate->pipe.sspp->idx,
>>>>                           &pipe_cfg->layout,
>>>> -                        pstate->multirect_index);
>>>> -        pstate->pipe_hw->ops.setup_sourceaddress(pstate->pipe_hw, 
>>>> pipe_cfg,
>>>> -                        pstate->multirect_index);
>>>> +                        pstate->pipe.multirect_index);
>>>> + pstate->pipe.sspp->ops.setup_sourceaddress(pstate->pipe.sspp, 
>>>> pipe_cfg,
>>>> +                        pstate->pipe.multirect_index);
>>>>       }
>>>>   }
>>>> @@ -553,7 +553,7 @@ static void _dpu_plane_setup_scaler3(struct 
>>>> dpu_plane *pdpu,
>>>>               scale_cfg->src_height[i] /= chroma_subsmpl_v;
>>>>           }
>>>> -        if (pstate->pipe_hw->cap->features &
>>>> +        if (pstate->pipe.sspp->cap->features &
>>>>               BIT(DPU_SSPP_SCALER_QSEED4)) {
>>>>               scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
>>>>               scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
>>>> @@ -624,7 +624,7 @@ static const struct dpu_csc_cfg 
>>>> *_dpu_plane_get_csc(struct dpu_plane *pdpu, cons
>>>>       if (!DPU_FORMAT_IS_YUV(fmt))
>>>>           return NULL;
>>>> -    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe_hw->cap->features)
>>>> +    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe.sspp->cap->features)
>>>>           csc_ptr = &dpu_csc10_YUV2RGB_601L;
>>>>       else
>>>>           csc_ptr = &dpu_csc_YUV2RGB_601L;
>>>> @@ -659,8 +659,8 @@ static void _dpu_plane_setup_scaler(struct 
>>>> dpu_plane *pdpu,
>>>>               &scaler3_cfg, &pixel_ext, fmt,
>>>>               info->hsub, info->vsub);
>>>> -    if (pstate->pipe_hw->ops.setup_pe)
>>>> -        pstate->pipe_hw->ops.setup_pe(pstate->pipe_hw,
>>>> +    if (pstate->pipe.sspp->ops.setup_pe)
>>>> +        pstate->pipe.sspp->ops.setup_pe(pstate->pipe.sspp,
>>>>                   &pixel_ext);
>>>>       /**
>>>> @@ -668,9 +668,9 @@ static void _dpu_plane_setup_scaler(struct 
>>>> dpu_plane *pdpu,
>>>>        * bypassed. Still we need to update alpha and bitwidth
>>>>        * ONLY for RECT0
>>>>        */
>>>> -    if (pstate->pipe_hw->ops.setup_scaler &&
>>>> -            pstate->multirect_index != DPU_SSPP_RECT_1)
>>>> -        pstate->pipe_hw->ops.setup_scaler(pstate->pipe_hw,
>>>> +    if (pstate->pipe.sspp->ops.setup_scaler &&
>>>> +            pstate->pipe.multirect_index != DPU_SSPP_RECT_1)
>>>> +        pstate->pipe.sspp->ops.setup_scaler(pstate->pipe.sspp,
>>>>                   pipe_cfg,
>>>>                   &scaler3_cfg);
>>>>   }
>>>> @@ -699,10 +699,10 @@ static int _dpu_plane_color_fill(struct 
>>>> dpu_plane *pdpu,
>>>>       fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>>>>       /* update sspp */
>>>> -    if (fmt && pstate->pipe_hw->ops.setup_solidfill) {
>>>> -        pstate->pipe_hw->ops.setup_solidfill(pstate->pipe_hw,
>>>> +    if (fmt && pstate->pipe.sspp->ops.setup_solidfill) {
>>>> +        pstate->pipe.sspp->ops.setup_solidfill(pstate->pipe.sspp,
>>>>                   (color & 0xFFFFFF) | ((alpha & 0xFF) << 24),
>>>> -                pstate->multirect_index);
>>>> +                pstate->pipe.multirect_index);
>>>>           /* override scaler/decimation if solid fill */
>>>>           pipe_cfg.dst_rect = pstate->base.dst;
>>>> @@ -714,15 +714,15 @@ static int _dpu_plane_color_fill(struct 
>>>> dpu_plane *pdpu,
>>>>           pipe_cfg.src_rect.y2 =
>>>>               drm_rect_height(&pipe_cfg.dst_rect);
>>>> -        if (pstate->pipe_hw->ops.setup_format)
>>>> -            pstate->pipe_hw->ops.setup_format(pstate->pipe_hw,
>>>> +        if (pstate->pipe.sspp->ops.setup_format)
>>>> +            pstate->pipe.sspp->ops.setup_format(pstate->pipe.sspp,
>>>>                       fmt, DPU_SSPP_SOLID_FILL,
>>>> -                    pstate->multirect_index);
>>>> +                    pstate->pipe.multirect_index);
>>>> -        if (pstate->pipe_hw->ops.setup_rects)
>>>> -            pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>>>> +        if (pstate->pipe.sspp->ops.setup_rects)
>>>> +            pstate->pipe.sspp->ops.setup_rects(pstate->pipe.sspp,
>>>>                       &pipe_cfg,
>>>> -                    pstate->multirect_index);
>>>> +                    pstate->pipe.multirect_index);
>>>>           _dpu_plane_setup_scaler(pdpu, pstate, fmt, true, &pipe_cfg);
>>>>       }
>>>> @@ -734,8 +734,8 @@ void dpu_plane_clear_multirect(const struct 
>>>> drm_plane_state *drm_state)
>>>>   {
>>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>>>> -    pstate->multirect_index = DPU_SSPP_RECT_SOLO;
>>>> -    pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>> +    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)
>>>> @@ -817,8 +817,8 @@ int dpu_plane_validate_multirect_v2(struct 
>>>> dpu_multirect_plane_states *plane)
>>>>       /* Prefer PARALLEL FETCH Mode over TIME_MX Mode */
>>>>       if (parallel_fetch_qualified) {
>>>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>> +        pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>> +        pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>>           goto done;
>>>>       }
>>>> @@ -828,8 +828,8 @@ int dpu_plane_validate_multirect_v2(struct 
>>>> dpu_multirect_plane_states *plane)
>>>>       if (dst[R1].y1 >= dst[R0].y2 + buffer_lines ||
>>>>           dst[R0].y1 >= dst[R1].y2 + buffer_lines) {
>>>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>> +        pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>> +        pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>>       } else {
>>>>           DPU_ERROR(
>>>>               "No multirect mode possible for the planes (%d - %d)\n",
>>>> @@ -839,13 +839,13 @@ int dpu_plane_validate_multirect_v2(struct 
>>>> dpu_multirect_plane_states *plane)
>>>>       }
>>>>   done:
>>>> -    pstate[R0]->multirect_index = DPU_SSPP_RECT_0;
>>>> -    pstate[R1]->multirect_index = DPU_SSPP_RECT_1;
>>>> +    pstate[R0]->pipe.multirect_index = DPU_SSPP_RECT_0;
>>>> +    pstate[R1]->pipe.multirect_index = DPU_SSPP_RECT_1;
>>>>       DPU_DEBUG_PLANE(dpu_plane[R0], "R0: %d - %d\n",
>>>> -        pstate[R0]->multirect_mode, pstate[R0]->multirect_index);
>>>> +        pstate[R0]->pipe.multirect_mode, 
>>>> pstate[R0]->pipe.multirect_index);
>>>>       DPU_DEBUG_PLANE(dpu_plane[R1], "R1: %d - %d\n",
>>>> -        pstate[R1]->multirect_mode, pstate[R1]->multirect_index);
>>>> +        pstate[R1]->pipe.multirect_mode, 
>>>> pstate[R1]->pipe.multirect_index);
>>>>       return 0;
>>>>   }
>>>> @@ -937,6 +937,7 @@ static int dpu_plane_atomic_check(struct 
>>>> drm_plane *plane,
>>>>       int ret = 0, min_scale;
>>>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>>       struct dpu_plane_state *pstate = 
>>>> to_dpu_plane_state(new_plane_state);
>>>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>>>       const struct drm_crtc_state *crtc_state = NULL;
>>>>       const struct dpu_format *fmt;
>>>>       struct drm_rect src, dst, fb_rect = { 0 };
>>>> @@ -946,10 +947,10 @@ static int dpu_plane_atomic_check(struct 
>>>> drm_plane *plane,
>>>>           crtc_state = drm_atomic_get_new_crtc_state(state,
>>>>                                  new_plane_state->crtc);
>>>> -    min_scale = FRAC_16_16(1, pstate->pipe_hw->cap->sblk->maxupscale);
>>>> +    min_scale = FRAC_16_16(1, pipe->sspp->cap->sblk->maxupscale);
>>>>       ret = drm_atomic_helper_check_plane_state(new_plane_state, 
>>>> crtc_state,
>>>>                             min_scale,
>>>> -                          pstate->pipe_hw->cap->sblk->maxdwnscale 
>>>> << 16,
>>>> +                          pipe->sspp->cap->sblk->maxdwnscale << 16,
>>>>                             true, true);
>>>>       if (ret) {
>>>>           DPU_DEBUG_PLANE(pdpu, "Check plane state failed (%d)\n", 
>>>> ret);
>>>> @@ -975,8 +976,8 @@ static int dpu_plane_atomic_check(struct 
>>>> drm_plane *plane,
>>>>       min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>>       if (DPU_FORMAT_IS_YUV(fmt) &&
>>>> -        (!(pstate->pipe_hw->cap->features & DPU_SSPP_SCALER) ||
>>>> -         !(pstate->pipe_hw->cap->features & DPU_SSPP_CSC_ANY))) {
>>>> +        (!(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;
>>>> @@ -1037,12 +1038,12 @@ void dpu_plane_flush(struct drm_plane *plane)
>>>>       else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>>>>           /* force 100% alpha */
>>>>           _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
>>>> -    else if (pstate->pipe_hw && pstate->pipe_hw->ops.setup_csc) {
>>>> +    else if (pstate->pipe.sspp && pstate->pipe.sspp->ops.setup_csc) {
>>>>           const struct dpu_format *fmt = 
>>>> to_dpu_format(msm_framebuffer_format(plane->state->fb));
>>>>           const struct dpu_csc_cfg *csc_ptr = 
>>>> _dpu_plane_get_csc(pdpu, fmt);
>>>>           if (csc_ptr)
>>>> -            pstate->pipe_hw->ops.setup_csc(pstate->pipe_hw, csc_ptr);
>>>> +            pstate->pipe.sspp->ops.setup_csc(pstate->pipe.sspp, 
>>>> csc_ptr);
>>>>       }
>>>>       /* flag h/w flush complete */
>>>> @@ -1072,6 +1073,7 @@ static void 
>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>>       struct drm_plane_state *state = plane->state;
>>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>>>       struct drm_crtc *crtc = state->crtc;
>>>>       struct drm_framebuffer *fb = state->fb;
>>>>       bool is_rt_pipe, update_qos_remap;
>>>> @@ -1109,21 +1111,21 @@ static void 
>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>           return;
>>>>       }
>>>> -    if (pstate->pipe_hw->ops.setup_rects) {
>>>> -        pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>>>> +    if (pipe->sspp->ops.setup_rects) {
>>>> +        pipe->sspp->ops.setup_rects(pipe->sspp,
>>>>                   &pipe_cfg,
>>>> -                pstate->multirect_index);
>>>> +                pipe->multirect_index);
>>>>       }
>>>>       _dpu_plane_setup_scaler(pdpu, pstate, fmt, false, &pipe_cfg);
>>>> -    if (pstate->pipe_hw->ops.setup_multirect)
>>>> -        pstate->pipe_hw->ops.setup_multirect(
>>>> -                pstate->pipe_hw,
>>>> -                pstate->multirect_index,
>>>> -                pstate->multirect_mode);
>>>> +    if (pipe->sspp->ops.setup_multirect)
>>>> +        pipe->sspp->ops.setup_multirect(
>>>> +                pipe->sspp,
>>>> +                pipe->multirect_index,
>>>> +                pipe->multirect_mode);
>>>> -    if (pstate->pipe_hw->ops.setup_format) {
>>>> +    if (pipe->sspp->ops.setup_format) {
>>>>           unsigned int rotation;
>>>>           src_flags = 0x0;
>>>> @@ -1140,10 +1142,10 @@ static void 
>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>               src_flags |= DPU_SSPP_FLIP_UD;
>>>>           /* update format */
>>>> -        pstate->pipe_hw->ops.setup_format(pstate->pipe_hw, fmt, 
>>>> src_flags,
>>>> -                pstate->multirect_index);
>>>> +        pipe->sspp->ops.setup_format(pipe->sspp, fmt, src_flags,
>>>> +                pipe->multirect_index);
>>>> -        if (pstate->pipe_hw->ops.setup_cdp) {
>>>> +        if (pipe->sspp->ops.setup_cdp) {
>>>>               struct dpu_hw_pipe_cdp_cfg cdp_cfg;
>>>>               memset(&cdp_cfg, 0, sizeof(struct dpu_hw_pipe_cdp_cfg));
>>>> @@ -1157,7 +1159,7 @@ static void 
>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>                       DPU_FORMAT_IS_TILE(fmt);
>>>>               cdp_cfg.preload_ahead = DPU_SSPP_CDP_PRELOAD_AHEAD_64;
>>>> -            pstate->pipe_hw->ops.setup_cdp(pstate->pipe_hw, 
>>>> &cdp_cfg, pstate->multirect_index);
>>>> +            pipe->sspp->ops.setup_cdp(pipe->sspp, &cdp_cfg, 
>>>> pipe->multirect_index);
>>>>           }
>>>>       }
>>>> @@ -1191,7 +1193,7 @@ static void _dpu_plane_atomic_disable(struct 
>>>> drm_plane *plane)
>>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>>       trace_dpu_plane_disable(DRMID(plane), false,
>>>> -                pstate->multirect_mode);
>>>> +                pstate->pipe.multirect_mode);
>>>>       pstate->pending = true;
>>>>   }
>>>> @@ -1305,9 +1307,9 @@ static void 
>>>> dpu_plane_atomic_print_state(struct drm_printer *p,
>>>>       const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>>       drm_printf(p, "\tstage=%d\n", pstate->stage);
>>>> -    drm_printf(p, "\tsspp=%s\n", pstate->pipe_hw->cap->name);
>>>> -    drm_printf(p, "\tmultirect_mode=%s\n", 
>>>> dpu_get_multirect_mode(pstate->multirect_mode));
>>>> -    drm_printf(p, "\tmultirect_index=%s\n", 
>>>> dpu_get_multirect_index(pstate->multirect_index));
>>>> +    drm_printf(p, "\tsspp=%s\n", pstate->pipe.sspp->cap->name);
>>>> +    drm_printf(p, "\tmultirect_mode=%s\n", 
>>>> dpu_get_multirect_mode(pstate->pipe.multirect_mode));
>>>> +    drm_printf(p, "\tmultirect_index=%s\n", 
>>>> dpu_get_multirect_index(pstate->pipe.multirect_index));
>>>>   }
>>>>   static void dpu_plane_reset(struct drm_plane *plane)
>>>> @@ -1336,7 +1338,7 @@ static void dpu_plane_reset(struct drm_plane 
>>>> *plane)
>>>>           return;
>>>>       }
>>>> -    pstate->pipe_hw = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe - 
>>>> SSPP_NONE]);
>>>> +    pstate->pipe.sspp = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe 
>>>> - SSPP_NONE]);
>>>>       __drm_atomic_helper_plane_reset(plane, &pstate->base);
>>>>   }
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>> index d745cde4ea77..b6fd6f856d6a 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>> @@ -31,11 +31,9 @@ struct dpu_plane_state {
>>>>       struct msm_gem_address_space *aspace;
>>>>       enum dpu_stage stage;
>>>>       bool needs_qos_remap;
>>>> -    uint32_t multirect_index;
>>>> -    uint32_t multirect_mode;
>>>>       bool pending;
>>>> -    struct dpu_hw_pipe *pipe_hw;
>>>> +    struct dpu_sw_pipe pipe;
>>>
>>> Are you sure we dont need this to be a pointer?
>>>
>>> Since this is now part of plane_state, its possible that across 
>>> commits, the hw pipe can change for this plane's state. So might be 
>>> better if this is pointer in that case ....
>>
>> A pointer means that it is allocated and deallocated by somebody. 
>> Probably you meant that it is a pointer into the dpu_hw_pipe member 
>> (with dpu_hw_pipe having sw_pipe's for rect0 and rect1).
>>
>> I thought about this. And ended up with the dpu_sw_pipe having a 
>> dpu_hw_pipe pointer inside, but itself being a member of plane state. 
>> This way the plane state contains such data as multirect mode and 
>> rectangle.
> 
> Right, so what I meant was, previously pipe_hw was a pointer but it was 
> not changed dynamically because it was assigned once in dpu_plane_init() 
> but after that was not updated to point to a different pipe_hw.
> 
> Now, pipe_hw is part of the plane state which means the actual hardware 
> which the plane state is using can change across commits with virtual 
> plane support.
> 
> So it made more sense to me that this was "struct dpu_sw_pipe *pipe" 
> because looking at it from a top level it tells me that the dpu_sw_pipe 
> can point to a different pipe. Just little more intuitive.
> 
> With it being just a member, sure it still means the same thing because 
> internal to it, pipe_hw is still a pointer.
> 
> Just felt it was more logical to have the whole dpu_sw_pipe as a pointer.

If it would have been possible to allocate REC0 and REC1 to different 
LMs, then the dpu_sw_pipe would be definitely a pointer into the 
rm-managed resource. However as REC0 and REC1 are not going to be 
handled by the RM, making dpu_sw_pipe a pointer would just complicate 
the design. It will be a pointer to the dpu_hw_pipe's sub-struct. With 
this struct having a back-pointer to the dpu_hw_pipe. And some state 
which we should always clear when reassigning the pipes, etc. So it 
landed naturally as a part of pstate.

> 
> But its just a thought not a strong inclination against current patch.

R-b?

> 
>>
>>
>>>
>>>>       u64 plane_fetch_bw;
>>>>       u64 plane_clk;
>>
>>

Abhinav Kumar May 6, 2022, 10:46 p.m. UTC | #5

On 5/6/2022 3:29 PM, Dmitry Baryshkov wrote:
> On 07/05/2022 00:48, Abhinav Kumar wrote:
>>
>>
>> On 5/6/2022 2:39 PM, Dmitry Baryshkov wrote:
>>> On 07/05/2022 00:30, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 2/9/2022 9:25 AM, Dmitry Baryshkov wrote:
>>>>> Wrap SSPP and multirect index/mode into a single structure that
>>>>> represents software view on the pipe used.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |   8 +-
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  16 ++-
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 136 
>>>>> ++++++++++----------
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h   |   4 +-
>>>>>   4 files changed, 86 insertions(+), 78 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> index ada7d5750536..751c64012058 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> @@ -441,14 +441,14 @@ static void _dpu_crtc_blend_setup_ctl(struct 
>>>>> drm_crtc *crtc)
>>>>>           pstate = to_dpu_plane_state(state);
>>>>> -        sspp_idx = pstate->pipe_hw->idx;
>>>>> +        sspp_idx = pstate->pipe.sspp->idx;
>>>>>           set_bit(sspp_idx, fetch_active);
>>>>>           stage_idx = zpos_cnt[pstate->stage]++;
>>>>>           stage_cfg.stage[pstate->stage][stage_idx] =
>>>>>                       sspp_idx;
>>>>>           stage_cfg.multirect_index[pstate->stage][stage_idx] =
>>>>> -                    pstate->multirect_index;
>>>>> +                    pstate->pipe.multirect_index;
>>>>>           /* blend config update */
>>>>>           for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++)
>>>>> @@ -1118,7 +1118,7 @@ static int dpu_crtc_atomic_check(struct 
>>>>> drm_crtc *crtc,
>>>>>           pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate);
>>>>>           pstates[cnt].drm_pstate = pstate;
>>>>>           pstates[cnt].stage = pstate->normalized_zpos;
>>>>> -        pstates[cnt].pipe_id = 
>>>>> to_dpu_plane_state(pstate)->pipe_hw->idx;
>>>>> +        pstates[cnt].pipe_id = 
>>>>> to_dpu_plane_state(pstate)->pipe.sspp->idx;
>>>>>           if (pipe_staged[pstates[cnt].pipe_id]) {
>>>>>               multirect_plane[multirect_count].r0 =
>>>>> @@ -1389,7 +1389,7 @@ static int _dpu_debugfs_status_show(struct 
>>>>> seq_file *s, void *data)
>>>>>               state->crtc_x, state->crtc_y, state->crtc_w,
>>>>>               state->crtc_h);
>>>>>           seq_printf(s, "\tmultirect: mode: %d index: %d\n",
>>>>> -            pstate->multirect_mode, pstate->multirect_index);
>>>>> +            pstate->pipe.multirect_mode, 
>>>>> pstate->pipe.multirect_index);
>>>>>           seq_puts(s, "\n");
>>>>>       }
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>>> index 674f311f99b4..0af2bc6e5df8 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
>>>>> @@ -159,15 +159,11 @@ struct dpu_hw_pixel_ext {
>>>>>    * @src_rect:  src ROI, caller takes into account the different 
>>>>> operations
>>>>>    *             such as decimation, flip etc to program this field
>>>>>    * @dest_rect: destination ROI.
>>>>> - * @index:     index of the rectangle of SSPP
>>>>> - * @mode:      parallel or time multiplex multirect mode
>>>>>    */
>>>>>   struct dpu_hw_pipe_cfg {
>>>>>       struct dpu_hw_fmt_layout layout;
>>>>>       struct drm_rect src_rect;
>>>>>       struct drm_rect dst_rect;
>>>>> -    enum dpu_sspp_multirect_index index;
>>>>> -    enum dpu_sspp_multirect_mode mode;
>>>>>   };
>>>>>   /**
>>>>> @@ -218,6 +214,18 @@ struct dpu_hw_pipe_ts_cfg {
>>>>>       u64 time;
>>>>>   };
>>>>> +/**
>>>>> + * struct dpu_sw_pipe - software pipe description
>>>>> + * @sspp:      backing SSPP pipe
>>>>> + * @index:     index of the rectangle of SSPP
>>>>> + * @mode:      parallel or time multiplex multirect mode
>>>>> + */
>>>>> +struct dpu_sw_pipe {
>>>>> +    struct dpu_hw_pipe *sspp;
>>>>> +    enum dpu_sspp_multirect_index multirect_index;
>>>>> +    enum dpu_sspp_multirect_mode multirect_mode;
>>>>> +};
>>>>> +
>>>>>   /**
>>>>>    * struct dpu_hw_sspp_ops - interface to the SSPP Hw driver 
>>>>> functions
>>>>>    * Caller must call the init function to get the pipe context for 
>>>>> each pipe
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> index d1f9b4bc10ac..51b5e8a3182b 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> @@ -247,7 +247,7 @@ static int _dpu_plane_calc_fill_level(struct 
>>>>> drm_plane *plane,
>>>>>                   ((src_width + 32) * fmt->bpp);
>>>>>           }
>>>>>       } else {
>>>>> -        if (pstate->multirect_mode == DPU_SSPP_MULTIRECT_PARALLEL) {
>>>>> +        if (pstate->pipe.multirect_mode == 
>>>>> DPU_SSPP_MULTIRECT_PARALLEL) {
>>>>>               total_fl = (fixed_buff_size / 2) * 2 /
>>>>>                   ((src_width + 32) * fmt->bpp);
>>>>>           } else {
>>>>> @@ -331,7 +331,7 @@ static void _dpu_plane_set_qos_lut(struct 
>>>>> drm_plane *plane,
>>>>>               fmt ? (char *)&fmt->base.pixel_format : NULL,
>>>>>               pdpu->is_rt_pipe, total_fl, qos_lut);
>>>>> -    pstate->pipe_hw->ops.setup_creq_lut(pstate->pipe_hw, qos_lut);
>>>>> +    pstate->pipe.sspp->ops.setup_creq_lut(pstate->pipe.sspp, 
>>>>> qos_lut);
>>>>>   }
>>>>>   /**
>>>>> @@ -383,7 +383,7 @@ static void _dpu_plane_set_danger_lut(struct 
>>>>> drm_plane *plane,
>>>>>           danger_lut,
>>>>>           safe_lut);
>>>>> -    pstate->pipe_hw->ops.setup_danger_safe_lut(pstate->pipe_hw,
>>>>> +    pstate->pipe.sspp->ops.setup_danger_safe_lut(pstate->pipe.sspp,
>>>>>               danger_lut, safe_lut);
>>>>>   }
>>>>> @@ -403,9 +403,9 @@ static void _dpu_plane_set_qos_ctrl(struct 
>>>>> drm_plane *plane,
>>>>>       memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>>>>       if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>>>>> -        pipe_qos_cfg.creq_vblank = 
>>>>> pstate->pipe_hw->cap->sblk->creq_vblank;
>>>>> +        pipe_qos_cfg.creq_vblank = 
>>>>> pstate->pipe.sspp->cap->sblk->creq_vblank;
>>>>>           pipe_qos_cfg.danger_vblank =
>>>>> -                pstate->pipe_hw->cap->sblk->danger_vblank;
>>>>> +                pstate->pipe.sspp->cap->sblk->danger_vblank;
>>>>>           pipe_qos_cfg.vblank_en = enable;
>>>>>       }
>>>>> @@ -431,7 +431,7 @@ static void _dpu_plane_set_qos_ctrl(struct 
>>>>> drm_plane *plane,
>>>>>           pipe_qos_cfg.danger_vblank,
>>>>>           pdpu->is_rt_pipe);
>>>>> -    pstate->pipe_hw->ops.setup_qos_ctrl(pstate->pipe_hw,
>>>>> +    pstate->pipe.sspp->ops.setup_qos_ctrl(pstate->pipe.sspp,
>>>>>               &pipe_qos_cfg);
>>>>>   }
>>>>> @@ -450,14 +450,14 @@ static void _dpu_plane_set_ot_limit(struct 
>>>>> drm_plane *plane,
>>>>>       struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
>>>>>       memset(&ot_params, 0, sizeof(ot_params));
>>>>> -    ot_params.xin_id = pstate->pipe_hw->cap->xin_id;
>>>>> -    ot_params.num = pstate->pipe_hw->idx - SSPP_NONE;
>>>>> +    ot_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>>>>> +    ot_params.num = pstate->pipe.sspp->idx - SSPP_NONE;
>>>>>       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.vbif_idx = VBIF_RT;
>>>>> -    ot_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>>>>> +    ot_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>>>>>       ot_params.rd = true;
>>>>>       dpu_vbif_set_ot_limit(dpu_kms, &ot_params);
>>>>> @@ -476,9 +476,9 @@ static void _dpu_plane_set_qos_remap(struct 
>>>>> drm_plane *plane)
>>>>>       memset(&qos_params, 0, sizeof(qos_params));
>>>>>       qos_params.vbif_idx = VBIF_RT;
>>>>> -    qos_params.clk_ctrl = pstate->pipe_hw->cap->clk_ctrl;
>>>>> -    qos_params.xin_id = pstate->pipe_hw->cap->xin_id;
>>>>> -    qos_params.num = pstate->pipe_hw->idx - SSPP_VIG0;
>>>>> +    qos_params.clk_ctrl = pstate->pipe.sspp->cap->clk_ctrl;
>>>>> +    qos_params.xin_id = pstate->pipe.sspp->cap->xin_id;
>>>>> +    qos_params.num = pstate->pipe.sspp->idx - SSPP_VIG0;
>>>>>       qos_params.is_rt = pdpu->is_rt_pipe;
>>>>>       DPU_DEBUG_PLANE(pdpu, "pipe:%d vbif:%d xin:%d rt:%d, 
>>>>> clk_ctrl:%d\n",
>>>>> @@ -505,12 +505,12 @@ static void _dpu_plane_set_scanout(struct 
>>>>> drm_plane *plane,
>>>>>           DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
>>>>>       else if (ret)
>>>>>           DPU_ERROR_PLANE(pdpu, "failed to get format layout, 
>>>>> %d\n", ret);
>>>>> -    else if (pstate->pipe_hw->ops.setup_sourceaddress) {
>>>>> -        trace_dpu_plane_set_scanout(pstate->pipe_hw->idx,
>>>>> +    else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
>>>>> +        trace_dpu_plane_set_scanout(pstate->pipe.sspp->idx,
>>>>>                           &pipe_cfg->layout,
>>>>> -                        pstate->multirect_index);
>>>>> -        pstate->pipe_hw->ops.setup_sourceaddress(pstate->pipe_hw, 
>>>>> pipe_cfg,
>>>>> -                        pstate->multirect_index);
>>>>> +                        pstate->pipe.multirect_index);
>>>>> + pstate->pipe.sspp->ops.setup_sourceaddress(pstate->pipe.sspp, 
>>>>> pipe_cfg,
>>>>> +                        pstate->pipe.multirect_index);
>>>>>       }
>>>>>   }
>>>>> @@ -553,7 +553,7 @@ static void _dpu_plane_setup_scaler3(struct 
>>>>> dpu_plane *pdpu,
>>>>>               scale_cfg->src_height[i] /= chroma_subsmpl_v;
>>>>>           }
>>>>> -        if (pstate->pipe_hw->cap->features &
>>>>> +        if (pstate->pipe.sspp->cap->features &
>>>>>               BIT(DPU_SSPP_SCALER_QSEED4)) {
>>>>>               scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
>>>>>               scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
>>>>> @@ -624,7 +624,7 @@ static const struct dpu_csc_cfg 
>>>>> *_dpu_plane_get_csc(struct dpu_plane *pdpu, cons
>>>>>       if (!DPU_FORMAT_IS_YUV(fmt))
>>>>>           return NULL;
>>>>> -    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe_hw->cap->features)
>>>>> +    if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe.sspp->cap->features)
>>>>>           csc_ptr = &dpu_csc10_YUV2RGB_601L;
>>>>>       else
>>>>>           csc_ptr = &dpu_csc_YUV2RGB_601L;
>>>>> @@ -659,8 +659,8 @@ static void _dpu_plane_setup_scaler(struct 
>>>>> dpu_plane *pdpu,
>>>>>               &scaler3_cfg, &pixel_ext, fmt,
>>>>>               info->hsub, info->vsub);
>>>>> -    if (pstate->pipe_hw->ops.setup_pe)
>>>>> -        pstate->pipe_hw->ops.setup_pe(pstate->pipe_hw,
>>>>> +    if (pstate->pipe.sspp->ops.setup_pe)
>>>>> +        pstate->pipe.sspp->ops.setup_pe(pstate->pipe.sspp,
>>>>>                   &pixel_ext);
>>>>>       /**
>>>>> @@ -668,9 +668,9 @@ static void _dpu_plane_setup_scaler(struct 
>>>>> dpu_plane *pdpu,
>>>>>        * bypassed. Still we need to update alpha and bitwidth
>>>>>        * ONLY for RECT0
>>>>>        */
>>>>> -    if (pstate->pipe_hw->ops.setup_scaler &&
>>>>> -            pstate->multirect_index != DPU_SSPP_RECT_1)
>>>>> -        pstate->pipe_hw->ops.setup_scaler(pstate->pipe_hw,
>>>>> +    if (pstate->pipe.sspp->ops.setup_scaler &&
>>>>> +            pstate->pipe.multirect_index != DPU_SSPP_RECT_1)
>>>>> +        pstate->pipe.sspp->ops.setup_scaler(pstate->pipe.sspp,
>>>>>                   pipe_cfg,
>>>>>                   &scaler3_cfg);
>>>>>   }
>>>>> @@ -699,10 +699,10 @@ static int _dpu_plane_color_fill(struct 
>>>>> dpu_plane *pdpu,
>>>>>       fmt = dpu_get_dpu_format(DRM_FORMAT_ABGR8888);
>>>>>       /* update sspp */
>>>>> -    if (fmt && pstate->pipe_hw->ops.setup_solidfill) {
>>>>> -        pstate->pipe_hw->ops.setup_solidfill(pstate->pipe_hw,
>>>>> +    if (fmt && pstate->pipe.sspp->ops.setup_solidfill) {
>>>>> +        pstate->pipe.sspp->ops.setup_solidfill(pstate->pipe.sspp,
>>>>>                   (color & 0xFFFFFF) | ((alpha & 0xFF) << 24),
>>>>> -                pstate->multirect_index);
>>>>> +                pstate->pipe.multirect_index);
>>>>>           /* override scaler/decimation if solid fill */
>>>>>           pipe_cfg.dst_rect = pstate->base.dst;
>>>>> @@ -714,15 +714,15 @@ static int _dpu_plane_color_fill(struct 
>>>>> dpu_plane *pdpu,
>>>>>           pipe_cfg.src_rect.y2 =
>>>>>               drm_rect_height(&pipe_cfg.dst_rect);
>>>>> -        if (pstate->pipe_hw->ops.setup_format)
>>>>> -            pstate->pipe_hw->ops.setup_format(pstate->pipe_hw,
>>>>> +        if (pstate->pipe.sspp->ops.setup_format)
>>>>> +            pstate->pipe.sspp->ops.setup_format(pstate->pipe.sspp,
>>>>>                       fmt, DPU_SSPP_SOLID_FILL,
>>>>> -                    pstate->multirect_index);
>>>>> +                    pstate->pipe.multirect_index);
>>>>> -        if (pstate->pipe_hw->ops.setup_rects)
>>>>> -            pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>>>>> +        if (pstate->pipe.sspp->ops.setup_rects)
>>>>> +            pstate->pipe.sspp->ops.setup_rects(pstate->pipe.sspp,
>>>>>                       &pipe_cfg,
>>>>> -                    pstate->multirect_index);
>>>>> +                    pstate->pipe.multirect_index);
>>>>>           _dpu_plane_setup_scaler(pdpu, pstate, fmt, true, &pipe_cfg);
>>>>>       }
>>>>> @@ -734,8 +734,8 @@ void dpu_plane_clear_multirect(const struct 
>>>>> drm_plane_state *drm_state)
>>>>>   {
>>>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>>>>> -    pstate->multirect_index = DPU_SSPP_RECT_SOLO;
>>>>> -    pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>> +    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)
>>>>> @@ -817,8 +817,8 @@ int dpu_plane_validate_multirect_v2(struct 
>>>>> dpu_multirect_plane_states *plane)
>>>>>       /* Prefer PARALLEL FETCH Mode over TIME_MX Mode */
>>>>>       if (parallel_fetch_qualified) {
>>>>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>>> +        pstate[R0]->pipe.multirect_mode = 
>>>>> DPU_SSPP_MULTIRECT_PARALLEL;
>>>>> +        pstate[R1]->pipe.multirect_mode = 
>>>>> DPU_SSPP_MULTIRECT_PARALLEL;
>>>>>           goto done;
>>>>>       }
>>>>> @@ -828,8 +828,8 @@ int dpu_plane_validate_multirect_v2(struct 
>>>>> dpu_multirect_plane_states *plane)
>>>>>       if (dst[R1].y1 >= dst[R0].y2 + buffer_lines ||
>>>>>           dst[R0].y1 >= dst[R1].y2 + buffer_lines) {
>>>>> -        pstate[R0]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>>> -        pstate[R1]->multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>>> +        pstate[R0]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>>> +        pstate[R1]->pipe.multirect_mode = DPU_SSPP_MULTIRECT_TIME_MX;
>>>>>       } else {
>>>>>           DPU_ERROR(
>>>>>               "No multirect mode possible for the planes (%d - %d)\n",
>>>>> @@ -839,13 +839,13 @@ int dpu_plane_validate_multirect_v2(struct 
>>>>> dpu_multirect_plane_states *plane)
>>>>>       }
>>>>>   done:
>>>>> -    pstate[R0]->multirect_index = DPU_SSPP_RECT_0;
>>>>> -    pstate[R1]->multirect_index = DPU_SSPP_RECT_1;
>>>>> +    pstate[R0]->pipe.multirect_index = DPU_SSPP_RECT_0;
>>>>> +    pstate[R1]->pipe.multirect_index = DPU_SSPP_RECT_1;
>>>>>       DPU_DEBUG_PLANE(dpu_plane[R0], "R0: %d - %d\n",
>>>>> -        pstate[R0]->multirect_mode, pstate[R0]->multirect_index);
>>>>> +        pstate[R0]->pipe.multirect_mode, 
>>>>> pstate[R0]->pipe.multirect_index);
>>>>>       DPU_DEBUG_PLANE(dpu_plane[R1], "R1: %d - %d\n",
>>>>> -        pstate[R1]->multirect_mode, pstate[R1]->multirect_index);
>>>>> +        pstate[R1]->pipe.multirect_mode, 
>>>>> pstate[R1]->pipe.multirect_index);
>>>>>       return 0;
>>>>>   }
>>>>> @@ -937,6 +937,7 @@ static int dpu_plane_atomic_check(struct 
>>>>> drm_plane *plane,
>>>>>       int ret = 0, min_scale;
>>>>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>>>       struct dpu_plane_state *pstate = 
>>>>> to_dpu_plane_state(new_plane_state);
>>>>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>>>>       const struct drm_crtc_state *crtc_state = NULL;
>>>>>       const struct dpu_format *fmt;
>>>>>       struct drm_rect src, dst, fb_rect = { 0 };
>>>>> @@ -946,10 +947,10 @@ static int dpu_plane_atomic_check(struct 
>>>>> drm_plane *plane,
>>>>>           crtc_state = drm_atomic_get_new_crtc_state(state,
>>>>>                                  new_plane_state->crtc);
>>>>> -    min_scale = FRAC_16_16(1, 
>>>>> pstate->pipe_hw->cap->sblk->maxupscale);
>>>>> +    min_scale = FRAC_16_16(1, pipe->sspp->cap->sblk->maxupscale);
>>>>>       ret = drm_atomic_helper_check_plane_state(new_plane_state, 
>>>>> crtc_state,
>>>>>                             min_scale,
>>>>> -                          pstate->pipe_hw->cap->sblk->maxdwnscale 
>>>>> << 16,
>>>>> +                          pipe->sspp->cap->sblk->maxdwnscale << 16,
>>>>>                             true, true);
>>>>>       if (ret) {
>>>>>           DPU_DEBUG_PLANE(pdpu, "Check plane state failed (%d)\n", 
>>>>> ret);
>>>>> @@ -975,8 +976,8 @@ static int dpu_plane_atomic_check(struct 
>>>>> drm_plane *plane,
>>>>>       min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>>>       if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>> -        (!(pstate->pipe_hw->cap->features & DPU_SSPP_SCALER) ||
>>>>> -         !(pstate->pipe_hw->cap->features & DPU_SSPP_CSC_ANY))) {
>>>>> +        (!(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;
>>>>> @@ -1037,12 +1038,12 @@ void dpu_plane_flush(struct drm_plane *plane)
>>>>>       else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>>>>>           /* force 100% alpha */
>>>>>           _dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
>>>>> -    else if (pstate->pipe_hw && pstate->pipe_hw->ops.setup_csc) {
>>>>> +    else if (pstate->pipe.sspp && pstate->pipe.sspp->ops.setup_csc) {
>>>>>           const struct dpu_format *fmt = 
>>>>> to_dpu_format(msm_framebuffer_format(plane->state->fb));
>>>>>           const struct dpu_csc_cfg *csc_ptr = 
>>>>> _dpu_plane_get_csc(pdpu, fmt);
>>>>>           if (csc_ptr)
>>>>> -            pstate->pipe_hw->ops.setup_csc(pstate->pipe_hw, csc_ptr);
>>>>> +            pstate->pipe.sspp->ops.setup_csc(pstate->pipe.sspp, 
>>>>> csc_ptr);
>>>>>       }
>>>>>       /* flag h/w flush complete */
>>>>> @@ -1072,6 +1073,7 @@ static void 
>>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>>       struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>>>       struct drm_plane_state *state = plane->state;
>>>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>>> +    struct dpu_sw_pipe *pipe = &pstate->pipe;
>>>>>       struct drm_crtc *crtc = state->crtc;
>>>>>       struct drm_framebuffer *fb = state->fb;
>>>>>       bool is_rt_pipe, update_qos_remap;
>>>>> @@ -1109,21 +1111,21 @@ static void 
>>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>>           return;
>>>>>       }
>>>>> -    if (pstate->pipe_hw->ops.setup_rects) {
>>>>> -        pstate->pipe_hw->ops.setup_rects(pstate->pipe_hw,
>>>>> +    if (pipe->sspp->ops.setup_rects) {
>>>>> +        pipe->sspp->ops.setup_rects(pipe->sspp,
>>>>>                   &pipe_cfg,
>>>>> -                pstate->multirect_index);
>>>>> +                pipe->multirect_index);
>>>>>       }
>>>>>       _dpu_plane_setup_scaler(pdpu, pstate, fmt, false, &pipe_cfg);
>>>>> -    if (pstate->pipe_hw->ops.setup_multirect)
>>>>> -        pstate->pipe_hw->ops.setup_multirect(
>>>>> -                pstate->pipe_hw,
>>>>> -                pstate->multirect_index,
>>>>> -                pstate->multirect_mode);
>>>>> +    if (pipe->sspp->ops.setup_multirect)
>>>>> +        pipe->sspp->ops.setup_multirect(
>>>>> +                pipe->sspp,
>>>>> +                pipe->multirect_index,
>>>>> +                pipe->multirect_mode);
>>>>> -    if (pstate->pipe_hw->ops.setup_format) {
>>>>> +    if (pipe->sspp->ops.setup_format) {
>>>>>           unsigned int rotation;
>>>>>           src_flags = 0x0;
>>>>> @@ -1140,10 +1142,10 @@ static void 
>>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>>               src_flags |= DPU_SSPP_FLIP_UD;
>>>>>           /* update format */
>>>>> -        pstate->pipe_hw->ops.setup_format(pstate->pipe_hw, fmt, 
>>>>> src_flags,
>>>>> -                pstate->multirect_index);
>>>>> +        pipe->sspp->ops.setup_format(pipe->sspp, fmt, src_flags,
>>>>> +                pipe->multirect_index);
>>>>> -        if (pstate->pipe_hw->ops.setup_cdp) {
>>>>> +        if (pipe->sspp->ops.setup_cdp) {
>>>>>               struct dpu_hw_pipe_cdp_cfg cdp_cfg;
>>>>>               memset(&cdp_cfg, 0, sizeof(struct dpu_hw_pipe_cdp_cfg));
>>>>> @@ -1157,7 +1159,7 @@ static void 
>>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>>                       DPU_FORMAT_IS_TILE(fmt);
>>>>>               cdp_cfg.preload_ahead = DPU_SSPP_CDP_PRELOAD_AHEAD_64;
>>>>> -            pstate->pipe_hw->ops.setup_cdp(pstate->pipe_hw, 
>>>>> &cdp_cfg, pstate->multirect_index);
>>>>> +            pipe->sspp->ops.setup_cdp(pipe->sspp, &cdp_cfg, 
>>>>> pipe->multirect_index);
>>>>>           }
>>>>>       }
>>>>> @@ -1191,7 +1193,7 @@ static void _dpu_plane_atomic_disable(struct 
>>>>> drm_plane *plane)
>>>>>       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>>>       trace_dpu_plane_disable(DRMID(plane), false,
>>>>> -                pstate->multirect_mode);
>>>>> +                pstate->pipe.multirect_mode);
>>>>>       pstate->pending = true;
>>>>>   }
>>>>> @@ -1305,9 +1307,9 @@ static void 
>>>>> dpu_plane_atomic_print_state(struct drm_printer *p,
>>>>>       const struct dpu_plane_state *pstate = 
>>>>> to_dpu_plane_state(state);
>>>>>       drm_printf(p, "\tstage=%d\n", pstate->stage);
>>>>> -    drm_printf(p, "\tsspp=%s\n", pstate->pipe_hw->cap->name);
>>>>> -    drm_printf(p, "\tmultirect_mode=%s\n", 
>>>>> dpu_get_multirect_mode(pstate->multirect_mode));
>>>>> -    drm_printf(p, "\tmultirect_index=%s\n", 
>>>>> dpu_get_multirect_index(pstate->multirect_index));
>>>>> +    drm_printf(p, "\tsspp=%s\n", pstate->pipe.sspp->cap->name);
>>>>> +    drm_printf(p, "\tmultirect_mode=%s\n", 
>>>>> dpu_get_multirect_mode(pstate->pipe.multirect_mode));
>>>>> +    drm_printf(p, "\tmultirect_index=%s\n", 
>>>>> dpu_get_multirect_index(pstate->pipe.multirect_index));
>>>>>   }
>>>>>   static void dpu_plane_reset(struct drm_plane *plane)
>>>>> @@ -1336,7 +1338,7 @@ static void dpu_plane_reset(struct drm_plane 
>>>>> *plane)
>>>>>           return;
>>>>>       }
>>>>> -    pstate->pipe_hw = to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe 
>>>>> - SSPP_NONE]);
>>>>> +    pstate->pipe.sspp = 
>>>>> to_dpu_hw_pipe(kms->rm.sspp_blks[pdpu->pipe - SSPP_NONE]);
>>>>>       __drm_atomic_helper_plane_reset(plane, &pstate->base);
>>>>>   }
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> index d745cde4ea77..b6fd6f856d6a 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> @@ -31,11 +31,9 @@ struct dpu_plane_state {
>>>>>       struct msm_gem_address_space *aspace;
>>>>>       enum dpu_stage stage;
>>>>>       bool needs_qos_remap;
>>>>> -    uint32_t multirect_index;
>>>>> -    uint32_t multirect_mode;
>>>>>       bool pending;
>>>>> -    struct dpu_hw_pipe *pipe_hw;
>>>>> +    struct dpu_sw_pipe pipe;
>>>>
>>>> Are you sure we dont need this to be a pointer?
>>>>
>>>> Since this is now part of plane_state, its possible that across 
>>>> commits, the hw pipe can change for this plane's state. So might be 
>>>> better if this is pointer in that case ....
>>>
>>> A pointer means that it is allocated and deallocated by somebody. 
>>> Probably you meant that it is a pointer into the dpu_hw_pipe member 
>>> (with dpu_hw_pipe having sw_pipe's for rect0 and rect1).
>>>
>>> I thought about this. And ended up with the dpu_sw_pipe having a 
>>> dpu_hw_pipe pointer inside, but itself being a member of plane state. 
>>> This way the plane state contains such data as multirect mode and 
>>> rectangle.
>>
>> Right, so what I meant was, previously pipe_hw was a pointer but it 
>> was not changed dynamically because it was assigned once in 
>> dpu_plane_init() but after that was not updated to point to a 
>> different pipe_hw.
>>
>> Now, pipe_hw is part of the plane state which means the actual 
>> hardware which the plane state is using can change across commits with 
>> virtual plane support.
>>
>> So it made more sense to me that this was "struct dpu_sw_pipe *pipe" 
>> because looking at it from a top level it tells me that the 
>> dpu_sw_pipe can point to a different pipe. Just little more intuitive.
>>
>> With it being just a member, sure it still means the same thing 
>> because internal to it, pipe_hw is still a pointer.
>>
>> Just felt it was more logical to have the whole dpu_sw_pipe as a pointer.
> 
> If it would have been possible to allocate REC0 and REC1 to different 
> LMs, then the dpu_sw_pipe would be definitely a pointer into the 
> rm-managed resource. However as REC0 and REC1 are not going to be 
> handled by the RM, making dpu_sw_pipe a pointer would just complicate 
> the design. It will be a pointer to the dpu_hw_pipe's sub-struct. With 
> this struct having a back-pointer to the dpu_hw_pipe. And some state 
> which we should always clear when reassigning the pipes, etc. So it 
> landed naturally as a part of pstate.
> 

Alright, understood. Lets see if this has to evolve further, but at the 
moment this is fine with me.

>>
>> But its just a thought not a strong inclination against current patch.
> 
> R-b?
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
>>
>>>
>>>
>>>>
>>>>>       u64 plane_fetch_bw;
>>>>>       u64 plane_clk;
>>>
>>>
> 
>

[10/25] drm/msm/dpu: introduce struct dpu_sw_pipe

Commit Message

Comments

Patch