Messages in this thread | | | Date | Mon, 19 Feb 2024 11:02:01 +0800 | Subject | Re: [PATCH] drm: rockchip: Don't require MIPI DSI device when it's used for ISP | From | Andy Yan <> |
| |
Hi Ondrej:
On 2/18/24 23:17, Ondřej Jirman wrote: > Hi Andy, > > On Sun, Feb 18, 2024 at 07:14:56PM +0800, Andy Yan wrote: >> Hi, >> >> On 2/18/24 02:39, Ondřej Jirman wrote: >>> From: Ondrej Jirman <megi@xff.cz> >>> >>> On RK3399 one MIPI DSI device can be alternatively used with the ISP1, >>> to provide RX DPHY. When this is the case (ISP1 is enabled in device >>> tree), probe success of DRM is tied to probe success of ISP1 connected >>> camera sensor. This can fail if the user is able to killswitch the camera >>> power, like on Pinephone Pro. >>> >>> Detect use of MIPI DSI controller by ISP, and don't include it in >>> component match list in that case. >>> >> >> Isn't this supposed to be taken care of within the dts? >> Since DPHY1 should exclusively used by MIPI DSI1 and ISP1, then if >> a device want to use ISP1, the DSI1 should be disabled in dts. > > DSI1 can't be disabled, because it provides PHY device for ISP1 in this > scenario. > > The problem is that in this scenario DRM keeps waiting for DSI1 device, > despite it just being used for PHY for ISP1 and not as a component for > rockchip DRM driver.
Oh, get it. With a quick look, seems that Heiko has already take this scenario into consideration when add PHY function in the dsi drivers[0] Does this mean the current drivers does not work as expected?
[0]https://patchwork.freedesktop.org/patch/420386/ > > See: > > isp1: isp1@ff920000 { > compatible = "rockchip,rk3399-cif-isp"; > reg = <0x0 0xff920000 0x0 0x4000>; > interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH 0>; > clocks = <&cru SCLK_ISP1>, > <&cru ACLK_ISP1_WRAPPER>, > <&cru HCLK_ISP1_WRAPPER>; > clock-names = "isp", "aclk", "hclk"; > iommus = <&isp1_mmu>; > phys = <&mipi_dsi1>; <--------- > phy-names = "dphy"; > power-domains = <&power RK3399_PD_ISP1>; > status = "disabled"; > > If mipi_dsi1 is disabled, isp1 will never probe. > > It's a consequence of this special dual use of mipi_dsi1. > > kind regards, > o. > >>> Signed-off-by: Ondrej Jirman <megi@xff.cz> >>> --- >>> drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 47 +++++++++++++++++++++ >>> 1 file changed, 47 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >>> index ab55d7132550..f47de94ad576 100644 >>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >>> @@ -354,6 +354,43 @@ static void rockchip_drm_match_remove(struct device *dev) >>> device_link_del(link); >>> } >>> +/* >>> + * Check if ISP block linked to a mipi-dsi device via phys phandle is >>> + * enabled in device tree. >>> + */ >>> +static bool rockchip_drm_is_mipi1_and_used_by_isp(struct device *dev) >>> +{ >>> + struct device_node *np = NULL, *phy_np; >>> + >>> + if (!of_device_is_compatible(dev->of_node, "rockchip,rk3399-mipi-dsi")) >>> + return false; >>> + >>> + while (true) { >>> + np = of_find_compatible_node(np, NULL, "rockchip,rk3399-cif-isp"); >>> + if (!np) >>> + break; >>> + >>> + if (!of_device_is_available(np)) { >>> + of_node_put(np); >>> + continue; >>> + } >>> + >>> + phy_np = of_parse_phandle(np, "phys", 0); >>> + if (!phy_np) { >>> + of_node_put(np); >>> + continue; >>> + } >>> + >>> + of_node_put(phy_np); >>> + of_node_put(np); >>> + >>> + if (phy_np == dev->of_node) >>> + return true; >>> + } >>> + >>> + return false; >>> +} >>> + >>> static struct component_match *rockchip_drm_match_add(struct device *dev) >>> { >>> struct component_match *match = NULL; >>> @@ -371,6 +408,16 @@ static struct component_match *rockchip_drm_match_add(struct device *dev) >>> if (!d) >>> break; >>> + /* >>> + * If mipi1 is connected to ISP, we don't want to wait for mipi1 component, >>> + * because it will not be used by DRM anyway, to not tie success of camera >>> + * driver probe to display pipeline initialization. >>> + */ >>> + if (rockchip_drm_is_mipi1_and_used_by_isp(d)) { >>> + dev_info(d, "used by ISP1, skipping from DRM\n"); >>> + continue; >>> + } >>> + >>> device_link_add(dev, d, DL_FLAG_STATELESS); >>> component_match_add(dev, &match, component_compare_dev, d); >>> } while (true);
| |