On 1/26/2023 10:05 PM, Dmitry Baryshkov wrote:
On Fri, 27 Jan 2023 at 02:52, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxxx>
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,
