On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar ajaykumar.rs@samsung.com wrote:

Assign the pointer to bridge ops structure(drm_bridge_funcs) in the bridge driver itself, instead of passing it to drm_bridge_init.

This will allow bridge driver developer to pack bridge private information inside the bridge object and pass only the drm-relevant information to drm_bridge_init.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

This matches how drm_panel works, which it looks like this set is trying to emulate.

Acked-by: Sean Paul seanpaul@chromium.org

---

drivers/gpu/drm/bridge/ptn3460.c | 3 ++- drivers/gpu/drm/drm_crtc.c | 5 +---- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 ++- drivers/gpu/drm/sti/sti_hda.c | 3 ++- drivers/gpu/drm/sti/sti_hdmi.c | 3 ++- include/drm/drm_crtc.h | 3 +-- 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index 4db38e1..a2ddc8d 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -313,7 +313,8 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, goto err; }

•   ret = drm_bridge_init(dev, &ptn_bridge->bridge, &ptn3460_bridge_funcs);
  

•   ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
  

•   ret = drm_bridge_init(dev, &ptn_bridge->bridge);
    if (ret) {
            DRM_ERROR("Failed to initialize bridge with drm\n");
            goto err;
  

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fa2be24..25f5cfa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1022,7 +1022,6 @@ EXPORT_SYMBOL(drm_connector_unplug_all);

• drm_bridge_init - initialize a drm transcoder/bridge
• @dev: drm device
• @bridge: transcoder/bridge to set up

• • @funcs: bridge function table
  • Initialises a preallocated bridge. Bridges should be
  • subclassed as part of driver connector objects.

@@ -1030,8 +1029,7 @@ EXPORT_SYMBOL(drm_connector_unplug_all);

• Returns:
• Zero on success, error code on failure.

*/ -int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,

•           const struct drm_bridge_funcs *funcs)
  

+int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge) { int ret;

@@ -1042,7 +1040,6 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, goto out;

    bridge->dev = dev;

•   bridge->funcs = funcs;
  
    list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
    dev->mode_config.num_bridge++;
  

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index f6cf745..0309539 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -221,8 +221,9 @@ struct drm_bridge *hdmi_bridge_init(struct hdmi *hdmi) hdmi_bridge->hdmi = hdmi_reference(hdmi);

    bridge = &hdmi_bridge->base;

•   bridge->funcs = &hdmi_bridge_funcs;
  
  

•   drm_bridge_init(hdmi->dev, bridge, &hdmi_bridge_funcs);
  

•   drm_bridge_init(hdmi->dev, bridge);
  
    return bridge;
  
  

diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c index 72d957f..b3344c7 100644 --- a/drivers/gpu/drm/sti/sti_hda.c +++ b/drivers/gpu/drm/sti/sti_hda.c @@ -664,7 +664,8 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data) return -ENOMEM;

    bridge->driver_private = hda;

•   drm_bridge_init(drm_dev, bridge, &sti_hda_bridge_funcs);
  

•   bridge->funcs = &sti_hda_bridge_funcs;
  

•   drm_bridge_init(drm_dev, bridge);
  
    encoder->bridge = bridge;
    connector->encoder = encoder;
  

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c index 284e541..3ad319f 100644 --- a/drivers/gpu/drm/sti/sti_hdmi.c +++ b/drivers/gpu/drm/sti/sti_hdmi.c @@ -629,7 +629,8 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data) return -ENOMEM;

    bridge->driver_private = hdmi;

•   drm_bridge_init(drm_dev, bridge, &sti_hdmi_bridge_funcs);
  

•   bridge->funcs = &sti_hdmi_bridge_funcs;
  

