[RFC V2 0/3] drm/bridge: panel and chaining

[RFC V2 0/3] drm/bridge: panel and chaining

[Ajay Kumar]

This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

I have just put up Rob's and Sean's idea of chaining up the bridges
in code, and have implemented basic panel controls as a chained bridge.
This works well with ptn3460 bridge chip on exynos5250-snow board.

Still need to make use of standard list calls and figure out proper way
of deleting the bridge chain. So, this is just a rough version.

Ajay Kumar (3):
  [RFC V2 1/3] drm: implement chaining of drm bridges
  [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
  [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining

 .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
 drivers/gpu/drm/bridge/Kconfig                     |   6 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
 drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
 drivers/gpu/drm/drm_crtc.c                         |  13 +-
 include/drm/bridge/bridge_panel.h                  |  37 ++++
 include/drm/drm_crtc.h                             |   2 +
 8 files changed, 360 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
 create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
 create mode 100644 include/drm/bridge/bridge_panel.h

-- 
1.8.1.2

[RFC V2 1/3] drm: implement chaining of drm bridges

[Ajay Kumar]

As of now, we can have only one bridge hanging off the encoder.
With this patch, we allow multiple bridges to hang onto the same encoder
with the use of a simple next_bridge ptr.

The drm core calls bridge->funcs for the first bridge which
is attached to it, and its upto the individual bridge drivers
to call bridge->funcs for the next bridge in the chain.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d8b7099..fe9905f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
  * 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)
+			struct drm_encoder *encoder,
+			const struct drm_bridge_funcs *funcs)
 {
+	struct drm_bridge *temp;
 	int ret;
 
 	drm_modeset_lock_all(dev);
@@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
 	bridge->dev = dev;
 	bridge->funcs = funcs;
 
+	if (encoder->bridge) {
+		temp = encoder->bridge;
+		while (temp->next_bridge)
+			temp = temp->next_bridge;
+
+		temp->next_bridge = bridge;
+	} else
+		encoder->bridge = bridge;
+
 	list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
 	dev->mode_config.num_bridge++;
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e55fccb..bb6ed88 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -619,6 +619,7 @@ struct drm_bridge_funcs {
 struct drm_bridge {
 	struct drm_device *dev;
 	struct list_head head;
+	struct drm_bridge *next_bridge;
 
 	struct drm_mode_object base;
 
@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
 extern void drm_connector_unplug_all(struct drm_device *dev);
 
 extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
+			   struct drm_encoder *encoder,
 			   const struct drm_bridge_funcs *funcs);
 extern void drm_bridge_cleanup(struct drm_bridge *bridge);
 
-- 
1.8.1.2

[RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay Kumar]

implement basic panel controls as a drm_bridge so that
the existing bridges can make use of it.

The driver assumes that it is the last entity in the bridge chain.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
 drivers/gpu/drm/bridge/Kconfig                     |   6 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
 include/drm/bridge/bridge_panel.h                  |  37 ++++
 5 files changed, 329 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
 create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
 create mode 100644 include/drm/bridge/bridge_panel.h

diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
new file mode 100644
index 0000000..0f916b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
@@ -0,0 +1,45 @@
+Simple panel interface for chaining along with bridges
+
+Required properties:
+  - compatible: "drm-bridge,panel"
+
+Optional properties:
+	-lcd-en-gpio:
+		eDP panel LCD poweron GPIO.
+			Indicates which GPIO needs to be powered up as output
+			to powerup/enable the switch to the LCD panel.
+	-led-en-gpio:
+		eDP panel LED enable GPIO.
+			Indicates which GPIO needs to be powered up as output
+			to enable the backlight.
+	-panel-pre-enable-delay:
+		delay value in ms required for panel_pre_enable process
+			Delay in ms needed for the eDP panel LCD unit to
+			powerup, and delay needed between panel_VCC and
+			video_enable.
+	-panel-enable-delay:
+		delay value in ms required for panel_enable process
+			Delay in ms needed for the eDP panel backlight/LED unit
+			to powerup, and delay needed between video_enable and
+			BL_EN.
+	-panel-disable-delay:
+		delay value in ms required for panel_disable process
+			Delay in ms needed for the eDP panel backlight/LED unit
+			powerdown, and delay needed between BL_DISABLE and
+			video_disable.
+	-panel-post-disable-delay:
+		delay value in ms required for panel_post_disable process
+			Delay in ms needed for the eDP panel LCD unit to
+			to powerdown, and delay between video_disable and
+			panel_VCC going down.
+
+Example:
+
+	bridge-panel {
+		compatible = "drm-bridge,panel";
+		led-en-gpio = <&gpx3 0 1>;
+		panel-pre-enable-delay = <40>;
+		panel-enable-delay = <20>;
+		panel-disable-delay = <20>;
+		panel-post-disable-delay = <30>;
+	};
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 884923f..654c5ea 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -3,3 +3,9 @@ config DRM_PTN3460
 	depends on DRM
 	select DRM_KMS_HELPER
 	---help---
+
+config DRM_BRIDGE_PANEL
+	tristate "dummy bridge panel"
+	depends on DRM
+	select DRM_KMS_HELPER
+	---help---
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index b4733e1..bf433cf 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,3 +1,4 @@
 ccflags-y := -Iinclude/drm
 
 obj-$(CONFIG_DRM_PTN3460) += ptn3460.o
+obj-$(CONFIG_DRM_BRIDGE_PANEL) += bridge_panel.o
diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
new file mode 100644
index 0000000..807118a
--- /dev/null
+++ b/drivers/gpu/drm/bridge/bridge_panel.c
@@ -0,0 +1,240 @@
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
+
+#include "drmP.h"
+
+#include "bridge/bridge_panel.h"
+
+enum panel_state {
+	PRE_ENABLE,
+	ENABLE,
+	DISABLE,
+	POST_DISABLE,
+};
+
+struct bridge_panel {
+	struct drm_connector	connector;
+	struct i2c_client	*client;
+	struct drm_encoder	*encoder;
+	struct drm_bridge	*bridge;
+	struct regulator	*backlight_fet;
+	struct regulator	*lcd_fet;
+	int			led_en_gpio;
+	int			lcd_en_gpio;
+	int			panel_pre_enable_delay;
+	int			panel_enable_delay;
+	int			panel_disable_delay;
+	int			panel_post_disable_delay;
+	enum panel_state	panel_state;
+};
+
+static void bridge_panel_pre_enable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (panel->panel_state != POST_DISABLE) {
+		DRM_ERROR("invoking bridge_panel_pre_enable " \
+					"from an improper state\n");
+		return;
+	}
+
+	if (!IS_ERR_OR_NULL(panel->lcd_fet))
+		if (regulator_enable(panel->lcd_fet))
+			DRM_ERROR("Failed to enable LCD fet\n");
+
+	if (gpio_is_valid(panel->lcd_en_gpio))
+		gpio_set_value(panel->lcd_en_gpio, 1);
+
+	msleep(panel->panel_pre_enable_delay);
+
+	panel->panel_state = PRE_ENABLE;
+}
+
+static void bridge_panel_enable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (panel->panel_state != PRE_ENABLE) {
+		DRM_ERROR("invoking bridge_panel_enable " \
+					"from an improper state\n");
+		return;
+	}
+
+	if (!IS_ERR_OR_NULL(panel->backlight_fet))
+		if (regulator_enable(panel->backlight_fet))
+				DRM_ERROR("Failed to enable LED fet\n");
+
+	msleep(panel->panel_enable_delay);
+
+	if (gpio_is_valid(panel->led_en_gpio))
+		gpio_set_value(panel->led_en_gpio, 1);
+
+	panel->panel_state = ENABLE;
+
+}
+
+static void bridge_panel_disable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (panel->panel_state != ENABLE) {
+		DRM_ERROR("invoking bridge_panel_disable " \
+					"from an improper state\n");
+		return;
+	}
+
+	if (gpio_is_valid(panel->led_en_gpio))
+		gpio_set_value(panel->led_en_gpio, 0);
+
+	if (!IS_ERR_OR_NULL(panel->backlight_fet))
+		regulator_disable(panel->backlight_fet);
+
+	msleep(panel->panel_disable_delay);
+
+	panel->panel_state = DISABLE;
+}
+
+static void bridge_panel_post_disable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (panel->panel_state != DISABLE) {
+		DRM_ERROR("invoking bridge_panel_post_disable " \
+					"from an improper state\n");
+		return;
+	}
+
+	if (gpio_is_valid(panel->lcd_en_gpio))
+		gpio_set_value(panel->lcd_en_gpio, 0);
+
+	if (!IS_ERR_OR_NULL(panel->lcd_fet))
+		regulator_disable(panel->lcd_fet);
+
+	msleep(panel->panel_post_disable_delay);
+
+	panel->panel_state = POST_DISABLE;
+}
+
+void bridge_panel_destroy(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	drm_bridge_cleanup(bridge);
+
+	if (gpio_is_valid(panel->lcd_en_gpio))
+		gpio_free(panel->lcd_en_gpio);
+	if (gpio_is_valid(panel->led_en_gpio))
+		gpio_free(panel->led_en_gpio);
+	/* Nothing else to free, we've got devm allocated memory */
+}
+
+struct drm_bridge_funcs bridge_panel_funcs = {
+	.pre_enable = bridge_panel_pre_enable,
+	.enable = bridge_panel_enable,
+	.disable = bridge_panel_disable,
+	.post_disable = bridge_panel_post_disable,
+	.destroy = bridge_panel_destroy,
+};
+
+int bridge_panel_init(struct drm_device *dev, struct drm_encoder *encoder,
+		struct i2c_client *client, struct device_node *node)
+{
+	int ret;
+	struct drm_bridge *bridge;
+	struct bridge_panel *panel;
+
+	bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL);
+	if (!bridge) {
+		DRM_ERROR("Failed to allocate drm bridge\n");
+		return -ENOMEM;
+	}
+
+	panel = devm_kzalloc(dev->dev, sizeof(*panel), GFP_KERNEL);
+	if (!panel) {
+		DRM_ERROR("Failed to allocate bridge panel\n");
+		return -ENOMEM;
+	}
+
+	panel->client = client;
+	panel->encoder = encoder;
+	panel->bridge = bridge;
+
+	panel->panel_state = POST_DISABLE;
+
+	panel->lcd_en_gpio = of_get_named_gpio(node, "lcd-en-gpio", 0);
+	panel->lcd_en_gpio = of_get_named_gpio(node, "led-en-gpio", 0);
+
+	of_property_read_u32(node, "panel-pre-enable-delay",
+					&panel->panel_pre_enable_delay);
+	of_property_read_u32(node, "panel-enable-delay",
+					&panel->panel_enable_delay);
+	of_property_read_u32(node, "panel-disable-delay",
+					&panel->panel_disable_delay);
+	of_property_read_u32(node, "panel-post-disable-delay",
+					&panel->panel_post_disable_delay);
+
+	panel->lcd_fet = devm_regulator_get(dev->dev, "lcd_vdd");
+	if (IS_ERR(panel->lcd_fet))
+		return PTR_ERR(panel->lcd_fet);
+
+	panel->backlight_fet = devm_regulator_get(dev->dev, "vcd_led");
+	if (IS_ERR(panel->backlight_fet))
+		return PTR_ERR(panel->backlight_fet);
+
+	if (gpio_is_valid(panel->lcd_en_gpio)) {
+		ret = devm_gpio_request_one(dev->dev, panel->lcd_en_gpio,
+					GPIOF_OUT_INIT_LOW, "lcd_en_gpio");
+		if (ret) {
+			DRM_ERROR("failed to get lcd-en gpio [%d]\n", ret);
+			return ret;
+		}
+	} else {
+		panel->lcd_en_gpio = -ENODEV;
+	}
+
+	if (gpio_is_valid(panel->led_en_gpio)) {
+		ret = devm_gpio_request_one(dev->dev, panel->led_en_gpio,
+					GPIOF_OUT_INIT_LOW, "led_en_gpio");
+		if (ret) {
+			DRM_ERROR("failed to get led-en gpio [%d]\n", ret);
+			return ret;
+		}
+	} else {
+		panel->led_en_gpio = -ENODEV;
+	}
+
+	ret = drm_bridge_init(dev, bridge, encoder, &bridge_panel_funcs);
+	if (ret) {
+		DRM_ERROR("Failed to initialize bridge with drm\n");
+		goto err;
+	}
+
+	bridge->driver_private = panel;
+
+	return 0;
+
+err:
+	if (gpio_is_valid(panel->lcd_en_gpio))
+		gpio_free(panel->lcd_en_gpio);
+	if (gpio_is_valid(panel->led_en_gpio))
+		gpio_free(panel->led_en_gpio);
+	return ret;
+}
+EXPORT_SYMBOL(bridge_panel_init);
diff --git a/include/drm/bridge/bridge_panel.h b/include/drm/bridge/bridge_panel.h
new file mode 100644
index 0000000..4a1b218
--- /dev/null
+++ b/include/drm/bridge/bridge_panel.h
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ */
+
+#ifndef _DRM_BRIDGE_PANEL_H_
+#define _DRM_BRIDGE_PANEL_H_
+
+struct drm_device;
+struct drm_encoder;
+struct i2c_client;
+struct device_node;
+
+#if defined(CONFIG_DRM_BRIDGE_PANEL)
+
+int bridge_panel_init(struct drm_device *dev, struct drm_encoder *encoder,
+			struct i2c_client *client, struct device_node *node);
+#else
+
+static inline int bridge_panel_init(struct drm_device *dev,
+		struct drm_encoder *encoder, struct i2c_client *client,
+		struct device_node *node)
+{
+	return 0;
+}
+
+#endif
+
+#endif
-- 
1.8.1.2

[RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining

[Ajay Kumar]

modify the driver to call the bridge->funcs of the next bridge
in the chain.
We assume that the bridge sitting next to ptn3460 is a bridge-panel.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
index b171901..969869a 100644
--- a/drivers/gpu/drm/bridge/ptn3460.c
+++ b/drivers/gpu/drm/bridge/ptn3460.c
@@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
 	}
 
