[v3,27/27] drm/msm/dpu: add support for wide planes

Commit Message

Dmitry Baryshkov Feb. 3, 2023, 6:21 p.m. UTC

Typically SSPP can support rectangle with width up to 2560. However it's
possible to use multirect feature and split source to use the SSPP to
output two consecutive rectangles. This commit brings in this capability
to support wider screen resolutions.

Signed-off-by: Dmitry Baryshkov <[e-mail]>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
 3 files changed, 114 insertions(+), 12 deletions(-)

Comments

Abhinav Kumar Feb. 9, 2023, 2:19 a.m. UTC | #1

On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> Typically SSPP can support rectangle with width up to 2560. However it's

Not always 2560. Depends on the chipset.

> possible to use multirect feature and split source to use the SSPP to
> output two consecutive rectangles. This commit brings in this capability
> to support wider screen resolutions.
> 
> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
>   3 files changed, 114 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 0ca3bc38ff7e..867832a752b2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -485,6 +485,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>   					   fetch_active,
>   					   &pstate->pipe);
>   
> +		_dpu_crtc_blend_setup_pipe(crtc, plane,
> +					   mixer, cstate->num_mixers,
> +					   stage_cfg, pstate->stage, 1,
> +					   fetch_active,
> +					   &pstate->r_pipe);
> +
>   		/* blend config update */
>   		for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
>   			_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index e2e85688ed3c..401ead64c6bd 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>   	struct dpu_plane *pdpu = to_dpu_plane(plane);
>   	struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
>   
> +	if (!pipe->sspp)
> +		return;
> +
>   	memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>   
>   	if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
> @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
>   {
>   	struct dpu_hw_sspp_cfg pipe_cfg;
>   
> +	if (!pipe->sspp)
> +		return 0;

instead of checking if sspp was present, is it not better for the caller 
to check if the rpipe is valid before calling this?

> +
>   	/* update sspp */
>   	if (!pipe->sspp->ops.setup_solidfill)
>   		return 0;
> @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
>   
>   	/* update sspp */
>   	_dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
> +
> +	_dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
>   }

So cant we do

if (pstate->r_pipe.sspp)
	_dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, 		
		&pstate->r_pipe_cfg, fill_color, fmt);

It just seems better to me as the caller would already know if the sspp 
was assigned.

>   
>   int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
> @@ -911,6 +919,9 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
>   {
>   	uint32_t min_src_size;
>   
> +	if (!pipe->sspp)
> +		return 0;
> +
>   	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>   
>   	if (DPU_FORMAT_IS_YUV(fmt) &&
> @@ -957,9 +968,12 @@ 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;
> +	struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>   	const struct drm_crtc_state *crtc_state = NULL;
>   	const struct dpu_format *fmt;
>   	struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> +	struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>   	struct drm_rect fb_rect = { 0 };
>   	uint32_t max_linewidth;
>   	unsigned int rotation;
> @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   	if (!new_plane_state->visible)
>   		return 0;
>   
> -	pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> -	pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> +	pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> +	pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> +	r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> +	r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> +	r_pipe->sspp = NULL;
>   
>   	pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
>   	if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
> @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   
>   	max_linewidth = pdpu->catalog->caps->max_linewidth;
>   
> -	/* check decimated source width */
>   	if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
> -		DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> -				DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> -		return -E2BIG;
> +		/* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
> +
> +		if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
> +			DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> +					DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> +			return -E2BIG;
> +		}

This is where I am a bit concerned enabling it for all chipsets in one go.

As you are aware,  we have an open bug today that we do not filter out 
the modes which we do not support.

https://gitlab.freedesktop.org/drm/msm/-/issues/21

Due to this, on all chipsets we will end up trying to do a 4K on 
external display which we dont know what bugs it will expose.

So lets say if we test it on sc7280 fully but not on sc7180, we will 
still hit this condition on sc7180 too but on that chipset we did not 
advertise 4K as a capability in the product spec.

With the max_linewidth check relaxed nothing prevents us from doing 4K 
on a chipset which doesnt support 4K.

> +
> +		/*
> +		 * FIXME: it's not possible to check if sourcesplit is supported,
> +		 * LMs is not assigned yet. It happens in dpu_encoder_virt_mode_set
> +		 */
> +		if (drm_rect_width(&pipe_cfg->src_rect) != drm_rect_width(&pipe_cfg->dst_rect) ||
> +			   drm_rect_height(&pipe_cfg->src_rect) != drm_rect_height(&pipe_cfg->dst_rect) ||
> +			   (!test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) &&
> +			    !test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features)) ||
> +			   /* cstate->num_mixers < 2 ||
> +			   !test_bit(DPU_MIXER_SOURCESPLIT, &cstate->mixers[0].hw_lm->cap->features) || */
> +			   DPU_FORMAT_IS_YUV(fmt)) {
> +			DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, can't use split source\n",
> +					DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> +			return -E2BIG;
> +		}
> +
> +		/* Use multirect for wide plane. We do not support dynamic assignment of SSPPs, so we know the configuration. */
> +		pipe->multirect_index = DPU_SSPP_RECT_0;
> +		pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
> +
> +		r_pipe->sspp = pipe->sspp;
> +		r_pipe->multirect_index = DPU_SSPP_RECT_1;
> +		r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;


> +
> +		*r_pipe_cfg = *pipe_cfg;
> +		pipe_cfg->src_rect.x2 = (pipe_cfg->src_rect.x1 + pipe_cfg->src_rect.x2) >> 1;
> +		pipe_cfg->dst_rect.x2 = (pipe_cfg->dst_rect.x1 + pipe_cfg->dst_rect.x2) >> 1;
> +		r_pipe_cfg->src_rect.x1 = pipe_cfg->src_rect.x2;
> +		r_pipe_cfg->dst_rect.x1 = pipe_cfg->dst_rect.x2;
>   	}
>   

As you requested just wanted to summarize the condition in the email.

In parallel fetch mode, the downstream driver for UBWC formats, we check 
whether the src width of each rectangle is > maxlinewidth/2

https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r3-00500-WAIPIO.0/msm/sde/sde_plane.c#L1835

For sc7280, maxlinewidth is 2400

static const struct dpu_caps sc7280_dpu_caps = {
         .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
         .max_mixer_blendstages = 0x7,
         .qseed_type = DPU_SSPP_SCALER_QSEED4,
         .smart_dma_rev = DPU_SSPP_SMART_DMA_V2,
         .ubwc_version = DPU_HW_UBWC_VER_30,
         .has_dim_layer = true,
         .has_idle_pc = true,
         .max_linewidth = 2400,
         .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
};

Hence for UBWC formats which are by default used on the sc7280 
chromebook, each rectangle should be < 1200

SmartDMA is therefore not enough to support 4K on sc7280 and we need 
true virtual planes ( using two SSPPs to display the 4K layer )

Also, probably worth commenting that time multiplex mode support is not 
added in this series.

>   	fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>   
> -	ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt);
> +	ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt);
> +	if (ret)
> +		return ret;
> +
> +	ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt);
>   	if (ret)
>   		return ret;
>   
> @@ -1094,8 +1148,10 @@ 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
> +	else {
>   		dpu_plane_flush_csc(pdpu, &pstate->pipe);
> +		dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
> +	}
>   
>   	/* flag h/w flush complete */
>   	if (plane->state)
> @@ -1130,6 +1186,9 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
>   	struct drm_plane_state *state = plane->state;
>   	struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>   
> +	if (!pipe->sspp)
> +		return;
> +
>   	if (layout && pipe->sspp->ops.setup_sourceaddress) {
>   		trace_dpu_plane_set_scanout(pipe, layout);
>   		pipe->sspp->ops.setup_sourceaddress(pipe, layout);
> @@ -1207,13 +1266,14 @@ static void dpu_plane_sspp_atomic_update(struct drm_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 dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>   	struct drm_crtc *crtc = state->crtc;
>   	struct drm_framebuffer *fb = state->fb;
>   	bool is_rt_pipe;
>   	const struct dpu_format *fmt =
>   		to_dpu_format(msm_framebuffer_format(fb));
>   	struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> -
> +	struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>   	struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
>   	struct msm_gem_address_space *aspace = kms->base.aspace;
>   	struct dpu_hw_fmt_layout layout;
> @@ -1241,12 +1301,22 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   				   drm_mode_vrefresh(&crtc->mode),
>   				   layout_valid ? &layout: NULL);
>   
> +	dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt,
> +				   drm_mode_vrefresh(&crtc->mode),
> +				   layout_valid ? &layout: NULL);
> +
>   	if (pstate->needs_qos_remap)
>   		pstate->needs_qos_remap = false;
>   
>   	pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, pipe_cfg);
>   
>   	pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, pipe_cfg);
> +
> +	if (r_pipe->sspp) {
> +		pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, r_pipe_cfg);
> +
> +		pstate->plane_clk = max(pstate->plane_clk, _dpu_plane_calc_clk(&crtc->mode, r_pipe_cfg));
> +	}
>   }
>   
>   static void _dpu_plane_atomic_disable(struct drm_plane *plane)
> @@ -1289,6 +1359,8 @@ static void dpu_plane_destroy(struct drm_plane *plane)
>   		pstate = to_dpu_plane_state(plane->state);
>   		_dpu_plane_set_qos_ctrl(plane, &pstate->pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
>   
> +		_dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
> +
>   		mutex_destroy(&pdpu->lock);
>   
>   		/* this will destroy the states as well */
> @@ -1369,11 +1441,26 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p,
>   		const struct drm_plane_state *state)
>   {
>   	const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> +	const struct dpu_sw_pipe *pipe = &pstate->pipe;
> +	const struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> +	const struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> +	const struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>   
>   	drm_printf(p, "\tstage=%d\n", pstate->stage);
> -	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));
> +
> +	drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name);
> +	drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode));
> +	drm_printf(p, "\tmultirect_index[0]=%s\n", dpu_get_multirect_index(pipe->multirect_index));
> +	drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect));
> +	drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect));
> +
> +	if (r_pipe->sspp) {
> +		drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name);
> +		drm_printf(p, "\tmultirect_mode[1]=%s\n", dpu_get_multirect_mode(r_pipe->multirect_mode));
> +		drm_printf(p, "\tmultirect_index[1]=%s\n", dpu_get_multirect_index(r_pipe->multirect_index));
> +		drm_printf(p, "\tsrc[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->src_rect));
> +		drm_printf(p, "\tdst[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->dst_rect));
> +	}
>   }

Do you think that changing the atomic_print_state to print the r_pipe 
sspp can be moved to a separate patch? So that way we only keep the core 
logic of atomic check of smartDMA in this patch.

>   
>   static void dpu_plane_reset(struct drm_plane *plane)
> @@ -1407,6 +1494,10 @@ static void dpu_plane_reset(struct drm_plane *plane)
>   	 * This is the place where the state is allocated, so fill it fully.
>   	 */
>   	pstate->pipe.sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe);
> +	pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> +	pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> +
> +	pstate->r_pipe.sspp = NULL;
>   
>   	__drm_atomic_helper_plane_reset(plane, &pstate->base);
>   }
> @@ -1423,6 +1514,7 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>   
>   	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>   	_dpu_plane_set_qos_ctrl(plane, &pstate->pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
> +	_dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
>   #endif
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> index 079dad83eb37..183c95949885 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> @@ -19,7 +19,9 @@
>    * @base:	base drm plane state object
>    * @aspace:	pointer to address space for input/output buffers
>    * @pipe:	software pipe description
> + * @r_pipe:	software pipe description of the second pipe
>    * @pipe_cfg:	software pipe configuration
> + * @r_pipe_cfg:	software pipe configuration for the second pipe
>    * @stage:	assigned by crtc blender
>    * @needs_qos_remap: qos remap settings need to be updated
>    * @multirect_index: index of the rectangle of SSPP
> @@ -34,7 +36,9 @@ struct dpu_plane_state {
>   	struct drm_plane_state base;
>   	struct msm_gem_address_space *aspace;
>   	struct dpu_sw_pipe pipe;
> +	struct dpu_sw_pipe r_pipe;
>   	struct dpu_hw_sspp_cfg pipe_cfg;
> +	struct dpu_hw_sspp_cfg r_pipe_cfg;
>   	enum dpu_stage stage;
>   	bool needs_qos_remap;
>   	bool pending;

Dmitry Baryshkov Feb. 9, 2023, 11:45 a.m. UTC | #2

On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar <[e-mail]> wrote:
>
>
>
> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> > Typically SSPP can support rectangle with width up to 2560. However it's
>
> Not always 2560. Depends on the chipset.

_typically_

>
> > possible to use multirect feature and split source to use the SSPP to
> > output two consecutive rectangles. This commit brings in this capability
> > to support wider screen resolutions.
> >
> > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
> >   3 files changed, 114 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 0ca3bc38ff7e..867832a752b2 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -485,6 +485,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
> >                                          fetch_active,
> >                                          &pstate->pipe);
> >
> > +             _dpu_crtc_blend_setup_pipe(crtc, plane,
> > +                                        mixer, cstate->num_mixers,
> > +                                        stage_cfg, pstate->stage, 1,
> > +                                        fetch_active,
> > +                                        &pstate->r_pipe);
> > +
> >               /* blend config update */
> >               for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
> >                       _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index e2e85688ed3c..401ead64c6bd 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
> >       struct dpu_plane *pdpu = to_dpu_plane(plane);
> >       struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
> >
> > +     if (!pipe->sspp)
> > +             return;
> > +
> >       memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
> >
> >       if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
> > @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
> >   {
> >       struct dpu_hw_sspp_cfg pipe_cfg;
> >
> > +     if (!pipe->sspp)
> > +             return 0;
>
> instead of checking if sspp was present, is it not better for the caller
> to check if the rpipe is valid before calling this?
>
> > +
> >       /* update sspp */
> >       if (!pipe->sspp->ops.setup_solidfill)
> >               return 0;
> > @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
> >
> >       /* update sspp */
> >       _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
> > +
> > +     _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
> >   }
>
> So cant we do
>
> if (pstate->r_pipe.sspp)
>         _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
>                 &pstate->r_pipe_cfg, fill_color, fmt);
>
> It just seems better to me as the caller would already know if the sspp
> was assigned.

 I think I had this kind of code earlier, but then I found it more
logical to move the check to the called function. I'll move it back.

>
> >
> >   int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
> > @@ -911,6 +919,9 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
> >   {
> >       uint32_t min_src_size;
> >
> > +     if (!pipe->sspp)
> > +             return 0;
> > +
> >       min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> >
> >       if (DPU_FORMAT_IS_YUV(fmt) &&
> > @@ -957,9 +968,12 @@ 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;
> > +     struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> >       const struct drm_crtc_state *crtc_state = NULL;
> >       const struct dpu_format *fmt;
> >       struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> > +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> >       struct drm_rect fb_rect = { 0 };
> >       uint32_t max_linewidth;
> >       unsigned int rotation;
> > @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >       if (!new_plane_state->visible)
> >               return 0;
> >
> > -     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> > -     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > +     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> > +     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +     r_pipe->sspp = NULL;
> >
> >       pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
> >       if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
> > @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >
> >       max_linewidth = pdpu->catalog->caps->max_linewidth;
> >
> > -     /* check decimated source width */
> >       if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
> > -             DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> > -                             DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> > -             return -E2BIG;
> > +             /* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
> > +
> > +             if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
> > +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> > +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> > +                     return -E2BIG;
> > +             }
>
> This is where I am a bit concerned enabling it for all chipsets in one go.

As I wrote earlier, I'd prefer the opt-out rather than opt-in here. It
is much easier to handle the reports "I have a device with sm6543,
where the display worked before 6.4, but started failing afterwards"
rather than trying to find a person with sm6543 and asking him if he
can enable this and that on his device. And even a lower chance of a
person with sm6543 coming up with a patch 'hey, I enabled this for my
phone and it works!'.

If we find any issues during or close to the end of the development
cycle, we can add a 'don't enable wide plane here' switch and enable
it for failing platforms. But each enablement of this switch should
come with a reason (wide planes not working here because ....). In the
end this switch should be gone and transformed into proper HW
limitation checks.

> As you are aware,  we have an open bug today that we do not filter out
> the modes which we do not support.
>
> https://gitlab.freedesktop.org/drm/msm/-/issues/21

I thought that with the link-frequencies in place and with the DSI
checking the OPP tables this issue is mostly handled. Isn't it?
Is a mode check in the DPU driver itself the last missing piece?

>
> Due to this, on all chipsets we will end up trying to do a 4K on
> external display which we dont know what bugs it will expose.

If we do not expose bugs, we do not have a way to fix them. And I
definitely think that all the bugs should be listed as early as
possible, while both of us still remember the code under the question.

>
> So lets say if we test it on sc7280 fully but not on sc7180, we will
> still hit this condition on sc7180 too but on that chipset we did not
> advertise 4K as a capability in the product spec.

Is it 'not advertised' or 'not supported by hw'?

>
> With the max_linewidth check relaxed nothing prevents us from doing 4K
> on a chipset which doesnt support 4K.

What prevents sc7180 from supporting 4k? Does it support Smart DMA?
Does it support having two LMs per INTF/CRTC? Is there a limitation on
the linewidth of two LMs or two SSPPs?

I see that sm7125 (which has the same DPU revision) even contains
"qcom,sde-vig-sspp-linewidth = <4096>;" in the DTS, despite official
'product brief' advertising only 2520x1080 output resolution.

>
> > +
> > +             /*
> > +              * FIXME: it's not possible to check if sourcesplit is supported,
> > +              * LMs is not assigned yet. It happens in dpu_encoder_virt_mode_set
> > +              */
> > +             if (drm_rect_width(&pipe_cfg->src_rect) != drm_rect_width(&pipe_cfg->dst_rect) ||
> > +                        drm_rect_height(&pipe_cfg->src_rect) != drm_rect_height(&pipe_cfg->dst_rect) ||
> > +                        (!test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) &&
> > +                         !test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features)) ||
> > +                        /* cstate->num_mixers < 2 ||
> > +                        !test_bit(DPU_MIXER_SOURCESPLIT, &cstate->mixers[0].hw_lm->cap->features) || */
> > +                        DPU_FORMAT_IS_YUV(fmt)) {
> > +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, can't use split source\n",
> > +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> > +                     return -E2BIG;
> > +             }
> > +
> > +             /* Use multirect for wide plane. We do not support dynamic assignment of SSPPs, so we know the configuration. */
> > +             pipe->multirect_index = DPU_SSPP_RECT_0;
> > +             pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
> > +
> > +             r_pipe->sspp = pipe->sspp;
> > +             r_pipe->multirect_index = DPU_SSPP_RECT_1;
> > +             r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>
>
> > +
> > +             *r_pipe_cfg = *pipe_cfg;
> > +             pipe_cfg->src_rect.x2 = (pipe_cfg->src_rect.x1 + pipe_cfg->src_rect.x2) >> 1;
> > +             pipe_cfg->dst_rect.x2 = (pipe_cfg->dst_rect.x1 + pipe_cfg->dst_rect.x2) >> 1;
> > +             r_pipe_cfg->src_rect.x1 = pipe_cfg->src_rect.x2;
> > +             r_pipe_cfg->dst_rect.x1 = pipe_cfg->dst_rect.x2;
> >       }
> >
>
> As you requested just wanted to summarize the condition in the email.
>
> In parallel fetch mode, the downstream driver for UBWC formats, we check
> whether the src width of each rectangle is > maxlinewidth/2
>
> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r3-00500-WAIPIO.0/msm/sde/sde_plane.c#L1835

