| Message ID | 20240217183941.1752463-1-megi@xff.cz (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: rockchip: Don't require MIPI DSI device when it's used for ISP | expand |
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. > 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);
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. 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);
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);
On Mon, Feb 19, 2024 at 11:02:01AM +0800, Andy Yan wrote: > 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? It didn't work anymore since some fw devlink changes that were added some time after 2021. But it might have got resolved since than. So please ignore this patch for now. kind regards, o. > [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);
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);