+	if (bridge->next_bridge)
+		bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
 	/*
 	 * There's a bug in the PTN chip where it falsely asserts hotplug before
 	 * it is fully functional. We're forced to wait for the maximum start up
@@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 
 static void ptn3460_enable(struct drm_bridge *bridge)
 {
+	if (bridge->next_bridge)
+		bridge->next_bridge->funcs->enable(bridge->next_bridge);
 }
 
 static void ptn3460_disable(struct drm_bridge *bridge)
@@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 
 	ptn_bridge->enabled = false;
 
+	if (bridge->next_bridge) {
+		bridge->next_bridge->funcs->disable(bridge->next_bridge);
+		bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
+	}
+
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
 
@@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
 		return drm_add_edid_modes(connector, ptn_bridge->edid);
 
 	power_off = !ptn_bridge->enabled;
-	ptn3460_pre_enable(ptn_bridge->bridge);
+	if (power_off) {
+		ptn3460_pre_enable(ptn_bridge->bridge);
+		ptn3460_enable(ptn_bridge->bridge);
+	}
 
 	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid) {
@@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
 	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
 
 out:
-	if (power_off)
+	if (power_off) {
 		ptn3460_disable(ptn_bridge->bridge);
+		ptn3460_post_disable(ptn_bridge->bridge);
+	}
 
 	return num_modes;
 }
@@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 		goto err;
 	}
 
-	ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
+	ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
 	if (ret) {
 		DRM_ERROR("Failed to initialize bridge with drm\n");
 		goto err;
 	}
 
 	bridge->driver_private = ptn_bridge;
-	encoder->bridge = bridge;
 
 	ret = drm_connector_init(dev, &ptn_bridge->connector,
 			&ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
-- 
1.8.1.2

Re: [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining

[Sean Paul]

On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> modify the driver to call the bridge->funcs of the next bridge
> in the chain.
> We assume that the bridge sitting next to ptn3460 is a bridge-panel.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
> index b171901..969869a 100644
> --- a/drivers/gpu/drm/bridge/ptn3460.c
> +++ b/drivers/gpu/drm/bridge/ptn3460.c
> @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>         }
>
> +       if (bridge->next_bridge)
> +               bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
>         /*
>          * There's a bug in the PTN chip where it falsely asserts hotplug before
>          * it is fully functional. We're forced to wait for the maximum start up
> @@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>
>  static void ptn3460_enable(struct drm_bridge *bridge)
>  {
> +       if (bridge->next_bridge)
> +               bridge->next_bridge->funcs->enable(bridge->next_bridge);
>  }
>
>  static void ptn3460_disable(struct drm_bridge *bridge)
> @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>
>         ptn_bridge->enabled = false;
>
> +       if (bridge->next_bridge) {
> +               bridge->next_bridge->funcs->disable(bridge->next_bridge);
> +               bridge->next_bridge->funcs->post_disable(bridge->next_bridge);

Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?

> +       }
> +
>         if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>
> @@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>                 return drm_add_edid_modes(connector, ptn_bridge->edid);
>
>         power_off = !ptn_bridge->enabled;
> -       ptn3460_pre_enable(ptn_bridge->bridge);
> +       if (power_off) {
> +               ptn3460_pre_enable(ptn_bridge->bridge);
> +               ptn3460_enable(ptn_bridge->bridge);

In this case, I don't think we need to power on the entire bridge
chain since we're just reading the edid from the bridge chip itself.
So I think I'd prefer to break out the power_on/power_off code from
the bridge chain such that we can just power up the bridge chip, check
the edid and then turn it off.

In other bridges, where we're actually reading the edid from a
downstream source, this decision might be different.



> +       }
>
>         edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>         if (!edid) {
> @@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>         num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
>
>  out:
> -       if (power_off)
> +       if (power_off) {
>                 ptn3460_disable(ptn_bridge->bridge);
> +               ptn3460_post_disable(ptn_bridge->bridge);
> +       }
>
>         return num_modes;
>  }
> @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>                 goto err;
>         }
>
> -       ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
> +       ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
>         if (ret) {
>                 DRM_ERROR("Failed to initialize bridge with drm\n");
>                 goto err;
>         }
>
>         bridge->driver_private = ptn_bridge;
> -       encoder->bridge = bridge;
>
>         ret = drm_connector_init(dev, &ptn_bridge->connector,
>                         &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
> --
> 1.8.1.2
>

Re: [RFC V2 1/3] drm: implement chaining of drm bridges

[Sean Paul]

On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> As of now, we can have only one bridge hanging off the encoder.
> With this patch, we allow multiple bridges to hang onto the same encoder
> with the use of a simple next_bridge ptr.
>
> The drm core calls bridge->funcs for the first bridge which
> is attached to it, and its upto the individual bridge drivers
> to call bridge->funcs for the next bridge in the chain.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
>  include/drm/drm_crtc.h     |  2 ++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d8b7099..fe9905f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
>   * 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)
> +                       struct drm_encoder *encoder,
> +                       const struct drm_bridge_funcs *funcs)


IMO, we should let whoever is spawning the bridges chain them
together, instead of passing encoder in init().

Sean


>  {
> +       struct drm_bridge *temp;
>         int ret;
>
>         drm_modeset_lock_all(dev);
> @@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>         bridge->dev = dev;
>         bridge->funcs = funcs;
>
> +       if (encoder->bridge) {
> +               temp = encoder->bridge;
> +               while (temp->next_bridge)
> +                       temp = temp->next_bridge;
> +
> +               temp->next_bridge = bridge;
> +       } else
> +               encoder->bridge = bridge;
> +
>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>         dev->mode_config.num_bridge++;
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e55fccb..bb6ed88 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -619,6 +619,7 @@ struct drm_bridge_funcs {
>  struct drm_bridge {
>         struct drm_device *dev;
>         struct list_head head;
> +       struct drm_bridge *next_bridge;
>
>         struct drm_mode_object base;
>
> @@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>  extern void drm_connector_unplug_all(struct drm_device *dev);
>
>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
> +                          struct drm_encoder *encoder,
>                            const struct drm_bridge_funcs *funcs);
>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>
> --
> 1.8.1.2
>

Re: [RFC V2 1/3] drm: implement chaining of drm bridges

[Rob Clark]

On Tue, May 6, 2014 at 11:55 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>> As of now, we can have only one bridge hanging off the encoder.
>> With this patch, we allow multiple bridges to hang onto the same encoder
>> with the use of a simple next_bridge ptr.
>>
>> The drm core calls bridge->funcs for the first bridge which
>> is attached to it, and its upto the individual bridge drivers
>> to call bridge->funcs for the next bridge in the chain.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
>>  include/drm/drm_crtc.h     |  2 ++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index d8b7099..fe9905f 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
>>   * 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)
>> +                       struct drm_encoder *encoder,
>> +                       const struct drm_bridge_funcs *funcs)
>
>
> IMO, we should let whoever is spawning the bridges chain them
> together, instead of passing encoder in init().

that plus, might be a good time to start adding some static-inline
helper fxns for fxn ptr calls like we have for drm_panel..  not a big
deal, but I guess it would be a good time to do it now before we start
adding chained bridge calls in all the bridges.

but yeah, in general this is what I had in mind

BR,
-R

> Sean
>
>
>>  {
>> +       struct drm_bridge *temp;
>>         int ret;
>>
>>         drm_modeset_lock_all(dev);
>> @@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>         bridge->dev = dev;
>>         bridge->funcs = funcs;
>>
>> +       if (encoder->bridge) {
>> +               temp = encoder->bridge;
>> +               while (temp->next_bridge)
>> +                       temp = temp->next_bridge;
>> +
>> +               temp->next_bridge = bridge;
>> +       } else
>> +               encoder->bridge = bridge;
>> +
>>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>>         dev->mode_config.num_bridge++;
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index e55fccb..bb6ed88 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -619,6 +619,7 @@ struct drm_bridge_funcs {
>>  struct drm_bridge {
>>         struct drm_device *dev;
>>         struct list_head head;
>> +       struct drm_bridge *next_bridge;
>>
>>         struct drm_mode_object base;
>>
>> @@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>
>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>> +                          struct drm_encoder *encoder,
>>                            const struct drm_bridge_funcs *funcs);
>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>
>> --
>> 1.8.1.2
>>

Re: [RFC V2 1/3] drm: implement chaining of drm bridges

[Ajay kumar]

Sean,


On Tue, May 6, 2014 at 9:25 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>> As of now, we can have only one bridge hanging off the encoder.
>> With this patch, we allow multiple bridges to hang onto the same encoder
>> with the use of a simple next_bridge ptr.
>>
>> The drm core calls bridge->funcs for the first bridge which
>> is attached to it, and its upto the individual bridge drivers
>> to call bridge->funcs for the next bridge in the chain.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> ---
>>  drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
>>  include/drm/drm_crtc.h     |  2 ++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index d8b7099..fe9905f 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
>>   * 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)
>> +                       struct drm_encoder *encoder,
>> +                       const struct drm_bridge_funcs *funcs)
>
>
> IMO, we should let whoever is spawning the bridges chain them
> together, instead of passing encoder in init().
For this, we have to modify drm_bridge_init to return a bridge pointer and
then, as Rob is suggesting, we can have some helper functions to update
next_bridge ptr to form a bridge chain, and these functions can be called from
the corresponding encoder implementation.

Ajay

>>  {
>> +       struct drm_bridge *temp;
>>         int ret;
>>
>>         drm_modeset_lock_all(dev);
>> @@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>         bridge->dev = dev;
>>         bridge->funcs = funcs;
>>
>> +       if (encoder->bridge) {
>> +               temp = encoder->bridge;
>> +               while (temp->next_bridge)
>> +                       temp = temp->next_bridge;
>> +
>> +               temp->next_bridge = bridge;
>> +       } else
>> +               encoder->bridge = bridge;
>> +
>>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>>         dev->mode_config.num_bridge++;
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index e55fccb..bb6ed88 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -619,6 +619,7 @@ struct drm_bridge_funcs {
>>  struct drm_bridge {
>>         struct drm_device *dev;
>>         struct list_head head;
>> +       struct drm_bridge *next_bridge;
>>
>>         struct drm_mode_object base;
>>
>> @@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>
>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>> +                          struct drm_encoder *encoder,
>>                            const struct drm_bridge_funcs *funcs);
>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>
>> --
>> 1.8.1.2
>>

Re: [RFC V2 1/3] drm: implement chaining of drm bridges

[Ajay kumar]

Rob,


On Tue, May 6, 2014 at 9:42 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Tue, May 6, 2014 at 11:55 AM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>> As of now, we can have only one bridge hanging off the encoder.
>>> With this patch, we allow multiple bridges to hang onto the same encoder
>>> with the use of a simple next_bridge ptr.
>>>
>>> The drm core calls bridge->funcs for the first bridge which
>>> is attached to it, and its upto the individual bridge drivers
>>> to call bridge->funcs for the next bridge in the chain.
>>>
>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>> ---
>>>  drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
>>>  include/drm/drm_crtc.h     |  2 ++
>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>> index d8b7099..fe9905f 100644
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
>>>   * 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)
>>> +                       struct drm_encoder *encoder,
>>> +                       const struct drm_bridge_funcs *funcs)
>>
>>
>> IMO, we should let whoever is spawning the bridges chain them
>> together, instead of passing encoder in init().
>
> that plus, might be a good time to start adding some static-inline
> helper fxns for fxn ptr calls like we have for drm_panel..  not a big
> deal, but I guess it would be a good time to do it now before we start
> adding chained bridge calls in all the bridges.

Right, I will try to add a few:
-- to update next_bridge ptr and form a chain.
-- to call pre_enable, enable, disable and post_disable for the next
bridge in the list.

Ajay

>
> BR,
> -R
>
>> Sean
>>
>>
>>>  {
>>> +       struct drm_bridge *temp;
>>>         int ret;
>>>
>>>         drm_modeset_lock_all(dev);
>>> @@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>>         bridge->dev = dev;
>>>         bridge->funcs = funcs;
>>>
>>> +       if (encoder->bridge) {
>>> +               temp = encoder->bridge;
>>> +               while (temp->next_bridge)
>>> +                       temp = temp->next_bridge;
>>> +
>>> +               temp->next_bridge = bridge;
>>> +       } else
>>> +               encoder->bridge = bridge;
>>> +
>>>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>>>         dev->mode_config.num_bridge++;
>>>
>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index e55fccb..bb6ed88 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -619,6 +619,7 @@ struct drm_bridge_funcs {
>>>  struct drm_bridge {
>>>         struct drm_device *dev;
>>>         struct list_head head;
>>> +       struct drm_bridge *next_bridge;
>>>
>>>         struct drm_mode_object base;
>>>
>>> @@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>>
>>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>> +                          struct drm_encoder *encoder,
>>>                            const struct drm_bridge_funcs *funcs);
>>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>>
>>> --
>>> 1.8.1.2
>>>

Re: [RFC V2 1/3] drm: implement chaining of drm bridges

[Sean Paul]

On Tue, May 6, 2014 at 12:45 PM, Ajay kumar <ajaynumb@gmail.com> wrote:
> Sean,
>
>
> On Tue, May 6, 2014 at 9:25 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>> As of now, we can have only one bridge hanging off the encoder.
>>> With this patch, we allow multiple bridges to hang onto the same encoder
>>> with the use of a simple next_bridge ptr.
>>>
>>> The drm core calls bridge->funcs for the first bridge which
>>> is attached to it, and its upto the individual bridge drivers
>>> to call bridge->funcs for the next bridge in the chain.
>>>
>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>> ---
>>>  drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
>>>  include/drm/drm_crtc.h     |  2 ++
>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>> index d8b7099..fe9905f 100644
>>> --- a/drivers/gpu/drm/drm_crtc.c
>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>> @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
>>>   * 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)
>>> +                       struct drm_encoder *encoder,
>>> +                       const struct drm_bridge_funcs *funcs)
>>
>>
>> IMO, we should let whoever is spawning the bridges chain them
>> together, instead of passing encoder in init().
> For this, we have to modify drm_bridge_init to return a bridge pointer

Sorry, I don't follow this. You are passing in the bridge pointer, so
presumably you already have it.

Sean



> and
> then, as Rob is suggesting, we can have some helper functions to update
> next_bridge ptr to form a bridge chain, and these functions can be called from
> the corresponding encoder implementation.
>
> Ajay
>
>>>  {
>>> +       struct drm_bridge *temp;
>>>         int ret;
>>>
>>>         drm_modeset_lock_all(dev);
>>> @@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>>         bridge->dev = dev;
>>>         bridge->funcs = funcs;
>>>
>>> +       if (encoder->bridge) {
>>> +               temp = encoder->bridge;
>>> +               while (temp->next_bridge)
>>> +                       temp = temp->next_bridge;
>>> +
>>> +               temp->next_bridge = bridge;
>>> +       } else
>>> +               encoder->bridge = bridge;
>>> +
>>>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>>>         dev->mode_config.num_bridge++;
>>>
>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index e55fccb..bb6ed88 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -619,6 +619,7 @@ struct drm_bridge_funcs {
>>>  struct drm_bridge {
>>>         struct drm_device *dev;
>>>         struct list_head head;
>>> +       struct drm_bridge *next_bridge;
>>>
>>>         struct drm_mode_object base;
>>>
>>> @@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>>
>>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>> +                          struct drm_encoder *encoder,
>>>                            const struct drm_bridge_funcs *funcs);
>>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>>
>>> --
>>> 1.8.1.2
>>>

Re: [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining

[Ajay kumar]

Sean,


On Tue, May 6, 2014 at 9:24 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>> modify the driver to call the bridge->funcs of the next bridge
>> in the chain.
>> We assume that the bridge sitting next to ptn3460 is a bridge-panel.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> ---
>>  drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
>> index b171901..969869a 100644
>> --- a/drivers/gpu/drm/bridge/ptn3460.c
>> +++ b/drivers/gpu/drm/bridge/ptn3460.c
>> @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>         }
>>
>> +       if (bridge->next_bridge)
>> +               bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
>>         /*
>>          * There's a bug in the PTN chip where it falsely asserts hotplug before
>>          * it is fully functional. We're forced to wait for the maximum start up
>> @@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>
>>  static void ptn3460_enable(struct drm_bridge *bridge)
>>  {
>> +       if (bridge->next_bridge)
>> +               bridge->next_bridge->funcs->enable(bridge->next_bridge);
>>  }
>>
>>  static void ptn3460_disable(struct drm_bridge *bridge)
>> @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>>
>>         ptn_bridge->enabled = false;
>>
>> +       if (bridge->next_bridge) {
>> +               bridge->next_bridge->funcs->disable(bridge->next_bridge);
>> +               bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
>
> Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?
Umm, right. But no point in delaying ->post_disable of the panel(which
switches off power to LCD) since
backlight would be already down in ->disable call itself. So, I
thought of calling post_disable here itself.

>> +       }
>> +
>>         if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>
>> @@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>                 return drm_add_edid_modes(connector, ptn_bridge->edid);
>>
>>         power_off = !ptn_bridge->enabled;
>> -       ptn3460_pre_enable(ptn_bridge->bridge);
>> +       if (power_off) {
>> +               ptn3460_pre_enable(ptn_bridge->bridge);
>> +               ptn3460_enable(ptn_bridge->bridge);
>
> In this case, I don't think we need to power on the entire bridge
> chain since we're just reading the edid from the bridge chip itself.
> So I think I'd prefer to break out the power_on/power_off code from
> the bridge chain such that we can just power up the bridge chip, check
> the edid and then turn it off.
Those extra calls were added to make sure that regulator_enable/disable would
be in sync for the panel. Check the other patch [2/3].
Another way of doing this is to just check if the bridge is off  and
switch it on by
explicitly setting up the gpios here, instead of just calling
ptn3460_pre_enable.

Ajay
> In other bridges, where we're actually reading the edid from a
> downstream source, this decision might be different.
>
>
>
>> +       }
>>
>>         edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>>         if (!edid) {
>> @@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>         num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
>>
>>  out:
>> -       if (power_off)
>> +       if (power_off) {
>>                 ptn3460_disable(ptn_bridge->bridge);
>> +               ptn3460_post_disable(ptn_bridge->bridge);
>> +       }
>>
>>         return num_modes;
>>  }
>> @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>>                 goto err;
>>         }
>>
>> -       ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
>> +       ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
>>         if (ret) {
>>                 DRM_ERROR("Failed to initialize bridge with drm\n");
>>                 goto err;
>>         }
>>
>>         bridge->driver_private = ptn_bridge;
>> -       encoder->bridge = bridge;
>>
>>         ret = drm_connector_init(dev, &ptn_bridge->connector,
>>                         &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
>> --
>> 1.8.1.2
>>

Re: [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining

[Sean Paul]

On Tue, May 6, 2014 at 1:03 PM, Ajay kumar <ajaynumb@gmail.com> wrote:
> Sean,
>
>
> On Tue, May 6, 2014 at 9:24 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>> modify the driver to call the bridge->funcs of the next bridge
>>> in the chain.
>>> We assume that the bridge sitting next to ptn3460 is a bridge-panel.
>>>
>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>> ---
>>>  drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++----
>>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
>>> index b171901..969869a 100644
>>> --- a/drivers/gpu/drm/bridge/ptn3460.c
>>> +++ b/drivers/gpu/drm/bridge/ptn3460.c
>>> @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>>         }
>>>
>>> +       if (bridge->next_bridge)
>>> +               bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
>>>         /*
>>>          * There's a bug in the PTN chip where it falsely asserts hotplug before
>>>          * it is fully functional. We're forced to wait for the maximum start up
>>> @@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>>>
>>>  static void ptn3460_enable(struct drm_bridge *bridge)
>>>  {
>>> +       if (bridge->next_bridge)
>>> +               bridge->next_bridge->funcs->enable(bridge->next_bridge);
>>>  }
>>>
>>>  static void ptn3460_disable(struct drm_bridge *bridge)
>>> @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>>>
>>>         ptn_bridge->enabled = false;
>>>
>>> +       if (bridge->next_bridge) {
>>> +               bridge->next_bridge->funcs->disable(bridge->next_bridge);
>>> +               bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
>>
>> Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?
> Umm, right. But no point in delaying ->post_disable of the panel(which
> switches off power to LCD) since
> backlight would be already down in ->disable call itself. So, I
> thought of calling post_disable here itself.
>
>>> +       }
>>> +
>>>         if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>>>                 gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>>
>>> @@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>>                 return drm_add_edid_modes(connector, ptn_bridge->edid);
>>>
>>>         power_off = !ptn_bridge->enabled;
>>> -       ptn3460_pre_enable(ptn_bridge->bridge);
>>> +       if (power_off) {
>>> +               ptn3460_pre_enable(ptn_bridge->bridge);
>>> +               ptn3460_enable(ptn_bridge->bridge);
>>
>> In this case, I don't think we need to power on the entire bridge
>> chain since we're just reading the edid from the bridge chip itself.
>> So I think I'd prefer to break out the power_on/power_off code from
>> the bridge chain such that we can just power up the bridge chip, check
>> the edid and then turn it off.
> Those extra calls were added to make sure that regulator_enable/disable would
> be in sync for the panel. Check the other patch [2/3].

Sure, but we don't need the panel to read the edid. This bridge in
particular provides the edid itself, so in this case, we should leave
the panel off.

Sean


> Another way of doing this is to just check if the bridge is off  and
> switch it on by
> explicitly setting up the gpios here, instead of just calling
> ptn3460_pre_enable.
>
> Ajay
>> In other bridges, where we're actually reading the edid from a
>> downstream source, this decision might be different.
>>
>>
>>
>>> +       }
>>>
>>>         edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>>>         if (!edid) {
>>> @@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>>         num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
>>>
>>>  out:
>>> -       if (power_off)
>>> +       if (power_off) {
>>>                 ptn3460_disable(ptn_bridge->bridge);
>>> +               ptn3460_post_disable(ptn_bridge->bridge);
>>> +       }
>>>
>>>         return num_modes;
>>>  }
>>> @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>>>                 goto err;
>>>         }
>>>
>>> -       ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
>>> +       ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs);
>>>         if (ret) {
>>>                 DRM_ERROR("Failed to initialize bridge with drm\n");
>>>                 goto err;
>>>         }
>>>
>>>         bridge->driver_private = ptn_bridge;
>>> -       encoder->bridge = bridge;
>>>
>>>         ret = drm_connector_init(dev, &ptn_bridge->connector,
>>>                         &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
>>> --
>>> 1.8.1.2
>>>

Re: [RFC V2 1/3] drm: implement chaining of drm bridges

[Ajay kumar]

On Wed, May 7, 2014 at 1:33 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Tue, May 6, 2014 at 12:45 PM, Ajay kumar <ajaynumb@gmail.com> wrote:
>> Sean,
>>
>>
>> On Tue, May 6, 2014 at 9:25 PM, Sean Paul <seanpaul@chromium.org> wrote:
>>> On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>>> As of now, we can have only one bridge hanging off the encoder.
>>>> With this patch, we allow multiple bridges to hang onto the same encoder
>>>> with the use of a simple next_bridge ptr.
>>>>
>>>> The drm core calls bridge->funcs for the first bridge which
>>>> is attached to it, and its upto the individual bridge drivers
>>>> to call bridge->funcs for the next bridge in the chain.
>>>>
>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++-
>>>>  include/drm/drm_crtc.h     |  2 ++
>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index d8b7099..fe9905f 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
>>>>   * 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)
>>>> +                       struct drm_encoder *encoder,
>>>> +                       const struct drm_bridge_funcs *funcs)
>>>
>>>
>>> IMO, we should let whoever is spawning the bridges chain them
>>> together, instead of passing encoder in init().
>> For this, we have to modify drm_bridge_init to return a bridge pointer
>
> Sorry, I don't follow this. You are passing in the bridge pointer, so
> presumably you already have it.
Ahh, Sorry for that. It should be ptn3460_init, and not drm_bridge_init :)
I mean, ptn3460_init shall return a bridge pointer and exynos_dp(encoder)
should make use of it to prepare the bridge chain.

Ajay

> Sean
>
>
>
>> and
>> then, as Rob is suggesting, we can have some helper functions to update
>> next_bridge ptr to form a bridge chain, and these functions can be called from
>> the corresponding encoder implementation.
>>
>> Ajay
>>
>>>>  {
>>>> +       struct drm_bridge *temp;
>>>>         int ret;
>>>>
>>>>         drm_modeset_lock_all(dev);
>>>> @@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>>>         bridge->dev = dev;
>>>>         bridge->funcs = funcs;
>>>>
>>>> +       if (encoder->bridge) {
>>>> +               temp = encoder->bridge;
>>>> +               while (temp->next_bridge)
>>>> +                       temp = temp->next_bridge;
>>>> +
>>>> +               temp->next_bridge = bridge;
>>>> +       } else
>>>> +               encoder->bridge = bridge;
>>>> +
>>>>         list_add_tail(&bridge->head, &dev->mode_config.bridge_list);
>>>>         dev->mode_config.num_bridge++;
>>>>
>>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>>> index e55fccb..bb6ed88 100644
>>>> --- a/include/drm/drm_crtc.h
>>>> +++ b/include/drm/drm_crtc.h
>>>> @@ -619,6 +619,7 @@ struct drm_bridge_funcs {
>>>>  struct drm_bridge {
>>>>         struct drm_device *dev;
>>>>         struct list_head head;
>>>> +       struct drm_bridge *next_bridge;
>>>>
>>>>         struct drm_mode_object base;
>>>>
>>>> @@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector);
>>>>  extern void drm_connector_unplug_all(struct drm_device *dev);
>>>>
>>>>  extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
>>>> +                          struct drm_encoder *encoder,
>>>>                            const struct drm_bridge_funcs *funcs);
>>>>  extern void drm_bridge_cleanup(struct drm_bridge *bridge);
>>>>
>>>> --
>>>> 1.8.1.2
>>>>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/05/2014 09:52 PM, Ajay Kumar wrote:
> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
> 
> I have just put up Rob's and Sean's idea of chaining up the bridges
> in code, and have implemented basic panel controls as a chained bridge.
> This works well with ptn3460 bridge chip on exynos5250-snow board.
> 
> Still need to make use of standard list calls and figure out proper way
> of deleting the bridge chain. So, this is just a rough version.

As I understand this patchset tries to solve two things:
1. Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel
2. Add support to drm_bridge chaining, to allow software chains:
drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel

It is done using Russian doll schema, ops from the bridge calls the same
ops from the next bridge and the next bridge ops can do the same.

This schema means that all the bridges including the last one are seen
from the drm core point of view as a one big drm_bridge. Additionally in
this particular case, the first bridge (ptn3460) implements connector
so it is hard to guess what is the location of the 2nd bridge in video
stream chain, sometimes it is after the connector, sometimes before.
All this is quite confusing.

But if you look at the bridge from upstream video interface point of
view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
video input side acts as a panel. On the output side it expects a panel,
lvds panel in this case.

So why not implement ptn3460 bridge as drm_panel which internally uses
another drm_panel. With this approach everything fits much better.
You do not need those (pre|post)_(enable|disable) calls, you do not need
to implement connector in the bridge and you have a driver following
linux driver model. And no single bit changed in drm core.

I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
It was not accepted as Inki preferred drm_bridge but as I see the
problems with drm_bridges I have decide to attract attention to much
more cleaner solution.

[1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
[2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044

Regards
Andrzej


> 
> Ajay Kumar (3):
>   [RFC V2 1/3] drm: implement chaining of drm bridges
>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
> 
>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>  include/drm/drm_crtc.h                             |   2 +
>  8 files changed, 360 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>  create mode 100644 include/drm/bridge/bridge_panel.h
> 

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Inki Dae]

Hi Andrzej


On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
>
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
>
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
>
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.
>
> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
>
> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the
> problems with drm_bridges I have decide to attract attention to much

Yes, in above email threads, I disagreed to using drm_panel framework 
for bridge device, especially, to implement connector/encoder to crtc 
driver.

However, I thought that it'd be more generic way that lvds drivers use 
driver-model, and the use of drm_panel infrastructure would be suitable 
to doing this.

So my intention in turn,  was that LVDS driver uses integrated 
drm_bridge based on drm_panel infrastructure[1], and RFC patch[2] for 
it. This way is same as your proposal in the point that LVDS and Panel 
drivers use driver-model. The only different point is that LVDS driver 
has some ops specific to LVDS device, not using existing ops of 
drm_panel commonly: we may need to consider the characteristic of LVDS 
device.

[1]:http://www.spinics.net/lists/dri-devel/msg55555.html
[2]:http://www.spinics.net/lists/dri-devel/msg55658.html

Thanks,
Inki Dae

> more cleaner solution.
>
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>
> Regards
> Andrzej
>
>
>>
>> Ajay Kumar (3):
>>    [RFC V2 1/3] drm: implement chaining of drm bridges
>>    [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>    [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>   .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>   drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>   drivers/gpu/drm/bridge/Makefile                    |   1 +
>>   drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>   drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>   drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>   include/drm/bridge/bridge_panel.h                  |  37 ++++
>>   include/drm/drm_crtc.h                             |   2 +
>>   8 files changed, 360 insertions(+), 5 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>   create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>   create mode 100644 include/drm/bridge/bridge_panel.h
>>
>
> --
> 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
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Inki Dae]

Just re-sending with text mode. Sorry for this.


On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
> 
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
> 
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
> 
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
> 
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.
> 
> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
> 
> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the

Yes, in above email threads, I disagreed to using drm_panel framework
for bridge device, especially, to implement connector/encoder to crtc
driver.

However, I thought that it'd be more generic way that lvds drivers use
driver-model, and the use of drm_panel infrastructure would be suitable
to doing this.

So my intention in turn, was that LVDS driver uses integrated drm_bridge
based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
is same as your proposal in the point that LVDS and Panel drivers use
driver-model. The only different point is that LVDS driver has some ops
specific to LVDS device, not using existing ops of drm_panel commonly:
we may need to consider the characteristic of LVDS device.

[1]:http://www.spinics.net/lists/dri-devel/msg55555.html
[2]:http://www.spinics.net/lists/dri-devel/msg55658.html

Thanks,
Inki Dae

> problems with drm_bridges I have decide to attract attention to much
> more cleaner solution.
> 
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
> 
> Regards
> Andrzej
> 
> 
>>
>> Ajay Kumar (3):
>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>  include/drm/drm_crtc.h                             |   2 +
>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>
> 
> --
> 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
> 

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

Hi Andrej,

On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
>
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
>
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
>
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.
>
> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
This discussion should have ideally happened when Sean added bridge
into drm-core!
And, we do need (pre|post)_(enable|disable) calls for bridges, but the drm_panel
framework supports only enable/disable.

> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the
> problems with drm_bridges I have decide to attract attention to much
> more cleaner solution.
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>
> Regards
> Andrzej
>
>
>>
>> Ajay Kumar (3):
>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>  include/drm/drm_crtc.h                             |   2 +
>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

+Dave
+Thierry

On Thu, May 8, 2014 at 1:14 PM, Inki Dae <inki.dae@samsung.com> wrote:
>
> Just re-sending with text mode. Sorry for this.
>
>
> On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>>
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
>>
>> So why not implement ptn3460 bridge as drm_panel which internally uses
>> another drm_panel. With this approach everything fits much better.
>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>> to implement connector in the bridge and you have a driver following
>> linux driver model. And no single bit changed in drm core.
>>
>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>> It was not accepted as Inki preferred drm_bridge but as I see the
>
> Yes, in above email threads, I disagreed to using drm_panel framework
> for bridge device, especially, to implement connector/encoder to crtc
> driver.
>
> However, I thought that it'd be more generic way that lvds drivers use
> driver-model, and the use of drm_panel infrastructure would be suitable
> to doing this.
>
> So my intention in turn, was that LVDS driver uses integrated drm_bridge
> based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
> is same as your proposal in the point that LVDS and Panel drivers use
> driver-model. The only different point is that LVDS driver has some ops
> specific to LVDS device, not using existing ops of drm_panel commonly:
> we may need to consider the characteristic of LVDS device.
>
> [1]:http://www.spinics.net/lists/dri-devel/msg55555.html
> [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
>
> Thanks,
> Inki Dae
I am just consolidating the discussion happening here.
1) This patchset started from a discussion wherein I tried to combine
drm_panel with drm_bridge.
    https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg28943.html
2) Sean and Rob suggested to implement a chain of bridges and then
consider adding
    basic panel controls as a bridge.
3) Andrej's idea is to drop the existing bridge infrastructure and
implement ptn3460 using drm_panel,
    the same way he has implemented
http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559.
4) Inki's suggestion is to combine drm_bridge, drm_panel and
drm_enhance into a single
    drm_hw_block.

I am currently trying to implement (2):chaining of bridges, and I
think we have not yet
reached to a consensus. So adding few other people from drm community
to comment.

Regards,
Ajay

>> problems with drm_bridges I have decide to attract attention to much
>> more cleaner solution.
>>
>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>
>> Regards
>> Andrzej
>>
>>
>>>
>>> Ajay Kumar (3):
>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>
>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>  include/drm/drm_crtc.h                             |   2 +
>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>
>>
>> --
>> 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
>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Inki Dae]

On 2014년 05월 08일 19:52, Ajay kumar wrote:
> +Dave
> +Thierry
> 
> On Thu, May 8, 2014 at 1:14 PM, Inki Dae <inki.dae@samsung.com> wrote:
>>
>> Just re-sending with text mode. Sorry for this.
>>
>>
>> On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>
>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>
>>>> Still need to make use of standard list calls and figure out proper way
>>>> of deleting the bridge chain. So, this is just a rough version.
>>>
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>>>
>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>> another drm_panel. With this approach everything fits much better.
>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>> to implement connector in the bridge and you have a driver following
>>> linux driver model. And no single bit changed in drm core.
>>>
>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>
>> Yes, in above email threads, I disagreed to using drm_panel framework
>> for bridge device, especially, to implement connector/encoder to crtc
>> driver.
>>
>> However, I thought that it'd be more generic way that lvds drivers use
>> driver-model, and the use of drm_panel infrastructure would be suitable
>> to doing this.
>>
>> So my intention in turn, was that LVDS driver uses integrated drm_bridge
>> based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
>> is same as your proposal in the point that LVDS and Panel drivers use
>> driver-model. The only different point is that LVDS driver has some ops
>> specific to LVDS device, not using existing ops of drm_panel commonly:
>> we may need to consider the characteristic of LVDS device.
>>
>> [1]:http://www.spinics.net/lists/dri-devel/msg55555.html
>> [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
>>
>> Thanks,
>> Inki Dae
> I am just consolidating the discussion happening here.
> 1) This patchset started from a discussion wherein I tried to combine
> drm_panel with drm_bridge.
>     https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg28943.html
> 2) Sean and Rob suggested to implement a chain of bridges and then
> consider adding
>     basic panel controls as a bridge.
> 3) Andrej's idea is to drop the existing bridge infrastructure and
> implement ptn3460 using drm_panel,
>     the same way he has implemented
> http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559.
> 4) Inki's suggestion is to combine drm_bridge, drm_panel and
> drm_enhance into a single
>     drm_hw_block.
> 

And more thing, we would need to consider image enhancer device placed
between crtc and connector/encoder devices. And it'd better to rename
drm_hw_block to drm_bridge, and existing drm_bridge relevant codes
should be removed.

Thanks,
Inki Dae

> I am currently trying to implement (2):chaining of bridges, and I
> think we have not yet
> reached to a consensus. So adding few other people from drm community
> to comment.
> 
> Regards,
> Ajay
> 
>>> problems with drm_bridges I have decide to attract attention to much
>>> more cleaner solution.
>>>
>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>
>>> Regards
>>> Andrzej
>>>
>>>
>>>>
>>>> Ajay Kumar (3):
>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>
>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>
>>>
>>> --
>>> 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
>>>
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/08/2014 12:26 PM, Ajay kumar wrote:
> Hi Andrej,
>
> On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
>>
>> So why not implement ptn3460 bridge as drm_panel which internally uses
>> another drm_panel. With this approach everything fits much better.
>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>> to implement connector in the bridge and you have a driver following
>> linux driver model. And no single bit changed in drm core.
> This discussion should have ideally happened when Sean added bridge
> into drm-core!

Yes, I agree with this :)

> And, we do need (pre|post)_(enable|disable) calls for bridges, but the drm_panel
> framework supports only enable/disable.

For true bridges pre|post can be just implemented as a part of the call,
for example:

bridge_enable()
{
    /* pre-enable stuff */
    panel_enable();
    /* post-enable stuff */
}

