| Message ID | 20230203182132.1307834-21-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: wide planes support | expand |
On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: > Now as all accesses to pipe_cfg and pstate have been cleaned, re-add > struct dpu_hw_pipe_cfg back to dpu_plane_state, so that > dpu_plane_atomic_check() and dpu_plane_atomic_update() do not have a > chance to disagree about src/dst rectangles (currently > dpu_plane_atomic_check() uses unclipped rectangles, while > dpu_plane_atomic_update() uses clipped rectangles calculated by > drm_atomic_helper_check_plane_state()). > The title of the patch should now say "add dpu_hw_sspp_cfg" I have a question on the commit text, why does it say "re-add" and not "add". dpu_hw_pipe_cfg/dpu_hw_sspp_cfg was not a part of dpu_plane_state even before and I dont recall it was removed in this series and then added back. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 64 ++++++++++------------- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 + > 2 files changed, 30 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index 09a3fde1c910..ecf5402ab61a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -951,7 +951,8 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); > const struct drm_crtc_state *crtc_state = NULL; > const struct dpu_format *fmt; > - struct drm_rect src, dst, fb_rect = { 0 }; > + struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; > + struct drm_rect fb_rect = { 0 }; > uint32_t min_src_size, max_linewidth; > unsigned int rotation; > uint32_t supported_rotations; > @@ -984,12 +985,15 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > return -EINVAL; > } > > - src.x1 = new_plane_state->src_x >> 16; > - src.y1 = new_plane_state->src_y >> 16; > - src.x2 = src.x1 + (new_plane_state->src_w >> 16); > - src.y2 = src.y1 + (new_plane_state->src_h >> 16); > + pipe_cfg->src_rect = new_plane_state->src; > > - dst = drm_plane_state_dest(new_plane_state); > + /* state->src is 16.16, src_rect is not */ > + pipe_cfg->src_rect.x1 >>= 16; > + pipe_cfg->src_rect.x2 >>= 16; > + pipe_cfg->src_rect.y1 >>= 16; > + pipe_cfg->src_rect.y2 >>= 16; > + > + pipe_cfg->dst_rect = new_plane_state->dst; > > fb_rect.x2 = new_plane_state->fb->width; > fb_rect.y2 = new_plane_state->fb->height; > @@ -1008,30 +1012,30 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > return -EINVAL; > > /* check src bounds */ > - } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) { > + } else if (!dpu_plane_validate_src(&pipe_cfg->src_rect, &fb_rect, min_src_size)) { > DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", > - DRM_RECT_ARG(&src)); > + DRM_RECT_ARG(&pipe_cfg->src_rect)); > return -E2BIG; > > /* valid yuv image */ > } else if (DPU_FORMAT_IS_YUV(fmt) && > - (src.x1 & 0x1 || src.y1 & 0x1 || > - drm_rect_width(&src) & 0x1 || > - drm_rect_height(&src) & 0x1)) { > + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & 0x1 || > + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || > + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { > DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", > - DRM_RECT_ARG(&src)); > + DRM_RECT_ARG(&pipe_cfg->src_rect)); > return -EINVAL; > > /* min dst support */ > - } else if (drm_rect_width(&dst) < 0x1 || drm_rect_height(&dst) < 0x1) { > + } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { > DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", > - DRM_RECT_ARG(&dst)); > + DRM_RECT_ARG(&pipe_cfg->dst_rect)); > return -EINVAL; > > /* check decimated source width */ > - } else if (drm_rect_width(&src) > max_linewidth) { > + } else 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(&src), max_linewidth); > + DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); > return -E2BIG; > } > > @@ -1045,7 +1049,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > > if ((pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION)) && > (rotation & DRM_MODE_ROTATE_90)) { > - ret = dpu_plane_check_inline_rotation(pdpu, sblk, src, fmt); > + ret = dpu_plane_check_inline_rotation(pdpu, sblk, pipe_cfg->src_rect, fmt); > if (ret) > return ret; > } > @@ -1120,9 +1124,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) > bool is_rt_pipe; > const struct dpu_format *fmt = > to_dpu_format(msm_framebuffer_format(fb)); > - struct dpu_hw_sspp_cfg pipe_cfg; > - > - memset(&pipe_cfg, 0, sizeof(struct dpu_hw_sspp_cfg)); > + struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; > > _dpu_plane_set_scanout(plane, pstate, fb); > > @@ -1139,16 +1141,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) > crtc->base.id, DRM_RECT_ARG(&state->dst), > (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt)); > > - pipe_cfg.src_rect = state->src; > - > - /* state->src is 16.16, src_rect is not */ > - pipe_cfg.src_rect.x1 >>= 16; > - pipe_cfg.src_rect.x2 >>= 16; > - pipe_cfg.src_rect.y1 >>= 16; > - pipe_cfg.src_rect.y2 >>= 16; > - > - pipe_cfg.dst_rect = state->dst; > - > /* override for color fill */ > if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) { > /* skip remaining processing on color fill */ > @@ -1157,10 +1149,10 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) > > if (pipe->sspp->ops.setup_rects) { > pipe->sspp->ops.setup_rects(pipe, > - &pipe_cfg); > + pipe_cfg); > } > > - _dpu_plane_setup_scaler(pipe, fmt, false, &pipe_cfg, pstate->rotation); > + _dpu_plane_setup_scaler(pipe, fmt, false, pipe_cfg, pstate->rotation); > > if (pipe->sspp->ops.setup_multirect) > pipe->sspp->ops.setup_multirect( > @@ -1201,12 +1193,12 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) > } > } > > - _dpu_plane_set_qos_lut(plane, pipe, fmt, &pipe_cfg); > + _dpu_plane_set_qos_lut(plane, pipe, fmt, &pstate->pipe_cfg); > _dpu_plane_set_danger_lut(plane, pipe, fmt); > > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > _dpu_plane_set_qos_ctrl(plane, pipe, true, DPU_PLANE_QOS_PANIC_CTRL); > - _dpu_plane_set_ot_limit(plane, pipe, crtc, &pipe_cfg); > + _dpu_plane_set_ot_limit(plane, pipe, crtc, &pstate->pipe_cfg); > } > > if (pstate->needs_qos_remap) { > @@ -1214,9 +1206,9 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) > _dpu_plane_set_qos_remap(plane, pipe); > } > > - pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, &pipe_cfg); > + pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, &pstate->pipe_cfg); > > - pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, &pipe_cfg); > + pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, &pstate->pipe_cfg); > } > > static void _dpu_plane_atomic_disable(struct drm_plane *plane) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > index a08b0539513b..079dad83eb37 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > @@ -19,6 +19,7 @@ > * @base: base drm plane state object > * @aspace: pointer to address space for input/output buffers > * @pipe: software pipe description > + * @pipe_cfg: software pipe configuration > * @stage: assigned by crtc blender > * @needs_qos_remap: qos remap settings need to be updated > * @multirect_index: index of the rectangle of SSPP > @@ -33,6 +34,7 @@ struct dpu_plane_state { > struct drm_plane_state base; > struct msm_gem_address_space *aspace; > struct dpu_sw_pipe pipe; > + struct dpu_hw_sspp_cfg pipe_cfg; > enum dpu_stage stage; > bool needs_qos_remap; > bool pending;
On 06/02/2023 21:07, Abhinav Kumar wrote: > > > On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >> Now as all accesses to pipe_cfg and pstate have been cleaned, re-add >> struct dpu_hw_pipe_cfg back to dpu_plane_state, so that >> dpu_plane_atomic_check() and dpu_plane_atomic_update() do not have a >> chance to disagree about src/dst rectangles (currently >> dpu_plane_atomic_check() uses unclipped rectangles, while >> dpu_plane_atomic_update() uses clipped rectangles calculated by >> drm_atomic_helper_check_plane_state()). >> > The title of the patch should now say "add dpu_hw_sspp_cfg" > > I have a question on the commit text, why does it say "re-add" and not > "add". > > dpu_hw_pipe_cfg/dpu_hw_sspp_cfg was not a part of dpu_plane_state even > before and I dont recall it was removed in this series and then added back. Ack, I'll fix both items in v4. > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 64 ++++++++++------------- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 + >> 2 files changed, 30 insertions(+), 36 deletions(-)-- With best wishes Dmitry
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 09a3fde1c910..ecf5402ab61a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -951,7 +951,8 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state); const struct drm_crtc_state *crtc_state = NULL; const struct dpu_format *fmt; - struct drm_rect src, dst, fb_rect = { 0 }; + struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; + struct drm_rect fb_rect = { 0 }; uint32_t min_src_size, max_linewidth; unsigned int rotation; uint32_t supported_rotations; @@ -984,12 +985,15 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, return -EINVAL; } - src.x1 = new_plane_state->src_x >> 16; - src.y1 = new_plane_state->src_y >> 16; - src.x2 = src.x1 + (new_plane_state->src_w >> 16); - src.y2 = src.y1 + (new_plane_state->src_h >> 16); + pipe_cfg->src_rect = new_plane_state->src; - dst = drm_plane_state_dest(new_plane_state); + /* state->src is 16.16, src_rect is not */ + pipe_cfg->src_rect.x1 >>= 16; + pipe_cfg->src_rect.x2 >>= 16; + pipe_cfg->src_rect.y1 >>= 16; + pipe_cfg->src_rect.y2 >>= 16; + + pipe_cfg->dst_rect = new_plane_state->dst; fb_rect.x2 = new_plane_state->fb->width; fb_rect.y2 = new_plane_state->fb->height; @@ -1008,30 +1012,30 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, return -EINVAL; /* check src bounds */ - } else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) { + } else if (!dpu_plane_validate_src(&pipe_cfg->src_rect, &fb_rect, min_src_size)) { DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", - DRM_RECT_ARG(&src)); + DRM_RECT_ARG(&pipe_cfg->src_rect)); return -E2BIG; /* valid yuv image */ } else if (DPU_FORMAT_IS_YUV(fmt) && - (src.x1 & 0x1 || src.y1 & 0x1 || - drm_rect_width(&src) & 0x1 || - drm_rect_height(&src) & 0x1)) { + (pipe_cfg->src_rect.x1 & 0x1 || pipe_cfg->src_rect.y1 & 0x1 || + drm_rect_width(&pipe_cfg->src_rect) & 0x1 || + drm_rect_height(&pipe_cfg->src_rect) & 0x1)) { DPU_DEBUG_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n", - DRM_RECT_ARG(&src)); + DRM_RECT_ARG(&pipe_cfg->src_rect)); return -EINVAL; /* min dst support */ - } else if (drm_rect_width(&dst) < 0x1 || drm_rect_height(&dst) < 0x1) { + } else if (drm_rect_width(&pipe_cfg->dst_rect) < 0x1 || drm_rect_height(&pipe_cfg->dst_rect) < 0x1) { DPU_DEBUG_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n", - DRM_RECT_ARG(&dst)); + DRM_RECT_ARG(&pipe_cfg->dst_rect)); return -EINVAL; /* check decimated source width */ - } else if (drm_rect_width(&src) > max_linewidth) { + } else 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(&src), max_linewidth); + DRM_RECT_ARG(&pipe_cfg->src_rect), max_linewidth); return -E2BIG; } @@ -1045,7 +1049,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, if ((pipe_hw_caps->features & BIT(DPU_SSPP_INLINE_ROTATION)) && (rotation & DRM_MODE_ROTATE_90)) { - ret = dpu_plane_check_inline_rotation(pdpu, sblk, src, fmt); + ret = dpu_plane_check_inline_rotation(pdpu, sblk, pipe_cfg->src_rect, fmt); if (ret) return ret; } @@ -1120,9 +1124,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) bool is_rt_pipe; const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(fb)); - struct dpu_hw_sspp_cfg pipe_cfg; - - memset(&pipe_cfg, 0, sizeof(struct dpu_hw_sspp_cfg)); + struct dpu_hw_sspp_cfg *pipe_cfg = &pstate->pipe_cfg; _dpu_plane_set_scanout(plane, pstate, fb); @@ -1139,16 +1141,6 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) crtc->base.id, DRM_RECT_ARG(&state->dst), (char *)&fmt->base.pixel_format, DPU_FORMAT_IS_UBWC(fmt)); - pipe_cfg.src_rect = state->src; - - /* state->src is 16.16, src_rect is not */ - pipe_cfg.src_rect.x1 >>= 16; - pipe_cfg.src_rect.x2 >>= 16; - pipe_cfg.src_rect.y1 >>= 16; - pipe_cfg.src_rect.y2 >>= 16; - - pipe_cfg.dst_rect = state->dst; - /* override for color fill */ if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) { /* skip remaining processing on color fill */ @@ -1157,10 +1149,10 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) if (pipe->sspp->ops.setup_rects) { pipe->sspp->ops.setup_rects(pipe, - &pipe_cfg); + pipe_cfg); } - _dpu_plane_setup_scaler(pipe, fmt, false, &pipe_cfg, pstate->rotation); + _dpu_plane_setup_scaler(pipe, fmt, false, pipe_cfg, pstate->rotation); if (pipe->sspp->ops.setup_multirect) pipe->sspp->ops.setup_multirect( @@ -1201,12 +1193,12 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) } } - _dpu_plane_set_qos_lut(plane, pipe, fmt, &pipe_cfg); + _dpu_plane_set_qos_lut(plane, pipe, fmt, &pstate->pipe_cfg); _dpu_plane_set_danger_lut(plane, pipe, fmt); if (plane->type != DRM_PLANE_TYPE_CURSOR) { _dpu_plane_set_qos_ctrl(plane, pipe, true, DPU_PLANE_QOS_PANIC_CTRL); - _dpu_plane_set_ot_limit(plane, pipe, crtc, &pipe_cfg); + _dpu_plane_set_ot_limit(plane, pipe, crtc, &pstate->pipe_cfg); } if (pstate->needs_qos_remap) { @@ -1214,9 +1206,9 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) _dpu_plane_set_qos_remap(plane, pipe); } - pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, &pipe_cfg); + pstate->plane_fetch_bw = _dpu_plane_calc_bw(pdpu->catalog, fmt, &crtc->mode, &pstate->pipe_cfg); - pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, &pipe_cfg); + pstate->plane_clk = _dpu_plane_calc_clk(&crtc->mode, &pstate->pipe_cfg); } static void _dpu_plane_atomic_disable(struct drm_plane *plane) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h index a08b0539513b..079dad83eb37 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h @@ -19,6 +19,7 @@ * @base: base drm plane state object * @aspace: pointer to address space for input/output buffers * @pipe: software pipe description + * @pipe_cfg: software pipe configuration * @stage: assigned by crtc blender * @needs_qos_remap: qos remap settings need to be updated * @multirect_index: index of the rectangle of SSPP @@ -33,6 +34,7 @@ struct dpu_plane_state { struct drm_plane_state base; struct msm_gem_address_space *aspace; struct dpu_sw_pipe pipe; + struct dpu_hw_sspp_cfg pipe_cfg; enum dpu_stage stage; bool needs_qos_remap; bool pending;
Now as all accesses to pipe_cfg and pstate have been cleaned, re-add struct dpu_hw_pipe_cfg back to dpu_plane_state, so that dpu_plane_atomic_check() and dpu_plane_atomic_update() do not have a chance to disagree about src/dst rectangles (currently dpu_plane_atomic_check() uses unclipped rectangles, while dpu_plane_atomic_update() uses clipped rectangles calculated by drm_atomic_helper_check_plane_state()). Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 64 ++++++++++------------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 + 2 files changed, 30 insertions(+), 36 deletions(-)