diff mbox series

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

Message ID 20230203182132.1307834-25-dmitry.baryshkov@linaro.org
State Superseded
Headers show
Series [v3,01/27] drm/msm/dpu: rename struct dpu_hw_pipe(_cfg) to dpu_hw_sspp(_cfg) | expand

Commit Message

Dmitry Baryshkov Feb. 3, 2023, 6:21 p.m. UTC
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(-)

Comments

Abhinav Kumar Feb. 7, 2023, 8:05 p.m. UTC | #1
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)?

> +	else
> +		dpu_plane_flush_csc(pdpu, &pstate->pipe);
>   
>   	/* flag h/w flush complete */
>   	if (plane->state)
Dmitry Baryshkov Feb. 7, 2023, 8:44 p.m. UTC | #2
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.

>
> > +     else
> > +             dpu_plane_flush_csc(pdpu, &pstate->pipe);
> >
> >       /* flag h/w flush complete */
> >       if (plane->state)
Abhinav Kumar Feb. 7, 2023, 8:57 p.m. UTC | #3
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)
> 
> 
>
diff mbox series

Patch

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);
-	}
+	else
+		dpu_plane_flush_csc(pdpu, &pstate->pipe);
 
 	/* flag h/w flush complete */
 	if (plane->state)