And for your particular problem you have written:
> The LVDS datasheet says at least 200ms delay is needed from "Valid
> data" to "BL on".

I am not sure what exactly means 'valid data' in this case, if it is
after lcd_en gpio
why not just use schedule_delayed_work. If it should be called later I
guess it should
be possible to find a proper callback to drm_panel.

Regards
Andrzej

>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>> It was not accepted as Inki preferred drm_bridge but as I see the
>> problems with drm_bridges I have decide to attract attention to much
>> more cleaner solution.
>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>
>> Regards
>> Andrzej
>>
>>
>>> Ajay Kumar (3):
>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>
>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>  include/drm/drm_crtc.h                             |   2 +
>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

On Thu, May 8, 2014 at 6:29 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 05/08/2014 12:26 PM, Ajay kumar wrote:
>> Hi Andrej,
>>
>> On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>
>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>
>>>> Still need to make use of standard list calls and figure out proper way
>>>> of deleting the bridge chain. So, this is just a rough version.
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>>>
>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>> another drm_panel. With this approach everything fits much better.
>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>> to implement connector in the bridge and you have a driver following
>>> linux driver model. And no single bit changed in drm core.
>> This discussion should have ideally happened when Sean added bridge
>> into drm-core!
>
> Yes, I agree with this :)
>
>> And, we do need (pre|post)_(enable|disable) calls for bridges, but the drm_panel
>> framework supports only enable/disable.
>
> For true bridges pre|post can be just implemented as a part of the call,
> for example:
>
> bridge_enable()
> {
>     /* pre-enable stuff */
>     panel_enable();
>     /* post-enable stuff */
> }
>
It should ideally be like this:
1) panel enable - switch on the LCD unit.
2) bridge enable - switch on the bridge.
3) encoder->commit/dpms on - valid data starts coming out of DP/MIPI
4) switch on the backlight unit and enable BL_EN.

