On Thu, 9 Feb 2023 at 04:19, Abhinav Kumar <quic_abhin...@quicinc.com> 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 <dmitry.barysh...@linaro.org>
> > ---
> > 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;
--
With best wishes
Dmitry
