[PATCH v2 05/27] drm/msm/dpu: drop EAGAIN check from dpu_format_populate_layout

[Abhinav Kumar]

On 1/26/2023 10:05 PM, Dmitry Baryshkov wrote:
> On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
>>> The pipe's layout is not cached, corresponding data structure is zeroed
>>> out each time in the dpu_plane_sspp_atomic_update(), right before the
>>> call to _dpu_plane_set_scanout() -> dpu_format_populate_layout().
>>>
>>> Drop plane_addr comparison against previous layout and corresponding
>>> EAGAIN handling.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>
>> The change itself LGTM, hence
>>
>> But, shouldnt we add this EAGAIN validation or in other words fix this
>> rather than drop this?
> 
> What for? Does it really save us anything? What's the price of
> re-programming the SSPP_SRC0_ADDR registers?
> 
There are 4 Src registers being programmed per sspp.

With number of layers going up this will be 4x.

So lets say there are 5 layers and only one of their address has 
changed, we need to reprogram only 4 regs but now will reprogram 20.

Thats why i thought this is a good optimization.

But still, that is a separate change so I am fine if this goes in first 
as its just removing dead code anyway.

Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>

>>
>> Like I wrote in the review last time, this makes sure to fail the commit
>> if the same addr is being programmed.
> 
> First, there is nothing wrong with committing the same source addr.
> For example setting the atomic property incurs an internal
> drm_atomic_commit() with no change to addresses at all.
> And then, this doesn't make atomic_commit fail. Instead it just
> shortcuts a call to SSPP->setup_sourceaddress.
> 

Ack, yes it wont fail the commit but will skip programming the new address.

>>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 10 +---------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   |  4 +---
>>>    2 files changed, 2 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
>>> index d95540309d4d..ec1001e10f4f 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
>>> @@ -918,8 +918,7 @@ int dpu_format_populate_layout(
>>>                struct drm_framebuffer *fb,
>>>                struct dpu_hw_fmt_layout *layout)
>>>    {
>>> -     uint32_t plane_addr[DPU_MAX_PLANES];
>>> -     int i, ret;
>>> +     int ret;
>>>
>>>        if (!fb || !layout) {
>>>                DRM_ERROR("invalid arguments\n");
>>> @@ -940,9 +939,6 @@ int dpu_format_populate_layout(
>>>        if (ret)
>>>                return ret;
>>>
>>> -     for (i = 0; i < DPU_MAX_PLANES; ++i)
>>> -             plane_addr[i] = layout->plane_addr[i];
>>> -
>>>        /* Populate the addresses given the fb */
>>>        if (DPU_FORMAT_IS_UBWC(layout->format) ||
>>>                        DPU_FORMAT_IS_TILE(layout->format))
>>> @@ -950,10 +946,6 @@ int dpu_format_populate_layout(
>>>        else
>>>                ret = _dpu_format_populate_addrs_linear(aspace, fb, layout);
>>>
>>> -     /* check if anything changed */
>>> -     if (!ret && !memcmp(plane_addr, layout->plane_addr, sizeof(plane_addr)))
>>> -             ret = -EAGAIN;
>>> -
>>>        return ret;
>>>    }
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> index cdde7b9ec882..43fb8e00ada6 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> @@ -476,9 +476,7 @@ static void _dpu_plane_set_scanout(struct drm_plane *plane,
>>>        int ret;
>>>
>>>        ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout);
>>> -     if (ret == -EAGAIN)
>>> -             DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n");
>>> -     else if (ret)
>>> +     if (ret)
>>>                DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
>>>        else if (pdpu->pipe_hw->ops.setup_sourceaddress) {
>>>                trace_dpu_plane_set_scanout(pdpu->pipe_hw->idx,
> 
> 
>