> And for your particular problem you have written:
>> The LVDS datasheet says at least 200ms delay is needed from "Valid
>> data" to "BL on".
>
> I am not sure what exactly means 'valid data' in this case, if it is
> after lcd_en gpio
> why not just use schedule_delayed_work. If it should be called later I
> guess it should
> be possible to find a proper callback to drm_panel.
Right. This was already discussed.
I suggested adding pre_enable/enable/disable and post_disable for drm_panel,
but Thierry was not ok with it.

Regards,
Ajay


>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>> problems with drm_bridges I have decide to attract attention to much
>>> more cleaner solution.
>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>
>>> Regards
>>> Andrzej
>>>
>>>
>>>> Ajay Kumar (3):
>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>
>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> I have just put up Rob's and Sean's idea of chaining up the bridges
>> in code, and have implemented basic panel controls as a chained bridge.
>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>
>> Still need to make use of standard list calls and figure out proper way
>> of deleting the bridge chain. So, this is just a rough version.
>
> As I understand this patchset tries to solve two things:
> 1. Implement panel as drm_bridge, to ease support for hardware chains:
> Crtc -> Encoder -> Bridge -> Panel
> 2. Add support to drm_bridge chaining, to allow software chains:
> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>
> It is done using Russian doll schema, ops from the bridge calls the same
> ops from the next bridge and the next bridge ops can do the same.
>
> This schema means that all the bridges including the last one are seen
> from the drm core point of view as a one big drm_bridge. Additionally in
> this particular case, the first bridge (ptn3460) implements connector
> so it is hard to guess what is the location of the 2nd bridge in video
> stream chain, sometimes it is after the connector, sometimes before.
> All this is quite confusing.
>
> But if you look at the bridge from upstream video interface point of
> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
> video input side acts as a panel. On the output side it expects a panel,
> lvds panel in this case.

tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
best name.. I wouldn't object to changing it.

But my thinking was to leave in drm_panel_funcs things that are just
needed by the connector (get_modes().. and maybe some day we need
detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
could (if needed) implement both interfaces.

That is basically the same as what you are proposing, but without
renaming bridge to panel ;-)

BR,
-R

> So why not implement ptn3460 bridge as drm_panel which internally uses
> another drm_panel. With this approach everything fits much better.
> You do not need those (pre|post)_(enable|disable) calls, you do not need
> to implement connector in the bridge and you have a driver following
> linux driver model. And no single bit changed in drm core.
>
> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
> It was not accepted as Inki preferred drm_bridge but as I see the
> problems with drm_bridges I have decide to attract attention to much
> more cleaner solution.
>
> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>
> Regards
> Andrzej
>
>
>>
>> Ajay Kumar (3):
>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>
>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>  include/drm/drm_crtc.h                             |   2 +
>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Thu, May 8, 2014 at 7:57 AM, Inki Dae <inki.dae@samsung.com> wrote:
> On 2014년 05월 08일 19:52, Ajay kumar wrote:
>> +Dave
>> +Thierry
>>
>> On Thu, May 8, 2014 at 1:14 PM, Inki Dae <inki.dae@samsung.com> wrote:
>>>
>>> Just re-sending with text mode. Sorry for this.
>>>
>>>
>>> On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>
>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>
>>>>> Still need to make use of standard list calls and figure out proper way
>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>
>>>> As I understand this patchset tries to solve two things:
>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>> Crtc -> Encoder -> Bridge -> Panel
>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>
>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>
>>>> This schema means that all the bridges including the last one are seen
>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>> this particular case, the first bridge (ptn3460) implements connector
>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>> All this is quite confusing.
>>>>
>>>> But if you look at the bridge from upstream video interface point of
>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>> video input side acts as a panel. On the output side it expects a panel,
>>>> lvds panel in this case.
>>>>
>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>> another drm_panel. With this approach everything fits much better.
>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>> to implement connector in the bridge and you have a driver following
>>>> linux driver model. And no single bit changed in drm core.
>>>>
>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>
>>> Yes, in above email threads, I disagreed to using drm_panel framework
>>> for bridge device, especially, to implement connector/encoder to crtc
>>> driver.
>>>
>>> However, I thought that it'd be more generic way that lvds drivers use
>>> driver-model, and the use of drm_panel infrastructure would be suitable
>>> to doing this.
>>>
>>> So my intention in turn, was that LVDS driver uses integrated drm_bridge
>>> based on drm_panel infrastructure[1], and RFC patch[2] for it. This way
>>> is same as your proposal in the point that LVDS and Panel drivers use
>>> driver-model. The only different point is that LVDS driver has some ops
>>> specific to LVDS device, not using existing ops of drm_panel commonly:
>>> we may need to consider the characteristic of LVDS device.
>>>
>>> [1]:http://www.spinics.net/lists/dri-devel/msg55555.html
>>> [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
>>>
>>> Thanks,
>>> Inki Dae
>> I am just consolidating the discussion happening here.
>> 1) This patchset started from a discussion wherein I tried to combine
>> drm_panel with drm_bridge.
>>     https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg28943.html
>> 2) Sean and Rob suggested to implement a chain of bridges and then
>> consider adding
>>     basic panel controls as a bridge.
>> 3) Andrej's idea is to drop the existing bridge infrastructure and
>> implement ptn3460 using drm_panel,
>>     the same way he has implemented
>> http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559.
>> 4) Inki's suggestion is to combine drm_bridge, drm_panel and
>> drm_enhance into a single
>>     drm_hw_block.
>>
>
> And more thing, we would need to consider image enhancer device placed
> between crtc and connector/encoder devices. And it'd better to rename
> drm_hw_block to drm_bridge, and existing drm_bridge relevant codes
> should be removed.

I don't object to changing the name to hw_block or something else.
Although to avoid introducing too much confusion in this discussion,
for now I am calling it bridge/drm_bridge_funcs/etc for now ;-)

BR,
-R

> Thanks,
> Inki Dae
>
>> I am currently trying to implement (2):chaining of bridges, and I
>> think we have not yet
>> reached to a consensus. So adding few other people from drm community
>> to comment.
>>
>> Regards,
>> Ajay
>>
>>>> problems with drm_bridges I have decide to attract attention to much
>>>> more cleaner solution.
>>>>
>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>
>>>>>
>>>>> Ajay Kumar (3):
>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>
>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>
>>>>
>>>> --
>>>> 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
>>>>
>>>
>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/08/2014 08:24 PM, Rob Clark wrote:
> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>
>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>> in code, and have implemented basic panel controls as a chained bridge.
>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>
>>> Still need to make use of standard list calls and figure out proper way
>>> of deleting the bridge chain. So, this is just a rough version.
>>
>> As I understand this patchset tries to solve two things:
>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>> Crtc -> Encoder -> Bridge -> Panel
>> 2. Add support to drm_bridge chaining, to allow software chains:
>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>
>> It is done using Russian doll schema, ops from the bridge calls the same
>> ops from the next bridge and the next bridge ops can do the same.
>>
>> This schema means that all the bridges including the last one are seen
>> from the drm core point of view as a one big drm_bridge. Additionally in
>> this particular case, the first bridge (ptn3460) implements connector
>> so it is hard to guess what is the location of the 2nd bridge in video
>> stream chain, sometimes it is after the connector, sometimes before.
>> All this is quite confusing.
>>
>> But if you look at the bridge from upstream video interface point of
>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>> video input side acts as a panel. On the output side it expects a panel,
>> lvds panel in this case.
> 
> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
> best name.. I wouldn't object to changing it.
> 
> But my thinking was to leave in drm_panel_funcs things that are just
> needed by the connector (get_modes().. and maybe some day we need
> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
> could (if needed) implement both interfaces.
> 
> That is basically the same as what you are proposing, but without
> renaming bridge to panel ;-)

Good to hear that. However there are points which are not clear for me.
But first lets clarify names, I will use panel and bridge words
to describe the hardware, and drm_panel, drm_bridge to describe the
software interfaces.

What bothers me:
1. You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
must implement connector internally because of this limitation. I guess
it is quite typical bridge. This problem does not exists in case
of using drm_panel for ptn3460.

2. drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below:
drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]

ptn3460 implements drm_bridge and drm_connector so it suggests its
drm_bridge should be the last one, so there should be no place to add
lvds panel implemented as a drm_bridge after it, as it is done in this
patchset.

Additionally it clearly shows that there should be two categories of
drm_bridges - non-terminal and terminal.

3. drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite
fragile - the whole drm will be down due to error in some of its components.
For this reason I prefer drm_panel as it is not real drm component
it can be attached/detached to/from drm_connector anytime. I am not
really sure but drm_bridge does not allow for that.

Real life example to show importance of it: I have a phone with MIPI-DSI
panel and HDMI. Due to initialization issues HDMI bridge driver
sometimes fails during probe and the drmdev do not start. Of course this
is development stage so I have serial console I can diagnose the
problem, disable HDMI, fix the problem, etc...
But what happens in case of end-user. He will see black screen - bricked
phone. In case the bridge will be implemented using drm_panel
he will have working phone with broken HDMI, much better.

4. And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device:
bridge, panel, whatever. In my approach I have an interface
to describe only one video input port of the device. And drm_panel is
in fact misleading name, drm_sink may be better. So real panel
would implement drm_sink interface. Bridge would implement drm_sink
interface and it will request other drm_sink interface, to interact with
the panel which is after it.
This approach seems to me more flexible. Beside things I have described
above it will allow to implement also more complicated devices, dsi
hubs, video mixers, etc.


Regards
Andrzej

