[Freedreno] [1/2] drm/msm/dpu: fix clocks settings for msm8998 SSPP blocks

[Dmitry Baryshkov]

On Tue, 24 Jan 2023 at 13:12, Marijn Suijten
<marijn.suijten at somainline.org> wrote:
>
> On 2023-01-24 12:19:27, Dmitry Baryshkov wrote:
> > On 24/01/2023 11:59, Marijn Suijten wrote:
> > > On 2023-01-15 14:41:42, Dmitry Baryshkov wrote:
> > >> DMA2 and DMA3 planes on msm8998 should use corresponding DMA2 and DMA3
> > >> clocks rather than CURSOR0/1 clocks (which are used for the CURSOR
> > >> planes). Correct corresponding SSPP declarations.
> > >>
> > >> Fixes: 94391a14fc27 ("drm/msm/dpu1: Add MSM8998 to hw catalog")
> > >> Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno at somainline.org>
> > >> Cc: Jami Kettunen <jami.kettunen at somainline.org>
> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > >> ---
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 ++--
> > >>   1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> 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 0f3da480b066..ad0c55464154 100644
> > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > >> @@ -1180,9 +1180,9 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
> > >>    SSPP_BLK("sspp_9", SSPP_DMA1, 0x26000,  DMA_MSM8998_MASK,
> > >>            sdm845_dma_sblk_1, 5, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA1),
> > >>    SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000,  DMA_CURSOR_MSM8998_MASK,
> > >
> > > Drop the _CURSOR mask here?  And the double space....
> >
> > Ack for the doublespace. By removing _CURSOR we would disallow using
> > these planes as hw cursor planes. This would switch all compositors into
> > sw cursor mode, thus damaging the performance.
>
> Doesn't that require special hardware support, or can any DMA pipe
> support "hw cursor mode/planes", whatever that means?  Sorry for not
> being well versed in this area, I'd expect DMA pipes and CURSOR pipes to
> have a different set of features / capabilities.

Yes, they have different capabilities. but DMA_CURSOR_MSM8998_MASK =
DMA_MSM8998_MASK | BIT(DPU_SSPP_CURSOR). And the DPU_SSPP_CURSOR is
used internally to tell the DRM core that the corresponding plane is
going to be used as a "userspace cursor" plane.

> Commit 07ca1fc0f8a0 ("drm/msm/dpu: enable cursor plane on dpu") leads me
> to believe that it's mostly to let userspace use these DMA pipes for
> cursors (having cursor planes available in uapi) rather than requiring
> any special hardware support (though semantics do seem to change in a
> nontrivial way).

Correct.
DRM/userspace cursor planes = planes which userspace can use to draw
mouse cursor. Legacy compositors and legacy cursor IOCTLs stick to
using them
DPU/MDP5 CURSOR plane (sspp_12/13) = lightweight limited plane without
additional features, targeting HW cursor only, not present since
sdm845
DPU_SSPP_CURSOR = bit which tells DPU core to mark a plane as
'DRM/userspace cursor plane'.

>
> > >> -          sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA, DPU_CLK_CTRL_CURSOR0),
> > >> +          sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA2),
> > >>    SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000,  DMA_CURSOR_MSM8998_MASK,
> > >> -          sdm845_dma_sblk_3, 13, SSPP_TYPE_DMA, DPU_CLK_CTRL_CURSOR1),
> > >> +          sdm845_dma_sblk_3, 13, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA3),
> > >
> > > Yes, msm8998_mdp defines both DMA2/3 and CURSOR0/1 clocks.  R-b after
> > > using DMA_MSM8998_MASK without the DPU_SSPP_CURSOR bit.
> > >
> > > However, my downstream sources still define cursor SSPPs that are
> > > missing here (after all, there's clk-ctrl for these already), at xin ID
> > > 2 and 10 with addresses 0x3500 and 0x37000 downstream (-0x1000 here):
> > >
> > >     SSPP_BLK("sspp_?", SSPP_CURSOR0, 0x34000, DMA_CURSOR_SM8998_MASK,
> > >             cursor sblk?, 2, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR0),
> > >     SSPP_BLK("sspp_?", SSPP_CURSOR1, 0x36000, DMA_CURSOR_SM8998_MASK,
> >
> > I think this should not be the DMA_CURSOR_MSM8998_MASK, but don't bet on
> > my words, I haven't check actual cursor plane capabilities.
>
> As we've seen in [1] (specifically [2]) there are a few more driver/hw
> changes required to properly implement/support DPU_SSPP_CURSOR?
>
> [1]: https://github.com/rawoul/linux/commits/next_20220624-msm8998-hdmi
> [2]; https://github.com/rawoul/linux/commit/7d8d739cfbfa551120868986d5824f7b2b116ac1
>
> - Marijn
>
> > >             cursor sblk?, 10, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR1),
> > >
> > > Or should you/I send that as a separate folloup patch?
> >
> > Ideally one can add these two planes and then switch two mentioned DMA
> > planes to plain DMA_MSM8998_MASK.
> >
> > >
> > > - Marijn
> > >
> > >>   };
> > >>
> > >>   static const struct dpu_sspp_cfg sdm845_sspp[] = {
> >
> > --
> > With best wishes
> > Dmitry
> >



-- 
With best wishes
Dmitry