Re: [PATCH v6 7/7] drm/mediatek: Add mt8195 DisplayPort driver

[Guillaume Ranquet]

Hi Angelo,

Quoting AngeloGioacchino Del Regno (2021-12-10 11:17:44)
> Il 10/11/21 14:06, Guillaume Ranquet ha scritto:
> > From: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
> >
> > This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a
> > according phy driver mediatek-dp-phy.
> >
> > It supports both functional units on the mt8195, the embedded
> > DisplayPort as well as the external DisplayPort units. It offers
> > hot-plug-detection, audio up to 8 channels, and DisplayPort 1.4 with up
> > to 4 lanes.
> >
> > The driver creates a child device for the phy. The child device will
> > never exist without the parent being active. As they are sharing a
> > register range, the parent passes a regmap pointer to the child so that
> > both can work with the same register range. The phy driver sets device
> > data that is read by the parent to get the phy device that can be used
> > to control the phy properties.
> >
> > This driver is based on an initial version by
> > Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>.
> >
> > Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
> > Signed-off-by: Guillaume Ranquet <granquet@xxxxxxxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
>
> Hello Markus, Guillaume,
>
> there is a critical issue with this patch. Please check below.
>
> > ---
> >   drivers/gpu/drm/drm_edid.c              |    2 +-
> >   drivers/gpu/drm/mediatek/Kconfig        |    7 +
> >   drivers/gpu/drm/mediatek/Makefile       |    2 +
> >   drivers/gpu/drm/mediatek/mtk_dp.c       | 3094 +++++++++++++++++++++++
> >   drivers/gpu/drm/mediatek/mtk_dp_reg.h   |  568 +++++
> >   drivers/gpu/drm/mediatek/mtk_dpi.c      |  111 +-
> >   drivers/gpu/drm/mediatek/mtk_dpi_regs.h |   26 +
> >   drivers/gpu/drm/mediatek/mtk_drm_drv.c  |    1 +
> >   drivers/gpu/drm/mediatek/mtk_drm_drv.h  |    1 +
> >   9 files changed, 3799 insertions(+), 13 deletions(-)
> >   create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c
> >   create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h
> >
>
> <snip>
>
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > index 384074f69111b..e6e88e3cd811d 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
>
> <snip>
>
> > @@ -979,6 +1051,16 @@ static int mtk_dpi_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > +     dpi->dpi_ck_cg = devm_clk_get(dev, "ck_cg");
>
> mtk_dpi is used on MT2701, MT7183, MT8183, MT8192, but these platforms haven't
> got any "ck_cg" clock defined in their device-trees (regardless of whether these
> can support adding this clock or not, any code change shall be retro-compatible
> hence not breaking compatibility/functionality with older device-trees).
>
> Reminding that:
> - mediatek-drm uses the component framework
> - mtk_drm_drv is the component master
> - mtk_drm_drv bind() won't be called unless all of the components added with
>    match aren't calling component_add()
>
> ... this change not only breaks DisplayPort support for *all* of the
> aforementioned SoCs, but also makes the entire mediatek-drm to not finish
> probing, producing a global breakage that also includes DSI and the entire
> stack of components of that master (so, no display on all of them).
>
> To avoid breaking any SoC that's not MT8195, please use devm_clk_get_optional()
> here in the next version.
>
> Thanks,
> - Angelo
>

This is a good catch, I will update for v7.

Thank you very much for your review.

Thx,
Guillaume.