•   drm_bridge_init(drm_dev, bridge);
  
    encoder->bridge = bridge;
    connector->encoder = encoder;
  

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f1105d0..f48a436 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -906,8 +906,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); /* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev);

-extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,

•                      const struct drm_bridge_funcs *funcs);
  

+extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); extern void drm_bridge_cleanup(struct drm_bridge *bridge);

extern int drm_encoder_init(struct drm_device *dev,

1.7.9.5

So don't ask why but I accidentally ended up in a branch looking at this patch and didn't like it. So very quick&grumpy review.

First, please make the patch subject more descriptive: I'd expect a helper function scaffolding like the various crtc/probe/dp ... helpers we already have. You instead add code to untangle the probe ordering. Please say so.

More comments below.

On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:

A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add".

The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.

The encoder driver should also call "drm_bridge_attach" to pass on the drm_device, encoder pointers to the bridge object.

drm_bridge_attach inturn calls drm_bridge_init to register itself with the drm core. Later, it calls "bridge->funcs->attach" so that bridge can continue with other initializations.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

[snip]

@@ -660,8 +662,11 @@ struct drm_bridge_funcs {

• @driver_private: pointer to the bridge driver's internal context

*/ struct drm_bridge {

• struct drm_device *dev;

• struct device *dev;

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

• struct drm_device *drm;
• struct drm_encoder *encoder;

This breaks bridge->bridge chaining (if we ever get there). It seems pretty much unused anyway except for an EBUSY check. Can't you use bridge->dev for that?

struct list_head head;

• struct list_head list;

These lists need better names. I know that the "head" is really awful, especially since it's actually not the head of the list but just an element.

struct drm_mode_object base;

Aside: I've noticed all this trying to update the kerneldoc for struct drm_bridge, which just showed that this patch makes inconsistent changes. Trying to write kerneldoc is a really great way to come up with better interfaces imo.

Cheers, Daniel

@@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); /* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev);

+extern int drm_bridge_add(struct drm_bridge *bridge); +extern void drm_bridge_remove(struct drm_bridge *bridge); +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); +extern int drm_bridge_attach(struct drm_bridge *bridge,

• 		struct drm_encoder *encoder);
  

extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); extern void drm_bridge_cleanup(struct drm_bridge *bridge);

-- 1.7.9.5

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote:

...

...

@@ -660,8 +662,11 @@ struct drm_bridge_funcs {

• @driver_private: pointer to the bridge driver's internal context

*/ struct drm_bridge {

• struct drm_device *dev;
  

• struct device *dev;
  

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

I think this is modeled after the naming in drm_panel, FWIW. However, seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.

Thierry? -Daniel

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:

Looking at the of_drm_find_panel function I actually wonder how that works - the drm_panel doesn't really need to stick around afaics. After all panel_list is global so some other driver can unload. Russell's of support for possible_crtcs code works differently since it only looks at per-drm_device lists.

This bridge code here though suffers from the same. So to me this looks rather fishy.

It doesn't help that we have drm_of.[hc] around but not all the of code is in there. Adding Russell too.

I rather dropped the ball with imx-drm when things became difficult with asking Greg to pull my git tree - and as a result, I decided that I would no longer be helping with patch submission for imx-drm, nor trying to get it out of the staging tree anymore.

I do still have the patch (dated from July) in my git tree which adds it to imx-drm - see below. Rebased to 3.18-rc2 today.

I also have a patch (from April) which creates a generic OF DDC connector helper, but that remains unfinished, in the sense that it lives beside imx-drm, pulling the DDC specific code out of imx-hdmi and imx-tve, even though there's nothing imx-drm specific about it.

8<==== From: Russell King rmk+kernel@arm.linux.org.uk Subject: [PATCH] imx-drm: convert imx-drm to use the generic DRM OF helper

Use the generic DRM OF helper to locate the possible CRTCs for the encoder, thereby shrinking the imx-drm driver some more.

Acked-by: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Russell King rmk+kernel@arm.linux.org.uk --- drivers/staging/imx-drm/imx-drm-core.c | 72 ++++++---------------------------- 1 file changed, 13 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index 9cb222e2996f..5e2c1f4b9165 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -24,6 +24,7 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_fb_cma_helper.h> +#include <drm/drm_of.h>

#include "imx-drm.h"

@@ -47,7 +48,6 @@ struct imx_drm_crtc { struct drm_crtc *crtc; int pipe; struct imx_drm_crtc_helper_funcs imx_drm_helper_funcs; - struct device_node *port; };

static int legacyfb_depth = 16; @@ -366,9 +366,10 @@ 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->port = port; imx_drm_crtc->crtc = crtc;

+ crtc->port = port; + imxdrm->crtc[imx_drm_crtc->pipe] = imx_drm_crtc;

*new_crtc = imx_drm_crtc; @@ -409,34 +410,6 @@ 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 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, - 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->port == port) - return drm_crtc_mask(imx_drm_crtc->crtc); - } - - return 0; -} - static struct device_node *imx_drm_of_get_next_endpoint( const struct device_node *parent, struct device_node *prev) { @@ -449,35 +422,16 @@ static struct device_node *imx_drm_of_get_next_endpoint( int imx_drm_encoder_parse_of(struct drm_device *drm, struct drm_encoder *encoder, struct device_node *np) { - struct imx_drm_device *imxdrm = drm->dev_private; - struct device_node *ep = NULL; - uint32_t crtc_mask = 0; - int i; + uint32_t crtc_mask = drm_of_find_possible_crtcs(drm, np);

- for (i = 0; ; i++) { - u32 mask; - - ep = imx_drm_of_get_next_endpoint(np, ep); - if (!ep) - break; - - mask = imx_drm_find_crtc_mask(imxdrm, ep); - - /* - * If we failed to find the CRTC(s) which this encoder is - * supposed to be connected to, it's because the CRTC has - * not been registered yet. Defer probing, and hope that - * the required CRTC is added later. - */ - if (mask == 0) - return -EPROBE_DEFER; - - crtc_mask |= mask; - } - - of_node_put(ep); - if (i == 0) - return -ENOENT; + /* + * If we failed to find the CRTC(s) which this encoder is + * supposed to be connected to, it's because the CRTC has + * not been registered yet. Defer probing, and hope that + * the required CRTC is added later. + */ + if (crtc_mask == 0) + return -EPROBE_DEFER;

encoder->possible_crtcs = crtc_mask;

@@ -511,7 +465,7 @@ int imx_drm_encoder_get_mux_id(struct device_node *node,

port = of_graph_get_remote_port(ep); of_node_put(port); - if (port == imx_crtc->port) { + if (port == imx_crtc->crtc->port) { ret = of_graph_parse_endpoint(ep, &endpoint); return ret ? ret : endpoint.port; }

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:

On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote:

...

...

...

@@ -660,8 +662,11 @@ struct drm_bridge_funcs {

• @driver_private: pointer to the bridge driver's internal context

*/ struct drm_bridge {

• struct drm_device *dev;
  

• struct device *dev;
  

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

I think this is modeled after the naming in drm_panel, FWIW. However, seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.

Thierry?

Looking at the of_drm_find_panel function I actually wonder how that works - the drm_panel doesn't really need to stick around afaics. After all panel_list is global so some other driver can unload. Russell's of support for possible_crtcs code works differently since it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by some driver that isn't tied to the DRM device until attached. It can't be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the typical use-case.

This bridge code here though suffers from the same. So to me this looks rather fishy.

Well, this version of the DRM bridge support was written to be close to DRM panel, so it isn't really surprising that it's similar =), but like I said, I don't really understand what you think is wrong with it.

It doesn't help that we have drm_of.[hc] around but not all the of code is in there. Adding Russell too.

DRM panel and DRM bridge aren't just OF helpers. They can be used with whatever type of device you want.

Thierry

On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:

On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:

...

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:

...

On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote:

...

...

> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > * @driver_private: pointer to the bridge driver's internal context > */ > struct drm_bridge { > - struct drm_device *dev; > + struct device *dev;

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

I think this is modeled after the naming in drm_panel, FWIW. However, seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.

Thierry?

Looking at the of_drm_find_panel function I actually wonder how that works - the drm_panel doesn't really need to stick around afaics. After all panel_list is global so some other driver can unload. Russell's of support for possible_crtcs code works differently since it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by some driver that isn't tied to the DRM device until attached. It can't be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the typical use-case.

...

This bridge code here though suffers from the same. So to me this looks rather fishy.

Well, this version of the DRM bridge support was written to be close to DRM panel, so it isn't really surprising that it's similar =), but like I said, I don't really understand what you think is wrong with it.

You have a mutex to protect the global list of bridges/panels. But if you do a lookup you don't grab a reference count on the panel, so the moment you drop the list mutex the panel/bridge can go away.

Yes usually you don't unload a driver on a soc but soc isn't everything and designing new stuff to not be hotunplug save isn't great either.

Yeah, I certainly agree that adding proper reference counting would be a good thing. I think perhaps we could just take a reference on the struct device * to prevent it from disappearing.

Although perhaps I misunderstand what you mean by "go away".

Thierry

On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:

On Wed, Oct 29, 2014 at 09:38:23AM +0100, Thierry Reding wrote:

...

On Wed, Oct 29, 2014 at 08:43:14AM +0100, Daniel Vetter wrote:

...

On Tue, Oct 28, 2014 at 03:35:50PM +0100, Thierry Reding wrote:

...

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:

...

On Mon, Oct 27, 2014 at 11:20 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote: >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>> * @driver_private: pointer to the bridge driver's internal context >>> */ >>> struct drm_bridge { >>> - struct drm_device *dev; >>> + struct device *dev; >> >> Please don't rename the ->dev pointer into drm. Because _all_ the other >> drm structures still call it ->dev. Also, can't we use struct device_node >> here like we do in the of helpers Russell added? See 7e435aad38083 >> > > I think this is modeled after the naming in drm_panel, FWIW. However, > seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.

Thierry?

Looking at the of_drm_find_panel function I actually wonder how that works - the drm_panel doesn't really need to stick around afaics. After all panel_list is global so some other driver can unload. Russell's of support for possible_crtcs code works differently since it only looks at per-drm_device lists.

I don't understand. Panels are global resources that get registered by some driver that isn't tied to the DRM device until attached. It can't be in a per-DRM device list, because it's external to the device.

And yes, they can go away when a driver is unloaded, though it's not the typical use-case.

...

This bridge code here though suffers from the same. So to me this looks rather fishy.

Well, this version of the DRM bridge support was written to be close to DRM panel, so it isn't really surprising that it's similar =), but like I said, I don't really understand what you think is wrong with it.

You have a mutex to protect the global list of bridges/panels. But if you do a lookup you don't grab a reference count on the panel, so the moment you drop the list mutex the panel/bridge can go away.

Yes usually you don't unload a driver on a soc but soc isn't everything and designing new stuff to not be hotunplug save isn't great either.

Yeah, I certainly agree that adding proper reference counting would be a good thing. I think perhaps we could just take a reference on the struct device * to prevent it from disappearing.

Although perhaps I misunderstand what you mean by "go away".

I meant the drm_panel/bridge could go away any second, since nothing prevents a module unload of the panel/bridge driver. So in theory you could get the unregister call right after you've done the lookup. Which means the bridge/panel pointer you've just returned is freed memory.

Ah yes, I see now.

I think we nee try_get_module for the code and kref on the actual data structures.

Agreed, that should do the trick. We'd probably need some sort of logic to also make operations return something like -ENODEV when the underlying device has disappeared. I think David had introduced something similar for DRM device not so long ago?

That makes the entire thing a bit non-trivial, which is why I think it would be better as some generic helper. Which then gets embedded or instantiated for specific cases, like dt&drm_panel or dt&drm_bridge. Or maybe even acpi&drm_bridge, who knows ;-)

I worry a little about type safety. How will this generic helper know what registry to look in for? Or conversely, if all these resources are added to a single registry how do you know that they're of the correct type? Failing to ensure this could cause situations where you're asking for a panel and get a bridge in return because you've wrongly wired it up in device tree for example.

But perhaps if both the registry and the device parts are turned into helpers we could still have a single core implementation and then instantiate that for each type, something roughly like this:

struct registry { struct list_head list; struct mutex lock; };

struct registry_record { struct list_head list; struct module *owner; struct kref *ref;

struct device *dev; };

int registry_add(struct registry *registry, struct registry_record *record) { ... try_module_get(record->owner); ... }

struct registry_record *registry_find_by_of_node(struct registry *registry, struct device_node *np) { ... kref_get(...); ... }

That way it should be possible to embed these into other structures, like so:

struct drm_panel { struct registry_record record; ... };

static struct registry drm_panels;

int drm_panel_add(struct drm_panel *panel) { return registry_add(&drm_panels, &panel->record); }

struct drm_panel *of_drm_panel_find(struct device_node *np) { struct registry_record *record;

record = registry_find_by_of_node(&drm_panels, np);

return container_of(record, struct drm_panel, record); }

Is that what you had in mind?

Thierry

On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:

On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:

...

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote:

...

...

...

@@ -660,8 +662,11 @@ struct drm_bridge_funcs {

• @driver_private: pointer to the bridge driver's internal context

*/ struct drm_bridge {

• struct drm_device *dev;
  

• struct device *dev;
  

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

I think this is modeled after the naming in drm_panel, FWIW. However, seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.

Thierry?

The struct device * is in DRM panel because there's nothing device tree specific about the concept. Having a struct device_node * instead would indicate that it can only be used with a device tree, whereas the framework doesn't care the tiniest bit what type of device we have.

While the trend clearly is to use more device tree, I don't think we should make it impossible for anybody else to use these frameworks.

There are other advantages to keeping a struct device *, like having access to the proper device and its name. Also you get access to the device_node * via dev->of_node anyway. I don't see any advantage in switching to just a struct device_node *, only disadvantages.

Well the idea is to make the lookup key specific, and conditional on #CONFIG_OF. If there's going to be another neat way to enumerate platform devices then I think we should add that, too. Or maybe we should have a void *platform_data or so.

The reason I really don't want a struct device * in core drm structures is that two releases down the road people will have found tons of really great ways to abuse them and re-create a midlayer. DRM core really should only care about the sw objects and not be hw specific at all. Heck there's not even an requirement to have any piece of actual hw, you could write a completely fake drm driver (for e.g. testing like the new v4l driver).

Tbh I wonder a bit why we even have this registery embedded into the core drm objects. Essentially the only thing you're doing is a list that maps some platform specific key onto some subsystem specific driver structure or fails the lookup. So instead of putting all these low-level details into drm core structures can't we just have a generic hashtable/list for this, plus some static inline helpers that cast the void * you get into the one you want?

I also get the feeling that this really should be in the driver core (like the component helpers), and that we should think a lot harder about lifetimes and refcounting (see my other reply on that). -Daniel

On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:

On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:

...

On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:

...

On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:

...

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote:

...

...

> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { > * @driver_private: pointer to the bridge driver's internal context > */ > struct drm_bridge { > - struct drm_device *dev; > + struct device *dev;

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

I think this is modeled after the naming in drm_panel, FWIW. However, seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.

Thierry?

The struct device * is in DRM panel because there's nothing device tree specific about the concept. Having a struct device_node * instead would indicate that it can only be used with a device tree, whereas the framework doesn't care the tiniest bit what type of device we have.

While the trend clearly is to use more device tree, I don't think we should make it impossible for anybody else to use these frameworks.

There are other advantages to keeping a struct device *, like having access to the proper device and its name. Also you get access to the device_node * via dev->of_node anyway. I don't see any advantage in switching to just a struct device_node *, only disadvantages.

Well the idea is to make the lookup key specific, and conditional on #CONFIG_OF. If there's going to be another neat way to enumerate platform devices then I think we should add that, too. Or maybe we should have a void *platform_data or so.

The reason I really don't want a struct device * in core drm structures is that two releases down the road people will have found tons of really great ways to abuse them and re-create a midlayer. DRM core really should only care about the sw objects and not be hw specific at all. Heck there's not even an requirement to have any piece of actual hw, you could write a completely fake drm driver (for e.g. testing like the new v4l driver).

Tbh I wonder a bit why we even have this registery embedded into the core drm objects. Essentially the only thing you're doing is a list that maps some platform specific key onto some subsystem specific driver structure or fails the lookup. So instead of putting all these low-level details into drm core structures can't we just have a generic hashtable/list for this, plus some static inline helpers that cast the void * you get into the one you want?

I also get the feeling that this really should be in the driver core (like the component helpers), and that we should think a lot harder about lifetimes and refcounting (see my other reply on that).

Yes, that sounds very useful indeed. Also see my reply to yours. =)

Just replying here with some of the irc discussions we've had. Since drm_bridge/panel isn't a core drm interface object exposed to userspace it's less critical. I still think that wasting a few brain cycles to have a clear separation between the abstract interface object and how to bind and unbind the pieces together is worthwhile, even though the cost when getting it wrong is much less severe than in the case of a mandatory piece of core infrastructure.

I think in general the recent armsoc motivated drm infrastructure lacks a bit in attention to these details. E.g. the cma helpers are built into the drm.ko module, but clearly are auxilliary library code. So they should be pulled out and the headers clean up, so that we have a clear separation between core and helpers. Otherwise someone will sooner or later screw up and insert a helper depency into the core, and then we've started with the midlayer mess. Same goes with drm_bridge/panel, which didn't even bother to have separate headers from the core modeset header drm_crtc.h.

So would be great if someone could invest a bit of time into cleaning this up. Writing proper api docs also helps a lot with achieving a clean and sensible split ;-) -Daniel

On Mon, Nov 03, 2014 at 09:06:04AM +0100, Thierry Reding wrote:

On Fri, Oct 31, 2014 at 04:59:40PM +0100, Daniel Vetter wrote:

...

On Wed, Oct 29, 2014 at 10:16:49AM +0100, Thierry Reding wrote:

...

On Wed, Oct 29, 2014 at 08:51:27AM +0100, Daniel Vetter wrote:

...

On Tue, Oct 28, 2014 at 03:29:47PM +0100, Thierry Reding wrote:

...

On Mon, Oct 27, 2014 at 11:20:31PM +0100, Daniel Vetter wrote:

...

On Mon, Oct 27, 2014 at 8:58 PM, Sean Paul seanpaul@chromium.org wrote: >>> @@ -660,8 +662,11 @@ struct drm_bridge_funcs { >>> * @driver_private: pointer to the bridge driver's internal context >>> */ >>> struct drm_bridge { >>> - struct drm_device *dev; >>> + struct device *dev; >> >> Please don't rename the ->dev pointer into drm. Because _all_ the other >> drm structures still call it ->dev. Also, can't we use struct device_node >> here like we do in the of helpers Russell added? See 7e435aad38083 >> > > I think this is modeled after the naming in drm_panel, FWIW. However, > seems reasonable to keep the device_node instead.

Hm, indeed. Tbh I vote to rename drm_panel->drm to ->dev and like with drm_crtc drop the struct device and go directly to a struct device_node. Since we don't really need the sturct device, the only thing we care about is the of_node. For added bonus wrap an #ifdef CONFIG_OF around all the various struct device_node in drm_foo.h. Should be all fairly simple to pull off with cocci.

Thierry?

The struct device * is in DRM panel because there's nothing device tree specific about the concept. Having a struct device_node * instead would indicate that it can only be used with a device tree, whereas the framework doesn't care the tiniest bit what type of device we have.

While the trend clearly is to use more device tree, I don't think we should make it impossible for anybody else to use these frameworks.

There are other advantages to keeping a struct device *, like having access to the proper device and its name. Also you get access to the device_node * via dev->of_node anyway. I don't see any advantage in switching to just a struct device_node *, only disadvantages.

Well the idea is to make the lookup key specific, and conditional on #CONFIG_OF. If there's going to be another neat way to enumerate platform devices then I think we should add that, too. Or maybe we should have a void *platform_data or so.

The reason I really don't want a struct device * in core drm structures is that two releases down the road people will have found tons of really great ways to abuse them and re-create a midlayer. DRM core really should only care about the sw objects and not be hw specific at all. Heck there's not even an requirement to have any piece of actual hw, you could write a completely fake drm driver (for e.g. testing like the new v4l driver).

Tbh I wonder a bit why we even have this registery embedded into the core drm objects. Essentially the only thing you're doing is a list that maps some platform specific key onto some subsystem specific driver structure or fails the lookup. So instead of putting all these low-level details into drm core structures can't we just have a generic hashtable/list for this, plus some static inline helpers that cast the void * you get into the one you want?

I also get the feeling that this really should be in the driver core (like the component helpers), and that we should think a lot harder about lifetimes and refcounting (see my other reply on that).

Yes, that sounds very useful indeed. Also see my reply to yours. =)

Just replying here with some of the irc discussions we've had. Since drm_bridge/panel isn't a core drm interface object exposed to userspace it's less critical. I still think that wasting a few brain cycles to have a clear separation between the abstract interface object and how to bind and unbind the pieces together is worthwhile, even though the cost when getting it wrong is much less severe than in the case of a mandatory piece of core infrastructure.

I think in general the recent armsoc motivated drm infrastructure lacks a bit in attention to these details. E.g. the cma helpers are built into the drm.ko module, but clearly are auxilliary library code. So they should be pulled out and the headers clean up, so that we have a clear separation between core and helpers. Otherwise someone will sooner or later screw up and insert a helper depency into the core, and then we've started with the midlayer mess. Same goes with drm_bridge/panel, which didn't even bother to have separate headers from the core modeset header drm_crtc.h.

DRM panel does have a separate header. It's still built into the core DRM module, but using a separate drm-$(CONFIG_DRM_PANEL) += drm_panel.o entry in the makefile. At the time it didn't seem worth to add a completely separate module given the size of the code and the overhead associated with having a separate module.

Do you still want me to split it off into a separate module to clarify that it isn't part of the core?

Oh, it doesn't need to be a complete standalone module, smashing it into drm_kms_helper is imo totally ok. It's just to make really sure that helpers are helpers and there's never a depency from drm.ko to any optional helper code. Having a separate module for all the helper code helps a lot in ensure that. Also, everything a helper can do, a driver should be able to do too. Again a separate helper ensures that you haven't missed any EXPORT_SYMBOL. Which should then be a good reminder to update the kerneldoc ;-)

...

So would be great if someone could invest a bit of time into cleaning this up. Writing proper api docs also helps a lot with achieving a clean and sensible split ;-)

There's a bit of API documentation for panels, but I'll see if I can find some time to enhance it.

Imo pulling into the DocBook template is also important, since if you do that the 0-day tester will complain if the kerneldoc gets out of sync. Which does increases the changes of it staying up-to-date a lot.

Cheers, Daniel

On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajaynumb@gmail.com wrote:

Hi Daniel and Sean,

Thanks for the comments!

On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanpaul@chromium.org wrote:

...

On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

So don't ask why but I accidentally ended up in a branch looking at this patch and didn't like it. So very quick&grumpy review.

First, please make the patch subject more descriptive: I'd expect a helper function scaffolding like the various crtc/probe/dp ... helpers we already have. You instead add code to untangle the probe ordering. Please say so.

Sure. I will reword it properly.

...

...

More comments below.

On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:

...

A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add".

The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.

The encoder driver should also call "drm_bridge_attach" to pass on the drm_device, encoder pointers to the bridge object.

drm_bridge_attach inturn calls drm_bridge_init to register itself with the drm core. Later, it calls "bridge->funcs->attach" so that bridge can continue with other initializations.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

[snip]

...

@@ -660,8 +662,11 @@ struct drm_bridge_funcs {

• @driver_private: pointer to the bridge driver's internal context

*/ struct drm_bridge {

• struct drm_device *dev;
  

• struct device *dev;
  

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

I think this is modeled after the naming in drm_panel,

Right, The entire rework is based on how drm_panel framework is structured.

...

FWIW. However, seems reasonable to keep the device_node instead.

Yes, its visible that just device_node would be sufficient. This will save us from renaming drm_device as well.

...

...

...

• struct drm_device *drm;
  

• struct drm_encoder *encoder;
  

This breaks bridge->bridge chaining (if we ever get there). It seems pretty much unused anyway except for an EBUSY check. Can't you use bridge->dev for that?

Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach and leave it up to the caller to establish the proper chain.

Ok. I will use drm_device pointer directly instead of passing encoder pointer.

Hm, if you do this can you pls also update drm_panel accordingly? It shouldn't be a lot of fuzz and would make things around drm+dt more consistent.

...

...

...

  struct list_head head;

• struct list_head list;
  

These lists need better names. I know that the "head" is really awful, especially since it's actually not the head of the list but just an element.

I think we can just rip bridge_list out of mode_config if we're going to keep track of bridges elsewhere. So we can nuke "head" and keep "list". This also means that bridge->destroy() goes away, but that's probably Ok if everything converts to the standalone driver model where we have driver->remove()

Sean

Great! Thierry actually mentioned about this once, and we have the confirmation now.

...

...

...

  struct drm_mode_object base;

Aside: I've noticed all this trying to update the kerneldoc for struct drm_bridge, which just showed that this patch makes inconsistent changes. Trying to write kerneldoc is a really great way to come up with better interfaces imo.

Cheers, Daniel

I din't get this actually. You want me to create Doc../drm_bridge.txt or something similar?

If you want to document drm_bridge then I recomment to sprinkle proper kerneldoc over drm_bridge.c and pull it all into the drm DocBook template. That way all the drm documentation is in one place. I've done that for drm_crtc.h in an unrelated patch series (but based upon a branch with your patch here included) and there's struct drm_bridge* in there. Hence why I've noticed.

If you do kerneldoc/DocBook integration for drm_bridge it's probably best to also do it for drm_panel and use the opportunity to review/rework the interfaces a bit for consistency. E.g. move dt related stuff into drm_of.c and have that in a separate section in the docs. -Daniel

On Tue, Oct 28, 2014 at 1:28 PM, Ajay kumar ajaynumb@gmail.com wrote:

On Tue, Oct 28, 2014 at 3:31 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Tue, Oct 28, 2014 at 10:21 AM, Ajay kumar ajaynumb@gmail.com wrote:

...

Hi Daniel and Sean,

Thanks for the comments!

On Tue, Oct 28, 2014 at 1:28 AM, Sean Paul seanpaul@chromium.org wrote:

...

On Mon, Oct 27, 2014 at 3:01 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

So don't ask why but I accidentally ended up in a branch looking at this patch and didn't like it. So very quick&grumpy review.

First, please make the patch subject more descriptive: I'd expect a helper function scaffolding like the various crtc/probe/dp ... helpers we already have. You instead add code to untangle the probe ordering. Please say so.

Sure. I will reword it properly.

...

...

More comments below.

On Wed, Aug 27, 2014 at 07:59:37PM +0530, Ajay Kumar wrote:

...

A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add".

The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.

The encoder driver should also call "drm_bridge_attach" to pass on the drm_device, encoder pointers to the bridge object.

drm_bridge_attach inturn calls drm_bridge_init to register itself with the drm core. Later, it calls "bridge->funcs->attach" so that bridge can continue with other initializations.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

[snip]

...

@@ -660,8 +662,11 @@ struct drm_bridge_funcs {

• @driver_private: pointer to the bridge driver's internal context

*/ struct drm_bridge {

• struct drm_device *dev;
  

• struct device *dev;
  

Please don't rename the ->dev pointer into drm. Because _all_ the other drm structures still call it ->dev. Also, can't we use struct device_node here like we do in the of helpers Russell added? See 7e435aad38083

I think this is modeled after the naming in drm_panel,

Right, The entire rework is based on how drm_panel framework is structured.

...

FWIW. However, seems reasonable to keep the device_node instead.

Yes, its visible that just device_node would be sufficient. This will save us from renaming drm_device as well.

...

...

...

• struct drm_device *drm;
  

• struct drm_encoder *encoder;
  

This breaks bridge->bridge chaining (if we ever get there). It seems pretty much unused anyway except for an EBUSY check. Can't you use bridge->dev for that?

Yeah, I'd prefer to pass drm_device directly into drm_bridge_attach and leave it up to the caller to establish the proper chain.

Ok. I will use drm_device pointer directly instead of passing encoder pointer.

Hm, if you do this can you pls also update drm_panel accordingly? It shouldn't be a lot of fuzz and would make things around drm+dt more consistent.

Are you talking about using struct device_node instead of struct device? I guess you have misplaced the comment under the wrong section!

Yeah, that should have been one up ;-)

...

...

...

...

...

  struct list_head head;

• struct list_head list;
  

These lists need better names. I know that the "head" is really awful, especially since it's actually not the head of the list but just an element.

I think we can just rip bridge_list out of mode_config if we're going to keep track of bridges elsewhere. So we can nuke "head" and keep "list". This also means that bridge->destroy() goes away, but that's probably Ok if everything converts to the standalone driver model where we have driver->remove()

Sean

Great! Thierry actually mentioned about this once, and we have the confirmation now.

...

...

...

  struct drm_mode_object base;

Aside: I've noticed all this trying to update the kerneldoc for struct drm_bridge, which just showed that this patch makes inconsistent changes. Trying to write kerneldoc is a really great way to come up with better interfaces imo.

Cheers, Daniel

I din't get this actually. You want me to create Doc../drm_bridge.txt or something similar?

If you want to document drm_bridge then I recomment to sprinkle proper kerneldoc over drm_bridge.c and pull it all into the drm DocBook template. That way all the drm documentation is in one place. I've done that for drm_crtc.h in an unrelated patch series (but based upon a branch with your patch here included) and there's struct drm_bridge* in there. Hence why I've noticed.

Can you send a link for that? And, is there any problem if the doc comes later?

Since quite a while we've asked for the kerneldoc polish as part of each drm core patch series. It's just that drm_bridge/panel kinda have flown under the radar of the people usually asking for docs ;-) -Daniel

On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar ajaykumar.rs@samsung.com wrote:

A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add".

The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.

The encoder driver should also call "drm_bridge_attach" to pass on the drm_device, encoder pointers to the bridge object.

drm_bridge_attach inturn calls drm_bridge_init to register itself with the drm core. Later, it calls "bridge->funcs->attach" so that bridge can continue with other initializations.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/bridge/Kconfig | 15 ++++- drivers/gpu/drm/drm_bridge.c | 102 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc.c | 4 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 4 +- include/drm/drm_crtc.h | 12 +++- 6 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 drivers/gpu/drm/drm_bridge.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 4a55d59..bdbfb6f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o drm-$(CONFIG_PCI) += ati_pcigart.o +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o drm-$(CONFIG_DRM_PANEL) += drm_panel.o drm-$(CONFIG_OF) += drm_of.o

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 884923f..5a8e907 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -1,5 +1,16 @@ -config DRM_PTN3460

•   tristate "PTN3460 DP/LVDS bridge"
  

+config DRM_BRIDGE

I'm not convinced this adds any value, to be honest. In addition to whether or not it's useful, it seems like you'd need to stub the drm_bridge_* functions that are declared in drm_crtc.h or break them out into drm_bridge.h.

Sean

•   tristate
    depends on DRM
    select DRM_KMS_HELPER
  

•   help
  

•     Bridge registration and lookup framework.
  

+menu "bridge chips"

•   depends on DRM_BRIDGE
  

+config DRM_PTN3460

•   tristate "PTN3460 DP/LVDS bridge"
  

•   depends on DRM_BRIDGE
    ---help---
  

+endmenu diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c new file mode 100644 index 0000000..b2d43fd --- /dev/null +++ b/drivers/gpu/drm/drm_bridge.c @@ -0,0 +1,102 @@ +/*

• • Copyright (c) 2014 Samsung Electronics Co., Ltd
• • Permission is hereby granted, free of charge, to any person obtaining a
• • copy of this software and associated documentation files (the "Software"),
• • to deal in the Software without restriction, including without limitation
• • the rights to use, copy, modify, merge, publish, distribute, sub license,
• • and/or sell copies of the Software, and to permit persons to whom the
• • Software is furnished to do so, subject to the following conditions:
• • The above copyright notice and this permission notice (including the
• • next paragraph) shall be included in all copies or substantial portions
• • of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
• • THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
• • LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
• • FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
• • DEALINGS IN THE SOFTWARE.
• */

+#include <linux/err.h> +#include <linux/module.h>

+#include <drm/drm_crtc.h>

+#include "drm/drmP.h"

+static DEFINE_MUTEX(bridge_lock); +static LIST_HEAD(bridge_list);

+int drm_bridge_add(struct drm_bridge *bridge) +{

•   mutex_lock(&bridge_lock);
  

•   list_add_tail(&bridge->list, &bridge_list);
  

•   mutex_unlock(&bridge_lock);
  

•   return 0;
  

+} +EXPORT_SYMBOL(drm_bridge_add);

+void drm_bridge_remove(struct drm_bridge *bridge) +{

•   mutex_lock(&bridge_lock);
  

•   list_del_init(&bridge->list);
  

•   mutex_unlock(&bridge_lock);
  

+} +EXPORT_SYMBOL(drm_bridge_remove);

+int drm_bridge_attach(struct drm_bridge *bridge,

•                           struct drm_encoder *encoder)
  

+{

•   int ret;
  

•   if (!bridge || !encoder)
  

•           return -EINVAL;
  

•   if (bridge->encoder)
  

•           return -EBUSY;
  

•   encoder->bridge = bridge;
  

•   bridge->encoder = encoder;
  

•   bridge->drm = encoder->dev;
  

•   ret = drm_bridge_init(bridge->drm, bridge);
  

•   if (ret) {
  

•           DRM_ERROR("Failed to register bridge with drm\n");
  

•           return ret;
  

•   }
  

•   if (bridge->funcs->attach)
  

•           return bridge->funcs->attach(bridge);
  

•   return 0;
  

+} +EXPORT_SYMBOL(drm_bridge_attach);

+#ifdef CONFIG_OF +struct drm_bridge *of_drm_find_bridge(struct device_node *np) +{

•   struct drm_bridge *bridge;
  

•   mutex_lock(&bridge_lock);
  

•   list_for_each_entry(bridge, &bridge_list, list) {
  

•           if (bridge->dev->of_node == np) {
  

•                   mutex_unlock(&bridge_lock);
  

•                   return bridge;
  

•           }
  

•   }
  

•   mutex_unlock(&bridge_lock);
  

•   return NULL;
  

+} +EXPORT_SYMBOL(of_drm_find_bridge); +#endif

+MODULE_AUTHOR("Ajay Kumar ajaykumar.rs@samsung.com"); +MODULE_DESCRIPTION("DRM bridge infrastructure"); +MODULE_LICENSE("GPL and additional rights"); diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 25f5cfa..2fb22fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1039,7 +1039,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge) if (ret) goto out;

•   bridge->dev = dev;
  

•   bridge->drm = dev;
  
    list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
    dev->mode_config.num_bridge++;
  

@@ -1058,7 +1058,7 @@ EXPORT_SYMBOL(drm_bridge_init); */ void drm_bridge_cleanup(struct drm_bridge *bridge) {

•   struct drm_device *dev = bridge->dev;
  

•   struct drm_device *dev = bridge->drm;
  
    drm_modeset_lock_all(dev);
    drm_mode_object_put(dev, &bridge->base);
  

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index 0309539..bc9e5ff 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -33,7 +33,7 @@ static void hdmi_bridge_destroy(struct drm_bridge *bridge)

static void power_on(struct drm_bridge *bridge) {

•   struct drm_device *dev = bridge->dev;
  

•   struct drm_device *dev = bridge->drm;
    struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
    struct hdmi *hdmi = hdmi_bridge->hdmi;
    const struct hdmi_platform_config *config = hdmi->config;
  

@@ -67,7 +67,7 @@ static void power_on(struct drm_bridge *bridge)

static void power_off(struct drm_bridge *bridge) {

•   struct drm_device *dev = bridge->dev;
  

•   struct drm_device *dev = bridge->drm;
    struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
    struct hdmi *hdmi = hdmi_bridge->hdmi;
    const struct hdmi_platform_config *config = hdmi->config;
  

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index f48a436..f6f426f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -629,6 +629,7 @@ struct drm_plane {

/**

• drm_bridge_funcs - drm_bridge control functions

• • @attach: Called during drm_bridge_attach
  • @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge
  • @disable: Called right before encoder prepare, disables the bridge
  • @post_disable: Called right after encoder prepare, for lockstepped disable

@@ -638,6 +639,7 @@ struct drm_plane {

• @destroy: make object go away

*/ struct drm_bridge_funcs {

•   int (*attach)(struct drm_bridge *bridge);
    bool (*mode_fixup)(struct drm_bridge *bridge,
                       const struct drm_display_mode *mode,
                       struct drm_display_mode *adjusted_mode);
  

@@ -660,8 +662,11 @@ struct drm_bridge_funcs {

• @driver_private: pointer to the bridge driver's internal context

*/ struct drm_bridge {

•   struct drm_device *dev;
  

•   struct device *dev;
  

•   struct drm_device *drm;
  

•   struct drm_encoder *encoder;
    struct list_head head;
  

•   struct list_head list;
  
    struct drm_mode_object base;
  
  

@@ -906,6 +911,11 @@ extern void drm_connector_cleanup(struct drm_connector *connector); /* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev);

+extern int drm_bridge_add(struct drm_bridge *bridge); +extern void drm_bridge_remove(struct drm_bridge *bridge); +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); +extern int drm_bridge_attach(struct drm_bridge *bridge,

•                           struct drm_encoder *encoder);
  

extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); extern void drm_bridge_cleanup(struct drm_bridge *bridge);

-- 1.7.9.5

-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Tue, Oct 28, 2014 at 1:41 AM, Sean Paul seanpaul@chromium.org wrote:

On Wed, Aug 27, 2014 at 10:29 AM, Ajay Kumar ajaykumar.rs@samsung.com wrote:

...

A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow.

The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add".

The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization.

The encoder driver should also call "drm_bridge_attach" to pass on the drm_device, encoder pointers to the bridge object.

drm_bridge_attach inturn calls drm_bridge_init to register itself with the drm core. Later, it calls "bridge->funcs->attach" so that bridge can continue with other initializations.

Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com

drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/bridge/Kconfig | 15 ++++- drivers/gpu/drm/drm_bridge.c | 102 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc.c | 4 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 4 +- include/drm/drm_crtc.h | 12 +++- 6 files changed, 131 insertions(+), 7 deletions(-) create mode 100644 drivers/gpu/drm/drm_bridge.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 4a55d59..bdbfb6f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o drm-$(CONFIG_PCI) += ati_pcigart.o +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o drm-$(CONFIG_DRM_PANEL) += drm_panel.o drm-$(CONFIG_OF) += drm_of.o

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 884923f..5a8e907 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -1,5 +1,16 @@ -config DRM_PTN3460

•   tristate "PTN3460 DP/LVDS bridge"
  

+config DRM_BRIDGE

I'm not convinced this adds any value, to be honest.

Hmm, then how to compile drm_bridge.c?

In addition to whether or not it's useful, it seems like you'd need to stub the drm_bridge_* functions that are declared in drm_crtc.h or break them out into drm_bridge.h.

Well, Thierry mentioned about this long back. Again, we have the confirmation now!

Ajay [snip]