Thanks. Please double check my understanding: If the rectangle is used
for the tiled format, then it's max_linewidth is effectively halved.
So we can use rect_solo with full width, but for rect_0/rect_1 we
should halve it, even if two rectangles are used in the time split?

>
> For sc7280, maxlinewidth is 2400
>
> static const struct dpu_caps sc7280_dpu_caps = {
>          .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
>          .max_mixer_blendstages = 0x7,
>          .qseed_type = DPU_SSPP_SCALER_QSEED4,
>          .smart_dma_rev = DPU_SSPP_SMART_DMA_V2,
>          .ubwc_version = DPU_HW_UBWC_VER_30,
>          .has_dim_layer = true,
>          .has_idle_pc = true,
>          .max_linewidth = 2400,
>          .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> };
>
> Hence for UBWC formats which are by default used on the sc7280
> chromebook, each rectangle should be < 1200
>
> SmartDMA is therefore not enough to support 4K on sc7280 and we need
> true virtual planes ( using two SSPPs to display the 4K layer )
>
> Also, probably worth commenting that time multiplex mode support is not
> added in this series.

Ack.

>
> >       fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >
> > -     ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt);
> > +     ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt);
> >       if (ret)
> >               return ret;
> >
> > @@ -1094,8 +1148,10 @@ 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
> > +     else {
> >               dpu_plane_flush_csc(pdpu, &pstate->pipe);
> > +             dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
> > +     }
> >
> >       /* flag h/w flush complete */
> >       if (plane->state)
> > @@ -1130,6 +1186,9 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
> >       struct drm_plane_state *state = plane->state;
> >       struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> >
> > +     if (!pipe->sspp)
> > +             return;
> > +
> >       if (layout && pipe->sspp->ops.setup_sourceaddress) {
> >               trace_dpu_plane_set_scanout(pipe, layout);
> >               pipe->sspp->ops.setup_sourceaddress(pipe, layout);
> > @@ -1207,13 +1266,14 @@ static void dpu_plane_sspp_atomic_update(struct drm_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 dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> >       struct drm_crtc *crtc = state->crtc;
> >       struct drm_framebuffer *fb = state->fb;
> >       bool is_rt_pipe;
> >       const struct dpu_format *fmt =
> >               to_dpu_format(msm_framebuffer_format(fb));
> >       struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> > -
> > +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> >       struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
> >       struct msm_gem_address_space *aspace = kms->base.aspace;
> >       struct dpu_hw_fmt_layout layout;
> > @@ -1241,12 +1301,22 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
> >                                  drm_mode_vrefresh(&crtc->mode),
> >                                  layout_valid ? &layout: NULL);
> >
> > +     dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt,
> > +                                drm_mode_vrefresh(&crtc->mode),
> > +                                layout_valid ? &layout: NULL);
> > +
> >       if (pstate->needs_qos_remap)
> >               pstate->needs_qos_remap = false;
> >
> >       pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, pipe_cfg);
> >
> >       pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, pipe_cfg);
> > +
> > +     if (r_pipe->sspp) {
> > +             pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, r_pipe_cfg);
> > +
> > +             pstate->plane_clk = max(pstate->plane_clk, _dpu_plane_calc_clk(&crtc->mode, r_pipe_cfg));
> > +     }
> >   }
> >
> >   static void _dpu_plane_atomic_disable(struct drm_plane *plane)
> > @@ -1289,6 +1359,8 @@ static void dpu_plane_destroy(struct drm_plane *plane)
> >               pstate = to_dpu_plane_state(plane->state);
> >               _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
> >
> > +             _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
> > +
> >               mutex_destroy(&pdpu->lock);
> >
> >               /* this will destroy the states as well */
> > @@ -1369,11 +1441,26 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p,
> >               const struct drm_plane_state *state)
> >   {
> >       const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> > +     const struct dpu_sw_pipe *pipe = &pstate->pipe;
> > +     const struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> > +     const struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> > +     const struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> >
> >       drm_printf(p, "\tstage=%d\n", pstate->stage);
> > -     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));
> > +
> > +     drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name);
> > +     drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode));
> > +     drm_printf(p, "\tmultirect_index[0]=%s\n", dpu_get_multirect_index(pipe->multirect_index));
> > +     drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect));
> > +     drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect));
> > +
> > +     if (r_pipe->sspp) {
> > +             drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name);
> > +             drm_printf(p, "\tmultirect_mode[1]=%s\n", dpu_get_multirect_mode(r_pipe->multirect_mode));
> > +             drm_printf(p, "\tmultirect_index[1]=%s\n", dpu_get_multirect_index(r_pipe->multirect_index));
> > +             drm_printf(p, "\tsrc[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->src_rect));
> > +             drm_printf(p, "\tdst[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->dst_rect));
> > +     }
> >   }
>
> Do you think that changing the atomic_print_state to print the r_pipe
> sspp can be moved to a separate patch? So that way we only keep the core
> logic of atomic check of smartDMA in this patch.
>
> >
> >   static void dpu_plane_reset(struct drm_plane *plane)
> > @@ -1407,6 +1494,10 @@ static void dpu_plane_reset(struct drm_plane *plane)
> >        * This is the place where the state is allocated, so fill it fully.
> >        */
> >       pstate->pipe.sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe);
> > +     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> > +     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > +
> > +     pstate->r_pipe.sspp = NULL;
> >
> >       __drm_atomic_helper_plane_reset(plane, &pstate->base);
> >   }
> > @@ -1423,6 +1514,7 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
> >
> >       pm_runtime_get_sync(&dpu_kms->pdev->dev);
> >       _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
> > +     _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
> >       pm_runtime_put_sync(&dpu_kms->pdev->dev);
> >   }
> >   #endif
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > index 079dad83eb37..183c95949885 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> > @@ -19,7 +19,9 @@
> >    * @base:   base drm plane state object
> >    * @aspace: pointer to address space for input/output buffers
> >    * @pipe:   software pipe description
> > + * @r_pipe:  software pipe description of the second pipe
> >    * @pipe_cfg:       software pipe configuration
> > + * @r_pipe_cfg:      software pipe configuration for the second pipe
> >    * @stage:  assigned by crtc blender
> >    * @needs_qos_remap: qos remap settings need to be updated
> >    * @multirect_index: index of the rectangle of SSPP
> > @@ -34,7 +36,9 @@ struct dpu_plane_state {
> >       struct drm_plane_state base;
> >       struct msm_gem_address_space *aspace;
> >       struct dpu_sw_pipe pipe;
> > +     struct dpu_sw_pipe r_pipe;
> >       struct dpu_hw_sspp_cfg pipe_cfg;
> > +     struct dpu_hw_sspp_cfg r_pipe_cfg;
> >       enum dpu_stage stage;
> >       bool needs_qos_remap;
> >       bool pending;

Abhinav Kumar Feb. 9, 2023, 7:25 p.m. UTC | #3

On 2/9/2023 3:45 AM, Dmitry Baryshkov wrote:
> On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar <[e-mail]> wrote:
>>
>>
>>
>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>> Typically SSPP can support rectangle with width up to 2560. However it's
>>
>> Not always 2560. Depends on the chipset.
> 
> _typically_
> 

Would just say maxlinewidth of SSPP instead of giving some hardcoded number.

>>
>>> possible to use multirect feature and split source to use the SSPP to
>>> output two consecutive rectangles. This commit brings in this capability
>>> to support wider screen resolutions.
>>>
>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
>>>    3 files changed, 114 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 0ca3bc38ff7e..867832a752b2 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -485,6 +485,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>>>                                           fetch_active,
>>>                                           &pstate->pipe);
>>>
>>> +             _dpu_crtc_blend_setup_pipe(crtc, plane,
>>> +                                        mixer, cstate->num_mixers,
>>> +                                        stage_cfg, pstate->stage, 1,
>>> +                                        fetch_active,
>>> +                                        &pstate->r_pipe);
>>> +
>>>                /* blend config update */
>>>                for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
>>>                        _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> index e2e85688ed3c..401ead64c6bd 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>>>        struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>        struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
>>>
>>> +     if (!pipe->sspp)
>>> +             return;
>>> +
>>>        memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>>
>>>        if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>>> @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
>>>    {
>>>        struct dpu_hw_sspp_cfg pipe_cfg;
>>>
>>> +     if (!pipe->sspp)
>>> +             return 0;
>>
>> instead of checking if sspp was present, is it not better for the caller
>> to check if the rpipe is valid before calling this?
>>
>>> +
>>>        /* update sspp */
>>>        if (!pipe->sspp->ops.setup_solidfill)
>>>                return 0;
>>> @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
>>>
>>>        /* update sspp */
>>>        _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
>>> +
>>> +     _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
>>>    }
>>
>> So cant we do
>>
>> if (pstate->r_pipe.sspp)
>>          _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
>>                  &pstate->r_pipe_cfg, fill_color, fmt);
>>
>> It just seems better to me as the caller would already know if the sspp
>> was assigned.
> 
>   I think I had this kind of code earlier, but then I found it more
> logical to move the check to the called function. I'll move it back.
> 
>>
>>>
>>>    int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>>> @@ -911,6 +919,9 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
>>>    {
>>>        uint32_t min_src_size;
>>>
>>> +     if (!pipe->sspp)
>>> +             return 0;
>>> +
>>>        min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>
>>>        if (DPU_FORMAT_IS_YUV(fmt) &&
>>> @@ -957,9 +968,12 @@ 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;
>>> +     struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>>        const struct drm_crtc_state *crtc_state = NULL;
>>>        const struct dpu_format *fmt;
>>>        struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>        struct drm_rect fb_rect = { 0 };
>>>        uint32_t max_linewidth;
>>>        unsigned int rotation;
>>> @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>>>        if (!new_plane_state->visible)
>>>                return 0;
>>>
>>> -     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>> -     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> +     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>> +     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> +     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>> +     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> +     r_pipe->sspp = NULL;
>>>
>>>        pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
>>>        if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
>>> @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>>>
>>>        max_linewidth = pdpu->catalog->caps->max_linewidth;
>>>
>>> -     /* check decimated source width */
>>>        if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
>>> -             DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
>>> -                             DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>> -             return -E2BIG;
>>> +             /* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
>>> +
>>> +             if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
>>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
>>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>> +                     return -E2BIG;
>>> +             }
>>
>> This is where I am a bit concerned enabling it for all chipsets in one go.
> 
> As I wrote earlier, I'd prefer the opt-out rather than opt-in here. It
> is much easier to handle the reports "I have a device with sm6543,
> where the display worked before 6.4, but started failing afterwards"
> rather than trying to find a person with sm6543 and asking him if he
> can enable this and that on his device. And even a lower chance of a
> person with sm6543 coming up with a patch 'hey, I enabled this for my
> phone and it works!'.
> 
> If we find any issues during or close to the end of the development
> cycle, we can add a 'don't enable wide plane here' switch and enable
> it for failing platforms. But each enablement of this switch should
> come with a reason (wide planes not working here because ....). In the
> end this switch should be gone and transformed into proper HW
> limitation checks.
> 

As it has become clear that with this patch series 4K with UBWC cannot 
be supported without true virtual planes (with two SSPPs), why do you 
need to relax this check right now?

You can relax this when you add the support for virtual planes till then 
let it be this way.

Its not going to break smartDMA as such. You can still use it for layers 
< 2560.

That way we stay true to the purpose of the feature. I think originally 
you wanted to get this in for smartDMA and not to support wide plane and 
that purpose will still be achieved even with keeping this check intact.

You can relax it in the virtual plane series.

Regarding issues, this is where it gets tricky. We should be aligning 
with what the product supports. QC will not support issues arising with 
4K on chipsets on which 4K is not advertized.

>> As you are aware,  we have an open bug today that we do not filter out
>> the modes which we do not support.
>>
>> https://gitlab.freedesktop.org/drm/msm/-/issues/21
> 
> I thought that with the link-frequencies in place and with the DSI
> checking the OPP tables this issue is mostly handled. Isn't it?
> Is a mode check in the DPU driver itself the last missing piece?
> 

opp based checking was implemented only for DSI. That one is byte clk based.

DP uses link rate for opp table.

Even with a 5.4G link rate (the one in sc7180 chromebook) 4k@30 would 
still be possible but it was not advertized

https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf

These docs are available in public domain.

As we synced up last time on 
https://patchwork.freedesktop.org/series/107917/, even with these limits 
in place, its not matching the advertized limits.

>>
>> Due to this, on all chipsets we will end up trying to do a 4K on
>> external display which we dont know what bugs it will expose.
> 
> If we do not expose bugs, we do not have a way to fix them. And I
> definitely think that all the bugs should be listed as early as
> possible, while both of us still remember the code under the question.
> 

Yes but on chipsets where 4K is supported ( and hence needed ).

>>
>> So lets say if we test it on sc7280 fully but not on sc7180, we will
>> still hit this condition on sc7180 too but on that chipset we did not
>> advertise 4K as a capability in the product spec.
> 
> Is it 'not advertised' or 'not supported by hw'?
> 

The document 
https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf 
is made from inputs from not just display team but overall system 
limits. So even though you could argue that this falls within the 
display capabilities, all I can say at the moment is we have to stick to 
the advertized limits as its compiled with inputs from all the teams 
(system/performance etc).

>>
>> With the max_linewidth check relaxed nothing prevents us from doing 4K
>> on a chipset which doesnt support 4K.
> 
> What prevents sc7180 from supporting 4k? Does it support Smart DMA?
> Does it support having two LMs per INTF/CRTC? Is there a limitation on
> the linewidth of two LMs or two SSPPs?
> 
> I see that sm7125 (which has the same DPU revision) even contains
> "qcom,sde-vig-sspp-linewidth = <4096>;" in the DTS, despite official
> 'product brief' advertising only 2520x1080 output resolution.
> 

My previous response should have answered this.

>>
>>> +
>>> +             /*
>>> +              * FIXME: it's not possible to check if sourcesplit is supported,
>>> +              * LMs is not assigned yet. It happens in dpu_encoder_virt_mode_set
>>> +              */
>>> +             if (drm_rect_width(&pipe_cfg->src_rect) != drm_rect_width(&pipe_cfg->dst_rect) ||
>>> +                        drm_rect_height(&pipe_cfg->src_rect) != drm_rect_height(&pipe_cfg->dst_rect) ||
>>> +                        (!test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) &&
>>> +                         !test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features)) ||
>>> +                        /* cstate->num_mixers < 2 ||
>>> +                        !test_bit(DPU_MIXER_SOURCESPLIT, &cstate->mixers[0].hw_lm->cap->features) || */
>>> +                        DPU_FORMAT_IS_YUV(fmt)) {
>>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, can't use split source\n",
>>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>> +                     return -E2BIG;
>>> +             }
>>> +
>>> +             /* Use multirect for wide plane. We do not support dynamic assignment of SSPPs, so we know the configuration. */
>>> +             pipe->multirect_index = DPU_SSPP_RECT_0;
>>> +             pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>> +
>>> +             r_pipe->sspp = pipe->sspp;
>>> +             r_pipe->multirect_index = DPU_SSPP_RECT_1;
>>> +             r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>
>>
>>> +
>>> +             *r_pipe_cfg = *pipe_cfg;
>>> +             pipe_cfg->src_rect.x2 = (pipe_cfg->src_rect.x1 + pipe_cfg->src_rect.x2) >> 1;
>>> +             pipe_cfg->dst_rect.x2 = (pipe_cfg->dst_rect.x1 + pipe_cfg->dst_rect.x2) >> 1;
>>> +             r_pipe_cfg->src_rect.x1 = pipe_cfg->src_rect.x2;
>>> +             r_pipe_cfg->dst_rect.x1 = pipe_cfg->dst_rect.x2;
>>>        }
>>>
>>
>> As you requested just wanted to summarize the condition in the email.
>>
>> In parallel fetch mode, the downstream driver for UBWC formats, we check
>> whether the src width of each rectangle is > maxlinewidth/2
>>
>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r3-00500-WAIPIO.0/msm/sde/sde_plane.c#L1835
> 
> Thanks. Please double check my understanding: If the rectangle is used
> for the tiled format, then it's max_linewidth is effectively halved.
> So we can use rect_solo with full width, but for rect_0/rect_1 we
> should halve it, even if two rectangles are used in the time split?
> 

Not in time split mode. Only in parallel fetch mode which is being used 
here. Rest of your understanding is correct.

