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

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

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?

Like I wrote in the review last time, this makes sure to fail the commit 
if the same addr is being programmed.

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