diff mbox series

[v2,11/27] drm/msm/dpu: move stride programming to dpu_hw_sspp_setup_sourceaddress

Message ID 20221229191856.3508092-12-dmitry.baryshkov@linaro.org
State New
Headers show
Series drm/msm/dpu: wide planes support | expand

Commit Message

Dmitry Baryshkov Dec. 29, 2022, 7:18 p.m. UTC
Move stride programming to dpu_hw_sspp_setup_sourceaddress(), so that
dpu_hw_sspp_setup_rects() programs only source and destination
rectangles.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 57 +++++++++++----------
 1 file changed, 29 insertions(+), 28 deletions(-)

Comments

Abhinav Kumar Feb. 2, 2023, 6:41 p.m. UTC | #1
On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
> Move stride programming to dpu_hw_sspp_setup_sourceaddress(), so that
> dpu_hw_sspp_setup_rects() programs only source and destination
> rectangles.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Sorry but once again, I dont see a response to my comment

https://patchwork.freedesktop.org/patch/473166/?series=99909&rev=1#comment_875313

So let me repeat that here:

"This separation is logically correct, but there is another codepath
using this.

_dpu_plane_color_fill() calls pdpu->pipe_hw->ops.setup_rects.

So for solid fill, I presume that stride getting programmed is 0 as
there is no buffer to fetch from.

But with this separation, we will miss re-programming stride and it will
remain at the old value even for solid fil cases?

You might want to add setup_sourceaddress call there? But that wont make
sense either because for solid fill there is nothing to fetch from.

Perhaps, another op for stride programming then?
"

Also, this is the second patch in the series where the previous comments 
were not resolved/responded to.

