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

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 30 ++++++++++-----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h |  6 ++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c   | 10 +++----
>   3 files changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> index 2bd39c13d54d..400d043f37fa 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> @@ -486,7 +486,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
>   }
>   
>   static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
> -		struct dpu_hw_pipe_cfg *cfg)
> +		struct dpu_hw_fmt_layout *layout)
>   {
>   	struct dpu_hw_sspp *ctx = pipe->sspp;
>   	u32 ystride0, ystride1;
> @@ -497,41 +497,41 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
>   		return;
>   
>   	if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) {
> -		for (i = 0; i < ARRAY_SIZE(cfg->layout.plane_addr); i++)
> +		for (i = 0; i < ARRAY_SIZE(layout->plane_addr); i++)
>   			DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx + i * 0x4,
> -					cfg->layout.plane_addr[i]);
> +					layout->plane_addr[i]);
>   	} else if (pipe->multirect_index == DPU_SSPP_RECT_0) {
>   		DPU_REG_WRITE(&ctx->hw, SSPP_SRC0_ADDR + idx,
> -				cfg->layout.plane_addr[0]);
> +				layout->plane_addr[0]);
>   		DPU_REG_WRITE(&ctx->hw, SSPP_SRC2_ADDR + idx,
> -				cfg->layout.plane_addr[2]);
> +				layout->plane_addr[2]);
>   	} else {
>   		DPU_REG_WRITE(&ctx->hw, SSPP_SRC1_ADDR + idx,
> -				cfg->layout.plane_addr[0]);
> +				layout->plane_addr[0]);
>   		DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx,
> -				cfg->layout.plane_addr[2]);
> +				layout->plane_addr[2]);
>   	}
>   
>   	if (pipe->multirect_index == DPU_SSPP_RECT_SOLO) {
> -		ystride0 = (cfg->layout.plane_pitch[0]) |
> -			(cfg->layout.plane_pitch[1] << 16);
> -		ystride1 = (cfg->layout.plane_pitch[2]) |
> -			(cfg->layout.plane_pitch[3] << 16);
> +		ystride0 = (layout->plane_pitch[0]) |
> +			(layout->plane_pitch[1] << 16);
> +		ystride1 = (layout->plane_pitch[2]) |
> +			(layout->plane_pitch[3] << 16);
>   	} else {
>   		ystride0 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx);
>   		ystride1 = DPU_REG_READ(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx);
>   
>   		if (pipe->multirect_index == DPU_SSPP_RECT_0) {
>   			ystride0 = (ystride0 & 0xFFFF0000) |
> -				(cfg->layout.plane_pitch[0] & 0x0000FFFF);
> +				(layout->plane_pitch[0] & 0x0000FFFF);
>   			ystride1 = (ystride1 & 0xFFFF0000)|
> -				(cfg->layout.plane_pitch[2] & 0x0000FFFF);
> +				(layout->plane_pitch[2] & 0x0000FFFF);
>   		} else {
>   			ystride0 = (ystride0 & 0x0000FFFF) |
> -				((cfg->layout.plane_pitch[0] << 16) &
> +				((layout->plane_pitch[0] << 16) &
>   				 0xFFFF0000);
>   			ystride1 = (ystride1 & 0x0000FFFF) |
> -				((cfg->layout.plane_pitch[2] << 16) &
> +				((layout->plane_pitch[2] << 16) &
>   				 0xFFFF0000);
>   		}
>   	}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> index c713343378aa..8dad52eb2a90 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h
> @@ -154,13 +154,11 @@ struct dpu_hw_pixel_ext {
>   
>   /**
>    * struct dpu_hw_pipe_cfg : Pipe description
> - * @layout:    format layout information for programming buffer to hardware
>    * @src_rect:  src ROI, caller takes into account the different operations
>    *             such as decimation, flip etc to program this field
>    * @dest_rect: destination ROI.
>    */
>   struct dpu_hw_pipe_cfg {
> -	struct dpu_hw_fmt_layout layout;
>   	struct drm_rect src_rect;
>   	struct drm_rect dst_rect;
>   };
> @@ -243,10 +241,10 @@ struct dpu_hw_sspp_ops {
>   	/**
>   	 * setup_sourceaddress - setup pipe source addresses
>   	 * @pipe: Pointer to software pipe context
> -	 * @cfg: Pointer to pipe config structure
> +	 * @layout: format layout information for programming buffer to hardware
>   	 */
>   	void (*setup_sourceaddress)(struct dpu_sw_pipe *ctx,
> -				    struct dpu_hw_pipe_cfg *cfg);
> +				    struct dpu_hw_fmt_layout *layout);
>   
>   	/**
>   	 * setup_csc - setup color space coversion
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index cbff4dea8662..0d2a7170e0ab 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -471,21 +471,21 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
>   
>   static void _dpu_plane_set_scanout(struct drm_plane *plane,
>   		struct dpu_plane_state *pstate,
> -		struct dpu_hw_pipe_cfg *pipe_cfg,
>   		struct drm_framebuffer *fb)
>   {
>   	struct dpu_plane *pdpu = to_dpu_plane(plane);
>   	struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
>   	struct msm_gem_address_space *aspace = kms->base.aspace;
> +	struct dpu_hw_fmt_layout layout;
>   	int ret;
>   
> -	ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout);
> +	ret = dpu_format_populate_layout(aspace, fb, &layout);
>   	if (ret)
>   		DPU_ERROR_PLANE(pdpu, "failed to get format layout, %d\n", ret);
>   	else if (pstate->pipe.sspp->ops.setup_sourceaddress) {
>   		trace_dpu_plane_set_scanout(&pstate->pipe,
> -					    &pipe_cfg->layout);
> -		pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, pipe_cfg);
> +					    &layout);
> +		pstate->pipe.sspp->ops.setup_sourceaddress(&pstate->pipe, &layout);
>   	}
>   }
>   
> @@ -1134,7 +1134,7 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane)
>   
>   	memset(&pipe_cfg, 0, sizeof(struct dpu_hw_pipe_cfg));
>   
> -	_dpu_plane_set_scanout(plane, pstate, &pipe_cfg, fb);
> +	_dpu_plane_set_scanout(plane, pstate, fb);
>   
>   	pstate->pending = true;
>