Re: [PATCH v5 01/11] staging: imx-drm-core: Use OF graph to find components and connections between encoder and crtcs
Am Mittwoch, den 05.03.2014, 10:05 + schrieb Russell King - ARM
Linux:
> On Wed, Mar 05, 2014 at 10:20:52AM +0100, Philipp Zabel wrote:
> > +struct imx_drm_component {
> > + struct device_node *of_node;
> > + struct list_head list;
> > +};
> > +
>
> The only thing this structure appears to be doing is ensuring that a
> single component doesn't get added twice - is that correct?
I also think of it as an optimization. Now we scan the whole device
graph once in the probe function into a list of needed components that
can be walked quickly every time master_ops' .add_components callback is
run, instead of having to walk the device tree graph over and over.
Functionally, it only protects against duplicate addition.
> If so, (and the troublesome problem with the IPU crtcs is now gone)
> we can modify the core component code such that it does this:
>
> if (c->master && c->master != master)
> continue;
>
> if (compare(c->dev, compare_data)) {
> if (!c->master)
> component_attach_master(master, c);
> ret = 0;
> break;
> }
>
> which will mean that you don't need to build this list anymore to track
> what will be added - though I'd like to think a little more about that
> before making that change. Please confirm whether this will eliminate
> your list generation.
Yes, I can confirm that with this change, I can remove the list, like
this (tested on i.MX6S with a single LVDS panel):
diff --git a/drivers/staging/imx-drm/imx-drm-core.c
b/drivers/staging/imx-drm/imx-drm-core.c
index 014e546..f6135b9 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -32,11 +32,6 @@
struct imx_drm_crtc;
-struct imx_drm_component {
- struct device_node *of_node;
- struct list_head list;
-};
-
struct imx_drm_device {
struct drm_device *drm;
struct imx_drm_crtc *crtc[MAX_CRTC];
@@ -587,72 +582,10 @@ static int compare_of(struct device *dev, void
*data)
return dev->of_node == np;
}
-static LIST_HEAD(imx_drm_components);
-
static int imx_drm_add_components(struct device *master, struct master
*m)
{
- struct imx_drm_component *component;
- int ret;
-
- list_for_each_entry(component, &imx_drm_components, list) {
- ret = component_master_add_child(m, compare_of,
-component->of_node);
- if (ret)
- return ret;
- }
- return 0;
-}
-
-static int imx_drm_bind(struct device *dev)
-{
- return drm_platform_init(&imx_drm_driver, to_platform_device(dev));
-}
-
-static void imx_drm_unbind(struct device *dev)
-{
- drm_put_dev(dev_get_drvdata(dev));
-}
-
-static const struct component_master_ops imx_drm_ops = {
- .add_components = imx_drm_add_components,
- .bind = imx_drm_bind,
- .unbind = imx_drm_unbind,
-};
-
-static struct imx_drm_component *imx_drm_find_component(struct device
*dev,
- struct device_node *node)
-{
- struct imx_drm_component *component;
-
- list_for_each_entry(component, &imx_drm_components, list)
- if (component->of_node == node)
- return component;
-
- return NULL;
-}
-
-static int imx_drm_add_component(struct device *dev, struct device_node
*node)
-{
- struct imx_drm_component *component;
-
- if (imx_drm_find_component(dev, node))
- return 0;
-
- component = devm_kzalloc(dev, sizeof(*component), GFP_KERNEL);
- if (!component)
- return -ENOMEM;
-
- component->of_node = node;
- list_add_tail(&component->list, &imx_drm_components);
-
- return 0;
-}
-
-static int imx_drm_platform_probe(struct platform_device *pdev)
-{
struct device_node *ep, *port, *remote;
- int ret;
- int i;
+ int i, ret;
/*
* Bind the IPU display interface ports first, so that
@@ -660,23 +593,18 @@ static int imx_drm_platform_probe(struct
platform_device *pdev)
* works as expected.
*/
for (i = 0; ; i++) {
- port = of_parse_phandle(pdev->dev.of_node, "ports", i);
+ port = of_parse_phandle(master->of_node, "ports", i);
if (!port)
break;
- ret = imx_drm_add_component(&pdev->dev, port);
- if (ret < 0)
+ ret = component_master_add_child(m, compare_of, port);
+ if (ret)
return ret;
}
- if (i == 0) {
- dev_err(&pdev->dev, "missing 'ports' property\n");
- return -ENODEV;
- }
-
/* Then bind all encoders */
for (i = 0; ; i++) {
- port = of_parse_phandle(pdev->dev.of_node, "p
Re: [PATCH v5 01/11] staging: imx-drm-core: Use OF graph to find components and connections between encoder and crtcs
On Wed, Mar 05, 2014 at 10:20:52AM +0100, Philipp Zabel wrote:
> +struct imx_drm_component {
> + struct device_node *of_node;
> + struct list_head list;
> +};
> +
The only thing this structure appears to be doing is ensuring that a
single component doesn't get added twice - is that correct? If so,
(and the troublesome problem with the IPU crtcs is now gone)
we can modify the core component code such that it does this:
if (c->master && c->master != master)
continue;
if (compare(c->dev, compare_data)) {
if (!c->master)
component_attach_master(master, c);
ret = 0;
break;
}
which will mean that you don't need to build this list anymore to track
what will be added - though I'd like to think a little more about that
before making that change. Please confirm whether this will eliminate
your list generation.
Thanks.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v5 01/11] staging: imx-drm-core: Use OF graph to find components and connections between encoder and crtcs
From: Philipp Zabel
This patch adds support to find the involved components connected to
the IPU display interface ports using the OF graph bindings documented
in Documentation/devicetree/bindings/media/video-interfaces.txt.
It makes use of the of_graph (formerly v4l2_of) parsing helpers and
thus depends on the patch that moves those out to drivers/of.
Each display interface needs to have an associated port node in the
device tree. We can associate this node with the crtc platform device
and use it to find the crtc corresponding to a given port node instead
of using a combination of parent device node and id number, as before.
Explicitly converting the void* cookie to the port device tree node
allows to get rid of the ipu_id and di_id fields. The multiplexer
setting on i.MX6 now can be obtained from the port id (reg property)
in the device tree.
The imx-drm node now needs a ports property that contains phandles
to each of the IPU display interface port nodes. From there, all
attached encoders are scanned and enabled encoders are added to a
waiting list.
The bind order makes sure that once all components are probed, crtcs
are bound before encoders, so that imx_drm_encoder_parse_of can be
called from the encoder bind callbacks.
For parsing the OF graph, temporary copies of the V4L2 OF graph
helpers are used, that can be removed again once those are available
at a generic place.
Signed-off-by: Philipp Zabel
---
Changes since v4:
- changed DT compatible string to 'fsl,imx-display-subsystem' instead
of Linux specific 'fsl,imx-drm'
---
drivers/staging/imx-drm/imx-drm-core.c | 217 +++--
drivers/staging/imx-drm/imx-drm.h | 5 +-
drivers/staging/imx-drm/imx-hdmi.c | 2 +-
drivers/staging/imx-drm/imx-ldb.c | 4 +-
drivers/staging/imx-drm/ipuv3-crtc.c | 47 +--
5 files changed, 199 insertions(+), 76 deletions(-)
diff --git a/drivers/staging/imx-drm/imx-drm-core.c
b/drivers/staging/imx-drm/imx-drm-core.c
index 6b91c8e..ecfc88b 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -30,6 +31,11 @@
struct imx_drm_crtc;
+struct imx_drm_component {
+ struct device_node *of_node;
+ struct list_head list;
+};
+
struct imx_drm_device {
struct drm_device *drm;
struct imx_drm_crtc *crtc[MAX_CRTC];
@@ -41,9 +47,7 @@ struct imx_drm_crtc {
struct drm_crtc *crtc;
int pipe;
struct imx_drm_crtc_helper_funcsimx_drm_helper_funcs;
- void*cookie;
- int id;
- int mux_id;
+ struct device_node *port;
};
static int legacyfb_depth = 16;
@@ -341,14 +345,11 @@ err_kms:
/*
* imx_drm_add_crtc - add a new crtc
- *
- * The return value if !NULL is a cookie for the caller to pass to
- * imx_drm_remove_crtc later.
*/
int imx_drm_add_crtc(struct drm_device *drm, struct drm_crtc *crtc,
struct imx_drm_crtc **new_crtc,
const struct imx_drm_crtc_helper_funcs *imx_drm_helper_funcs,
- void *cookie, int id)
+ struct device_node *port)
{
struct imx_drm_device *imxdrm = drm->dev_private;
struct imx_drm_crtc *imx_drm_crtc;
@@ -370,9 +371,7 @@ int imx_drm_add_crtc(struct drm_device *drm, struct
drm_crtc *crtc,
imx_drm_crtc->imx_drm_helper_funcs = *imx_drm_helper_funcs;
imx_drm_crtc->pipe = imxdrm->pipes++;
- imx_drm_crtc->cookie = cookie;
- imx_drm_crtc->id = id;
- imx_drm_crtc->mux_id = imx_drm_crtc->pipe;
+ imx_drm_crtc->port = port;
imx_drm_crtc->crtc = crtc;
imxdrm->crtc[imx_drm_crtc->pipe] = imx_drm_crtc;
@@ -416,49 +415,56 @@ int imx_drm_remove_crtc(struct imx_drm_crtc *imx_drm_crtc)
EXPORT_SYMBOL_GPL(imx_drm_remove_crtc);
/*
- * Find the DRM CRTC possible mask for the device node cookie/id.
+ * Find the DRM CRTC possible mask for the connected endpoint.
*
* The encoder possible masks are defined by their position in the
* mode_config crtc_list. This means that CRTCs must not be added
* or removed once the DRM device has been fully initialised.
*/
static uint32_t imx_drm_find_crtc_mask(struct imx_drm_device *imxdrm,
- void *cookie, int id)
+ struct device_node *endpoint)
{
+ struct device_node *port;
unsigned i;
+ port = of_graph_get_remote_port(endpoint);
+ if (!port)
+ return 0;
+ of_node_put(port);
+
for (i = 0; i < MAX_CRTC; i++) {
struct imx_drm_crtc *imx_drm_crtc = imxdrm->crtc[i];
- if (imx_drm_crtc && imx_drm_crtc->id == id &&
- imx_drm_crtc->cooki