Hope that this was not just another rebase without looking at the prior 
comments.

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 57 +++++++++++----------
>   1 file changed, 29 insertions(+), 28 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 176cd6dc9a69..2bd39c13d54d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> @@ -447,7 +447,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
>   {
>   	struct dpu_hw_sspp *ctx = pipe->sspp;
>   	struct dpu_hw_blk_reg_map *c;
> -	u32 src_size, src_xy, dst_size, dst_xy, ystride0, ystride1;
> +	u32 src_size, src_xy, dst_size, dst_xy;
>   	u32 src_size_off, src_xy_off, out_size_off, out_xy_off;
>   	u32 idx;
>   
> @@ -478,44 +478,18 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
>   	dst_size = (drm_rect_height(&cfg->dst_rect) << 16) |
>   		drm_rect_width(&cfg->dst_rect);
>   
> -	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);
> -	} else {
> -		ystride0 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE0 + idx);
> -		ystride1 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE1 + idx);
> -
> -		if (pipe->multirect_index == DPU_SSPP_RECT_0) {
> -			ystride0 = (ystride0 & 0xFFFF0000) |
> -				(cfg->layout.plane_pitch[0] & 0x0000FFFF);
> -			ystride1 = (ystride1 & 0xFFFF0000)|
> -				(cfg->layout.plane_pitch[2] & 0x0000FFFF);
> -		} else {
> -			ystride0 = (ystride0 & 0x0000FFFF) |
> -				((cfg->layout.plane_pitch[0] << 16) &
> -				 0xFFFF0000);
> -			ystride1 = (ystride1 & 0x0000FFFF) |
> -				((cfg->layout.plane_pitch[2] << 16) &
> -				 0xFFFF0000);
> -		}
> -	}
> -
>   	/* rectangle register programming */
>   	DPU_REG_WRITE(c, src_size_off + idx, src_size);
>   	DPU_REG_WRITE(c, src_xy_off + idx, src_xy);
>   	DPU_REG_WRITE(c, out_size_off + idx, dst_size);
>   	DPU_REG_WRITE(c, out_xy_off + idx, dst_xy);
> -
> -	DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE0 + idx, ystride0);
> -	DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE1 + idx, ystride1);
>   }
>   
>   static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
>   		struct dpu_hw_pipe_cfg *cfg)
>   {
>   	struct dpu_hw_sspp *ctx = pipe->sspp;
> +	u32 ystride0, ystride1;
>   	int i;
>   	u32 idx;
>   
> @@ -537,6 +511,33 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
>   		DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx,
>   				cfg->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);
> +	} 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);
> +			ystride1 = (ystride1 & 0xFFFF0000)|
> +				(cfg->layout.plane_pitch[2] & 0x0000FFFF);
> +		} else {
> +			ystride0 = (ystride0 & 0x0000FFFF) |
> +				((cfg->layout.plane_pitch[0] << 16) &
> +				 0xFFFF0000);
> +			ystride1 = (ystride1 & 0x0000FFFF) |
> +				((cfg->layout.plane_pitch[2] << 16) &
> +				 0xFFFF0000);
> +		}
> +	}
> +
> +	DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx, ystride0);
> +	DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx, ystride1);
>   }
>   
>   static void dpu_hw_sspp_setup_csc(struct dpu_hw_sspp *ctx,
Dmitry Baryshkov Feb. 2, 2023, 6:55 p.m. UTC | #2
Hi Abhinav,

On Thu, 2 Feb 2023 at 20:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
> > Move stride programming to dpu_hw_sspp_setup_sourceaddress(), so that
> > dpu_hw_sspp_setup_rects() programs only source and destination
> > rectangles.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Sorry but once again, I dont see a response to my comment
>
> https://patchwork.freedesktop.org/patch/473166/?series=99909&rev=1#comment_875313
>
> So let me repeat that here:
>
> "This separation is logically correct, but there is another codepath
> using this.
>
> _dpu_plane_color_fill() calls pdpu->pipe_hw->ops.setup_rects.
>
> So for solid fill, I presume that stride getting programmed is 0 as
> there is no buffer to fetch from.

Could you please verify with the HW team what should be the correct
stride programming for the solid fill? I'll have to check what is
being programmed ATM.

>
> But with this separation, we will miss re-programming stride and it will
> remain at the old value even for solid fil cases?
>
> You might want to add setup_sourceaddress call there? But that wont make
> sense either because for solid fill there is nothing to fetch from.
>
> Perhaps, another op for stride programming then?
> "
>
> Also, this is the second patch in the series where the previous comments
> were not resolved/responded to.
>
> Hope that this was not just another rebase without looking at the prior
> comments.

I might have missed some of the comments during the rebase, please
excuse me. There was no intent to ignore them.


>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 57 +++++++++++----------
> >   1 file changed, 29 insertions(+), 28 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 176cd6dc9a69..2bd39c13d54d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
> > @@ -447,7 +447,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
> >   {
> >       struct dpu_hw_sspp *ctx = pipe->sspp;
> >       struct dpu_hw_blk_reg_map *c;
> > -     u32 src_size, src_xy, dst_size, dst_xy, ystride0, ystride1;
> > +     u32 src_size, src_xy, dst_size, dst_xy;
> >       u32 src_size_off, src_xy_off, out_size_off, out_xy_off;
> >       u32 idx;
> >
> > @@ -478,44 +478,18 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
> >       dst_size = (drm_rect_height(&cfg->dst_rect) << 16) |
> >               drm_rect_width(&cfg->dst_rect);
> >
> > -     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);
> > -     } else {
> > -             ystride0 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE0 + idx);
> > -             ystride1 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE1 + idx);
> > -
> > -             if (pipe->multirect_index == DPU_SSPP_RECT_0) {
> > -                     ystride0 = (ystride0 & 0xFFFF0000) |
> > -                             (cfg->layout.plane_pitch[0] & 0x0000FFFF);
> > -                     ystride1 = (ystride1 & 0xFFFF0000)|
> > -                             (cfg->layout.plane_pitch[2] & 0x0000FFFF);
> > -             } else {
> > -                     ystride0 = (ystride0 & 0x0000FFFF) |
> > -                             ((cfg->layout.plane_pitch[0] << 16) &
> > -                              0xFFFF0000);
> > -                     ystride1 = (ystride1 & 0x0000FFFF) |
> > -                             ((cfg->layout.plane_pitch[2] << 16) &
> > -                              0xFFFF0000);
> > -             }
> > -     }
> > -
> >       /* rectangle register programming */
> >       DPU_REG_WRITE(c, src_size_off + idx, src_size);
> >       DPU_REG_WRITE(c, src_xy_off + idx, src_xy);
> >       DPU_REG_WRITE(c, out_size_off + idx, dst_size);
> >       DPU_REG_WRITE(c, out_xy_off + idx, dst_xy);
> > -
> > -     DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE0 + idx, ystride0);
> > -     DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE1 + idx, ystride1);
> >   }
> >
> >   static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
> >               struct dpu_hw_pipe_cfg *cfg)
> >   {
> >       struct dpu_hw_sspp *ctx = pipe->sspp;
> > +     u32 ystride0, ystride1;
> >       int i;
> >       u32 idx;
> >
> > @@ -537,6 +511,33 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
> >               DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx,
> >                               cfg->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);
> > +     } 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);
> > +                     ystride1 = (ystride1 & 0xFFFF0000)|
> > +                             (cfg->layout.plane_pitch[2] & 0x0000FFFF);
> > +             } else {
> > +                     ystride0 = (ystride0 & 0x0000FFFF) |
> > +                             ((cfg->layout.plane_pitch[0] << 16) &
> > +                              0xFFFF0000);
> > +                     ystride1 = (ystride1 & 0x0000FFFF) |
> > +                             ((cfg->layout.plane_pitch[2] << 16) &
> > +                              0xFFFF0000);
> > +             }
> > +     }
> > +
> > +     DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx, ystride0);
> > +     DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx, ystride1);
> >   }
> >
> >   static void dpu_hw_sspp_setup_csc(struct dpu_hw_sspp *ctx,
Abhinav Kumar Feb. 2, 2023, 7:15 p.m. UTC | #3
On 2/2/2023 10:55 AM, Dmitry Baryshkov wrote:
> Hi Abhinav,
> 
> On Thu, 2 Feb 2023 at 20:41, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
>>> Move stride programming to dpu_hw_sspp_setup_sourceaddress(), so that
>>> dpu_hw_sspp_setup_rects() programs only source and destination
>>> rectangles.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Sorry but once again, I dont see a response to my comment
>>
>> https://patchwork.freedesktop.org/patch/473166/?series=99909&rev=1#comment_875313
>>
>> So let me repeat that here:
>>
>> "This separation is logically correct, but there is another codepath
>> using this.
>>
>> _dpu_plane_color_fill() calls pdpu->pipe_hw->ops.setup_rects.
>>
>> So for solid fill, I presume that stride getting programmed is 0 as
>> there is no buffer to fetch from.
> 
> Could you please verify with the HW team what should be the correct
> stride programming for the solid fill? I'll have to check what is
> being programmed ATM.
> 

Sure, I can check but in the _dpu_plane_color_fill() method the 
pipe_cfg->layout is not filled up so it should be a 0 stride.

>>
>> But with this separation, we will miss re-programming stride and it will
>> remain at the old value even for solid fil cases?
>>
>> You might want to add setup_sourceaddress call there? But that wont make
>> sense either because for solid fill there is nothing to fetch from.
>>
>> Perhaps, another op for stride programming then?
>> "
>>
>> Also, this is the second patch in the series where the previous comments
>> were not resolved/responded to.
>>
>> Hope that this was not just another rebase without looking at the prior
>> comments.
> 
> I might have missed some of the comments during the rebase, please
> excuse me. There was no intent to ignore them.
> 

Ack.

> 
>>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 57 +++++++++++----------
>>>    1 file changed, 29 insertions(+), 28 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 176cd6dc9a69..2bd39c13d54d 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
>>> @@ -447,7 +447,7 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
>>>    {
>>>        struct dpu_hw_sspp *ctx = pipe->sspp;
>>>        struct dpu_hw_blk_reg_map *c;
>>> -     u32 src_size, src_xy, dst_size, dst_xy, ystride0, ystride1;
>>> +     u32 src_size, src_xy, dst_size, dst_xy;
>>>        u32 src_size_off, src_xy_off, out_size_off, out_xy_off;
>>>        u32 idx;
>>>
>>> @@ -478,44 +478,18 @@ static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
>>>        dst_size = (drm_rect_height(&cfg->dst_rect) << 16) |
>>>                drm_rect_width(&cfg->dst_rect);
>>>
>>> -     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);
>>> -     } else {
>>> -             ystride0 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE0 + idx);
>>> -             ystride1 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE1 + idx);
>>> -
>>> -             if (pipe->multirect_index == DPU_SSPP_RECT_0) {
>>> -                     ystride0 = (ystride0 & 0xFFFF0000) |
>>> -                             (cfg->layout.plane_pitch[0] & 0x0000FFFF);
>>> -                     ystride1 = (ystride1 & 0xFFFF0000)|
>>> -                             (cfg->layout.plane_pitch[2] & 0x0000FFFF);
>>> -             } else {
>>> -                     ystride0 = (ystride0 & 0x0000FFFF) |
>>> -                             ((cfg->layout.plane_pitch[0] << 16) &
>>> -                              0xFFFF0000);
>>> -                     ystride1 = (ystride1 & 0x0000FFFF) |
>>> -                             ((cfg->layout.plane_pitch[2] << 16) &
>>> -                              0xFFFF0000);
>>> -             }
>>> -     }
>>> -
>>>        /* rectangle register programming */
>>>        DPU_REG_WRITE(c, src_size_off + idx, src_size);
>>>        DPU_REG_WRITE(c, src_xy_off + idx, src_xy);
>>>        DPU_REG_WRITE(c, out_size_off + idx, dst_size);
>>>        DPU_REG_WRITE(c, out_xy_off + idx, dst_xy);
>>> -
>>> -     DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE0 + idx, ystride0);
>>> -     DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE1 + idx, ystride1);
>>>    }
>>>
>>>    static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
>>>                struct dpu_hw_pipe_cfg *cfg)
>>>    {
>>>        struct dpu_hw_sspp *ctx = pipe->sspp;
>>> +     u32 ystride0, ystride1;
>>>        int i;
>>>        u32 idx;
>>>
>>> @@ -537,6 +511,33 @@ static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
>>>                DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx,
>>>                                cfg->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);
>>> +     } 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);
>>> +                     ystride1 = (ystride1 & 0xFFFF0000)|
>>> +                             (cfg->layout.plane_pitch[2] & 0x0000FFFF);
>>> +             } else {
>>> +                     ystride0 = (ystride0 & 0x0000FFFF) |
>>> +                             ((cfg->layout.plane_pitch[0] << 16) &
>>> +                              0xFFFF0000);
>>> +                     ystride1 = (ystride1 & 0x0000FFFF) |
>>> +                             ((cfg->layout.plane_pitch[2] << 16) &
>>> +                              0xFFFF0000);
>>> +             }
>>> +     }
>>> +
>>> +     DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx, ystride0);
>>> +     DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx, ystride1);
>>>    }
>>>
>>>    static void dpu_hw_sspp_setup_csc(struct dpu_hw_sspp *ctx,
> 
> 
>
Dmitry Baryshkov Feb. 3, 2023, 2:12 p.m. UTC | #4
On 02/02/2023 21:15, Abhinav Kumar wrote:
> 
> 
> On 2/2/2023 10:55 AM, Dmitry Baryshkov wrote:
>> Hi Abhinav,
>>
>> On Thu, 2 Feb 2023 at 20:41, Abhinav Kumar <quic_abhinavk@quicinc.com> 
>> wrote:
>>>
>>>
>>>
>>> On 12/29/2022 11:18 AM, Dmitry Baryshkov wrote:
>>>> Move stride programming to dpu_hw_sspp_setup_sourceaddress(), so that
>>>> dpu_hw_sspp_setup_rects() programs only source and destination
>>>> rectangles.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> Sorry but once again, I dont see a response to my comment
>>>
>>> https://patchwork.freedesktop.org/patch/473166/?series=99909&rev=1#comment_875313
>>>
>>> So let me repeat that here:
>>>
>>> "This separation is logically correct, but there is another codepath
>>> using this.
>>>
>>> _dpu_plane_color_fill() calls pdpu->pipe_hw->ops.setup_rects.
>>>
>>> So for solid fill, I presume that stride getting programmed is 0 as
>>> there is no buffer to fetch from.
>>
>> Could you please verify with the HW team what should be the correct
>> stride programming for the solid fill? I'll have to check what is
>> being programmed ATM.
>>
> 
> Sure, I can check but in the _dpu_plane_color_fill() method the 
> pipe_cfg->layout is not filled up so it should be a 0 stride.

Actually I think we should call setup_sourceaddress for the color-filled 
planes too. Otherwise the SSPP's adddress registers can point to the 
memory regions which are no longer mapped/available.
diff mbox series

Patch

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 176cd6dc9a69..2bd39c13d54d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -447,7 +447,7 @@  static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
 {
 	struct dpu_hw_sspp *ctx = pipe->sspp;
 	struct dpu_hw_blk_reg_map *c;
-	u32 src_size, src_xy, dst_size, dst_xy, ystride0, ystride1;
+	u32 src_size, src_xy, dst_size, dst_xy;
 	u32 src_size_off, src_xy_off, out_size_off, out_xy_off;
 	u32 idx;
 
@@ -478,44 +478,18 @@  static void dpu_hw_sspp_setup_rects(struct dpu_sw_pipe *pipe,
 	dst_size = (drm_rect_height(&cfg->dst_rect) << 16) |
 		drm_rect_width(&cfg->dst_rect);
 
-	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);
-	} else {
-		ystride0 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE0 + idx);
-		ystride1 = DPU_REG_READ(c, SSPP_SRC_YSTRIDE1 + idx);
-
-		if (pipe->multirect_index == DPU_SSPP_RECT_0) {
-			ystride0 = (ystride0 & 0xFFFF0000) |
-				(cfg->layout.plane_pitch[0] & 0x0000FFFF);
-			ystride1 = (ystride1 & 0xFFFF0000)|
-				(cfg->layout.plane_pitch[2] & 0x0000FFFF);
-		} else {
-			ystride0 = (ystride0 & 0x0000FFFF) |
-				((cfg->layout.plane_pitch[0] << 16) &
-				 0xFFFF0000);
-			ystride1 = (ystride1 & 0x0000FFFF) |
-				((cfg->layout.plane_pitch[2] << 16) &
-				 0xFFFF0000);
-		}
-	}
-
 	/* rectangle register programming */
 	DPU_REG_WRITE(c, src_size_off + idx, src_size);
 	DPU_REG_WRITE(c, src_xy_off + idx, src_xy);
 	DPU_REG_WRITE(c, out_size_off + idx, dst_size);
 	DPU_REG_WRITE(c, out_xy_off + idx, dst_xy);
