[PATCH v2 12/27] drm/msm/dpu: remove dpu_hw_fmt_layout from struct dpu_hw_pipe_cfg
Abhinav Kumar
quic_abhinavk at quicinc.com
Thu Feb 2 20:14:53 UTC 2023
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 at 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 at quicinc.com>
>>
>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at 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;
>>>>>
>>>
>>>
>>>
>
More information about the dri-devel
mailing list