| Message ID | 20221229191856.3508092-13-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: wide planes support | expand |
On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: > Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, leaving > only src_rect and dst_rect. This way right and left pipes will have > separate dpu_hw_pipe_cfg isntances, while the layout is common to both > of them. > Sorry for not responding to this comment earlier. https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370 From the perspective of wide planes you are right that the layout is common but not true from smart DMA point of view. For wide planes, yes, its usually the same buffer with just the src_x being different but conceptually and even HW wise each rectangle of the smart DMA is capable of fetching from a different buffer. From the pov, this decision of not having the dpu_hw_fmt_layout as part of dpu_hw_pipe_cfg seems incorrect to me. > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 ++++++++++----------- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 ++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++---- > 3 files changed, 22 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > index 2bd39c13d54d..400d043f37fa 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > @@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe, > } > > static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, > - struct dpu_hw_pipe_cfg *cfg) > + struct dpu_hw_fmt_layout *layout) > { > struct dpu_hw_sspp *ctx = pipe->sspp; > u32 ystride0, ystride1; > @@ -497,41 +497,41 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, > return; > > if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { > - for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++) > + for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++) > DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx + i * 0x4, > - cfg->layout.plane_addr[i]); > + layout->plane_addr[i]); > } else if (pipe->multirect_index == DPU_SSPP_RECT_0) { > DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx, > - cfg->layout.plane_addr[0]); > + layout->plane_addr[0]); > DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx, > - cfg->layout.plane_addr[2]); > + layout->plane_addr[2]); > } else { > DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx, > - cfg->layout.plane_addr[0]); > + layout->plane_addr[0]); > DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx, > - cfg->layout.plane_addr[2]); > + layout->plane_addr[2]); > } > > if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { > - ystride0 = (cfg->layout.plane_pitch[0]) | > - (cfg->layout.plane_pitch[1] << 16); > - ystride1 = (cfg->layout.plane_pitch[2]) | > - (cfg->layout.plane_pitch[3] << 16); > + ystride0 = (layout->plane_pitch[0]) | > + (layout->plane_pitch[1] << 16); > + ystride1 = (layout->plane_pitch[2]) | > + (layout->plane_pitch[3] << 16); > } else { > ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx); > ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx); > > if (pipe->multirect_index == DPU_SSPP_RECT_0) { > ystride0 = (ystride0 & 0xFFFF0000) | > - (cfg->layout.plane_pitch[0] & 0x0000FFFF); > + (layout->plane_pitch[0] & 0x0000FFFF); > ystride1 = (ystride1 & 0xFFFF0000)| > - (cfg->layout.plane_pitch[2] & 0x0000FFFF); > + (layout->plane_pitch[2] & 0x0000FFFF); > } else { > ystride0 = (ystride0 & 0x0000FFFF) | > - ((cfg->layout.plane_pitch[0] << 16) & > + ((layout->plane_pitch[0] << 16) & > 0xFFFF0000); > ystride1 = (ystride1 & 0x0000FFFF) | > - ((cfg->layout.plane_pitch[2] << 16) & > + ((layout->plane_pitch[2] << 16) & > 0xFFFF0000); > } > } > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > index c713343378aa..8dad52eb2a90 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > @@ -154,13 +154,11 @@ struct dpu_hw_pixel_ext { > > /** > * struct dpu_hw_pipe_cfg : Pipe description > - * @layout: format layout information for programming buffer to hardware > * @src_rect: src ROI, caller takes into account the different operations > * such as decimation, flip etc to program this field > * @dest_rect: destination ROI. > */ > struct dpu_hw_pipe_cfg { > - struct dpu_hw_fmt_layout layout; > struct drm_rect src_rect; > struct drm_rect dst_rect; > }; > @@ -243,10 +241,10 @@ struct dpu_hw_sspp_ops { > /** > * setup_sourceaddress - setup pipe source addresses > * @pipe: Pointer to software pipe context > - * @cfg: Pointer to pipe config structure > + * @layout: format layout information for programming buffer to hardware > */ > void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx, > - struct dpu_hw_pipe_cfg *cfg); > + struct dpu_hw_fmt_layout *layout); > > /** > * setup_csc - setup color space coversion > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index cbff4dea8662..0d2a7170e0ab 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane) > > static void _dpu_plane_set_scanout(struct drm_plane *plane, > struct dpu_plane_state *pstate, > - struct dpu_hw_pipe_cfg *pipe_cfg, > struct drm_framebuffer *fb) > { > struct dpu_plane *pdpu = to_dpu_plane(plane); > 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; > int ret; > > - ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); > + ret = dpu_format_populate_layout(aspace, fb, &layout); > if (ret) > DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); > else if (pstate->pipe.sspp->ops.setup_sourceaddress) { > trace_dpu_plane_set_scanout(&pstate->pipe, > - &pipe_cfg->layout); > - pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, pipe_cfg); > + &layout); > + pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, &layout); > } > } > > @@ -1134,7 +1134,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) > > memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); > > - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb); > + _dpu_plane_set_scanout(plane, pstate, fb); > > pstate->pending = true; >
On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: > > Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, leaving > > only src_rect and dst_rect. This way right and left pipes will have > > separate dpu_hw_pipe_cfg isntances, while the layout is common to both > > of them. > > > > Sorry for not responding to this comment earlier. > > https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370 > > From the perspective of wide planes you are right that the layout is > common but not true from smart DMA point of view. > > For wide planes, yes, its usually the same buffer with just the src_x > being different but conceptually and even HW wise each rectangle of the > smart DMA is capable of fetching from a different buffer. > > From the pov, this decision of not having the dpu_hw_fmt_layout as part > of dpu_hw_pipe_cfg seems incorrect to me. Yes, each rectangle/pipe can fetch from a different buffer. However in our use case the layout is not defined for each pipe. It is defined for a plane, no matter how many pipes are used for the plane, since the buffer is also defined per plane. > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 ++++++++++----------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 ++--- > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++---- > > 3 files changed, 22 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > index 2bd39c13d54d..400d043f37fa 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c > > @@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe, > > } > > > > static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, > > - struct dpu_hw_pipe_cfg *cfg) > > + struct dpu_hw_fmt_layout *layout) > > { > > struct dpu_hw_sspp *ctx = pipe->sspp; > > u32 ystride0, ystride1; > > @@ -497,41 +497,41 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, > > return; > > > > if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { > > - for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++) > > + for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++) > > DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx + i * 0x4, > > - cfg->layout.plane_addr[i]); > > + layout->plane_addr[i]); > > } else if (pipe->multirect_index == DPU_SSPP_RECT_0) { > > DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx, > > - cfg->layout.plane_addr[0]); > > + layout->plane_addr[0]); > > DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx, > > - cfg->layout.plane_addr[2]); > > + layout->plane_addr[2]); > > } else { > > DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx, > > - cfg->layout.plane_addr[0]); > > + layout->plane_addr[0]); > > DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx, > > - cfg->layout.plane_addr[2]); > > + layout->plane_addr[2]); > > } > > > > if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { > > - ystride0 = (cfg->layout.plane_pitch[0]) | > > - (cfg->layout.plane_pitch[1] << 16); > > - ystride1 = (cfg->layout.plane_pitch[2]) | > > - (cfg->layout.plane_pitch[3] << 16); > > + ystride0 = (layout->plane_pitch[0]) | > > + (layout->plane_pitch[1] << 16); > > + ystride1 = (layout->plane_pitch[2]) | > > + (layout->plane_pitch[3] << 16); > > } else { > > ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx); > > ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx); > > > > if (pipe->multirect_index == DPU_SSPP_RECT_0) { > > ystride0 = (ystride0 & 0xFFFF0000) | > > - (cfg->layout.plane_pitch[0] & 0x0000FFFF); > > + (layout->plane_pitch[0] & 0x0000FFFF); > > ystride1 = (ystride1 & 0xFFFF0000)| > > - (cfg->layout.plane_pitch[2] & 0x0000FFFF); > > + (layout->plane_pitch[2] & 0x0000FFFF); > > } else { > > ystride0 = (ystride0 & 0x0000FFFF) | > > - ((cfg->layout.plane_pitch[0] << 16) & > > + ((layout->plane_pitch[0] << 16) & > > 0xFFFF0000); > > ystride1 = (ystride1 & 0x0000FFFF) | > > - ((cfg->layout.plane_pitch[2] << 16) & > > + ((layout->plane_pitch[2] << 16) & > > 0xFFFF0000); > > } > > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > > index c713343378aa..8dad52eb2a90 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h > > @@ -154,13 +154,11 @@ struct dpu_hw_pixel_ext { > > > > /** > > * struct dpu_hw_pipe_cfg : Pipe description > > - * @layout: format layout information for programming buffer to hardware > > * @src_rect: src ROI, caller takes into account the different operations > > * such as decimation, flip etc to program this field > > * @dest_rect: destination ROI. > > */ > > struct dpu_hw_pipe_cfg { > > - struct dpu_hw_fmt_layout layout; > > struct drm_rect src_rect; > > struct drm_rect dst_rect; > > }; > > @@ -243,10 +241,10 @@ struct dpu_hw_sspp_ops { > > /** > > * setup_sourceaddress - setup pipe source addresses > > * @pipe: Pointer to software pipe context > > - * @cfg: Pointer to pipe config structure > > + * @layout: format layout information for programming buffer to hardware > > */ > > void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx, > > - struct dpu_hw_pipe_cfg *cfg); > > + struct dpu_hw_fmt_layout *layout); > > > > /** > > * setup_csc - setup color space coversion > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > index cbff4dea8662..0d2a7170e0ab 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > @@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane) > > > > static void _dpu_plane_set_scanout(struct drm_plane *plane, > > struct dpu_plane_state *pstate, > > - struct dpu_hw_pipe_cfg *pipe_cfg, > > struct drm_framebuffer *fb) > > { > > struct dpu_plane *pdpu = to_dpu_plane(plane); > > 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; > > int ret; > > > > - ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); > > + ret = dpu_format_populate_layout(aspace, fb, &layout); > > if (ret) > > DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); > > else if (pstate->pipe.sspp->ops.setup_sourceaddress) { > > trace_dpu_plane_set_scanout(&pstate->pipe, > > - &pipe_cfg->layout); > > - pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, pipe_cfg); > > + &layout); > > + pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, &layout); > > } > > } > > > > @@ -1134,7 +1134,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) > > > > memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); > > > > - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb); > > + _dpu_plane_set_scanout(plane, pstate, fb); > > > > pstate->pending = true; > >
On 2/2/2023 11:45 AM, Dmitry Baryshkov wrote: > On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>> Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, leaving >>> only src_rect and dst_rect. This way right and left pipes will have >>> separate dpu_hw_pipe_cfg isntances, while the layout is common to both >>> of them. >>> >> >> Sorry for not responding to this comment earlier. >> >> https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370 >> >> From the perspective of wide planes you are right that the layout is >> common but not true from smart DMA point of view. >> >> For wide planes, yes, its usually the same buffer with just the src_x >> being different but conceptually and even HW wise each rectangle of the >> smart DMA is capable of fetching from a different buffer. >> >> From the pov, this decision of not having the dpu_hw_fmt_layout as part >> of dpu_hw_pipe_cfg seems incorrect to me. > > Yes, each rectangle/pipe can fetch from a different buffer. However in > our use case the layout is not defined for each pipe. It is defined > for a plane, no matter how many pipes are used for the plane, since > the buffer is also defined per plane. > Even if the layout is defined per plane. So lets say plane A with layout A maps to rect 1 of DMA0 plane B with layout B maps to rect 2 of DMA0 How can layout be assumed to be duplicated in this case? This is not a wide plane use-case but just smartDMA case of two different layers. Maybe I am missing something but this is the example i am interested about. >> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 ++++++++++----------- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 ++--- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++---- >>> 3 files changed, 22 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>> index 2bd39c13d54d..400d043f37fa 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>> @@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe, >>> } >>> >>> static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, >>> - struct dpu_hw_pipe_cfg *cfg) >>> + struct dpu_hw_fmt_layout *layout) >>> { >>> struct dpu_hw_sspp *ctx = pipe->sspp; >>> u32 ystride0, ystride1; >>> @@ -497,41 +497,41 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, >>> return; >>> >>> if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { >>> - for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++) >>> + for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++) >>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx + i * 0x4, >>> - cfg->layout.plane_addr[i]); >>> + layout->plane_addr[i]); >>> } else if (pipe->multirect_index == DPU_SSPP_RECT_0) { >>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx, >>> - cfg->layout.plane_addr[0]); >>> + layout->plane_addr[0]); >>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx, >>> - cfg->layout.plane_addr[2]); >>> + layout->plane_addr[2]); >>> } else { >>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx, >>> - cfg->layout.plane_addr[0]); >>> + layout->plane_addr[0]); >>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx, >>> - cfg->layout.plane_addr[2]); >>> + layout->plane_addr[2]); >>> } >>> >>> if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { >>> - ystride0 = (cfg->layout.plane_pitch[0]) | >>> - (cfg->layout.plane_pitch[1] << 16); >>> - ystride1 = (cfg->layout.plane_pitch[2]) | >>> - (cfg->layout.plane_pitch[3] << 16); >>> + ystride0 = (layout->plane_pitch[0]) | >>> + (layout->plane_pitch[1] << 16); >>> + ystride1 = (layout->plane_pitch[2]) | >>> + (layout->plane_pitch[3] << 16); >>> } else { >>> ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx); >>> ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx); >>> >>> if (pipe->multirect_index == DPU_SSPP_RECT_0) { >>> ystride0 = (ystride0 & 0xFFFF0000) | >>> - (cfg->layout.plane_pitch[0] & 0x0000FFFF); >>> + (layout->plane_pitch[0] & 0x0000FFFF); >>> ystride1 = (ystride1 & 0xFFFF0000)| >>> - (cfg->layout.plane_pitch[2] & 0x0000FFFF); >>> + (layout->plane_pitch[2] & 0x0000FFFF); >>> } else { >>> ystride0 = (ystride0 & 0x0000FFFF) | >>> - ((cfg->layout.plane_pitch[0] << 16) & >>> + ((layout->plane_pitch[0] << 16) & >>> 0xFFFF0000); >>> ystride1 = (ystride1 & 0x0000FFFF) | >>> - ((cfg->layout.plane_pitch[2] << 16) & >>> + ((layout->plane_pitch[2] << 16) & >>> 0xFFFF0000); >>> } >>> } >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>> index c713343378aa..8dad52eb2a90 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>> @@ -154,13 +154,11 @@ struct dpu_hw_pixel_ext { >>> >>> /** >>> * struct dpu_hw_pipe_cfg : Pipe description >>> - * @layout: format layout information for programming buffer to hardware >>> * @src_rect: src ROI, caller takes into account the different operations >>> * such as decimation, flip etc to program this field >>> * @dest_rect: destination ROI. >>> */ >>> struct dpu_hw_pipe_cfg { >>> - struct dpu_hw_fmt_layout layout; >>> struct drm_rect src_rect; >>> struct drm_rect dst_rect; >>> }; >>> @@ -243,10 +241,10 @@ struct dpu_hw_sspp_ops { >>> /** >>> * setup_sourceaddress - setup pipe source addresses >>> * @pipe: Pointer to software pipe context >>> - * @cfg: Pointer to pipe config structure >>> + * @layout: format layout information for programming buffer to hardware >>> */ >>> void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx, >>> - struct dpu_hw_pipe_cfg *cfg); >>> + struct dpu_hw_fmt_layout *layout); >>> >>> /** >>> * setup_csc - setup color space coversion >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> index cbff4dea8662..0d2a7170e0ab 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> @@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane) >>> >>> static void _dpu_plane_set_scanout(struct drm_plane *plane, >>> struct dpu_plane_state *pstate, >>> - struct dpu_hw_pipe_cfg *pipe_cfg, >>> struct drm_framebuffer *fb) >>> { >>> struct dpu_plane *pdpu = to_dpu_plane(plane); >>> 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; >>> int ret; >>> >>> - ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); >>> + ret = dpu_format_populate_layout(aspace, fb, &layout); >>> if (ret) >>> DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); >>> else if (pstate->pipe.sspp->ops.setup_sourceaddress) { >>> trace_dpu_plane_set_scanout(&pstate->pipe, >>> - &pipe_cfg->layout); >>> - pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, pipe_cfg); >>> + &layout); >>> + pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, &layout); >>> } >>> } >>> >>> @@ -1134,7 +1134,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) >>> >>> memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); >>> >>> - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb); >>> + _dpu_plane_set_scanout(plane, pstate, fb); >>> >>> pstate->pending = true; >>> > > >
On 02/02/2023 21:54, Abhinav Kumar wrote: > > > On 2/2/2023 11:45 AM, Dmitry Baryshkov wrote: >> On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar <quic_abhinavk@quicinc.com> >> wrote: >>> >>> >>> >>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>>> Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, leaving >>>> only src_rect and dst_rect. This way right and left pipes will have >>>> separate dpu_hw_pipe_cfg isntances, while the layout is common to both >>>> of them. >>>> >>> >>> Sorry for not responding to this comment earlier. >>> >>> https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370 >>> >>> From the perspective of wide planes you are right that the layout is >>> common but not true from smart DMA point of view. >>> >>> For wide planes, yes, its usually the same buffer with just the src_x >>> being different but conceptually and even HW wise each rectangle of the >>> smart DMA is capable of fetching from a different buffer. >>> >>> From the pov, this decision of not having the dpu_hw_fmt_layout as >>> part >>> of dpu_hw_pipe_cfg seems incorrect to me. >> >> Yes, each rectangle/pipe can fetch from a different buffer. However in >> our use case the layout is not defined for each pipe. It is defined >> for a plane, no matter how many pipes are used for the plane, since >> the buffer is also defined per plane. >> > Even if the layout is defined per plane. > > So lets say > > plane A with layout A maps to rect 1 of DMA0 > plane B with layout B maps to rect 2 of DMA0 > > How can layout be assumed to be duplicated in this case? > > This is not a wide plane use-case but just smartDMA case of two > different layers. > > Maybe I am missing something but this is the example i am interested about. PlaneA has layoutA. So dpu_plane_sspp_update_pipe() will program layoutA using (DMA0, rect1)->setup_sourceaddress(layoutA). PlaneB has layoutB, so (DMA0, rect2)->setup_sourceaddress(layoutB). Maybe the commit message is misleading. The layout is not common to rect1 and rect2. It is common to all pipes/rectangles driving a single plane. > >>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 >>>> ++++++++++----------- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 ++--- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++---- >>>> 3 files changed, 22 insertions(+), 24 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>> index 2bd39c13d54d..400d043f37fa 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>> @@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct >>>> dpu_sw_pipe *pipe, >>>> } >>>> >>>> static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe >>>> *pipe, >>>> - struct dpu_hw_pipe_cfg *cfg) >>>> + struct dpu_hw_fmt_layout *layout) >>>> { >>>> struct dpu_hw_sspp *ctx = pipe->sspp; >>>> u32 ystride0, ystride1; >>>> @@ -497,41 +497,41 @@ static void >>>> dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, >>>> return; >>>> >>>> if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { >>>> - for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++) >>>> + for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++) >>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx >>>> + i * 0x4, >>>> - cfg->layout.plane_addr[i]); >>>> + layout->plane_addr[i]); >>>> } else if (pipe->multirect_index == DPU_SSPP_RECT_0) { >>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx, >>>> - cfg->layout.plane_addr[0]); >>>> + layout->plane_addr[0]); >>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx, >>>> - cfg->layout.plane_addr[2]); >>>> + layout->plane_addr[2]); >>>> } else { >>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx, >>>> - cfg->layout.plane_addr[0]); >>>> + layout->plane_addr[0]); >>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx, >>>> - cfg->layout.plane_addr[2]); >>>> + layout->plane_addr[2]); >>>> } >>>> >>>> if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { >>>> - ystride0 = (cfg->layout.plane_pitch[0]) | >>>> - (cfg->layout.plane_pitch[1] << 16); >>>> - ystride1 = (cfg->layout.plane_pitch[2]) | >>>> - (cfg->layout.plane_pitch[3] << 16); >>>> + ystride0 = (layout->plane_pitch[0]) | >>>> + (layout->plane_pitch[1] << 16); >>>> + ystride1 = (layout->plane_pitch[2]) | >>>> + (layout->plane_pitch[3] << 16); >>>> } else { >>>> ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 + >>>> idx); >>>> ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 + >>>> idx); >>>> >>>> if (pipe->multirect_index == DPU_SSPP_RECT_0) { >>>> ystride0 = (ystride0 & 0xFFFF0000) | >>>> - (cfg->layout.plane_pitch[0] & >>>> 0x0000FFFF); >>>> + (layout->plane_pitch[0] & 0x0000FFFF); >>>> ystride1 = (ystride1 & 0xFFFF0000)| >>>> - (cfg->layout.plane_pitch[2] & >>>> 0x0000FFFF); >>>> + (layout->plane_pitch[2] & 0x0000FFFF); >>>> } else { >>>> ystride0 = (ystride0 & 0x0000FFFF) | >>>> - ((cfg->layout.plane_pitch[0] << 16) & >>>> + ((layout->plane_pitch[0] << 16) & >>>> 0xFFFF0000); >>>> ystride1 = (ystride1 & 0x0000FFFF) | >>>> - ((cfg->layout.plane_pitch[2] << 16) & >>>> + ((layout->plane_pitch[2] << 16) & >>>> 0xFFFF0000); >>>> } >>>> } >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>> index c713343378aa..8dad52eb2a90 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>> @@ -154,13 +154,11 @@ struct dpu_hw_pixel_ext { >>>> >>>> /** >>>> * struct dpu_hw_pipe_cfg : Pipe description >>>> - * @layout: format layout information for programming buffer to >>>> hardware >>>> * @src_rect: src ROI, caller takes into account the different >>>> operations >>>> * such as decimation, flip etc to program this field >>>> * @dest_rect: destination ROI. >>>> */ >>>> struct dpu_hw_pipe_cfg { >>>> - struct dpu_hw_fmt_layout layout; >>>> struct drm_rect src_rect; >>>> struct drm_rect dst_rect; >>>> }; >>>> @@ -243,10 +241,10 @@ struct dpu_hw_sspp_ops { >>>> /** >>>> * setup_sourceaddress - setup pipe source addresses >>>> * @pipe: Pointer to software pipe context >>>> - * @cfg: Pointer to pipe config structure >>>> + * @layout: format layout information for programming buffer >>>> to hardware >>>> */ >>>> void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx, >>>> - struct dpu_hw_pipe_cfg *cfg); >>>> + struct dpu_hw_fmt_layout *layout); >>>> >>>> /** >>>> * setup_csc - setup color space coversion >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> index cbff4dea8662..0d2a7170e0ab 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>> @@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct >>>> drm_plane *plane) >>>> >>>> static void _dpu_plane_set_scanout(struct drm_plane *plane, >>>> struct dpu_plane_state *pstate, >>>> - struct dpu_hw_pipe_cfg *pipe_cfg, >>>> struct drm_framebuffer *fb) >>>> { >>>> struct dpu_plane *pdpu = to_dpu_plane(plane); >>>> 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; >>>> int ret; >>>> >>>> - ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); >>>> + ret = dpu_format_populate_layout(aspace, fb, &layout); >>>> if (ret) >>>> DPU_ERROR_PLANE(pdpu, "failed to get format layout, >>>> %d\n", ret); >>>> else if (pstate->pipe.sspp->ops.setup_sourceaddress) { >>>> trace_dpu_plane_set_scanout(&pstate->pipe, >>>> - &pipe_cfg->layout); >>>> - >>>> pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, pipe_cfg); >>>> + &layout); >>>> + >>>> pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, &layout); >>>> } >>>> } >>>> >>>> @@ -1134,7 +1134,7 @@ static void >>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane) >>>> >>>> memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); >>>> >>>> - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb); >>>> + _dpu_plane_set_scanout(plane, pstate, fb); >>>> >>>> pstate->pending = true; >>>> >> >> >>
On 2/2/2023 12:10 PM, Dmitry Baryshkov wrote: > On 02/02/2023 21:54, Abhinav Kumar wrote: >> >> >> On 2/2/2023 11:45 AM, Dmitry Baryshkov wrote: >>> On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar >>> <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>>>> Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, leaving >>>>> only src_rect and dst_rect. This way right and left pipes will have >>>>> separate dpu_hw_pipe_cfg isntances, while the layout is common to both >>>>> of them. >>>>> >>>> >>>> Sorry for not responding to this comment earlier. >>>> >>>> https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370 >>>> >>>> >>>> From the perspective of wide planes you are right that the layout is >>>> common but not true from smart DMA point of view. >>>> >>>> For wide planes, yes, its usually the same buffer with just the src_x >>>> being different but conceptually and even HW wise each rectangle of the >>>> smart DMA is capable of fetching from a different buffer. >>>> >>>> From the pov, this decision of not having the dpu_hw_fmt_layout as >>>> part >>>> of dpu_hw_pipe_cfg seems incorrect to me. >>> >>> Yes, each rectangle/pipe can fetch from a different buffer. However in >>> our use case the layout is not defined for each pipe. It is defined >>> for a plane, no matter how many pipes are used for the plane, since >>> the buffer is also defined per plane. >>> >> Even if the layout is defined per plane. >> >> So lets say >> >> plane A with layout A maps to rect 1 of DMA0 >> plane B with layout B maps to rect 2 of DMA0 >> >> How can layout be assumed to be duplicated in this case? >> >> This is not a wide plane use-case but just smartDMA case of two >> different layers. >> >> Maybe I am missing something but this is the example i am interested >> about. > > PlaneA has layoutA. So dpu_plane_sspp_update_pipe() will program layoutA > using (DMA0, rect1)->setup_sourceaddress(layoutA). > > PlaneB has layoutB, so (DMA0, rect2)->setup_sourceaddress(layoutB). > > Maybe the commit message is misleading. The layout is not common to > rect1 and rect2. It is common to all pipes/rectangles driving a single > plane. > Ack, Its clear now. Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> >>>> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>>> --- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 >>>>> ++++++++++----------- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 ++--- >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++---- >>>>> 3 files changed, 22 insertions(+), 24 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> index 2bd39c13d54d..400d043f37fa 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c >>>>> @@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct >>>>> dpu_sw_pipe *pipe, >>>>> } >>>>> >>>>> static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe >>>>> *pipe, >>>>> - struct dpu_hw_pipe_cfg *cfg) >>>>> + struct dpu_hw_fmt_layout *layout) >>>>> { >>>>> struct dpu_hw_sspp *ctx = pipe->sspp; >>>>> u32 ystride0, ystride1; >>>>> @@ -497,41 +497,41 @@ static void >>>>> dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, >>>>> return; >>>>> >>>>> if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { >>>>> - for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++) >>>>> + for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++) >>>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx >>>>> + i * 0x4, >>>>> - cfg->layout.plane_addr[i]); >>>>> + layout->plane_addr[i]); >>>>> } else if (pipe->multirect_index == DPU_SSPP_RECT_0) { >>>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx, >>>>> - cfg->layout.plane_addr[0]); >>>>> + layout->plane_addr[0]); >>>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx, >>>>> - cfg->layout.plane_addr[2]); >>>>> + layout->plane_addr[2]); >>>>> } else { >>>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx, >>>>> - cfg->layout.plane_addr[0]); >>>>> + layout->plane_addr[0]); >>>>> DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx, >>>>> - cfg->layout.plane_addr[2]); >>>>> + layout->plane_addr[2]); >>>>> } >>>>> >>>>> if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { >>>>> - ystride0 = (cfg->layout.plane_pitch[0]) | >>>>> - (cfg->layout.plane_pitch[1] << 16); >>>>> - ystride1 = (cfg->layout.plane_pitch[2]) | >>>>> - (cfg->layout.plane_pitch[3] << 16); >>>>> + ystride0 = (layout->plane_pitch[0]) | >>>>> + (layout->plane_pitch[1] << 16); >>>>> + ystride1 = (layout->plane_pitch[2]) | >>>>> + (layout->plane_pitch[3] << 16); >>>>> } else { >>>>> ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 >>>>> + idx); >>>>> ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 >>>>> + idx); >>>>> >>>>> if (pipe->multirect_index == DPU_SSPP_RECT_0) { >>>>> ystride0 = (ystride0 & 0xFFFF0000) | >>>>> - (cfg->layout.plane_pitch[0] & >>>>> 0x0000FFFF); >>>>> + (layout->plane_pitch[0] & 0x0000FFFF); >>>>> ystride1 = (ystride1 & 0xFFFF0000)| >>>>> - (cfg->layout.plane_pitch[2] & >>>>> 0x0000FFFF); >>>>> + (layout->plane_pitch[2] & 0x0000FFFF); >>>>> } else { >>>>> ystride0 = (ystride0 & 0x0000FFFF) | >>>>> - ((cfg->layout.plane_pitch[0] << 16) & >>>>> + ((layout->plane_pitch[0] << 16) & >>>>> 0xFFFF0000); >>>>> ystride1 = (ystride1 & 0x0000FFFF) | >>>>> - ((cfg->layout.plane_pitch[2] << 16) & >>>>> + ((layout->plane_pitch[2] << 16) & >>>>> 0xFFFF0000); >>>>> } >>>>> } >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>>> index c713343378aa..8dad52eb2a90 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h >>>>> @@ -154,13 +154,11 @@ struct dpu_hw_pixel_ext { >>>>> >>>>> /** >>>>> * struct dpu_hw_pipe_cfg : Pipe description >>>>> - * @layout: format layout information for programming buffer to >>>>> hardware >>>>> * @src_rect: src ROI, caller takes into account the different >>>>> operations >>>>> * such as decimation, flip etc to program this field >>>>> * @dest_rect: destination ROI. >>>>> */ >>>>> struct dpu_hw_pipe_cfg { >>>>> - struct dpu_hw_fmt_layout layout; >>>>> struct drm_rect src_rect; >>>>> struct drm_rect dst_rect; >>>>> }; >>>>> @@ -243,10 +241,10 @@ struct dpu_hw_sspp_ops { >>>>> /** >>>>> * setup_sourceaddress - setup pipe source addresses >>>>> * @pipe: Pointer to software pipe context >>>>> - * @cfg: Pointer to pipe config structure >>>>> + * @layout: format layout information for programming buffer >>>>> to hardware >>>>> */ >>>>> void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx, >>>>> - struct dpu_hw_pipe_cfg *cfg); >>>>> + struct dpu_hw_fmt_layout *layout); >>>>> >>>>> /** >>>>> * setup_csc - setup color space coversion >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> index cbff4dea8662..0d2a7170e0ab 100644 >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>>>> @@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct >>>>> drm_plane *plane) >>>>> >>>>> static void _dpu_plane_set_scanout(struct drm_plane *plane, >>>>> struct dpu_plane_state *pstate, >>>>> - struct dpu_hw_pipe_cfg *pipe_cfg, >>>>> struct drm_framebuffer *fb) >>>>> { >>>>> struct dpu_plane *pdpu = to_dpu_plane(plane); >>>>> 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; >>>>> int ret; >>>>> >>>>> - ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); >>>>> + ret = dpu_format_populate_layout(aspace, fb, &layout); >>>>> if (ret) >>>>> DPU_ERROR_PLANE(pdpu, "failed to get format layout, >>>>> %d\n", ret); >>>>> else if (pstate->pipe.sspp->ops.setup_sourceaddress) { >>>>> trace_dpu_plane_set_scanout(&pstate->pipe, >>>>> - &pipe_cfg->layout); >>>>> - pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, pipe_cfg); >>>>> + &layout); >>>>> + pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, &layout); >>>>> } >>>>> } >>>>> >>>>> @@ -1134,7 +1134,7 @@ static void >>>>> dpu_plane_sspp_atomic_update(struct drm_plane *plane) >>>>> >>>>> memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); >>>>> >>>>> - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb); >>>>> + _dpu_plane_set_scanout(plane, pstate, fb); >>>>> >>>>> pstate->pending = true; >>>>> >>> >>> >>> >
On 02/02/2023 22:14, Abhinav Kumar wrote: > > > On 2/2/2023 12:10 PM, Dmitry Baryshkov wrote: >> On 02/02/2023 21:54, Abhinav Kumar wrote: >>> >>> >>> On 2/2/2023 11:45 AM, Dmitry Baryshkov wrote: >>>> On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar >>>> <quic_abhinavk@quicinc.com> wrote: >>>>> >>>>> >>>>> >>>>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>>>>> Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, >>>>>> leaving >>>>>> only src_rect and dst_rect. This way right and left pipes will have >>>>>> separate dpu_hw_pipe_cfg isntances, while the layout is common to >>>>>> both >>>>>> of them. >>>>>> >>>>> >>>>> Sorry for not responding to this comment earlier. >>>>> >>>>> https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370 >>>>> >>>>> From the perspective of wide planes you are right that the layout is >>>>> common but not true from smart DMA point of view. >>>>> >>>>> For wide planes, yes, its usually the same buffer with just the src_x >>>>> being different but conceptually and even HW wise each rectangle of >>>>> the >>>>> smart DMA is capable of fetching from a different buffer. >>>>> >>>>> From the pov, this decision of not having the dpu_hw_fmt_layout >>>>> as part >>>>> of dpu_hw_pipe_cfg seems incorrect to me. >>>> >>>> Yes, each rectangle/pipe can fetch from a different buffer. However in >>>> our use case the layout is not defined for each pipe. It is defined >>>> for a plane, no matter how many pipes are used for the plane, since >>>> the buffer is also defined per plane. >>>> >>> Even if the layout is defined per plane. >>> >>> So lets say >>> >>> plane A with layout A maps to rect 1 of DMA0 >>> plane B with layout B maps to rect 2 of DMA0 >>> >>> How can layout be assumed to be duplicated in this case? >>> >>> This is not a wide plane use-case but just smartDMA case of two >>> different layers. >>> >>> Maybe I am missing something but this is the example i am interested >>> about. >> >> PlaneA has layoutA. So dpu_plane_sspp_update_pipe() will program >> layoutA using (DMA0, rect1)->setup_sourceaddress(layoutA). >> >> PlaneB has layoutB, so (DMA0, rect2)->setup_sourceaddress(layoutB). >> >> Maybe the commit message is misleading. The layout is not common to >> rect1 and rect2. It is common to all pipes/rectangles driving a single >> plane. >> > > Ack, Its clear now. > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> I have rephrased the last sentence of the commit message in the following way. Hopefully it will be cleaner now: This way all the pipes used by the plane will have a common layout instance (as the framebuffer is shared between them), while still keeping a separate src/dst rectangle configuration for each pipe.
On 2/3/2023 6:09 AM, Dmitry Baryshkov wrote: > On 02/02/2023 22:14, Abhinav Kumar wrote: >> >> >> On 2/2/2023 12:10 PM, Dmitry Baryshkov wrote: >>> On 02/02/2023 21:54, Abhinav Kumar wrote: >>>> >>>> >>>> On 2/2/2023 11:45 AM, Dmitry Baryshkov wrote: >>>>> On Thu, 2 Feb 2023 at 21:38, Abhinav Kumar >>>>> <quic_abhinavk@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote: >>>>>>> Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, >>>>>>> leaving >>>>>>> only src_rect and dst_rect. This way right and left pipes will have >>>>>>> separate dpu_hw_pipe_cfg isntances, while the layout is common to >>>>>>> both >>>>>>> of them. >>>>>>> >>>>>> >>>>>> Sorry for not responding to this comment earlier. >>>>>> >>>>>> https://patchwork.freedesktop.org/patch/473168/?series=99909&rev=1#comment_875370 >>>>>> >>>>>> >>>>>> From the perspective of wide planes you are right that the >>>>>> layout is >>>>>> common but not true from smart DMA point of view. >>>>>> >>>>>> For wide planes, yes, its usually the same buffer with just the src_x >>>>>> being different but conceptually and even HW wise each rectangle >>>>>> of the >>>>>> smart DMA is capable of fetching from a different buffer. >>>>>> >>>>>> From the pov, this decision of not having the dpu_hw_fmt_layout >>>>>> as part >>>>>> of dpu_hw_pipe_cfg seems incorrect to me. >>>>> >>>>> Yes, each rectangle/pipe can fetch from a different buffer. However in >>>>> our use case the layout is not defined for each pipe. It is defined >>>>> for a plane, no matter how many pipes are used for the plane, since >>>>> the buffer is also defined per plane. >>>>> >>>> Even if the layout is defined per plane. >>>> >>>> So lets say >>>> >>>> plane A with layout A maps to rect 1 of DMA0 >>>> plane B with layout B maps to rect 2 of DMA0 >>>> >>>> How can layout be assumed to be duplicated in this case? >>>> >>>> This is not a wide plane use-case but just smartDMA case of two >>>> different layers. >>>> >>>> Maybe I am missing something but this is the example i am interested >>>> about. >>> >>> PlaneA has layoutA. So dpu_plane_sspp_update_pipe() will program >>> layoutA using (DMA0, rect1)->setup_sourceaddress(layoutA). >>> >>> PlaneB has layoutB, so (DMA0, rect2)->setup_sourceaddress(layoutB). >>> >>> Maybe the commit message is misleading. The layout is not common to >>> rect1 and rect2. It is common to all pipes/rectangles driving a >>> single plane. >>> >> >> Ack, Its clear now. >> >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > I have rephrased the last sentence of the commit message in the > following way. Hopefully it will be cleaner now: > > This way all the pipes used by the plane > will have a common layout instance (as the framebuffer is shared between > them), while still keeping a separate src/dst rectangle configuration > for each pipe. > Ack, thanks.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c index 2bd39c13d54d..400d043f37fa 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c @@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe, } static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, - struct dpu_hw_pipe_cfg *cfg) + struct dpu_hw_fmt_layout *layout) { struct dpu_hw_sspp *ctx = pipe->sspp; u32 ystride0, ystride1; @@ -497,41 +497,41 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe, return; if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { - for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++) + for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++) DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx + i * 0x4, - cfg->layout.plane_addr[i]); + layout->plane_addr[i]); } else if (pipe->multirect_index == DPU_SSPP_RECT_0) { DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx, - cfg->layout.plane_addr[0]); + layout->plane_addr[0]); DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx, - cfg->layout.plane_addr[2]); + layout->plane_addr[2]); } else { DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx, - cfg->layout.plane_addr[0]); + layout->plane_addr[0]); DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx, - cfg->layout.plane_addr[2]); + layout->plane_addr[2]); } if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) { - ystride0 = (cfg->layout.plane_pitch[0]) | - (cfg->layout.plane_pitch[1] << 16); - ystride1 = (cfg->layout.plane_pitch[2]) | - (cfg->layout.plane_pitch[3] << 16); + ystride0 = (layout->plane_pitch[0]) | + (layout->plane_pitch[1] << 16); + ystride1 = (layout->plane_pitch[2]) | + (layout->plane_pitch[3] << 16); } else { ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx); ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx); if (pipe->multirect_index == DPU_SSPP_RECT_0) { ystride0 = (ystride0 & 0xFFFF0000) | - (cfg->layout.plane_pitch[0] & 0x0000FFFF); + (layout->plane_pitch[0] & 0x0000FFFF); ystride1 = (ystride1 & 0xFFFF0000)| - (cfg->layout.plane_pitch[2] & 0x0000FFFF); + (layout->plane_pitch[2] & 0x0000FFFF); } else { ystride0 = (ystride0 & 0x0000FFFF) | - ((cfg->layout.plane_pitch[0] << 16) & + ((layout->plane_pitch[0] << 16) & 0xFFFF0000); ystride1 = (ystride1 & 0x0000FFFF) | - ((cfg->layout.plane_pitch[2] << 16) & + ((layout->plane_pitch[2] << 16) & 0xFFFF0000); } } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h index c713343378aa..8dad52eb2a90 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h @@ -154,13 +154,11 @@ struct dpu_hw_pixel_ext { /** * struct dpu_hw_pipe_cfg : Pipe description - * @layout: format layout information for programming buffer to hardware * @src_rect: src ROI, caller takes into account the different operations * such as decimation, flip etc to program this field * @dest_rect: destination ROI. */ struct dpu_hw_pipe_cfg { - struct dpu_hw_fmt_layout layout; struct drm_rect src_rect; struct drm_rect dst_rect; }; @@ -243,10 +241,10 @@ struct dpu_hw_sspp_ops { /** * setup_sourceaddress - setup pipe source addresses * @pipe: Pointer to software pipe context - * @cfg: Pointer to pipe config structure + * @layout: format layout information for programming buffer to hardware */ void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx, - struct dpu_hw_pipe_cfg *cfg); + struct dpu_hw_fmt_layout *layout); /** * setup_csc - setup color space coversion diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index cbff4dea8662..0d2a7170e0ab 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane) static void _dpu_plane_set_scanout(struct drm_plane *plane, struct dpu_plane_state *pstate, - struct dpu_hw_pipe_cfg *pipe_cfg, struct drm_framebuffer *fb) { struct dpu_plane *pdpu = to_dpu_plane(plane); 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; int ret; - ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); + ret = dpu_format_populate_layout(aspace, fb, &layout); if (ret) DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret); else if (pstate->pipe.sspp->ops.setup_sourceaddress) { trace_dpu_plane_set_scanout(&pstate->pipe, - &pipe_cfg->layout); - pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, pipe_cfg); + &layout); + pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, &layout); } } @@ -1134,7 +1134,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane) memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg)); - _dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb); + _dpu_plane_set_scanout(plane, pstate, fb); pstate->pending = true;
Remove dpu_hw_fmt_layout instance from struct dpu_hw_pipe_cfg, leaving only src_rect and dst_rect. This way right and left pipes will have separate dpu_hw_pipe_cfg isntances, while the layout is common to both of them. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 ++++++++++----------- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 6 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++---- 3 files changed, 22 insertions(+), 24 deletions(-)