-
-	DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE0 + idx, ystride0);
-	DPU_REG_WRITE(c, SSPP_SRC_YSTRIDE1 + idx, ystride1);
 }
 
 static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
 		struct dpu_hw_pipe_cfg *cfg)
 {
 	struct dpu_hw_sspp *ctx = pipe->sspp;
+	u32 ystride0, ystride1;
 	int i;
 	u32 idx;
 
@@ -537,6 +511,33 @@  static void dpu_hw_sspp_setup_sourceaddress(struct dpu_sw_pipe *pipe,
 		DPU_REG_WRITE(&ctx->hw, SSPP_SRC3_ADDR + idx,
 				cfg->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);
+	} 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);
+			ystride1 = (ystride1 & 0xFFFF0000)|
+				(cfg->layout.plane_pitch[2] & 0x0000FFFF);
+		} else {
+			ystride0 = (ystride0 & 0x0000FFFF) |
+				((cfg->layout.plane_pitch[0] << 16) &
+				 0xFFFF0000);
+			ystride1 = (ystride1 & 0x0000FFFF) |
+				((cfg->layout.plane_pitch[2] << 16) &
+				 0xFFFF0000);
+		}
+	}
+
+	DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE0 + idx, ystride0);
+	DPU_REG_WRITE(&ctx->hw, SSPP_SRC_YSTRIDE1 + idx, ystride1);
 }
 
 static void dpu_hw_sspp_setup_csc(struct dpu_hw_sspp *ctx,