Re: [PATCH 3/6] drm/msm/dpu: add support for SM8550

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: neil.armstrong@linaro.org
Cc: Rob Clark <robdclark@gmail.com>,
	Abhinav Kumar <quic_abhinavk@quicinc.com>,
	Sean Paul <sean@poorly.run>, David Airlie <airlied@gmail.com>,
	Daniel Vetter <daniel@ffwll.ch>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Jonathan Marek <jonathan@marek.ca>,
	linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org,
	freedreno@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/6] drm/msm/dpu: add support for SM8550
Date: Wed, 4 Jan 2023 19:48:17 +0200	[thread overview]
Message-ID: <CAA8EJpoi8QFpvR0qWpNpenZKzEZAQpwp3gNpGd3RwHovgC+Odw@mail.gmail.com> (raw)
In-Reply-To: <de3860ba-40f9-cdd5-097c-e015f6b19255@linaro.org>

On Wed, 4 Jan 2023 at 12:08, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 04/01/2023 10:45, Dmitry Baryshkov wrote:
> > On 04/01/2023 11:08, Neil Armstrong wrote:
> >> Add definitions for the display hardware used on Qualcomm SM8550
> >> platform.
> >>
> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 197 +++++++++++++++++++++++++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |   1 +
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h    |   2 +
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        |   1 +
> >>   4 files changed, 201 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> index b4ca123d8e69..adf5e25269dc 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>
> <snip>
>
> >> @@ -776,6 +821,45 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = {
> >>       },
> >>   };
> >> +static const struct dpu_ctl_cfg sm8550_ctl[] = {
> >> +    {
> >> +    .name = "ctl_0", .id = CTL_0,
> >> +    .base = 0x15000, .len = 0x290,?
> >> +    .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_SPLIT_DISPLAY) | BIT(DPU_CTL_FETCH_ACTIVE),
> >
> > CTL_SC7280_MASK | BIT(DPU_CTL_SPLIT_DISPLAY) ?
>
> Indeed DPU_CTL_VM_CFG is missing, will switch to that.
>
> >
> >> +    .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9),
> >> +    },
> >> +    {
> >> +    .name = "ctl_1", .id = CTL_1,
> >> +    .base = 0x16000, .len = 0x290,
> >> +    .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_SPLIT_DISPLAY) | BIT(DPU_CTL_FETCH_ACTIVE),
> >> +    .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 10),
> >> +    },
> >> +    {
> >> +    .name = "ctl_2", .id = CTL_2,
> >> +    .base = 0x17000, .len = 0x290,
> >> +    .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE),
> >
> > CTL_SC7280_MASK?
>
> Ack
>
> >
> >> +    .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 11),
> >> +    },
> >> +    {
> >> +    .name = "ctl_3", .id = CTL_3,
> >> +    .base = 0x18000, .len = 0x290,
> >> +    .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE),
> >> +    .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 12),
> >> +    },
> >> +    {
> >> +    .name = "ctl_4", .id = CTL_4,
> >> +    .base = 0x19000, .len = 0x290,
> >> +    .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE),
> >> +    .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 13),
> >> +    },
> >> +    {
> >> +    .name = "ctl_5", .id = CTL_5,
> >> +    .base = 0x1a000, .len = 0x290,
> >> +    .features = BIT(DPU_CTL_ACTIVE_CFG) | BIT(DPU_CTL_FETCH_ACTIVE),
> >> +    .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 23),
> >> +    },
> >> +};
> >> +
> >>   static const struct dpu_ctl_cfg sc7280_ctl[] = {
> >>       {
> >>       .name = "ctl_0", .id = CTL_0,
>
> <snip>
>
> >> @@ -1268,6 +1386,16 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
> >>       .len = 0x20, .version = 0x20000},
> >>   };
> >> +#define PP_BLK_DIPHER(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
> >> +    {\
> >> +    .name = _name, .id = _id, \
> >> +    .base = _base, .len = 0, \
> >
> > len = 0 looks incorrect. Any particular reason why can't we use plain PP_BLK here?
>
> The TE block has been moved to the DSI INTF blocks since SM8350 I think, or earlier.

I think, 8150. Marijn has been working on adding support for INTF-based TE.

> This removes the DPU_PINGPONG_DITHER feature used downstream to enable the PP TE callbacks.
> Since there's only the DIPHER sub-block remaining, this is why I set len to 0.

I went on with some research. Usually PP len is 0xd4. However it seems
since 8350 (since the change of DSC block) the PP size should be 0x0),
despite dowsnstream DTs having sde-pp-size=0xd4 for sm8350 and sm8450
(or 0x4 for neo, DPU 9.1.0).
So, it looks like you are correct here (and we should fix 8350/8450
patches instead).

>
> >
> >> +    .features = BIT(DPU_PINGPONG_DITHER), \
> >> +    .merge_3d = _merge_3d, \
> >> +    .sblk = &_sblk, \
> >> +    .intr_done = _done, \
> >> +    .intr_rdptr = _rdptr, \
> >> +    }
> >>   #define PP_BLK_TE(_name, _id, _base, _merge_3d, _sblk, _done, _rdptr) \
> >>       {\
> >>       .name = _name, .id = _id, \
>
> <snip>
>

-- 
With best wishes
Dmitry