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

From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	Stephen Boyd <swboyd@chromium.org>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Bjorn Andersson <andersson@kernel.org>,
	<linux-arm-msm@vger.kernel.org>,
	<dri-devel@lists.freedesktop.org>,
	<freedreno@lists.freedesktop.org>
Subject: Re: [PATCH v3 24/27] drm/msm/dpu: rework plane CSC setting
Date: Tue, 7 Feb 2023 12:57:09 -0800	[thread overview]
Message-ID: <d908ff60-f0c9-7c20-da98-3be8db243ab9@quicinc.com> (raw)
In-Reply-To: <CAA8EJpqQJdrvx4KML1pFYwM1ogYYie6Wct+ueJYMBAYb2-k=xQ@mail.gmail.com>

On 2/7/2023 12:44 PM, Dmitry Baryshkov wrote:
> On Tue, 7 Feb 2023 at 22:05, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> 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@linaro.org>
>>> ---
>>>    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)?
> 
> pstate->pipe is not a pointer, there is nothing to check.

Ah my bad. Only pipe.sspp is a pointer which you are already checking. 
In that case,

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

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