| Message ID | 20240314000216.392549-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dpu: support virtual wide planes | expand |
On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > Take into account the plane rotation and flipping when calculating src > positions for the wide plane parts. > > This is not an issue yet, because rotation is only supported for the > UBWC planes and wide UBWC planes are rejected anyway because in parallel > multirect case only the half of the usual width is supported for tiled > formats. However it's better to fix this now rather than stumbling upon > it later. > > Fixes: 80e8ae3b38ab ("drm/msm/dpu: add support for wide planes") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > Split dpu_plane_atomic_check() function into two pieces: > > dpu_plane_atomic_check_nopipe() performing generic checks on the pstate, > without touching the associated pipe, > > and > > dpu_plane_atomic_check_pipes(), which takes into account used pipes. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 184 ++++++++++++++-------- > 1 file changed, 117 insertions(+), 67 deletions(-) > One thing which seemed odd to me is even dpu_plane_atomic_check_nopipe() does use pipe_cfg even though its named "nopipe". Perhaps were you targetting a split of SW planes vs SSPP atomic_check? I tried applying this patch on top of msm-next to more closely review the split up but it does not apply. So, I will review this patch a little better after it is re-spun. But will proceed with remaining patches. > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index 6360052523b5..187ac2767a2b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -788,50 +788,22 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, > #define MAX_UPSCALE_RATIO 20 > #define MAX_DOWNSCALE_RATIO 4 > > -static int dpu_plane_atomic_check(struct drm_plane *plane, > - struct drm_atomic_state *state) > +static int dpu_plane_atomic_check_nopipe(struct drm_plane *plane, > + struct drm_plane_state *new_plane_state, > + const struct drm_crtc_state *crtc_state) > { > - struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > - plane); > int ret = 0, min_scale, max_scale; > struct dpu_plane *pdpu = to_dpu_plane(plane); > struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base); > u64 max_mdp_clk_rate = kms->perf.max_core_clk_rate; > struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); > - struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > - 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_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > struct drm_rect fb_rect = { 0 }; > uint32_t max_linewidth; > - unsigned int rotation; > - uint32_t supported_rotations; > - const struct dpu_sspp_cfg *pipe_hw_caps; > - const struct dpu_sspp_sub_blks *sblk; > > - if (new_plane_state->crtc) > - crtc_state = drm_atomic_get_new_crtc_state(state, > - new_plane_state->crtc); > - > - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > - r_pipe->sspp = NULL; > - > - if (!pipe->sspp) > - return -EINVAL; > - > - pipe_hw_caps = pipe->sspp->cap; > - sblk = pipe->sspp->cap->sblk; > - > - if (sblk->scaler_blk.len) { > - min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); > - max_scale = MAX_DOWNSCALE_RATIO << 16; > - } else { > - min_scale = 1 << 16; > - max_scale = 1 << 16; > - } > + min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); > + max_scale = MAX_DOWNSCALE_RATIO << 16; > > ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, > min_scale, > @@ -844,11 +816,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > if (!new_plane_state->visible) > return 0; > > - 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; > - > pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > DPU_ERROR("> %d plane stages assigned\n", > @@ -872,8 +839,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > return -E2BIG; > } > > - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > - > max_linewidth = pdpu->catalog->caps->max_linewidth; > > drm_rect_rotate(&pipe_cfg->src_rect, > @@ -882,6 +847,83 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > if ((drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) || > _dpu_plane_calc_clk(&crtc_state->adjusted_mode, pipe_cfg) > max_mdp_clk_rate) { > + 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; > + } > + > + *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; > + } else { > + memset(r_pipe_cfg, 0, sizeof(*r_pipe_cfg)); > + } > + > + drm_rect_rotate_inv(&pipe_cfg->src_rect, > + new_plane_state->fb->width, new_plane_state->fb->height, > + new_plane_state->rotation); > + if (r_pipe_cfg->src_rect.x1 != 0) > + drm_rect_rotate_inv(&r_pipe_cfg->src_rect, > + new_plane_state->fb->width, new_plane_state->fb->height, > + new_plane_state->rotation); > + > + pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); > + > + return 0; > +} > + > +static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, > + struct drm_atomic_state *state, > + const struct drm_crtc_state *crtc_state) > +{ > + struct drm_plane_state *new_plane_state = > + drm_atomic_get_new_plane_state(state, plane); > + 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 dpu_format *fmt; > + struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > + struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > + uint32_t max_linewidth; > + unsigned int rotation; > + uint32_t supported_rotations; > + const struct dpu_sspp_cfg *pipe_hw_caps; > + const struct dpu_sspp_sub_blks *sblk; > + int ret = 0; > + > + pipe_hw_caps = pipe->sspp->cap; > + sblk = pipe->sspp->cap->sblk; > + > + /* > + * We already have verified scaling against platform limitations. > + * Now check if the SSPP supports scaling at all. > + */ > + if (!sblk->scaler_blk.len && > + ((drm_rect_width(&new_plane_state->src) >> 16 != > + drm_rect_width(&new_plane_state->dst)) || > + (drm_rect_height(&new_plane_state->src) >> 16 != > + drm_rect_height(&new_plane_state->dst)))) > + return -ERANGE; > + > + 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; > + > + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > + > + max_linewidth = pdpu->catalog->caps->max_linewidth; > + > + ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, > + &crtc_state->adjusted_mode); > + if (ret) > + return ret; > + > + if (r_pipe_cfg->src_rect.x1 != 0) { > /* > * In parallel multirect case only the half of the usual width > * is supported for tiled formats. If we are here, we know that > @@ -895,12 +937,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > return -E2BIG; > } > > - 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; > - } > - > 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) && > @@ -922,26 +958,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > 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; > - } > - > - drm_rect_rotate_inv(&pipe_cfg->src_rect, > - new_plane_state->fb->width, new_plane_state->fb->height, > - new_plane_state->rotation); > - if (r_pipe->sspp) > - drm_rect_rotate_inv(&r_pipe_cfg->src_rect, > - new_plane_state->fb->width, new_plane_state->fb->height, > - new_plane_state->rotation); > - > - ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, &crtc_state->adjusted_mode); > - if (ret) > - return ret; > - > - if (r_pipe->sspp) { > ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt, > &crtc_state->adjusted_mode); > if (ret) > @@ -964,11 +980,45 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > } > > pstate->rotation = rotation; > - pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); > > return 0; > } > > +static int dpu_plane_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > + plane); > + int ret = 0; > + struct dpu_plane *pdpu = to_dpu_plane(plane); > + struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); > + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > + struct dpu_sw_pipe *pipe = &pstate->pipe; > + struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; > + const struct drm_crtc_state *crtc_state = NULL; > + > + if (new_plane_state->crtc) > + crtc_state = drm_atomic_get_new_crtc_state(state, > + new_plane_state->crtc); > + > + if (pdpu->pipe != SSPP_NONE) { This check was not present iirc, why did you have to add this? RM will return the same SSPP unless freed. So why this additional check? > + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > + r_pipe->sspp = NULL; > + } > + > + if (!pipe->sspp) > + return -EINVAL; > + > + ret = dpu_plane_atomic_check_nopipe(plane, new_plane_state, crtc_state); > + if (ret) > + return ret; > + > + if (!new_plane_state->visible) > + return 0; > + > + return dpu_plane_atomic_check_pipes(plane, state, crtc_state); > +} > + > static void dpu_plane_flush_csc(struct dpu_plane *pdpu, struct dpu_sw_pipe *pipe) > { > const struct dpu_format *format =
On Thu, 6 Jun 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > > Split dpu_plane_atomic_check() function into two pieces: > > > > dpu_plane_atomic_check_nopipe() performing generic checks on the pstate, > > without touching the associated pipe, > > > > and > > > > dpu_plane_atomic_check_pipes(), which takes into account used pipes. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 184 ++++++++++++++-------- > > 1 file changed, 117 insertions(+), 67 deletions(-) > > > > One thing which seemed odd to me is even dpu_plane_atomic_check_nopipe() > does use pipe_cfg even though its named "nopipe". > > Perhaps were you targetting a split of SW planes vs SSPP atomic_check? > > I tried applying this patch on top of msm-next to more closely review > the split up but it does not apply. So, I will review this patch a > little better after it is re-spun. But will proceed with remaining patches. > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > index 6360052523b5..187ac2767a2b 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > @@ -788,50 +788,22 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, > > #define MAX_UPSCALE_RATIO 20 > > #define MAX_DOWNSCALE_RATIO 4 > > > > -static int dpu_plane_atomic_check(struct drm_plane *plane, > > - struct drm_atomic_state *state) > > +static int dpu_plane_atomic_check_nopipe(struct drm_plane *plane, > > + struct drm_plane_state *new_plane_state, > > + const struct drm_crtc_state *crtc_state) > > { > > - struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > > - plane); > > int ret = 0, min_scale, max_scale; > > struct dpu_plane *pdpu = to_dpu_plane(plane); > > struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base); > > u64 max_mdp_clk_rate = kms->perf.max_core_clk_rate; > > struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); > > - struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > > - 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_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > > struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > > struct drm_rect fb_rect = { 0 }; > > uint32_t max_linewidth; > > - unsigned int rotation; > > - uint32_t supported_rotations; > > - const struct dpu_sspp_cfg *pipe_hw_caps; > > - const struct dpu_sspp_sub_blks *sblk; > > > > - if (new_plane_state->crtc) > > - crtc_state = drm_atomic_get_new_crtc_state(state, > > - new_plane_state->crtc); > > - > > - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > > - r_pipe->sspp = NULL; > > - > > - if (!pipe->sspp) > > - return -EINVAL; > > - > > - pipe_hw_caps = pipe->sspp->cap; > > - sblk = pipe->sspp->cap->sblk; > > - > > - if (sblk->scaler_blk.len) { > > - min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); > > - max_scale = MAX_DOWNSCALE_RATIO << 16; > > - } else { > > - min_scale = 1 << 16; > > - max_scale = 1 << 16; > > - } > > + min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); > > + max_scale = MAX_DOWNSCALE_RATIO << 16; > > > > ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, > > min_scale, > > @@ -844,11 +816,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > if (!new_plane_state->visible) > > return 0; > > > > - 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; > > - > > pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > > if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > > DPU_ERROR("> %d plane stages assigned\n", > > @@ -872,8 +839,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > return -E2BIG; > > } > > > > - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > > - > > max_linewidth = pdpu->catalog->caps->max_linewidth; > > > > drm_rect_rotate(&pipe_cfg->src_rect, > > @@ -882,6 +847,83 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > > > if ((drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) || > > _dpu_plane_calc_clk(&crtc_state->adjusted_mode, pipe_cfg) > max_mdp_clk_rate) { > > + 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; > > + } > > + > > + *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; > > + } else { > > + memset(r_pipe_cfg, 0, sizeof(*r_pipe_cfg)); > > + } > > + > > + drm_rect_rotate_inv(&pipe_cfg->src_rect, > > + new_plane_state->fb->width, new_plane_state->fb->height, > > + new_plane_state->rotation); > > + if (r_pipe_cfg->src_rect.x1 != 0) > > + drm_rect_rotate_inv(&r_pipe_cfg->src_rect, > > + new_plane_state->fb->width, new_plane_state->fb->height, > > + new_plane_state->rotation); > > + > > + pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); > > + > > + return 0; > > +} > > + > > +static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, > > + struct drm_atomic_state *state, > > + const struct drm_crtc_state *crtc_state) > > +{ > > + struct drm_plane_state *new_plane_state = > > + drm_atomic_get_new_plane_state(state, plane); > > + 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 dpu_format *fmt; > > + struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > > + struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > > + uint32_t max_linewidth; > > + unsigned int rotation; > > + uint32_t supported_rotations; > > + const struct dpu_sspp_cfg *pipe_hw_caps; > > + const struct dpu_sspp_sub_blks *sblk; > > + int ret = 0; > > + > > + pipe_hw_caps = pipe->sspp->cap; > > + sblk = pipe->sspp->cap->sblk; > > + > > + /* > > + * We already have verified scaling against platform limitations. > > + * Now check if the SSPP supports scaling at all. > > + */ > > + if (!sblk->scaler_blk.len && > > + ((drm_rect_width(&new_plane_state->src) >> 16 != > > + drm_rect_width(&new_plane_state->dst)) || > > + (drm_rect_height(&new_plane_state->src) >> 16 != > > + drm_rect_height(&new_plane_state->dst)))) > > + return -ERANGE; > > + > > + 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; > > + > > + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > > + > > + max_linewidth = pdpu->catalog->caps->max_linewidth; > > + > > + ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, > > + &crtc_state->adjusted_mode); > > + if (ret) > > + return ret; > > + > > + if (r_pipe_cfg->src_rect.x1 != 0) { > > /* > > * In parallel multirect case only the half of the usual width > > * is supported for tiled formats. If we are here, we know that > > @@ -895,12 +937,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > return -E2BIG; > > } > > > > - 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; > > - } > > - > > 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) && > > @@ -922,26 +958,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > 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; > > - } > > - > > - drm_rect_rotate_inv(&pipe_cfg->src_rect, > > - new_plane_state->fb->width, new_plane_state->fb->height, > > - new_plane_state->rotation); > > - if (r_pipe->sspp) > > - drm_rect_rotate_inv(&r_pipe_cfg->src_rect, > > - new_plane_state->fb->width, new_plane_state->fb->height, > > - new_plane_state->rotation); > > - > > - ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, &crtc_state->adjusted_mode); > > - if (ret) > > - return ret; > > - > > - if (r_pipe->sspp) { > > ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt, > > &crtc_state->adjusted_mode); > > if (ret) > > @@ -964,11 +980,45 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > } > > > > pstate->rotation = rotation; > > - pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); > > > > return 0; > > } > > > > +static int dpu_plane_atomic_check(struct drm_plane *plane, > > + struct drm_atomic_state *state) > > +{ > > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > > + plane); > > + int ret = 0; > > + struct dpu_plane *pdpu = to_dpu_plane(plane); > > + struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); > > + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > > + struct dpu_sw_pipe *pipe = &pstate->pipe; > > + struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; > > + const struct drm_crtc_state *crtc_state = NULL; > > + > > + if (new_plane_state->crtc) > > + crtc_state = drm_atomic_get_new_crtc_state(state, > > + new_plane_state->crtc); > > + > > + if (pdpu->pipe != SSPP_NONE) { > > This check was not present iirc, why did you have to add this? > RM will return the same SSPP unless freed. So why this additional check? If pdpu->pipe is not SSPP_NONE, then virtual planes are disabled and there is a fixed 1:1 relationship between planes and SSPP blocks. > > > + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > > + r_pipe->sspp = NULL; > > + } > > + > > + if (!pipe->sspp) > > + return -EINVAL; > > + > > + ret = dpu_plane_atomic_check_nopipe(plane, new_plane_state, crtc_state); > > + if (ret) > > + return ret; > > + > > + if (!new_plane_state->visible) > > + return 0; > > + > > + return dpu_plane_atomic_check_pipes(plane, state, crtc_state); > > +} > > + > > static void dpu_plane_flush_csc(struct dpu_plane *pdpu, struct dpu_sw_pipe *pipe) > > { > > const struct dpu_format *format =
On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > Move a call to dpu_plane_check_inline_rotation() to the > dpu_plane_atomic_check_pipe() function, so that the rot90 constraints > are checked for both pipes. Also move rotation field from struct > dpu_plane_state to struct dpu_sw_pipe_cfg. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 + > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 55 +++++++++++---------- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 - > 3 files changed, 31 insertions(+), 28 deletions(-) > From the first glance, no major comments, I will give my R-b on this once its re-spun on msm-next.
On 6/5/2024 4:32 PM, Dmitry Baryshkov wrote: > On Thu, 6 Jun 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: >>> Split dpu_plane_atomic_check() function into two pieces: >>> >>> dpu_plane_atomic_check_nopipe() performing generic checks on the pstate, >>> without touching the associated pipe, >>> >>> and >>> >>> dpu_plane_atomic_check_pipes(), which takes into account used pipes. >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 184 ++++++++++++++-------- >>> 1 file changed, 117 insertions(+), 67 deletions(-) >>> >> >> One thing which seemed odd to me is even dpu_plane_atomic_check_nopipe() >> does use pipe_cfg even though its named "nopipe". >> >> Perhaps were you targetting a split of SW planes vs SSPP atomic_check? >> >> I tried applying this patch on top of msm-next to more closely review >> the split up but it does not apply. So, I will review this patch a >> little better after it is re-spun. But will proceed with remaining patches. >> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> index 6360052523b5..187ac2767a2b 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> @@ -788,50 +788,22 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, >>> #define MAX_UPSCALE_RATIO 20 >>> #define MAX_DOWNSCALE_RATIO 4 >>> >>> -static int dpu_plane_atomic_check(struct drm_plane *plane, >>> - struct drm_atomic_state *state) >>> +static int dpu_plane_atomic_check_nopipe(struct drm_plane *plane, >>> + struct drm_plane_state *new_plane_state, >>> + const struct drm_crtc_state *crtc_state) >>> { >>> - struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, >>> - plane); >>> int ret = 0, min_scale, max_scale; >>> struct dpu_plane *pdpu = to_dpu_plane(plane); >>> struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base); >>> u64 max_mdp_clk_rate = kms->perf.max_core_clk_rate; >>> struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); >>> - struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); >>> - 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_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; >>> struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; >>> struct drm_rect fb_rect = { 0 }; >>> uint32_t max_linewidth; >>> - unsigned int rotation; >>> - uint32_t supported_rotations; >>> - const struct dpu_sspp_cfg *pipe_hw_caps; >>> - const struct dpu_sspp_sub_blks *sblk; >>> >>> - if (new_plane_state->crtc) >>> - crtc_state = drm_atomic_get_new_crtc_state(state, >>> - new_plane_state->crtc); >>> - >>> - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); >>> - r_pipe->sspp = NULL; >>> - >>> - if (!pipe->sspp) >>> - return -EINVAL; >>> - >>> - pipe_hw_caps = pipe->sspp->cap; >>> - sblk = pipe->sspp->cap->sblk; >>> - >>> - if (sblk->scaler_blk.len) { >>> - min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); >>> - max_scale = MAX_DOWNSCALE_RATIO << 16; >>> - } else { >>> - min_scale = 1 << 16; >>> - max_scale = 1 << 16; >>> - } >>> + min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); >>> + max_scale = MAX_DOWNSCALE_RATIO << 16; >>> >>> ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, >>> min_scale, >>> @@ -844,11 +816,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, >>> if (!new_plane_state->visible) >>> return 0; >>> >>> - 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; >>> - >>> pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; >>> if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { >>> DPU_ERROR("> %d plane stages assigned\n", >>> @@ -872,8 +839,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, >>> return -E2BIG; >>> } >>> >>> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>> - >>> max_linewidth = pdpu->catalog->caps->max_linewidth; >>> >>> drm_rect_rotate(&pipe_cfg->src_rect, >>> @@ -882,6 +847,83 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, >>> >>> if ((drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) || >>> _dpu_plane_calc_clk(&crtc_state->adjusted_mode, pipe_cfg) > max_mdp_clk_rate) { >>> + 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; >>> + } >>> + >>> + *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; >>> + } else { >>> + memset(r_pipe_cfg, 0, sizeof(*r_pipe_cfg)); >>> + } >>> + >>> + drm_rect_rotate_inv(&pipe_cfg->src_rect, >>> + new_plane_state->fb->width, new_plane_state->fb->height, >>> + new_plane_state->rotation); >>> + if (r_pipe_cfg->src_rect.x1 != 0) >>> + drm_rect_rotate_inv(&r_pipe_cfg->src_rect, >>> + new_plane_state->fb->width, new_plane_state->fb->height, >>> + new_plane_state->rotation); >>> + >>> + pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); >>> + >>> + return 0; >>> +} >>> + >>> +static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, >>> + struct drm_atomic_state *state, >>> + const struct drm_crtc_state *crtc_state) >>> +{ >>> + struct drm_plane_state *new_plane_state = >>> + drm_atomic_get_new_plane_state(state, plane); >>> + 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 dpu_format *fmt; >>> + struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; >>> + struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; >>> + uint32_t max_linewidth; >>> + unsigned int rotation; >>> + uint32_t supported_rotations; >>> + const struct dpu_sspp_cfg *pipe_hw_caps; >>> + const struct dpu_sspp_sub_blks *sblk; >>> + int ret = 0; >>> + >>> + pipe_hw_caps = pipe->sspp->cap; >>> + sblk = pipe->sspp->cap->sblk; >>> + >>> + /* >>> + * We already have verified scaling against platform limitations. >>> + * Now check if the SSPP supports scaling at all. >>> + */ >>> + if (!sblk->scaler_blk.len && >>> + ((drm_rect_width(&new_plane_state->src) >> 16 != >>> + drm_rect_width(&new_plane_state->dst)) || >>> + (drm_rect_height(&new_plane_state->src) >> 16 != >>> + drm_rect_height(&new_plane_state->dst)))) >>> + return -ERANGE; >>> + >>> + 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; >>> + >>> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>> + >>> + max_linewidth = pdpu->catalog->caps->max_linewidth; >>> + >>> + ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, >>> + &crtc_state->adjusted_mode); >>> + if (ret) >>> + return ret; >>> + >>> + if (r_pipe_cfg->src_rect.x1 != 0) { >>> /* >>> * In parallel multirect case only the half of the usual width >>> * is supported for tiled formats. If we are here, we know that >>> @@ -895,12 +937,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, >>> return -E2BIG; >>> } >>> >>> - 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; >>> - } >>> - >>> 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) && >>> @@ -922,26 +958,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, >>> 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; >>> - } >>> - >>> - drm_rect_rotate_inv(&pipe_cfg->src_rect, >>> - new_plane_state->fb->width, new_plane_state->fb->height, >>> - new_plane_state->rotation); >>> - if (r_pipe->sspp) >>> - drm_rect_rotate_inv(&r_pipe_cfg->src_rect, >>> - new_plane_state->fb->width, new_plane_state->fb->height, >>> - new_plane_state->rotation); >>> - >>> - ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, &crtc_state->adjusted_mode); >>> - if (ret) >>> - return ret; >>> - >>> - if (r_pipe->sspp) { >>> ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt, >>> &crtc_state->adjusted_mode); >>> if (ret) >>> @@ -964,11 +980,45 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, >>> } >>> >>> pstate->rotation = rotation; >>> - pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); >>> >>> return 0; >>> } >>> >>> +static int dpu_plane_atomic_check(struct drm_plane *plane, >>> + struct drm_atomic_state *state) >>> +{ >>> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, >>> + plane); >>> + int ret = 0; >>> + struct dpu_plane *pdpu = to_dpu_plane(plane); >>> + struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); >>> + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); >>> + struct dpu_sw_pipe *pipe = &pstate->pipe; >>> + struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; >>> + const struct drm_crtc_state *crtc_state = NULL; >>> + >>> + if (new_plane_state->crtc) >>> + crtc_state = drm_atomic_get_new_crtc_state(state, >>> + new_plane_state->crtc); >>> + >>> + if (pdpu->pipe != SSPP_NONE) { >> >> This check was not present iirc, why did you have to add this? >> RM will return the same SSPP unless freed. So why this additional check? > > If pdpu->pipe is not SSPP_NONE, then virtual planes are disabled and > there is a fixed 1:1 relationship between planes and SSPP blocks. > True, pdpu->pipe is currently assigned in dpu_plane_init(), so we will always be hitting this condition. Perhaps the patches later on are changing that, so shouldnt this part come along with those? >> >>> + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); >>> + r_pipe->sspp = NULL; >>> + } >>> + >>> + if (!pipe->sspp) >>> + return -EINVAL; >>> + >>> + ret = dpu_plane_atomic_check_nopipe(plane, new_plane_state, crtc_state); >>> + if (ret) >>> + return ret; >>> + >>> + if (!new_plane_state->visible) >>> + return 0; >>> + >>> + return dpu_plane_atomic_check_pipes(plane, state, crtc_state); >>> +} >>> + >>> static void dpu_plane_flush_csc(struct dpu_plane *pdpu, struct dpu_sw_pipe *pipe) >>> { >>> const struct dpu_format *format = > > >
On Thu, 6 Jun 2024 at 02:47, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 6/5/2024 4:32 PM, Dmitry Baryshkov wrote: > > On Thu, 6 Jun 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > >>> Split dpu_plane_atomic_check() function into two pieces: > >>> > >>> dpu_plane_atomic_check_nopipe() performing generic checks on the pstate, > >>> without touching the associated pipe, > >>> > >>> and > >>> > >>> dpu_plane_atomic_check_pipes(), which takes into account used pipes. > >>> > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>> --- > >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 184 ++++++++++++++-------- > >>> 1 file changed, 117 insertions(+), 67 deletions(-) > >>> > >> > >> One thing which seemed odd to me is even dpu_plane_atomic_check_nopipe() > >> does use pipe_cfg even though its named "nopipe". > >> > >> Perhaps were you targetting a split of SW planes vs SSPP atomic_check? > >> > >> I tried applying this patch on top of msm-next to more closely review > >> the split up but it does not apply. So, I will review this patch a > >> little better after it is re-spun. But will proceed with remaining patches. > >> > >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > >>> index 6360052523b5..187ac2767a2b 100644 > >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > >>> @@ -788,50 +788,22 @@ static int dpu_plane_atomic_check_pipe(struct dpu_plane *pdpu, > >>> #define MAX_UPSCALE_RATIO 20 > >>> #define MAX_DOWNSCALE_RATIO 4 > >>> > >>> -static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> - struct drm_atomic_state *state) > >>> +static int dpu_plane_atomic_check_nopipe(struct drm_plane *plane, > >>> + struct drm_plane_state *new_plane_state, > >>> + const struct drm_crtc_state *crtc_state) > >>> { > >>> - struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > >>> - plane); > >>> int ret = 0, min_scale, max_scale; > >>> struct dpu_plane *pdpu = to_dpu_plane(plane); > >>> struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base); > >>> u64 max_mdp_clk_rate = kms->perf.max_core_clk_rate; > >>> struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); > >>> - struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > >>> - 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_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > >>> struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > >>> struct drm_rect fb_rect = { 0 }; > >>> uint32_t max_linewidth; > >>> - unsigned int rotation; > >>> - uint32_t supported_rotations; > >>> - const struct dpu_sspp_cfg *pipe_hw_caps; > >>> - const struct dpu_sspp_sub_blks *sblk; > >>> > >>> - if (new_plane_state->crtc) > >>> - crtc_state = drm_atomic_get_new_crtc_state(state, > >>> - new_plane_state->crtc); > >>> - > >>> - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > >>> - r_pipe->sspp = NULL; > >>> - > >>> - if (!pipe->sspp) > >>> - return -EINVAL; > >>> - > >>> - pipe_hw_caps = pipe->sspp->cap; > >>> - sblk = pipe->sspp->cap->sblk; > >>> - > >>> - if (sblk->scaler_blk.len) { > >>> - min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); > >>> - max_scale = MAX_DOWNSCALE_RATIO << 16; > >>> - } else { > >>> - min_scale = 1 << 16; > >>> - max_scale = 1 << 16; > >>> - } > >>> + min_scale = FRAC_16_16(1, MAX_UPSCALE_RATIO); > >>> + max_scale = MAX_DOWNSCALE_RATIO << 16; > >>> > >>> ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, > >>> min_scale, > >>> @@ -844,11 +816,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> if (!new_plane_state->visible) > >>> return 0; > >>> > >>> - 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; > >>> - > >>> pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > >>> if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > >>> DPU_ERROR("> %d plane stages assigned\n", > >>> @@ -872,8 +839,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> return -E2BIG; > >>> } > >>> > >>> - fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > >>> - > >>> max_linewidth = pdpu->catalog->caps->max_linewidth; > >>> > >>> drm_rect_rotate(&pipe_cfg->src_rect, > >>> @@ -882,6 +847,83 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> > >>> if ((drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) || > >>> _dpu_plane_calc_clk(&crtc_state->adjusted_mode, pipe_cfg) > max_mdp_clk_rate) { > >>> + 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; > >>> + } > >>> + > >>> + *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; > >>> + } else { > >>> + memset(r_pipe_cfg, 0, sizeof(*r_pipe_cfg)); > >>> + } > >>> + > >>> + drm_rect_rotate_inv(&pipe_cfg->src_rect, > >>> + new_plane_state->fb->width, new_plane_state->fb->height, > >>> + new_plane_state->rotation); > >>> + if (r_pipe_cfg->src_rect.x1 != 0) > >>> + drm_rect_rotate_inv(&r_pipe_cfg->src_rect, > >>> + new_plane_state->fb->width, new_plane_state->fb->height, > >>> + new_plane_state->rotation); > >>> + > >>> + pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, > >>> + struct drm_atomic_state *state, > >>> + const struct drm_crtc_state *crtc_state) > >>> +{ > >>> + struct drm_plane_state *new_plane_state = > >>> + drm_atomic_get_new_plane_state(state, plane); > >>> + 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 dpu_format *fmt; > >>> + struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > >>> + struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > >>> + uint32_t max_linewidth; > >>> + unsigned int rotation; > >>> + uint32_t supported_rotations; > >>> + const struct dpu_sspp_cfg *pipe_hw_caps; > >>> + const struct dpu_sspp_sub_blks *sblk; > >>> + int ret = 0; > >>> + > >>> + pipe_hw_caps = pipe->sspp->cap; > >>> + sblk = pipe->sspp->cap->sblk; > >>> + > >>> + /* > >>> + * We already have verified scaling against platform limitations. > >>> + * Now check if the SSPP supports scaling at all. > >>> + */ > >>> + if (!sblk->scaler_blk.len && > >>> + ((drm_rect_width(&new_plane_state->src) >> 16 != > >>> + drm_rect_width(&new_plane_state->dst)) || > >>> + (drm_rect_height(&new_plane_state->src) >> 16 != > >>> + drm_rect_height(&new_plane_state->dst)))) > >>> + return -ERANGE; > >>> + > >>> + 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; > >>> + > >>> + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > >>> + > >>> + max_linewidth = pdpu->catalog->caps->max_linewidth; > >>> + > >>> + ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, > >>> + &crtc_state->adjusted_mode); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + if (r_pipe_cfg->src_rect.x1 != 0) { > >>> /* > >>> * In parallel multirect case only the half of the usual width > >>> * is supported for tiled formats. If we are here, we know that > >>> @@ -895,12 +937,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> return -E2BIG; > >>> } > >>> > >>> - 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; > >>> - } > >>> - > >>> 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) && > >>> @@ -922,26 +958,6 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> 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; > >>> - } > >>> - > >>> - drm_rect_rotate_inv(&pipe_cfg->src_rect, > >>> - new_plane_state->fb->width, new_plane_state->fb->height, > >>> - new_plane_state->rotation); > >>> - if (r_pipe->sspp) > >>> - drm_rect_rotate_inv(&r_pipe_cfg->src_rect, > >>> - new_plane_state->fb->width, new_plane_state->fb->height, > >>> - new_plane_state->rotation); > >>> - > >>> - ret = dpu_plane_atomic_check_pipe(pdpu, pipe, pipe_cfg, fmt, &crtc_state->adjusted_mode); > >>> - if (ret) > >>> - return ret; > >>> - > >>> - if (r_pipe->sspp) { > >>> ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt, > >>> &crtc_state->adjusted_mode); > >>> if (ret) > >>> @@ -964,11 +980,45 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> } > >>> > >>> pstate->rotation = rotation; > >>> - pstate->needs_qos_remap = drm_atomic_crtc_needs_modeset(crtc_state); > >>> > >>> return 0; > >>> } > >>> > >>> +static int dpu_plane_atomic_check(struct drm_plane *plane, > >>> + struct drm_atomic_state *state) > >>> +{ > >>> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, > >>> + plane); > >>> + int ret = 0; > >>> + struct dpu_plane *pdpu = to_dpu_plane(plane); > >>> + struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); > >>> + struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > >>> + struct dpu_sw_pipe *pipe = &pstate->pipe; > >>> + struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; > >>> + const struct drm_crtc_state *crtc_state = NULL; > >>> + > >>> + if (new_plane_state->crtc) > >>> + crtc_state = drm_atomic_get_new_crtc_state(state, > >>> + new_plane_state->crtc); > >>> + > >>> + if (pdpu->pipe != SSPP_NONE) { > >> > >> This check was not present iirc, why did you have to add this? > >> RM will return the same SSPP unless freed. So why this additional check? > > > > If pdpu->pipe is not SSPP_NONE, then virtual planes are disabled and > > there is a fixed 1:1 relationship between planes and SSPP blocks. > > > > True, pdpu->pipe is currently assigned in dpu_plane_init(), so we will > always be hitting this condition. > > Perhaps the patches later on are changing that, so shouldnt this part > come along with those? Ack, I'll move it to patch 5. > > >> > >>> + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > >>> + r_pipe->sspp = NULL; > >>> + } > >>> + > >>> + if (!pipe->sspp) > >>> + return -EINVAL; > >>> + > >>> + ret = dpu_plane_atomic_check_nopipe(plane, new_plane_state, crtc_state); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + if (!new_plane_state->visible) > >>> + return 0; > >>> + > >>> + return dpu_plane_atomic_check_pipes(plane, state, crtc_state); > >>> +} > >>> + > >>> static void dpu_plane_flush_csc(struct dpu_plane *pdpu, struct dpu_sw_pipe *pipe) > >>> { > >>> const struct dpu_format *format = > > > > > >
On Thu, 6 Jun 2024 at 11:53, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 6 Jun 2024 at 02:47, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > On 6/5/2024 4:32 PM, Dmitry Baryshkov wrote: > > > On Thu, 6 Jun 2024 at 02:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > >> > > >> > > >> > > >> On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > > >>> Split dpu_plane_atomic_check() function into two pieces: > > >>> > > >>> dpu_plane_atomic_check_nopipe() performing generic checks on the pstate, > > >>> without touching the associated pipe, > > >>> > > >>> and > > >>> > > >>> dpu_plane_atomic_check_pipes(), which takes into account used pipes. > > >>> > > >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > >>> --- > > >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 184 ++++++++++++++-------- > > >>> 1 file changed, 117 insertions(+), 67 deletions(-) > > >>> > > >>> + if (new_plane_state->crtc) > > >>> + crtc_state = drm_atomic_get_new_crtc_state(state, > > >>> + new_plane_state->crtc); > > >>> + > > >>> + if (pdpu->pipe != SSPP_NONE) { > > >> > > >> This check was not present iirc, why did you have to add this? > > >> RM will return the same SSPP unless freed. So why this additional check? > > > > > > If pdpu->pipe is not SSPP_NONE, then virtual planes are disabled and > > > there is a fixed 1:1 relationship between planes and SSPP blocks. > > > > > > > True, pdpu->pipe is currently assigned in dpu_plane_init(), so we will > > always be hitting this condition. > > > > Perhaps the patches later on are changing that, so shouldnt this part > > come along with those? > > Ack, I'll move it to patch 5. Patch 8, of course.
On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > Virtual wide planes give high amount of flexibility, but it is not > always enough: > > In parallel multirect case only the half of the usual width is supported > for tiled formats. Thus the whole width of two tiled multirect > rectangles can not be greater than max_linewidth, which is not enough > for some platforms/compositors. > > Another example is as simple as wide YUV plane. YUV planes can not use > multirect, so currently they are limited to max_linewidth too. > > Now that the planes are fully virtualized, add support for allocating > two SSPP blocks to drive a single DRM plane. This fixes both mentioned > cases and allows all planes to go up to 2*max_linewidth (at the cost of > making some of the planes unavailable to the user). > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 172 ++++++++++++++++------ > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 8 + > 2 files changed, 131 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index 2961b809ccf3..cde20c1fa90d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -886,6 +886,28 @@ static int dpu_plane_atomic_check_nopipe(struct drm_plane *plane, > return 0; > } > > +static int dpu_plane_is_multirect_parallel_capable(struct dpu_sw_pipe *pipe, > + struct dpu_sw_pipe_cfg *pipe_cfg, > + const struct dpu_format *fmt, > + uint32_t max_linewidth) > +{ > + 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)) > + return false; > + > + if (pipe_cfg->rotation & DRM_MODE_ROTATE_90) > + return false; > + > + if (DPU_FORMAT_IS_YUV(fmt)) > + return false; > + > + if (DPU_FORMAT_IS_UBWC(fmt) && > + drm_rect_width(&pipe_cfg->src_rect) > max_linewidth / 2) > + return false; > + > + return true; > +} > + This is a good idea to separate out multirect checks to a separate API. I think can push this part of the change even today. > static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, > struct drm_atomic_state *state, > const struct drm_crtc_state *crtc_state) > @@ -899,7 +921,6 @@ static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, > const struct dpu_format *fmt; > struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > - uint32_t max_linewidth; > uint32_t supported_rotations; > const struct dpu_sspp_cfg *pipe_hw_caps; > const struct dpu_sspp_sub_blks *sblk; > @@ -919,15 +940,8 @@ static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, > drm_rect_height(&new_plane_state->dst)))) > return -ERANGE; > > - 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; > - > fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > > - max_linewidth = pdpu->catalog->caps->max_linewidth; > - > supported_rotations = DRM_MODE_REFLECT_MASK | DRM_MODE_ROTATE_0; > > if (pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION)) > @@ -943,41 +957,6 @@ static int dpu_plane_atomic_check_pipes(struct drm_plane *plane, > return ret; > > if (drm_rect_width(&r_pipe_cfg->src_rect) != 0) { > - /* > - * In parallel multirect case only the half of the usual width > - * is supported for tiled formats. If we are here, we know that > - * full width is more than max_linewidth, thus each rect is > - * wider than allowed. > - */ > - if (DPU_FORMAT_IS_UBWC(fmt) && > - drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { > - DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, tiled format\n", > - DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); > - return -E2BIG; > - } > - > - 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)) || > - pipe_cfg->rotation & DRM_MODE_ROTATE_90 || > - 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; > - > ret = dpu_plane_atomic_check_pipe(pdpu, r_pipe, r_pipe_cfg, fmt, > &crtc_state->adjusted_mode); > if (ret) > @@ -998,16 +977,16 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane); > struct dpu_sw_pipe *pipe = &pstate->pipe; > struct dpu_sw_pipe *r_pipe = &pstate->r_pipe; > + struct dpu_sw_pipe_cfg *pipe_cfg = &pstate->pipe_cfg; > + struct dpu_sw_pipe_cfg *r_pipe_cfg = &pstate->r_pipe_cfg; > const struct drm_crtc_state *crtc_state = NULL; > > if (new_plane_state->crtc) > crtc_state = drm_atomic_get_new_crtc_state(state, > new_plane_state->crtc); > > - if (pdpu->pipe != SSPP_NONE) { > - pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > - r_pipe->sspp = NULL; > - } > + pipe->sspp = dpu_rm_get_sspp(&dpu_kms->rm, pdpu->pipe); > + r_pipe->sspp = NULL; > > if (!pipe->sspp) > return -EINVAL; > @@ -1019,6 +998,52 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > if (!new_plane_state->visible) > return 0; > > + 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; > + > + if (drm_rect_width(&r_pipe_cfg->src_rect) != 0) { > + uint32_t max_linewidth = dpu_kms->catalog->caps->max_linewidth; > + const struct dpu_format *fmt; > + > + fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > + > + /* > + * In parallel multirect case only the half of the usual width > + * is supported for tiled formats. If we are here, we know that > + * full width is more than max_linewidth, thus each rect is > + * wider than allowed. > + */ > + if (DPU_FORMAT_IS_UBWC(fmt) && > + drm_rect_width(&pipe_cfg->src_rect) > max_linewidth) { > + DPU_DEBUG_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u, tiled format\n", > + DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); > + return -E2BIG; > + } > + > + r_pipe->sspp = pipe->sspp; > + > + if (!dpu_plane_is_multirect_parallel_capable(pipe, pipe_cfg, fmt, max_linewidth) || > + !dpu_plane_is_multirect_parallel_capable(r_pipe, r_pipe_cfg, fmt, max_linewidth) || > + !(test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) || > + test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features))) { > + 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->multirect_index = DPU_SSPP_RECT_1; > + r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL; > + } > + I might be wrong here, but it seems like this part was moved to dpu_plane_atomic_check_nopipe() in patch 6 and now being brought back (atleast most of it) to dpu_plane_atomic_check(). If this is correct, then the migration in patch 6 was not necessary. > return dpu_plane_atomic_check_pipes(plane, state, crtc_state); > } > > @@ -1053,10 +1078,18 @@ static int dpu_plane_virtual_atomic_check(struct drm_plane *plane, > > format = to_dpu_format(msm_framebuffer_format(plane_state->fb)); > > - /* force resource reallocation if the format of FB has changed */ > - if (pstate->saved_fmt != format) { > + /* force resource reallocation if the format of FB or src/dst have changed */ > + if (pstate->saved_fmt != format || > + pstate->saved_src_w != plane_state->src_w || > + pstate->saved_src_h != plane_state->src_h || > + pstate->saved_src_w != plane_state->src_w || > + pstate->saved_crtc_h != plane_state->crtc_h) { > crtc_state->planes_changed = true; > pstate->saved_fmt = format; > + pstate->saved_src_w = plane_state->src_w; > + pstate->saved_src_h = plane_state->src_h; > + pstate->saved_crtc_w = plane_state->crtc_w; > + pstate->saved_crtc_h = plane_state->crtc_h; > } Is thic chunk checking that if the src/dst have changed, this plane might span more than one LM? Will be better to add more comment on why we are also checking src/dest changes. Overall, can we just use for_each_oldnew_plane_in_state to compare the prev and current dimensions and drop saved_****? > > return 0; > @@ -1074,7 +1107,10 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, > struct dpu_plane_state *pstate; > struct dpu_sw_pipe *pipe; > struct dpu_sw_pipe *r_pipe; > + struct dpu_sw_pipe_cfg *pipe_cfg; > + struct dpu_sw_pipe_cfg *r_pipe_cfg; > const struct dpu_format *fmt; > + uint32_t max_linewidth; > > if (plane_state->crtc) > crtc_state = drm_atomic_get_new_crtc_state(state, > @@ -1083,6 +1119,8 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, > pstate = to_dpu_plane_state(plane_state); > pipe = &pstate->pipe; > r_pipe = &pstate->r_pipe; > + pipe_cfg = &pstate->pipe_cfg; > + r_pipe_cfg = &pstate->r_pipe_cfg; > > pipe->sspp = NULL; > r_pipe->sspp = NULL; > @@ -1097,10 +1135,46 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, > > reqs.rot90 = drm_rotation_90_or_270(plane_state->rotation); > > + max_linewidth = dpu_kms->catalog->caps->max_linewidth; > + > pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, &reqs); > if (!pipe->sspp) > return -ENODEV; > > + if (drm_rect_width(&r_pipe_cfg->src_rect) == 0) { > + 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; > + } else { > + if (dpu_plane_is_multirect_parallel_capable(pipe, pipe_cfg, fmt, max_linewidth) && > + dpu_plane_is_multirect_parallel_capable(r_pipe, r_pipe_cfg, fmt, max_linewidth) && > + (test_bit(DPU_SSPP_SMART_DMA_V1, &pipe->sspp->cap->features) || > + test_bit(DPU_SSPP_SMART_DMA_V2, &pipe->sspp->cap->features))) { > + r_pipe->sspp = pipe->sspp; > + > + pipe->multirect_index = DPU_SSPP_RECT_0; > + pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL; > + > + r_pipe->multirect_index = DPU_SSPP_RECT_1; > + r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL; > + } else { > + /* multirect is not possible, use two SSPP blocks */ > + r_pipe->sspp = dpu_rm_reserve_sspp(&dpu_kms->rm, global_state, crtc, &reqs); > + if (!r_pipe->sspp) > + return -ENODEV; > + > + 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; > + } > + } > + This chunk LGTM. > return dpu_plane_atomic_check_pipes(plane, state, crtc_state); > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > index 15f7d60d8b85..5522f9035d68 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > @@ -31,6 +31,10 @@ > * @plane_clk: calculated clk per plane > * @needs_dirtyfb: whether attached CRTC needs pixel data explicitly flushed > * @saved_fmt: format used by the plane's FB, saved for for virtual plane support > + * @saved_src_w: cached value of plane's src_w, saved for for virtual plane support > + * @saved_src_h: cached value of plane's src_h, saved for for virtual plane support > + * @saved_crtc_w: cached value of plane's crtc_w, saved for for virtual plane support > + * @saved_crtc_h: cached value of plane's crtc_h, saved for for virtual plane support > */ > struct dpu_plane_state { > struct drm_plane_state base; > @@ -49,6 +53,10 @@ struct dpu_plane_state { > bool needs_dirtyfb; > > const struct dpu_format *saved_fmt; > + uint32_t saved_src_w; > + uint32_t saved_src_h; > + uint32_t saved_crtc_w; > + uint32_t saved_crtc_h; > }; > > #define to_dpu_plane_state(x) \
On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > Since we have enabled sharing of SSPP blocks between two planes, it is > now possible to use twice as much planes as there are hardware SSPP > blocks. Create additional overlay planes. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > It is possible to slightly bend the limitations of the HW blender. If > two rectangles are contiguous (like two rectangles of a single plane) > they can be blended using a single LM blending stage, allowing one to > blend more planes via a single LM. > Can you pls let me know the source of this optimization (assuming its present downstream) ? Otherwise I will have to lookup some more docs to confirm this. It certainly makes sense, that if the same layer is being split across two SSPP's we can certainly use the same blend stage. But want to make sure this is already in place somewhere and not something which was tried and just worked. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 37 ++++++++++++++++++----- > 2 files changed, 37 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 794c5643584f..fbbd7f635d04 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -445,6 +445,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, > > uint32_t lm_idx; > bool bg_alpha_enable = false; > + unsigned int stage_indices[DPU_STAGE_MAX] = {}; > DECLARE_BITMAP(fetch_active, SSPP_MAX); > > memset(fetch_active, 0, sizeof(fetch_active)); > @@ -469,7 +470,9 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, > mixer, cstate->num_mixers, > pstate->stage, > format, fb ? fb->modifier : 0, > - &pstate->pipe, 0, stage_cfg); > + &pstate->pipe, > + stage_indices[pstate->stage]++, > + stage_cfg); > > if (pstate->r_pipe.sspp) { > set_bit(pstate->r_pipe.sspp->idx, fetch_active); > @@ -477,7 +480,9 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, > mixer, cstate->num_mixers, > pstate->stage, > format, fb ? fb->modifier : 0, > - &pstate->r_pipe, 1, stage_cfg); > + &pstate->r_pipe, > + stage_indices[pstate->stage]++, > + stage_cfg); > } Is this part of the change related to this patch? We moved from hard-coding 0 and 1 for the stage_idx to stage_indices[pstate->stage] will still result in the same values of 0 and 1 right? The sharing will be achieved with the change below of doing pstate->stage = prev_pstate->stage. Rest of the change LGTM. > > /* blend config update */ > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index 2e1c544efc4a..43dfe13eb298 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -827,13 +827,6 @@ static int dpu_plane_atomic_check_nopipe(struct drm_plane *plane, > if (!new_plane_state->visible) > return 0; > > - pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > - if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > - DPU_ERROR("> %d plane stages assigned\n", > - pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > - return -EINVAL; > - } > - > /* state->src is 16.16, src_rect is not */ > drm_rect_fp_to_int(&pipe_cfg->src_rect, &new_plane_state->src); > > @@ -971,6 +964,18 @@ static int dpu_plane_try_multirect(struct dpu_plane_state *pstate, > prev_pipe->multirect_index = DPU_SSPP_RECT_0; > prev_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL; > > + if (pipe_cfg->dst_rect.y1 == prev_pipe_cfg->dst_rect.y1 && > + pipe_cfg->dst_rect.y2 == prev_pipe_cfg->dst_rect.y2 && > + pipe_cfg->dst_rect.x1 == prev_pipe_cfg->dst_rect.x2) { > + pstate->stage = prev_pstate->stage; > + } else if (pipe_cfg->dst_rect.y1 == prev_pipe_cfg->dst_rect.y1 && > + pipe_cfg->dst_rect.y2 == prev_pipe_cfg->dst_rect.y2 && > + pipe_cfg->dst_rect.x2 == prev_pipe_cfg->dst_rect.x1) { > + pstate->stage = prev_pstate->stage; > + pipe->multirect_index = DPU_SSPP_RECT_0; > + prev_pipe->multirect_index = DPU_SSPP_RECT_1; > + } > + > return true; > } > > @@ -1080,6 +1085,13 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > if (!new_plane_state->visible) > return 0; > > + pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > + if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > + DPU_ERROR("> %d plane stages assigned\n", > + pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > + return -EINVAL; > + } > + > pipe->multirect_index = DPU_SSPP_RECT_SOLO; > pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE; > r_pipe->multirect_index = DPU_SSPP_RECT_SOLO; > @@ -1221,6 +1233,11 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, > > max_linewidth = dpu_kms->catalog->caps->max_linewidth; > > + if (prev_pstate) > + pstate->stage = prev_pstate->stage + 1; > + else > + pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > + > if (drm_rect_width(&r_pipe_cfg->src_rect) == 0) { > if (!prev_pstate || > !dpu_plane_try_multirect(pstate, prev_pstate, fmt, max_linewidth)) { > @@ -1267,6 +1284,12 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, > } > } > > + if (pstate->stage >= dpu_kms->catalog->caps->max_mixer_blendstages) { > + DPU_ERROR("> %d plane stages assigned\n", > + dpu_kms->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > + return -EINVAL; > + } > + > return dpu_plane_atomic_check_pipes(plane, state, crtc_state); > } >
On Wed, 12 Jun 2024 at 04:48, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 3/13/2024 5:02 PM, Dmitry Baryshkov wrote: > > It is possible to slightly bend the limitations of the HW blender. If > > two rectangles are contiguous (like two rectangles of a single plane) > > they can be blended using a single LM blending stage, allowing one to > > blend more planes via a single LM. > > > > Can you pls let me know the source of this optimization (assuming its > present downstream) ? > > Otherwise I will have to lookup some more docs to confirm this. > > It certainly makes sense, that if the same layer is being split across > two SSPP's we can certainly use the same blend stage. But want to make > sure this is already in place somewhere and not something which was > tried and just worked. My source was the original 'virtual' / 'multirect' implementation in the SDE driver. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 9 ++++-- > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 37 ++++++++++++++++++----- > > 2 files changed, 37 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index 794c5643584f..fbbd7f635d04 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -445,6 +445,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, > > > > uint32_t lm_idx; > > bool bg_alpha_enable = false; > > + unsigned int stage_indices[DPU_STAGE_MAX] = {}; > > DECLARE_BITMAP(fetch_active, SSPP_MAX); > > > > memset(fetch_active, 0, sizeof(fetch_active)); > > @@ -469,7 +470,9 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, > > mixer, cstate->num_mixers, > > pstate->stage, > > format, fb ? fb->modifier : 0, > > - &pstate->pipe, 0, stage_cfg); > > + &pstate->pipe, > > + stage_indices[pstate->stage]++, > > + stage_cfg); > > > > if (pstate->r_pipe.sspp) { > > set_bit(pstate->r_pipe.sspp->idx, fetch_active); > > @@ -477,7 +480,9 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, > > mixer, cstate->num_mixers, > > pstate->stage, > > format, fb ? fb->modifier : 0, > > - &pstate->r_pipe, 1, stage_cfg); > > + &pstate->r_pipe, > > + stage_indices[pstate->stage]++, > > + stage_cfg); > > } > > Is this part of the change related to this patch? We moved from > hard-coding 0 and 1 for the stage_idx to stage_indices[pstate->stage] > will still result in the same values of 0 and 1 right? No. The stage can span multiple planes now, see one of the chunks below. > > The sharing will be achieved with the change below of doing > pstate->stage = prev_pstate->stage. > > Rest of the change LGTM. > > > > > > /* blend config update */ > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > index 2e1c544efc4a..43dfe13eb298 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > @@ -827,13 +827,6 @@ static int dpu_plane_atomic_check_nopipe(struct drm_plane *plane, > > if (!new_plane_state->visible) > > return 0; > > > > - pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > > - if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > > - DPU_ERROR("> %d plane stages assigned\n", > > - pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > > - return -EINVAL; > > - } > > - > > /* state->src is 16.16, src_rect is not */ > > drm_rect_fp_to_int(&pipe_cfg->src_rect, &new_plane_state->src); > > > > @@ -971,6 +964,18 @@ static int dpu_plane_try_multirect(struct dpu_plane_state *pstate, > > prev_pipe->multirect_index = DPU_SSPP_RECT_0; > > prev_pipe->multirect_mode = DPU_SSPP_MULTIRECT_PARALLEL; > > > > + if (pipe_cfg->dst_rect.y1 == prev_pipe_cfg->dst_rect.y1 && > > + pipe_cfg->dst_rect.y2 == prev_pipe_cfg->dst_rect.y2 && > > + pipe_cfg->dst_rect.x1 == prev_pipe_cfg->dst_rect.x2) { > > + pstate->stage = prev_pstate->stage; > > + } else if (pipe_cfg->dst_rect.y1 == prev_pipe_cfg->dst_rect.y1 && > > + pipe_cfg->dst_rect.y2 == prev_pipe_cfg->dst_rect.y2 && > > + pipe_cfg->dst_rect.x2 == prev_pipe_cfg->dst_rect.x1) { > > + pstate->stage = prev_pstate->stage; > > + pipe->multirect_index = DPU_SSPP_RECT_0; > > + prev_pipe->multirect_index = DPU_SSPP_RECT_1; > > + } > > + > > return true; > > } > > > > @@ -1080,6 +1085,13 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > if (!new_plane_state->visible) > > return 0; > > > > + pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > > + if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > > + DPU_ERROR("> %d plane stages assigned\n", > > + pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > > + return -EINVAL; > > + } > > + > > pipe->multirect_index = DPU_SSPP_RECT_SOLO; > > pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE; > > r_pipe->multirect_index = DPU_SSPP_RECT_SOLO; > > @@ -1221,6 +1233,11 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, > > > > max_linewidth = dpu_kms->catalog->caps->max_linewidth; > > > > + if (prev_pstate) > > + pstate->stage = prev_pstate->stage + 1; > > + else > > + pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > > + > > if (drm_rect_width(&r_pipe_cfg->src_rect) == 0) { > > if (!prev_pstate || > > !dpu_plane_try_multirect(pstate, prev_pstate, fmt, max_linewidth)) { > > @@ -1267,6 +1284,12 @@ static int dpu_plane_virtual_assign_resources(struct drm_crtc *crtc, > > } > > } > > > > + if (pstate->stage >= dpu_kms->catalog->caps->max_mixer_blendstages) { > > + DPU_ERROR("> %d plane stages assigned\n", > > + dpu_kms->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > > + return -EINVAL; > > + } > > + > > return dpu_plane_atomic_check_pipes(plane, state, crtc_state); > > } > >
As promised in the basic wide planes support ([1]) here comes a series supporting 2*max_linewidth for all the planes. Note: Unlike v1 and v2 this series finally includes support for additional planes - having more planes than the number of SSPP blocks. Note: this iteration features handling of rotation and reflection of the wide plane. However rot90 is still not tested: it is enabled on sc7280 and it only supports UBWC (tiled) framebuffers, it was quite low on my priority list. [1] https://patchwork.freedesktop.org/series/99909/ Changes since v3: - Dropped the drm_atomic_helper_check_plane_noscale (Ville) - Reworked the scaling factor according to global value and then check if SSPP has scaler_blk later on. - Split drm_rect_fp_to_int from the rotation-related fix (Abhinav) Changes since v2: - Dropped the encoder-related parts, leave all resource allocation as is (Abhinav) - Significantly reworked the SSPP allocation code - Added debugging code to dump RM state in dri/N/state Changes since v1: - Fixed build error due to me missing one of fixups, it was left uncommitted. - Implementated proper handling of wide plane rotation & reflection. Dmitry Baryshkov (13): drm/msm/dpu: take plane rotation into account for wide planes drm/msm/dpu: use drm_rect_fp_to_int() drm/msm/dpu: move pstate->pipe initialization to dpu_plane_atomic_check drm/msm/dpu: drop virt_formats from SSPP subblock configuration drm/msm/dpu: move scaling limitations out of the hw_catalog drm/msm/dpu: split dpu_plane_atomic_check() drm/msm/dpu: move rot90 checking to dpu_plane_atomic_check_pipe() drm/msm/dpu: add support for virtual planes drm/msm/dpu: allow using two SSPP blocks for a single plane drm/msm/dpu: allow sharing SSPP between planes drm/msm/dpu: create additional virtual planes drm/msm/dpu: allow sharing of blending stages drm/msm/dpu: include SSPP allocation state into the dumped state drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 59 +- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 - .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 + drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 675 +++++++++++++++--- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 29 +- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 84 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 28 + 10 files changed, 779 insertions(+), 152 deletions(-)