> 
> BR,
> -R
> 
>> So why not implement ptn3460 bridge as drm_panel which internally uses
>> another drm_panel. With this approach everything fits much better.
>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>> to implement connector in the bridge and you have a driver following
>> linux driver model. And no single bit changed in drm core.
>>
>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>> It was not accepted as Inki preferred drm_bridge but as I see the
>> problems with drm_bridges I have decide to attract attention to much
>> more cleaner solution.
>>
>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>
>> Regards
>> Andrzej
>>
>>
>>>
>>> Ajay Kumar (3):
>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>
>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>  include/drm/drm_crtc.h                             |   2 +
>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>
>>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 05/08/2014 08:24 PM, Rob Clark wrote:
>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>
>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>
>>>> Still need to make use of standard list calls and figure out proper way
>>>> of deleting the bridge chain. So, this is just a rough version.
>>>
>>> As I understand this patchset tries to solve two things:
>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>> Crtc -> Encoder -> Bridge -> Panel
>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>
>>> It is done using Russian doll schema, ops from the bridge calls the same
>>> ops from the next bridge and the next bridge ops can do the same.
>>>
>>> This schema means that all the bridges including the last one are seen
>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>> this particular case, the first bridge (ptn3460) implements connector
>>> so it is hard to guess what is the location of the 2nd bridge in video
>>> stream chain, sometimes it is after the connector, sometimes before.
>>> All this is quite confusing.
>>>
>>> But if you look at the bridge from upstream video interface point of
>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>> video input side acts as a panel. On the output side it expects a panel,
>>> lvds panel in this case.
>>
>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>> best name.. I wouldn't object to changing it.
>>
>> But my thinking was to leave in drm_panel_funcs things that are just
>> needed by the connector (get_modes().. and maybe some day we need
>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>> could (if needed) implement both interfaces.
>>
>> That is basically the same as what you are proposing, but without
>> renaming bridge to panel ;-)
>
> Good to hear that. However there are points which are not clear for me.
> But first lets clarify names, I will use panel and bridge words
> to describe the hardware, and drm_panel, drm_bridge to describe the
> software interfaces.
>
> What bothers me:
> 1. You want to leave connector related callbacks in drm_panel and
> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
> must implement connector internally because of this limitation. I guess
> it is quite typical bridge. This problem does not exists in case
> of using drm_panel for ptn3460.
>
> 2. drm_bridge is attached to the encoder, this and the callback order
> suggests the video data flow should be as below:
> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>
> ptn3460 implements drm_bridge and drm_connector so it suggests its
> drm_bridge should be the last one, so there should be no place to add
> lvds panel implemented as a drm_bridge after it, as it is done in this
> patchset.
>
> Additionally it clearly shows that there should be two categories of
> drm_bridges - non-terminal and terminal.
>
> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
> its components are up. It simplifies synchronization but is quite
> fragile - the whole drm will be down due to error in some of its components.
> For this reason I prefer drm_panel as it is not real drm component
> it can be attached/detached to/from drm_connector anytime. I am not
> really sure but drm_bridge does not allow for that.

So I do think we need to stick to this all-or-nothing approach for
anything that is visible to userspace
(drm_{plane,crtc,encoder,connector}).  We don't currently have a way
to "hotplug" those so I don't see a real smooth upgrade path to add
that in a backwards compatible way that won't cause problems with old
userspace.

But, that said, we have more flexibility with things not visible to
userspace (drm_{panel,bridge}).  I'm not sure how much we want to
allow things to be completely dynamic (we already have some hard
enough locking fun).  But proposals/rfcs/etc welcome.

I guess I'm not completely familiar w/ ptn3460, but the fact that it
needs to implement drm_connector makes me a bit suspicious.  Seems
like a symptom of missing things in drm_panel_funcs.  It would be
better to always create the connector statically, and just have
_detect() -> disconnected if panel==NULL.

> Real life example to show importance of it: I have a phone with MIPI-DSI
> panel and HDMI. Due to initialization issues HDMI bridge driver
> sometimes fails during probe and the drmdev do not start. Of course this
> is development stage so I have serial console I can diagnose the
> problem, disable HDMI, fix the problem, etc...
> But what happens in case of end-user. He will see black screen - bricked
> phone. In case the bridge will be implemented using drm_panel
> he will have working phone with broken HDMI, much better.

well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)

I suppose if bridge/panel where loaded dynamically (or at least after
drm device and drm_{connector,encoder,etc} are created, it would help
a bit here.  I'd kinda hope that isn't the only benefit/reason to make
things more dynamic.  Especially if we allow bridges/panels to be
unloaded.. (just loading them dynamically doesn't seem as scary from
locking perspective)

> 4. And the last thing, it is more about the concept/design. drm_bridge,
> drm_hw_block suggests that those interfaces describes the whole device:
> bridge, panel, whatever.

hmm, I don't think this is the case.  I can easily see things like:

  struct foo_panel {
    struct drm_panel base;
    struct drm_bridge bridge;
    ...
  }

where a panel implementation implements both panel and bridge.  In
fact that is kinda what I was encouraging.

BR,
-R

> In my approach I have an interface
> to describe only one video input port of the device. And drm_panel is
> in fact misleading name, drm_sink may be better. So real panel
> would implement drm_sink interface. Bridge would implement drm_sink
> interface and it will request other drm_sink interface, to interact with
> the panel which is after it.
> This approach seems to me more flexible. Beside things I have described
> above it will allow to implement also more complicated devices, dsi
> hubs, video mixers, etc.
>
>
> Regards
> Andrzej
>
>>
>> BR,
>> -R
>>
>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>> another drm_panel. With this approach everything fits much better.
>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>> to implement connector in the bridge and you have a driver following
>>> linux driver model. And no single bit changed in drm core.
>>>
>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>> problems with drm_bridges I have decide to attract attention to much
>>> more cleaner solution.
>>>
>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>
>>> Regards
>>> Andrzej
>>>
>>>
>>>>
>>>> Ajay Kumar (3):
>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>
>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>
>>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>
>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>
>>>>> Still need to make use of standard list calls and figure out proper way
>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>
>>>> As I understand this patchset tries to solve two things:
>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>> Crtc -> Encoder -> Bridge -> Panel
>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>
>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>
>>>> This schema means that all the bridges including the last one are seen
>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>> this particular case, the first bridge (ptn3460) implements connector
>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>> All this is quite confusing.
>>>>
>>>> But if you look at the bridge from upstream video interface point of
>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>> video input side acts as a panel. On the output side it expects a panel,
>>>> lvds panel in this case.
>>>
>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>> best name.. I wouldn't object to changing it.
>>>
>>> But my thinking was to leave in drm_panel_funcs things that are just
>>> needed by the connector (get_modes().. and maybe some day we need
>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>> could (if needed) implement both interfaces.
>>>
>>> That is basically the same as what you are proposing, but without
>>> renaming bridge to panel ;-)
>>
>> Good to hear that. However there are points which are not clear for me.
>> But first lets clarify names, I will use panel and bridge words
>> to describe the hardware, and drm_panel, drm_bridge to describe the
>> software interfaces.
>>
>> What bothers me:
>> 1. You want to leave connector related callbacks in drm_panel and
>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>> must implement connector internally because of this limitation. I guess
>> it is quite typical bridge. This problem does not exists in case
>> of using drm_panel for ptn3460.
>>
>> 2. drm_bridge is attached to the encoder, this and the callback order
>> suggests the video data flow should be as below:
>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>
>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>> drm_bridge should be the last one, so there should be no place to add
>> lvds panel implemented as a drm_bridge after it, as it is done in this
>> patchset.
>>
>> Additionally it clearly shows that there should be two categories of
>> drm_bridges - non-terminal and terminal.
>>
>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>> its components are up. It simplifies synchronization but is quite
>> fragile - the whole drm will be down due to error in some of its components.
>> For this reason I prefer drm_panel as it is not real drm component
>> it can be attached/detached to/from drm_connector anytime. I am not
>> really sure but drm_bridge does not allow for that.
>
> So I do think we need to stick to this all-or-nothing approach for
> anything that is visible to userspace
> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
> to "hotplug" those so I don't see a real smooth upgrade path to add
> that in a backwards compatible way that won't cause problems with old
> userspace.
>
> But, that said, we have more flexibility with things not visible to
> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
> allow things to be completely dynamic (we already have some hard
> enough locking fun).  But proposals/rfcs/etc welcome.
>
> I guess I'm not completely familiar w/ ptn3460, but the fact that it
> needs to implement drm_connector makes me a bit suspicious.  Seems
> like a symptom of missing things in drm_panel_funcs.  It would be
> better to always create the connector statically, and just have
> _detect() -> disconnected if panel==NULL.
This is something which only Sean can answer!
I guess he implemented ptn3460 as connector thinking that bridge would
be the last
entity in the video pipeline. If that's a real problem, we can still
move out the
connector part.

Regards,
Ajay
>> Real life example to show importance of it: I have a phone with MIPI-DSI
>> panel and HDMI. Due to initialization issues HDMI bridge driver
>> sometimes fails during probe and the drmdev do not start. Of course this
>> is development stage so I have serial console I can diagnose the
>> problem, disable HDMI, fix the problem, etc...
>> But what happens in case of end-user. He will see black screen - bricked
>> phone. In case the bridge will be implemented using drm_panel
>> he will have working phone with broken HDMI, much better.
>
> well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
>
> I suppose if bridge/panel where loaded dynamically (or at least after
> drm device and drm_{connector,encoder,etc} are created, it would help
> a bit here.  I'd kinda hope that isn't the only benefit/reason to make
> things more dynamic.  Especially if we allow bridges/panels to be
> unloaded.. (just loading them dynamically doesn't seem as scary from
> locking perspective)
>
>> 4. And the last thing, it is more about the concept/design. drm_bridge,
>> drm_hw_block suggests that those interfaces describes the whole device:
>> bridge, panel, whatever.
>
> hmm, I don't think this is the case.  I can easily see things like:
>
>   struct foo_panel {
>     struct drm_panel base;
>     struct drm_bridge bridge;
>     ...
>   }
>
> where a panel implementation implements both panel and bridge.  In
> fact that is kinda what I was encouraging.
>
> BR,
> -R
>
>> In my approach I have an interface
>> to describe only one video input port of the device. And drm_panel is
>> in fact misleading name, drm_sink may be better. So real panel
>> would implement drm_sink interface. Bridge would implement drm_sink
>> interface and it will request other drm_sink interface, to interact with
>> the panel which is after it.
>> This approach seems to me more flexible. Beside things I have described
>> above it will allow to implement also more complicated devices, dsi
>> hubs, video mixers, etc.
>>
>>
>> Regards
>> Andrzej
>>
>>>
>>> BR,
>>> -R
>>>
>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>> another drm_panel. With this approach everything fits much better.
>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>> to implement connector in the bridge and you have a driver following
>>>> linux driver model. And no single bit changed in drm core.
>>>>
>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>> problems with drm_bridges I have decide to attract attention to much
>>>> more cleaner solution.
>>>>
>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>
>>>>>
>>>>> Ajay Kumar (3):
>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>
>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>
>>>>
>>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/09/2014 05:05 PM, Ajay kumar wrote:
> On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark@gmail.com> wrote:
>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>>
>>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>>
>>>>>> Still need to make use of standard list calls and figure out proper way
>>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>> As I understand this patchset tries to solve two things:
>>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>>> Crtc -> Encoder -> Bridge -> Panel
>>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>>
>>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>>
>>>>> This schema means that all the bridges including the last one are seen
>>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>>> this particular case, the first bridge (ptn3460) implements connector
>>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>>> All this is quite confusing.
>>>>>
>>>>> But if you look at the bridge from upstream video interface point of
>>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>>> video input side acts as a panel. On the output side it expects a panel,
>>>>> lvds panel in this case.
>>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>>> best name.. I wouldn't object to changing it.
>>>>
>>>> But my thinking was to leave in drm_panel_funcs things that are just
>>>> needed by the connector (get_modes().. and maybe some day we need
>>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>>> could (if needed) implement both interfaces.
>>>>
>>>> That is basically the same as what you are proposing, but without
>>>> renaming bridge to panel ;-)
>>> Good to hear that. However there are points which are not clear for me.
>>> But first lets clarify names, I will use panel and bridge words
>>> to describe the hardware, and drm_panel, drm_bridge to describe the
>>> software interfaces.
>>>
>>> What bothers me:
>>> 1. You want to leave connector related callbacks in drm_panel and
>>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>>> must implement connector internally because of this limitation. I guess
>>> it is quite typical bridge. This problem does not exists in case
>>> of using drm_panel for ptn3460.
>>>
>>> 2. drm_bridge is attached to the encoder, this and the callback order
>>> suggests the video data flow should be as below:
>>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>>
>>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>>> drm_bridge should be the last one, so there should be no place to add
>>> lvds panel implemented as a drm_bridge after it, as it is done in this
>>> patchset.
>>>
>>> Additionally it clearly shows that there should be two categories of
>>> drm_bridges - non-terminal and terminal.
>>>
>>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>>> its components are up. It simplifies synchronization but is quite
>>> fragile - the whole drm will be down due to error in some of its components.
>>> For this reason I prefer drm_panel as it is not real drm component
>>> it can be attached/detached to/from drm_connector anytime. I am not
>>> really sure but drm_bridge does not allow for that.
>> So I do think we need to stick to this all-or-nothing approach for
>> anything that is visible to userspace
>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>> to "hotplug" those so I don't see a real smooth upgrade path to add
>> that in a backwards compatible way that won't cause problems with old
>> userspace.
>>
>> But, that said, we have more flexibility with things not visible to
>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>> allow things to be completely dynamic (we already have some hard
>> enough locking fun).  But proposals/rfcs/etc welcome.
>>
>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>> needs to implement drm_connector makes me a bit suspicious.  Seems
>> like a symptom of missing things in drm_panel_funcs.  It would be
>> better to always create the connector statically, and just have
>> _detect() -> disconnected if panel==NULL.

ptn3460 has been implemented using drm_bridge and drm_connector, not by
me, to be clear :)
And to make it more clear from what I see ptn3460 exposes following ops:
- pre_enable (via drm_bridge).
- disable (via drm_bridge),
- get_modes (via drm_connector).
Other ops are exposed just to fulfill requirements of drm frameworks, I
guess.


> This is something which only Sean can answer!
> I guess he implemented ptn3460 as connector thinking that bridge would
> be the last
> entity in the video pipeline. If that's a real problem, we can still
> move out the
> connector part.
>
> Regards,
> Ajay

The question is how it can be implemented using only drm_bridge.

>>> Real life example to show importance of it: I have a phone with MIPI-DSI
>>> panel and HDMI. Due to initialization issues HDMI bridge driver
>>> sometimes fails during probe and the drmdev do not start. Of course this
>>> is development stage so I have serial console I can diagnose the
>>> problem, disable HDMI, fix the problem, etc...
>>> But what happens in case of end-user. He will see black screen - bricked
>>> phone. In case the bridge will be implemented using drm_panel
>>> he will have working phone with broken HDMI, much better.
>> well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)

It can break also during phone utilization.

>>
>> I suppose if bridge/panel where loaded dynamically (or at least after
>> drm device and drm_{connector,encoder,etc} are created, it would help
>> a bit here.  I'd kinda hope that isn't the only benefit/reason to make
>> things more dynamic.  Especially if we allow bridges/panels to be
>> unloaded.. (just loading them dynamically doesn't seem as scary from
>> locking perspective)
>>
>>> 4. And the last thing, it is more about the concept/design. drm_bridge,
>>> drm_hw_block suggests that those interfaces describes the whole device:
>>> bridge, panel, whatever.
>> hmm, I don't think this is the case.  I can easily see things like:
>>
>>   struct foo_panel {
>>     struct drm_panel base;
>>     struct drm_bridge bridge;
>>     ...
>>   }
>>
>> where a panel implementation implements both panel and bridge.  In
>> fact that is kinda what I was encouraging.

I guess it can work, but I see it sub-optimal. In general, looking on
the hardware
the same video data goes to the panel and to the bridge (if they are of
the same type of course),
I do not know why it couldn't be mapped to software interfaces. For
example drm_sink, as I described
previously (now it is cited below).

Regards
Andrzej

>>
>> BR,
>> -R
>>
>>> In my approach I have an interface
>>> to describe only one video input port of the device. And drm_panel is
>>> in fact misleading name, drm_sink may be better. So real panel
>>> would implement drm_sink interface. Bridge would implement drm_sink
>>> interface and it will request other drm_sink interface, to interact with
>>> the panel which is after it.
>>> This approach seems to me more flexible. Beside things I have described
>>> above it will allow to implement also more complicated devices, dsi
>>> hubs, video mixers, etc.
>>>
>>>
>>> Regards
>>> Andrzej
>>>
>>>> BR,
>>>> -R
>>>>
>>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>>> another drm_panel. With this approach everything fits much better.
>>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>>> to implement connector in the bridge and you have a driver following
>>>>> linux driver model. And no single bit changed in drm core.
>>>>>
>>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>>> problems with drm_bridges I have decide to attract attention to much
>>>>> more cleaner solution.
>>>>>
>>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>>
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>
>>>>>> Ajay Kumar (3):
>>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>>
>>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Rob Clark]

