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

[Abhinav Kumar]

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 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>
> 
> 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.