* [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" @ 2022-04-20 23:12 Bjorn Andersson 2022-04-20 23:12 ` [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" Bjorn Andersson ` (3 more replies) 0 siblings, 4 replies; 41+ messages in thread From: Bjorn Andersson @ 2022-04-20 23:12 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Paul Kocialkowski, Rob Clark Cc: dri-devel, linux-kernel Commit '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")' introduced the ability to describe a panel under a display controller without having to use a graph to connect the controller to its single child panel (or bridge). The implementation of this would find the first non-graph node and attempt to acquire the related panel or bridge. This prevents cases where any other child node, such as a aux bus for a DisplayPort controller, or an opp-table to find the referenced panel. Commit '67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")' attempted to solve this problem by not bypassing the graph reference lookup before attempting to find the panel or bridge. While this does solve the case where a proper graph reference is present, it does not allow the caller to distinguish between a yet-to-be-probed panel or bridge and the absence of a reference to a panel. One such case is a DisplayPort controller that on some boards have an explicitly described reference to a panel, but on others have a discoverable DisplayPort display attached (which doesn't need to be expressed in DeviceTree). This reverts commit '67bae5f28c89 ("drm: of: Properly try all possible cases for bridge/panel detection")', as a step towards reverting commit '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")'. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index f4df344509a8..026e4e29a0f3 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -214,29 +214,6 @@ int drm_of_encoder_active_endpoint(struct device_node *node, } EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); -static int find_panel_or_bridge(struct device_node *node, - struct drm_panel **panel, - struct drm_bridge **bridge) -{ - if (panel) { - *panel = of_drm_find_panel(node); - if (!IS_ERR(*panel)) - return 0; - - /* Clear the panel pointer in case of error. */ - *panel = NULL; - } - - /* No panel found yet, check for a bridge next. */ - if (bridge) { - *bridge = of_drm_find_bridge(node); - if (*bridge) - return 0; - } - - return -EPROBE_DEFER; -} - /** * drm_of_find_panel_or_bridge - return connected panel or bridge device * @np: device tree node containing encoder output ports @@ -259,44 +236,66 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_panel **panel, struct drm_bridge **bridge) { - struct device_node *node; - int ret; + int ret = -EPROBE_DEFER; + struct device_node *remote; if (!panel && !bridge) return -EINVAL; - if (panel) *panel = NULL; - if (bridge) - *bridge = NULL; - - /* Check for a graph on the device node first. */ - if (of_graph_is_present(np)) { - node = of_graph_get_remote_node(np, port, endpoint); - if (node) { - ret = find_panel_or_bridge(node, panel, bridge); - of_node_put(node); - - if (!ret) - return 0; - } - } - /* Otherwise check for any child node other than port/ports. */ - for_each_available_child_of_node(np, node) { - if (of_node_name_eq(node, "port") || - of_node_name_eq(node, "ports")) + /** + * Devices can also be child nodes when we also control that device + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). + * + * Lookup for a child node of the given parent that isn't either port + * or ports. + */ + for_each_available_child_of_node(np, remote) { + if (of_node_name_eq(remote, "port") || + of_node_name_eq(remote, "ports")) continue; - ret = find_panel_or_bridge(node, panel, bridge); - of_node_put(node); + goto of_find_panel_or_bridge; + } + + /* + * of_graph_get_remote_node() produces a noisy error message if port + * node isn't found and the absence of the port is a legit case here, + * so at first we silently check whether graph presents in the + * device-tree node. + */ + if (!of_graph_is_present(np)) + return -ENODEV; + + remote = of_graph_get_remote_node(np, port, endpoint); + +of_find_panel_or_bridge: + if (!remote) + return -ENODEV; + + if (panel) { + *panel = of_drm_find_panel(remote); + if (!IS_ERR(*panel)) + ret = 0; + else + *panel = NULL; + } + + /* No panel found yet, check for a bridge next. */ + if (bridge) { + if (ret) { + *bridge = of_drm_find_bridge(remote); + if (*bridge) + ret = 0; + } else { + *bridge = NULL; + } - /* Stop at the first found occurrence. */ - if (!ret) - return 0; } - return -EPROBE_DEFER; + of_node_put(remote); + return ret; } EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); -- 2.35.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-20 23:12 [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson @ 2022-04-20 23:12 ` Bjorn Andersson 2022-04-21 7:20 ` (subset) " Maxime Ripard 2022-04-21 7:45 ` Jagan Teki 2022-04-20 23:19 ` [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson ` (2 subsequent siblings) 3 siblings, 2 replies; 41+ messages in thread From: Bjorn Andersson @ 2022-04-20 23:12 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Paul Kocialkowski, Rob Clark Cc: dri-devel, linux-kernel Commit '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")' attempted to simplify the case of expressing a simple panel under a DSI controller, by assuming that the first non-graph child node was a panel or bridge. Unfortunately for non-trivial cases the first child node might not be a panel or bridge. Examples of this can be a aux-bus in the case of DisplayPort, or an opp-table represented before the panel node. In these cases the reverted commit prevents the caller from ever finding a reference to the panel. This reverts commit '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")', in favor of using an explicit graph reference to the panel in the trivial case as well. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/gpu/drm/drm_of.c | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 026e4e29a0f3..9a2cfab3a177 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -244,21 +244,6 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, if (panel) *panel = NULL; - /** - * Devices can also be child nodes when we also control that device - * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). - * - * Lookup for a child node of the given parent that isn't either port - * or ports. - */ - for_each_available_child_of_node(np, remote) { - if (of_node_name_eq(remote, "port") || - of_node_name_eq(remote, "ports")) - continue; - - goto of_find_panel_or_bridge; - } - /* * of_graph_get_remote_node() produces a noisy error message if port * node isn't found and the absence of the port is a legit case here, @@ -269,8 +254,6 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return -ENODEV; remote = of_graph_get_remote_node(np, port, endpoint); - -of_find_panel_or_bridge: if (!remote) return -ENODEV; -- 2.35.1 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: (subset) [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-20 23:12 ` [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" Bjorn Andersson @ 2022-04-21 7:20 ` Maxime Ripard 2022-04-21 7:45 ` Jagan Teki 1 sibling, 0 replies; 41+ messages in thread From: Maxime Ripard @ 2022-04-21 7:20 UTC (permalink / raw) To: Maarten Lankhorst, David Airlie, Thierry Reding, Daniel Vetter, Thomas Zimmermann, Bjorn Andersson, Paul Kocialkowski, Maxime Ripard, Rob Clark Cc: Maxime Ripard, linux-kernel, dri-devel On Wed, 20 Apr 2022 16:12:30 -0700, Bjorn Andersson wrote: > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > bridge")' attempted to simplify the case of expressing a simple panel > under a DSI controller, by assuming that the first non-graph child node > was a panel or bridge. > > Unfortunately for non-trivial cases the first child node might not be a > panel or bridge. Examples of this can be a aux-bus in the case of > DisplayPort, or an opp-table represented before the panel node. > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-20 23:12 ` [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" Bjorn Andersson 2022-04-21 7:20 ` (subset) " Maxime Ripard @ 2022-04-21 7:45 ` Jagan Teki 2022-04-21 8:23 ` Maxime Ripard 1 sibling, 1 reply; 41+ messages in thread From: Jagan Teki @ 2022-04-21 7:45 UTC (permalink / raw) To: Bjorn Andersson Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Paul Kocialkowski, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss + Linus + Marek + Laurent + Robert On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > bridge")' attempted to simplify the case of expressing a simple panel > under a DSI controller, by assuming that the first non-graph child node > was a panel or bridge. > > Unfortunately for non-trivial cases the first child node might not be a > panel or bridge. Examples of this can be a aux-bus in the case of > DisplayPort, or an opp-table represented before the panel node. > > In these cases the reverted commit prevents the caller from ever finding > a reference to the panel. > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > panel or bridge")', in favor of using an explicit graph reference to the > panel in the trivial case as well. This eventually breaks many child-based devm_drm_of_get_bridge switched drivers. Do you have any suggestions on how to proceed to succeed in those use cases as well? Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-21 7:45 ` Jagan Teki @ 2022-04-21 8:23 ` Maxime Ripard 2022-04-21 8:59 ` Paul Kocialkowski 2022-04-27 6:59 ` Jagan Teki 0 siblings, 2 replies; 41+ messages in thread From: Maxime Ripard @ 2022-04-21 8:23 UTC (permalink / raw) To: Jagan Teki Cc: Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Paul Kocialkowski, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss [-- Attachment #1: Type: text/plain, Size: 1289 bytes --] On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > + Linus > + Marek > + Laurent > + Robert > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > bridge")' attempted to simplify the case of expressing a simple panel > > under a DSI controller, by assuming that the first non-graph child node > > was a panel or bridge. > > > > Unfortunately for non-trivial cases the first child node might not be a > > panel or bridge. Examples of this can be a aux-bus in the case of > > DisplayPort, or an opp-table represented before the panel node. > > > > In these cases the reverted commit prevents the caller from ever finding > > a reference to the panel. > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > panel or bridge")', in favor of using an explicit graph reference to the > > panel in the trivial case as well. > > This eventually breaks many child-based devm_drm_of_get_bridge > switched drivers. Do you have any suggestions on how to proceed to > succeed in those use cases as well? I guess we could create a new helper for those, like devm_drm_of_get_bridge_with_panel, or something. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-21 8:23 ` Maxime Ripard @ 2022-04-21 8:59 ` Paul Kocialkowski 2022-04-26 7:54 ` Paul Kocialkowski 2022-04-27 6:59 ` Jagan Teki 1 sibling, 1 reply; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-21 8:59 UTC (permalink / raw) To: Maxime Ripard Cc: Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss [-- Attachment #1: Type: text/plain, Size: 1621 bytes --] Hi Maxime, On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > + Linus > > + Marek > > + Laurent > > + Robert > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > bridge")' attempted to simplify the case of expressing a simple panel > > > under a DSI controller, by assuming that the first non-graph child node > > > was a panel or bridge. > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > a reference to the panel. > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > panel or bridge")', in favor of using an explicit graph reference to the > > > panel in the trivial case as well. > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > switched drivers. Do you have any suggestions on how to proceed to > > succeed in those use cases as well? > > I guess we could create a new helper for those, like > devm_drm_of_get_bridge_with_panel, or something. Oh wow I feel stupid for not thinking about that. Yeah I agree that it seems like the best option. Cheers, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-21 8:59 ` Paul Kocialkowski @ 2022-04-26 7:54 ` Paul Kocialkowski 2022-04-26 8:10 ` Jagan Teki ` (2 more replies) 0 siblings, 3 replies; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-26 7:54 UTC (permalink / raw) To: Maxime Ripard Cc: Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss [-- Attachment #1: Type: text/plain, Size: 2255 bytes --] Hi, On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > Hi Maxime, > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > + Linus > > > + Marek > > > + Laurent > > > + Robert > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > under a DSI controller, by assuming that the first non-graph child node > > > > was a panel or bridge. > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > a reference to the panel. > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > panel in the trivial case as well. > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > switched drivers. Do you have any suggestions on how to proceed to > > > succeed in those use cases as well? > > > > I guess we could create a new helper for those, like > > devm_drm_of_get_bridge_with_panel, or something. > > Oh wow I feel stupid for not thinking about that. > > Yeah I agree that it seems like the best option. Should I prepare a patch with such a new helper? The idea would be to keep drm_of_find_panel_or_bridge only for the of graph case and add one for the child node case, maybe: drm_of_find_child_panel_or_bridge. I really don't have a clear idea of which driver would need to be switched over though. Could someone (Jagan?) let me know where it would be needed? Are there cases where we could both expect of graph and child node? (i.e. does the new helper also need to try via of graph?) Thanks, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 7:54 ` Paul Kocialkowski @ 2022-04-26 8:10 ` Jagan Teki 2022-04-27 14:34 ` Maxime Ripard 2022-04-26 11:33 ` Laurent Pinchart 2022-04-26 12:51 ` Maxime Ripard 2 siblings, 1 reply; 41+ messages in thread From: Jagan Teki @ 2022-04-26 8:10 UTC (permalink / raw) To: Paul Kocialkowski Cc: Maxime Ripard, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi, > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > Hi Maxime, > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > Oh wow I feel stupid for not thinking about that. > > > > Yeah I agree that it seems like the best option. > > Should I prepare a patch with such a new helper? > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > case and add one for the child node case, maybe: > drm_of_find_child_panel_or_bridge. > > I really don't have a clear idea of which driver would need to be switched > over though. Could someone (Jagan?) let me know where it would be needed? sun6i_mipi_dsi, exynos_drm_dsi, mcde_dsi (as of now) > > Are there cases where we could both expect of graph and child node? > (i.e. does the new helper also need to try via of graph?) One finding so far from my side would be if the check iterates the child and identify the panel or bridge child irrespective of the position it has and untouched non-trivial child-like dp, opp-table can help to use same change what we have it before. Still working on getting a proper check. Thanks, Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 8:10 ` Jagan Teki @ 2022-04-27 14:34 ` Maxime Ripard 2022-04-28 6:17 ` Marek Szyprowski 2022-04-28 8:39 ` Jagan Teki 0 siblings, 2 replies; 41+ messages in thread From: Maxime Ripard @ 2022-04-27 14:34 UTC (permalink / raw) To: Jagan Teki Cc: Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski > <paul.kocialkowski@bootlin.com> wrote: > > > > Hi, > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > Hi Maxime, > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > + Linus > > > > > + Marek > > > > > + Laurent > > > > > + Robert > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > was a panel or bridge. > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > a reference to the panel. > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > panel in the trivial case as well. > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > succeed in those use cases as well? > > > > > > > > I guess we could create a new helper for those, like > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > Yeah I agree that it seems like the best option. > > > > Should I prepare a patch with such a new helper? > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > case and add one for the child node case, maybe: > > drm_of_find_child_panel_or_bridge. > > > > I really don't have a clear idea of which driver would need to be switched > > over though. Could someone (Jagan?) let me know where it would be needed? > > sun6i_mipi_dsi It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > exynos_drm_dsi If you reference 711c7adc4687, I don't see why we would need to switch it back to the old behaviour. It wasn't iterating over its child node before, so what does the switch to drm_of_get_bridge broke exactly? > mcde_dsi (as of now) Yeah, we do need to revert 3730bc6147b0 and 3d7039e1e649 Maxime ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-27 14:34 ` Maxime Ripard @ 2022-04-28 6:17 ` Marek Szyprowski 2022-04-28 8:25 ` Jagan Teki 2022-04-28 8:39 ` Jagan Teki 1 sibling, 1 reply; 41+ messages in thread From: Marek Szyprowski @ 2022-04-28 6:17 UTC (permalink / raw) To: Maxime Ripard, Jagan Teki Cc: Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Laurent Pinchart, Robert Foss Hi Maxime, On 27.04.2022 16:34, Maxime Ripard wrote: > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: >> On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski >> <paul.kocialkowski@bootlin.com> wrote: >>> On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: >>>> On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: >>>>> On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: >>>>>> + Linus >>>>>> + Marek >>>>>> + Laurent >>>>>> + Robert >>>>>> >>>>>> On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson >>>>>> <bjorn.andersson@linaro.org> wrote: >>>>>>> Commit '80253168dbfd ("drm: of: Lookup if child node has panel or >>>>>>> bridge")' attempted to simplify the case of expressing a simple panel >>>>>>> under a DSI controller, by assuming that the first non-graph child node >>>>>>> was a panel or bridge. >>>>>>> >>>>>>> Unfortunately for non-trivial cases the first child node might not be a >>>>>>> panel or bridge. Examples of this can be a aux-bus in the case of >>>>>>> DisplayPort, or an opp-table represented before the panel node. >>>>>>> >>>>>>> In these cases the reverted commit prevents the caller from ever finding >>>>>>> a reference to the panel. >>>>>>> >>>>>>> This reverts commit '80253168dbfd ("drm: of: Lookup if child node has >>>>>>> panel or bridge")', in favor of using an explicit graph reference to the >>>>>>> panel in the trivial case as well. >>>>>> This eventually breaks many child-based devm_drm_of_get_bridge >>>>>> switched drivers. Do you have any suggestions on how to proceed to >>>>>> succeed in those use cases as well? >>>>> I guess we could create a new helper for those, like >>>>> devm_drm_of_get_bridge_with_panel, or something. >>>> Oh wow I feel stupid for not thinking about that. >>>> >>>> Yeah I agree that it seems like the best option. >>> Should I prepare a patch with such a new helper? >>> >>> The idea would be to keep drm_of_find_panel_or_bridge only for the of graph >>> case and add one for the child node case, maybe: >>> drm_of_find_child_panel_or_bridge. >>> >>> I really don't have a clear idea of which driver would need to be switched >>> over though. Could someone (Jagan?) let me know where it would be needed? >> sun6i_mipi_dsi > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > >> exynos_drm_dsi > If you reference 711c7adc4687, I don't see why we would need to switch > it back to the old behaviour. It wasn't iterating over its child node > before, so what does the switch to drm_of_get_bridge broke exactly? It broke getting the panel if it is a direct child of the DSI device node. It worked before because it used following code: dsi->panel = of_drm_find_panel(device->dev.of_node); which got replaced by devm_drm_of_get_bridge(). >> mcde_dsi (as of now) > Yeah, we do need to revert 3730bc6147b0 and 3d7039e1e649 > > Maxime > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-28 6:17 ` Marek Szyprowski @ 2022-04-28 8:25 ` Jagan Teki 0 siblings, 0 replies; 41+ messages in thread From: Jagan Teki @ 2022-04-28 8:25 UTC (permalink / raw) To: Marek Szyprowski Cc: Maxime Ripard, Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Laurent Pinchart, Robert Foss Hi Marek, On Thu, Apr 28, 2022 at 11:47 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi Maxime, > > On 27.04.2022 16:34, Maxime Ripard wrote: > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > >> On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski > >> <paul.kocialkowski@bootlin.com> wrote: > >>> On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > >>>> On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > >>>>> On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > >>>>>> + Linus > >>>>>> + Marek > >>>>>> + Laurent > >>>>>> + Robert > >>>>>> > >>>>>> On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > >>>>>> <bjorn.andersson@linaro.org> wrote: > >>>>>>> Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > >>>>>>> bridge")' attempted to simplify the case of expressing a simple panel > >>>>>>> under a DSI controller, by assuming that the first non-graph child node > >>>>>>> was a panel or bridge. > >>>>>>> > >>>>>>> Unfortunately for non-trivial cases the first child node might not be a > >>>>>>> panel or bridge. Examples of this can be a aux-bus in the case of > >>>>>>> DisplayPort, or an opp-table represented before the panel node. > >>>>>>> > >>>>>>> In these cases the reverted commit prevents the caller from ever finding > >>>>>>> a reference to the panel. > >>>>>>> > >>>>>>> This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > >>>>>>> panel or bridge")', in favor of using an explicit graph reference to the > >>>>>>> panel in the trivial case as well. > >>>>>> This eventually breaks many child-based devm_drm_of_get_bridge > >>>>>> switched drivers. Do you have any suggestions on how to proceed to > >>>>>> succeed in those use cases as well? > >>>>> I guess we could create a new helper for those, like > >>>>> devm_drm_of_get_bridge_with_panel, or something. > >>>> Oh wow I feel stupid for not thinking about that. > >>>> > >>>> Yeah I agree that it seems like the best option. > >>> Should I prepare a patch with such a new helper? > >>> > >>> The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > >>> case and add one for the child node case, maybe: > >>> drm_of_find_child_panel_or_bridge. > >>> > >>> I really don't have a clear idea of which driver would need to be switched > >>> over though. Could someone (Jagan?) let me know where it would be needed? > >> sun6i_mipi_dsi > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > > >> exynos_drm_dsi > > If you reference 711c7adc4687, I don't see why we would need to switch > > it back to the old behaviour. It wasn't iterating over its child node > > before, so what does the switch to drm_of_get_bridge broke exactly? > > It broke getting the panel if it is a direct child of the DSI device > node. It worked before because it used following code: > > dsi->panel = of_drm_find_panel(device->dev.of_node); > > which got replaced by devm_drm_of_get_bridge(). Yes, we need to revert that change back to find the individual panel and bridge. I'm preparing a patch for it. Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-27 14:34 ` Maxime Ripard 2022-04-28 6:17 ` Marek Szyprowski @ 2022-04-28 8:39 ` Jagan Teki 2022-04-28 22:17 ` Laurent Pinchart 1 sibling, 1 reply; 41+ messages in thread From: Jagan Teki @ 2022-04-28 8:39 UTC (permalink / raw) To: Maxime Ripard Cc: Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski > > <paul.kocialkowski@bootlin.com> wrote: > > > > > > Hi, > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > Hi Maxime, > > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > + Linus > > > > > > + Marek > > > > > > + Laurent > > > > > > + Robert > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > a reference to the panel. > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > succeed in those use cases as well? > > > > > > > > > > I guess we could create a new helper for those, like > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > Should I prepare a patch with such a new helper? > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > case and add one for the child node case, maybe: > > > drm_of_find_child_panel_or_bridge. > > > > > > I really don't have a clear idea of which driver would need to be switched > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > sun6i_mipi_dsi > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? Correct, patch for this on the mailing list. > > > exynos_drm_dsi > > If you reference 711c7adc4687, I don't see why we would need to switch > it back to the old behaviour. It wasn't iterating over its child node > before, so what does the switch to drm_of_get_bridge broke exactly? Exynos bindings have a child node (unlike OF-graph), the old code is checking panel and bridge individually so it broke once switch to devm_drm_of_get_bridge Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-28 8:39 ` Jagan Teki @ 2022-04-28 22:17 ` Laurent Pinchart 2022-04-29 8:24 ` Jagan Teki 2022-04-29 15:46 ` Maxime Ripard 0 siblings, 2 replies; 41+ messages in thread From: Laurent Pinchart @ 2022-04-28 22:17 UTC (permalink / raw) To: Jagan Teki Cc: Maxime Ripard, Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss Hi Jagan, On Thu, Apr 28, 2022 at 02:09:42PM +0530, Jagan Teki wrote: > On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard wrote: > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski wrote: > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > + Linus > > > > > > > + Marek > > > > > > > + Laurent > > > > > > > + Robert > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > case and add one for the child node case, maybe: > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > sun6i_mipi_dsi > > > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > Correct, patch for this on the mailing list. I've lost track of how we're solving the fallout of this for v5.18. I have received a report that the original commit (80253168dbfd) also broke the rcar-du driver. Could you please provide a git branch (based on drm-fixes or drm-misc-fixes) with any patch that you plan to get merged in v5.18, to let me test them locally ? > > > exynos_drm_dsi > > > > If you reference 711c7adc4687, I don't see why we would need to switch > > it back to the old behaviour. It wasn't iterating over its child node > > before, so what does the switch to drm_of_get_bridge broke exactly? > > Exynos bindings have a child node (unlike OF-graph), the old code is > checking panel and bridge individually so it broke once switch to > devm_drm_of_get_bridge -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-28 22:17 ` Laurent Pinchart @ 2022-04-29 8:24 ` Jagan Teki 2022-04-29 15:46 ` Maxime Ripard 1 sibling, 0 replies; 41+ messages in thread From: Jagan Teki @ 2022-04-29 8:24 UTC (permalink / raw) To: Laurent Pinchart Cc: Maxime Ripard, Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss Hi Laurent, On Fri, Apr 29, 2022 at 3:47 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jagan, > > On Thu, Apr 28, 2022 at 02:09:42PM +0530, Jagan Teki wrote: > > On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard wrote: > > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > + Linus > > > > > > > > + Marek > > > > > > > > + Laurent > > > > > > > > + Robert > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > case and add one for the child node case, maybe: > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > sun6i_mipi_dsi > > > > > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > > > Correct, patch for this on the mailing list. > > I've lost track of how we're solving the fallout of this for v5.18. I > have received a report that the original commit (80253168dbfd) also > broke the rcar-du driver. Could you please provide a git branch (based > on drm-fixes or drm-misc-fixes) with any patch that you plan to get > merged in v5.18, to let me test them locally ? The affected patches for 80253168dbfd revert are 711c7adc4687 3730bc6147b0 and 3d7039e1e649 Both these are not present drm-misc-fixes but there in linux-next. I've sent a patch for 711c7adc4687 https://patchwork.kernel.org/project/dri-devel/patch/20220428094808.782938-1-jagan@amarulasolutions.com/ This is my repo on top of linux-next https://github.com/openedev/kernel/tree/linux-next/drm-misc As I have seen before rcar-du ("155358310f013") is OF-graph and it doesn't affect the child node lookup was introduced in ("80253168dbfd") Let me know if you have any further information. Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-28 22:17 ` Laurent Pinchart 2022-04-29 8:24 ` Jagan Teki @ 2022-04-29 15:46 ` Maxime Ripard 2022-04-29 16:05 ` Laurent Pinchart 1 sibling, 1 reply; 41+ messages in thread From: Maxime Ripard @ 2022-04-29 15:46 UTC (permalink / raw) To: Laurent Pinchart Cc: Jagan Teki, Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 3377 bytes --] On Fri, Apr 29, 2022 at 01:17:26AM +0300, Laurent Pinchart wrote: > Hi Jagan, > > On Thu, Apr 28, 2022 at 02:09:42PM +0530, Jagan Teki wrote: > > On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard wrote: > > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > + Linus > > > > > > > > + Marek > > > > > > > > + Laurent > > > > > > > > + Robert > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > case and add one for the child node case, maybe: > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > sun6i_mipi_dsi > > > > > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > > > Correct, patch for this on the mailing list. > > I've lost track of how we're solving the fallout of this for v5.18. I > have received a report that the original commit (80253168dbfd) also > broke the rcar-du driver. Could you please provide a git branch (based > on drm-fixes or drm-misc-fixes) with any patch that you plan to get > merged in v5.18, to let me test them locally ? Was that report about 5.18 or drm-misc-next? It appears that all the drivers conversions are in drm-misc-next. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-29 15:46 ` Maxime Ripard @ 2022-04-29 16:05 ` Laurent Pinchart 2022-05-04 15:08 ` Maxime Ripard 0 siblings, 1 reply; 41+ messages in thread From: Laurent Pinchart @ 2022-04-29 16:05 UTC (permalink / raw) To: Maxime Ripard Cc: Jagan Teki, Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss Hi Maxime, On Fri, Apr 29, 2022 at 05:46:45PM +0200, Maxime Ripard wrote: > On Fri, Apr 29, 2022 at 01:17:26AM +0300, Laurent Pinchart wrote: > > On Thu, Apr 28, 2022 at 02:09:42PM +0530, Jagan Teki wrote: > > > On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard wrote: > > > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > > > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski wrote: > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > > + Linus > > > > > > > > > + Marek > > > > > > > > > + Laurent > > > > > > > > > + Robert > > > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > > case and add one for the child node case, maybe: > > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > > > sun6i_mipi_dsi > > > > > > > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > > > > > Correct, patch for this on the mailing list. > > > > I've lost track of how we're solving the fallout of this for v5.18. I > > have received a report that the original commit (80253168dbfd) also > > broke the rcar-du driver. Could you please provide a git branch (based > > on drm-fixes or drm-misc-fixes) with any patch that you plan to get > > merged in v5.18, to let me test them locally ? > > Was that report about 5.18 or drm-misc-next? It appears that all the > drivers conversions are in drm-misc-next. v5.18-rc2. I've double-checked, and it has been bisected to commit 67bae5f28c89, which is a fix of the commit this patch reverts (80253168dbfd). -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-29 16:05 ` Laurent Pinchart @ 2022-05-04 15:08 ` Maxime Ripard 0 siblings, 0 replies; 41+ messages in thread From: Maxime Ripard @ 2022-05-04 15:08 UTC (permalink / raw) To: Laurent Pinchart Cc: Jagan Teki, Paul Kocialkowski, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 4001 bytes --] On Fri, Apr 29, 2022 at 07:05:59PM +0300, Laurent Pinchart wrote: > Hi Maxime, > > On Fri, Apr 29, 2022 at 05:46:45PM +0200, Maxime Ripard wrote: > > On Fri, Apr 29, 2022 at 01:17:26AM +0300, Laurent Pinchart wrote: > > > On Thu, Apr 28, 2022 at 02:09:42PM +0530, Jagan Teki wrote: > > > > On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard wrote: > > > > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > > > > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski wrote: > > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > > > + Linus > > > > > > > > > > + Marek > > > > > > > > > > + Laurent > > > > > > > > > > + Robert > > > > > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > > > case and add one for the child node case, maybe: > > > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > > > > > sun6i_mipi_dsi > > > > > > > > > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > > > > > > > Correct, patch for this on the mailing list. > > > > > > I've lost track of how we're solving the fallout of this for v5.18. I > > > have received a report that the original commit (80253168dbfd) also > > > broke the rcar-du driver. Could you please provide a git branch (based > > > on drm-fixes or drm-misc-fixes) with any patch that you plan to get > > > merged in v5.18, to let me test them locally ? > > > > Was that report about 5.18 or drm-misc-next? It appears that all the > > drivers conversions are in drm-misc-next. > > v5.18-rc2. I've double-checked, and it has been bisected to commit > 67bae5f28c89, which is a fix of the commit this patch reverts > (80253168dbfd). We've reverted 67bae5f28c89 in -rc4, so it should work just fine now Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 7:54 ` Paul Kocialkowski 2022-04-26 8:10 ` Jagan Teki @ 2022-04-26 11:33 ` Laurent Pinchart 2022-04-26 12:41 ` Paul Kocialkowski 2022-04-26 12:51 ` Maxime Ripard 2 siblings, 1 reply; 41+ messages in thread From: Laurent Pinchart @ 2022-04-26 11:33 UTC (permalink / raw) To: Paul Kocialkowski Cc: Maxime Ripard, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > Oh wow I feel stupid for not thinking about that. > > > > Yeah I agree that it seems like the best option. > > Should I prepare a patch with such a new helper? > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > case and add one for the child node case, maybe: > drm_of_find_child_panel_or_bridge. > > I really don't have a clear idea of which driver would need to be switched > over though. Could someone (Jagan?) let me know where it would be needed? > > Are there cases where we could both expect of graph and child node? > (i.e. does the new helper also need to try via of graph?) I still think we should use OF graph uncondtionally, even in the DSI case. We need to ensure backward-compatibility, but I'd like new bindings (and thus new drivers) to always use OF graph. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 11:33 ` Laurent Pinchart @ 2022-04-26 12:41 ` Paul Kocialkowski 2022-04-26 12:54 ` Maxime Ripard 0 siblings, 1 reply; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-26 12:41 UTC (permalink / raw) To: Laurent Pinchart Cc: Maxime Ripard, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 3495 bytes --] Hi, On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > + Linus > > > > > + Marek > > > > > + Laurent > > > > > + Robert > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > was a panel or bridge. > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > a reference to the panel. > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > panel in the trivial case as well. > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > succeed in those use cases as well? > > > > > > > > I guess we could create a new helper for those, like > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > Yeah I agree that it seems like the best option. > > > > Should I prepare a patch with such a new helper? > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > case and add one for the child node case, maybe: > > drm_of_find_child_panel_or_bridge. > > > > I really don't have a clear idea of which driver would need to be switched > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > Are there cases where we could both expect of graph and child node? > > (i.e. does the new helper also need to try via of graph?) > > I still think we should use OF graph uncondtionally, even in the DSI > case. We need to ensure backward-compatibility, but I'd like new > bindings (and thus new drivers) to always use OF graph. I just went over the thread on "drm: of: Improve error handling in bridge/panel detection" again and I'm no longer sure there's actually still an issue that stands, with the fix that allows returning -ENODEV when possible. The remaining issue that was brought up was with a connector node, but it should be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge in such situations. So with that in mind it feels like the child node approach can be viable (and integrated in the same helper). We might still want to favor an explicit OF graph approach, but note that dsi-controller.yaml also specifies extra properties that are specific to MIPI DSI and I'm not sure there are equivalent definitions for the OF graph approach. What do you think? Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 12:41 ` Paul Kocialkowski @ 2022-04-26 12:54 ` Maxime Ripard 2022-04-26 12:55 ` Maxime Ripard ` (2 more replies) 0 siblings, 3 replies; 41+ messages in thread From: Maxime Ripard @ 2022-04-26 12:54 UTC (permalink / raw) To: Paul Kocialkowski Cc: Laurent Pinchart, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 3997 bytes --] On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > + Linus > > > > > > + Marek > > > > > > + Laurent > > > > > > + Robert > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > a reference to the panel. > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > succeed in those use cases as well? > > > > > > > > > > I guess we could create a new helper for those, like > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > Should I prepare a patch with such a new helper? > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > case and add one for the child node case, maybe: > > > drm_of_find_child_panel_or_bridge. > > > > > > I really don't have a clear idea of which driver would need to be switched > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > Are there cases where we could both expect of graph and child node? > > > (i.e. does the new helper also need to try via of graph?) > > > > I still think we should use OF graph uncondtionally, even in the DSI > > case. We need to ensure backward-compatibility, but I'd like new > > bindings (and thus new drivers) to always use OF graph. > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > detection" again and I'm no longer sure there's actually still an issue that > stands, with the fix that allows returning -ENODEV when possible. > > The remaining issue that was brought up was with a connector node, but it should > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > in such situations. > > So with that in mind it feels like the child node approach can be viable > (and integrated in the same helper). > > We might still want to favor an explicit OF graph approach, but note that > dsi-controller.yaml also specifies extra properties that are specific to > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > approach. > > What do you think? I don't think Laurent's point was to move the child node away from its DSI controller, that part doesn't make much sense. The panel or bridge is still accessed through the DSI bus, so it very much belongs there. What he meant I think was that we mandate the OF graph for all panels, so for panels/bridges controlled through DCS, you would still list the output through the graph. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 12:54 ` Maxime Ripard @ 2022-04-26 12:55 ` Maxime Ripard 2022-04-26 13:04 ` Paul Kocialkowski 2022-04-26 12:58 ` Paul Kocialkowski 2022-04-27 13:10 ` Laurent Pinchart 2 siblings, 1 reply; 41+ messages in thread From: Maxime Ripard @ 2022-04-26 12:55 UTC (permalink / raw) To: Paul Kocialkowski Cc: Laurent Pinchart, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 4490 bytes --] On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote: > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > + Linus > > > > > > > + Marek > > > > > > > + Laurent > > > > > > > + Robert > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > case and add one for the child node case, maybe: > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > case. We need to ensure backward-compatibility, but I'd like new > > > bindings (and thus new drivers) to always use OF graph. > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > detection" again and I'm no longer sure there's actually still an issue that > > stands, with the fix that allows returning -ENODEV when possible. > > > > The remaining issue that was brought up was with a connector node, but it should > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > in such situations. > > > > So with that in mind it feels like the child node approach can be viable > > (and integrated in the same helper). > > > > We might still want to favor an explicit OF graph approach, but note that > > dsi-controller.yaml also specifies extra properties that are specific to > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > approach. > > > > What do you think? > > I don't think Laurent's point was to move the child node away from its > DSI controller, that part doesn't make much sense. The panel or bridge > is still accessed through the DSI bus, so it very much belongs there. > > What he meant I think was that we mandate the OF graph for all panels, > so for panels/bridges controlled through DCS, you would still list the > output through the graph. Also, we're already in a bit of a mess right now. I don't think rushing that kind of patches in a (late) rc is making much sense, but as I said, if you want to start working on this, then I'll take a revert for the next rc, and then we can work calmly on this. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 12:55 ` Maxime Ripard @ 2022-04-26 13:04 ` Paul Kocialkowski 2022-04-26 13:19 ` Maxime Ripard 0 siblings, 1 reply; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-26 13:04 UTC (permalink / raw) To: Maxime Ripard Cc: Laurent Pinchart, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 5570 bytes --] On Tue 26 Apr 22, 14:55, Maxime Ripard wrote: > On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote: > > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > + Linus > > > > > > > > + Marek > > > > > > > > + Laurent > > > > > > > > + Robert > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > case and add one for the child node case, maybe: > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > > case. We need to ensure backward-compatibility, but I'd like new > > > > bindings (and thus new drivers) to always use OF graph. > > > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > > detection" again and I'm no longer sure there's actually still an issue that > > > stands, with the fix that allows returning -ENODEV when possible. > > > > > > The remaining issue that was brought up was with a connector node, but it should > > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > > in such situations. > > > > > > So with that in mind it feels like the child node approach can be viable > > > (and integrated in the same helper). > > > > > > We might still want to favor an explicit OF graph approach, but note that > > > dsi-controller.yaml also specifies extra properties that are specific to > > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > > approach. > > > > > > What do you think? > > > > I don't think Laurent's point was to move the child node away from its > > DSI controller, that part doesn't make much sense. The panel or bridge > > is still accessed through the DSI bus, so it very much belongs there. > > > > What he meant I think was that we mandate the OF graph for all panels, > > so for panels/bridges controlled through DCS, you would still list the > > output through the graph. > > Also, we're already in a bit of a mess right now. I don't think rushing > that kind of patches in a (late) rc is making much sense, but as I said, > if you want to start working on this, then I'll take a revert for the > next rc, and then we can work calmly on this. As I understand it we either have some broken stuff because of the revert of: - drm: of: Lookup if child node has panel or bridge - drm: of: Properly try all possible cases for bridge/panel detection because the child node is already used in places, or we can have broken stuff because with the patches because with these two patches -ENODEV is no longer returned. Now with the extra patch that I sent: - drm: of: Improve error handling in bridge/panel detection we get -ENODEV back, except for the connector case but this one should be handled in drivers directly and drm_of_find_panel_or_bridge should not be called in that situation. So all in all it seems that all the pieces are there, unless I'm missing something. What do you think? Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 13:04 ` Paul Kocialkowski @ 2022-04-26 13:19 ` Maxime Ripard 2022-04-26 13:50 ` Paul Kocialkowski 0 siblings, 1 reply; 41+ messages in thread From: Maxime Ripard @ 2022-04-26 13:19 UTC (permalink / raw) To: Paul Kocialkowski Cc: Laurent Pinchart, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 5880 bytes --] On Tue, Apr 26, 2022 at 03:04:17PM +0200, Paul Kocialkowski wrote: > On Tue 26 Apr 22, 14:55, Maxime Ripard wrote: > > On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote: > > > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > > + Linus > > > > > > > > > + Marek > > > > > > > > > + Laurent > > > > > > > > > + Robert > > > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > > case and add one for the child node case, maybe: > > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > > > case. We need to ensure backward-compatibility, but I'd like new > > > > > bindings (and thus new drivers) to always use OF graph. > > > > > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > > > detection" again and I'm no longer sure there's actually still an issue that > > > > stands, with the fix that allows returning -ENODEV when possible. > > > > > > > > The remaining issue that was brought up was with a connector node, but it should > > > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > > > in such situations. > > > > > > > > So with that in mind it feels like the child node approach can be viable > > > > (and integrated in the same helper). > > > > > > > > We might still want to favor an explicit OF graph approach, but note that > > > > dsi-controller.yaml also specifies extra properties that are specific to > > > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > > > approach. > > > > > > > > What do you think? > > > > > > I don't think Laurent's point was to move the child node away from its > > > DSI controller, that part doesn't make much sense. The panel or bridge > > > is still accessed through the DSI bus, so it very much belongs there. > > > > > > What he meant I think was that we mandate the OF graph for all panels, > > > so for panels/bridges controlled through DCS, you would still list the > > > output through the graph. > > > > Also, we're already in a bit of a mess right now. I don't think rushing > > that kind of patches in a (late) rc is making much sense, but as I said, > > if you want to start working on this, then I'll take a revert for the > > next rc, and then we can work calmly on this. > > As I understand it we either have some broken stuff because of the revert of: > - drm: of: Lookup if child node has panel or bridge > - drm: of: Properly try all possible cases for bridge/panel detection > > because the child node is already used in places, or we can have broken stuff > because with the patches because with these two patches -ENODEV is no longer > returned. > > Now with the extra patch that I sent: > - drm: of: Improve error handling in bridge/panel detection > > we get -ENODEV back, except for the connector case but this one should be > handled in drivers directly and drm_of_find_panel_or_bridge should not be > called in that situation. > > So all in all it seems that all the pieces are there, unless I'm missing > something. > > What do you think? If Bjorn and Thierry can confirm that it indeeds work in their case, I'll be happy to apply those patches as well. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 13:19 ` Maxime Ripard @ 2022-04-26 13:50 ` Paul Kocialkowski 2022-04-26 21:10 ` Bjorn Andersson 0 siblings, 1 reply; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-26 13:50 UTC (permalink / raw) To: Maxime Ripard Cc: Laurent Pinchart, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 6429 bytes --] On Tue 26 Apr 22, 15:19, Maxime Ripard wrote: > On Tue, Apr 26, 2022 at 03:04:17PM +0200, Paul Kocialkowski wrote: > > On Tue 26 Apr 22, 14:55, Maxime Ripard wrote: > > > On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote: > > > > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > > > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > > > + Linus > > > > > > > > > > + Marek > > > > > > > > > > + Laurent > > > > > > > > > > + Robert > > > > > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > > > case and add one for the child node case, maybe: > > > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > > > > case. We need to ensure backward-compatibility, but I'd like new > > > > > > bindings (and thus new drivers) to always use OF graph. > > > > > > > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > > > > detection" again and I'm no longer sure there's actually still an issue that > > > > > stands, with the fix that allows returning -ENODEV when possible. > > > > > > > > > > The remaining issue that was brought up was with a connector node, but it should > > > > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > > > > in such situations. > > > > > > > > > > So with that in mind it feels like the child node approach can be viable > > > > > (and integrated in the same helper). > > > > > > > > > > We might still want to favor an explicit OF graph approach, but note that > > > > > dsi-controller.yaml also specifies extra properties that are specific to > > > > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > > > > approach. > > > > > > > > > > What do you think? > > > > > > > > I don't think Laurent's point was to move the child node away from its > > > > DSI controller, that part doesn't make much sense. The panel or bridge > > > > is still accessed through the DSI bus, so it very much belongs there. > > > > > > > > What he meant I think was that we mandate the OF graph for all panels, > > > > so for panels/bridges controlled through DCS, you would still list the > > > > output through the graph. > > > > > > Also, we're already in a bit of a mess right now. I don't think rushing > > > that kind of patches in a (late) rc is making much sense, but as I said, > > > if you want to start working on this, then I'll take a revert for the > > > next rc, and then we can work calmly on this. > > > > As I understand it we either have some broken stuff because of the revert of: > > - drm: of: Lookup if child node has panel or bridge > > - drm: of: Properly try all possible cases for bridge/panel detection > > > > because the child node is already used in places, or we can have broken stuff > > because with the patches because with these two patches -ENODEV is no longer > > returned. > > > > Now with the extra patch that I sent: > > - drm: of: Improve error handling in bridge/panel detection > > > > we get -ENODEV back, except for the connector case but this one should be > > handled in drivers directly and drm_of_find_panel_or_bridge should not be > > called in that situation. > > > > So all in all it seems that all the pieces are there, unless I'm missing > > something. > > > > What do you think? > > If Bjorn and Thierry can confirm that it indeeds work in their case, > I'll be happy to apply those patches as well. I still think we'd need a fix for Bjorn's connector case though. Not sure I would be confident providing that one without the hardware to test with. Bjorn, what do you think? Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 13:50 ` Paul Kocialkowski @ 2022-04-26 21:10 ` Bjorn Andersson 2022-04-27 7:34 ` Paul Kocialkowski 0 siblings, 1 reply; 41+ messages in thread From: Bjorn Andersson @ 2022-04-26 21:10 UTC (permalink / raw) To: Paul Kocialkowski Cc: Maxime Ripard, Laurent Pinchart, Jagan Teki, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss On Tue 26 Apr 06:50 PDT 2022, Paul Kocialkowski wrote: > On Tue 26 Apr 22, 15:19, Maxime Ripard wrote: > > On Tue, Apr 26, 2022 at 03:04:17PM +0200, Paul Kocialkowski wrote: > > > On Tue 26 Apr 22, 14:55, Maxime Ripard wrote: > > > > On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote: > > > > > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > > > > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > > > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > > > > + Linus > > > > > > > > > > > + Marek > > > > > > > > > > > + Laurent > > > > > > > > > > > + Robert > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > > > > case and add one for the child node case, maybe: > > > > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > > > > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > > > > > case. We need to ensure backward-compatibility, but I'd like new > > > > > > > bindings (and thus new drivers) to always use OF graph. > > > > > > > > > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > > > > > detection" again and I'm no longer sure there's actually still an issue that > > > > > > stands, with the fix that allows returning -ENODEV when possible. > > > > > > > > > > > > The remaining issue that was brought up was with a connector node, but it should > > > > > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > > > > > in such situations. > > > > > > > > > > > > So with that in mind it feels like the child node approach can be viable > > > > > > (and integrated in the same helper). > > > > > > > > > > > > We might still want to favor an explicit OF graph approach, but note that > > > > > > dsi-controller.yaml also specifies extra properties that are specific to > > > > > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > > > > > approach. > > > > > > > > > > > > What do you think? > > > > > > > > > > I don't think Laurent's point was to move the child node away from its > > > > > DSI controller, that part doesn't make much sense. The panel or bridge > > > > > is still accessed through the DSI bus, so it very much belongs there. > > > > > > > > > > What he meant I think was that we mandate the OF graph for all panels, > > > > > so for panels/bridges controlled through DCS, you would still list the > > > > > output through the graph. > > > > > > > > Also, we're already in a bit of a mess right now. I don't think rushing > > > > that kind of patches in a (late) rc is making much sense, but as I said, > > > > if you want to start working on this, then I'll take a revert for the > > > > next rc, and then we can work calmly on this. > > > > > > As I understand it we either have some broken stuff because of the revert of: > > > - drm: of: Lookup if child node has panel or bridge > > > - drm: of: Properly try all possible cases for bridge/panel detection > > > > > > because the child node is already used in places, or we can have broken stuff > > > because with the patches because with these two patches -ENODEV is no longer > > > returned. > > > > > > Now with the extra patch that I sent: > > > - drm: of: Improve error handling in bridge/panel detection > > > > > > we get -ENODEV back, except for the connector case but this one should be > > > handled in drivers directly and drm_of_find_panel_or_bridge should not be > > > called in that situation. > > > > > > So all in all it seems that all the pieces are there, unless I'm missing > > > something. > > > > > > What do you think? > > > > If Bjorn and Thierry can confirm that it indeeds work in their case, > > I'll be happy to apply those patches as well. > > I still think we'd need a fix for Bjorn's connector case though. > Not sure I would be confident providing that one without the hardware > to test with. > > Bjorn, what do you think? > I'm okay with the idea that it's up the driver to check that the output port references an usb-c-connector - either before the call or upon drm_of_find_panel_or_bridge() returning an error. Regards, Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 21:10 ` Bjorn Andersson @ 2022-04-27 7:34 ` Paul Kocialkowski 2022-05-03 0:03 ` Bjorn Andersson 0 siblings, 1 reply; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-27 7:34 UTC (permalink / raw) To: Bjorn Andersson Cc: Maxime Ripard, Laurent Pinchart, Jagan Teki, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 7462 bytes --] Hi Bjorn, On Tue 26 Apr 22, 14:10, Bjorn Andersson wrote: > On Tue 26 Apr 06:50 PDT 2022, Paul Kocialkowski wrote: > > > On Tue 26 Apr 22, 15:19, Maxime Ripard wrote: > > > On Tue, Apr 26, 2022 at 03:04:17PM +0200, Paul Kocialkowski wrote: > > > > On Tue 26 Apr 22, 14:55, Maxime Ripard wrote: > > > > > On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote: > > > > > > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > > > > > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > > > > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > > > > > + Linus > > > > > > > > > > > > + Marek > > > > > > > > > > > > + Laurent > > > > > > > > > > > > + Robert > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > > > > > case and add one for the child node case, maybe: > > > > > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > > > > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > > > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > > > > > > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > > > > > > case. We need to ensure backward-compatibility, but I'd like new > > > > > > > > bindings (and thus new drivers) to always use OF graph. > > > > > > > > > > > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > > > > > > detection" again and I'm no longer sure there's actually still an issue that > > > > > > > stands, with the fix that allows returning -ENODEV when possible. > > > > > > > > > > > > > > The remaining issue that was brought up was with a connector node, but it should > > > > > > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > > > > > > in such situations. > > > > > > > > > > > > > > So with that in mind it feels like the child node approach can be viable > > > > > > > (and integrated in the same helper). > > > > > > > > > > > > > > We might still want to favor an explicit OF graph approach, but note that > > > > > > > dsi-controller.yaml also specifies extra properties that are specific to > > > > > > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > > > > > > approach. > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > I don't think Laurent's point was to move the child node away from its > > > > > > DSI controller, that part doesn't make much sense. The panel or bridge > > > > > > is still accessed through the DSI bus, so it very much belongs there. > > > > > > > > > > > > What he meant I think was that we mandate the OF graph for all panels, > > > > > > so for panels/bridges controlled through DCS, you would still list the > > > > > > output through the graph. > > > > > > > > > > Also, we're already in a bit of a mess right now. I don't think rushing > > > > > that kind of patches in a (late) rc is making much sense, but as I said, > > > > > if you want to start working on this, then I'll take a revert for the > > > > > next rc, and then we can work calmly on this. > > > > > > > > As I understand it we either have some broken stuff because of the revert of: > > > > - drm: of: Lookup if child node has panel or bridge > > > > - drm: of: Properly try all possible cases for bridge/panel detection > > > > > > > > because the child node is already used in places, or we can have broken stuff > > > > because with the patches because with these two patches -ENODEV is no longer > > > > returned. > > > > > > > > Now with the extra patch that I sent: > > > > - drm: of: Improve error handling in bridge/panel detection > > > > > > > > we get -ENODEV back, except for the connector case but this one should be > > > > handled in drivers directly and drm_of_find_panel_or_bridge should not be > > > > called in that situation. > > > > > > > > So all in all it seems that all the pieces are there, unless I'm missing > > > > something. > > > > > > > > What do you think? > > > > > > If Bjorn and Thierry can confirm that it indeeds work in their case, > > > I'll be happy to apply those patches as well. > > > > I still think we'd need a fix for Bjorn's connector case though. > > Not sure I would be confident providing that one without the hardware > > to test with. > > > > Bjorn, what do you think? > > > > I'm okay with the idea that it's up the driver to check that the output > port references an usb-c-connector - either before the call or upon > drm_of_find_panel_or_bridge() returning an error. Actually I'm starting to think might be wrong on this one: there's a display-connector bridge driver that should register a bridge, so drm_of_find_panel_or_bridge should work. Did you have that driver enabled? Cheers, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-27 7:34 ` Paul Kocialkowski @ 2022-05-03 0:03 ` Bjorn Andersson 0 siblings, 0 replies; 41+ messages in thread From: Bjorn Andersson @ 2022-05-03 0:03 UTC (permalink / raw) To: Paul Kocialkowski Cc: Maxime Ripard, Laurent Pinchart, Jagan Teki, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss On Wed, Apr 27, 2022 at 2:34 AM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi Bjorn, > > On Tue 26 Apr 22, 14:10, Bjorn Andersson wrote: > > On Tue 26 Apr 06:50 PDT 2022, Paul Kocialkowski wrote: [..] > > > Bjorn, what do you think? > > > > > > > I'm okay with the idea that it's up the driver to check that the output > > port references an usb-c-connector - either before the call or upon > > drm_of_find_panel_or_bridge() returning an error. > > Actually I'm starting to think might be wrong on this one: there's a > display-connector bridge driver that should register a bridge, so > drm_of_find_panel_or_bridge should work. Did you have that driver enabled? > I don't have this driver enabled, but that seems like it would solve the problem when the remote node is a dp-connector. Unfortunately in my particular case, I have a usb-c-connector. So I don't see that I would be able to reuse this straight off. But I assume that this is trying to say that the USB Type-C code is supposed to provide a bridge for each of the connectors exposed by my USB Type-C controller? I've been building on top of drm_connector_oob_hotplug_event() to achieve this (with the link in the other direction)... Regards, Bjorn ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 12:54 ` Maxime Ripard 2022-04-26 12:55 ` Maxime Ripard @ 2022-04-26 12:58 ` Paul Kocialkowski 2022-04-27 13:10 ` Laurent Pinchart 2 siblings, 0 replies; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-26 12:58 UTC (permalink / raw) To: Maxime Ripard Cc: Laurent Pinchart, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 4347 bytes --] On Tue 26 Apr 22, 14:54, Maxime Ripard wrote: > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > + Linus > > > > > > > + Marek > > > > > > > + Laurent > > > > > > > + Robert > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > case and add one for the child node case, maybe: > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > case. We need to ensure backward-compatibility, but I'd like new > > > bindings (and thus new drivers) to always use OF graph. > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > detection" again and I'm no longer sure there's actually still an issue that > > stands, with the fix that allows returning -ENODEV when possible. > > > > The remaining issue that was brought up was with a connector node, but it should > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > in such situations. > > > > So with that in mind it feels like the child node approach can be viable > > (and integrated in the same helper). > > > > We might still want to favor an explicit OF graph approach, but note that > > dsi-controller.yaml also specifies extra properties that are specific to > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > approach. > > > > What do you think? > > I don't think Laurent's point was to move the child node away from its > DSI controller, that part doesn't make much sense. The panel or bridge > is still accessed through the DSI bus, so it very much belongs there. > > What he meant I think was that we mandate the OF graph for all panels, > so for panels/bridges controlled through DCS, you would still list the > output through the graph. Oh okay that makes sense, thanks. Cheers, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 12:54 ` Maxime Ripard 2022-04-26 12:55 ` Maxime Ripard 2022-04-26 12:58 ` Paul Kocialkowski @ 2022-04-27 13:10 ` Laurent Pinchart 2 siblings, 0 replies; 41+ messages in thread From: Laurent Pinchart @ 2022-04-27 13:10 UTC (permalink / raw) To: Maxime Ripard Cc: Paul Kocialkowski, Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss On Tue, Apr 26, 2022 at 02:54:01PM +0200, Maxime Ripard wrote: > On Tue, Apr 26, 2022 at 02:41:44PM +0200, Paul Kocialkowski wrote: > > On Tue 26 Apr 22, 14:33, Laurent Pinchart wrote: > > > On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > + Linus > > > > > > > + Marek > > > > > > > + Laurent > > > > > > > + Robert > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > case and add one for the child node case, maybe: > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > Are there cases where we could both expect of graph and child node? > > > > (i.e. does the new helper also need to try via of graph?) > > > > > > I still think we should use OF graph uncondtionally, even in the DSI > > > case. We need to ensure backward-compatibility, but I'd like new > > > bindings (and thus new drivers) to always use OF graph. > > > > I just went over the thread on "drm: of: Improve error handling in bridge/panel > > detection" again and I'm no longer sure there's actually still an issue that > > stands, with the fix that allows returning -ENODEV when possible. > > > > The remaining issue that was brought up was with a connector node, but it should > > be up to the driver to detect that and avoid calling drm_of_find_panel_or_bridge > > in such situations. > > > > So with that in mind it feels like the child node approach can be viable > > (and integrated in the same helper). > > > > We might still want to favor an explicit OF graph approach, but note that > > dsi-controller.yaml also specifies extra properties that are specific to > > MIPI DSI and I'm not sure there are equivalent definitions for the OF graph > > approach. > > > > What do you think? > > I don't think Laurent's point was to move the child node away from its > DSI controller, that part doesn't make much sense. The panel or bridge > is still accessed through the DSI bus, so it very much belongs there. > > What he meant I think was that we mandate the OF graph for all panels, > so for panels/bridges controlled through DCS, you would still list the > output through the graph. That's right. A DCS panel would still be a child of the DSI controller. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-26 7:54 ` Paul Kocialkowski 2022-04-26 8:10 ` Jagan Teki 2022-04-26 11:33 ` Laurent Pinchart @ 2022-04-26 12:51 ` Maxime Ripard 2 siblings, 0 replies; 41+ messages in thread From: Maxime Ripard @ 2022-04-26 12:51 UTC (permalink / raw) To: Paul Kocialkowski Cc: Jagan Teki, Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Laurent Pinchart, Robert Foss [-- Attachment #1: Type: text/plain, Size: 2608 bytes --] On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > Hi Maxime, > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > Oh wow I feel stupid for not thinking about that. > > > > Yeah I agree that it seems like the best option. > > Should I prepare a patch with such a new helper? > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > case and add one for the child node case, maybe: > drm_of_find_child_panel_or_bridge. > > I really don't have a clear idea of which driver would need to be switched > over though. Could someone (Jagan?) let me know where it would be needed? > > Are there cases where we could both expect of graph and child node? > (i.e. does the new helper also need to try via of graph?) Yeah, we should figure it out this week. I mentioned this to Dave, who in turn talked about it Linus, so the fastest it's figured out the best. The helper would probably be best, but if you don't have time to do it by then, we can always revert those 3 patches until a helper is there. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-21 8:23 ` Maxime Ripard 2022-04-21 8:59 ` Paul Kocialkowski @ 2022-04-27 6:59 ` Jagan Teki 2022-04-27 11:52 ` Jagan Teki 1 sibling, 1 reply; 41+ messages in thread From: Jagan Teki @ 2022-04-27 6:59 UTC (permalink / raw) To: Maxime Ripard, Bjorn Andersson, Laurent Pinchart, Paul Kocialkowski, Michael Nazzareno Trimarchi Cc: Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > + Linus > > + Marek > > + Laurent > > + Robert > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > bridge")' attempted to simplify the case of expressing a simple panel > > > under a DSI controller, by assuming that the first non-graph child node > > > was a panel or bridge. > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > a reference to the panel. > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > panel or bridge")', in favor of using an explicit graph reference to the > > > panel in the trivial case as well. > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > switched drivers. Do you have any suggestions on how to proceed to > > succeed in those use cases as well? > > I guess we could create a new helper for those, like > devm_drm_of_get_bridge_with_panel, or something. I think using the same existing helper and updating child support is make sense, as there is a possibility to use the same host for child and OF-graph bindings. I can see two possible solutions (as of now) 1. adding "dcs-child-type" bindings for child-based panel or bridge 2. iterate child and skip those nodes other than panel or bridge. or iterate sub-child to find it has a panel or bridge-like aux-bus (which is indeed hard as this configuration seems not 'standard' i think ) Any inputs? Thanks, Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-27 6:59 ` Jagan Teki @ 2022-04-27 11:52 ` Jagan Teki 2022-04-27 12:19 ` Paul Kocialkowski 2022-04-27 13:10 ` Laurent Pinchart 0 siblings, 2 replies; 41+ messages in thread From: Jagan Teki @ 2022-04-27 11:52 UTC (permalink / raw) To: Maxime Ripard, Bjorn Andersson, Laurent Pinchart, Paul Kocialkowski, Michael Nazzareno Trimarchi Cc: Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > + Linus > > > + Marek > > > + Laurent > > > + Robert > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > under a DSI controller, by assuming that the first non-graph child node > > > > was a panel or bridge. > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > a reference to the panel. > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > panel in the trivial case as well. > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > switched drivers. Do you have any suggestions on how to proceed to > > > succeed in those use cases as well? > > > > I guess we could create a new helper for those, like > > devm_drm_of_get_bridge_with_panel, or something. > > I think using the same existing helper and updating child support is > make sense, as there is a possibility to use the same host for child > and OF-graph bindings. > > I can see two possible solutions (as of now) > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > 2. iterate child and skip those nodes other than panel or bridge. or > iterate sub-child to find it has a panel or bridge-like aux-bus (which > is indeed hard as this configuration seems not 'standard' i think ) > > Any inputs? Checking aux-bus with the sub-node panel can be a possible check to look at it, any comments? --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -244,6 +244,25 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, if (panel) *panel = NULL; + /** + * Devices can also be child nodes when we also control that device + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). + * + * Lookup for a child node of the given parent that isn't either port + * or ports. + */ + for_each_available_child_of_node(np, remote) { + if (of_node_name_eq(remote, "port") || + of_node_name_eq(remote, "ports")) + continue; + + if (!(of_node_name_eq(remote, "aux-bus") && + of_get_child_by_name(remote, "panel"))) + continue; + + goto of_find_panel_or_bridge; + } + /* * of_graph_get_remote_node() produces a noisy error message if port * node isn't found and the absence of the port is a legit case here, @@ -254,6 +273,8 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return -ENODEV; remote = of_graph_get_remote_node(np, port, endpoint); + +of_find_panel_or_bridge: if (!remote) return -ENODEV; Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-27 11:52 ` Jagan Teki @ 2022-04-27 12:19 ` Paul Kocialkowski 2022-04-27 12:59 ` Jagan Teki 2022-04-27 13:10 ` Laurent Pinchart 1 sibling, 1 reply; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-27 12:19 UTC (permalink / raw) To: Jagan Teki Cc: Maxime Ripard, Bjorn Andersson, Laurent Pinchart, Michael Nazzareno Trimarchi, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss [-- Attachment #1: Type: text/plain, Size: 4509 bytes --] Hi Jagan, On Wed 27 Apr 22, 17:22, Jagan Teki wrote: > On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > I think using the same existing helper and updating child support is > > make sense, as there is a possibility to use the same host for child > > and OF-graph bindings. > > > > I can see two possible solutions (as of now) > > > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > > 2. iterate child and skip those nodes other than panel or bridge. or > > iterate sub-child to find it has a panel or bridge-like aux-bus (which > > is indeed hard as this configuration seems not 'standard' i think ) > > > > Any inputs? > > Checking aux-bus with the sub-node panel can be a possible check to > look at it, any comments? That looks very fragile and oddly specific. Also why base changes on the original patch that you made? With the follow-up fixes, we are checking the of graph first and only considering child nodes if the of graph and remote are missing, so there isn't really a need to be more specific in the child noise discrimination. Actually I should also make a new version of "drm: of: Improve error handling in bridge/panel detection" to also return -ENODEV if of_graph_get_remote_node fails, so that it doesn't try to use the child node when the graph is defined but not remote is defined. Paul > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -244,6 +244,25 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > if (panel) > *panel = NULL; > > + /** > + * Devices can also be child nodes when we also control that device > + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > + * > + * Lookup for a child node of the given parent that isn't either port > + * or ports. > + */ > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > + continue; > + > + if (!(of_node_name_eq(remote, "aux-bus") && > + of_get_child_by_name(remote, "panel"))) > + continue; > + > + goto of_find_panel_or_bridge; > + } > + > /* > * of_graph_get_remote_node() produces a noisy error message if port > * node isn't found and the absence of the port is a legit case here, > @@ -254,6 +273,8 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > return -ENODEV; > > remote = of_graph_get_remote_node(np, port, endpoint); > + > +of_find_panel_or_bridge: > if (!remote) > return -ENODEV; > > Jagan. -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-27 12:19 ` Paul Kocialkowski @ 2022-04-27 12:59 ` Jagan Teki 0 siblings, 0 replies; 41+ messages in thread From: Jagan Teki @ 2022-04-27 12:59 UTC (permalink / raw) To: Paul Kocialkowski Cc: Maxime Ripard, Bjorn Andersson, Laurent Pinchart, Michael Nazzareno Trimarchi, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss Hi Paul, On Wed, Apr 27, 2022 at 5:49 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi Jagan, > > On Wed 27 Apr 22, 17:22, Jagan Teki wrote: > > On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > + Linus > > > > > + Marek > > > > > + Laurent > > > > > + Robert > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > was a panel or bridge. > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > a reference to the panel. > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > panel in the trivial case as well. > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > succeed in those use cases as well? > > > > > > > > I guess we could create a new helper for those, like > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > I think using the same existing helper and updating child support is > > > make sense, as there is a possibility to use the same host for child > > > and OF-graph bindings. > > > > > > I can see two possible solutions (as of now) > > > > > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > > > 2. iterate child and skip those nodes other than panel or bridge. or > > > iterate sub-child to find it has a panel or bridge-like aux-bus (which > > > is indeed hard as this configuration seems not 'standard' i think ) > > > > > > Any inputs? > > > > Checking aux-bus with the sub-node panel can be a possible check to > > look at it, any comments? > > That looks very fragile and oddly specific. Also why base changes on the > original patch that you made? It is just showcased a snippet to check the child's conditions. > > With the follow-up fixes, we are checking the of graph first and only > considering child nodes if the of graph and remote are missing, so there isn't > really a need to be more specific in the child noise discrimination. So, does it handle child panel or bridge finding? just keep in mind the same call from the host need to handle with and without OF-graph representation. > > Actually I should also make a new version of "drm: of: Improve error handling in > bridge/panel detection" to also return -ENODEV if of_graph_get_remote_node > fails, so that it doesn't try to use the child node when the graph is defined > but not remote is defined. Jagan. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" 2022-04-27 11:52 ` Jagan Teki 2022-04-27 12:19 ` Paul Kocialkowski @ 2022-04-27 13:10 ` Laurent Pinchart 1 sibling, 0 replies; 41+ messages in thread From: Laurent Pinchart @ 2022-04-27 13:10 UTC (permalink / raw) To: Jagan Teki Cc: Maxime Ripard, Bjorn Andersson, Paul Kocialkowski, Michael Nazzareno Trimarchi, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, linux-kernel, dri-devel, Linus Walleij, Marek Szyprowski, Robert Foss On Wed, Apr 27, 2022 at 05:22:32PM +0530, Jagan Teki wrote: > On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki wrote: > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard wrote: > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > I think using the same existing helper and updating child support is > > make sense, as there is a possibility to use the same host for child > > and OF-graph bindings. > > > > I can see two possible solutions (as of now) > > > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > > 2. iterate child and skip those nodes other than panel or bridge. or > > iterate sub-child to find it has a panel or bridge-like aux-bus (which > > is indeed hard as this configuration seems not 'standard' i think ) > > > > Any inputs? > > Checking aux-bus with the sub-node panel can be a possible check to > look at it, any comments? Can we stop piling hacks and move towards OF graph everywhere, please ? > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -244,6 +244,25 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > if (panel) > *panel = NULL; > > + /** > + * Devices can also be child nodes when we also control that device > + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > + * > + * Lookup for a child node of the given parent that isn't either port > + * or ports. > + */ > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > + continue; > + > + if (!(of_node_name_eq(remote, "aux-bus") && > + of_get_child_by_name(remote, "panel"))) > + continue; > + > + goto of_find_panel_or_bridge; > + } > + > /* > * of_graph_get_remote_node() produces a noisy error message if port > * node isn't found and the absence of the port is a legit case here, > @@ -254,6 +273,8 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > return -ENODEV; > > remote = of_graph_get_remote_node(np, port, endpoint); > + > +of_find_panel_or_bridge: > if (!remote) > return -ENODEV; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" 2022-04-20 23:12 [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson 2022-04-20 23:12 ` [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" Bjorn Andersson @ 2022-04-20 23:19 ` Bjorn Andersson 2022-04-21 7:13 ` Paul Kocialkowski 2022-04-21 7:11 ` Paul Kocialkowski 2022-04-21 7:20 ` (subset) " Maxime Ripard 3 siblings, 1 reply; 41+ messages in thread From: Bjorn Andersson @ 2022-04-20 23:19 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Paul Kocialkowski, Rob Clark, Linus Walleij, Jagan Teki Cc: dri-devel, linux-kernel On Wed 20 Apr 16:12 PDT 2022, Bjorn Andersson wrote: Sorry, I missed Jagan and Linus, author and reviewer of the reverted patch 2, among the recipients. Regards, Bjorn > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > bridge")' introduced the ability to describe a panel under a display > controller without having to use a graph to connect the controller to > its single child panel (or bridge). > > The implementation of this would find the first non-graph node and > attempt to acquire the related panel or bridge. This prevents cases > where any other child node, such as a aux bus for a DisplayPort > controller, or an opp-table to find the referenced panel. > > Commit '67bae5f28c89 ("drm: of: Properly try all possible cases for > bridge/panel detection")' attempted to solve this problem by not > bypassing the graph reference lookup before attempting to find the panel > or bridge. > > While this does solve the case where a proper graph reference is > present, it does not allow the caller to distinguish between a > yet-to-be-probed panel or bridge and the absence of a reference to a > panel. > > One such case is a DisplayPort controller that on some boards have an > explicitly described reference to a panel, but on others have a > discoverable DisplayPort display attached (which doesn't need to be > expressed in DeviceTree). > > This reverts commit '67bae5f28c89 ("drm: of: Properly try all possible > cases for bridge/panel detection")', as a step towards reverting commit > '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")'. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++-------------------- > 1 file changed, 49 insertions(+), 50 deletions(-) > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index f4df344509a8..026e4e29a0f3 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -214,29 +214,6 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > } > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > -static int find_panel_or_bridge(struct device_node *node, > - struct drm_panel **panel, > - struct drm_bridge **bridge) > -{ > - if (panel) { > - *panel = of_drm_find_panel(node); > - if (!IS_ERR(*panel)) > - return 0; > - > - /* Clear the panel pointer in case of error. */ > - *panel = NULL; > - } > - > - /* No panel found yet, check for a bridge next. */ > - if (bridge) { > - *bridge = of_drm_find_bridge(node); > - if (*bridge) > - return 0; > - } > - > - return -EPROBE_DEFER; > -} > - > /** > * drm_of_find_panel_or_bridge - return connected panel or bridge device > * @np: device tree node containing encoder output ports > @@ -259,44 +236,66 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > struct drm_panel **panel, > struct drm_bridge **bridge) > { > - struct device_node *node; > - int ret; > + int ret = -EPROBE_DEFER; > + struct device_node *remote; > > if (!panel && !bridge) > return -EINVAL; > - > if (panel) > *panel = NULL; > - if (bridge) > - *bridge = NULL; > - > - /* Check for a graph on the device node first. */ > - if (of_graph_is_present(np)) { > - node = of_graph_get_remote_node(np, port, endpoint); > - if (node) { > - ret = find_panel_or_bridge(node, panel, bridge); > - of_node_put(node); > - > - if (!ret) > - return 0; > - } > - } > > - /* Otherwise check for any child node other than port/ports. */ > - for_each_available_child_of_node(np, node) { > - if (of_node_name_eq(node, "port") || > - of_node_name_eq(node, "ports")) > + /** > + * Devices can also be child nodes when we also control that device > + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > + * > + * Lookup for a child node of the given parent that isn't either port > + * or ports. > + */ > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > continue; > > - ret = find_panel_or_bridge(node, panel, bridge); > - of_node_put(node); > + goto of_find_panel_or_bridge; > + } > + > + /* > + * of_graph_get_remote_node() produces a noisy error message if port > + * node isn't found and the absence of the port is a legit case here, > + * so at first we silently check whether graph presents in the > + * device-tree node. > + */ > + if (!of_graph_is_present(np)) > + return -ENODEV; > + > + remote = of_graph_get_remote_node(np, port, endpoint); > + > +of_find_panel_or_bridge: > + if (!remote) > + return -ENODEV; > + > + if (panel) { > + *panel = of_drm_find_panel(remote); > + if (!IS_ERR(*panel)) > + ret = 0; > + else > + *panel = NULL; > + } > + > + /* No panel found yet, check for a bridge next. */ > + if (bridge) { > + if (ret) { > + *bridge = of_drm_find_bridge(remote); > + if (*bridge) > + ret = 0; > + } else { > + *bridge = NULL; > + } > > - /* Stop at the first found occurrence. */ > - if (!ret) > - return 0; > } > > - return -EPROBE_DEFER; > + of_node_put(remote); > + return ret; > } > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); > > -- > 2.35.1 > ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" 2022-04-20 23:19 ` [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson @ 2022-04-21 7:13 ` Paul Kocialkowski 2022-04-21 7:26 ` Maxime Ripard 2022-04-22 7:58 ` Raphael Gallais-Pou 0 siblings, 2 replies; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-21 7:13 UTC (permalink / raw) To: Bjorn Andersson Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, Linus Walleij, Jagan Teki, dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 6123 bytes --] Hi, On Wed 20 Apr 22, 16:19, Bjorn Andersson wrote: > On Wed 20 Apr 16:12 PDT 2022, Bjorn Andersson wrote: > > Sorry, I missed Jagan and Linus, author and reviewer of the reverted > patch 2, among the recipients. I'd be curious to have Jagan's feedback on why the change was needed in the first place and whether an accepted dt binding relies on it. We might be able to just keep the whole thing reverted (forever). Paul > Regards, > Bjorn > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > bridge")' introduced the ability to describe a panel under a display > > controller without having to use a graph to connect the controller to > > its single child panel (or bridge). > > > > The implementation of this would find the first non-graph node and > > attempt to acquire the related panel or bridge. This prevents cases > > where any other child node, such as a aux bus for a DisplayPort > > controller, or an opp-table to find the referenced panel. > > > > Commit '67bae5f28c89 ("drm: of: Properly try all possible cases for > > bridge/panel detection")' attempted to solve this problem by not > > bypassing the graph reference lookup before attempting to find the panel > > or bridge. > > > > While this does solve the case where a proper graph reference is > > present, it does not allow the caller to distinguish between a > > yet-to-be-probed panel or bridge and the absence of a reference to a > > panel. > > > > One such case is a DisplayPort controller that on some boards have an > > explicitly described reference to a panel, but on others have a > > discoverable DisplayPort display attached (which doesn't need to be > > expressed in DeviceTree). > > > > This reverts commit '67bae5f28c89 ("drm: of: Properly try all possible > > cases for bridge/panel detection")', as a step towards reverting commit > > '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")'. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++-------------------- > > 1 file changed, 49 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > > index f4df344509a8..026e4e29a0f3 100644 > > --- a/drivers/gpu/drm/drm_of.c > > +++ b/drivers/gpu/drm/drm_of.c > > @@ -214,29 +214,6 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > > } > > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > > > -static int find_panel_or_bridge(struct device_node *node, > > - struct drm_panel **panel, > > - struct drm_bridge **bridge) > > -{ > > - if (panel) { > > - *panel = of_drm_find_panel(node); > > - if (!IS_ERR(*panel)) > > - return 0; > > - > > - /* Clear the panel pointer in case of error. */ > > - *panel = NULL; > > - } > > - > > - /* No panel found yet, check for a bridge next. */ > > - if (bridge) { > > - *bridge = of_drm_find_bridge(node); > > - if (*bridge) > > - return 0; > > - } > > - > > - return -EPROBE_DEFER; > > -} > > - > > /** > > * drm_of_find_panel_or_bridge - return connected panel or bridge device > > * @np: device tree node containing encoder output ports > > @@ -259,44 +236,66 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > > struct drm_panel **panel, > > struct drm_bridge **bridge) > > { > > - struct device_node *node; > > - int ret; > > + int ret = -EPROBE_DEFER; > > + struct device_node *remote; > > > > if (!panel && !bridge) > > return -EINVAL; > > - > > if (panel) > > *panel = NULL; > > - if (bridge) > > - *bridge = NULL; > > - > > - /* Check for a graph on the device node first. */ > > - if (of_graph_is_present(np)) { > > - node = of_graph_get_remote_node(np, port, endpoint); > > - if (node) { > > - ret = find_panel_or_bridge(node, panel, bridge); > > - of_node_put(node); > > - > > - if (!ret) > > - return 0; > > - } > > - } > > > > - /* Otherwise check for any child node other than port/ports. */ > > - for_each_available_child_of_node(np, node) { > > - if (of_node_name_eq(node, "port") || > > - of_node_name_eq(node, "ports")) > > + /** > > + * Devices can also be child nodes when we also control that device > > + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > > + * > > + * Lookup for a child node of the given parent that isn't either port > > + * or ports. > > + */ > > + for_each_available_child_of_node(np, remote) { > > + if (of_node_name_eq(remote, "port") || > > + of_node_name_eq(remote, "ports")) > > continue; > > > > - ret = find_panel_or_bridge(node, panel, bridge); > > - of_node_put(node); > > + goto of_find_panel_or_bridge; > > + } > > + > > + /* > > + * of_graph_get_remote_node() produces a noisy error message if port > > + * node isn't found and the absence of the port is a legit case here, > > + * so at first we silently check whether graph presents in the > > + * device-tree node. > > + */ > > + if (!of_graph_is_present(np)) > > + return -ENODEV; > > + > > + remote = of_graph_get_remote_node(np, port, endpoint); > > + > > +of_find_panel_or_bridge: > > + if (!remote) > > + return -ENODEV; > > + > > + if (panel) { > > + *panel = of_drm_find_panel(remote); > > + if (!IS_ERR(*panel)) > > + ret = 0; > > + else > > + *panel = NULL; > > + } > > + > > + /* No panel found yet, check for a bridge next. */ > > + if (bridge) { > > + if (ret) { > > + *bridge = of_drm_find_bridge(remote); > > + if (*bridge) > > + ret = 0; > > + } else { > > + *bridge = NULL; > > + } > > > > - /* Stop at the first found occurrence. */ > > - if (!ret) > > - return 0; > > } > > > > - return -EPROBE_DEFER; > > + of_node_put(remote); > > + return ret; > > } > > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); > > > > -- > > 2.35.1 > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" 2022-04-21 7:13 ` Paul Kocialkowski @ 2022-04-21 7:26 ` Maxime Ripard 2022-04-22 7:58 ` Raphael Gallais-Pou 1 sibling, 0 replies; 41+ messages in thread From: Maxime Ripard @ 2022-04-21 7:26 UTC (permalink / raw) To: Paul Kocialkowski Cc: Bjorn Andersson, Maarten Lankhorst, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, Linus Walleij, Jagan Teki, dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 665 bytes --] On Thu, Apr 21, 2022 at 09:13:02AM +0200, Paul Kocialkowski wrote: > Hi, > > On Wed 20 Apr 22, 16:19, Bjorn Andersson wrote: > > On Wed 20 Apr 16:12 PDT 2022, Bjorn Andersson wrote: > > > > Sorry, I missed Jagan and Linus, author and reviewer of the reverted > > patch 2, among the recipients. > > I'd be curious to have Jagan's feedback on why the change was needed in the > first place and whether an accepted dt binding relies on it. > > We might be able to just keep the whole thing reverted (forever). I was the one that asked for it because I thought this would be more convenient for everyone. Turns out I was very wrong :) Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" 2022-04-21 7:13 ` Paul Kocialkowski 2022-04-21 7:26 ` Maxime Ripard @ 2022-04-22 7:58 ` Raphael Gallais-Pou 1 sibling, 0 replies; 41+ messages in thread From: Raphael Gallais-Pou @ 2022-04-22 7:58 UTC (permalink / raw) To: Paul Kocialkowski, Bjorn Andersson Cc: dri-devel, Thomas Zimmermann, David Airlie, linux-kernel, Thierry Reding, Jagan Teki Hi On 4/21/22 09:13, Paul Kocialkowski wrote: > Hi, > > On Wed 20 Apr 22, 16:19, Bjorn Andersson wrote: >> On Wed 20 Apr 16:12 PDT 2022, Bjorn Andersson wrote: >> >> Sorry, I missed Jagan and Linus, author and reviewer of the reverted >> patch 2, among the recipients. > I'd be curious to have Jagan's feedback on why the change was needed in the > first place and whether an accepted dt binding relies on it. > > We might be able to just keep the whole thing reverted (forever). This patch was merged within the last mainline kernel and was breaking our STM32MP15 platforms. Switching back to drm-based kernel (drm-next-fixes branch) made me realize this patch was faulty. I'm glad it was reverted. Thanks, Raphaël > > Paul > >> Regards, >> Bjorn >> >>> Commit '80253168dbfd ("drm: of: Lookup if child node has panel or >>> bridge")' introduced the ability to describe a panel under a display >>> controller without having to use a graph to connect the controller to >>> its single child panel (or bridge). >>> >>> The implementation of this would find the first non-graph node and >>> attempt to acquire the related panel or bridge. This prevents cases >>> where any other child node, such as a aux bus for a DisplayPort >>> controller, or an opp-table to find the referenced panel. >>> >>> Commit '67bae5f28c89 ("drm: of: Properly try all possible cases for >>> bridge/panel detection")' attempted to solve this problem by not >>> bypassing the graph reference lookup before attempting to find the panel >>> or bridge. >>> >>> While this does solve the case where a proper graph reference is >>> present, it does not allow the caller to distinguish between a >>> yet-to-be-probed panel or bridge and the absence of a reference to a >>> panel. >>> >>> One such case is a DisplayPort controller that on some boards have an >>> explicitly described reference to a panel, but on others have a >>> discoverable DisplayPort display attached (which doesn't need to be >>> expressed in DeviceTree). >>> >>> This reverts commit '67bae5f28c89 ("drm: of: Properly try all possible >>> cases for bridge/panel detection")', as a step towards reverting commit >>> '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")'. >>> >>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> >>> --- >>> drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++-------------------- >>> 1 file changed, 49 insertions(+), 50 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >>> index f4df344509a8..026e4e29a0f3 100644 >>> --- a/drivers/gpu/drm/drm_of.c >>> +++ b/drivers/gpu/drm/drm_of.c >>> @@ -214,29 +214,6 @@ int drm_of_encoder_active_endpoint(struct device_node *node, >>> } >>> EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); >>> >>> -static int find_panel_or_bridge(struct device_node *node, >>> - struct drm_panel **panel, >>> - struct drm_bridge **bridge) >>> -{ >>> - if (panel) { >>> - *panel = of_drm_find_panel(node); >>> - if (!IS_ERR(*panel)) >>> - return 0; >>> - >>> - /* Clear the panel pointer in case of error. */ >>> - *panel = NULL; >>> - } >>> - >>> - /* No panel found yet, check for a bridge next. */ >>> - if (bridge) { >>> - *bridge = of_drm_find_bridge(node); >>> - if (*bridge) >>> - return 0; >>> - } >>> - >>> - return -EPROBE_DEFER; >>> -} >>> - >>> /** >>> * drm_of_find_panel_or_bridge - return connected panel or bridge device >>> * @np: device tree node containing encoder output ports >>> @@ -259,44 +236,66 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, >>> struct drm_panel **panel, >>> struct drm_bridge **bridge) >>> { >>> - struct device_node *node; >>> - int ret; >>> + int ret = -EPROBE_DEFER; >>> + struct device_node *remote; >>> >>> if (!panel && !bridge) >>> return -EINVAL; >>> - >>> if (panel) >>> *panel = NULL; >>> - if (bridge) >>> - *bridge = NULL; >>> - >>> - /* Check for a graph on the device node first. */ >>> - if (of_graph_is_present(np)) { >>> - node = of_graph_get_remote_node(np, port, endpoint); >>> - if (node) { >>> - ret = find_panel_or_bridge(node, panel, bridge); >>> - of_node_put(node); >>> - >>> - if (!ret) >>> - return 0; >>> - } >>> - } >>> >>> - /* Otherwise check for any child node other than port/ports. */ >>> - for_each_available_child_of_node(np, node) { >>> - if (of_node_name_eq(node, "port") || >>> - of_node_name_eq(node, "ports")) >>> + /** >>> + * Devices can also be child nodes when we also control that device >>> + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). >>> + * >>> + * Lookup for a child node of the given parent that isn't either port >>> + * or ports. >>> + */ >>> + for_each_available_child_of_node(np, remote) { >>> + if (of_node_name_eq(remote, "port") || >>> + of_node_name_eq(remote, "ports")) >>> continue; >>> >>> - ret = find_panel_or_bridge(node, panel, bridge); >>> - of_node_put(node); >>> + goto of_find_panel_or_bridge; >>> + } >>> + >>> + /* >>> + * of_graph_get_remote_node() produces a noisy error message if port >>> + * node isn't found and the absence of the port is a legit case here, >>> + * so at first we silently check whether graph presents in the >>> + * device-tree node. >>> + */ >>> + if (!of_graph_is_present(np)) >>> + return -ENODEV; >>> + >>> + remote = of_graph_get_remote_node(np, port, endpoint); >>> + >>> +of_find_panel_or_bridge: >>> + if (!remote) >>> + return -ENODEV; >>> + >>> + if (panel) { >>> + *panel = of_drm_find_panel(remote); >>> + if (!IS_ERR(*panel)) >>> + ret = 0; >>> + else >>> + *panel = NULL; >>> + } >>> + >>> + /* No panel found yet, check for a bridge next. */ >>> + if (bridge) { >>> + if (ret) { >>> + *bridge = of_drm_find_bridge(remote); >>> + if (*bridge) >>> + ret = 0; >>> + } else { >>> + *bridge = NULL; >>> + } >>> >>> - /* Stop at the first found occurrence. */ >>> - if (!ret) >>> - return 0; >>> } >>> >>> - return -EPROBE_DEFER; >>> + of_node_put(remote); >>> + return ret; >>> } >>> EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); >>> >>> -- >>> 2.35.1 >>> ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" 2022-04-20 23:12 [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson 2022-04-20 23:12 ` [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" Bjorn Andersson 2022-04-20 23:19 ` [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson @ 2022-04-21 7:11 ` Paul Kocialkowski 2022-04-21 7:20 ` (subset) " Maxime Ripard 3 siblings, 0 replies; 41+ messages in thread From: Paul Kocialkowski @ 2022-04-21 7:11 UTC (permalink / raw) To: Bjorn Andersson Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Daniel Vetter, Thierry Reding, Rob Clark, dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5472 bytes --] Hi Bjorn, On Wed 20 Apr 22, 16:12, Bjorn Andersson wrote: > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > bridge")' introduced the ability to describe a panel under a display > controller without having to use a graph to connect the controller to > its single child panel (or bridge). > > The implementation of this would find the first non-graph node and > attempt to acquire the related panel or bridge. This prevents cases > where any other child node, such as a aux bus for a DisplayPort > controller, or an opp-table to find the referenced panel. > > Commit '67bae5f28c89 ("drm: of: Properly try all possible cases for > bridge/panel detection")' attempted to solve this problem by not > bypassing the graph reference lookup before attempting to find the panel > or bridge. > > While this does solve the case where a proper graph reference is > present, it does not allow the caller to distinguish between a > yet-to-be-probed panel or bridge and the absence of a reference to a > panel. > > One such case is a DisplayPort controller that on some boards have an > explicitly described reference to a panel, but on others have a > discoverable DisplayPort display attached (which doesn't need to be > expressed in DeviceTree). > > This reverts commit '67bae5f28c89 ("drm: of: Properly try all possible > cases for bridge/panel detection")', as a step towards reverting commit > '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")'. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cheers, Paul > --- > drivers/gpu/drm/drm_of.c | 99 ++++++++++++++++++++-------------------- > 1 file changed, 49 insertions(+), 50 deletions(-) > > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c > index f4df344509a8..026e4e29a0f3 100644 > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -214,29 +214,6 @@ int drm_of_encoder_active_endpoint(struct device_node *node, > } > EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); > > -static int find_panel_or_bridge(struct device_node *node, > - struct drm_panel **panel, > - struct drm_bridge **bridge) > -{ > - if (panel) { > - *panel = of_drm_find_panel(node); > - if (!IS_ERR(*panel)) > - return 0; > - > - /* Clear the panel pointer in case of error. */ > - *panel = NULL; > - } > - > - /* No panel found yet, check for a bridge next. */ > - if (bridge) { > - *bridge = of_drm_find_bridge(node); > - if (*bridge) > - return 0; > - } > - > - return -EPROBE_DEFER; > -} > - > /** > * drm_of_find_panel_or_bridge - return connected panel or bridge device > * @np: device tree node containing encoder output ports > @@ -259,44 +236,66 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, > struct drm_panel **panel, > struct drm_bridge **bridge) > { > - struct device_node *node; > - int ret; > + int ret = -EPROBE_DEFER; > + struct device_node *remote; > > if (!panel && !bridge) > return -EINVAL; > - > if (panel) > *panel = NULL; > - if (bridge) > - *bridge = NULL; > - > - /* Check for a graph on the device node first. */ > - if (of_graph_is_present(np)) { > - node = of_graph_get_remote_node(np, port, endpoint); > - if (node) { > - ret = find_panel_or_bridge(node, panel, bridge); > - of_node_put(node); > - > - if (!ret) > - return 0; > - } > - } > > - /* Otherwise check for any child node other than port/ports. */ > - for_each_available_child_of_node(np, node) { > - if (of_node_name_eq(node, "port") || > - of_node_name_eq(node, "ports")) > + /** > + * Devices can also be child nodes when we also control that device > + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > + * > + * Lookup for a child node of the given parent that isn't either port > + * or ports. > + */ > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > continue; > > - ret = find_panel_or_bridge(node, panel, bridge); > - of_node_put(node); > + goto of_find_panel_or_bridge; > + } > + > + /* > + * of_graph_get_remote_node() produces a noisy error message if port > + * node isn't found and the absence of the port is a legit case here, > + * so at first we silently check whether graph presents in the > + * device-tree node. > + */ > + if (!of_graph_is_present(np)) > + return -ENODEV; > + > + remote = of_graph_get_remote_node(np, port, endpoint); > + > +of_find_panel_or_bridge: > + if (!remote) > + return -ENODEV; > + > + if (panel) { > + *panel = of_drm_find_panel(remote); > + if (!IS_ERR(*panel)) > + ret = 0; > + else > + *panel = NULL; > + } > + > + /* No panel found yet, check for a bridge next. */ > + if (bridge) { > + if (ret) { > + *bridge = of_drm_find_bridge(remote); > + if (*bridge) > + ret = 0; > + } else { > + *bridge = NULL; > + } > > - /* Stop at the first found occurrence. */ > - if (!ret) > - return 0; > } > > - return -EPROBE_DEFER; > + of_node_put(remote); > + return ret; > } > EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); > > -- > 2.35.1 > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: (subset) [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" 2022-04-20 23:12 [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson ` (2 preceding siblings ...) 2022-04-21 7:11 ` Paul Kocialkowski @ 2022-04-21 7:20 ` Maxime Ripard 3 siblings, 0 replies; 41+ messages in thread From: Maxime Ripard @ 2022-04-21 7:20 UTC (permalink / raw) To: Maarten Lankhorst, Daniel Vetter, Thierry Reding, David Airlie, Thomas Zimmermann, Bjorn Andersson, Paul Kocialkowski, Maxime Ripard, Rob Clark Cc: Maxime Ripard, linux-kernel, dri-devel On Wed, 20 Apr 2022 16:12:29 -0700, Bjorn Andersson wrote: > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > bridge")' introduced the ability to describe a panel under a display > controller without having to use a graph to connect the controller to > its single child panel (or bridge). > > The implementation of this would find the first non-graph node and > attempt to acquire the related panel or bridge. This prevents cases > where any other child node, such as a aux bus for a DisplayPort > controller, or an opp-table to find the referenced panel. > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2022-05-04 15:09 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-20 23:12 [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson 2022-04-20 23:12 ` [PATCH 2/2] Revert "drm: of: Lookup if child node has panel or bridge" Bjorn Andersson 2022-04-21 7:20 ` (subset) " Maxime Ripard 2022-04-21 7:45 ` Jagan Teki 2022-04-21 8:23 ` Maxime Ripard 2022-04-21 8:59 ` Paul Kocialkowski 2022-04-26 7:54 ` Paul Kocialkowski 2022-04-26 8:10 ` Jagan Teki 2022-04-27 14:34 ` Maxime Ripard 2022-04-28 6:17 ` Marek Szyprowski 2022-04-28 8:25 ` Jagan Teki 2022-04-28 8:39 ` Jagan Teki 2022-04-28 22:17 ` Laurent Pinchart 2022-04-29 8:24 ` Jagan Teki 2022-04-29 15:46 ` Maxime Ripard 2022-04-29 16:05 ` Laurent Pinchart 2022-05-04 15:08 ` Maxime Ripard 2022-04-26 11:33 ` Laurent Pinchart 2022-04-26 12:41 ` Paul Kocialkowski 2022-04-26 12:54 ` Maxime Ripard 2022-04-26 12:55 ` Maxime Ripard 2022-04-26 13:04 ` Paul Kocialkowski 2022-04-26 13:19 ` Maxime Ripard 2022-04-26 13:50 ` Paul Kocialkowski 2022-04-26 21:10 ` Bjorn Andersson 2022-04-27 7:34 ` Paul Kocialkowski 2022-05-03 0:03 ` Bjorn Andersson 2022-04-26 12:58 ` Paul Kocialkowski 2022-04-27 13:10 ` Laurent Pinchart 2022-04-26 12:51 ` Maxime Ripard 2022-04-27 6:59 ` Jagan Teki 2022-04-27 11:52 ` Jagan Teki 2022-04-27 12:19 ` Paul Kocialkowski 2022-04-27 12:59 ` Jagan Teki 2022-04-27 13:10 ` Laurent Pinchart 2022-04-20 23:19 ` [PATCH 1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" Bjorn Andersson 2022-04-21 7:13 ` Paul Kocialkowski 2022-04-21 7:26 ` Maxime Ripard 2022-04-22 7:58 ` Raphael Gallais-Pou 2022-04-21 7:11 ` Paul Kocialkowski 2022-04-21 7:20 ` (subset) " Maxime Ripard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).