On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark@gmail.com> wrote:
>>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>>>
>>>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>>>
>>>>>>> Still need to make use of standard list calls and figure out proper way
>>>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>>> As I understand this patchset tries to solve two things:
>>>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>>>> Crtc -> Encoder -> Bridge -> Panel
>>>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>>>
>>>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>>>
>>>>>> This schema means that all the bridges including the last one are seen
>>>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>>>> this particular case, the first bridge (ptn3460) implements connector
>>>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>>>> All this is quite confusing.
>>>>>>
>>>>>> But if you look at the bridge from upstream video interface point of
>>>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>>>> video input side acts as a panel. On the output side it expects a panel,
>>>>>> lvds panel in this case.
>>>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>>>> best name.. I wouldn't object to changing it.
>>>>>
>>>>> But my thinking was to leave in drm_panel_funcs things that are just
>>>>> needed by the connector (get_modes().. and maybe some day we need
>>>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>>>> could (if needed) implement both interfaces.
>>>>>
>>>>> That is basically the same as what you are proposing, but without
>>>>> renaming bridge to panel ;-)
>>>> Good to hear that. However there are points which are not clear for me.
>>>> But first lets clarify names, I will use panel and bridge words
>>>> to describe the hardware, and drm_panel, drm_bridge to describe the
>>>> software interfaces.
>>>>
>>>> What bothers me:
>>>> 1. You want to leave connector related callbacks in drm_panel and
>>>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>>>> must implement connector internally because of this limitation. I guess
>>>> it is quite typical bridge. This problem does not exists in case
>>>> of using drm_panel for ptn3460.
>>>>
>>>> 2. drm_bridge is attached to the encoder, this and the callback order
>>>> suggests the video data flow should be as below:
>>>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>>>
>>>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>>>> drm_bridge should be the last one, so there should be no place to add
>>>> lvds panel implemented as a drm_bridge after it, as it is done in this
>>>> patchset.
>>>>
>>>> Additionally it clearly shows that there should be two categories of
>>>> drm_bridges - non-terminal and terminal.
>>>>
>>>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>>>> its components are up. It simplifies synchronization but is quite
>>>> fragile - the whole drm will be down due to error in some of its components.
>>>> For this reason I prefer drm_panel as it is not real drm component
>>>> it can be attached/detached to/from drm_connector anytime. I am not
>>>> really sure but drm_bridge does not allow for that.
>>> So I do think we need to stick to this all-or-nothing approach for
>>> anything that is visible to userspace
>>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>>> to "hotplug" those so I don't see a real smooth upgrade path to add
>>> that in a backwards compatible way that won't cause problems with old
>>> userspace.
>>>
>>> But, that said, we have more flexibility with things not visible to
>>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>>> allow things to be completely dynamic (we already have some hard
>>> enough locking fun).  But proposals/rfcs/etc welcome.
>>>
>>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>>> needs to implement drm_connector makes me a bit suspicious.  Seems
>>> like a symptom of missing things in drm_panel_funcs.  It would be
>>> better to always create the connector statically, and just have
>>> _detect() -> disconnected if panel==NULL.
>
> ptn3460 has been implemented using drm_bridge and drm_connector, not by
> me, to be clear :)

sure, and afaiu it was adapted from a pre-bridge implementation on
chromeos tree.  So between that, and the fact that bridge and panel
are relatively new, it is not unexpected that some
evolution/refactoring will happen as we go.

> And to make it more clear from what I see ptn3460 exposes following ops:
> - pre_enable (via drm_bridge).
> - disable (via drm_bridge),
> - get_modes (via drm_connector).

sure, this is why I'm leaning towards saying that drm_panel_funcs
should be anything a connector needs that a bridge does not need (to
avoid putting fxn ptrs in drm_bridge_funcs which don't make sense for
a pure bridge)

> Other ops are exposed just to fulfill requirements of drm frameworks, I
> guess.
>
>
>> This is something which only Sean can answer!
>> I guess he implemented ptn3460 as connector thinking that bridge would
>> be the last
>> entity in the video pipeline. If that's a real problem, we can still
>> move out the
>> connector part.
>>
>> Regards,
>> Ajay
>
> The question is how it can be implemented using only drm_bridge.

I'm not entirely sure I understand why.  I think you would want to
have a ptn3460 bridge (pure bridge) + chaining + foo_panel which has
it's bridge interface chained up to ptn3460 and a panel interface
passed to the connector.

(At some point, maybe it makes sense to have a generic
drm_panel_connector which drivers can re-use to avoid duplicating the
connector code, but that is an implementation detail.)

>>>> Real life example to show importance of it: I have a phone with MIPI-DSI
>>>> panel and HDMI. Due to initialization issues HDMI bridge driver
>>>> sometimes fails during probe and the drmdev do not start. Of course this
>>>> is development stage so I have serial console I can diagnose the
>>>> problem, disable HDMI, fix the problem, etc...
>>>> But what happens in case of end-user. He will see black screen - bricked
>>>> phone. In case the bridge will be implemented using drm_panel
>>>> he will have working phone with broken HDMI, much better.
>>> well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
>
> It can break also during phone utilization.
>
>>>
>>> I suppose if bridge/panel where loaded dynamically (or at least after
>>> drm device and drm_{connector,encoder,etc} are created, it would help
>>> a bit here.  I'd kinda hope that isn't the only benefit/reason to make
>>> things more dynamic.  Especially if we allow bridges/panels to be
>>> unloaded.. (just loading them dynamically doesn't seem as scary from
>>> locking perspective)
>>>
>>>> 4. And the last thing, it is more about the concept/design. drm_bridge,
>>>> drm_hw_block suggests that those interfaces describes the whole device:
>>>> bridge, panel, whatever.
>>> hmm, I don't think this is the case.  I can easily see things like:
>>>
>>>   struct foo_panel {
>>>     struct drm_panel base;
>>>     struct drm_bridge bridge;
>>>     ...
>>>   }
>>>
>>> where a panel implementation implements both panel and bridge.  In
>>> fact that is kinda what I was encouraging.
>
> I guess it can work, but I see it sub-optimal. In general, looking on
> the hardware
> the same video data goes to the panel and to the bridge (if they are of
> the same type of course),
> I do not know why it couldn't be mapped to software interfaces. For
> example drm_sink, as I described
> previously (now it is cited below).

I'm not entirely sure why letting a panel implement multiple different
interfaces (where needed) is suboptimal.  It seems more sub-optimal to
put panel related fxns which are only applicable to panels in
drm_bridge_funcs.

Well, my initial reaction when you start talking about drm_src and
drm_sinks is that this can quickly get over-designed.  I'm not trying
to turn kms into v4l2 unless there is a good reason.  But maybe I'm
assuming too much about what you are proposing.

BR,
-R

> Regards
> Andrzej
>
>>>
>>> BR,
>>> -R
>>>
>>>> In my approach I have an interface
>>>> to describe only one video input port of the device. And drm_panel is
>>>> in fact misleading name, drm_sink may be better. So real panel
>>>> would implement drm_sink interface. Bridge would implement drm_sink
>>>> interface and it will request other drm_sink interface, to interact with
>>>> the panel which is after it.
>>>> This approach seems to me more flexible. Beside things I have described
>>>> above it will allow to implement also more complicated devices, dsi
>>>> hubs, video mixers, etc.
>>>>
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>> BR,
>>>>> -R
>>>>>
>>>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>>>> another drm_panel. With this approach everything fits much better.
>>>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>>>> to implement connector in the bridge and you have a driver following
>>>>>> linux driver model. And no single bit changed in drm core.
>>>>>>
>>>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>>>> problems with drm_bridges I have decide to attract attention to much
>>>>>> more cleaner solution.
>>>>>>
>>>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>>>
>>>>>> Regards
>>>>>> Andrzej
>>>>>>
>>>>>>
>>>>>>> Ajay Kumar (3):
>>>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>>>
>>>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Sean Paul]

On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark@gmail.com> wrote:
>>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>>>
>>>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>>>
>>>>>>> Still need to make use of standard list calls and figure out proper way
>>>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>>> As I understand this patchset tries to solve two things:
>>>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>>>> Crtc -> Encoder -> Bridge -> Panel
>>>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>>>
>>>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>>>
>>>>>> This schema means that all the bridges including the last one are seen
>>>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>>>> this particular case, the first bridge (ptn3460) implements connector
>>>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>>>> All this is quite confusing.
>>>>>>
>>>>>> But if you look at the bridge from upstream video interface point of
>>>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>>>> video input side acts as a panel. On the output side it expects a panel,
>>>>>> lvds panel in this case.
>>>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>>>> best name.. I wouldn't object to changing it.
>>>>>
>>>>> But my thinking was to leave in drm_panel_funcs things that are just
>>>>> needed by the connector (get_modes().. and maybe some day we need
>>>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>>>> could (if needed) implement both interfaces.
>>>>>
>>>>> That is basically the same as what you are proposing, but without
>>>>> renaming bridge to panel ;-)
>>>> Good to hear that. However there are points which are not clear for me.
>>>> But first lets clarify names, I will use panel and bridge words
>>>> to describe the hardware, and drm_panel, drm_bridge to describe the
>>>> software interfaces.
>>>>
>>>> What bothers me:
>>>> 1. You want to leave connector related callbacks in drm_panel and
>>>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>>>> must implement connector internally because of this limitation. I guess
>>>> it is quite typical bridge. This problem does not exists in case
>>>> of using drm_panel for ptn3460.
>>>>
>>>> 2. drm_bridge is attached to the encoder, this and the callback order
>>>> suggests the video data flow should be as below:
>>>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>>>
>>>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>>>> drm_bridge should be the last one, so there should be no place to add
>>>> lvds panel implemented as a drm_bridge after it, as it is done in this
>>>> patchset.
>>>>
>>>> Additionally it clearly shows that there should be two categories of
>>>> drm_bridges - non-terminal and terminal.
>>>>
>>>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>>>> its components are up. It simplifies synchronization but is quite
>>>> fragile - the whole drm will be down due to error in some of its components.
>>>> For this reason I prefer drm_panel as it is not real drm component
>>>> it can be attached/detached to/from drm_connector anytime. I am not
>>>> really sure but drm_bridge does not allow for that.
>>> So I do think we need to stick to this all-or-nothing approach for
>>> anything that is visible to userspace
>>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>>> to "hotplug" those so I don't see a real smooth upgrade path to add
>>> that in a backwards compatible way that won't cause problems with old
>>> userspace.
>>>
>>> But, that said, we have more flexibility with things not visible to
>>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>>> allow things to be completely dynamic (we already have some hard
>>> enough locking fun).  But proposals/rfcs/etc welcome.
>>>
>>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>>> needs to implement drm_connector makes me a bit suspicious.  Seems
>>> like a symptom of missing things in drm_panel_funcs.  It would be
>>> better to always create the connector statically, and just have
>>> _detect() -> disconnected if panel==NULL.
>
> ptn3460 has been implemented using drm_bridge and drm_connector, not by
> me, to be clear :)

Right, that was me.


> And to make it more clear from what I see ptn3460 exposes following ops:
> - pre_enable (via drm_bridge).
> - disable (via drm_bridge),
> - get_modes (via drm_connector).
> Other ops are exposed just to fulfill requirements of drm frameworks, I
> guess.
>

Also detect().


>
>> This is something which only Sean can answer!
>> I guess he implemented ptn3460 as connector thinking that bridge would
>> be the last
>> entity in the video pipeline. If that's a real problem, we can still
>> move out the
>> connector part.
>>
>> Regards,
>> Ajay
>
> The question is how it can be implemented using only drm_bridge.
>

It was originally implemented this way. Check out the original
drm_bridge patch and discussion here:
http://lists.freedesktop.org/archives/dri-devel/2013-August/043237.html.

The tl;dr is: If you don't implement the connector as part of the
bridge, you need to hack detect_connection() in drm_crtc_helper to
either go to the existing connector (which, btw will be the incorrect
connector type), or the bridge if there's one connected. That, or the
existing connector needs to know about the bridge's existence and call
into it for detect() and get_modes(). In the second case, the existing
connector will probably also be the wrong connector type.


