Support DRM bridges on NVIDIA Tegra
Subsystems (5)
v7 cover Support DRM bridges on NVIDIA Tegra
Hello,
This series adds initial support for the DRM bridges to NVIDIA Tegra DRM driver. This is required by newer device-trees where we model the LVDS encoder bridge properly.
Changelog:
v7: - Removed the obscure unused structs (which GCC doesn't detect, but CLANG
does) in the patch "Wrap directly-connected panel into DRM bridge", which was reported by kernel test robot for v6.
v6: - Added r-b and acks from Rob Herring and Sam Ravnborg.
- Rebased on a recent linux-next, patches now apply without fuzz.
v5: - Added new patches that make drm_of_find_panel_or_bridge() more usable
if graph isn't defined in a device-tree:
of_graph: add of_graph_get_local_port()
drm/of: Make drm_of_find_panel_or_bridge() to check graph's presence
- Updated "Support DRM bridges" patch to use drm_of_find_panel_or_bridge() directly and added WARN_ON(output->panel || output->bridge) sanity-check.
- Added new "Wrap directly-connected panel into DRM bridge" patch, as was suggested by Laurent Pinchart.
v4: - Following review comments that were made by Laurent Pinchart to the v3,
we now create and use the "bridge connector".
v3: - Following recommendation from Sam Ravnborg, the new bridge attachment
model is now being used, i.e. we ask bridge to *not* create a connector using the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
- The bridge is now created only for the RGB (LVDS) output, and only when necessary. For now we don't need bridges for HDMI or DSI outputs.
- I noticed that we're leaking OF node in the panel's error code path, this is fixed now by the new patch "Don't leak OF node on error".
v2: - Added the new "rgb: Don't register connector if bridge is used"
patch, which hides the unused connector provided by the Tegra DRM driver when bridge is used, since bridge provides its own connector to us.
Dmitry Osipenko (6):
of_graph: add of_graph_get_local_port()
drm/of: Make drm_of_find_panel_or_bridge() to check graph's presence drm/tegra: output: Don't leak OF node on error
drm/tegra: output: Support DRM bridges
drm/tegra: output: rgb: Support LVDS encoder bridge drm/tegra: output: rgb: Wrap directly-connected panel into DRM bridge
drivers/gpu/drm/drm_of.c | 13 ++++- drivers/gpu/drm/tegra/drm.h | 2 + drivers/gpu/drm/tegra/output.c | 21 +++++-- drivers/gpu/drm/tegra/rgb.c | 102 +++++++++++++++++---------------- drivers/of/property.c | 32 ++++++++--- include/linux/of_graph.h | 7 +++ 6 files changed, 114 insertions(+), 63 deletions(-)
Signature · 1 line
2.26.0
PATCH v7 1/6 of_graph: add of_graph_get_local_port()
In some case, like a DRM display code for example, it's useful to silently check whether port node exists at all in a device-tree before proceeding with parsing the graph.
This patch adds of_graph_get_local_port() which returns pointer to a local port node, or NULL if graph isn't specified in a device-tree for a given device node.
--- drivers/of/property.c | 32 +++++++++++++++++++++++--------- include/linux/of_graph.h | 7 +++++++ 2 files changed, 30 insertions(+), 9 deletions(-)
drivers/of/property.c +23 -9
@@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,* parent port node. */ if (!prev) { -struct device_node *node; - - node =of_get_child_by_name (parent,"ports" ); -if (node) - parent = node; - - port =of_get_child_by_name (parent,"port" ); -of_node_put (node); - + port =of_graph_get_local_port (parent);if (!port) {pr_err ("graph: no port node found in %pOF\n" , parent);return NULL ;
@@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) }EXPORT_SYMBOL (of_graph_get_remote_port); +/** +* of_graph_get_local_port() - get local port node +* @node: pointer to a local endpoint device_node +* +* Return: First local port node associated with local endpoint node linked +* to @node. Use of_node_put() on it when done. +*/ +struct device_node *of_graph_get_local_port (const struct device_node *node) +{ +struct device_node *ports, *port; + + ports =of_get_child_by_name (node,"ports" ); +if (ports) + node = ports; + + port =of_get_child_by_name (node,"port" ); +of_node_put (ports); + +return port; +} +EXPORT_SYMBOL (of_graph_get_local_port); +int of_graph_get_endpoint_count (const struct device_node *np) {struct device_node *endpoint;
include/linux/of_graph.h +7 -0
@@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent(struct device_node *of_graph_get_remote_port (const struct device_node *node);struct device_node *of_graph_get_remote_node (const struct device_node *node,u32 port,u32 endpoint); +struct device_node *of_graph_get_local_port (const struct device_node *node);#else static inline int of_graph_parse_endpoint(const struct device_node *node,
@@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node(return NULL ; } +static inline struct device_node *of_graph_get_local_port ( +const struct device_node *node) +{ +return NULL ; +} +#endif /* CONFIG_OF */ #endif /* __LINUX_OF_GRAPH_H */
Signature · 1 line
2.26.0
Re: [PATCH v7 1/6] of_graph: add of_graph_get_local_port()
Hi Dmitry,
Thank you for the patch.
Quoted 50 lines · Dmitry Osipenko
In some case, like a DRM display code for example, it's useful to silently check whether port node exists at all in a device-tree before proceeding with parsing the graph.
This patch adds
of_graph_get_local_port()which returns pointer to a local port node, or NULL if graph isn't specified in a device-tree for a given device node.Reviewed-by Rob Herring <[email protected]>Reviewed-by Sam Ravnborg <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/of/property.c | 32 +++++++++++++++++++++++--------- include/linux/of_graph.h | 7 +++++++ 2 files changed, 30 insertions(+), 9 deletions(-)drivers/of/property.c +10
-9@@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,* parent port node. */ if (!prev) { -struct device_node *node; - - node =of_get_child_by_name (parent,"ports" ); -if (node) - parent = node; - - port =of_get_child_by_name (parent,"port" ); -of_node_put (node); - + port =of_graph_get_local_port (parent);if (!port) {pr_err ("graph: no port node found in %pOF\n" , parent);return NULL ;@@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) }EXPORT_SYMBOL (of_graph_get_remote_port); +/** +* of_graph_get_local_port() - get local port node +* @node: pointer to a local endpoint device_node +* +* Return: First local port node associated with local endpoint node linked +* to @node. Use of_node_put() on it when done. +*/ +struct device_node *of_graph_get_local_port (const struct device_node *node) +{
drivers/of/property.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 778
Quoted 10 lines
drivers/of/property.c +10
-0+struct device_node *ports, *port; + + ports =of_get_child_by_name (node,"ports" ); +if (ports) + node = ports; + + port =of_get_child_by_name (node,"port" ); +of_node_put (ports); + +return port;
The implementation doesn't seem to match the documentation. If node is a pointer to an endpoint, it should not have any ports child.
Quoted 34 lines
drivers/of/property.c +3
-0+} +EXPORT_SYMBOL (of_graph_get_local_port); +int of_graph_get_endpoint_count (const struct device_node *np) {struct device_node *endpoint;include/linux/of_graph.h +7
-0@@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent(struct device_node *of_graph_get_remote_port (const struct device_node *node);struct device_node *of_graph_get_remote_node (const struct device_node *node,u32 port,u32 endpoint); +struct device_node *of_graph_get_local_port (const struct device_node *node);#else static inline int of_graph_parse_endpoint(const struct device_node *node,@@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node(return NULL ; } +static inline struct device_node *of_graph_get_local_port ( +const struct device_node *node) +{ +return NULL ; +} +#endif /* CONFIG_OF */ #endif /* __LINUX_OF_GRAPH_H */ Signature · 1 line
2.26.0
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v7 1/6] of_graph: add of_graph_get_local_port()
Quoted 46 lines · Laurent Pinchart
Hi Dmitry,
Thank you for the patch.
Quoted 41 lines · Dmitry Osipenko
In some case, like a DRM display code for example, it's useful to silently check whether port node exists at all in a device-tree before proceeding with parsing the graph.
This patch adds
of_graph_get_local_port()which returns pointer to a local port node, or NULL if graph isn't specified in a device-tree for a given device node.Reviewed-by Rob Herring <[email protected]>Reviewed-by Sam Ravnborg <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/of/property.c | 32 +++++++++++++++++++++++--------- include/linux/of_graph.h | 7 +++++++ 2 files changed, 30 insertions(+), 9 deletions(-)drivers/of/property.c +1
-9@@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,* parent port node. */ if (!prev) { -struct device_node *node; - - node =of_get_child_by_name (parent,"ports" ); -if (node) - parent = node; - - port =of_get_child_by_name (parent,"port" ); -of_node_put (node); - + port =of_graph_get_local_port (parent);if (!port) {pr_err ("graph: no port node found in %pOF\n" , parent);return NULL ;@@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) }EXPORT_SYMBOL (of_graph_get_remote_port);
Quoted 8 lines
drivers/of/property.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 767
Quoted 8 lines
drivers/of/property.c +8
-0+/** +* of_graph_get_local_port() - get local port node +* @node: pointer to a local endpoint device_node +* +* Return: First local port node associated with local endpoint node linked +* to @node. Use of_node_put() on it when done. +*/ +struct device_node *of_graph_get_local_port (const struct device_node *node)
I forgot to mention that, given that there could be multiple 'port' nodes, this function would be better named of_graph_get_first_local_port(). 'first' here would refer to the nodes order in the device tree, which I believe may not match the port number. For instance, in the following case
ports {
#address-cells = <1>;
#size-cells = <1>;
port@1 {
reg = <1>;
};
port@0 {
reg = <0>;
};
};the function would I believe return port@1. It may be a good idea to explain this in the documentation. Depending on how you use this function, if your only use case is to test for the presence of port nodes, it may be best to return a bool and name it of_graph_has_port() or something similar.
Quoted 46 lines
drivers/of/property.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 778
Quoted 11 lines
drivers/of/property.c +11
-0+{ +struct device_node *ports, *port; + + ports =of_get_child_by_name (node,"ports" ); +if (ports) + node = ports; + + port =of_get_child_by_name (node,"port" ); +of_node_put (ports); + +return port;The implementation doesn't seem to match the documentation. If node is a pointer to an endpoint, it should not have any ports child.
Quoted 31 lines
drivers/of/property.c +3
-0+} +EXPORT_SYMBOL (of_graph_get_local_port); +int of_graph_get_endpoint_count (const struct device_node *np) {struct device_node *endpoint;include/linux/of_graph.h +7
-0@@ -54,6 +54,7 @@ struct device_node *of_graph_get_remote_port_parent(struct device_node *of_graph_get_remote_port (const struct device_node *node);struct device_node *of_graph_get_remote_node (const struct device_node *node,u32 port,u32 endpoint); +struct device_node *of_graph_get_local_port (const struct device_node *node);#else static inline int of_graph_parse_endpoint(const struct device_node *node,@@ -116,6 +117,12 @@ static inline struct device_node *of_graph_get_remote_node(return NULL ; } +static inline struct device_node *of_graph_get_local_port ( +const struct device_node *node) +{ +return NULL ; +} +#endif /* CONFIG_OF */ #endif /* __LINUX_OF_GRAPH_H */
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v7 1/6] of_graph: add of_graph_get_local_port()
16.06.2020 04:25, Laurent Pinchart пишет:
Quoted 73 lines · Laurent Pinchart
Quoted 46 lines
Hi Dmitry,
Thank you for the patch.
Quoted 41 lines · Dmitry Osipenko
In some case, like a DRM display code for example, it's useful to silently check whether port node exists at all in a device-tree before proceeding with parsing the graph.
This patch adds
of_graph_get_local_port()which returns pointer to a local port node, or NULL if graph isn't specified in a device-tree for a given device node.Reviewed-by Rob Herring <[email protected]>Reviewed-by Sam Ravnborg <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/of/property.c | 32 +++++++++++++++++++++++--------- include/linux/of_graph.h | 7 +++++++ 2 files changed, 30 insertions(+), 9 deletions(-)drivers/of/property.c +1
-9@@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,* parent port node. */ if (!prev) { -struct device_node *node; - - node =of_get_child_by_name (parent,"ports" ); -if (node) - parent = node; - - port =of_get_child_by_name (parent,"port" ); -of_node_put (node); - + port =of_graph_get_local_port (parent);if (!port) {pr_err ("graph: no port node found in %pOF\n" , parent);return NULL ;@@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) }EXPORT_SYMBOL (of_graph_get_remote_port);Quoted 8 lines
drivers/of/property.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 767
Quoted 8 lines
drivers/of/property.c +8
-0+/** +* of_graph_get_local_port() - get local port node +* @node: pointer to a local endpoint device_node +* +* Return: First local port node associated with local endpoint node linked +* to @node. Use of_node_put() on it when done. +*/ +struct device_node *of_graph_get_local_port (const struct device_node *node)I forgot to mention that, given that there could be multiple 'port' nodes, this function would be better named
of_graph_get_first_local_port(). 'first' here would refer to the nodes order in the device tree, which I believe may not match the port number. For instance, in the following caseports { #address-cells = <1>; #size-cells = <1>; port@1 { reg = <1>; }; port@0 { reg = <0>; }; };
Quoted 2 lines
the function would I believe return port@1. It may be a good idea to explain this in the documentation.
Hello Laurent,
It's correct that the port@1 will be returned in yours example.
I'll improve the doc and the function's name in the next revision, thank you for the suggestions!
Quoted 19 lines
Depending on how you use this
function, if your only use case is to test for the presence of port nodes, it may be best to return a bool and name itof_graph_has_port()or something similar.Quoted 14 lines
drivers/of/property.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 778
Quoted 11 lines
drivers/of/property.c +11
-0+{ +struct device_node *ports, *port; + + ports =of_get_child_by_name (node,"ports" ); +if (ports) + node = ports; + + port =of_get_child_by_name (node,"port" ); +of_node_put (ports); + +return port;The implementation doesn't seem to match the documentation. If node is a pointer to an endpoint, it should not have any ports child.
Right, I'll reword the doc in v8. This function doesn't differentiate between start / end points. It's up to a user of this function to check whether node is endpoint or something else if needed.
Thank you very much for the comments!
Re: [PATCH v7 1/6] of_graph: add of_graph_get_local_port()
16.06.2020 16:56, Dmitry Osipenko пишет:
Quoted 104 lines
16.06.2020 04:25, Laurent Pinchart пишет:
Quoted 73 lines · Laurent Pinchart
Quoted 46 lines
Hi Dmitry,
Thank you for the patch.
Quoted 41 lines · Dmitry Osipenko
In some case, like a DRM display code for example, it's useful to silently check whether port node exists at all in a device-tree before proceeding with parsing the graph.
This patch adds
of_graph_get_local_port()which returns pointer to a local port node, or NULL if graph isn't specified in a device-tree for a given device node.Reviewed-by Rob Herring <[email protected]>Reviewed-by Sam Ravnborg <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/of/property.c | 32 +++++++++++++++++++++++--------- include/linux/of_graph.h | 7 +++++++ 2 files changed, 30 insertions(+), 9 deletions(-)drivers/of/property.c +1
-9@@ -608,15 +608,7 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,* parent port node. */ if (!prev) { -struct device_node *node; - - node =of_get_child_by_name (parent,"ports" ); -if (node) - parent = node; - - port =of_get_child_by_name (parent,"port" ); -of_node_put (node); - + port =of_graph_get_local_port (parent);if (!port) {pr_err ("graph: no port node found in %pOF\n" , parent);return NULL ;@@ -765,6 +757,28 @@ struct device_node *of_graph_get_remote_port(const struct device_node *node) }EXPORT_SYMBOL (of_graph_get_remote_port);Quoted 8 lines
drivers/of/property.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 767
Quoted 8 lines
drivers/of/property.c +8
-0+/** +* of_graph_get_local_port() - get local port node +* @node: pointer to a local endpoint device_node +* +* Return: First local port node associated with local endpoint node linked +* to @node. Use of_node_put() on it when done. +*/ +struct device_node *of_graph_get_local_port (const struct device_node *node)I forgot to mention that, given that there could be multiple 'port' nodes, this function would be better named
of_graph_get_first_local_port(). 'first' here would refer to the nodes order in the device tree, which I believe may not match the port number. For instance, in the following caseports { #address-cells = <1>; #size-cells = <1>; port@1 { reg = <1>; }; port@0 { reg = <0>; }; };Quoted 2 lines
the function would I believe return port@1. It may be a good idea to explain this in the documentation.
Hello Laurent,
It's correct that the port@1 will be returned in yours example.
I'll improve the doc and the function's name in the next revision, thank you for the suggestions!
Quoted 19 lines
Depending on how you use this
function, if your only use case is to test for the presence of port nodes, it may be best to return a bool and name itof_graph_has_port()or something similar.Quoted 14 lines
drivers/of/property.c struct device_node *of_graph_get_remote_port(const struct device_node *node) · line 778
Quoted 11 lines
drivers/of/property.c +11
-0+{ +struct device_node *ports, *port; + + ports =of_get_child_by_name (node,"ports" ); +if (ports) + node = ports; + + port =of_get_child_by_name (node,"port" ); +of_node_put (ports); + +return port;The implementation doesn't seem to match the documentation. If node is a pointer to an endpoint, it should not have any ports child.
Quoted 6 lines
Right, I'll reword the doc in v8. This function doesn't differentiate between start / end points. It's up to a user of this function to check whether node is endpoint or something else if needed.
Thank you very much for the comments!
Although, I re-read the doc comment and compared it to the wording of the other of_graph_*() functions and it's already good as-is to me because the doc explicitly says the "Return: First local port associated with local endpoint...", which is exactly what this function does.
But still the function name and it's brief description could be improved.
PATCH v7 2/6 drm/of: Make drm_of_find_panel_or_bridge() to check graph's presence
In reply to Dmitry OsipenkoWhen graph isn't defined in a device-tree, the of_graph_get_remote_node() prints a noisy error message, telling that port node is not found. This is undesirable behaviour in our case because absence of a panel/bridge graph is a valid case. Let's check presence of the local port in a device-tree before proceeding with parsing the graph.
--- drivers/gpu/drm/drm_of.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_of.c +12 -1
@@ -239,13 +239,24 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_bridge **bridge) {int ret = -EPROBE_DEFER ; -struct device_node *remote; +struct device_node *local, *remote;if (!panel && !bridge)return -EINVAL ;if (panel) *panel =NULL ; +/* +* of_graph_get_remote_node() produces a noisy error message if port +* node isn't found and the absence of the port is a legit case here, +* so at first we silently check presence of the local port. +*/ + local =of_graph_get_local_port (np); +if (!local) +return -ENODEV ; + +of_node_put (local); + remote =of_graph_get_remote_node (np, port, endpoint);if (!remote)return -ENODEV ;
Signature · 1 line
2.26.0
Re: [PATCH v7 2/6] drm/of: Make drm_of_find_panel_or_bridge() to check graph's presence
Hi Dmitry,
Thank you for the patch.
Quoted 29 lines · Dmitry Osipenko
When graph isn't defined in a device-tree, the
of_graph_get_remote_node()prints a noisy error message, telling that port node is not found. This is undesirable behaviour in our case because absence of a panel/bridge graph is a valid case. Let's check presence of the local port in a device-tree before proceeding with parsing the graph.Reviewed-by Sam Ravnborg <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/drm/drm_of.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_of.c +2
-1@@ -239,13 +239,24 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_bridge **bridge) {int ret = -EPROBE_DEFER ; -struct device_node *remote; +struct device_node *local, *remote;if (!panel && !bridge)return -EINVAL ;if (panel) *panel =NULL ; +/*
drivers/gpu/drm/drm_of.c int drm_of_find_panel_or_bridge(const struct device_node *np, · line 259
Quoted 10 lines
drivers/gpu/drm/drm_of.c +10
-0+* of_graph_get_remote_node() produces a noisy error message if port +* node isn't found and the absence of the port is a legit case here, +* so at first we silently check presence of the local port. +*/ + local =of_graph_get_local_port (np); +if (!local) +return -ENODEV ; + +of_node_put (local); +
The code looks fine, but you may want to take into account my proposal in 1/7 to instead create a of_graph_has_port() function. The could would be simpler here.
Quoted 3 lines
drivers/gpu/drm/drm_of.c +0
-0remote =of_graph_get_remote_node (np, port, endpoint);if (!remote)return -ENODEV ;
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v7 2/6] drm/of: Make drm_of_find_panel_or_bridge() to check graph's presence
16.06.2020 04:26, Laurent Pinchart пишет:
Quoted 33 lines
Hi Dmitry,
Thank you for the patch.
Quoted 28 lines · Dmitry Osipenko
When graph isn't defined in a device-tree, the
of_graph_get_remote_node()prints a noisy error message, telling that port node is not found. This is undesirable behaviour in our case because absence of a panel/bridge graph is a valid case. Let's check presence of the local port in a device-tree before proceeding with parsing the graph.Reviewed-by Sam Ravnborg <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- drivers/gpu/drm/drm_of.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)drivers/gpu/drm/drm_of.c +1
-1@@ -239,13 +239,24 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_bridge **bridge) {int ret = -EPROBE_DEFER ; -struct device_node *remote; +struct device_node *local, *remote;if (!panel && !bridge)return -EINVAL ;if (panel) *panel =NULL ;
Quoted 20 lines
drivers/gpu/drm/drm_of.c int drm_of_find_panel_or_bridge(const struct device_node *np, · line 259
Quoted 11 lines
drivers/gpu/drm/drm_of.c +11
-0+/* +* of_graph_get_remote_node() produces a noisy error message if port +* node isn't found and the absence of the port is a legit case here, +* so at first we silently check presence of the local port. +*/ + local =of_graph_get_local_port (np); +if (!local) +return -ENODEV ; + +of_node_put (local); +The code looks fine, but you may want to take into account my proposal in 1/7 to instead create a
of_graph_has_port()function. The could would be simpler here.Quoted 3 lines
drivers/gpu/drm/drm_of.c +0
-0remote =of_graph_get_remote_node (np, port, endpoint);if (!remote)return -ENODEV ;
I like yours proposals and will prepare v8 based on them, thanks!
PATCH v7 3/6 drm/tegra: output: Don't leak OF node on error
In reply to Dmitry OsipenkoThe OF node should be put before returning error in tegra_output_probe(), otherwise node's refcount will be leaked.
--- drivers/gpu/drm/tegra/output.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/gpu/drm/tegra/output.c +4 -5
@@ -102,10 +102,10 @@ int tegra_output_probe(struct tegra_output *output) panel =of_parse_phandle (output->of_node,"nvidia,panel" ,0 );if (panel) { output->panel =of_drm_find_panel (panel); +of_node_put (panel); +if (IS_ERR (output->panel))return PTR_ERR (output->panel); - -of_node_put (panel); } output->edid =of_get_property (output->of_node,"nvidia,edid" , &size);
@@ -113,13 +113,12 @@ int tegra_output_probe(struct tegra_output *output) ddc =of_parse_phandle (output->of_node,"nvidia,ddc-i2c-bus" ,0 );if (ddc) { output->ddc =of_find_i2c_adapter_by_node (ddc); +of_node_put (ddc); +if (!output->ddc) { err = -EPROBE_DEFER ; -of_node_put (ddc);return err; } - -of_node_put (ddc); } output->hpd_gpio = devm_gpiod_get_from_of_node(output->dev,
Signature · 1 line
2.26.0
PATCH v7 4/6 drm/tegra: output: Support DRM bridges
In reply to Dmitry OsipenkoNewer Tegra device-trees will specify a video output graph which involves a bridge. This patch adds initial support for the DRM bridges to the Tegra DRM output.
--- drivers/gpu/drm/tegra/drm.h | 2 ++ drivers/gpu/drm/tegra/output.c | 12 ++++++++++++ 2 files changed, 14 insertions(+)
drivers/gpu/drm/tegra/drm.h +2 -0
@@ -12,6 +12,7 @@#include <linux/gpio/consumer.h> #include <drm/drm_atomic.h> +#include <drm/drm_bridge.h> #include <drm/drm_edid.h> #include <drm/drm_encoder.h> #include <drm/drm_fb_helper.h>
@@ -116,6 +117,7 @@ struct tegra_output {struct device_node *of_node;struct device *dev; +struct drm_bridge *bridge;struct drm_panel *panel;struct i2c_adapter *ddc;const struct edid *edid;
drivers/gpu/drm/tegra/output.c +12 -0
@@ -5,6 +5,7 @@*/ #include <drm/drm_atomic_helper.h> +#include <drm/drm_of.h> #include <drm/drm_panel.h> #include <drm/drm_simple_kms_helper.h>
@@ -99,8 +100,19 @@ int tegra_output_probe(struct tegra_output *output)if (!output->of_node) output->of_node = output->dev->of_node; + err =drm_of_find_panel_or_bridge (output->of_node,-1 ,-1 , + &output->panel, &output->bridge); +if (err && err != -ENODEV ) +return err; + panel =of_parse_phandle (output->of_node,"nvidia,panel" ,0 );if (panel) { +/* +* Don't mix nvidia,panel phandle with the graph in a +* device-tree. +*/ +WARN_ON (output->panel || output->bridge); + output->panel =of_drm_find_panel (panel);of_node_put (panel);
Signature · 1 line
2.26.0
PATCH v7 5/6 drm/tegra: output: rgb: Support LVDS encoder bridge
In reply to Dmitry OsipenkoNewer Tegra device-trees will specify a video output graph, which involves LVDS encoder bridge. This patch adds support for the LVDS encoder bridge to the RGB output, allowing us to model the display hardware properly.
--- drivers/gpu/drm/tegra/rgb.c | 58 +++++++++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 9 deletions(-)
drivers/gpu/drm/tegra/rgb.c +49 -9
@@ -7,6 +7,7 @@#include <linux/clk.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_panel.h> #include <drm/drm_simple_kms_helper.h>
@@ -267,24 +268,63 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)int tegra_dc_rgb_init (struct drm_device *drm,struct tegra_dc *dc) {struct tegra_output *output = dc->rgb; +struct drm_connector *connector;int err;if (!dc->rgb)return -ENODEV ; -drm_connector_init (drm, &output->connector, &tegra_rgb_connector_funcs, -DRM_MODE_CONNECTOR_LVDS ); -drm_connector_helper_add (&output->connector, - &tegra_rgb_connector_helper_funcs); - output->connector.dpms =DRM_MODE_DPMS_OFF ; -drm_simple_encoder_init (drm, &output->encoder,DRM_MODE_ENCODER_LVDS );drm_encoder_helper_add (&output->encoder, &tegra_rgb_encoder_helper_funcs); -drm_connector_attach_encoder (&output->connector, - &output->encoder); -drm_connector_register (&output->connector); +/* +* Tegra devices that have LVDS panel utilize LVDS encoder bridge +* for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that +* go to display panel's receiver. +* +* Encoder usually have a power-down control which needs to be enabled +* in order to transmit data to the panel. Historically devices that +* use an older device-tree version didn't model the bridge, assuming +* that encoder is turned ON by default, while today's DRM allows us +* to model LVDS encoder properly. +* +* Newer device-trees utilize LVDS encoder bridge, which provides +* us with a connector and handles the display panel. +* +* For older device-trees we fall back to our own connector and use +* nvidia,panel phandle. +*/ +if (output->bridge) { + err =drm_bridge_attach (&output->encoder, output->bridge, +NULL ,DRM_BRIDGE_ATTACH_NO_CONNECTOR ); +if (err) { +dev_err (output->dev,"failed to attach bridge: %d\n" , + err); +return err; + } + + connector =drm_bridge_connector_init (drm, &output->encoder); +if (IS_ERR (connector)) { +dev_err (output->dev, +"failed to initialize bridge connector: %pe\n" , + connector); +return PTR_ERR (connector); + } + +drm_connector_attach_encoder (connector, &output->encoder); + }else { +drm_connector_init (drm, &output->connector, + &tegra_rgb_connector_funcs, +DRM_MODE_CONNECTOR_LVDS ); +drm_connector_helper_add (&output->connector, + &tegra_rgb_connector_helper_funcs); + output->connector.dpms =DRM_MODE_DPMS_OFF ; + +drm_connector_attach_encoder (&output->connector, + &output->encoder); +drm_connector_register (&output->connector); + } err =tegra_output_init (drm, output);if (err <0 ) {
Signature · 1 line
2.26.0
PATCH v7 6/6 drm/tegra: output: rgb: Wrap directly-connected panel into DRM bridge
In reply to Dmitry OsipenkoCurrently Tegra DRM driver manually manages display panel, but this management could be moved out into DRM core if we'll wrap panel into DRM bridge. This patch wraps RGB panel into a DRM bridge and removes manual handling of the panel from the RGB output code.
--- drivers/gpu/drm/tegra/rgb.c | 70 ++++++++++--------------------------- 1 file changed, 18 insertions(+), 52 deletions(-)
drivers/gpu/drm/tegra/rgb.c +18 -52
@@ -8,7 +8,6 @@#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge_connector.h> -#include <drm/drm_panel.h> #include <drm/drm_simple_kms_helper.h> #include "drm.h"
@@ -86,45 +85,13 @@ static void tegra_dc_write_regs(struct tegra_dc *dc,tegra_dc_writel (dc, table[i].value, table[i].offset); } -static const struct drm_connector_funcs tegra_rgb_connector_funcs = { - .reset = drm_atomic_helper_connector_reset, - .detect = tegra_output_connector_detect, - .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = tegra_output_connector_destroy, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - -static enum drm_mode_status -tegra_rgb_connector_mode_valid (struct drm_connector *connector, -struct drm_display_mode *mode) -{ -/* -* FIXME: For now, always assume that the mode is okay. There are -* unresolved issues with clk_round_rate(), which doesn't always -* reliably report whether a frequency can be set or not. -*/ -return MODE_OK ; -} - -static const struct drm_connector_helper_funcs tegra_rgb_connector_helper_funcs = { - .get_modes = tegra_output_connector_get_modes, - .mode_valid = tegra_rgb_connector_mode_valid, -}; -static void tegra_rgb_encoder_disable (struct drm_encoder *encoder) {struct tegra_output *output =encoder_to_output (encoder);struct tegra_rgb *rgb =to_rgb (output); -if (output->panel) -drm_panel_disable (output->panel); -tegra_dc_write_regs (rgb->dc, rgb_disable,ARRAY_SIZE (rgb_disable));tegra_dc_commit (rgb->dc); - -if (output->panel) -drm_panel_unprepare (output->panel); }static void tegra_rgb_encoder_enable (struct drm_encoder *encoder)
@@ -133,9 +100,6 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)struct tegra_rgb *rgb =to_rgb (output);u32 value; -if (output->panel) -drm_panel_prepare (output->panel); -tegra_dc_write_regs (rgb->dc, rgb_enable,ARRAY_SIZE (rgb_enable)); value =DE_SELECT_ACTIVE |DE_CONTROL_NORMAL ;
@@ -157,9 +121,6 @@ static void tegra_rgb_encoder_enable(struct drm_encoder *encoder)tegra_dc_writel (rgb->dc, value,DC_DISP_SHIFT_CLOCK_OPTIONS );tegra_dc_commit (rgb->dc); - -if (output->panel) -drm_panel_enable (output->panel); } static int
@@ -278,6 +239,23 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)drm_encoder_helper_add (&output->encoder, &tegra_rgb_encoder_helper_funcs); +/* +* Wrap directly-connected panel into DRM bridge in order to let +* DRM core to handle panel for us. +*/ +if (output->panel) { + output->bridge =devm_drm_panel_bridge_add (output->dev, + output->panel); +if (IS_ERR (output->bridge)) { +dev_err (output->dev, +"failed to wrap panel into bridge: %pe\n" , + output->bridge); +return PTR_ERR (output->bridge); + } + + output->panel =NULL ; + } +/* * Tegra devices that have LVDS panel utilize LVDS encoder bridge * for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that
@@ -292,8 +270,7 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)* Newer device-trees utilize LVDS encoder bridge, which provides * us with a connector and handles the display panel. * -* For older device-trees we fall back to our own connector and use -* nvidia,panel phandle. +* For older device-trees we wrapped panel into the panel-bridge. */ if (output->bridge) { err = drm_bridge_attach(&output->encoder, output->bridge,
@@ -313,17 +290,6 @@ int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc) }drm_connector_attach_encoder (connector, &output->encoder); - }else { -drm_connector_init (drm, &output->connector, - &tegra_rgb_connector_funcs, -DRM_MODE_CONNECTOR_LVDS ); -drm_connector_helper_add (&output->connector, - &tegra_rgb_connector_helper_funcs); - output->connector.dpms =DRM_MODE_DPMS_OFF ; - -drm_connector_attach_encoder (&output->connector, - &output->encoder); -drm_connector_register (&output->connector); } err =tegra_output_init (drm, output);
Signature · 1 line
2.26.0