So I thought it would be easier to explain what I had in mind regarding Ajay's patchset (to add panel support) in patch form. Originally Thierry had some concerns with adding panel support in bridges ad-hoc. So this idea adds the support of chaining multiple bridges, one of which may be a panal adapter (maybe I should have called it drm_panel_adapter_bridge). There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly different approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the pre/enable/disable/post steps.
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251 +++++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251 +++++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ 3 files changed, 282 insertions(+) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index b4733e1..da6b301 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,3 +1,5 @@ ccflags-y := -Iinclude/drm
obj-$(CONFIG_DRM_PTN3460) += ptn3460.o +# TODO kconfig for drivers who use this to select it.. +obj-y += drm_bridge_util.o \ No newline at end of file diff --git a/drivers/gpu/drm/bridge/drm_bridge_util.c b/drivers/gpu/drm/bridge/drm_bridge_util.c new file mode 100644 index 0000000..89964b2 --- /dev/null +++ b/drivers/gpu/drm/bridge/drm_bridge_util.c @@ -0,0 +1,251 @@ +/* + * Copyright (C) 2014 Red Hat + * Author: Rob Clark robdclark@gmail.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see http://www.gnu.org/licenses/. + */ + +#include "drm_bridge_util.h" + +/* + * Composite Bridge: + */ + +/** + * drm_composite_bridge - helper bridge class to construct a graph + * with multiple actual bridges + */ +struct drm_composite_bridge { + struct drm_bridge base; + struct drm_bridge *b1, *b2; +}; +#define to_composite_bridge(x) container_of(x, struct drm_composite_bridge, base) + +static bool +composite_bridge_mode_fixup(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ + struct drm_composite_bridge *cbridge = to_composite_bridge(bridge); + struct drm_bridge *b1 = cbridge->b1; + struct drm_bridge *b2 = cbridge->b2; + + if (b1->funcs->mode_fixup) + if (!b1->funcs->mode_fixup(b1, mode, adjusted_mode)) + return false; + if (b2->funcs->mode_fixup) + if (!b2->funcs->mode_fixup(b2, mode, adjusted_mode)) + return false; + + return true; +} + +static void +composite_bridge_mode_set(struct drm_bridge *bridge, + struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ + struct drm_composite_bridge *cbridge = to_composite_bridge(bridge); + struct drm_bridge *b1 = cbridge->b1; + struct drm_bridge *b2 = cbridge->b2; + + if (b1->funcs->mode_set) + b1->funcs->mode_set(b1, mode, adjusted_mode); + if (b2->funcs->mode_set) + b2->funcs->mode_set(b2, mode, adjusted_mode); +} + +static void +composite_bridge_disable(struct drm_bridge *bridge) +{ + struct drm_composite_bridge *cbridge = to_composite_bridge(bridge); + struct drm_bridge *b1 = cbridge->b1; + struct drm_bridge *b2 = cbridge->b2; + + /* note: order inverted in disable path: */ + b2->funcs->disable(b2); + b1->funcs->disable(b1); +} + +static void +composite_bridge_post_disable(struct drm_bridge *bridge) +{ + struct drm_composite_bridge *cbridge = to_composite_bridge(bridge); + struct drm_bridge *b1 = cbridge->b1; + struct drm_bridge *b2 = cbridge->b2; + + /* note: order inverted in disable path: */ + b2->funcs->post_disable(b2); + b1->funcs->post_disable(b1); +} + +static void +composite_bridge_pre_enable(struct drm_bridge *bridge) +{ + struct drm_composite_bridge *cbridge = to_composite_bridge(bridge); + struct drm_bridge *b1 = cbridge->b1; + struct drm_bridge *b2 = cbridge->b2; + + b1->funcs->pre_enable(b1); + b2->funcs->pre_enable(b2); +} + +static void +composite_bridge_enable(struct drm_bridge *bridge) +{ + struct drm_composite_bridge *cbridge = to_composite_bridge(bridge); + struct drm_bridge *b1 = cbridge->b1; + struct drm_bridge *b2 = cbridge->b2; + + b1->funcs->enable(b1); + b2->funcs->enable(b2); +} + +static void +composite_bridge_destroy(struct drm_bridge *bridge) +{ + struct drm_composite_bridge *cbridge = to_composite_bridge(bridge); + struct drm_bridge *b1 = cbridge->b1; + struct drm_bridge *b2 = cbridge->b2; + + b1->funcs->destroy(b1); + b2->funcs->destroy(b2); + + kfree(cbridge); +} + +static const struct drm_bridge_funcs composite_bridge_funcs = { + .mode_fixup = composite_bridge_mode_fixup, + .disable = composite_bridge_disable, + .post_disable = composite_bridge_post_disable, + .mode_set = composite_bridge_mode_set, + .pre_enable = composite_bridge_pre_enable, + .enable = composite_bridge_enable, + .destroy = composite_bridge_destroy, +}; + +struct drm_bridge * drm_composite_bridge_init(struct drm_bridge *b1, + struct drm_bridge *b2) +{ + struct drm_bridge *bridge = NULL; + struct drm_composite_bridge *cbridge; + int ret; + + WARN_ON(b1->dev != b2->dev); + + cbridge = kzalloc(sizeof(*cbridge), GFP_KERNEL); + if (!cbridge) { + ret = -ENOMEM; + goto fail; + } + + bridge = &cbridge->base; + + cbridge->b1 = b1; + cbridge->b2 = b2; + + drm_bridge_init(b1->dev, bridge, &composite_bridge_funcs); + + return bridge; + +fail: + if (bridge) + composite_bridge_destroy(bridge); + + return ERR_PTR(ret); +} + +/* + * Panel Bridge: + */ + +/** + * drm_panel_bridge - helper to chain up a panel + */ +struct drm_panel_bridge { + struct drm_bridge base; + struct drm_panel *panel; +}; +#define to_panel_bridge(x) container_of(x, struct drm_panel_bridge, base) + +static void +panel_bridge_disable(struct drm_bridge *bridge) +{ + struct drm_panel_bridge *pbridge = to_panel_bridge(bridge); + drm_panel_disable(pbridge->panel); +} + +static void +panel_bridge_post_disable(struct drm_bridge *bridge) +{ +//TODO +// struct drm_panel_bridge *pbridge = to_panel_bridge(bridge); +// drm_panel_post_disable(pbridge->panel); +} + +static void +panel_bridge_pre_enable(struct drm_bridge *bridge) +{ +//TODO +// struct drm_panel_bridge *pbridge = to_panel_bridge(bridge); +// drm_panel_pre_enable(pbridge->panel); +} + +static void +panel_bridge_enable(struct drm_bridge *bridge) +{ + struct drm_panel_bridge *pbridge = to_panel_bridge(bridge); + drm_panel_enable(pbridge->panel); +} + +static void +panel_bridge_destroy(struct drm_bridge *bridge) +{ + struct drm_panel_bridge *pbridge = to_panel_bridge(bridge); + kfree(pbridge); +} + +static const struct drm_bridge_funcs panel_bridge_funcs = { + .disable = panel_bridge_disable, + .post_disable = panel_bridge_post_disable, + .pre_enable = panel_bridge_pre_enable, + .enable = panel_bridge_enable, + .destroy = panel_bridge_destroy, +}; + +struct drm_bridge * drm_panel_bridge_init(struct drm_panel *panel) +{ + struct drm_bridge *bridge = NULL; + struct drm_panel_bridge *pbridge; + int ret; + + pbridge = kzalloc(sizeof(*pbridge), GFP_KERNEL); + if (!pbridge) { + ret = -ENOMEM; + goto fail; + } + + bridge = &pbridge->base; + + pbridge->panel = panel; + + drm_bridge_init(panel->drm, bridge, &panel_bridge_funcs); + + return bridge; + +fail: + if (bridge) + panel_bridge_destroy(bridge); + + return ERR_PTR(ret); +} diff --git a/drivers/gpu/drm/bridge/drm_bridge_util.h b/drivers/gpu/drm/bridge/drm_bridge_util.h new file mode 100644 index 0000000..4f7a994 --- /dev/null +++ b/drivers/gpu/drm/bridge/drm_bridge_util.h @@ -0,0 +1,29 @@ +/* + * Copyright (C) 2014 Red Hat + * Author: Rob Clark robdclark@gmail.com + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see http://www.gnu.org/licenses/. + */ + +#ifndef DRM_BRIDGE_UTIL_H_ +#define DRM_BRIDGE_UTIL_H_ + +#include "drmP.h" +#include "drm_crtc.h" +#include "drm_panel.h" + +struct drm_bridge * drm_composite_bridge_init(struct drm_bridge *b1, + struct drm_bridge *b2); +struct drm_bridge * drm_panel_bridge_init(struct drm_panel *panel); + +#endif /* DRM_BRIDGE_UTIL_H_ */
Signed-off-by: Rob Clark robdclark@gmail.com --- drivers/gpu/drm/bridge/ptn3460.c | 39 +++++++++++++++++++++++++++++++++------ include/drm/bridge/ptn3460.h | 6 ++++-- 2 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index b171901..e3e6b46 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -26,6 +26,7 @@ #include "drm_crtc_helper.h"
#include "bridge/ptn3460.h" +#include "drm_bridge_util.h"
#define PTN3460_EDID_ADDR 0x0 #define PTN3460_EDID_EMULATION_ADDR 0x84 @@ -112,7 +113,6 @@ static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) static void ptn3460_pre_enable(struct drm_bridge *bridge) { struct ptn3460_bridge *ptn_bridge = bridge->driver_private; - int ret;
if (ptn_bridge->enabled) return; @@ -132,6 +132,15 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) * time specified in the chip's datasheet to make sure we're really up. */ msleep(90); +} + +static void ptn3460_enable(struct drm_bridge *bridge) +{ + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; + int ret; + + if (ptn_bridge->enabled) + return;
ret = ptn3460_select_edid(ptn_bridge); if (ret) @@ -140,10 +149,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) ptn_bridge->enabled = true; }
-static void ptn3460_enable(struct drm_bridge *bridge) -{ -} - static void ptn3460_disable(struct drm_bridge *bridge) { struct ptn3460_bridge *ptn_bridge = bridge->driver_private; @@ -265,7 +270,8 @@ struct drm_connector_funcs ptn3460_connector_funcs = { };
int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, - struct i2c_client *client, struct device_node *node) + struct i2c_client *client, struct device_node *node, + struct drm_panel *panel) { int ret; struct drm_bridge *bridge; @@ -324,6 +330,27 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, goto err; }
+ if (panel) { + struct drm_bridge *cbridge, *pbridge; + + pbridge = drm_panel_bridge_init(panel); + if (IS_ERR(pbridge)) { + ret = PTR_ERR(pbridge); + goto err; + } + + /* panel sequence is after ptn4360 bridge bridge in + * enable path, before in disable path: + */ + cbridge = drm_composite_bridge_init(bridge, pbridge); + if (IS_ERR(cbridge)) { + ret = PTR_ERR(cbridge); + goto err; + } + + bridge = cbridge; + } + bridge->driver_private = ptn_bridge; encoder->bridge = bridge;
diff --git a/include/drm/bridge/ptn3460.h b/include/drm/bridge/ptn3460.h index ff62344..9a557da 100644 --- a/include/drm/bridge/ptn3460.h +++ b/include/drm/bridge/ptn3460.h @@ -16,18 +16,20 @@
struct drm_device; struct drm_encoder; +struct drm_panel; struct i2c_client; struct device_node;
#if defined(CONFIG_DRM_PTN3460) || defined(CONFIG_DRM_PTN3460_MODULE)
int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, - struct i2c_client *client, struct device_node *node); + struct i2c_client *client, struct device_node *node, + struct drm_panel *panel); #else
static inline int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, struct i2c_client *client, - struct device_node *node) + struct device_node *node, struct drm_panel *panel) { return 0; }
On Tue, Apr 29, 2014 at 3:57 PM, Rob Clark robdclark@gmail.com wrote:
So I thought it would be easier to explain what I had in mind regarding Ajay's patchset (to add panel support) in patch form. Originally Thierry had some concerns with adding panel support in bridges ad-hoc. So this idea adds the support of chaining multiple bridges, one of which may be a panal adapter (maybe I should have called it drm_panel_adapter_bridge). There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly different approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the pre/enable/disable/post steps.
Arbitrarily chaining bridges seems like a more robust solution to me than the composite bridge.
For the panel case, I wonder if we could change drm_bridge_init to accept a panel, then we could just chain the panel calls off the various places the bridge hooks are invoked in the drm layer.
Feel free to ignore if this has already been explored on the other thread (which I haven't been following).
Sean
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251 +++++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Rob,
-----Original Message----- From: dri-devel [mailto:dri-devel-bounces@lists.freedesktop.org] On Behalf Of Rob Clark Sent: Wednesday, April 30, 2014 4:58 AM To: dri-devel@lists.freedesktop.org Cc: Ajay Kumar Subject: [RFC 0/2] drm/bridge: panel and chaining
So I thought it would be easier to explain what I had in mind regarding Ajay's patchset (to add panel support) in patch form. Originally Thierry had some concerns with adding panel support in bridges ad-hoc. So this idea adds the support of chaining multiple bridges, one of which may be a panal adapter (maybe I should have called it drm_panel_adapter_bridge). There are a few rough edges and TODOs, it isn't trying to be complete yet.
So the one question is about that hunk I had to move in ptn3460 from pre_enable() to enable(). If that really needs to come before the encoder and after the panel, then we should go for a slightly different approach instead: add a 'struct drm_bridge *next' ptr in 'struct drm_bridge'. Then each bridge implementation is responsible to call the next in the chain (if not null) at the appropriate points. That gives a bit more flexibility to bridges to have something both pre and post the downstream bridge/panel in each of the pre/enable/disable/post steps.
I think there would be better idea than chaining multiple bridges. For this, we had already discussed it[1], and I had posted relevant patch[2] for RFC.
My idea was that encoder driver has one or two bridge objects - for panel and lvds, and also image enhancer for crtc driver. The integrated drm_bridge structure including callback set for them. Originally, it was called "drm_hw_block" but would be reasonable to change to "drm_bridge" and existing drm_bridge relevant codes should be removed.
The important thing in my approach is that the integrated drm_bridge structure uses existing drm_panel infrastructure. The reason I did so was for that bridge device drivers such as lvds and image enhancer could comply with driver-model - now bridge drivers are non driver-model driver so encoder drivers should call some function directly to initialize the bridge driver. e.g. ptn3460_init function.
And also with this approach crtc drivers could control bridge device such as image enhancer - Image enhancer can be placed between crtc and encoder/connector, and actually Exynos SoC has such hardware.
For this, I'm planning to post next patch series, at least within this cycle.
[1] http://www.spinics.net/lists/dri-devel/msg55555.html [2]http://www.spinics.net/lists/dri-devel/msg55658.html
Thanks, Inki Dae
Rob Clark (2): drm/bridge: add composite and panel bridges drm/bridge/ptn3460: add panel support
drivers/gpu/drm/bridge/Makefile | 2 + drivers/gpu/drm/bridge/drm_bridge_util.c | 251 +++++++++++++++++++++++++++++++ drivers/gpu/drm/bridge/drm_bridge_util.h | 29 ++++ drivers/gpu/drm/bridge/ptn3460.c | 39 ++++- include/drm/bridge/ptn3460.h | 6 +- 5 files changed, 319 insertions(+), 8 deletions(-) create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.c create mode 100644 drivers/gpu/drm/bridge/drm_bridge_util.h
-- 1.9.0
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Inki Dae -
Rob Clark -
Sean Paul