>>>> Real life example to show importance of it: I have a phone with MIPI-DSI
>>>> panel and HDMI. Due to initialization issues HDMI bridge driver
>>>> sometimes fails during probe and the drmdev do not start. Of course this
>>>> is development stage so I have serial console I can diagnose the
>>>> problem, disable HDMI, fix the problem, etc...
>>>> But what happens in case of end-user. He will see black screen - bricked
>>>> phone. In case the bridge will be implemented using drm_panel
>>>> he will have working phone with broken HDMI, much better.
>>> well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
>
> It can break also during phone utilization.
>
>>>
>>> I suppose if bridge/panel where loaded dynamically (or at least after
>>> drm device and drm_{connector,encoder,etc} are created, it would help
>>> a bit here.  I'd kinda hope that isn't the only benefit/reason to make
>>> things more dynamic.  Especially if we allow bridges/panels to be
>>> unloaded.. (just loading them dynamically doesn't seem as scary from
>>> locking perspective)
>>>
>>>> 4. And the last thing, it is more about the concept/design. drm_bridge,
>>>> drm_hw_block suggests that those interfaces describes the whole device:
>>>> bridge, panel, whatever.
>>> hmm, I don't think this is the case.  I can easily see things like:
>>>
>>>   struct foo_panel {
>>>     struct drm_panel base;
>>>     struct drm_bridge bridge;
>>>     ...
>>>   }
>>>
>>> where a panel implementation implements both panel and bridge.  In
>>> fact that is kinda what I was encouraging.
>
> I guess it can work, but I see it sub-optimal. In general, looking on
> the hardware
> the same video data goes to the panel and to the bridge (if they are of
> the same type of course),
> I do not know why it couldn't be mapped to software interfaces. For
> example drm_sink, as I described
> previously (now it is cited below).
>
> Regards
> Andrzej
>
>>>
>>> BR,
>>> -R
>>>
>>>> In my approach I have an interface
>>>> to describe only one video input port of the device. And drm_panel is
>>>> in fact misleading name, drm_sink may be better. So real panel
>>>> would implement drm_sink interface. Bridge would implement drm_sink
>>>> interface and it will request other drm_sink interface, to interact with
>>>> the panel which is after it.
>>>> This approach seems to me more flexible. Beside things I have described
>>>> above it will allow to implement also more complicated devices, dsi
>>>> hubs, video mixers, etc.
>>>>
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>> BR,
>>>>> -R
>>>>>
>>>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>>>> another drm_panel. With this approach everything fits much better.
>>>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>>>> to implement connector in the bridge and you have a driver following
>>>>>> linux driver model. And no single bit changed in drm core.
>>>>>>
>>>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>>>> problems with drm_bridges I have decide to attract attention to much
>>>>>> more cleaner solution.
>>>>>>
>>>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>>>
>>>>>> Regards
>>>>>> Andrzej
>>>>>>
>>>>>>
>>>>>>> Ajay Kumar (3):
>>>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>>>
>>>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>>>
>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/12/2014 02:45 PM, Rob Clark wrote:
> On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark@gmail.com> wrote:
>>>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>>>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>>>>
>>>>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>>>>
>>>>>>>> Still need to make use of standard list calls and figure out proper way
>>>>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>>>> As I understand this patchset tries to solve two things:
>>>>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>>>>> Crtc -> Encoder -> Bridge -> Panel
>>>>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>>>>
>>>>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>>>>
>>>>>>> This schema means that all the bridges including the last one are seen
>>>>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>>>>> this particular case, the first bridge (ptn3460) implements connector
>>>>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>>>>> All this is quite confusing.
>>>>>>>
>>>>>>> But if you look at the bridge from upstream video interface point of
>>>>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>>>>> video input side acts as a panel. On the output side it expects a panel,
>>>>>>> lvds panel in this case.
>>>>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>>>>> best name.. I wouldn't object to changing it.
>>>>>>
>>>>>> But my thinking was to leave in drm_panel_funcs things that are just
>>>>>> needed by the connector (get_modes().. and maybe some day we need
>>>>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>>>>> could (if needed) implement both interfaces.
>>>>>>
>>>>>> That is basically the same as what you are proposing, but without
>>>>>> renaming bridge to panel ;-)
>>>>> Good to hear that. However there are points which are not clear for me.
>>>>> But first lets clarify names, I will use panel and bridge words
>>>>> to describe the hardware, and drm_panel, drm_bridge to describe the
>>>>> software interfaces.
>>>>>
>>>>> What bothers me:
>>>>> 1. You want to leave connector related callbacks in drm_panel and
>>>>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>>>>> must implement connector internally because of this limitation. I guess
>>>>> it is quite typical bridge. This problem does not exists in case
>>>>> of using drm_panel for ptn3460.
>>>>>
>>>>> 2. drm_bridge is attached to the encoder, this and the callback order
>>>>> suggests the video data flow should be as below:
>>>>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>>>>
>>>>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>>>>> drm_bridge should be the last one, so there should be no place to add
>>>>> lvds panel implemented as a drm_bridge after it, as it is done in this
>>>>> patchset.
>>>>>
>>>>> Additionally it clearly shows that there should be two categories of
>>>>> drm_bridges - non-terminal and terminal.
>>>>>
>>>>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>>>>> its components are up. It simplifies synchronization but is quite
>>>>> fragile - the whole drm will be down due to error in some of its components.
>>>>> For this reason I prefer drm_panel as it is not real drm component
>>>>> it can be attached/detached to/from drm_connector anytime. I am not
>>>>> really sure but drm_bridge does not allow for that.
>>>> So I do think we need to stick to this all-or-nothing approach for
>>>> anything that is visible to userspace
>>>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>>>> to "hotplug" those so I don't see a real smooth upgrade path to add
>>>> that in a backwards compatible way that won't cause problems with old
>>>> userspace.
>>>>
>>>> But, that said, we have more flexibility with things not visible to
>>>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>>>> allow things to be completely dynamic (we already have some hard
>>>> enough locking fun).  But proposals/rfcs/etc welcome.
>>>>
>>>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>>>> needs to implement drm_connector makes me a bit suspicious.  Seems
>>>> like a symptom of missing things in drm_panel_funcs.  It would be
>>>> better to always create the connector statically, and just have
>>>> _detect() -> disconnected if panel==NULL.
>> ptn3460 has been implemented using drm_bridge and drm_connector, not by
>> me, to be clear :)
> sure, and afaiu it was adapted from a pre-bridge implementation on
> chromeos tree.  So between that, and the fact that bridge and panel
> are relatively new, it is not unexpected that some
> evolution/refactoring will happen as we go.
>
>> And to make it more clear from what I see ptn3460 exposes following ops:
>> - pre_enable (via drm_bridge).
>> - disable (via drm_bridge),
>> - get_modes (via drm_connector).
> sure, this is why I'm leaning towards saying that drm_panel_funcs
> should be anything a connector needs that a bridge does not need (to
> avoid putting fxn ptrs in drm_bridge_funcs which don't make sense for
> a pure bridge)
>
>> Other ops are exposed just to fulfill requirements of drm frameworks, I
>> guess.
>>
>>
>>> This is something which only Sean can answer!
>>> I guess he implemented ptn3460 as connector thinking that bridge would
>>> be the last
>>> entity in the video pipeline. If that's a real problem, we can still
>>> move out the
>>> connector part.
>>>
>>> Regards,
>>> Ajay
>> The question is how it can be implemented using only drm_bridge.
> I'm not entirely sure I understand why.  I think you would want to
> have a ptn3460 bridge (pure bridge) + chaining + foo_panel which has
> it's bridge interface chained up to ptn3460 and a panel interface
> passed to the connector.
>
> (At some point, maybe it makes sense to have a generic
> drm_panel_connector which drivers can re-use to avoid duplicating the
> connector code, but that is an implementation detail.)
>
>>>>> Real life example to show importance of it: I have a phone with MIPI-DSI
>>>>> panel and HDMI. Due to initialization issues HDMI bridge driver
>>>>> sometimes fails during probe and the drmdev do not start. Of course this
>>>>> is development stage so I have serial console I can diagnose the
>>>>> problem, disable HDMI, fix the problem, etc...
>>>>> But what happens in case of end-user. He will see black screen - bricked
>>>>> phone. In case the bridge will be implemented using drm_panel
>>>>> he will have working phone with broken HDMI, much better.
>>>> well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
>> It can break also during phone utilization.
>>
>>>> I suppose if bridge/panel where loaded dynamically (or at least after
>>>> drm device and drm_{connector,encoder,etc} are created, it would help
>>>> a bit here.  I'd kinda hope that isn't the only benefit/reason to make
>>>> things more dynamic.  Especially if we allow bridges/panels to be
>>>> unloaded.. (just loading them dynamically doesn't seem as scary from
>>>> locking perspective)
>>>>
>>>>> 4. And the last thing, it is more about the concept/design. drm_bridge,
>>>>> drm_hw_block suggests that those interfaces describes the whole device:
>>>>> bridge, panel, whatever.
>>>> hmm, I don't think this is the case.  I can easily see things like:
>>>>
>>>>   struct foo_panel {
>>>>     struct drm_panel base;
>>>>     struct drm_bridge bridge;
>>>>     ...
>>>>   }
>>>>
>>>> where a panel implementation implements both panel and bridge.  In
>>>> fact that is kinda what I was encouraging.
>> I guess it can work, but I see it sub-optimal. In general, looking on
>> the hardware
>> the same video data goes to the panel and to the bridge (if they are of
>> the same type of course),
>> I do not know why it couldn't be mapped to software interfaces. For
>> example drm_sink, as I described
>> previously (now it is cited below).
> I'm not entirely sure why letting a panel implement multiple different
> interfaces (where needed) is suboptimal.  It seems more sub-optimal to
> put panel related fxns which are only applicable to panels in
> drm_bridge_funcs.

foo implemented using drm_panel and drm_bridge seems to me less optimal
than foo implemented with the same functionality but using drm_panel only.

>
> Well, my initial reaction when you start talking about drm_src and
> drm_sinks is that this can quickly get over-designed.  I'm not trying
> to turn kms into v4l2 unless there is a good reason.  But maybe I'm
> assuming too much about what you are proposing.

OK, lesson learned: avoid using word sink :)

Regards
Andrzej

>
> BR,
> -R
>
>> Regards
>> Andrzej
>>
>>>> BR,
>>>> -R
>>>>
>>>>> In my approach I have an interface
>>>>> to describe only one video input port of the device. And drm_panel is
>>>>> in fact misleading name, drm_sink may be better. So real panel
>>>>> would implement drm_sink interface. Bridge would implement drm_sink
>>>>> interface and it will request other drm_sink interface, to interact with
>>>>> the panel which is after it.
>>>>> This approach seems to me more flexible. Beside things I have described
>>>>> above it will allow to implement also more complicated devices, dsi
>>>>> hubs, video mixers, etc.
>>>>>
>>>>>
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>> BR,
>>>>>> -R
>>>>>>
>>>>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>>>>> another drm_panel. With this approach everything fits much better.
>>>>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>>>>> to implement connector in the bridge and you have a driver following
>>>>>>> linux driver model. And no single bit changed in drm core.
>>>>>>>
>>>>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>>>>> problems with drm_bridges I have decide to attract attention to much
>>>>>>> more cleaner solution.
>>>>>>>
>>>>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>>>>
>>>>>>> Regards
>>>>>>> Andrzej
>>>>>>>
>>>>>>>
>>>>>>>> Ajay Kumar (3):
>>>>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>>>>
>>>>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>>>>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Andrzej Hajda]

On 05/12/2014 06:00 PM, Sean Paul wrote:
> On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 05/09/2014 05:05 PM, Ajay kumar wrote:
>>> On Fri, May 9, 2014 at 7:29 PM, Rob Clark <robdclark@gmail.com> wrote:
>>>> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>> On 05/08/2014 08:24 PM, Rob Clark wrote:
>>>>>> On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>>>>>> On 05/05/2014 09:52 PM, Ajay Kumar wrote:
>>>>>>>> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at:
>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>>>>>>>
>>>>>>>> I have just put up Rob's and Sean's idea of chaining up the bridges
>>>>>>>> in code, and have implemented basic panel controls as a chained bridge.
>>>>>>>> This works well with ptn3460 bridge chip on exynos5250-snow board.
>>>>>>>>
>>>>>>>> Still need to make use of standard list calls and figure out proper way
>>>>>>>> of deleting the bridge chain. So, this is just a rough version.
>>>>>>> As I understand this patchset tries to solve two things:
>>>>>>> 1. Implement panel as drm_bridge, to ease support for hardware chains:
>>>>>>> Crtc -> Encoder -> Bridge -> Panel
>>>>>>> 2. Add support to drm_bridge chaining, to allow software chains:
>>>>>>> drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
>>>>>>>
>>>>>>> It is done using Russian doll schema, ops from the bridge calls the same
>>>>>>> ops from the next bridge and the next bridge ops can do the same.
>>>>>>>
>>>>>>> This schema means that all the bridges including the last one are seen
>>>>>>> from the drm core point of view as a one big drm_bridge. Additionally in
>>>>>>> this particular case, the first bridge (ptn3460) implements connector
>>>>>>> so it is hard to guess what is the location of the 2nd bridge in video
>>>>>>> stream chain, sometimes it is after the connector, sometimes before.
>>>>>>> All this is quite confusing.
>>>>>>>
>>>>>>> But if you look at the bridge from upstream video interface point of
>>>>>>> view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its
>>>>>>> video input side acts as a panel. On the output side it expects a panel,
>>>>>>> lvds panel in this case.
>>>>>> tbh, this is mostly about what we call it.  Perhaps "bridge" isn't the
>>>>>> best name.. I wouldn't object to changing it.
>>>>>>
>>>>>> But my thinking was to leave in drm_panel_funcs things that are just
>>>>>> needed by the connector (get_modes().. and maybe some day we need
>>>>>> detect/etc).  Then leave everything else in drm_bridge_funcs.  A panel
>>>>>> could (if needed) implement both interfaces.
>>>>>>
>>>>>> That is basically the same as what you are proposing, but without
>>>>>> renaming bridge to panel ;-)
>>>>> Good to hear that. However there are points which are not clear for me.
>>>>> But first lets clarify names, I will use panel and bridge words
>>>>> to describe the hardware, and drm_panel, drm_bridge to describe the
>>>>> software interfaces.
>>>>>
>>>>> What bothers me:
>>>>> 1. You want to leave connector related callbacks in drm_panel and
>>>>> the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460
>>>>> must implement connector internally because of this limitation. I guess
>>>>> it is quite typical bridge. This problem does not exists in case
>>>>> of using drm_panel for ptn3460.
>>>>>
>>>>> 2. drm_bridge is attached to the encoder, this and the callback order
>>>>> suggests the video data flow should be as below:
>>>>> drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
>>>>>
>>>>> ptn3460 implements drm_bridge and drm_connector so it suggests its
>>>>> drm_bridge should be the last one, so there should be no place to add
>>>>> lvds panel implemented as a drm_bridge after it, as it is done in this
>>>>> patchset.
>>>>>
>>>>> Additionally it clearly shows that there should be two categories of
>>>>> drm_bridges - non-terminal and terminal.
>>>>>
>>>>> 3. drm_dev uses all-or-nothing approach, ie. it will start only when all
>>>>> its components are up. It simplifies synchronization but is quite
>>>>> fragile - the whole drm will be down due to error in some of its components.
>>>>> For this reason I prefer drm_panel as it is not real drm component
>>>>> it can be attached/detached to/from drm_connector anytime. I am not
>>>>> really sure but drm_bridge does not allow for that.
>>>> So I do think we need to stick to this all-or-nothing approach for
>>>> anything that is visible to userspace
>>>> (drm_{plane,crtc,encoder,connector}).  We don't currently have a way
>>>> to "hotplug" those so I don't see a real smooth upgrade path to add
>>>> that in a backwards compatible way that won't cause problems with old
>>>> userspace.
>>>>
>>>> But, that said, we have more flexibility with things not visible to
>>>> userspace (drm_{panel,bridge}).  I'm not sure how much we want to
>>>> allow things to be completely dynamic (we already have some hard
>>>> enough locking fun).  But proposals/rfcs/etc welcome.
>>>>
>>>> I guess I'm not completely familiar w/ ptn3460, but the fact that it
>>>> needs to implement drm_connector makes me a bit suspicious.  Seems
>>>> like a symptom of missing things in drm_panel_funcs.  It would be
>>>> better to always create the connector statically, and just have
>>>> _detect() -> disconnected if panel==NULL.
>>
>> ptn3460 has been implemented using drm_bridge and drm_connector, not by
>> me, to be clear :)
> 
> Right, that was me.
> 
> 
>> And to make it more clear from what I see ptn3460 exposes following ops:
>> - pre_enable (via drm_bridge).
>> - disable (via drm_bridge),
>> - get_modes (via drm_connector).
>> Other ops are exposed just to fulfill requirements of drm frameworks, I
>> guess.
>>
> 
> Also detect().
> 
> 
>>
>>> This is something which only Sean can answer!
>>> I guess he implemented ptn3460 as connector thinking that bridge would
>>> be the last
>>> entity in the video pipeline. If that's a real problem, we can still
>>> move out the
>>> connector part.
>>>
>>> Regards,
>>> Ajay
>>
>> The question is how it can be implemented using only drm_bridge.
>>
> 
> It was originally implemented this way. Check out the original
> drm_bridge patch and discussion here:
> http://lists.freedesktop.org/archives/dri-devel/2013-August/043237.html.
> 
> The tl;dr is: If you don't implement the connector as part of the
> bridge, you need to hack detect_connection() in drm_crtc_helper to
> either go to the existing connector (which, btw will be the incorrect
> connector type), or the bridge if there's one connected. That, or the
> existing connector needs to know about the bridge's existence and call
> into it for detect() and get_modes(). In the second case, the existing
> connector will probably also be the wrong connector type.

Yes, I understand it and my idea was to use drm_panel instead of
drm_bridge, at least for bridges which provides connector related
functions (get_modes, detect). It seems more straightforward to
implement and it simplifies chaining.

Regards
Andrzej

