Re: [PATCH v3 24/27] drm/msm/dpu: rework plane CSC setting

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
> Rework the code flushing CSC settings for the plane. Separate out the
> pipe and pipe_cfg as a preparation for r_pipe support.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 45 +++++++++++++----------
>   1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index e69499490d39..05047192cb37 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -576,29 +576,18 @@ static const struct dpu_csc_cfg dpu_csc10_YUV2RGB_601L = {
>   	{ 0x00, 0x3ff, 0x00, 0x3ff, 0x00, 0x3ff,},
>   };
>   
> -static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_plane *pdpu, const struct dpu_format *fmt)
> +static const struct dpu_csc_cfg *_dpu_plane_get_csc(struct dpu_sw_pipe *pipe, const struct dpu_format *fmt)
>   {
> -	struct dpu_plane_state *pstate = to_dpu_plane_state(pdpu->base.state);
>   	const struct dpu_csc_cfg *csc_ptr;
>   
> -	if (!pdpu) {
> -		DPU_ERROR("invalid plane\n");
> -		return NULL;
> -	}
> -
>   	if (!DPU_FORMAT_IS_YUV(fmt))
>   		return NULL;
>   
> -	if (BIT(DPU_SSPP_CSC_10BIT) & pstate->pipe.sspp->cap->features)
> +	if (BIT(DPU_SSPP_CSC_10BIT) & pipe->sspp->cap->features)
>   		csc_ptr = &dpu_csc10_YUV2RGB_601L;
>   	else
>   		csc_ptr = &dpu_csc_YUV2RGB_601L;
>   
> -	DPU_DEBUG_PLANE(pdpu, "using 0x%X 0x%X 0x%X...\n",
> -			csc_ptr->csc_mv[0],
> -			csc_ptr->csc_mv[1],
> -			csc_ptr->csc_mv[2]);
> -
>   	return csc_ptr;
>   }
>   
> @@ -1049,6 +1038,27 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>   	return 0;
>   }
>   
> +static void dpu_plane_flush_csc(struct dpu_plane *pdpu, struct dpu_sw_pipe *pipe)
> +{
> +	const struct dpu_format *format = to_dpu_format(msm_framebuffer_format(pdpu->base.state->fb));
> +	const struct dpu_csc_cfg *csc_ptr;
> +
> +	if (!pipe->sspp || !pipe->sspp->ops.setup_csc)
> +		return;
> +
> +	csc_ptr = _dpu_plane_get_csc(pipe, format);
> +	if (!csc_ptr)
> +		return;
> +
> +	DPU_DEBUG_PLANE(pdpu, "using 0x%X 0x%X 0x%X...\n",
> +			csc_ptr->csc_mv[0],
> +			csc_ptr->csc_mv[1],
> +			csc_ptr->csc_mv[2]);
> +
> +	pipe->sspp->ops.setup_csc(pipe->sspp, csc_ptr);
> +
> +}
> +
>   void dpu_plane_flush(struct drm_plane *plane)
>   {
>   	struct dpu_plane *pdpu;
> @@ -1072,13 +1082,8 @@ void dpu_plane_flush(struct drm_plane *plane)
>   	else if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG)
>   		/* force 100% alpha */
>   		_dpu_plane_color_fill(pdpu, pdpu->color_fill, 0xFF);
> -	else if (pstate->pipe.sspp && pstate->pipe.sspp->ops.setup_csc) {
> -		const struct dpu_format *fmt = to_dpu_format(msm_framebuffer_format(plane->state->fb));
> -		const struct dpu_csc_cfg *csc_ptr = _dpu_plane_get_csc(pdpu, fmt);
> -
> -		if (csc_ptr)
> -			pstate->pipe.sspp->ops.setup_csc(pstate->pipe.sspp, csc_ptr);
> -	}

Do we need to check for pipe being valid too (pstate->pipe) && 
(pstate->pipe.sspp) && pstate->pipe.sspp->ops.setup_csc) ?

Before moving the pipe_hw/sw pipe to the state the code used to check 
for pdpu->pipe_hw to be valid. Since dpu_plane_flush() can be called 
from other files too , dont we need to check for (pstate->pipe)?

> +	else
> +		dpu_plane_flush_csc(pdpu, &pstate->pipe);
>   
>   	/* flag h/w flush complete */
>   	if (plane->state)