| Message ID | 20220420231230.58499-2-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] Revert "drm: of: Properly try all possible cases for bridge/panel detection" | expand |
On Wed, 20 Apr 2022 16:12:30 -0700, Bjorn Andersson wrote: > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > bridge")' attempted to simplify the case of expressing a simple panel > under a DSI controller, by assuming that the first non-graph child node > was a panel or bridge. > > Unfortunately for non-trivial cases the first child node might not be a > panel or bridge. Examples of this can be a aux-bus in the case of > DisplayPort, or an opp-table represented before the panel node. > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime
+ Linus + Marek + Laurent + Robert On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > bridge")' attempted to simplify the case of expressing a simple panel > under a DSI controller, by assuming that the first non-graph child node > was a panel or bridge. > > Unfortunately for non-trivial cases the first child node might not be a > panel or bridge. Examples of this can be a aux-bus in the case of > DisplayPort, or an opp-table represented before the panel node. > > In these cases the reverted commit prevents the caller from ever finding > a reference to the panel. > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > panel or bridge")', in favor of using an explicit graph reference to the > panel in the trivial case as well. This eventually breaks many child-based devm_drm_of_get_bridge switched drivers. Do you have any suggestions on how to proceed to succeed in those use cases as well? Jagan.
On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > + Linus > + Marek > + Laurent > + Robert > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > bridge")' attempted to simplify the case of expressing a simple panel > > under a DSI controller, by assuming that the first non-graph child node > > was a panel or bridge. > > > > Unfortunately for non-trivial cases the first child node might not be a > > panel or bridge. Examples of this can be a aux-bus in the case of > > DisplayPort, or an opp-table represented before the panel node. > > > > In these cases the reverted commit prevents the caller from ever finding > > a reference to the panel. > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > panel or bridge")', in favor of using an explicit graph reference to the > > panel in the trivial case as well. > > This eventually breaks many child-based devm_drm_of_get_bridge > switched drivers. Do you have any suggestions on how to proceed to > succeed in those use cases as well? I guess we could create a new helper for those, like devm_drm_of_get_bridge_with_panel, or something. Maxime
Hi Maxime, On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > + Linus > > + Marek > > + Laurent > > + Robert > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > bridge")' attempted to simplify the case of expressing a simple panel > > > under a DSI controller, by assuming that the first non-graph child node > > > was a panel or bridge. > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > a reference to the panel. > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > panel or bridge")', in favor of using an explicit graph reference to the > > > panel in the trivial case as well. > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > switched drivers. Do you have any suggestions on how to proceed to > > succeed in those use cases as well? > > I guess we could create a new helper for those, like > devm_drm_of_get_bridge_with_panel, or something. Oh wow I feel stupid for not thinking about that. Yeah I agree that it seems like the best option. Cheers, Paul
Hi, On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > Hi Maxime, > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > + Linus > > > + Marek > > > + Laurent > > > + Robert > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > under a DSI controller, by assuming that the first non-graph child node > > > > was a panel or bridge. > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > a reference to the panel. > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > panel in the trivial case as well. > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > switched drivers. Do you have any suggestions on how to proceed to > > > succeed in those use cases as well? > > > > I guess we could create a new helper for those, like > > devm_drm_of_get_bridge_with_panel, or something. > > Oh wow I feel stupid for not thinking about that. > > Yeah I agree that it seems like the best option. Should I prepare a patch with such a new helper? The idea would be to keep drm_of_find_panel_or_bridge only for the of graph case and add one for the child node case, maybe: drm_of_find_child_panel_or_bridge. I really don't have a clear idea of which driver would need to be switched over though. Could someone (Jagan?) let me know where it would be needed? Are there cases where we could both expect of graph and child node? (i.e. does the new helper also need to try via of graph?) Thanks, Paul
On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi, > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > Hi Maxime, > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > Oh wow I feel stupid for not thinking about that. > > > > Yeah I agree that it seems like the best option. > > Should I prepare a patch with such a new helper? > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > case and add one for the child node case, maybe: > drm_of_find_child_panel_or_bridge. > > I really don't have a clear idea of which driver would need to be switched > over though. Could someone (Jagan?) let me know where it would be needed? sun6i_mipi_dsi, exynos_drm_dsi, mcde_dsi (as of now) > > Are there cases where we could both expect of graph and child node? > (i.e. does the new helper also need to try via of graph?) One finding so far from my side would be if the check iterates the child and identify the panel or bridge child irrespective of the position it has and untouched non-trivial child-like dp, opp-table can help to use same change what we have it before. Still working on getting a proper check. Thanks, Jagan.
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.
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
On Tue, Apr 26, 2022 at 09:54:36AM +0200, Paul Kocialkowski wrote: > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > Hi Maxime, > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > Oh wow I feel stupid for not thinking about that. > > > > Yeah I agree that it seems like the best option. > > Should I prepare a patch with such a new helper? > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > case and add one for the child node case, maybe: > drm_of_find_child_panel_or_bridge. > > I really don't have a clear idea of which driver would need to be switched > over though. Could someone (Jagan?) let me know where it would be needed? > > Are there cases where we could both expect of graph and child node? > (i.e. does the new helper also need to try via of graph?) Yeah, we should figure it out this week. I mentioned this to Dave, who in turn talked about it Linus, so the fastest it's figured out the best. The helper would probably be best, but if you don't have time to do it by then, we can always revert those 3 patches until a helper is there. Maxime
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, 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
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
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 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
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 Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > + Linus > > + Marek > > + Laurent > > + Robert > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > <bjorn.andersson@linaro.org> wrote: > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > bridge")' attempted to simplify the case of expressing a simple panel > > > under a DSI controller, by assuming that the first non-graph child node > > > was a panel or bridge. > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > a reference to the panel. > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > panel or bridge")', in favor of using an explicit graph reference to the > > > panel in the trivial case as well. > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > switched drivers. Do you have any suggestions on how to proceed to > > succeed in those use cases as well? > > I guess we could create a new helper for those, like > devm_drm_of_get_bridge_with_panel, or something. I think using the same existing helper and updating child support is make sense, as there is a possibility to use the same host for child and OF-graph bindings. I can see two possible solutions (as of now) 1. adding "dcs-child-type" bindings for child-based panel or bridge 2. iterate child and skip those nodes other than panel or bridge. or iterate sub-child to find it has a panel or bridge-like aux-bus (which is indeed hard as this configuration seems not 'standard' i think ) Any inputs? Thanks, Jagan.
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
On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > + Linus > > > + Marek > > > + Laurent > > > + Robert > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > under a DSI controller, by assuming that the first non-graph child node > > > > was a panel or bridge. > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > a reference to the panel. > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > panel in the trivial case as well. > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > switched drivers. Do you have any suggestions on how to proceed to > > > succeed in those use cases as well? > > > > I guess we could create a new helper for those, like > > devm_drm_of_get_bridge_with_panel, or something. > > I think using the same existing helper and updating child support is > make sense, as there is a possibility to use the same host for child > and OF-graph bindings. > > I can see two possible solutions (as of now) > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > 2. iterate child and skip those nodes other than panel or bridge. or > iterate sub-child to find it has a panel or bridge-like aux-bus (which > is indeed hard as this configuration seems not 'standard' i think ) > > Any inputs? Checking aux-bus with the sub-node panel can be a possible check to look at it, any comments? --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -244,6 +244,25 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, if (panel) *panel = NULL; + /** + * Devices can also be child nodes when we also control that device + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). + * + * Lookup for a child node of the given parent that isn't either port + * or ports. + */ + for_each_available_child_of_node(np, remote) { + if (of_node_name_eq(remote, "port") || + of_node_name_eq(remote, "ports")) + continue; + + if (!(of_node_name_eq(remote, "aux-bus") && + of_get_child_by_name(remote, "panel"))) + continue; + + goto of_find_panel_or_bridge; + } + /* * of_graph_get_remote_node() produces a noisy error message if port * node isn't found and the absence of the port is a legit case here, @@ -254,6 +273,8 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, return -ENODEV; remote = of_graph_get_remote_node(np, port, endpoint); + +of_find_panel_or_bridge: if (!remote) return -ENODEV; Jagan.
Hi Jagan, On Wed 27 Apr 22, 17:22, Jagan Teki wrote: > On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > I think using the same existing helper and updating child support is > > make sense, as there is a possibility to use the same host for child > > and OF-graph bindings. > > > > I can see two possible solutions (as of now) > > > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > > 2. iterate child and skip those nodes other than panel or bridge. or > > iterate sub-child to find it has a panel or bridge-like aux-bus (which > > is indeed hard as this configuration seems not 'standard' i think ) > > > > Any inputs? > > Checking aux-bus with the sub-node panel can be a possible check to > look at it, any comments? That looks very fragile and oddly specific. Also why base changes on the original patch that you made? With the follow-up fixes, we are checking the of graph first and only considering child nodes if the of graph and remote are missing, so there isn't really a need to be more specific in the child noise discrimination. Actually I should also make a new version of "drm: of: Improve error handling in bridge/panel detection" to also return -ENODEV if of_graph_get_remote_node fails, so that it doesn't try to use the child node when the graph is defined but not remote is defined. Paul > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -244,6 +244,25 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > if (panel) > *panel = NULL; > > + /** > + * Devices can also be child nodes when we also control that device > + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > + * > + * Lookup for a child node of the given parent that isn't either port > + * or ports. > + */ > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > + continue; > + > + if (!(of_node_name_eq(remote, "aux-bus") && > + of_get_child_by_name(remote, "panel"))) > + continue; > + > + goto of_find_panel_or_bridge; > + } > + > /* > * of_graph_get_remote_node() produces a noisy error message if port > * node isn't found and the absence of the port is a legit case here, > @@ -254,6 +273,8 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > return -ENODEV; > > remote = of_graph_get_remote_node(np, port, endpoint); > + > +of_find_panel_or_bridge: > if (!remote) > return -ENODEV; > > Jagan.
Hi Paul, On Wed, Apr 27, 2022 at 5:49 PM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi Jagan, > > On Wed 27 Apr 22, 17:22, Jagan Teki wrote: > > On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > + Linus > > > > > + Marek > > > > > + Laurent > > > > > + Robert > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > was a panel or bridge. > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > a reference to the panel. > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > panel in the trivial case as well. > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > succeed in those use cases as well? > > > > > > > > I guess we could create a new helper for those, like > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > I think using the same existing helper and updating child support is > > > make sense, as there is a possibility to use the same host for child > > > and OF-graph bindings. > > > > > > I can see two possible solutions (as of now) > > > > > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > > > 2. iterate child and skip those nodes other than panel or bridge. or > > > iterate sub-child to find it has a panel or bridge-like aux-bus (which > > > is indeed hard as this configuration seems not 'standard' i think ) > > > > > > Any inputs? > > > > Checking aux-bus with the sub-node panel can be a possible check to > > look at it, any comments? > > That looks very fragile and oddly specific. Also why base changes on the > original patch that you made? It is just showcased a snippet to check the child's conditions. > > With the follow-up fixes, we are checking the of graph first and only > considering child nodes if the of graph and remote are missing, so there isn't > really a need to be more specific in the child noise discrimination. So, does it handle child panel or bridge finding? just keep in mind the same call from the host need to handle with and without OF-graph representation. > > Actually I should also make a new version of "drm: of: Improve error handling in > bridge/panel detection" to also return -ENODEV if of_graph_get_remote_node > fails, so that it doesn't try to use the child node when the graph is defined > but not remote is defined. Jagan.
On Wed, Apr 27, 2022 at 05:22:32PM +0530, Jagan Teki wrote: > On Wed, Apr 27, 2022 at 12:29 PM Jagan Teki wrote: > > On Thu, Apr 21, 2022 at 1:54 PM Maxime Ripard wrote: > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > + Linus > > > > + Marek > > > > + Laurent > > > > + Robert > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > was a panel or bridge. > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > a reference to the panel. > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > panel in the trivial case as well. > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > succeed in those use cases as well? > > > > > > I guess we could create a new helper for those, like > > > devm_drm_of_get_bridge_with_panel, or something. > > > > I think using the same existing helper and updating child support is > > make sense, as there is a possibility to use the same host for child > > and OF-graph bindings. > > > > I can see two possible solutions (as of now) > > > > 1. adding "dcs-child-type" bindings for child-based panel or bridge > > 2. iterate child and skip those nodes other than panel or bridge. or > > iterate sub-child to find it has a panel or bridge-like aux-bus (which > > is indeed hard as this configuration seems not 'standard' i think ) > > > > Any inputs? > > Checking aux-bus with the sub-node panel can be a possible check to > look at it, any comments? Can we stop piling hacks and move towards OF graph everywhere, please ? > --- a/drivers/gpu/drm/drm_of.c > +++ b/drivers/gpu/drm/drm_of.c > @@ -244,6 +244,25 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > if (panel) > *panel = NULL; > > + /** > + * Devices can also be child nodes when we also control that device > + * through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). > + * > + * Lookup for a child node of the given parent that isn't either port > + * or ports. > + */ > + for_each_available_child_of_node(np, remote) { > + if (of_node_name_eq(remote, "port") || > + of_node_name_eq(remote, "ports")) > + continue; > + > + if (!(of_node_name_eq(remote, "aux-bus") && > + of_get_child_by_name(remote, "panel"))) > + continue; > + > + goto of_find_panel_or_bridge; > + } > + > /* > * of_graph_get_remote_node() produces a noisy error message if port > * node isn't found and the absence of the port is a legit case here, > @@ -254,6 +273,8 @@ int drm_of_find_panel_or_bridge(const struct > device_node *np, > return -ENODEV; > > remote = of_graph_get_remote_node(np, port, endpoint); > + > +of_find_panel_or_bridge: > if (!remote) > return -ENODEV;
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.
On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski > <paul.kocialkowski@bootlin.com> wrote: > > > > Hi, > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > Hi Maxime, > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > + Linus > > > > > + Marek > > > > > + Laurent > > > > > + Robert > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > was a panel or bridge. > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > a reference to the panel. > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > panel in the trivial case as well. > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > succeed in those use cases as well? > > > > > > > > I guess we could create a new helper for those, like > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > Yeah I agree that it seems like the best option. > > > > Should I prepare a patch with such a new helper? > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > case and add one for the child node case, maybe: > > drm_of_find_child_panel_or_bridge. > > > > I really don't have a clear idea of which driver would need to be switched > > over though. Could someone (Jagan?) let me know where it would be needed? > > sun6i_mipi_dsi It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > exynos_drm_dsi If you reference 711c7adc4687, I don't see why we would need to switch it back to the old behaviour. It wasn't iterating over its child node before, so what does the switch to drm_of_get_bridge broke exactly? > mcde_dsi (as of now) Yeah, we do need to revert 3730bc6147b0 and 3d7039e1e649 Maxime
Hi Maxime, On 27.04.2022 16:34, Maxime Ripard wrote: > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: >> On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski >> <paul.kocialkowski@bootlin.com> wrote: >>> On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: >>>> On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: >>>>> On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: >>>>>> + Linus >>>>>> + Marek >>>>>> + Laurent >>>>>> + Robert >>>>>> >>>>>> On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson >>>>>> <bjorn.andersson@linaro.org> wrote: >>>>>>> Commit '80253168dbfd ("drm: of: Lookup if child node has panel or >>>>>>> bridge")' attempted to simplify the case of expressing a simple panel >>>>>>> under a DSI controller, by assuming that the first non-graph child node >>>>>>> was a panel or bridge. >>>>>>> >>>>>>> Unfortunately for non-trivial cases the first child node might not be a >>>>>>> panel or bridge. Examples of this can be a aux-bus in the case of >>>>>>> DisplayPort, or an opp-table represented before the panel node. >>>>>>> >>>>>>> In these cases the reverted commit prevents the caller from ever finding >>>>>>> a reference to the panel. >>>>>>> >>>>>>> This reverts commit '80253168dbfd ("drm: of: Lookup if child node has >>>>>>> panel or bridge")', in favor of using an explicit graph reference to the >>>>>>> panel in the trivial case as well. >>>>>> This eventually breaks many child-based devm_drm_of_get_bridge >>>>>> switched drivers. Do you have any suggestions on how to proceed to >>>>>> succeed in those use cases as well? >>>>> I guess we could create a new helper for those, like >>>>> devm_drm_of_get_bridge_with_panel, or something. >>>> Oh wow I feel stupid for not thinking about that. >>>> >>>> Yeah I agree that it seems like the best option. >>> Should I prepare a patch with such a new helper? >>> >>> The idea would be to keep drm_of_find_panel_or_bridge only for the of graph >>> case and add one for the child node case, maybe: >>> drm_of_find_child_panel_or_bridge. >>> >>> I really don't have a clear idea of which driver would need to be switched >>> over though. Could someone (Jagan?) let me know where it would be needed? >> sun6i_mipi_dsi > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > >> exynos_drm_dsi > If you reference 711c7adc4687, I don't see why we would need to switch > it back to the old behaviour. It wasn't iterating over its child node > before, so what does the switch to drm_of_get_bridge broke exactly? It broke getting the panel if it is a direct child of the DSI device node. It worked before because it used following code: dsi->panel = of_drm_find_panel(device->dev.of_node); which got replaced by devm_drm_of_get_bridge(). >> mcde_dsi (as of now) > Yeah, we do need to revert 3730bc6147b0 and 3d7039e1e649 > > Maxime > Best regards
Hi Marek, On Thu, Apr 28, 2022 at 11:47 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi Maxime, > > On 27.04.2022 16:34, Maxime Ripard wrote: > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > >> On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski > >> <paul.kocialkowski@bootlin.com> wrote: > >>> On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > >>>> On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > >>>>> On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > >>>>>> + Linus > >>>>>> + Marek > >>>>>> + Laurent > >>>>>> + Robert > >>>>>> > >>>>>> On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > >>>>>> <bjorn.andersson@linaro.org> wrote: > >>>>>>> Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > >>>>>>> bridge")' attempted to simplify the case of expressing a simple panel > >>>>>>> under a DSI controller, by assuming that the first non-graph child node > >>>>>>> was a panel or bridge. > >>>>>>> > >>>>>>> Unfortunately for non-trivial cases the first child node might not be a > >>>>>>> panel or bridge. Examples of this can be a aux-bus in the case of > >>>>>>> DisplayPort, or an opp-table represented before the panel node. > >>>>>>> > >>>>>>> In these cases the reverted commit prevents the caller from ever finding > >>>>>>> a reference to the panel. > >>>>>>> > >>>>>>> This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > >>>>>>> panel or bridge")', in favor of using an explicit graph reference to the > >>>>>>> panel in the trivial case as well. > >>>>>> This eventually breaks many child-based devm_drm_of_get_bridge > >>>>>> switched drivers. Do you have any suggestions on how to proceed to > >>>>>> succeed in those use cases as well? > >>>>> I guess we could create a new helper for those, like > >>>>> devm_drm_of_get_bridge_with_panel, or something. > >>>> Oh wow I feel stupid for not thinking about that. > >>>> > >>>> Yeah I agree that it seems like the best option. > >>> Should I prepare a patch with such a new helper? > >>> > >>> The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > >>> case and add one for the child node case, maybe: > >>> drm_of_find_child_panel_or_bridge. > >>> > >>> I really don't have a clear idea of which driver would need to be switched > >>> over though. Could someone (Jagan?) let me know where it would be needed? > >> sun6i_mipi_dsi > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > > >> exynos_drm_dsi > > If you reference 711c7adc4687, I don't see why we would need to switch > > it back to the old behaviour. It wasn't iterating over its child node > > before, so what does the switch to drm_of_get_bridge broke exactly? > > It broke getting the panel if it is a direct child of the DSI device > node. It worked before because it used following code: > > dsi->panel = of_drm_find_panel(device->dev.of_node); > > which got replaced by devm_drm_of_get_bridge(). Yes, we need to revert that change back to find the individual panel and bridge. I'm preparing a patch for it. Jagan.
On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard <maxime@cerno.tech> wrote: > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski > > <paul.kocialkowski@bootlin.com> wrote: > > > > > > Hi, > > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > Hi Maxime, > > > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > + Linus > > > > > > + Marek > > > > > > + Laurent > > > > > > + Robert > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson > > > > > > <bjorn.andersson@linaro.org> wrote: > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > a reference to the panel. > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > succeed in those use cases as well? > > > > > > > > > > I guess we could create a new helper for those, like > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > Should I prepare a patch with such a new helper? > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > case and add one for the child node case, maybe: > > > drm_of_find_child_panel_or_bridge. > > > > > > I really don't have a clear idea of which driver would need to be switched > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > sun6i_mipi_dsi > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? Correct, patch for this on the mailing list. > > > exynos_drm_dsi > > If you reference 711c7adc4687, I don't see why we would need to switch > it back to the old behaviour. It wasn't iterating over its child node > before, so what does the switch to drm_of_get_bridge broke exactly? Exynos bindings have a child node (unlike OF-graph), the old code is checking panel and bridge individually so it broke once switch to devm_drm_of_get_bridge Jagan.
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
Hi Laurent, On Fri, Apr 29, 2022 at 3:47 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jagan, > > On Thu, Apr 28, 2022 at 02:09:42PM +0530, Jagan Teki wrote: > > On Wed, Apr 27, 2022 at 8:04 PM Maxime Ripard wrote: > > > On Tue, Apr 26, 2022 at 01:40:31PM +0530, Jagan Teki wrote: > > > > On Tue, Apr 26, 2022 at 1:24 PM Paul Kocialkowski wrote: > > > > > On Thu 21 Apr 22, 10:59, Paul Kocialkowski wrote: > > > > > > On Thu 21 Apr 22, 10:23, Maxime Ripard wrote: > > > > > > > On Thu, Apr 21, 2022 at 01:15:54PM +0530, Jagan Teki wrote: > > > > > > > > + Linus > > > > > > > > + Marek > > > > > > > > + Laurent > > > > > > > > + Robert > > > > > > > > > > > > > > > > On Thu, Apr 21, 2022 at 4:40 AM Bjorn Andersson wrote: > > > > > > > > > > > > > > > > > > Commit '80253168dbfd ("drm: of: Lookup if child node has panel or > > > > > > > > > bridge")' attempted to simplify the case of expressing a simple panel > > > > > > > > > under a DSI controller, by assuming that the first non-graph child node > > > > > > > > > was a panel or bridge. > > > > > > > > > > > > > > > > > > Unfortunately for non-trivial cases the first child node might not be a > > > > > > > > > panel or bridge. Examples of this can be a aux-bus in the case of > > > > > > > > > DisplayPort, or an opp-table represented before the panel node. > > > > > > > > > > > > > > > > > > In these cases the reverted commit prevents the caller from ever finding > > > > > > > > > a reference to the panel. > > > > > > > > > > > > > > > > > > This reverts commit '80253168dbfd ("drm: of: Lookup if child node has > > > > > > > > > panel or bridge")', in favor of using an explicit graph reference to the > > > > > > > > > panel in the trivial case as well. > > > > > > > > > > > > > > > > This eventually breaks many child-based devm_drm_of_get_bridge > > > > > > > > switched drivers. Do you have any suggestions on how to proceed to > > > > > > > > succeed in those use cases as well? > > > > > > > > > > > > > > I guess we could create a new helper for those, like > > > > > > > devm_drm_of_get_bridge_with_panel, or something. > > > > > > > > > > > > Oh wow I feel stupid for not thinking about that. > > > > > > > > > > > > Yeah I agree that it seems like the best option. > > > > > > > > > > Should I prepare a patch with such a new helper? > > > > > > > > > > The idea would be to keep drm_of_find_panel_or_bridge only for the of graph > > > > > case and add one for the child node case, maybe: > > > > > drm_of_find_child_panel_or_bridge. > > > > > > > > > > I really don't have a clear idea of which driver would need to be switched > > > > > over though. Could someone (Jagan?) let me know where it would be needed? > > > > > > > > sun6i_mipi_dsi > > > > > > It doesn't look like sun6i_mipi_dsi is using devm_drm_of_get_bridge at all? > > > > Correct, patch for this on the mailing list. > > I've lost track of how we're solving the fallout of this for v5.18. I > have received a report that the original commit (80253168dbfd) also > broke the rcar-du driver. Could you please provide a git branch (based > on drm-fixes or drm-misc-fixes) with any patch that you plan to get > merged in v5.18, to let me test them locally ? The affected patches for 80253168dbfd revert are 711c7adc4687 3730bc6147b0 and 3d7039e1e649 Both these are not present drm-misc-fixes but there in linux-next. I've sent a patch for 711c7adc4687 https://patchwork.kernel.org/project/dri-devel/patch/20220428094808.782938-1-jagan@amarulasolutions.com/ This is my repo on top of linux-next https://github.com/openedev/kernel/tree/linux-next/drm-misc As I have seen before rcar-du ("155358310f013") is OF-graph and it doesn't affect the child node lookup was introduced in ("80253168dbfd") Let me know if you have any further information. Jagan.
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
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).
On Wed, Apr 27, 2022 at 2:34 AM Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote: > > Hi Bjorn, > > On Tue 26 Apr 22, 14:10, Bjorn Andersson wrote: > > On Tue 26 Apr 06:50 PDT 2022, Paul Kocialkowski wrote: [..] > > > Bjorn, what do you think? > > > > > > > I'm okay with the idea that it's up the driver to check that the output > > port references an usb-c-connector - either before the call or upon > > drm_of_find_panel_or_bridge() returning an error. > > Actually I'm starting to think might be wrong on this one: there's a > display-connector bridge driver that should register a bridge, so > drm_of_find_panel_or_bridge should work. Did you have that driver enabled? > I don't have this driver enabled, but that seems like it would solve the problem when the remote node is a dp-connector. Unfortunately in my particular case, I have a usb-c-connector. So I don't see that I would be able to reuse this straight off. But I assume that this is trying to say that the USB Type-C code is supposed to provide a bridge for each of the connectors exposed by my USB Type-C controller? I've been building on top of drm_connector_oob_hotplug_event() to achieve this (with the link in the other direction)... Regards, Bjorn
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
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;
Commit '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")' attempted to simplify the case of expressing a simple panel under a DSI controller, by assuming that the first non-graph child node was a panel or bridge. Unfortunately for non-trivial cases the first child node might not be a panel or bridge. Examples of this can be a aux-bus in the case of DisplayPort, or an opp-table represented before the panel node. In these cases the reverted commit prevents the caller from ever finding a reference to the panel. This reverts commit '80253168dbfd ("drm: of: Lookup if child node has panel or bridge")', in favor of using an explicit graph reference to the panel in the trivial case as well. Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/gpu/drm/drm_of.c | 17 ----------------- 1 file changed, 17 deletions(-)