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.
Maxime
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
> > <[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.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
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
> > > <[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?
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
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
> > > > <[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?
>
> 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
On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <[email protected]> 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.
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.
On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski
<[email protected]> 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
> > > > <[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, 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.
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
On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <[email protected]> wrote:
>
> On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <[email protected]> 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.
>
> 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.
Hi Jagan,
On Wed 27 Apr 22, 17:22, Jagan Teki wrote:
> On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <[email protected]> wrote:
> >
> > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <[email protected]> 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.
> >
> > 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
Hi Paul,
On Wed, Apr 27, 2022 at 5:49 PM Paul Kocialkowski
<[email protected]> wrote:
>
> Hi Jagan,
>
> On Wed 27 Apr 22, 17:22, Jagan Teki wrote:
> > On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <[email protected]> wrote:
> > >
> > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <[email protected]> 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.
> > >
> > > 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.
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
> > > > <[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.
> >
> > 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
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:
> >
> > 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
> > > > > <[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?
> mcde_dsi (as of now)
Yeah, we do need to revert 3730bc6147b0 and 3d7039e1e649
Maxime
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().
>> 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
On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard <[email protected]> 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:
> > >
> > > 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
> > > > > > <[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?
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.
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