Changelog: v3: Removed the call to dma_set_coherent_mask() from the generic drm_of_component_probe(). Also changes to shorten lines over 80 chars long. v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc
A few drivers in drivers/gpu/drm are component-enabled and use quite similar code sequences to probe for their encoder slaves at the remote end of the ports. Move the code into a "generic" function and remove it from the drivers.
The end results is that drivers get a reference count fix (imx), more thorough error checking (imx again) plus a decrease in the overall count of LoC.
I'm looking for comments and testing of the patchset (only compile tested from my end as I don't have access to all the devices touched by the changes). My main interest is in finding out if -EINVAL is the correct code to return if dev->of_node == NULL (handy now, as it is different from the other possible error codes and used in armada to trigger old platform_data support. Also looking for thoughts on the correctness of the patch and if it possible to co-opt more drivers into using the function.
Best regards, Liviu
Liviu Dudau (4): drm: Introduce generic probe function for component based masters. drm/imx: Convert the probe function to the generic drm_of_component_probe() drm/rockchip: Convert the probe function to the generic drm_of_component_probe() drm/armada: Convert the probe function to the generic drm_of_component_probe()
drivers/gpu/drm/armada/armada_drv.c | 73 ++++++++---------------- drivers/gpu/drm/drm_of.c | 88 +++++++++++++++++++++++++++++ drivers/gpu/drm/imx/imx-drm-core.c | 55 ++---------------- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 ++------------------------ include/drm/drm_of.h | 13 +++++ 5 files changed, 133 insertions(+), 177 deletions(-)
A lot of component based DRM drivers use a variant of the same code as the probe function. They bind the crtc ports in the first iteration and then scan through the child nodes and bind the encoders attached to the remote endpoints. Factor the common code into a separate function called drm_of_component_probe() in order to increase code reuse.
Cc: David Airlie airlied@linux.ie Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com --- drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 13 +++++++ 2 files changed, 101 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index be38840..493c05c 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,3 +1,4 @@ +#include <linux/component.h> #include <linux/export.h> #include <linux/list.h> #include <linux/of_graph.h> @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, return possible_crtcs; } EXPORT_SYMBOL(drm_of_find_possible_crtcs); + +/** + * drm_of_component_probe - Generic probe function for a component based master + * @dev: master device containing the OF node + * @compare_of: compare function used for matching components + * @master_ops: component master ops to be used + * + * Parse the platform device OF node and bind all the components associated + * with the master. Interface ports are added before the encoders in order to + * satisfy their .bind requirements + * See Documentation/devicetree/bindings/graph.txt for the bindings. + * + * Returns zero if successful, or one of the standard error codes if it fails. + */ +int drm_of_component_probe(struct device *dev, + int (*compare_of)(struct device *, void *), + const struct component_master_ops *m_ops) +{ + struct device_node *ep, *port, *remote; + struct component_match *match = NULL; + int i; + + if (!dev->of_node) + return -EINVAL; + + /* + * Bind the crtc's ports first, so that drm_of_find_possible_crtcs() + * called from encoder's .bind callbacks works as expected + */ + for (i = 0; ; i++) { + port = of_parse_phandle(dev->of_node, "ports", i); + if (!port) + break; + + if (!of_device_is_available(port->parent)) { + of_node_put(port); + continue; + } + + component_match_add(dev, &match, compare_of, port); + of_node_put(port); + } + + if (i == 0) { + dev_err(dev, "missing 'ports' property\n"); + return -ENODEV; + } + + if (!match) { + dev_err(dev, "no available port\n"); + return -ENODEV; + } + + /* + * For bound crtcs, bind the encoders attached to their remote endpoint + */ + for (i = 0; ; i++) { + port = of_parse_phandle(dev->of_node, "ports", i); + if (!port) + break; + + if (!of_device_is_available(port->parent)) { + of_node_put(port); + continue; + } + + for_each_child_of_node(port, ep) { + remote = of_graph_get_remote_port_parent(ep); + if (!remote || !of_device_is_available(remote)) { + of_node_put(remote); + continue; + } else if (!of_device_is_available(remote->parent)) { + dev_warn(dev, "parent device of %s is not available\n", + remote->full_name); + of_node_put(remote); + continue; + } + + component_match_add(dev, &match, compare_of, remote); + of_node_put(remote); + } + of_node_put(port); + } + + return component_master_add_with_match(dev, m_ops, match); +} +EXPORT_SYMBOL(drm_of_component_probe); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index 2441f71..8544665 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -1,18 +1,31 @@ #ifndef __DRM_OF_H__ #define __DRM_OF_H__
+struct component_master_ops; +struct device; struct drm_device; struct device_node;
#ifdef CONFIG_OF extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, struct device_node *port); +extern int drm_of_component_probe(struct device *dev, + int (*compare_of)(struct device *, void *), + const struct component_master_ops *m_ops); #else static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, struct device_node *port) { return 0; } + +static inline int +drm_of_component_probe(struct device *dev, + int (*compare_of)(struct device *, void *), + const struct component_master_ops *m_ops) +{ + return -EINVAL; +} #endif
#endif /* __DRM_OF_H__ */
On Mon, Oct 19, 2015 at 04:07:47PM +0100, Liviu Dudau wrote:
A lot of component based DRM drivers use a variant of the same code as the probe function. They bind the crtc ports in the first iteration and then scan through the child nodes and bind the encoders attached to the remote endpoints. Factor the common code into a separate function called drm_of_component_probe() in order to increase code reuse.
Thanks, this now looks perfect.
Acked-by: Russell King rmk+kernel@arm.linux.org.uk
Liviu Dudau Liviu.Dudau@arm.com writes:
A lot of component based DRM drivers use a variant of the same code as the probe function. They bind the crtc ports in the first iteration and then scan through the child nodes and bind the encoders attached to the remote endpoints. Factor the common code into a separate function called drm_of_component_probe() in order to increase code reuse.
Cc: David Airlie airlied@linux.ie Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
Acked-by: Eric Anholt eric@anholt.net
I'm unimpressed with of-graph, but this will really help me in trying it out for vc4.
On 2015年10月19日 23:07, Liviu Dudau wrote:
A lot of component based DRM drivers use a variant of the same code as the probe function. They bind the crtc ports in the first iteration and then scan through the child nodes and bind the encoders attached to the remote endpoints. Factor the common code into a separate function called drm_of_component_probe() in order to increase code reuse.
Cc: David Airlie airlied@linux.ie Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 13 +++++++ 2 files changed, 101 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index be38840..493c05c 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,3 +1,4 @@ +#include <linux/component.h> #include <linux/export.h> #include <linux/list.h> #include <linux/of_graph.h> @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, return possible_crtcs; } EXPORT_SYMBOL(drm_of_find_possible_crtcs);
+/**
- drm_of_component_probe - Generic probe function for a component based master
- @dev: master device containing the OF node
- @compare_of: compare function used for matching components
- @master_ops: component master ops to be used
- Parse the platform device OF node and bind all the components associated
- with the master. Interface ports are added before the encoders in order to
- satisfy their .bind requirements
- See Documentation/devicetree/bindings/graph.txt for the bindings.
- Returns zero if successful, or one of the standard error codes if it fails.
- */
+int drm_of_component_probe(struct device *dev,
int (*compare_of)(struct device *, void *),const struct component_master_ops *m_ops)+{
- struct device_node *ep, *port, *remote;
- struct component_match *match = NULL;
- int i;
- if (!dev->of_node)
return -EINVAL;- /*
* Bind the crtc's ports first, so that drm_of_find_possible_crtcs()* called from encoder's .bind callbacks works as expected*/- for (i = 0; ; i++) {
port = of_parse_phandle(dev->of_node, "ports", i);if (!port)break;if (!of_device_is_available(port->parent)) {of_node_put(port);continue;}component_match_add(dev, &match, compare_of, port);
Hi Liviu Rockchip drm can't work with drm_of_component_probe function now,
At drm_of_component_probe: component_match_add(dev, &match, compare_of, port); And original rockchip drm use: component_match_add(dev, &match, compare_of, port->parent);
That different "port" and "port->parent" cause crtc device node always mis-match.
I'm confused that rockchip use same dts node map as imx drm driver, but it works for imx drm, not work on rockchip drm.
of_node_put(port);- }
-- Ｍark Yao
On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
On 2015年10月19日 23:07, Liviu Dudau wrote:
A lot of component based DRM drivers use a variant of the same code as the probe function. They bind the crtc ports in the first iteration and then scan through the child nodes and bind the encoders attached to the remote endpoints. Factor the common code into a separate function called drm_of_component_probe() in order to increase code reuse.
Cc: David Airlie airlied@linux.ie Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 13 +++++++ 2 files changed, 101 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index be38840..493c05c 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,3 +1,4 @@ +#include <linux/component.h> #include <linux/export.h> #include <linux/list.h> #include <linux/of_graph.h> @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, return possible_crtcs; } EXPORT_SYMBOL(drm_of_find_possible_crtcs);
+/**
- drm_of_component_probe - Generic probe function for a component based master
- @dev: master device containing the OF node
- @compare_of: compare function used for matching components
- @master_ops: component master ops to be used
- Parse the platform device OF node and bind all the components associated
- with the master. Interface ports are added before the encoders in order to
- satisfy their .bind requirements
- See Documentation/devicetree/bindings/graph.txt for the bindings.
- Returns zero if successful, or one of the standard error codes if it fails.
- */
+int drm_of_component_probe(struct device *dev,
int (*compare_of)(struct device *, void *),const struct component_master_ops *m_ops)+{
- struct device_node *ep, *port, *remote;
- struct component_match *match = NULL;
- int i;
- if (!dev->of_node)
return -EINVAL;- /*
* Bind the crtc's ports first, so that drm_of_find_possible_crtcs()* called from encoder's .bind callbacks works as expected*/- for (i = 0; ; i++) {
port = of_parse_phandle(dev->of_node, "ports", i);if (!port)break;if (!of_device_is_available(port->parent)) {of_node_put(port);continue;}component_match_add(dev, &match, compare_of, port);Hi Liviu Rockchip drm can't work with drm_of_component_probe function now,
At drm_of_component_probe: component_match_add(dev, &match, compare_of, port); And original rockchip drm use: component_match_add(dev, &match, compare_of, port->parent); That different "port" and "port->parent" cause crtc device node alwaysmis-match.
I'm confused that rockchip use same dts node map as imx drm driver, butit works for imx drm, not work on rockchip drm.
Hi Mark,
I'm (slightly) confused as well. The drivers are different so there must be a reason to account for the different behaviour. Unfortunately I don't have a Rockchip based platform ready for testing, so I would appreciate if you could add some debugging messages to drm_of_component_probe() when component_match_add is being called and compare that with the version before my patch.
Best regards, Liviu
of_node_put(port);- }
-- Ｍark Yao
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Am Montag, den 09.11.2015, 10:53 +0000 schrieb Liviu Dudau:
On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
On 2015年10月19日 23:07, Liviu Dudau wrote:
A lot of component based DRM drivers use a variant of the same code as the probe function. They bind the crtc ports in the first iteration and then scan through the child nodes and bind the encoders attached to the remote endpoints. Factor the common code into a separate function called drm_of_component_probe() in order to increase code reuse.
Cc: David Airlie airlied@linux.ie Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
drivers/gpu/drm/drm_of.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_of.h | 13 +++++++ 2 files changed, 101 insertions(+)
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index be38840..493c05c 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -1,3 +1,4 @@ +#include <linux/component.h> #include <linux/export.h> #include <linux/list.h> #include <linux/of_graph.h> @@ -61,3 +62,90 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, return possible_crtcs; } EXPORT_SYMBOL(drm_of_find_possible_crtcs);
+/**
- drm_of_component_probe - Generic probe function for a component based master
- @dev: master device containing the OF node
- @compare_of: compare function used for matching components
- @master_ops: component master ops to be used
- Parse the platform device OF node and bind all the components associated
- with the master. Interface ports are added before the encoders in order to
- satisfy their .bind requirements
- See Documentation/devicetree/bindings/graph.txt for the bindings.
- Returns zero if successful, or one of the standard error codes if it fails.
- */
+int drm_of_component_probe(struct device *dev,
int (*compare_of)(struct device *, void *),const struct component_master_ops *m_ops)+{
- struct device_node *ep, *port, *remote;
- struct component_match *match = NULL;
- int i;
- if (!dev->of_node)
return -EINVAL;- /*
* Bind the crtc's ports first, so that drm_of_find_possible_crtcs()* called from encoder's .bind callbacks works as expected*/- for (i = 0; ; i++) {
port = of_parse_phandle(dev->of_node, "ports", i);if (!port)break;if (!of_device_is_available(port->parent)) {of_node_put(port);continue;}component_match_add(dev, &match, compare_of, port);Hi Liviu Rockchip drm can't work with drm_of_component_probe function now,
At drm_of_component_probe: component_match_add(dev, &match, compare_of, port); And original rockchip drm use: component_match_add(dev, &match, compare_of, port->parent); That different "port" and "port->parent" cause crtc device node alwaysmis-match.
I'm confused that rockchip use same dts node map as imx drm driver, butit works for imx drm, not work on rockchip drm.
Hi Mark,
I'm (slightly) confused as well. The drivers are different so there must be a reason to account for the different behaviour. Unfortunately I don't have a Rockchip based platform ready for testing, so I would appreciate if you could add some debugging messages to drm_of_component_probe() when component_match_add is being called and compare that with the version before my patch.
The reason is that rockchip has device tree probed devices that contain the ports, but on imx crtc devices are platform devices created by the driver, which don't have their own device tree node. They are associated with the port nodes directly in ipu_drm_probe (ipuv3-crtc.c).
regards Philipp
On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
Hi Liviu Rockchip drm can't work with drm_of_component_probe function now,
At drm_of_component_probe: component_match_add(dev, &match, compare_of, port); And original rockchip drm use: component_match_add(dev, &match, compare_of, port->parent); That different "port" and "port->parent" cause crtc device node alwaysmis-match.
I'm confused that rockchip use same dts node map as imx drm driver, butit works for imx drm, not work on rockchip drm.
iMX is rather confusing because the whole device is rather complex. It's a complete image processor unit, which has multiple functions within a single device. It's a less than perfect example of how to deal with these issues (each time I look at it, I find more stuff it shouldn't be doing... like what I've found today.)
Basically, the device is declared in DT as:
ipu1: ipu@02400000 { compatible = "fsl,imx6q-ipu"; ipu1_csi0: port@0 { ... }; ipu1_csi1: port@1 { ... }; ipu1_di0: port@2 { ... }; ipu1_di1: port@3 { ... }; };
ipu1 is the platform device, which is the _entire_ IPU device, containing multiple sub-devices - eg, two camera interfaces (csi) and two "CRTCs" (di).
The ipuv3 code creates platform devices for these, calling the CSI devices "imx-ipuv3-camera" and the DI devices "imx-ipuv3-crtc". Initially, these have no of_node attached (yuck!) but later have an of_node attached when the device is probed (yuck yuck yuck!) by ipu_drm_probe() - the of_node being the ipu1_di0 or ipu1_di1 node.
The display-subsystem property references these ipu1_di0/ipu1_di1 nodes:
display-subsystem { compatible = "fsl,imx-display-subsystem"; ports = <&ipu1_di0>, <&ipu1_di1>, <&ipu2_di0>, <&ipu2_di1>; };
and so finds the "imx-ipuv3-crtc" platform devices rather than the parent ipu1 device.
It's not nice - I'd like to see this:
if (!dev->of_node) { /* Associate crtc device with the corresponding DI port node */ dev->of_node = ipu_drm_get_port_by_id(dev->parent->of_node, pdata->di + 2); if (!dev->of_node) { dev_err(dev, "missing port@%d node in %s\n", pdata->di + 2, dev->parent->of_node->full_name); return -ENODEV; } }
moved out of drivers/gpu/drm/imx/ipuv3-crtc.c and into drivers/gpu/ipu-v3/ipu-common.c where the platform devices are created so that we're only setting device of_node pointers at device creation time and not randomly during the probe sequence, even though the above is "safe" as far as the component helper's concerned. There's no reason why it can't be done at the device creation time.
Now, as to how to handle the differences here, I think a solution would be to pass in two compare_of function pointers: one for CRTCs and one for the encoders, since the two will need to be handled separately depending on the implementation. Where you have one "parent" device of the CRTC node containing exactly one CRTC, then the "rockchip" method makes sense - though comparing the parent of the port node in CRTC compare_of would be where I'd put it. If you have multiple CRTCs within one parent device, then something more complex like the iMX solution would be required.
In any case, passing port->parent is a data loss in the generic case: you lose the information in the compare_of function about exactly which port is required, so that must go into the CRTC compare_of.
So...
rockchip's CRTC compare_of() should be:
static int crtc_compare_of(struct device *dev, void *data) { struct device_node *np = data;
return dev->of_node == np->parent; }
and it should have an encoder_compare_of() which is its existing compare_of() renamed as such.
Then, we need drm_of_component_probe() to take _two_ comparison functions, one for the CRTCs and one for the encoders.
On Mon, Nov 09, 2015 at 11:43:00AM +0000, Russell King - ARM Linux wrote:
On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
Hi Liviu Rockchip drm can't work with drm_of_component_probe function now,
At drm_of_component_probe: component_match_add(dev, &match, compare_of, port); And original rockchip drm use: component_match_add(dev, &match, compare_of, port->parent); That different "port" and "port->parent" cause crtc device node alwaysmis-match.
I'm confused that rockchip use same dts node map as imx drm driver, butit works for imx drm, not work on rockchip drm.
iMX is rather confusing because the whole device is rather complex. It's a complete image processor unit, which has multiple functions within a single device. It's a less than perfect example of how to deal with these issues (each time I look at it, I find more stuff it shouldn't be doing... like what I've found today.)
Basically, the device is declared in DT as:
ipu1: ipu@02400000 { compatible = "fsl,imx6q-ipu"; ipu1_csi0: port@0 {... }; ipu1_csi1: port@1 { ... }; ipu1_di0: port@2 { ... }; ipu1_di1: port@3 { ... }; };
ipu1 is the platform device, which is the _entire_ IPU device, containing multiple sub-devices - eg, two camera interfaces (csi) and two "CRTCs" (di).
The ipuv3 code creates platform devices for these, calling the CSI devices "imx-ipuv3-camera" and the DI devices "imx-ipuv3-crtc". Initially, these have no of_node attached (yuck!) but later have an of_node attached when the device is probed (yuck yuck yuck!) by ipu_drm_probe() - the of_node being the ipu1_di0 or ipu1_di1 node.
The display-subsystem property references these ipu1_di0/ipu1_di1 nodes:
display-subsystem { compatible = "fsl,imx-display-subsystem"; ports = <&ipu1_di0>, <&ipu1_di1>, <&ipu2_di0>, <&ipu2_di1>; };and so finds the "imx-ipuv3-crtc" platform devices rather than the parent ipu1 device.
It's not nice - I'd like to see this:
if (!dev->of_node) { /* Associate crtc device with the corresponding DI port node */ dev->of_node = ipu_drm_get_port_by_id(dev->parent->of_node, pdata->di + 2); if (!dev->of_node) { dev_err(dev, "missing port@%d node in %s\n", pdata->di + 2, dev->parent->of_node->full_name); return -ENODEV; } }moved out of drivers/gpu/drm/imx/ipuv3-crtc.c and into drivers/gpu/ipu-v3/ipu-common.c where the platform devices are created so that we're only setting device of_node pointers at device creation time and not randomly during the probe sequence, even though the above is "safe" as far as the component helper's concerned. There's no reason why it can't be done at the device creation time.
Hi Russell,
Thanks for your analysis, I keep delaying looking into the imx code more seriously even if I have a SabreLite board that I could play with (not on my day-to-day list of tasks).
Now, as to how to handle the differences here, I think a solution would be to pass in two compare_of function pointers: one for CRTCs and one for the encoders, since the two will need to be handled separately depending on the implementation. Where you have one "parent" device of the CRTC node containing exactly one CRTC, then the "rockchip" method makes sense - though comparing the parent of the port node in CRTC compare_of would be where I'd put it. If you have multiple CRTCs within one parent device, then something more complex like the iMX solution would be required.
In any case, passing port->parent is a data loss in the generic case: you lose the information in the compare_of function about exactly which port is required, so that must go into the CRTC compare_of.
So...
rockchip's CRTC compare_of() should be:
static int crtc_compare_of(struct device *dev, void *data) { struct device_node *np = data;
return dev->of_node == np->parent;}
and it should have an encoder_compare_of() which is its existing compare_of() renamed as such.
Then, we need drm_of_component_probe() to take _two_ comparison functions, one for the CRTCs and one for the encoders.
This hits very close to my experience. After sending this patchset I started converting my HDLCD driver to use it and hit exactly the same issue, that the same compare function is used for selecting CRTCs and encoders. My hardware is even simpler, with one CRTC connected to only one encoder. My board has two of each but the only link between CRTCs is the fact that they are fed most of the time from the same clock source. In the end I've backed out of using the drm_of_component_probe() code as it meant I've had to modify the device tree in a way that did no longer describe the hardware accurately. I will have another look this week if I can extend the function and make it work for all cases.
Meanwhile, what is your suggestion regarding the patchset. I've seen David has sent Linus a pull request for 4.4-rc1 that includes it. Should we send a revert for rockchip commit and then patch later the function?
Best regards, Liviu
-- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
On Mon, Nov 09, 2015 at 11:57:27AM +0000, Liviu Dudau wrote:
Meanwhile, what is your suggestion regarding the patchset. I've seen David has sent Linus a pull request for 4.4-rc1 that includes it. Should we send a revert for rockchip commit and then patch later the function?
It definitely needs to be fixed, and I'd suggest its early enough in the -rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe() to take two compare functions.
I'd also suggest at this point another change: please rename it to drm_of_kms_component_probe() since this is only a generic case for KMS drivers. GPU DRM drivers need something different.
On Mon, Nov 09, 2015 at 12:03:35PM +0000, Russell King - ARM Linux wrote:
On Mon, Nov 09, 2015 at 11:57:27AM +0000, Liviu Dudau wrote:
Meanwhile, what is your suggestion regarding the patchset. I've seen David has sent Linus a pull request for 4.4-rc1 that includes it. Should we send a revert for rockchip commit and then patch later the function?
It definitely needs to be fixed, and I'd suggest its early enough in the -rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe() to take two compare functions.
I still don't have a Rockchip board to test the patch, so I need to find out someone willing to test them. Mark?
I'd also suggest at this point another change: please rename it to drm_of_kms_component_probe() since this is only a generic case for KMS drivers. GPU DRM drivers need something different.
OK, will do.
-- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Hi Liviu,
Am Montag, 9. November 2015, 12:07:20 schrieb Liviu Dudau:
On Mon, Nov 09, 2015 at 12:03:35PM +0000, Russell King - ARM Linux wrote:
On Mon, Nov 09, 2015 at 11:57:27AM +0000, Liviu Dudau wrote:
Meanwhile, what is your suggestion regarding the patchset. I've seen David has sent Linus a pull request for 4.4-rc1 that includes it. Should we send a revert for rockchip commit and then patch later the function?
It definitely needs to be fixed, and I'd suggest its early enough in the -rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe() to take two compare functions.
I still don't have a Rockchip board to test the patch, so I need to find out someone willing to test them. Mark?
I of course also have a plethora of rockchip boards, so can test stuff as well.
Heiko
On 2015年11月10日 03:49, Heiko Stuebner wrote:
Hi Liviu,
Am Montag, 9. November 2015, 12:07:20 schrieb Liviu Dudau:
On Mon, Nov 09, 2015 at 12:03:35PM +0000, Russell King - ARM Linux wrote:
On Mon, Nov 09, 2015 at 11:57:27AM +0000, Liviu Dudau wrote:
Meanwhile, what is your suggestion regarding the patchset. I've seen David has sent Linus a pull request for 4.4-rc1 that includes it. Should we send a revert for rockchip commit and then patch later the function?
It definitely needs to be fixed, and I'd suggest its early enough in the -rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe() to take two compare functions.
I still don't have a Rockchip board to test the patch, so I need to find out someone willing to test them. Mark?
I of course also have a plethora of rockchip boards, so can test stuff as well.
Heiko
Hi Liviu Sorry to reply late , of course I have rockchip boards and can do the test.
Hi Russell and Philipp, Thanks for your analysis, let me know what happen.
On 2015年11月09日 19:57, Liviu Dudau wrote:
Meanwhile, what is your suggestion regarding the patchset. I've seen David has sent Linus a pull request for 4.4-rc1 that includes it. Should we send a revert for rockchip commit and then patch later the function?
Hi Liviu I had sent a patch to revert this rockchip commit, verfied rockchip drm works. So we can do the fix later.
Thanks all the same. :-)
-- Ｍark Yao
On Tue, Nov 10, 2015 at 04:53:09PM +0800, Mark yao wrote:
On 2015年11月09日 19:57, Liviu Dudau wrote:
Meanwhile, what is your suggestion regarding the patchset. I've seen David has sent Linus a pull request for 4.4-rc1 that includes it. Should we send a revert for rockchip commit and then patch later the function?
Hi Liviu I had sent a patch to revert this rockchip commit, verfied rockchip drm works. So we can do the fix later.
Yeah, and I'm thinking about ACK-ing it. On one hand I would like to have this fixed, on the other hand I don't want the rockchip platform to be broken. But looking at the rest of the changes I need to do it doesn't feel right to make you wait for them.
Best regards, Liviu
Thanks all the same. :-)
-- Ｍark Yao
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Oct 19, 2015 at 04:07:47PM +0100, Liviu Dudau wrote:
A lot of component based DRM drivers use a variant of the same code as the probe function. They bind the crtc ports in the first iteration and then scan through the child nodes and bind the encoders attached to the remote endpoints. Factor the common code into a separate function called drm_of_component_probe() in order to increase code reuse.
Sorry, I take back my Acked-by.
- /*
* Bind the crtc's ports first, so that drm_of_find_possible_crtcs()* called from encoder's .bind callbacks works as expected*/- for (i = 0; ; i++) {
port = of_parse_phandle(dev->of_node, "ports", i);if (!port)break;if (!of_device_is_available(port->parent)) {of_node_put(port);continue;}component_match_add(dev, &match, compare_of, port);of_node_put(port);
We shouldn't be putting the ports here - we're still retaining a reference against the port, even though it's by address. Yes, we have no way to release this reference, which probably ought to be fixed. Please replace this of_node_put() here with a comment to that effect.
for_each_child_of_node(port, ep) {remote = of_graph_get_remote_port_parent(ep);if (!remote || !of_device_is_available(remote)) {of_node_put(remote);continue;} else if (!of_device_is_available(remote->parent)) {dev_warn(dev, "parent device of %s is not available\n",remote->full_name);of_node_put(remote);continue;}component_match_add(dev, &match, compare_of, remote);of_node_put(remote);
Same here.
The generic function is functionally equivalent to the driver's imx_drm_platform_probe(). Use the generic function and reduce the overall code size.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com --- drivers/gpu/drm/imx/imx-drm-core.c | 55 +++----------------------------------- 1 file changed, 4 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index de00a6c..64f16ea 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -531,59 +531,12 @@ static const struct component_master_ops imx_drm_ops = {
static int imx_drm_platform_probe(struct platform_device *pdev) { - struct device_node *ep, *port, *remote; - struct component_match *match = NULL; - int ret; - int i; - - /* - * Bind the IPU display interface ports first, so that - * imx_drm_encoder_parse_of called from encoder .bind callbacks - * works as expected. - */ - for (i = 0; ; i++) { - port = of_parse_phandle(pdev->dev.of_node, "ports", i); - if (!port) - break; - - component_match_add(&pdev->dev, &match, compare_of, port); - } + int ret = drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
- if (i == 0) { - dev_err(&pdev->dev, "missing 'ports' property\n"); - return -ENODEV; - } + if (!ret) + ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
- /* Then bind all encoders */ - for (i = 0; ; i++) { - port = of_parse_phandle(pdev->dev.of_node, "ports", i); - if (!port) - break; - - for_each_child_of_node(port, ep) { - remote = of_graph_get_remote_port_parent(ep); - if (!remote || !of_device_is_available(remote)) { - of_node_put(remote); - continue; - } else if (!of_device_is_available(remote->parent)) { - dev_warn(&pdev->dev, "parent device of %s is not available\n", - remote->full_name); - of_node_put(remote); - continue; - } - - component_match_add(&pdev->dev, &match, compare_of, - remote); - of_node_put(remote); - } - of_node_put(port); - } - - ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)); - if (ret) - return ret; - - return component_master_add_with_match(&pdev->dev, &imx_drm_ops, match); + return ret; }
static int imx_drm_platform_remove(struct platform_device *pdev)
On Mon, Oct 19, 2015 at 04:07:48PM +0100, Liviu Dudau wrote:
The generic function is functionally equivalent to the driver's imx_drm_platform_probe(). Use the generic function and reduce the overall code size.
Looks fine, thanks.
Acked-by: Russell King rmk+kernel@arm.linux.org.uk
Use the generic drm_of_component_probe() function to probe for components.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 81 +++-------------------------- 1 file changed, 6 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index f22e1e1..d26e0cc 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -19,6 +19,7 @@ #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> +#include <drm/drm_of.h> #include <linux/dma-mapping.h> #include <linux/pm_runtime.h> #include <linux/module.h> @@ -418,29 +419,6 @@ static int compare_of(struct device *dev, void *data) return dev->of_node == np; }
-static void rockchip_add_endpoints(struct device *dev, - struct component_match **match, - struct device_node *port) -{ - struct device_node *ep, *remote; - - for_each_child_of_node(port, ep) { - remote = of_graph_get_remote_port_parent(ep); - if (!remote || !of_device_is_available(remote)) { - of_node_put(remote); - continue; - } else if (!of_device_is_available(remote->parent)) { - dev_warn(dev, "parent device of %s is not available\n", - remote->full_name); - of_node_put(remote); - continue; - } - - component_match_add(dev, match, compare_of, remote); - of_node_put(remote); - } -} - static int rockchip_drm_bind(struct device *dev) { struct drm_device *drm; @@ -483,61 +461,14 @@ static const struct component_master_ops rockchip_drm_ops = {
static int rockchip_drm_platform_probe(struct platform_device *pdev) { - struct device *dev = &pdev->dev; - struct component_match *match = NULL; - struct device_node *np = dev->of_node; - struct device_node *port; - int i; - - if (!np) - return -ENODEV; - /* - * Bind the crtc ports first, so that - * drm_of_find_possible_crtcs called from encoder .bind callbacks - * works as expected. - */ - for (i = 0;; i++) { - port = of_parse_phandle(np, "ports", i); - if (!port) - break; - - if (!of_device_is_available(port->parent)) { - of_node_put(port); - continue; - } - - component_match_add(dev, &match, compare_of, port->parent); - of_node_put(port); - } + int ret = drm_of_component_probe(&pdev->dev, compare_of, + &rockchip_drm_ops);
- if (i == 0) { - dev_err(dev, "missing 'ports' property\n"); + /* keep compatibility with old code that was returning -ENODEV */ + if (ret == -EINVAL) return -ENODEV; - }
- if (!match) { - dev_err(dev, "No available vop found for display-subsystem.\n"); - return -ENODEV; - } - /* - * For each bound crtc, bind the encoders attached to its - * remote endpoint. - */ - for (i = 0;; i++) { - port = of_parse_phandle(np, "ports", i); - if (!port) - break; - - if (!of_device_is_available(port->parent)) { - of_node_put(port); - continue; - } - - rockchip_add_endpoints(dev, &match, port); - of_node_put(port); - } - - return component_master_add_with_match(dev, &rockchip_drm_ops, match); + return ret; }
static int rockchip_drm_platform_remove(struct platform_device *pdev)
The armada DRM driver keeps some old platform data compatibility in the probe function that makes moving to the generic drm_of_component_probe() a bit more complicated that it should. Refactor the probe function to do the platform_data processing after the generic probe (and only if that fails). This way future cleanup can further remove support for it.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com Cc: Russell King rmk+kernel@arm.linux.org.uk --- drivers/gpu/drm/armada/armada_drv.c | 73 +++++++++++-------------------------- 1 file changed, 22 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index 63d909e..25c42f3 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -11,6 +11,7 @@ #include <linux/of_graph.h> #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> +#include <drm/drm_of.h> #include "armada_crtc.h" #include "armada_drm.h" #include "armada_gem.h" @@ -265,78 +266,48 @@ static void armada_add_endpoints(struct device *dev, } }
-static int armada_drm_find_components(struct device *dev, - struct component_match **match) -{ - struct device_node *port; - int i; - - if (dev->of_node) { - struct device_node *np = dev->of_node; - - for (i = 0; ; i++) { - port = of_parse_phandle(np, "ports", i); - if (!port) - break; - - component_match_add(dev, match, compare_of, port); - of_node_put(port); - } +static const struct component_master_ops armada_master_ops = { + .bind = armada_drm_bind, + .unbind = armada_drm_unbind, +};
- if (i == 0) { - dev_err(dev, "missing 'ports' property\n"); - return -ENODEV; - } +static int armada_drm_probe(struct platform_device *pdev) +{ + struct component_match *match = NULL; + int ret;
- for (i = 0; ; i++) { - port = of_parse_phandle(np, "ports", i); - if (!port) - break; + ret = drm_of_component_probe(&pdev->dev, compare_dev_name, + &armada_master_ops); + if (ret != -EINVAL) + return ret;
- armada_add_endpoints(dev, match, port); - of_node_put(port); - } - } else if (dev->platform_data) { - char **devices = dev->platform_data; + if (pdev->dev.platform_data) { + char **devices = pdev->dev.platform_data; + struct device_node *port; struct device *d; + int i;
for (i = 0; devices[i]; i++) - component_match_add(dev, match, compare_dev_name, + component_match_add(&pdev->dev, match, compare_dev_name, devices[i]);
if (i == 0) { - dev_err(dev, "missing 'ports' property\n"); + dev_err(&pdev->dev, "missing 'ports' property\n"); return -ENODEV; }
for (i = 0; devices[i]; i++) { d = bus_find_device_by_name(&platform_bus_type, NULL, - devices[i]); + devices[i]); if (d && d->of_node) { for_each_child_of_node(d->of_node, port) - armada_add_endpoints(dev, match, port); + armada_add_endpoints(&pdev->dev, match, + port); } put_device(d); } }
- return 0; -} - -static const struct component_master_ops armada_master_ops = { - .bind = armada_drm_bind, - .unbind = armada_drm_unbind, -}; - -static int armada_drm_probe(struct platform_device *pdev) -{ - struct component_match *match = NULL; - int ret; - - ret = armada_drm_find_components(&pdev->dev, &match); - if (ret < 0) - return ret; - return component_master_add_with_match(&pdev->dev, &armada_master_ops, match); }
On Mon, Oct 19, 2015 at 04:07:50PM +0100, Liviu Dudau wrote:
The armada DRM driver keeps some old platform data compatibility in the probe function that makes moving to the generic drm_of_component_probe() a bit more complicated that it should. Refactor the probe function to do the platform_data processing after the generic probe (and only if that fails). This way future cleanup can further remove support for it.
I'm mainly using the old code, even with DT based platforms, because it needs a block of reserved memory which I haven't got around to converting to DT stuff (each time I've looked into doing that, bits for dealing with reserved memory nodes were missing.)
Acked-by: Russell King rmk+kernel@arm.linux.org.uk
On Mon, Oct 19, 2015 at 04:17:14PM +0100, Russell King - ARM Linux wrote:
On Mon, Oct 19, 2015 at 04:07:50PM +0100, Liviu Dudau wrote:
The armada DRM driver keeps some old platform data compatibility in the probe function that makes moving to the generic drm_of_component_probe() a bit more complicated that it should. Refactor the probe function to do the platform_data processing after the generic probe (and only if that fails). This way future cleanup can further remove support for it.
I'm mainly using the old code, even with DT based platforms, because it needs a block of reserved memory which I haven't got around to converting to DT stuff (each time I've looked into doing that, bits for dealing with reserved memory nodes were missing.)
Thanks for explaining this!
Acked-by: Russell King rmk+kernel@arm.linux.org.uk
And thanks for all the ACKs!
Best regards, Liviu
-- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Hi Liviu,
[auto build test WARNING on drm/drm-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]
url: https://github.com/0day-ci/linux/commits/Liviu-Dudau/drm-Introduce-generic-p... config: arm-allmodconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm
All warnings (new ones prefixed by >>):
drivers/gpu/drm/armada/armada_drv.c: In function 'armada_drm_probe':
drivers/gpu/drm/armada/armada_drv.c:291:4: warning: passing argument 2 of 'component_match_add' from incompatible pointer type
component_match_add(&pdev->dev, match, compare_dev_name, ^ In file included from drivers/gpu/drm/armada/armada_drv.c:9:0: include/linux/component.h:36:6: note: expected 'struct component_match **' but argument is of type 'struct component_match *' void component_match_add(struct device *, struct component_match **, ^
drivers/gpu/drm/armada/armada_drv.c:304:6: warning: passing argument 2 of 'armada_add_endpoints' from incompatible pointer type
armada_add_endpoints(&pdev->dev, match, ^ drivers/gpu/drm/armada/armada_drv.c:247:13: note: expected 'struct component_match **' but argument is of type 'struct component_match *' static void armada_add_endpoints(struct device *dev, ^
vim +/component_match_add +291 drivers/gpu/drm/armada/armada_drv.c
285 char **devices = pdev->dev.platform_data; 286 struct device_node *port; 287 struct device *d; 288 int i; 289 290 for (i = 0; devices[i]; i++)
291 component_match_add(&pdev->dev, match, compare_dev_name,
292 devices[i]); 293 294 if (i == 0) { 295 dev_err(&pdev->dev, "missing 'ports' property\n"); 296 return -ENODEV; 297 } 298 299 for (i = 0; devices[i]; i++) { 300 d = bus_find_device_by_name(&platform_bus_type, NULL, 301 devices[i]); 302 if (d && d->of_node) { 303 for_each_child_of_node(d->of_node, port)
304 armada_add_endpoints(&pdev->dev, match,
305 port); 306 } 307 put_device(d);
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
dri-devel@lists.freedesktop.org
-
Eric Anholt -
Heiko Stuebner -
kbuild test robot -
Liviu Dudau -
Mark yao -
Philipp Zabel -
Russell King - ARM Linux