>>
>> For sc7280, maxlinewidth is 2400
>>
>> static const struct dpu_caps sc7280_dpu_caps = {
>>           .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
>>           .max_mixer_blendstages = 0x7,
>>           .qseed_type = DPU_SSPP_SCALER_QSEED4,
>>           .smart_dma_rev = DPU_SSPP_SMART_DMA_V2,
>>           .ubwc_version = DPU_HW_UBWC_VER_30,
>>           .has_dim_layer = true,
>>           .has_idle_pc = true,
>>           .max_linewidth = 2400,
>>           .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>> };
>>
>> Hence for UBWC formats which are by default used on the sc7280
>> chromebook, each rectangle should be < 1200
>>
>> SmartDMA is therefore not enough to support 4K on sc7280 and we need
>> true virtual planes ( using two SSPPs to display the 4K layer )
>>
>> Also, probably worth commenting that time multiplex mode support is not
>> added in this series.
> 
> Ack.
> 
>>
>>>        fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>>>
>>> -     ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt);
>>> +     ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt);
>>>        if (ret)
>>>                return ret;
>>>
>>> @@ -1094,8 +1148,10 @@ 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
>>> +     else {
>>>                dpu_plane_flush_csc(pdpu, &pstate->pipe);
>>> +             dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
>>> +     }
>>>
>>>        /* flag h/w flush complete */
>>>        if (plane->state)
>>> @@ -1130,6 +1186,9 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
>>>        struct drm_plane_state *state = plane->state;
>>>        struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>
>>> +     if (!pipe->sspp)
>>> +             return;
>>> +
>>>        if (layout && pipe->sspp->ops.setup_sourceaddress) {
>>>                trace_dpu_plane_set_scanout(pipe, layout);
>>>                pipe->sspp->ops.setup_sourceaddress(pipe, layout);
>>> @@ -1207,13 +1266,14 @@ static void dpu_plane_sspp_atomic_update(struct drm_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 dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>>        struct drm_crtc *crtc = state->crtc;
>>>        struct drm_framebuffer *fb = state->fb;
>>>        bool is_rt_pipe;
>>>        const struct dpu_format *fmt =
>>>                to_dpu_format(msm_framebuffer_format(fb));
>>>        struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>> -
>>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>        struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
>>>        struct msm_gem_address_space *aspace = kms->base.aspace;
>>>        struct dpu_hw_fmt_layout layout;
>>> @@ -1241,12 +1301,22 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>                                   drm_mode_vrefresh(&crtc->mode),
>>>                                   layout_valid ? &layout: NULL);
>>>
>>> +     dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt,
>>> +                                drm_mode_vrefresh(&crtc->mode),
>>> +                                layout_valid ? &layout: NULL);
>>> +
>>>        if (pstate->needs_qos_remap)
>>>                pstate->needs_qos_remap = false;
>>>
>>>        pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, pipe_cfg);
>>>
>>>        pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, pipe_cfg);
>>> +
>>> +     if (r_pipe->sspp) {
>>> +             pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, r_pipe_cfg);
>>> +
>>> +             pstate->plane_clk = max(pstate->plane_clk, _dpu_plane_calc_clk(&crtc->mode, r_pipe_cfg));
>>> +     }
>>>    }
>>>
>>>    static void _dpu_plane_atomic_disable(struct drm_plane *plane)
>>> @@ -1289,6 +1359,8 @@ static void dpu_plane_destroy(struct drm_plane *plane)
>>>                pstate = to_dpu_plane_state(plane->state);
>>>                _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
>>>
>>> +             _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
>>> +
>>>                mutex_destroy(&pdpu->lock);
>>>
>>>                /* this will destroy the states as well */
>>> @@ -1369,11 +1441,26 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p,
>>>                const struct drm_plane_state *state)
>>>    {
>>>        const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>> +     const struct dpu_sw_pipe *pipe = &pstate->pipe;
>>> +     const struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>> +     const struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>> +     const struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>
>>>        drm_printf(p, "\tstage=%d\n", pstate->stage);
>>> -     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));
>>> +
>>> +     drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name);
>>> +     drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode));
>>> +     drm_printf(p, "\tmultirect_index[0]=%s\n", dpu_get_multirect_index(pipe->multirect_index));
>>> +     drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect));
>>> +     drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect));
>>> +
>>> +     if (r_pipe->sspp) {
>>> +             drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name);
>>> +             drm_printf(p, "\tmultirect_mode[1]=%s\n", dpu_get_multirect_mode(r_pipe->multirect_mode));
>>> +             drm_printf(p, "\tmultirect_index[1]=%s\n", dpu_get_multirect_index(r_pipe->multirect_index));
>>> +             drm_printf(p, "\tsrc[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->src_rect));
>>> +             drm_printf(p, "\tdst[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->dst_rect));
>>> +     }
>>>    }
>>
>> Do you think that changing the atomic_print_state to print the r_pipe
>> sspp can be moved to a separate patch? So that way we only keep the core
>> logic of atomic check of smartDMA in this patch.
>>
>>>
>>>    static void dpu_plane_reset(struct drm_plane *plane)
>>> @@ -1407,6 +1494,10 @@ static void dpu_plane_reset(struct drm_plane *plane)
>>>         * This is the place where the state is allocated, so fill it fully.
>>>         */
>>>        pstate->pipe.sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe);
>>> +     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>> +     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> +
>>> +     pstate->r_pipe.sspp = NULL;
>>>
>>>        __drm_atomic_helper_plane_reset(plane, &pstate->base);
>>>    }
>>> @@ -1423,6 +1514,7 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>>>
>>>        pm_runtime_get_sync(&dpu_kms->pdev->dev);
>>>        _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
>>> +     _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
>>>        pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>    }
>>>    #endif
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>> index 079dad83eb37..183c95949885 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>> @@ -19,7 +19,9 @@
>>>     * @base:   base drm plane state object
>>>     * @aspace: pointer to address space for input/output buffers
>>>     * @pipe:   software pipe description
>>> + * @r_pipe:  software pipe description of the second pipe
>>>     * @pipe_cfg:       software pipe configuration
>>> + * @r_pipe_cfg:      software pipe configuration for the second pipe
>>>     * @stage:  assigned by crtc blender
>>>     * @needs_qos_remap: qos remap settings need to be updated
>>>     * @multirect_index: index of the rectangle of SSPP
>>> @@ -34,7 +36,9 @@ struct dpu_plane_state {
>>>        struct drm_plane_state base;
>>>        struct msm_gem_address_space *aspace;
>>>        struct dpu_sw_pipe pipe;
>>> +     struct dpu_sw_pipe r_pipe;
>>>        struct dpu_hw_sspp_cfg pipe_cfg;
>>> +     struct dpu_hw_sspp_cfg r_pipe_cfg;
>>>        enum dpu_stage stage;
>>>        bool needs_qos_remap;
>>>        bool pending;
> 
> 
>

Dmitry Baryshkov Feb. 9, 2023, 9:23 p.m. UTC | #4

Hi Abhinav,

On Thu, 9 Feb 2023 at 21:25, Abhinav Kumar <[e-mail]> wrote:
>
>
>
> On 2/9/2023 3:45 AM, Dmitry Baryshkov wrote:
> > On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar <[e-mail]> wrote:
> >>
> >>
> >>
> >> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> >>> Typically SSPP can support rectangle with width up to 2560. However it's
> >>
> >> Not always 2560. Depends on the chipset.
> >
> > _typically_
> >
>
> Would just say maxlinewidth of SSPP instead of giving some hardcoded number.

Ack.

>
> >>
> >>> possible to use multirect feature and split source to use the SSPP to
> >>> output two consecutive rectangles. This commit brings in this capability
> >>> to support wider screen resolutions.
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> >>> ---
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
> >>>    3 files changed, 114 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> index 0ca3bc38ff7e..867832a752b2 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> @@ -485,6 +485,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
> >>>                                           fetch_active,
> >>>                                           &pstate->pipe);
> >>>
> >>> +             _dpu_crtc_blend_setup_pipe(crtc, plane,
> >>> +                                        mixer, cstate->num_mixers,
> >>> +                                        stage_cfg, pstate->stage, 1,
> >>> +                                        fetch_active,
> >>> +                                        &pstate->r_pipe);
> >>> +
> >>>                /* blend config update */
> >>>                for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
> >>>                        _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>> index e2e85688ed3c..401ead64c6bd 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>> @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
> >>>        struct dpu_plane *pdpu = to_dpu_plane(plane);
> >>>        struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
> >>>
> >>> +     if (!pipe->sspp)
> >>> +             return;
> >>> +
> >>>        memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
> >>>
> >>>        if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
> >>> @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
> >>>    {
> >>>        struct dpu_hw_sspp_cfg pipe_cfg;
> >>>
> >>> +     if (!pipe->sspp)
> >>> +             return 0;
> >>
> >> instead of checking if sspp was present, is it not better for the caller
> >> to check if the rpipe is valid before calling this?
> >>
> >>> +
> >>>        /* update sspp */
> >>>        if (!pipe->sspp->ops.setup_solidfill)
> >>>                return 0;
> >>> @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
> >>>
> >>>        /* update sspp */
> >>>        _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
> >>> +
> >>> +     _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
> >>>    }
> >>
> >> So cant we do
> >>
> >> if (pstate->r_pipe.sspp)
> >>          _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
> >>                  &pstate->r_pipe_cfg, fill_color, fmt);
> >>
> >> It just seems better to me as the caller would already know if the sspp
> >> was assigned.
> >
> >   I think I had this kind of code earlier, but then I found it more
> > logical to move the check to the called function. I'll move it back.
> >
> >>
> >>>
> >>>    int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
> >>> @@ -911,6 +919,9 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
> >>>    {
> >>>        uint32_t min_src_size;
> >>>
> >>> +     if (!pipe->sspp)
> >>> +             return 0;
> >>> +
> >>>        min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> >>>
> >>>        if (DPU_FORMAT_IS_YUV(fmt) &&
> >>> @@ -957,9 +968,12 @@ 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;
> >>> +     struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> >>>        const struct drm_crtc_state *crtc_state = NULL;
> >>>        const struct dpu_format *fmt;
> >>>        struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> >>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> >>>        struct drm_rect fb_rect = { 0 };
> >>>        uint32_t max_linewidth;
> >>>        unsigned int rotation;
> >>> @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >>>        if (!new_plane_state->visible)
> >>>                return 0;
> >>>
> >>> -     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> >>> -     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> +     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> +     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> +     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>> +     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> +     r_pipe->sspp = NULL;
> >>>
> >>>        pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
> >>>        if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
> >>> @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >>>
> >>>        max_linewidth = pdpu->catalog->caps->max_linewidth;
> >>>
> >>> -     /* check decimated source width */
> >>>        if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
> >>> -             DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> >>> -                             DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> >>> -             return -E2BIG;
> >>> +             /* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
> >>> +
> >>> +             if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
> >>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> >>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> >>> +                     return -E2BIG;
> >>> +             }
> >>
> >> This is where I am a bit concerned enabling it for all chipsets in one go.
> >
> > As I wrote earlier, I'd prefer the opt-out rather than opt-in here. It
> > is much easier to handle the reports "I have a device with sm6543,
> > where the display worked before 6.4, but started failing afterwards"
> > rather than trying to find a person with sm6543 and asking him if he
> > can enable this and that on his device. And even a lower chance of a
> > person with sm6543 coming up with a patch 'hey, I enabled this for my
> > phone and it works!'.
> >
> > If we find any issues during or close to the end of the development
> > cycle, we can add a 'don't enable wide plane here' switch and enable
> > it for failing platforms. But each enablement of this switch should
> > come with a reason (wide planes not working here because ....). In the
> > end this switch should be gone and transformed into proper HW
> > limitation checks.
> >
>
> As it has become clear that with this patch series 4K with UBWC cannot
> be supported without true virtual planes (with two SSPPs), why do you
> need to relax this check right now?

Yes. It enables support for 4k @ linear formats. So my plan for this
series is to land 4k with all the proper applicable restrictions.

> You can relax this when you add the support for virtual planes till then
> let it be this way.
>
> Its not going to break smartDMA as such. You can still use it for layers
> < 2560.
>
> That way we stay true to the purpose of the feature. I think originally
> you wanted to get this in for smartDMA and not to support wide plane and
> that purpose will still be achieved even with keeping this check intact.

Actually, no. With this series I wanted to get 4k. It was developed in
parallel with the 4k enablement for RB3 (posted, bridge patches are
being merged for 6.3) and RB5 (delayed for now, I have other issues
there).

> You can relax it in the virtual plane series.
>
> Regarding issues, this is where it gets tricky. We should be aligning
> with what the product supports. QC will not support issues arising with
> 4K on chipsets on which 4K is not advertized.

So, we have several different items here:
- SmartDMA v2 per se, supporting two rectangles per VIG or DMA plane,
- Source split support,
- Supporting 4k modes.

I think we should tend them one by one. This series concerns SmartDMA
v2. Using SmartDMA it is possible to use two rectangles side by side
to emulate a wide plane. This series doesn't care at all about max
resolutions. These two items are completely orthogonal.

> >> As you are aware,  we have an open bug today that we do not filter out
> >> the modes which we do not support.
> >>
> >> https://gitlab.freedesktop.org/drm/msm/-/issues/21
> >
> > I thought that with the link-frequencies in place and with the DSI
> > checking the OPP tables this issue is mostly handled. Isn't it?
> > Is a mode check in the DPU driver itself the last missing piece?
> >
>
> opp based checking was implemented only for DSI. That one is byte clk based.
>
> DP uses link rate for opp table.
>
> Even with a 5.4G link rate (the one in sc7180 chromebook) 4k@30 would
> still be possible but it was not advertized
>
> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
>
> These docs are available in public domain.
>
> As we synced up last time on
> https://patchwork.freedesktop.org/series/107917/, even with these limits
> in place, its not matching the advertized limits.
>
> >>
> >> Due to this, on all chipsets we will end up trying to do a 4K on
> >> external display which we dont know what bugs it will expose.
> >
> > If we do not expose bugs, we do not have a way to fix them. And I
> > definitely think that all the bugs should be listed as early as
> > possible, while both of us still remember the code under the question.
> >
>
> Yes but on chipsets where 4K is supported ( and hence needed ).

4k, SmartDMA, src-split, split-display, etc.


>
> >>
> >> So lets say if we test it on sc7280 fully but not on sc7180, we will
> >> still hit this condition on sc7180 too but on that chipset we did not
> >> advertise 4K as a capability in the product spec.
> >
> > Is it 'not advertised' or 'not supported by hw'?
> >
>
> The document
> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
> is made from inputs from not just display team but overall system
> limits. So even though you could argue that this falls within the
> display capabilities, all I can say at the moment is we have to stick to
> the advertized limits as its compiled with inputs from all the teams
> (system/performance etc).

So, there should be a limiting factor (or a combination of them).
Filter out 4k modes on sc7180. Or modes using fill rate higher than N.
Pixel clock rate higher than M. But it has nothing to do with these
patches enabling SmartDMA support on this platform.

Even if we look at the vendor kernels, we don't see 'maximum external
resolution'. Instead I see a combination of linewidth and bandwidth
limitations. If we can stick to that, that would be great.

>
> >>
> >> With the max_linewidth check relaxed nothing prevents us from doing 4K
> >> on a chipset which doesnt support 4K.
> >
> > What prevents sc7180 from supporting 4k? Does it support Smart DMA?
> > Does it support having two LMs per INTF/CRTC? Is there a limitation on
> > the linewidth of two LMs or two SSPPs?
> >
> > I see that sm7125 (which has the same DPU revision) even contains
> > "qcom,sde-vig-sspp-linewidth = <4096>;" in the DTS, despite official
> > 'product brief' advertising only 2520x1080 output resolution.
> >
>
> My previous response should have answered this.

Up to some point, thanks.

>
> >>
> >>> +
> >>> +             /*
> >>> +              * FIXME: it's not possible to check if sourcesplit is supported,
> >>> +              * LMs is not assigned yet. It happens in dpu_encoder_virt_mode_set
> >>> +              */
> >>> +             if (drm_rect_width(&pipe_cfg->src_rect) != drm_rect_width(&pipe_cfg->dst_rect) ||
> >>> +                        drm_rect_height(&pipe_cfg->src_rect) != drm_rect_height(&pipe_cfg->dst_rect) ||
> >>> +                        (!test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) &&
> >>> +                         !test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features)) ||
> >>> +                        /* cstate->num_mixers < 2 ||
> >>> +                        !test_bit(DPU_MIXER_SOURCESPLIT, &cstate->mixers[0].hw_lm->cap->features) || */
> >>> +                        DPU_FORMAT_IS_YUV(fmt)) {
> >>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, can't use split source\n",
> >>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> >>> +                     return -E2BIG;
> >>> +             }
> >>> +
> >>> +             /* Use multirect for wide plane. We do not support dynamic assignment of SSPPs, so we know the configuration. */
> >>> +             pipe->multirect_index = DPU_SSPP_RECT_0;
> >>> +             pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
> >>> +
> >>> +             r_pipe->sspp = pipe->sspp;
> >>> +             r_pipe->multirect_index = DPU_SSPP_RECT_1;
> >>> +             r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
> >>
> >>
> >>> +
> >>> +             *r_pipe_cfg = *pipe_cfg;
> >>> +             pipe_cfg->src_rect.x2 = (pipe_cfg->src_rect.x1 + pipe_cfg->src_rect.x2) >> 1;
> >>> +             pipe_cfg->dst_rect.x2 = (pipe_cfg->dst_rect.x1 + pipe_cfg->dst_rect.x2) >> 1;
> >>> +             r_pipe_cfg->src_rect.x1 = pipe_cfg->src_rect.x2;
> >>> +             r_pipe_cfg->dst_rect.x1 = pipe_cfg->dst_rect.x2;
> >>>        }
> >>>
> >>
> >> As you requested just wanted to summarize the condition in the email.
> >>
> >> In parallel fetch mode, the downstream driver for UBWC formats, we check
> >> whether the src width of each rectangle is > maxlinewidth/2
> >>
> >> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r3-00500-WAIPIO.0/msm/sde/sde_plane.c#L1835
> >
> > Thanks. Please double check my understanding: If the rectangle is used
> > for the tiled format, then it's max_linewidth is effectively halved.
> > So we can use rect_solo with full width, but for rect_0/rect_1 we
> > should halve it, even if two rectangles are used in the time split?
> >
>
> Not in time split mode. Only in parallel fetch mode which is being used
> here. Rest of your understanding is correct.

Ack, thanks for the correction. This is important for plane checks.

