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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Paul Kocialkowski <[email protected]>
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
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
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
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
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
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
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
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
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
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
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
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
Hi Marek,
On Thu, Apr 28, 2022 at 11:47 AM Marek Szyprowski
<[email protected]> 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
> >> <[email protected]> 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
> >>>>>> <[email protected]> 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.
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
Hi Laurent,
On Fri, Apr 29, 2022 at 3:47 AM Laurent Pinchart
<[email protected]> 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/[email protected]/
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.
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
On Wed, Apr 27, 2022 at 2:34 AM Paul Kocialkowski
<[email protected]> 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
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