> 
> 
>>>>> Real life example to show importance of it: I have a phone with MIPI-DSI
>>>>> panel and HDMI. Due to initialization issues HDMI bridge driver
>>>>> sometimes fails during probe and the drmdev do not start. Of course this
>>>>> is development stage so I have serial console I can diagnose the
>>>>> problem, disable HDMI, fix the problem, etc...
>>>>> But what happens in case of end-user. He will see black screen - bricked
>>>>> phone. In case the bridge will be implemented using drm_panel
>>>>> he will have working phone with broken HDMI, much better.
>>>> well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
>>
>> It can break also during phone utilization.
>>
>>>>
>>>> I suppose if bridge/panel where loaded dynamically (or at least after
>>>> drm device and drm_{connector,encoder,etc} are created, it would help
>>>> a bit here.  I'd kinda hope that isn't the only benefit/reason to make
>>>> things more dynamic.  Especially if we allow bridges/panels to be
>>>> unloaded.. (just loading them dynamically doesn't seem as scary from
>>>> locking perspective)
>>>>
>>>>> 4. And the last thing, it is more about the concept/design. drm_bridge,
>>>>> drm_hw_block suggests that those interfaces describes the whole device:
>>>>> bridge, panel, whatever.
>>>> hmm, I don't think this is the case.  I can easily see things like:
>>>>
>>>>   struct foo_panel {
>>>>     struct drm_panel base;
>>>>     struct drm_bridge bridge;
>>>>     ...
>>>>   }
>>>>
>>>> where a panel implementation implements both panel and bridge.  In
>>>> fact that is kinda what I was encouraging.
>>
>> I guess it can work, but I see it sub-optimal. In general, looking on
>> the hardware
>> the same video data goes to the panel and to the bridge (if they are of
>> the same type of course),
>> I do not know why it couldn't be mapped to software interfaces. For
>> example drm_sink, as I described
>> previously (now it is cited below).
>>
>> Regards
>> Andrzej
>>
>>>>
>>>> BR,
>>>> -R
>>>>
>>>>> In my approach I have an interface
>>>>> to describe only one video input port of the device. And drm_panel is
>>>>> in fact misleading name, drm_sink may be better. So real panel
>>>>> would implement drm_sink interface. Bridge would implement drm_sink
>>>>> interface and it will request other drm_sink interface, to interact with
>>>>> the panel which is after it.
>>>>> This approach seems to me more flexible. Beside things I have described
>>>>> above it will allow to implement also more complicated devices, dsi
>>>>> hubs, video mixers, etc.
>>>>>
>>>>>
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>> BR,
>>>>>> -R
>>>>>>
>>>>>>> So why not implement ptn3460 bridge as drm_panel which internally uses
>>>>>>> another drm_panel. With this approach everything fits much better.
>>>>>>> You do not need those (pre|post)_(enable|disable) calls, you do not need
>>>>>>> to implement connector in the bridge and you have a driver following
>>>>>>> linux driver model. And no single bit changed in drm core.
>>>>>>>
>>>>>>> I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2].
>>>>>>> It was not accepted as Inki preferred drm_bridge but as I see the
>>>>>>> problems with drm_bridges I have decide to attract attention to much
>>>>>>> more cleaner solution.
>>>>>>>
>>>>>>> [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559
>>>>>>> [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
>>>>>>>
>>>>>>> Regards
>>>>>>> Andrzej
>>>>>>>
>>>>>>>
>>>>>>>> Ajay Kumar (3):
>>>>>>>>   [RFC V2 1/3] drm: implement chaining of drm bridges
>>>>>>>>   [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
>>>>>>>>   [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
>>>>>>>>
>>>>>>>>  .../bindings/drm/bridge/bridge_panel.txt           |  45 ++++
>>>>>>>>  drivers/gpu/drm/bridge/Kconfig                     |   6 +
>>>>>>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>>>>>>  drivers/gpu/drm/bridge/bridge_panel.c              | 240 +++++++++++++++++++++
>>>>>>>>  drivers/gpu/drm/bridge/ptn3460.c                   |  21 +-
>>>>>>>>  drivers/gpu/drm/drm_crtc.c                         |  13 +-
>>>>>>>>  include/drm/bridge/bridge_panel.h                  |  37 ++++
>>>>>>>>  include/drm/drm_crtc.h                             |   2 +
>>>>>>>>  8 files changed, 360 insertions(+), 5 deletions(-)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>>>>>>>  create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c
>>>>>>>>  create mode 100644 include/drm/bridge/bridge_panel.h
>>>>>>>>
>>

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 785 bytes --]

On Thu, May 08, 2014 at 02:28:12PM -0400, Rob Clark wrote:
> On Thu, May 8, 2014 at 7:57 AM, Inki Dae <inki.dae@samsung.com> wrote:
[...]
> > And more thing, we would need to consider image enhancer device placed
> > between crtc and connector/encoder devices. And it'd better to rename
> > drm_hw_block to drm_bridge, and existing drm_bridge relevant codes
> > should be removed.
> 
> I don't object to changing the name to hw_block or something else.
> Although to avoid introducing too much confusion in this discussion,
> for now I am calling it bridge/drm_bridge_funcs/etc for now ;-)

FWIW, I think the name drm_bridge is fine. It describes a device that
takes a video signal as an input and outputs a video signal, possibly
using a different encoding.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3375 bytes --]

On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
> On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
[...]
> > 4. And the last thing, it is more about the concept/design. drm_bridge,
> > drm_hw_block suggests that those interfaces describes the whole device:
> > bridge, panel, whatever.
> 
> hmm, I don't think this is the case.  I can easily see things like:
> 
>   struct foo_panel {
>     struct drm_panel base;
>     struct drm_bridge bridge;
>     ...
>   }
> 
> where a panel implementation implements both panel and bridge.  In
> fact that is kinda what I was encouraging.

For lack of a better entry point into the discussion, let me pick this
example. It seems like in general we all agree that the basic structure
would have to be something like this:

	CRTC -> encoder -> bridge [ -> bridge ... ] -> panel

Where panel could optionally be a bridge/panel hybrid as Rob pointed
out. I'm not sure if there's panels like this, but I suspect the use
case would be where a panel had built-in controls to modify the image in
some fashion (brightness, saturation, ...). I think those things exist
in DCS for example.

What's missing here, and if I understand correctly what Sean said about
the connector type, what we need to solve is how to wire up the
connector so that it reflects the correct type. So the above would have
to become something like this:

	CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
		connector -----------------------------^

One of the problems with that approach is that panel is more than just a
video sink. It also controls the panel (and optionally backlight) power.
The standard DRM connector helpers don't work well in that case, because
drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
though that could possibly be solved by wrapping it in a small custom
implementation that also controls the panel. Anyway, that's really just
an implementation detail.

So once a complete chain from encoder to panel has been created, we need
a way to find the appropriate connector type. Perhaps to achieve that it
would be useful to instantiate the connector by the bridge that's
connected to the panel. So essentially something like this:

	struct drm_bridge {
		/* to allow chaining */
		struct drm_bridge *next;
		/* for bridges directly connected to a panel or monitor */
		struct drm_connector *connector;
		/* for bridges directly connected to a panel */
		struct drm_panel *panel;

		...
	};

To make this work in a generic way, I think we're missing one bridge
instance. Currently the bridge in an encoder is completely optional, so
if there is no bridge in the system this needs to be special cased. One
way to unify this would be to make drm_encoder implement drm_bridge, or
an alternative would be to make drm_panel implement a bridge. Both can
possibly be dummies stubbed out with simple helpers.

I wonder if this would have any consequences on Dave's DP MST work,
since presumably an MST hub could be considered a bridge. In that case I
guess there would need to be support for multiple "next" bridges,
perhaps a simple doubly-linked list instead of a next pointer. The first
bridge would then be used to model the hub's input and child bridges
would be used to model each
of the outputs.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Daniel Vetter]

On Thu, May 15, 2014 at 12:32:58PM +0200, Thierry Reding wrote:
> On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
> > On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> [...]
> > > 4. And the last thing, it is more about the concept/design. drm_bridge,
> > > drm_hw_block suggests that those interfaces describes the whole device:
> > > bridge, panel, whatever.
> > 
> > hmm, I don't think this is the case.  I can easily see things like:
> > 
> >   struct foo_panel {
> >     struct drm_panel base;
> >     struct drm_bridge bridge;
> >     ...
> >   }
> > 
> > where a panel implementation implements both panel and bridge.  In
> > fact that is kinda what I was encouraging.
> 
> For lack of a better entry point into the discussion, let me pick this
> example. It seems like in general we all agree that the basic structure
> would have to be something like this:
> 
> 	CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
> 
> Where panel could optionally be a bridge/panel hybrid as Rob pointed
> out. I'm not sure if there's panels like this, but I suspect the use
> case would be where a panel had built-in controls to modify the image in
> some fashion (brightness, saturation, ...). I think those things exist
> in DCS for example.
> 
> What's missing here, and if I understand correctly what Sean said about
> the connector type, what we need to solve is how to wire up the
> connector so that it reflects the correct type. So the above would have
> to become something like this:
> 
> 	CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
> 		connector -----------------------------^

This is kinda always how I've thought it should play out: The last item in
the chain actually implements the drm connector, everyone else just
ignores it.

> One of the problems with that approach is that panel is more than just a
> video sink. It also controls the panel (and optionally backlight) power.
> The standard DRM connector helpers don't work well in that case, because
> drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
> though that could possibly be solved by wrapping it in a small custom
> implementation that also controls the panel. Anyway, that's really just
> an implementation detail.

I don't see an issue with the dpms really. At worst the overall kms driver
(which provides the crtc and encoder implementation) needs to pass a
suitable dpms implementation for the drm_bridge to use in the connector.
Or we could just move this vfunc into the core kms funtion table since
everyone uses the same (well except i915, but that's a different story).

dpms changes would then still be driven through the crtc -> encoder ->
bridge ... chain.

> So once a complete chain from encoder to panel has been created, we need
> a way to find the appropriate connector type. Perhaps to achieve that it
> would be useful to instantiate the connector by the bridge that's
> connected to the panel. So essentially something like this:
> 
> 	struct drm_bridge {
> 		/* to allow chaining */
> 		struct drm_bridge *next;
> 		/* for bridges directly connected to a panel or monitor */
> 		struct drm_connector *connector;
> 		/* for bridges directly connected to a panel */
> 		struct drm_panel *panel;

Imo these two shouldn't be here, but instead should be embedded into the
overall structure the drm_bridge driver is using.
> 
> 		...
> 	};
> 
> To make this work in a generic way, I think we're missing one bridge
> instance. Currently the bridge in an encoder is completely optional, so
> if there is no bridge in the system this needs to be special cased. One
> way to unify this would be to make drm_encoder implement drm_bridge, or
> an alternative would be to make drm_panel implement a bridge. Both can
> possibly be dummies stubbed out with simple helpers.

Yeah, imo if the panel isn't just a dumb thing but needs to actively take
part in the modeset dance, it simply needs to implement itself as a
drm_bridge+panel combo and do the magic in the various hooks provided.

> I wonder if this would have any consequences on Dave's DP MST work,
> since presumably an MST hub could be considered a bridge. In that case I
> guess there would need to be support for multiple "next" bridges,
> perhaps a simple doubly-linked list instead of a next pointer. The first
> bridge would then be used to model the hub's input and child bridges
> would be used to model each
> of the outputs.

DP is standardize. No need to wrestle the complexity through a drm_bridge,
since code reuse anywhere else (non-DP) will be know to be zero. And for
all the guys implementing a DP output can simply use the helpers. So I
don't see an upside of this idea.

Imo we should restrict drm_bridge to all the crazy transcoder chips/blocks
commonly found on SoCs which all do non-standard stuff and where we
actually have an established or anticipated need for sharing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC V2 0/3] drm/bridge: panel and chaining

[Ajay kumar]

On Thu, May 15, 2014 at 6:04 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, May 15, 2014 at 12:32:58PM +0200, Thierry Reding wrote:
>> On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
>> > On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> [...]
>> > > 4. And the last thing, it is more about the concept/design. drm_bridge,
>> > > drm_hw_block suggests that those interfaces describes the whole device:
>> > > bridge, panel, whatever.
>> >
>> > hmm, I don't think this is the case.  I can easily see things like:
>> >
>> >   struct foo_panel {
>> >     struct drm_panel base;
>> >     struct drm_bridge bridge;
>> >     ...
>> >   }
>> >
>> > where a panel implementation implements both panel and bridge.  In
>> > fact that is kinda what I was encouraging.
>>
>> For lack of a better entry point into the discussion, let me pick this
>> example. It seems like in general we all agree that the basic structure
>> would have to be something like this:
>>
>>       CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
>>
>> Where panel could optionally be a bridge/panel hybrid as Rob pointed
>> out. I'm not sure if there's panels like this, but I suspect the use
>> case would be where a panel had built-in controls to modify the image in
>> some fashion (brightness, saturation, ...). I think those things exist
>> in DCS for example.
>>
>> What's missing here, and if I understand correctly what Sean said about
>> the connector type, what we need to solve is how to wire up the
>> connector so that it reflects the correct type. So the above would have
>> to become something like this:
>>
>>       CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
>>               connector -----------------------------^
>
> This is kinda always how I've thought it should play out: The last item in
> the chain actually implements the drm connector, everyone else just
> ignores it.
The last bridge or the panel?


>> One of the problems with that approach is that panel is more than just a
>> video sink. It also controls the panel (and optionally backlight) power.
>> The standard DRM connector helpers don't work well in that case, because
>> drm_helper_connector_dpms() forwards changes to the CRTC and encoder,
>> though that could possibly be solved by wrapping it in a small custom
>> implementation that also controls the panel. Anyway, that's really just
>> an implementation detail.
Why did this discussion come up? Are we going to call panel controls
from a common layer?


> I don't see an issue with the dpms really. At worst the overall kms driver
> (which provides the crtc and encoder implementation) needs to pass a
> suitable dpms implementation for the drm_bridge to use in the connector.
> Or we could just move this vfunc into the core kms funtion table since
> everyone uses the same (well except i915, but that's a different story).
>
> dpms changes would then still be driven through the crtc -> encoder ->
> bridge ... chain.
>
>> So once a complete chain from encoder to panel has been created, we need
>> a way to find the appropriate connector type. Perhaps to achieve that it
>> would be useful to instantiate the connector by the bridge that's
>> connected to the panel. So essentially something like this:
>>
>>       struct drm_bridge {
>>               /* to allow chaining */
>>               struct drm_bridge *next;
>>               /* for bridges directly connected to a panel or monitor */
>>               struct drm_connector *connector;
>>               /* for bridges directly connected to a panel */
>>               struct drm_panel *panel;
>
> Imo these two shouldn't be here, but instead should be embedded into the
> overall structure the drm_bridge driver is using.
>>
>>               ...
>>       };
>>
>> To make this work in a generic way, I think we're missing one bridge
>> instance. Currently the bridge in an encoder is completely optional, so
>> if there is no bridge in the system this needs to be special cased. One
>> way to unify this would be to make drm_encoder implement drm_bridge, or
>> an alternative would be to make drm_panel implement a bridge. Both can
>> possibly be dummies stubbed out with simple helpers.
>
> Yeah, imo if the panel isn't just a dumb thing but needs to actively take
> part in the modeset dance, it simply needs to implement itself as a
> drm_bridge+panel combo and do the magic in the various hooks provided.

And, what does one mean when a real panel should implement both
drm_panel and drm_bridge?
who will call drm_bridge->funcs for the panel?, and
who will call drm_panel->funcs for the panel?
Can someone please elaborate this with existing implementation details
for bridge and panel? I am really getting confused.

>> I wonder if this would have any consequences on Dave's DP MST work,
>> since presumably an MST hub could be considered a bridge. In that case I
>> guess there would need to be support for multiple "next" bridges,
>> perhaps a simple doubly-linked list instead of a next pointer. The first
>> bridge would then be used to model the hub's input and child bridges
>> would be used to model each
>> of the outputs.
>
> DP is standardize. No need to wrestle the complexity through a drm_bridge,
> since code reuse anywhere else (non-DP) will be know to be zero. And for
> all the guys implementing a DP output can simply use the helpers. So I
> don't see an upside of this idea.
>
> Imo we should restrict drm_bridge to all the crazy transcoder chips/blocks
> commonly found on SoCs which all do non-standard stuff and where we
> actually have an established or anticipated need for sharing.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch


Thanks,
Ajay