>
> >>
> >> For sc7280, maxlinewidth is 2400
> >>
> >> static const struct dpu_caps sc7280_dpu_caps = {
> >>           .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
> >>           .max_mixer_blendstages = 0x7,
> >>           .qseed_type = DPU_SSPP_SCALER_QSEED4,
> >>           .smart_dma_rev = DPU_SSPP_SMART_DMA_V2,
> >>           .ubwc_version = DPU_HW_UBWC_VER_30,
> >>           .has_dim_layer = true,
> >>           .has_idle_pc = true,
> >>           .max_linewidth = 2400,
> >>           .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
> >> };
> >>
> >> Hence for UBWC formats which are by default used on the sc7280
> >> chromebook, each rectangle should be < 1200
> >>
> >> SmartDMA is therefore not enough to support 4K on sc7280 and we need
> >> true virtual planes ( using two SSPPs to display the 4K layer )
> >>
> >> Also, probably worth commenting that time multiplex mode support is not
> >> added in this series.
> >
> > Ack.
> >
> >>
> >>>        fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
> >>>
> >>> -     ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt);
> >>> +     ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt);
> >>>        if (ret)
> >>>                return ret;
> >>>
> >>> @@ -1094,8 +1148,10 @@ 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
> >>> +     else {
> >>>                dpu_plane_flush_csc(pdpu, &pstate->pipe);
> >>> +             dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
> >>> +     }
> >>>
> >>>        /* flag h/w flush complete */
> >>>        if (plane->state)
> >>> @@ -1130,6 +1186,9 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
> >>>        struct drm_plane_state *state = plane->state;
> >>>        struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> >>>
> >>> +     if (!pipe->sspp)
> >>> +             return;
> >>> +
> >>>        if (layout && pipe->sspp->ops.setup_sourceaddress) {
> >>>                trace_dpu_plane_set_scanout(pipe, layout);
> >>>                pipe->sspp->ops.setup_sourceaddress(pipe, layout);
> >>> @@ -1207,13 +1266,14 @@ static void dpu_plane_sspp_atomic_update(struct drm_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 dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> >>>        struct drm_crtc *crtc = state->crtc;
> >>>        struct drm_framebuffer *fb = state->fb;
> >>>        bool is_rt_pipe;
> >>>        const struct dpu_format *fmt =
> >>>                to_dpu_format(msm_framebuffer_format(fb));
> >>>        struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> >>> -
> >>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> >>>        struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
> >>>        struct msm_gem_address_space *aspace = kms->base.aspace;
> >>>        struct dpu_hw_fmt_layout layout;
> >>> @@ -1241,12 +1301,22 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
> >>>                                   drm_mode_vrefresh(&crtc->mode),
> >>>                                   layout_valid ? &layout: NULL);
> >>>
> >>> +     dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt,
> >>> +                                drm_mode_vrefresh(&crtc->mode),
> >>> +                                layout_valid ? &layout: NULL);
> >>> +
> >>>        if (pstate->needs_qos_remap)
> >>>                pstate->needs_qos_remap = false;
> >>>
> >>>        pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, pipe_cfg);
> >>>
> >>>        pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, pipe_cfg);
> >>> +
> >>> +     if (r_pipe->sspp) {
> >>> +             pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, r_pipe_cfg);
> >>> +
> >>> +             pstate->plane_clk = max(pstate->plane_clk, _dpu_plane_calc_clk(&crtc->mode, r_pipe_cfg));
> >>> +     }
> >>>    }
> >>>
> >>>    static void _dpu_plane_atomic_disable(struct drm_plane *plane)
> >>> @@ -1289,6 +1359,8 @@ static void dpu_plane_destroy(struct drm_plane *plane)
> >>>                pstate = to_dpu_plane_state(plane->state);
> >>>                _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
> >>>
> >>> +             _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
> >>> +
> >>>                mutex_destroy(&pdpu->lock);
> >>>
> >>>                /* this will destroy the states as well */
> >>> @@ -1369,11 +1441,26 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p,
> >>>                const struct drm_plane_state *state)
> >>>    {
> >>>        const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
> >>> +     const struct dpu_sw_pipe *pipe = &pstate->pipe;
> >>> +     const struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> >>> +     const struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> >>> +     const struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> >>>
> >>>        drm_printf(p, "\tstage=%d\n", pstate->stage);
> >>> -     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));
> >>> +
> >>> +     drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name);
> >>> +     drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode));
> >>> +     drm_printf(p, "\tmultirect_index[0]=%s\n", dpu_get_multirect_index(pipe->multirect_index));
> >>> +     drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect));
> >>> +     drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect));
> >>> +
> >>> +     if (r_pipe->sspp) {
> >>> +             drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name);
> >>> +             drm_printf(p, "\tmultirect_mode[1]=%s\n", dpu_get_multirect_mode(r_pipe->multirect_mode));
> >>> +             drm_printf(p, "\tmultirect_index[1]=%s\n", dpu_get_multirect_index(r_pipe->multirect_index));
> >>> +             drm_printf(p, "\tsrc[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->src_rect));
> >>> +             drm_printf(p, "\tdst[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->dst_rect));
> >>> +     }
> >>>    }
> >>
> >> Do you think that changing the atomic_print_state to print the r_pipe
> >> sspp can be moved to a separate patch? So that way we only keep the core
> >> logic of atomic check of smartDMA in this patch.
> >>
> >>>
> >>>    static void dpu_plane_reset(struct drm_plane *plane)
> >>> @@ -1407,6 +1494,10 @@ static void dpu_plane_reset(struct drm_plane *plane)
> >>>         * This is the place where the state is allocated, so fill it fully.
> >>>         */
> >>>        pstate->pipe.sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe);
> >>> +     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> >>> +     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>> +
> >>> +     pstate->r_pipe.sspp = NULL;
> >>>
> >>>        __drm_atomic_helper_plane_reset(plane, &pstate->base);
> >>>    }
> >>> @@ -1423,6 +1514,7 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
> >>>
> >>>        pm_runtime_get_sync(&dpu_kms->pdev->dev);
> >>>        _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
> >>> +     _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
> >>>        pm_runtime_put_sync(&dpu_kms->pdev->dev);
> >>>    }
> >>>    #endif
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> >>> index 079dad83eb37..183c95949885 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
> >>> @@ -19,7 +19,9 @@
> >>>     * @base:   base drm plane state object
> >>>     * @aspace: pointer to address space for input/output buffers
> >>>     * @pipe:   software pipe description
> >>> + * @r_pipe:  software pipe description of the second pipe
> >>>     * @pipe_cfg:       software pipe configuration
> >>> + * @r_pipe_cfg:      software pipe configuration for the second pipe
> >>>     * @stage:  assigned by crtc blender
> >>>     * @needs_qos_remap: qos remap settings need to be updated
> >>>     * @multirect_index: index of the rectangle of SSPP
> >>> @@ -34,7 +36,9 @@ struct dpu_plane_state {
> >>>        struct drm_plane_state base;
> >>>        struct msm_gem_address_space *aspace;
> >>>        struct dpu_sw_pipe pipe;
> >>> +     struct dpu_sw_pipe r_pipe;
> >>>        struct dpu_hw_sspp_cfg pipe_cfg;
> >>> +     struct dpu_hw_sspp_cfg r_pipe_cfg;
> >>>        enum dpu_stage stage;
> >>>        bool needs_qos_remap;
> >>>        bool pending;
> >
> >
> >



--
With best wishes
Dmitry

Abhinav Kumar Feb. 9, 2023, 10:12 p.m. UTC | #5

Hi Dmitry

On 2/9/2023 1:23 PM, Dmitry Baryshkov wrote:
> Hi Abhinav,
> 
> On Thu, 9 Feb 2023 at 21:25, Abhinav Kumar <[e-mail]> wrote:
>>
>>
>>
>> On 2/9/2023 3:45 AM, Dmitry Baryshkov wrote:
>>> On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar <[e-mail]> wrote:
>>>>
>>>>
>>>>
>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>>>> Typically SSPP can support rectangle with width up to 2560. However it's
>>>>
>>>> Not always 2560. Depends on the chipset.
>>>
>>> _typically_
>>>
>>
>> Would just say maxlinewidth of SSPP instead of giving some hardcoded number.
> 
> Ack.
> 
>>
>>>>
>>>>> possible to use multirect feature and split source to use the SSPP to
>>>>> output two consecutive rectangles. This commit brings in this capability
>>>>> to support wider screen resolutions.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
>>>>>     3 files changed, 114 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> index 0ca3bc38ff7e..867832a752b2 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> @@ -485,6 +485,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>>>>>                                            fetch_active,
>>>>>                                            &pstate->pipe);
>>>>>
>>>>> +             _dpu_crtc_blend_setup_pipe(crtc, plane,
>>>>> +                                        mixer, cstate->num_mixers,
>>>>> +                                        stage_cfg, pstate->stage, 1,
>>>>> +                                        fetch_active,
>>>>> +                                        &pstate->r_pipe);
>>>>> +
>>>>>                 /* blend config update */
>>>>>                 for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
>>>>>                         _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> index e2e85688ed3c..401ead64c6bd 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>> @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>>>>>         struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>>>         struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
>>>>>
>>>>> +     if (!pipe->sspp)
>>>>> +             return;
>>>>> +
>>>>>         memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>>>>
>>>>>         if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>>>>> @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
>>>>>     {
>>>>>         struct dpu_hw_sspp_cfg pipe_cfg;
>>>>>
>>>>> +     if (!pipe->sspp)
>>>>> +             return 0;
>>>>
>>>> instead of checking if sspp was present, is it not better for the caller
>>>> to check if the rpipe is valid before calling this?
>>>>
>>>>> +
>>>>>         /* update sspp */
>>>>>         if (!pipe->sspp->ops.setup_solidfill)
>>>>>                 return 0;
>>>>> @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
>>>>>
>>>>>         /* update sspp */
>>>>>         _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
>>>>> +
>>>>> +     _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
>>>>>     }
>>>>
>>>> So cant we do
>>>>
>>>> if (pstate->r_pipe.sspp)
>>>>           _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
>>>>                   &pstate->r_pipe_cfg, fill_color, fmt);
>>>>
>>>> It just seems better to me as the caller would already know if the sspp
>>>> was assigned.
>>>
>>>    I think I had this kind of code earlier, but then I found it more
>>> logical to move the check to the called function. I'll move it back.
>>>
>>>>
>>>>>
>>>>>     int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>>>>> @@ -911,6 +919,9 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
>>>>>     {
>>>>>         uint32_t min_src_size;
>>>>>
>>>>> +     if (!pipe->sspp)
>>>>> +             return 0;
>>>>> +
>>>>>         min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>>>
>>>>>         if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>> @@ -957,9 +968,12 @@ 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;
>>>>> +     struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>>>>         const struct drm_crtc_state *crtc_state = NULL;
>>>>>         const struct dpu_format *fmt;
>>>>>         struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>>>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>>>         struct drm_rect fb_rect = { 0 };
>>>>>         uint32_t max_linewidth;
>>>>>         unsigned int rotation;
>>>>> @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>>>>>         if (!new_plane_state->visible)
>>>>>                 return 0;
>>>>>
>>>>> -     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>>>> -     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>> +     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>>>> +     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>> +     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>>>> +     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>> +     r_pipe->sspp = NULL;
>>>>>
>>>>>         pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
>>>>>         if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
>>>>> @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>>>>>
>>>>>         max_linewidth = pdpu->catalog->caps->max_linewidth;
>>>>>
>>>>> -     /* check decimated source width */
>>>>>         if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
>>>>> -             DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
>>>>> -                             DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>>>> -             return -E2BIG;
>>>>> +             /* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
>>>>> +
>>>>> +             if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
>>>>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
>>>>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>>>> +                     return -E2BIG;
>>>>> +             }
>>>>
>>>> This is where I am a bit concerned enabling it for all chipsets in one go.
>>>
>>> As I wrote earlier, I'd prefer the opt-out rather than opt-in here. It
>>> is much easier to handle the reports "I have a device with sm6543,
>>> where the display worked before 6.4, but started failing afterwards"
>>> rather than trying to find a person with sm6543 and asking him if he
>>> can enable this and that on his device. And even a lower chance of a
>>> person with sm6543 coming up with a patch 'hey, I enabled this for my
>>> phone and it works!'.
>>>
>>> If we find any issues during or close to the end of the development
>>> cycle, we can add a 'don't enable wide plane here' switch and enable
>>> it for failing platforms. But each enablement of this switch should
>>> come with a reason (wide planes not working here because ....). In the
>>> end this switch should be gone and transformed into proper HW
>>> limitation checks.
>>>
>>
>> As it has become clear that with this patch series 4K with UBWC cannot
>> be supported without true virtual planes (with two SSPPs), why do you
>> need to relax this check right now?
> 
> Yes. It enables support for 4k @ linear formats. So my plan for this
> series is to land 4k with all the proper applicable restrictions.
> 
>> You can relax this when you add the support for virtual planes till then
>> let it be this way.
>>
>> Its not going to break smartDMA as such. You can still use it for layers
>> < 2560.
>>
>> That way we stay true to the purpose of the feature. I think originally
>> you wanted to get this in for smartDMA and not to support wide plane and
>> that purpose will still be achieved even with keeping this check intact.
> 
> Actually, no. With this series I wanted to get 4k. It was developed in
> parallel with the 4k enablement for RB3 (posted, bridge patches are
> being merged for 6.3) and RB5 (delayed for now, I have other issues
> there).
> 

With the UBWC related checks, this wont support 4K for UBWC layers which 
is default on QC chipsets. So I am fine with respect to that. But still 
this does not address the product spec advertized modes. Like I 
mentioned before, relaxing the maxlinewidth check with the added UBWC 
checks is fine from DPU point of view but not from the product POV.

As things stand today, this is the only check failing the 4K modes on 
chipsets which shouldnt support 4k (linear or UBWC doesnt matter).

>> You can relax it in the virtual plane series.
>>
>> Regarding issues, this is where it gets tricky. We should be aligning
>> with what the product supports. QC will not support issues arising with
>> 4K on chipsets on which 4K is not advertized.
> 
> So, we have several different items here:
> - SmartDMA v2 per se, supporting two rectangles per VIG or DMA plane,
> - Source split support,
> - Supporting 4k modes.
> 
> I think we should tend them one by one. This series concerns SmartDMA
> v2. Using SmartDMA it is possible to use two rectangles side by side
> to emulate a wide plane. This series doesn't care at all about max
> resolutions. These two items are completely orthogonal.
> 

No its not orthogonal. Relaxing the maxlinewidth check in the 
atomic_check() will allow 4K layers now even on chipsets where 4K wasnt 
advertized. Linear or UBWC doesnt matter as the spec doesnt go into that.

>>>> As you are aware,  we have an open bug today that we do not filter out
>>>> the modes which we do not support.
>>>>
>>>> https://gitlab.freedesktop.org/drm/msm/-/issues/21
>>>
>>> I thought that with the link-frequencies in place and with the DSI
>>> checking the OPP tables this issue is mostly handled. Isn't it?
>>> Is a mode check in the DPU driver itself the last missing piece?
>>>
>>
>> opp based checking was implemented only for DSI. That one is byte clk based.
>>
>> DP uses link rate for opp table.
>>
>> Even with a 5.4G link rate (the one in sc7180 chromebook) 4k@30 would
>> still be possible but it was not advertized
>>
>> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
>>
>> These docs are available in public domain.
>>
>> As we synced up last time on
>> https://patchwork.freedesktop.org/series/107917/, even with these limits
>> in place, its not matching the advertized limits.
>>
>>>>
>>>> Due to this, on all chipsets we will end up trying to do a 4K on
>>>> external display which we dont know what bugs it will expose.
>>>
>>> If we do not expose bugs, we do not have a way to fix them. And I
>>> definitely think that all the bugs should be listed as early as
>>> possible, while both of us still remember the code under the question.
>>>
>>
>> Yes but on chipsets where 4K is supported ( and hence needed ).
> 
> 4k, SmartDMA, src-split, split-display, etc.
> 

The visual issues reported on sdm845 on the other thread are a classic 
example of what I just wrote on that patchset and thats why I was 
emphasizing a visual validation OR in other words enable the feature on 
which you are able to visually validate it.

We can evaluate and enable smartDMA on other chipsets on a need basis.

We discussed this again even today in the team discussion. Our team's 
PoV doesnt change. We would still like to enable smartDMA only on 
chipsets which can be visually validated first to limit the debugging 
effort to one chipset first and then perfect it. Otherwise its too much 
effort on QC side to debug those issues on all chipsets.


> 
>>
>>>>
>>>> So lets say if we test it on sc7280 fully but not on sc7180, we will
>>>> still hit this condition on sc7180 too but on that chipset we did not
>>>> advertise 4K as a capability in the product spec.
>>>
>>> Is it 'not advertised' or 'not supported by hw'?
>>>
>>
>> The document
>> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
>> is made from inputs from not just display team but overall system
>> limits. So even though you could argue that this falls within the
>> display capabilities, all I can say at the moment is we have to stick to
>> the advertized limits as its compiled with inputs from all the teams
>> (system/performance etc).
> 
> So, there should be a limiting factor (or a combination of them).
> Filter out 4k modes on sc7180. Or modes using fill rate higher than N.
> Pixel clock rate higher than M. But it has nothing to do with these
> patches enabling SmartDMA support on this platform.
> 
> Even if we look at the vendor kernels, we don't see 'maximum external
> resolution'. Instead I see a combination of linewidth and bandwidth
> limitations. If we can stick to that, that would be great.
> 

Can you please point me to bandwidth limitation checks? How are other 
vendors coming up with this number? It has to be based on some 
resolution too right?

My RFC https://patchwork.freedesktop.org/series/107917/ considered pixel 
clk as the limiting factor which was posted after discussions 
internally. In the absence of another way, that remains the only 
solution to tackle this.

>>
>>>>
>>>> With the max_linewidth check relaxed nothing prevents us from doing 4K
>>>> on a chipset which doesnt support 4K.
>>>
>>> What prevents sc7180 from supporting 4k? Does it support Smart DMA?
>>> Does it support having two LMs per INTF/CRTC? Is there a limitation on
>>> the linewidth of two LMs or two SSPPs?
>>>
>>> I see that sm7125 (which has the same DPU revision) even contains
>>> "qcom,sde-vig-sspp-linewidth = <4096>;" in the DTS, despite official
>>> 'product brief' advertising only 2520x1080 output resolution.
>>>
>>
>> My previous response should have answered this.
> 
> Up to some point, thanks.
> 
>>
>>>>
>>>>> +
>>>>> +             /*
>>>>> +              * FIXME: it's not possible to check if sourcesplit is supported,
>>>>> +              * LMs is not assigned yet. It happens in dpu_encoder_virt_mode_set
>>>>> +              */
>>>>> +             if (drm_rect_width(&pipe_cfg->src_rect) != drm_rect_width(&pipe_cfg->dst_rect) ||
>>>>> +                        drm_rect_height(&pipe_cfg->src_rect) != drm_rect_height(&pipe_cfg->dst_rect) ||
>>>>> +                        (!test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) &&
>>>>> +                         !test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features)) ||
>>>>> +                        /* cstate->num_mixers < 2 ||
>>>>> +                        !test_bit(DPU_MIXER_SOURCESPLIT, &cstate->mixers[0].hw_lm->cap->features) || */
>>>>> +                        DPU_FORMAT_IS_YUV(fmt)) {
>>>>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, can't use split source\n",
>>>>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>>>> +                     return -E2BIG;
>>>>> +             }
>>>>> +
>>>>> +             /* Use multirect for wide plane. We do not support dynamic assignment of SSPPs, so we know the configuration. */
>>>>> +             pipe->multirect_index = DPU_SSPP_RECT_0;
>>>>> +             pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>>> +
>>>>> +             r_pipe->sspp = pipe->sspp;
>>>>> +             r_pipe->multirect_index = DPU_SSPP_RECT_1;
>>>>> +             r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
>>>>
>>>>
>>>>> +
>>>>> +             *r_pipe_cfg = *pipe_cfg;
>>>>> +             pipe_cfg->src_rect.x2 = (pipe_cfg->src_rect.x1 + pipe_cfg->src_rect.x2) >> 1;
>>>>> +             pipe_cfg->dst_rect.x2 = (pipe_cfg->dst_rect.x1 + pipe_cfg->dst_rect.x2) >> 1;
>>>>> +             r_pipe_cfg->src_rect.x1 = pipe_cfg->src_rect.x2;
>>>>> +             r_pipe_cfg->dst_rect.x1 = pipe_cfg->dst_rect.x2;
>>>>>         }
>>>>>
>>>>
>>>> As you requested just wanted to summarize the condition in the email.
>>>>
>>>> In parallel fetch mode, the downstream driver for UBWC formats, we check
>>>> whether the src width of each rectangle is > maxlinewidth/2
>>>>
>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r3-00500-WAIPIO.0/msm/sde/sde_plane.c#L1835
>>>
>>> Thanks. Please double check my understanding: If the rectangle is used
>>> for the tiled format, then it's max_linewidth is effectively halved.
>>> So we can use rect_solo with full width, but for rect_0/rect_1 we
>>> should halve it, even if two rectangles are used in the time split?
>>>
>>
>> Not in time split mode. Only in parallel fetch mode which is being used
>> here. Rest of your understanding is correct.
> 
> Ack, thanks for the correction. This is important for plane checks.
> 
>>
>>>>
>>>> For sc7280, maxlinewidth is 2400
>>>>
>>>> static const struct dpu_caps sc7280_dpu_caps = {
>>>>            .max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
>>>>            .max_mixer_blendstages = 0x7,
>>>>            .qseed_type = DPU_SSPP_SCALER_QSEED4,
>>>>            .smart_dma_rev = DPU_SSPP_SMART_DMA_V2,
>>>>            .ubwc_version = DPU_HW_UBWC_VER_30,
>>>>            .has_dim_layer = true,
>>>>            .has_idle_pc = true,
>>>>            .max_linewidth = 2400,
>>>>            .pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
>>>> };
>>>>
>>>> Hence for UBWC formats which are by default used on the sc7280
>>>> chromebook, each rectangle should be < 1200
>>>>
>>>> SmartDMA is therefore not enough to support 4K on sc7280 and we need
>>>> true virtual planes ( using two SSPPs to display the 4K layer )
>>>>
>>>> Also, probably worth commenting that time multiplex mode support is not
>>>> added in this series.
>>>
>>> Ack.
>>>
>>>>
>>>>>         fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
>>>>>
>>>>> -     ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt);
>>>>> +     ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt);
>>>>>         if (ret)
>>>>>                 return ret;
>>>>>
>>>>> @@ -1094,8 +1148,10 @@ 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
>>>>> +     else {
>>>>>                 dpu_plane_flush_csc(pdpu, &pstate->pipe);
>>>>> +             dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
>>>>> +     }
>>>>>
>>>>>         /* flag h/w flush complete */
>>>>>         if (plane->state)
>>>>> @@ -1130,6 +1186,9 @@ static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
>>>>>         struct drm_plane_state *state = plane->state;
>>>>>         struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>>>
>>>>> +     if (!pipe->sspp)
>>>>> +             return;
>>>>> +
>>>>>         if (layout && pipe->sspp->ops.setup_sourceaddress) {
>>>>>                 trace_dpu_plane_set_scanout(pipe, layout);
>>>>>                 pipe->sspp->ops.setup_sourceaddress(pipe, layout);
>>>>> @@ -1207,13 +1266,14 @@ static void dpu_plane_sspp_atomic_update(struct drm_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 dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>>>>         struct drm_crtc *crtc = state->crtc;
>>>>>         struct drm_framebuffer *fb = state->fb;
>>>>>         bool is_rt_pipe;
>>>>>         const struct dpu_format *fmt =
>>>>>                 to_dpu_format(msm_framebuffer_format(fb));
>>>>>         struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>>>> -
>>>>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>>>         struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
>>>>>         struct msm_gem_address_space *aspace = kms->base.aspace;
>>>>>         struct dpu_hw_fmt_layout layout;
>>>>> @@ -1241,12 +1301,22 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>>>>>                                    drm_mode_vrefresh(&crtc->mode),
>>>>>                                    layout_valid ? &layout: NULL);
>>>>>
>>>>> +     dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt,
>>>>> +                                drm_mode_vrefresh(&crtc->mode),
>>>>> +                                layout_valid ? &layout: NULL);
>>>>> +
>>>>>         if (pstate->needs_qos_remap)
>>>>>                 pstate->needs_qos_remap = false;
>>>>>
>>>>>         pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, pipe_cfg);
>>>>>
>>>>>         pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, pipe_cfg);
>>>>> +
>>>>> +     if (r_pipe->sspp) {
>>>>> +             pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, r_pipe_cfg);
>>>>> +
>>>>> +             pstate->plane_clk = max(pstate->plane_clk, _dpu_plane_calc_clk(&crtc->mode, r_pipe_cfg));
>>>>> +     }
>>>>>     }
>>>>>
>>>>>     static void _dpu_plane_atomic_disable(struct drm_plane *plane)
>>>>> @@ -1289,6 +1359,8 @@ static void dpu_plane_destroy(struct drm_plane *plane)
>>>>>                 pstate = to_dpu_plane_state(plane->state);
>>>>>                 _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
>>>>>
>>>>> +             _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
>>>>> +
>>>>>                 mutex_destroy(&pdpu->lock);
>>>>>
>>>>>                 /* this will destroy the states as well */
>>>>> @@ -1369,11 +1441,26 @@ static void dpu_plane_atomic_print_state(struct drm_printer *p,
>>>>>                 const struct drm_plane_state *state)
>>>>>     {
>>>>>         const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
>>>>> +     const struct dpu_sw_pipe *pipe = &pstate->pipe;
>>>>> +     const struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>>>> +     const struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>>>> +     const struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>>>
>>>>>         drm_printf(p, "\tstage=%d\n", pstate->stage);
>>>>> -     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));
>>>>> +
>>>>> +     drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name);
>>>>> +     drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode));
>>>>> +     drm_printf(p, "\tmultirect_index[0]=%s\n", dpu_get_multirect_index(pipe->multirect_index));
>>>>> +     drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect));
>>>>> +     drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect));
>>>>> +
>>>>> +     if (r_pipe->sspp) {
>>>>> +             drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name);
>>>>> +             drm_printf(p, "\tmultirect_mode[1]=%s\n", dpu_get_multirect_mode(r_pipe->multirect_mode));
>>>>> +             drm_printf(p, "\tmultirect_index[1]=%s\n", dpu_get_multirect_index(r_pipe->multirect_index));
>>>>> +             drm_printf(p, "\tsrc[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->src_rect));
>>>>> +             drm_printf(p, "\tdst[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->dst_rect));
>>>>> +     }
>>>>>     }
>>>>
>>>> Do you think that changing the atomic_print_state to print the r_pipe
>>>> sspp can be moved to a separate patch? So that way we only keep the core
>>>> logic of atomic check of smartDMA in this patch.
>>>>
>>>>>
>>>>>     static void dpu_plane_reset(struct drm_plane *plane)
>>>>> @@ -1407,6 +1494,10 @@ static void dpu_plane_reset(struct drm_plane *plane)
>>>>>          * This is the place where the state is allocated, so fill it fully.
>>>>>          */
>>>>>         pstate->pipe.sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe);
>>>>> +     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>>>> +     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>> +
>>>>> +     pstate->r_pipe.sspp = NULL;
>>>>>
>>>>>         __drm_atomic_helper_plane_reset(plane, &pstate->base);
>>>>>     }
>>>>> @@ -1423,6 +1514,7 @@ void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>>>>>
>>>>>         pm_runtime_get_sync(&dpu_kms->pdev->dev);
>>>>>         _dpu_plane_set_qos_ctrl(plane, &pstate->pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
>>>>> +     _dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
>>>>>         pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>>>     }
>>>>>     #endif
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> index 079dad83eb37..183c95949885 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
>>>>> @@ -19,7 +19,9 @@
>>>>>      * @base:   base drm plane state object
>>>>>      * @aspace: pointer to address space for input/output buffers
>>>>>      * @pipe:   software pipe description
>>>>> + * @r_pipe:  software pipe description of the second pipe
>>>>>      * @pipe_cfg:       software pipe configuration
>>>>> + * @r_pipe_cfg:      software pipe configuration for the second pipe
>>>>>      * @stage:  assigned by crtc blender
>>>>>      * @needs_qos_remap: qos remap settings need to be updated
>>>>>      * @multirect_index: index of the rectangle of SSPP
>>>>> @@ -34,7 +36,9 @@ struct dpu_plane_state {
>>>>>         struct drm_plane_state base;
>>>>>         struct msm_gem_address_space *aspace;
>>>>>         struct dpu_sw_pipe pipe;
>>>>> +     struct dpu_sw_pipe r_pipe;
>>>>>         struct dpu_hw_sspp_cfg pipe_cfg;
>>>>> +     struct dpu_hw_sspp_cfg r_pipe_cfg;
>>>>>         enum dpu_stage stage;
>>>>>         bool needs_qos_remap;
>>>>>         bool pending;
>>>
>>>
>>>
> 
> 
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov Feb. 10, 2023, 12:09 a.m. UTC | #6

.

On Fri, 10 Feb 2023 at 00:12, Abhinav Kumar <[e-mail]> wrote:
>
> Hi Dmitry
>
> On 2/9/2023 1:23 PM, Dmitry Baryshkov wrote:
> > Hi Abhinav,
> >
> > On Thu, 9 Feb 2023 at 21:25, Abhinav Kumar <[e-mail]> wrote:
> >>
> >>
> >>
> >> On 2/9/2023 3:45 AM, Dmitry Baryshkov wrote:
> >>> On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar <[e-mail]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> >>>>> Typically SSPP can support rectangle with width up to 2560. However it's
> >>>>
> >>>> Not always 2560. Depends on the chipset.
> >>>
> >>> _typically_
> >>>
> >>
> >> Would just say maxlinewidth of SSPP instead of giving some hardcoded number.
> >
> > Ack.
> >
> >>
> >>>>
> >>>>> possible to use multirect feature and split source to use the SSPP to
> >>>>> output two consecutive rectangles. This commit brings in this capability
> >>>>> to support wider screen resolutions.
> >>>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> >>>>> ---
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
> >>>>>     3 files changed, 114 insertions(+), 12 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>>>> index 0ca3bc38ff7e..867832a752b2 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>>>> @@ -485,6 +485,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
> >>>>>                                            fetch_active,
> >>>>>                                            &pstate->pipe);
> >>>>>
> >>>>> +             _dpu_crtc_blend_setup_pipe(crtc, plane,
> >>>>> +                                        mixer, cstate->num_mixers,
> >>>>> +                                        stage_cfg, pstate->stage, 1,
> >>>>> +                                        fetch_active,
> >>>>> +                                        &pstate->r_pipe);
> >>>>> +
> >>>>>                 /* blend config update */
> >>>>>                 for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
> >>>>>                         _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>>>> index e2e85688ed3c..401ead64c6bd 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> >>>>> @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
> >>>>>         struct dpu_plane *pdpu = to_dpu_plane(plane);
> >>>>>         struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
> >>>>>
> >>>>> +     if (!pipe->sspp)
> >>>>> +             return;
> >>>>> +
> >>>>>         memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
> >>>>>
> >>>>>         if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
> >>>>> @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
> >>>>>     {
> >>>>>         struct dpu_hw_sspp_cfg pipe_cfg;
> >>>>>
> >>>>> +     if (!pipe->sspp)
> >>>>> +             return 0;
> >>>>
> >>>> instead of checking if sspp was present, is it not better for the caller
> >>>> to check if the rpipe is valid before calling this?
> >>>>
> >>>>> +
> >>>>>         /* update sspp */
> >>>>>         if (!pipe->sspp->ops.setup_solidfill)
> >>>>>                 return 0;
> >>>>> @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
> >>>>>
> >>>>>         /* update sspp */
> >>>>>         _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
> >>>>> +
> >>>>> +     _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
> >>>>>     }
> >>>>
> >>>> So cant we do
> >>>>
> >>>> if (pstate->r_pipe.sspp)
> >>>>           _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
> >>>>                   &pstate->r_pipe_cfg, fill_color, fmt);
> >>>>
> >>>> It just seems better to me as the caller would already know if the sspp
> >>>> was assigned.
> >>>
> >>>    I think I had this kind of code earlier, but then I found it more
> >>> logical to move the check to the called function. I'll move it back.
> >>>
> >>>>
> >>>>>
> >>>>>     int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
> >>>>> @@ -911,6 +919,9 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
> >>>>>     {
> >>>>>         uint32_t min_src_size;
> >>>>>
> >>>>> +     if (!pipe->sspp)
> >>>>> +             return 0;
> >>>>> +
> >>>>>         min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> >>>>>
> >>>>>         if (DPU_FORMAT_IS_YUV(fmt) &&
> >>>>> @@ -957,9 +968,12 @@ 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;
> >>>>> +     struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
> >>>>>         const struct drm_crtc_state *crtc_state = NULL;
> >>>>>         const struct dpu_format *fmt;
> >>>>>         struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
> >>>>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
> >>>>>         struct drm_rect fb_rect = { 0 };
> >>>>>         uint32_t max_linewidth;
> >>>>>         unsigned int rotation;
> >>>>> @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >>>>>         if (!new_plane_state->visible)
> >>>>>                 return 0;
> >>>>>
> >>>>> -     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
> >>>>> -     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>>>> +     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>>>> +     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>>>> +     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
> >>>>> +     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> >>>>> +     r_pipe->sspp = NULL;
> >>>>>
> >>>>>         pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
> >>>>>         if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
> >>>>> @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> >>>>>
> >>>>>         max_linewidth = pdpu->catalog->caps->max_linewidth;
> >>>>>
> >>>>> -     /* check decimated source width */
> >>>>>         if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
> >>>>> -             DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> >>>>> -                             DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> >>>>> -             return -E2BIG;
> >>>>> +             /* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
> >>>>> +
> >>>>> +             if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
> >>>>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
> >>>>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
> >>>>> +                     return -E2BIG;
> >>>>> +             }
> >>>>
> >>>> This is where I am a bit concerned enabling it for all chipsets in one go.
> >>>
> >>> As I wrote earlier, I'd prefer the opt-out rather than opt-in here. It
> >>> is much easier to handle the reports "I have a device with sm6543,
> >>> where the display worked before 6.4, but started failing afterwards"
> >>> rather than trying to find a person with sm6543 and asking him if he
> >>> can enable this and that on his device. And even a lower chance of a
> >>> person with sm6543 coming up with a patch 'hey, I enabled this for my
> >>> phone and it works!'.
> >>>
> >>> If we find any issues during or close to the end of the development
> >>> cycle, we can add a 'don't enable wide plane here' switch and enable
> >>> it for failing platforms. But each enablement of this switch should
> >>> come with a reason (wide planes not working here because ....). In the
> >>> end this switch should be gone and transformed into proper HW
> >>> limitation checks.
> >>>
> >>
> >> As it has become clear that with this patch series 4K with UBWC cannot
> >> be supported without true virtual planes (with two SSPPs), why do you
> >> need to relax this check right now?
> >
> > Yes. It enables support for 4k @ linear formats. So my plan for this
> > series is to land 4k with all the proper applicable restrictions.
> >
> >> You can relax this when you add the support for virtual planes till then
> >> let it be this way.
> >>
> >> Its not going to break smartDMA as such. You can still use it for layers
> >> < 2560.
> >>
> >> That way we stay true to the purpose of the feature. I think originally
> >> you wanted to get this in for smartDMA and not to support wide plane and
> >> that purpose will still be achieved even with keeping this check intact.
> >
> > Actually, no. With this series I wanted to get 4k. It was developed in
> > parallel with the 4k enablement for RB3 (posted, bridge patches are
> > being merged for 6.3) and RB5 (delayed for now, I have other issues
> > there).
> >
>
> With the UBWC related checks, this wont support 4K for UBWC layers which
> is default on QC chipsets. So I am fine with respect to that. But still
> this does not address the product spec advertized modes. Like I
> mentioned before, relaxing the maxlinewidth check with the added UBWC
> checks is fine from DPU point of view but not from the product POV.
>
> As things stand today, this is the only check failing the 4K modes on
> chipsets which shouldnt support 4k (linear or UBWC doesnt matter).
>
> >> You can relax it in the virtual plane series.
> >>
> >> Regarding issues, this is where it gets tricky. We should be aligning
> >> with what the product supports. QC will not support issues arising with
> >> 4K on chipsets on which 4K is not advertized.
> >
> > So, we have several different items here:
> > - SmartDMA v2 per se, supporting two rectangles per VIG or DMA plane,
> > - Source split support,
> > - Supporting 4k modes.
> >
> > I think we should tend them one by one. This series concerns SmartDMA
> > v2. Using SmartDMA it is possible to use two rectangles side by side
> > to emulate a wide plane. This series doesn't care at all about max
> > resolutions. These two items are completely orthogonal.
> >
>
> No its not orthogonal. Relaxing the maxlinewidth check in the
> atomic_check() will allow 4K layers now even on chipsets where 4K wasnt
> advertized. Linear or UBWC doesnt matter as the spec doesnt go into that.

Please correct my answers, if I got something wrong here:

Does sc7180 support SmartDMA? Yes it does.
Can QC or CrOS validate SmartDMA separately on sc7180? I hope you can.
Should the hw-supported feature be enabled? Yes, it should.

Now limiting out 4k by not supporting SmartDMA looks like a misfeature.
I can only suggest sending a change to block 4k on sc7180.

> >>>> As you are aware,  we have an open bug today that we do not filter out
> >>>> the modes which we do not support.
> >>>>
> >>>> https://gitlab.freedesktop.org/drm/msm/-/issues/21
> >>>
> >>> I thought that with the link-frequencies in place and with the DSI
> >>> checking the OPP tables this issue is mostly handled. Isn't it?
> >>> Is a mode check in the DPU driver itself the last missing piece?
> >>>
> >>
> >> opp based checking was implemented only for DSI. That one is byte clk based.
> >>
> >> DP uses link rate for opp table.
> >>
> >> Even with a 5.4G link rate (the one in sc7180 chromebook) 4k@30 would
> >> still be possible but it was not advertized
> >>
> >> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
> >>
> >> These docs are available in public domain.
> >>
> >> As we synced up last time on
> >> https://patchwork.freedesktop.org/series/107917/, even with these limits
> >> in place, its not matching the advertized limits.
> >>
> >>>>
> >>>> Due to this, on all chipsets we will end up trying to do a 4K on
> >>>> external display which we dont know what bugs it will expose.
> >>>
> >>> If we do not expose bugs, we do not have a way to fix them. And I
> >>> definitely think that all the bugs should be listed as early as
> >>> possible, while both of us still remember the code under the question.
> >>>
> >>
> >> Yes but on chipsets where 4K is supported ( and hence needed ).
> >
> > 4k, SmartDMA, src-split, split-display, etc.
> >
>
> The visual issues reported on sdm845 on the other thread are a classic
> example of what I just wrote on that patchset and thats why I was
> emphasizing a visual validation OR in other words enable the feature on
> which you are able to visually validate it.

Yes. And if we did not enable the feature, Amit would not be able to
spot that. I can repeat my suggestion:

- prevalidate these features for the accessible platforms (e.g. I only
have sdm845, sm8250 and sm8350 at hand)
- enable SmartDMA for all chipsets where SmartDMA is supported
- collect possible tested-by and broken-at reports, while the patches
sit in linux-next
- disable SmartDMA basing on the feedback from the previous step (e.g.
select from 'mostly disable', 'disable for the bugged cases', 'do not
disable at all', etc).
  I can promise that if we see a significant validation failure rate I
will not oppose disabling SmartDMA.

As a reminder: if the patchset is ready at the time of 6.3-rc1 (in
three weeks from now), it is going to be merged into linux-next first,
after that it can go into the main Linus'es tree at 6.4-rc1. So we
will have _two_ kernel cycles to collect bug reports and to disable
(or fix) broken cases.

> We can evaluate and enable smartDMA on other chipsets on a need basis.
>
> We discussed this again even today in the team discussion. Our team's
> PoV doesnt change. We would still like to enable smartDMA only on
> chipsets which can be visually validated first to limit the debugging
> effort to one chipset first and then perfect it. Otherwise its too much
> effort on QC side to debug those issues on all chipsets.

I think QC mostly debugs issues on sc7180/sc7280 and sometimes on
sdm845/sm8250 (and now on sm8350). I think we can let people
(somainline, PmOS) test features on other platforms.
And testing happens better if we can say 'please test linux-next'
rather than 'please test linux-next + this patch to enable the feature
+ that patch to enable the second patch of the feature'.

> >>>> So lets say if we test it on sc7280 fully but not on sc7180, we will
> >>>> still hit this condition on sc7180 too but on that chipset we did not
> >>>> advertise 4K as a capability in the product spec.
> >>>
> >>> Is it 'not advertised' or 'not supported by hw'?
> >>>
> >>
> >> The document
> >> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
> >> is made from inputs from not just display team but overall system
> >> limits. So even though you could argue that this falls within the
> >> display capabilities, all I can say at the moment is we have to stick to
> >> the advertized limits as its compiled with inputs from all the teams
> >> (system/performance etc).
> >
> > So, there should be a limiting factor (or a combination of them).
> > Filter out 4k modes on sc7180. Or modes using fill rate higher than N.
> > Pixel clock rate higher than M. But it has nothing to do with these
> > patches enabling SmartDMA support on this platform.
> >
> > Even if we look at the vendor kernels, we don't see 'maximum external
> > resolution'. Instead I see a combination of linewidth and bandwidth
> > limitations. If we can stick to that, that would be great.
> >
>
> Can you please point me to bandwidth limitation checks? How are other
> vendors coming up with this number? It has to be based on some
> resolution too right?

Usually it is considered in the other direction. The SoC can support
this-and-that pixel clock and bandwidth, so put the NxM resolution to
the datasheet as the max supported one.

Can you point out how the vendor kernel limits DP modes? I checked out
several dtsi files. For sm8250 the DP is limited to 1920x1080 (while
PB explicitly mentions 4k@60).
sm7125 is limited to 2560x1600. sm6150 again 1920x1080. From the pile
of the DTS that I have here the rest lists only
qcom,max-pclk-frequency-khz

> My RFC https://patchwork.freedesktop.org/series/107917/ considered pixel
> clk as the limiting factor which was posted after discussions
> internally. In the absence of another way, that remains the only
> solution to tackle this.

If I remember correctly the mentioned patchset used manually crafted
pixel clocks. And for example for sm8250 this clock doesn't correspond
to verifiable source.
Your patchset used 594000 KHz as max ext pclk for sm8250. vendor dtsi
lists 187500 KHz as maximum DP pclk for RB5 and 150000 KHz in all
other cases. And, at the same time, it lists a 3840x2160@60 mode for
the DSI/lt9611uxc with the pixel clock as high as 608040 KHz.

I suggest we stop the discussion at this point, unless there is
anything else wrong with this patchset itself.

I noted the point regarding UBWC & parallel mode. I will handle it in
the next iteration.

I noted your valid point about the visual verification. I proposed a
way to ease validation, allowing it to be enabled for testers and
early adopters for nearly two cycles. I hope you'd agree to this plan.
I on the other side agree to revert to opt-in if the failure rate is
high.

Please stop bringing max resolution issues to this patchserie. It must
be handled separately. I hope to see the mode filter patch targeting
sc7180 & sc7280. With the DSI opp check in place I think we should
concentrate on the DP case. If nothing else, I think even adding the
max PCLK to the msm_dp_desc should be sufficient to your worries. I'd
prefer to be able to override it for the particular board, but I think
this can come later, as it would require an agreement from the DT
schema team.

--
With best wishes
Dmitry

Abhinav Kumar Feb. 10, 2023, 1:12 a.m. UTC | #7

Hi Dmitry

On 2/9/2023 4:09 PM, Dmitry Baryshkov wrote:
>   .
> 
> On Fri, 10 Feb 2023 at 00:12, Abhinav Kumar <[e-mail]> wrote:
>>
>> Hi Dmitry
>>
>> On 2/9/2023 1:23 PM, Dmitry Baryshkov wrote:
>>> Hi Abhinav,
>>>
>>> On Thu, 9 Feb 2023 at 21:25, Abhinav Kumar <[e-mail]> wrote:
>>>>
>>>>
>>>>
>>>> On 2/9/2023 3:45 AM, Dmitry Baryshkov wrote:
>>>>> On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar <[e-mail]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>>>>>> Typically SSPP can support rectangle with width up to 2560. However it's
>>>>>>
>>>>>> Not always 2560. Depends on the chipset.
>>>>>
>>>>> _typically_
>>>>>
>>>>
>>>> Would just say maxlinewidth of SSPP instead of giving some hardcoded number.
>>>
>>> Ack.
>>>
>>>>
>>>>>>
>>>>>>> possible to use multirect feature and split source to use the SSPP to
>>>>>>> output two consecutive rectangles. This commit brings in this capability
>>>>>>> to support wider screen resolutions.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 +++++++++++++++++++---
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
>>>>>>>      3 files changed, 114 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> index 0ca3bc38ff7e..867832a752b2 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> @@ -485,6 +485,12 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>>>>>>>                                             fetch_active,
>>>>>>>                                             &pstate->pipe);
>>>>>>>
>>>>>>> +             _dpu_crtc_blend_setup_pipe(crtc, plane,
>>>>>>> +                                        mixer, cstate->num_mixers,
>>>>>>> +                                        stage_cfg, pstate->stage, 1,
>>>>>>> +                                        fetch_active,
>>>>>>> +                                        &pstate->r_pipe);
>>>>>>> +
>>>>>>>                  /* blend config update */
>>>>>>>                  for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
>>>>>>>                          _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>> index e2e85688ed3c..401ead64c6bd 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>> @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>>>>>>>          struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>>>>>          struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
>>>>>>>
>>>>>>> +     if (!pipe->sspp)
>>>>>>> +             return;
>>>>>>> +
>>>>>>>          memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>>>>>>
>>>>>>>          if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>>>>>>> @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
>>>>>>>      {
>>>>>>>          struct dpu_hw_sspp_cfg pipe_cfg;
>>>>>>>
>>>>>>> +     if (!pipe->sspp)
>>>>>>> +             return 0;
>>>>>>
>>>>>> instead of checking if sspp was present, is it not better for the caller
>>>>>> to check if the rpipe is valid before calling this?
>>>>>>
>>>>>>> +
>>>>>>>          /* update sspp */
>>>>>>>          if (!pipe->sspp->ops.setup_solidfill)
>>>>>>>                  return 0;
>>>>>>> @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
>>>>>>>
>>>>>>>          /* update sspp */
>>>>>>>          _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
>>>>>>> +
>>>>>>> +     _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
>>>>>>>      }
>>>>>>
>>>>>> So cant we do
>>>>>>
>>>>>> if (pstate->r_pipe.sspp)
>>>>>>            _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
>>>>>>                    &pstate->r_pipe_cfg, fill_color, fmt);
>>>>>>
>>>>>> It just seems better to me as the caller would already know if the sspp
>>>>>> was assigned.
>>>>>
>>>>>     I think I had this kind of code earlier, but then I found it more
>>>>> logical to move the check to the called function. I'll move it back.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>      int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>>>>>>> @@ -911,6 +919,9 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
>>>>>>>      {
>>>>>>>          uint32_t min_src_size;
>>>>>>>
>>>>>>> +     if (!pipe->sspp)
>>>>>>> +             return 0;
>>>>>>> +
>>>>>>>          min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>>>>>
>>>>>>>          if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>>>> @@ -957,9 +968,12 @@ 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;
>>>>>>> +     struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>>>>>>          const struct drm_crtc_state *crtc_state = NULL;
>>>>>>>          const struct dpu_format *fmt;
>>>>>>>          struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>>>>>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>>>>>          struct drm_rect fb_rect = { 0 };
>>>>>>>          uint32_t max_linewidth;
>>>>>>>          unsigned int rotation;
>>>>>>> @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>>>>>>>          if (!new_plane_state->visible)
>>>>>>>                  return 0;
>>>>>>>
>>>>>>> -     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>>>>>> -     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>>>> +     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>>>>>> +     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>>>> +     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>>>>>> +     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>>>> +     r_pipe->sspp = NULL;
>>>>>>>
>>>>>>>          pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
>>>>>>>          if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
>>>>>>> @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>>>>>>>
>>>>>>>          max_linewidth = pdpu->catalog->caps->max_linewidth;
>>>>>>>
>>>>>>> -     /* check decimated source width */
>>>>>>>          if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
>>>>>>> -             DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
>>>>>>> -                             DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>>>>>> -             return -E2BIG;
>>>>>>> +             /* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
>>>>>>> +
>>>>>>> +             if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
>>>>>>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
>>>>>>> +                                     DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>>>>>> +                     return -E2BIG;
>>>>>>> +             }
>>>>>>
>>>>>> This is where I am a bit concerned enabling it for all chipsets in one go.
>>>>>
>>>>> As I wrote earlier, I'd prefer the opt-out rather than opt-in here. It
>>>>> is much easier to handle the reports "I have a device with sm6543,
>>>>> where the display worked before 6.4, but started failing afterwards"
>>>>> rather than trying to find a person with sm6543 and asking him if he
>>>>> can enable this and that on his device. And even a lower chance of a
>>>>> person with sm6543 coming up with a patch 'hey, I enabled this for my
>>>>> phone and it works!'.
>>>>>
>>>>> If we find any issues during or close to the end of the development
>>>>> cycle, we can add a 'don't enable wide plane here' switch and enable
>>>>> it for failing platforms. But each enablement of this switch should
>>>>> come with a reason (wide planes not working here because ....). In the
>>>>> end this switch should be gone and transformed into proper HW
>>>>> limitation checks.
>>>>>
>>>>
>>>> As it has become clear that with this patch series 4K with UBWC cannot
>>>> be supported without true virtual planes (with two SSPPs), why do you
>>>> need to relax this check right now?
>>>
>>> Yes. It enables support for 4k @ linear formats. So my plan for this
>>> series is to land 4k with all the proper applicable restrictions.
>>>
>>>> You can relax this when you add the support for virtual planes till then
>>>> let it be this way.
>>>>
>>>> Its not going to break smartDMA as such. You can still use it for layers
>>>> < 2560.
>>>>
>>>> That way we stay true to the purpose of the feature. I think originally
>>>> you wanted to get this in for smartDMA and not to support wide plane and
>>>> that purpose will still be achieved even with keeping this check intact.
>>>
>>> Actually, no. With this series I wanted to get 4k. It was developed in
>>> parallel with the 4k enablement for RB3 (posted, bridge patches are
>>> being merged for 6.3) and RB5 (delayed for now, I have other issues
>>> there).
>>>
>>
>> With the UBWC related checks, this wont support 4K for UBWC layers which
>> is default on QC chipsets. So I am fine with respect to that. But still
>> this does not address the product spec advertized modes. Like I
>> mentioned before, relaxing the maxlinewidth check with the added UBWC
>> checks is fine from DPU point of view but not from the product POV.
>>
>> As things stand today, this is the only check failing the 4K modes on
>> chipsets which shouldnt support 4k (linear or UBWC doesnt matter).
>>
>>>> You can relax it in the virtual plane series.
>>>>
>>>> Regarding issues, this is where it gets tricky. We should be aligning
>>>> with what the product supports. QC will not support issues arising with
>>>> 4K on chipsets on which 4K is not advertized.
>>>
>>> So, we have several different items here:
>>> - SmartDMA v2 per se, supporting two rectangles per VIG or DMA plane,
>>> - Source split support,
>>> - Supporting 4k modes.
>>>
>>> I think we should tend them one by one. This series concerns SmartDMA
>>> v2. Using SmartDMA it is possible to use two rectangles side by side
>>> to emulate a wide plane. This series doesn't care at all about max
>>> resolutions. These two items are completely orthogonal.
>>>
>>
>> No its not orthogonal. Relaxing the maxlinewidth check in the
>> atomic_check() will allow 4K layers now even on chipsets where 4K wasnt
>> advertized. Linear or UBWC doesnt matter as the spec doesnt go into that.
> 
> Please correct my answers, if I got something wrong here:
> 
> Does sc7180 support SmartDMA? Yes it does.
> Can QC or CrOS validate SmartDMA separately on sc7180? I hope you can.
> Should the hw-supported feature be enabled? Yes, it should.
> 
> Now limiting out 4k by not supporting SmartDMA looks like a misfeature.
> I can only suggest sending a change to block 4k on sc7180.
> 

Agreed. We should first block 4K on sc7180 and other devices which dont 
advertize 4k. No, it was never a question of limiting out 4K by not 
supporting smartDMA but the other way around. By supporting smartDMA, we 
would have had to support 4K on some chipsets due to this change which I 
didnt want to do.

And yes agreed that we can stop discussing 4K anymore on this patch but 
not smartDMA itself. Please see below.

>>>>>> As you are aware,  we have an open bug today that we do not filter out
>>>>>> the modes which we do not support.
>>>>>>
>>>>>> https://gitlab.freedesktop.org/drm/msm/-/issues/21
>>>>>
>>>>> I thought that with the link-frequencies in place and with the DSI
>>>>> checking the OPP tables this issue is mostly handled. Isn't it?
>>>>> Is a mode check in the DPU driver itself the last missing piece?
>>>>>
>>>>
>>>> opp based checking was implemented only for DSI. That one is byte clk based.
>>>>
>>>> DP uses link rate for opp table.
>>>>
>>>> Even with a 5.4G link rate (the one in sc7180 chromebook) 4k@30 would
>>>> still be possible but it was not advertized
>>>>
>>>> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
>>>>
>>>> These docs are available in public domain.
>>>>
>>>> As we synced up last time on
>>>> https://patchwork.freedesktop.org/series/107917/, even with these limits
>>>> in place, its not matching the advertized limits.
>>>>
>>>>>>
>>>>>> Due to this, on all chipsets we will end up trying to do a 4K on
>>>>>> external display which we dont know what bugs it will expose.
>>>>>
>>>>> If we do not expose bugs, we do not have a way to fix them. And I
>>>>> definitely think that all the bugs should be listed as early as
>>>>> possible, while both of us still remember the code under the question.
>>>>>
>>>>
>>>> Yes but on chipsets where 4K is supported ( and hence needed ).
>>>
>>> 4k, SmartDMA, src-split, split-display, etc.
>>>
>>
>> The visual issues reported on sdm845 on the other thread are a classic
>> example of what I just wrote on that patchset and thats why I was
>> emphasizing a visual validation OR in other words enable the feature on
>> which you are able to visually validate it.
> 
> Yes. And if we did not enable the feature, Amit would not be able to
> spot that. I can repeat my suggestion:
> 
> - prevalidate these features for the accessible platforms (e.g. I only
> have sdm845, sm8250 and sm8350 at hand)

How? Your validation is not with a compositor in most cases and just 
with modetest.

> - enable SmartDMA for all chipsets where SmartDMA is supported
> - collect possible tested-by and broken-at reports, while the patches
> sit in linux-next

Question:

1) Who will test these on all the devices for us? This is what I had 
written even before that someone should give Tested-by tags then.

Even if you have them on linux-next, someone has to test them voluntarily.

2) Who will look at these broken-at reports and debug them?

> - disable SmartDMA basing on the feedback from the previous step (e.g.
> select from 'mostly disable', 'disable for the bugged cases', 'do not
> disable at all', etc).
>    I can promise that if we see a significant validation failure rate I
> will not oppose disabling SmartDMA.
> 

Thanks for agreeing on this :)

> As a reminder: if the patchset is ready at the time of 6.3-rc1 (in
> three weeks from now), it is going to be merged into linux-next first,
> after that it can go into the main Linus'es tree at 6.4-rc1. So we
> will have _two_ kernel cycles to collect bug reports and to disable
> (or fix) broken cases.
> 

>> We can evaluate and enable smartDMA on other chipsets on a need basis.
>>
>> We discussed this again even today in the team discussion. Our team's
>> PoV doesnt change. We would still like to enable smartDMA only on
>> chipsets which can be visually validated first to limit the debugging
>> effort to one chipset first and then perfect it. Otherwise its too much
>> effort on QC side to debug those issues on all chipsets.
> 
> I think QC mostly debugs issues on sc7180/sc7280 and sometimes on
> sdm845/sm8250 (and now on sm8350). I think we can let people
> (somainline, PmOS) test features on other platforms.
> And testing happens better if we can say 'please test linux-next'
> rather than 'please test linux-next + this patch to enable the feature
> + that patch to enable the second patch of the feature'.
> 

In this particular case, in the current timeframe QC can only commit to 
validate sc7280 at this point and not sc7180, sdm845, sm8250 and sm8350 
for this feature.

So it will be upto you and others for all other targets.

Yes, I agree, people prefer to test on a branch rather than branch + 
patches.

>>>>>> So lets say if we test it on sc7280 fully but not on sc7180, we will
>>>>>> still hit this condition on sc7180 too but on that chipset we did not
>>>>>> advertise 4K as a capability in the product spec.
>>>>>
>>>>> Is it 'not advertised' or 'not supported by hw'?
>>>>>
>>>>
>>>> The document
>>>> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
>>>> is made from inputs from not just display team but overall system
>>>> limits. So even though you could argue that this falls within the
>>>> display capabilities, all I can say at the moment is we have to stick to
>>>> the advertized limits as its compiled with inputs from all the teams
>>>> (system/performance etc).
>>>
>>> So, there should be a limiting factor (or a combination of them).
>>> Filter out 4k modes on sc7180. Or modes using fill rate higher than N.
>>> Pixel clock rate higher than M. But it has nothing to do with these
>>> patches enabling SmartDMA support on this platform.
>>>
>>> Even if we look at the vendor kernels, we don't see 'maximum external
>>> resolution'. Instead I see a combination of linewidth and bandwidth
>>> limitations. If we can stick to that, that would be great.
>>>
>>
>> Can you please point me to bandwidth limitation checks? How are other
>> vendors coming up with this number? It has to be based on some
>> resolution too right?
> 
> Usually it is considered in the other direction. The SoC can support
> this-and-that pixel clock and bandwidth, so put the NxM resolution to
> the datasheet as the max supported one.
>
Our data sheet also gave the resolution so I chose the pixel clk last time.

> Can you point out how the vendor kernel limits DP modes? I checked out
> several dtsi files. For sm8250 the DP is limited to 1920x1080 (while
> PB explicitly mentions 4k@60).

Where do you see 1920x1080? In kona-sde.dtsi I see 675000

> sm7125 is limited to 2560x1600. sm6150 again 1920x1080. From the pile
> of the DTS that I have here the rest lists only
> qcom,max-pclk-frequency-khz

Yes, qcom,max-pclk-frequency-khz is the method they use.

> 
>> My RFC https://patchwork.freedesktop.org/series/107917/ considered pixel
>> clk as the limiting factor which was posted after discussions
>> internally. In the absence of another way, that remains the only
>> solution to tackle this.
> 
> If I remember correctly the mentioned patchset used manually crafted
> pixel clocks. And for example for sm8250 this clock doesn't correspond
> to verifiable source.
> Your patchset used 594000 KHz as max ext pclk for sm8250. vendor dtsi
> lists 187500 KHz as maximum DP pclk for RB5 and 150000 KHz in all

Lets discuss this more when I re-post the patch to limit 4K modes.

> other cases. And, at the same time, it lists a 3840x2160@60 mode for
> the DSI/lt9611uxc with the pixel clock as high as 608040 KHz.
> 
> I suggest we stop the discussion at this point, unless there is
> anything else wrong with this patchset itself.
> 

Agreed but please look at my comments about smartDMA validation above 
and let me know.

> I noted the point regarding UBWC & parallel mode. I will handle it in
> the next iteration.
> 
> I noted your valid point about the visual verification. I proposed a
> way to ease validation, allowing it to be enabled for testers and
> early adopters for nearly two cycles. I hope you'd agree to this plan.
> I on the other side agree to revert to opt-in if the failure rate is
> high.
> 

Agreed but with above concerns.

> Please stop bringing max resolution issues to this patchserie. It must

It got dragged into this patch because this patch relaxed the checks for 
max resolution.

> be handled separately. I hope to see the mode filter patch targeting
> sc7180 & sc7280. With the DSI opp check in place I think we should
> concentrate on the DP case. If nothing else, I think even adding the
> max PCLK to the msm_dp_desc should be sufficient to your worries. I'd
> prefer to be able to override it for the particular board, but I think
> this can come later, as it would require an agreement from the DT
> schema team.
> 

Alright, I can rebase that RFC to cover the DP cases for now and we can 
discuss all the max resolution discussions there.

> --
> With best wishes
> Dmitry

Dmitry Baryshkov Feb. 10, 2023, 2:46 a.m. UTC | #8

On 10/02/2023 03:12, Abhinav Kumar wrote:
> Hi Dmitry
> 
> On 2/9/2023 4:09 PM, Dmitry Baryshkov wrote:
>>   .
>>
>> On Fri, 10 Feb 2023 at 00:12, Abhinav Kumar 
>> <[e-mail]> wrote:
>>>
>>> Hi Dmitry
>>>
>>> On 2/9/2023 1:23 PM, Dmitry Baryshkov wrote:
>>>> Hi Abhinav,
>>>>
>>>> On Thu, 9 Feb 2023 at 21:25, Abhinav Kumar 
>>>> <[e-mail]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2/9/2023 3:45 AM, Dmitry Baryshkov wrote:
>>>>>> On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar 
>>>>>> <[e-mail]> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>>>>>>> Typically SSPP can support rectangle with width up to 2560. 
>>>>>>>> However it's
>>>>>>>
>>>>>>> Not always 2560. Depends on the chipset.
>>>>>>
>>>>>> _typically_
>>>>>>
>>>>>
>>>>> Would just say maxlinewidth of SSPP instead of giving some 
>>>>> hardcoded number.
>>>>
>>>> Ack.
>>>>
>>>>>
>>>>>>>
>>>>>>>> possible to use multirect feature and split source to use the 
>>>>>>>> SSPP to
>>>>>>>> output two consecutive rectangles. This commit brings in this 
>>>>>>>> capability
>>>>>>>> to support wider screen resolutions.
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Baryshkov <[e-mail]>
>>>>>>>> ---
>>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |   6 ++
>>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 116 
>>>>>>>> +++++++++++++++++++---
>>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h |   4 +
>>>>>>>>      3 files changed, 114 insertions(+), 12 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c 
>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>>> index 0ca3bc38ff7e..867832a752b2 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>>> @@ -485,6 +485,12 @@ static void 
>>>>>>>> _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>>>>>>>>                                             fetch_active,
>>>>>>>>                                             &pstate->pipe);
>>>>>>>>
>>>>>>>> +             _dpu_crtc_blend_setup_pipe(crtc, plane,
>>>>>>>> +                                        mixer, cstate->num_mixers,
>>>>>>>> +                                        stage_cfg, 
>>>>>>>> pstate->stage, 1,
>>>>>>>> +                                        fetch_active,
>>>>>>>> +                                        &pstate->r_pipe);
>>>>>>>> +
>>>>>>>>                  /* blend config update */
>>>>>>>>                  for (lm_idx = 0; lm_idx < cstate->num_mixers; 
>>>>>>>> lm_idx++) {
>>>>>>>>                          _dpu_crtc_setup_blend_cfg(mixer + 
>>>>>>>> lm_idx, pstate, format);
>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c 
>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>>> index e2e85688ed3c..401ead64c6bd 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>>>>>>> @@ -365,6 +365,9 @@ static void _dpu_plane_set_qos_ctrl(struct 
>>>>>>>> drm_plane *plane,
>>>>>>>>          struct dpu_plane *pdpu = to_dpu_plane(plane);
>>>>>>>>          struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
>>>>>>>>
>>>>>>>> +     if (!pipe->sspp)
>>>>>>>> +             return;
>>>>>>>> +
>>>>>>>>          memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
>>>>>>>>
>>>>>>>>          if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>>>>>>>> @@ -647,6 +650,9 @@ static int _dpu_plane_color_fill_pipe(struct 
>>>>>>>> dpu_plane_state *pstate,
>>>>>>>>      {
>>>>>>>>          struct dpu_hw_sspp_cfg pipe_cfg;
>>>>>>>>
>>>>>>>> +     if (!pipe->sspp)
>>>>>>>> +             return 0;
>>>>>>>
>>>>>>> instead of checking if sspp was present, is it not better for the 
>>>>>>> caller
>>>>>>> to check if the rpipe is valid before calling this?
>>>>>>>
>>>>>>>> +
>>>>>>>>          /* update sspp */
>>>>>>>>          if (!pipe->sspp->ops.setup_solidfill)
>>>>>>>>                  return 0;
>>>>>>>> @@ -701,6 +707,8 @@ static void _dpu_plane_color_fill(struct 
>>>>>>>> dpu_plane *pdpu,
>>>>>>>>
>>>>>>>>          /* update sspp */
>>>>>>>>          _dpu_plane_color_fill_pipe(pstate, &pstate->pipe, 
>>>>>>>> &pstate->pipe_cfg, fill_color, fmt);
>>>>>>>> +
>>>>>>>> +     _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, 
>>>>>>>> &pstate->r_pipe_cfg, fill_color, fmt);
>>>>>>>>      }
>>>>>>>
>>>>>>> So cant we do
>>>>>>>
>>>>>>> if (pstate->r_pipe.sspp)
>>>>>>>            _dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe,
>>>>>>>                    &pstate->r_pipe_cfg, fill_color, fmt);
>>>>>>>
>>>>>>> It just seems better to me as the caller would already know if 
>>>>>>> the sspp
>>>>>>> was assigned.
>>>>>>
>>>>>>     I think I had this kind of code earlier, but then I found it more
>>>>>> logical to move the check to the called function. I'll move it back.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>      int dpu_plane_validate_multirect_v2(struct 
>>>>>>>> dpu_multirect_plane_states *plane)
>>>>>>>> @@ -911,6 +919,9 @@ static int 
>>>>>>>> dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
>>>>>>>>      {
>>>>>>>>          uint32_t min_src_size;
>>>>>>>>
>>>>>>>> +     if (!pipe->sspp)
>>>>>>>> +             return 0;
>>>>>>>> +
>>>>>>>>          min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
>>>>>>>>
>>>>>>>>          if (DPU_FORMAT_IS_YUV(fmt) &&
>>>>>>>> @@ -957,9 +968,12 @@ 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;
>>>>>>>> +     struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
>>>>>>>>          const struct drm_crtc_state *crtc_state = NULL;
>>>>>>>>          const struct dpu_format *fmt;
>>>>>>>>          struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
>>>>>>>> +     struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
>>>>>>>>          struct drm_rect fb_rect = { 0 };
>>>>>>>>          uint32_t max_linewidth;
>>>>>>>>          unsigned int rotation;
>>>>>>>> @@ -983,8 +997,11 @@ static int dpu_plane_atomic_check(struct 
>>>>>>>> drm_plane *plane,
>>>>>>>>          if (!new_plane_state->visible)
>>>>>>>>                  return 0;
>>>>>>>>
>>>>>>>> -     pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
>>>>>>>> -     pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>>>>> +     pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>>>>>>> +     pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>>>>> +     r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
>>>>>>>> +     r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>>>>>>> +     r_pipe->sspp = NULL;
>>>>>>>>
>>>>>>>>          pstate->stage = DPU_STAGE_0 + 
>>>>>>>> pstate->base.normalized_zpos;
>>>>>>>>          if (pstate->stage >= 
>>>>>>>> pdpu->catalog->caps->max_mixer_blendstages) {
>>>>>>>> @@ -1016,16 +1033,53 @@ static int dpu_plane_atomic_check(struct 
>>>>>>>> drm_plane *plane,
>>>>>>>>
>>>>>>>>          max_linewidth = pdpu->catalog->caps->max_linewidth;
>>>>>>>>
>>>>>>>> -     /* check decimated source width */
>>>>>>>>          if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
>>>>>>>> -             DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT 
>>>>>>>> " line:%u\n",
>>>>>>>> -                             DRM_RECT_ARG(&pipe_cfg->src_rect), 
>>>>>>>> max_linewidth);
>>>>>>>> -             return -E2BIG;
>>>>>>>> +             /* struct dpu_crtc_state *cstate = 
>>>>>>>> to_dpu_crtc_state(crtc_state); */
>>>>>>>> +
>>>>>>>> +             if (drm_rect_width(&pipe_cfg->src_rect) > 2 * 
>>>>>>>> max_linewidth) {
>>>>>>>> +                     DPU_DEBUG_PLANE(pdpu, "invalid src " 
>>>>>>>> DRM_RECT_FMT " line:%u\n",
>>>>>>>> +                                     
>>>>>>>> DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
>>>>>>>> +                     return -E2BIG;
>>>>>>>> +             }
>>>>>>>
>>>>>>> This is where I am a bit concerned enabling it for all chipsets 
>>>>>>> in one go.
>>>>>>
>>>>>> As I wrote earlier, I'd prefer the opt-out rather than opt-in 
>>>>>> here. It
>>>>>> is much easier to handle the reports "I have a device with sm6543,
>>>>>> where the display worked before 6.4, but started failing afterwards"
>>>>>> rather than trying to find a person with sm6543 and asking him if he
>>>>>> can enable this and that on his device. And even a lower chance of a
>>>>>> person with sm6543 coming up with a patch 'hey, I enabled this for my
>>>>>> phone and it works!'.
>>>>>>
>>>>>> If we find any issues during or close to the end of the development
>>>>>> cycle, we can add a 'don't enable wide plane here' switch and enable
>>>>>> it for failing platforms. But each enablement of this switch should
>>>>>> come with a reason (wide planes not working here because ....). In 
>>>>>> the
>>>>>> end this switch should be gone and transformed into proper HW
>>>>>> limitation checks.
>>>>>>
>>>>>
>>>>> As it has become clear that with this patch series 4K with UBWC cannot
>>>>> be supported without true virtual planes (with two SSPPs), why do you
>>>>> need to relax this check right now?
>>>>
>>>> Yes. It enables support for 4k @ linear formats. So my plan for this
>>>> series is to land 4k with all the proper applicable restrictions.
>>>>
>>>>> You can relax this when you add the support for virtual planes till 
>>>>> then
>>>>> let it be this way.
>>>>>
>>>>> Its not going to break smartDMA as such. You can still use it for 
>>>>> layers
>>>>> < 2560.
>>>>>
>>>>> That way we stay true to the purpose of the feature. I think 
>>>>> originally
>>>>> you wanted to get this in for smartDMA and not to support wide 
>>>>> plane and
>>>>> that purpose will still be achieved even with keeping this check 
>>>>> intact.
>>>>
>>>> Actually, no. With this series I wanted to get 4k. It was developed in
>>>> parallel with the 4k enablement for RB3 (posted, bridge patches are
>>>> being merged for 6.3) and RB5 (delayed for now, I have other issues
>>>> there).
>>>>
>>>
>>> With the UBWC related checks, this wont support 4K for UBWC layers which
>>> is default on QC chipsets. So I am fine with respect to that. But still
>>> this does not address the product spec advertized modes. Like I
>>> mentioned before, relaxing the maxlinewidth check with the added UBWC
>>> checks is fine from DPU point of view but not from the product POV.
>>>
>>> As things stand today, this is the only check failing the 4K modes on
>>> chipsets which shouldnt support 4k (linear or UBWC doesnt matter).
>>>
>>>>> You can relax it in the virtual plane series.
>>>>>
>>>>> Regarding issues, this is where it gets tricky. We should be aligning
>>>>> with what the product supports. QC will not support issues arising 
>>>>> with
>>>>> 4K on chipsets on which 4K is not advertized.
>>>>
>>>> So, we have several different items here:
>>>> - SmartDMA v2 per se, supporting two rectangles per VIG or DMA plane,
>>>> - Source split support,
>>>> - Supporting 4k modes.
>>>>
>>>> I think we should tend them one by one. This series concerns SmartDMA
>>>> v2. Using SmartDMA it is possible to use two rectangles side by side
>>>> to emulate a wide plane. This series doesn't care at all about max
>>>> resolutions. These two items are completely orthogonal.
>>>>
>>>
>>> No its not orthogonal. Relaxing the maxlinewidth check in the
>>> atomic_check() will allow 4K layers now even on chipsets where 4K wasnt
>>> advertized. Linear or UBWC doesnt matter as the spec doesnt go into 
>>> that.
>>
>> Please correct my answers, if I got something wrong here:
>>
>> Does sc7180 support SmartDMA? Yes it does.
>> Can QC or CrOS validate SmartDMA separately on sc7180? I hope you can.
>> Should the hw-supported feature be enabled? Yes, it should.
>>
>> Now limiting out 4k by not supporting SmartDMA looks like a misfeature.
>> I can only suggest sending a change to block 4k on sc7180.
>>
> 
> Agreed. We should first block 4K on sc7180 and other devices which dont 
> advertize 4k. No, it was never a question of limiting out 4K by not 
> supporting smartDMA but the other way around. By supporting smartDMA, we 
> would have had to support 4K on some chipsets due to this change which I 
> didnt want to do.
> 
> And yes agreed that we can stop discussing 4K anymore on this patch but 
> not smartDMA itself. Please see below.
> 
>>>>>>> As you are aware,  we have an open bug today that we do not 
>>>>>>> filter out
>>>>>>> the modes which we do not support.
>>>>>>>
>>>>>>> https://gitlab.freedesktop.org/drm/msm/-/issues/21
>>>>>>
>>>>>> I thought that with the link-frequencies in place and with the DSI
>>>>>> checking the OPP tables this issue is mostly handled. Isn't it?
>>>>>> Is a mode check in the DPU driver itself the last missing piece?
>>>>>>
>>>>>
>>>>> opp based checking was implemented only for DSI. That one is byte 
>>>>> clk based.
>>>>>
>>>>> DP uses link rate for opp table.
>>>>>
>>>>> Even with a 5.4G link rate (the one in sc7180 chromebook) 4k@30 would
>>>>> still be possible but it was not advertized
>>>>>
>>>>> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
>>>>>
>>>>> These docs are available in public domain.
>>>>>
>>>>> As we synced up last time on
>>>>> https://patchwork.freedesktop.org/series/107917/, even with these 
>>>>> limits
>>>>> in place, its not matching the advertized limits.
>>>>>
>>>>>>>
>>>>>>> Due to this, on all chipsets we will end up trying to do a 4K on
>>>>>>> external display which we dont know what bugs it will expose.
>>>>>>
>>>>>> If we do not expose bugs, we do not have a way to fix them. And I
>>>>>> definitely think that all the bugs should be listed as early as
>>>>>> possible, while both of us still remember the code under the 
>>>>>> question.
>>>>>>
>>>>>
>>>>> Yes but on chipsets where 4K is supported ( and hence needed ).
>>>>
>>>> 4k, SmartDMA, src-split, split-display, etc.
>>>>
>>>
>>> The visual issues reported on sdm845 on the other thread are a classic
>>> example of what I just wrote on that patchset and thats why I was
>>> emphasizing a visual validation OR in other words enable the feature on
>>> which you are able to visually validate it.
>>
>> Yes. And if we did not enable the feature, Amit would not be able to
>> spot that. I can repeat my suggestion:
>>
>> - prevalidate these features for the accessible platforms (e.g. I only
>> have sdm845, sm8250 and sm8350 at hand)
> 
> How? Your validation is not with a compositor in most cases and just 
> with modetest.

Yes, I mostly validated it using the modetest up to now. It was enough 
to spot most of the issues during development.

My plan is to validate it with wayland on C630 (sdm850, DP), X11 on RB3 
(sdm845, DSI) and, once I get back to the office, X11 on RB5 (sm8250, 
DSI). I did not have particular plans for sm8350, but it might also be 
worth giving it a try.

>> - enable SmartDMA for all chipsets where SmartDMA is supported
>> - collect possible tested-by and broken-at reports, while the patches
>> sit in linux-next
> 
> Question:
> 
> 1) Who will test these on all the devices for us? This is what I had 
> written even before that someone should give Tested-by tags then.
> 
> Even if you have them on linux-next, someone has to test them voluntarily.

My plan is to depend on the contributors of the corresponding platforms. 
In other words, Bjorn (for sc8180x/sc8280xp), Neal (sm8450/sm8550) and 
hopefully Marijn or Adam (for sm6115 and maybe other platforms)

> 2) Who will look at these broken-at reports and debug them?

The author of the patchset, me. With the hope that you can help in 
obscure cases.

> 
>> - disable SmartDMA basing on the feedback from the previous step (e.g.
>> select from 'mostly disable', 'disable for the bugged cases', 'do not
>> disable at all', etc).
>>    I can promise that if we see a significant validation failure rate I
>> will not oppose disabling SmartDMA.
>>
> 
> Thanks for agreeing on this :)
> 
>> As a reminder: if the patchset is ready at the time of 6.3-rc1 (in
>> three weeks from now), it is going to be merged into linux-next first,
>> after that it can go into the main Linus'es tree at 6.4-rc1. So we
>> will have _two_ kernel cycles to collect bug reports and to disable
>> (or fix) broken cases.
>>
> 
>>> We can evaluate and enable smartDMA on other chipsets on a need basis.
>>>
>>> We discussed this again even today in the team discussion. Our team's
>>> PoV doesnt change. We would still like to enable smartDMA only on
>>> chipsets which can be visually validated first to limit the debugging
>>> effort to one chipset first and then perfect it. Otherwise its too much
>>> effort on QC side to debug those issues on all chipsets.
>>
>> I think QC mostly debugs issues on sc7180/sc7280 and sometimes on
>> sdm845/sm8250 (and now on sm8350). I think we can let people
>> (somainline, PmOS) test features on other platforms.
>> And testing happens better if we can say 'please test linux-next'
>> rather than 'please test linux-next + this patch to enable the feature
>> + that patch to enable the second patch of the feature'.
>>
> 
> In this particular case, in the current timeframe QC can only commit to 
> validate sc7280 at this point and not sc7180, sdm845, sm8250 and sm8350 
> for this feature.

This is both good and bad. Not being able to test sc7180 sounds 
particularly bad. I still didn't have time to work on my sc7180 laptop. 
It still has WoA.

> 
> So it will be upto you and others for all other targets.
> 
> Yes, I agree, people prefer to test on a branch rather than branch + 
> patches.

Good.

> 
>>>>>>> So lets say if we test it on sc7280 fully but not on sc7180, we will
>>>>>>> still hit this condition on sc7180 too but on that chipset we did 
>>>>>>> not
>>>>>>> advertise 4K as a capability in the product spec.
>>>>>>
>>>>>> Is it 'not advertised' or 'not supported by hw'?
>>>>>>
>>>>>
>>>>> The document
>>>>> https://www.qualcomm.com/content/dam/qcomm-martech/dm-assets/documents/prod_brief_qcom_sd7c.pdf
>>>>> is made from inputs from not just display team but overall system
>>>>> limits. So even though you could argue that this falls within the
>>>>> display capabilities, all I can say at the moment is we have to 
>>>>> stick to
>>>>> the advertized limits as its compiled with inputs from all the teams
>>>>> (system/performance etc).
>>>>
>>>> So, there should be a limiting factor (or a combination of them).
>>>> Filter out 4k modes on sc7180. Or modes using fill rate higher than N.
>>>> Pixel clock rate higher than M. But it has nothing to do with these
>>>> patches enabling SmartDMA support on this platform.
>>>>
>>>> Even if we look at the vendor kernels, we don't see 'maximum external
>>>> resolution'. Instead I see a combination of linewidth and bandwidth
>>>> limitations. If we can stick to that, that would be great.
>>>>
>>>
>>> Can you please point me to bandwidth limitation checks? How are other
>>> vendors coming up with this number? It has to be based on some
>>> resolution too right?
>>
>> Usually it is considered in the other direction. The SoC can support
>> this-and-that pixel clock and bandwidth, so put the NxM resolution to
>> the datasheet as the max supported one.
>>
> Our data sheet also gave the resolution so I chose the pixel clk last time.
> 
>> Can you point out how the vendor kernel limits DP modes? I checked out
>> several dtsi files. For sm8250 the DP is limited to 1920x1080 (while
>> PB explicitly mentions 4k@60).
> 
> Where do you see 1920x1080? In kona-sde.dtsi I see 675000

I have been looking at the DTSI files for the RB5 board. See post-CS8 
and post-CS9 releases.

> 
>> sm7125 is limited to 2560x1600. sm6150 again 1920x1080. From the pile
>> of the DTS that I have here the rest lists only
>> qcom,max-pclk-frequency-khz
> 
> Yes, qcom,max-pclk-frequency-khz is the method they use.
> 
>>
>>> My RFC https://patchwork.freedesktop.org/series/107917/ considered pixel
>>> clk as the limiting factor which was posted after discussions
>>> internally. In the absence of another way, that remains the only
>>> solution to tackle this.
>>
>> If I remember correctly the mentioned patchset used manually crafted
>> pixel clocks. And for example for sm8250 this clock doesn't correspond
>> to verifiable source.
>> Your patchset used 594000 KHz as max ext pclk for sm8250. vendor dtsi
>> lists 187500 KHz as maximum DP pclk for RB5 and 150000 KHz in all
> 
> Lets discuss this more when I re-post the patch to limit 4K modes.
> 
>> other cases. And, at the same time, it lists a 3840x2160@60 mode for
>> the DSI/lt9611uxc with the pixel clock as high as 608040 KHz.
>>
>> I suggest we stop the discussion at this point, unless there is
>> anything else wrong with this patchset itself.
>>
> 
> Agreed but please look at my comments about smartDMA validation above 
> and let me know.
> 
>> I noted the point regarding UBWC & parallel mode. I will handle it in
>> the next iteration.
>>
>> I noted your valid point about the visual verification. I proposed a
>> way to ease validation, allowing it to be enabled for testers and
>> early adopters for nearly two cycles. I hope you'd agree to this plan.
>> I on the other side agree to revert to opt-in if the failure rate is
>> high.
>>
> 
> Agreed but with above concerns.

Thanks!

> 
>> Please stop bringing max resolution issues to this patchserie. It must
> 
> It got dragged into this patch because this patch relaxed the checks for 
> max resolution.
> 
>> be handled separately. I hope to see the mode filter patch targeting
>> sc7180 & sc7280. With the DSI opp check in place I think we should
>> concentrate on the DP case. If nothing else, I think even adding the
>> max PCLK to the msm_dp_desc should be sufficient to your worries. I'd
>> prefer to be able to override it for the particular board, but I think
>> this can come later, as it would require an agreement from the DT
>> schema team.
>>
> 
> Alright, I can rebase that RFC to cover the DP cases for now and we can 
> discuss all the max resolution discussions there.

Thanks!

> 
>> -- 
>> With best wishes
>> Dmitry

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 0ca3bc38ff7e..867832a752b2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -485,6 +485,12 @@  static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
 					   fetch_active,
 					   &pstate->pipe);
 
+		_dpu_crtc_blend_setup_pipe(crtc, plane,
+					   mixer, cstate->num_mixers,
+					   stage_cfg, pstate->stage, 1,
+					   fetch_active,
+					   &pstate->r_pipe);
+
 		/* blend config update */
 		for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
 			_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate, format);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index e2e85688ed3c..401ead64c6bd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -365,6 +365,9 @@  static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
 	struct dpu_plane *pdpu = to_dpu_plane(plane);
 	struct dpu_hw_pipe_qos_cfg pipe_qos_cfg;
 
+	if (!pipe->sspp)
+		return;
+
 	memset(&pipe_qos_cfg, 0, sizeof(pipe_qos_cfg));
 
 	if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
@@ -647,6 +650,9 @@  static int _dpu_plane_color_fill_pipe(struct dpu_plane_state *pstate,
 {
 	struct dpu_hw_sspp_cfg pipe_cfg;
 
+	if (!pipe->sspp)
+		return 0;
+
 	/* update sspp */
 	if (!pipe->sspp->ops.setup_solidfill)
 		return 0;
@@ -701,6 +707,8 @@  static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
 
 	/* update sspp */
 	_dpu_plane_color_fill_pipe(pstate, &pstate->pipe, &pstate->pipe_cfg, fill_color, fmt);
+
+	_dpu_plane_color_fill_pipe(pstate, &pstate->r_pipe, &pstate->r_pipe_cfg, fill_color, fmt);
 }
 
 int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
@@ -911,6 +919,9 @@  static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu,
 {
 	uint32_t min_src_size;
 
+	if (!pipe->sspp)
+		return 0;
+
 	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
 
 	if (DPU_FORMAT_IS_YUV(fmt) &&
@@ -957,9 +968,12 @@  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;
+	struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
 	const struct drm_crtc_state *crtc_state = NULL;
 	const struct dpu_format *fmt;
 	struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
+	struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
 	struct drm_rect fb_rect = { 0 };
 	uint32_t max_linewidth;
 	unsigned int rotation;
@@ -983,8 +997,11 @@  static int dpu_plane_atomic_check(struct drm_plane *plane,
 	if (!new_plane_state->visible)
 		return 0;
 
-	pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
-	pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
+	pipe->multirect_index = DPU_SSPP_RECT_SOLO;
+	pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
+	r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
+	r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
+	r_pipe->sspp = NULL;
 
 	pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
 	if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
@@ -1016,16 +1033,53 @@  static int dpu_plane_atomic_check(struct drm_plane *plane,
 
 	max_linewidth = pdpu->catalog->caps->max_linewidth;
 
-	/* check decimated source width */
 	if (drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) {
-		DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
-				DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
-		return -E2BIG;
+		/* struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); */
+
+		if (drm_rect_width(&pipe_cfg->src_rect) > 2 * max_linewidth) {
+			DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
+					DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
+			return -E2BIG;
+		}
+
+		/*
+		 * FIXME: it's not possible to check if sourcesplit is supported,
+		 * LMs is not assigned yet. It happens in dpu_encoder_virt_mode_set
+		 */
+		if (drm_rect_width(&pipe_cfg->src_rect) != drm_rect_width(&pipe_cfg->dst_rect) ||
+			   drm_rect_height(&pipe_cfg->src_rect) != drm_rect_height(&pipe_cfg->dst_rect) ||
+			   (!test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) &&
+			    !test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features)) ||
+			   /* cstate->num_mixers < 2 ||
+			   !test_bit(DPU_MIXER_SOURCESPLIT, &cstate->mixers[0].hw_lm->cap->features) || */
+			   DPU_FORMAT_IS_YUV(fmt)) {
+			DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, can't use split source\n",
+					DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth);
+			return -E2BIG;
+		}
+
+		/* Use multirect for wide plane. We do not support dynamic assignment of SSPPs, so we know the configuration. */
+		pipe->multirect_index = DPU_SSPP_RECT_0;
+		pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
+
+		r_pipe->sspp = pipe->sspp;
+		r_pipe->multirect_index = DPU_SSPP_RECT_1;
+		r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL;
+
+		*r_pipe_cfg = *pipe_cfg;
+		pipe_cfg->src_rect.x2 = (pipe_cfg->src_rect.x1 + pipe_cfg->src_rect.x2) >> 1;
+		pipe_cfg->dst_rect.x2 = (pipe_cfg->dst_rect.x1 + pipe_cfg->dst_rect.x2) >> 1;
+		r_pipe_cfg->src_rect.x1 = pipe_cfg->src_rect.x2;
+		r_pipe_cfg->dst_rect.x1 = pipe_cfg->dst_rect.x2;
 	}
 
 	fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb));
 
-	ret = dpu_plane_atomic_check_pipe(pdpu, &pstate->pipe, pipe_cfg, fmt);
+	ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt);
+	if (ret)
+		return ret;
+
+	ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt);
 	if (ret)
 		return ret;
 
@@ -1094,8 +1148,10 @@  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
+	else {
 		dpu_plane_flush_csc(pdpu, &pstate->pipe);
+		dpu_plane_flush_csc(pdpu, &pstate->r_pipe);
+	}
 
 	/* flag h/w flush complete */
 	if (plane->state)
@@ -1130,6 +1186,9 @@  static void dpu_plane_sspp_update_pipe(struct drm_plane *plane,
 	struct drm_plane_state *state = plane->state;
 	struct dpu_plane_state *pstate = to_dpu_plane_state(state);
 
+	if (!pipe->sspp)
+		return;
+
 	if (layout && pipe->sspp->ops.setup_sourceaddress) {
 		trace_dpu_plane_set_scanout(pipe, layout);
 		pipe->sspp->ops.setup_sourceaddress(pipe, layout);
@@ -1207,13 +1266,14 @@  static void dpu_plane_sspp_atomic_update(struct drm_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 dpu_sw_pipe *r_pipe = &pstate->r_pipe;
 	struct drm_crtc *crtc = state->crtc;
 	struct drm_framebuffer *fb = state->fb;
 	bool is_rt_pipe;
 	const struct dpu_format *fmt =
 		to_dpu_format(msm_framebuffer_format(fb));
 	struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
-
+	struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
 	struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
 	struct msm_gem_address_space *aspace = kms->base.aspace;
 	struct dpu_hw_fmt_layout layout;
@@ -1241,12 +1301,22 @@  static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
 				   drm_mode_vrefresh(&crtc->mode),
 				   layout_valid ? &layout: NULL);
 
+	dpu_plane_sspp_update_pipe(plane, r_pipe, r_pipe_cfg, fmt,
+				   drm_mode_vrefresh(&crtc->mode),
+				   layout_valid ? &layout: NULL);
+
 	if (pstate->needs_qos_remap)
 		pstate->needs_qos_remap = false;
 
 	pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, pipe_cfg);
 
 	pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, pipe_cfg);
+
+	if (r_pipe->sspp) {
+		pstate->plane_fetch_bw += _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, r_pipe_cfg);
+
+		pstate->plane_clk = max(pstate->plane_clk, _dpu_plane_calc_clk(&crtc->mode, r_pipe_cfg));
+	}
 }
 
 static void _dpu_plane_atomic_disable(struct drm_plane *plane)
