Re: [PATCH v2 12/27] drm/msm/dpu: remove dpu_hw_fmt_layout from struct dpu_hw_pipe_cfg

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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.

-- 
With best wishes
Dmitry