@@ -1289,6 +1359,8 @@  static void dpu_plane_destroy(struct drm_plane *plane)
 		pstate = to_dpu_plane_state(plane->state);
 		_dpu_plane_set_qos_ctrl(plane, &pstate->pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
 
+		_dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, false, DPU_PLANE_QOS_PANIC_CTRL);
+
 		mutex_destroy(&pdpu->lock);
 
 		/* this will destroy the states as well */
@@ -1369,11 +1441,26 @@  static void dpu_plane_atomic_print_state(struct drm_printer *p,
 		const struct drm_plane_state *state)
 {
 	const struct dpu_plane_state *pstate = to_dpu_plane_state(state);
+	const struct dpu_sw_pipe *pipe = &pstate->pipe;
+	const struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg;
+	const struct dpu_sw_pipe *r_pipe = &pstate->r_pipe;
+	const struct dpu_hw_sspp_cfg *r_pipe_cfg = &pstate->r_pipe_cfg;
 
 	drm_printf(p, "\tstage=%d\n", pstate->stage);
-	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));
+
+	drm_printf(p, "\tsspp[0]=%s\n", pipe->sspp->cap->name);
+	drm_printf(p, "\tmultirect_mode[0]=%s\n", dpu_get_multirect_mode(pipe->multirect_mode));
+	drm_printf(p, "\tmultirect_index[0]=%s\n", dpu_get_multirect_index(pipe->multirect_index));
+	drm_printf(p, "\tsrc[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect));
+	drm_printf(p, "\tdst[0]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->dst_rect));
+
+	if (r_pipe->sspp) {
+		drm_printf(p, "\tsspp[1]=%s\n", r_pipe->sspp->cap->name);
+		drm_printf(p, "\tmultirect_mode[1]=%s\n", dpu_get_multirect_mode(r_pipe->multirect_mode));
+		drm_printf(p, "\tmultirect_index[1]=%s\n", dpu_get_multirect_index(r_pipe->multirect_index));
+		drm_printf(p, "\tsrc[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->src_rect));
+		drm_printf(p, "\tdst[1]=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&r_pipe_cfg->dst_rect));
+	}
 }
 
 static void dpu_plane_reset(struct drm_plane *plane)
@@ -1407,6 +1494,10 @@  static void dpu_plane_reset(struct drm_plane *plane)
 	 * This is the place where the state is allocated, so fill it fully.
 	 */
 	pstate->pipe.sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe);
+	pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
+	pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
+
+	pstate->r_pipe.sspp = NULL;
 
 	__drm_atomic_helper_plane_reset(plane, &pstate->base);
 }
@@ -1423,6 +1514,7 @@  void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
 
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 	_dpu_plane_set_qos_ctrl(plane, &pstate->pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
+	_dpu_plane_set_qos_ctrl(plane, &pstate->r_pipe, enable, DPU_PLANE_QOS_PANIC_CTRL);
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
 #endif
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
index 079dad83eb37..183c95949885 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h
@@ -19,7 +19,9 @@ 
  * @base:	base drm plane state object
  * @aspace:	pointer to address space for input/output buffers
  * @pipe:	software pipe description
+ * @r_pipe:	software pipe description of the second pipe
  * @pipe_cfg:	software pipe configuration
+ * @r_pipe_cfg:	software pipe configuration for the second pipe
  * @stage:	assigned by crtc blender
  * @needs_qos_remap: qos remap settings need to be updated
  * @multirect_index: index of the rectangle of SSPP
@@ -34,7 +36,9 @@  struct dpu_plane_state {
 	struct drm_plane_state base;
 	struct msm_gem_address_space *aspace;
 	struct dpu_sw_pipe pipe;
+	struct dpu_sw_pipe r_pipe;
 	struct dpu_hw_sspp_cfg pipe_cfg;
+	struct dpu_hw_sspp_cfg r_pipe_cfg;
 	enum dpu_stage stage;
 	bool needs_qos_remap;
 	bool pending;