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

[RFC V3 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

Inital discussion can be found at:
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg30008.html

As Rob suggested, I have added few static-inline helpers for bridge chaining,
and as Sean suggested, I have moved the bridge chain creation to
encoder implementation(exynos_dp).

Ajay Kumar (3):
  [RFC V3 1/3] drm/bridge: add helper functions to support bridge chain
  [RFC V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges
  [RFC V3 3/3] drm: create and demonstrate bridge chaining using exynos_dp

 .../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              |  217 ++++++++++++++++++++
 drivers/gpu/drm/bridge/ptn3460.c                   |   60 ++++--
 drivers/gpu/drm/exynos/exynos_dp_core.c            |   25 ++-
 include/drm/bridge/bridge_panel.h                  |   40 ++++
 include/drm/bridge/ptn3460.h                       |   15 +-
 include/drm/drm_crtc.h                             |   72 +++++++
 9 files changed, 453 insertions(+), 28 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.7.9.5

[RFC V3 1/3] drm/bridge: add helper functions to support bridge chain

[Ajay Kumar]

Add helper functions to create bridge chain and to call the
corresponding next_bridge functions.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Suggested-by: Rob Clark <robdclark@gmail.com>
---
 include/drm/drm_crtc.h |   72 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e55fccb..91ab89c 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;
 
@@ -1073,6 +1074,77 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 	return mo ? obj_to_encoder(mo) : NULL;
 }
 
+static inline int drm_bridge_add_to_chain(struct drm_bridge *head,
+					  struct drm_bridge *last)
+{
+	struct drm_bridge *temp = head;
+
+	if (head && last) {
+		while (temp->next_bridge)
+			temp = temp->next_bridge;
+
+		temp->next_bridge = last;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static inline void drm_next_bridge_mode_fixup(struct drm_bridge *bridge,
+					const struct drm_display_mode *mode,
+					struct drm_display_mode *adjusted_mode)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->mode_fixup)
+		bridge->next_bridge->funcs->mode_fixup(bridge->next_bridge,
+							mode, adjusted_mode);
+}
+
+static inline void drm_next_bridge_disable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->disable)
+		bridge->next_bridge->funcs->disable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_post_disable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->post_disable)
+		bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_mode_set(struct drm_bridge *bridge,
+					struct drm_display_mode *mode,
+					struct drm_display_mode *adjusted_mode)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->mode_set)
+		bridge->next_bridge->funcs->mode_set(bridge->next_bridge,
+							mode, adjusted_mode);
+}
+
+static inline void drm_next_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->pre_enable)
+		bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_enable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->enable)
+		bridge->next_bridge->funcs->enable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_destroy(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->destroy)
+		bridge->next_bridge->funcs->destroy(bridge->next_bridge);
+}
+
 /* Plane list iterator for legacy (overlay only) planes. */
 #define drm_for_each_legacy_plane(plane, planelist) \
 	list_for_each_entry(plane, planelist, head) \
-- 
1.7.9.5

[RFC V3 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              |  217 ++++++++++++++++++++
 include/drm/bridge/bridge_panel.h                  |   40 ++++
 5 files changed, 309 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..c629e93
--- /dev/null
+++ b/drivers/gpu/drm/bridge/bridge_panel.c
@@ -0,0 +1,217 @@
+/*
+ * 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"
+
+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;
+	bool			backlight_fet_enabled;
+	bool			lcd_fet_enabled;
+	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;
+};
+
+static void bridge_panel_pre_enable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (!IS_ERR_OR_NULL(panel->lcd_fet))
+		if (!panel->lcd_fet_enabled) {
+			if (regulator_enable(panel->lcd_fet))
+				DRM_ERROR("Failed to enable LCD fet\n");
+			panel->lcd_fet_enabled = true;
+		}
+
+	if (gpio_is_valid(panel->lcd_en_gpio))
+		gpio_set_value(panel->lcd_en_gpio, 1);
+
+	msleep(panel->panel_pre_enable_delay);
+}
+
+static void bridge_panel_enable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (!IS_ERR_OR_NULL(panel->backlight_fet))
+		if (!panel->backlight_fet_enabled) {
+			if (regulator_enable(panel->backlight_fet))
+				DRM_ERROR("Failed to enable LED fet\n");
+			panel->backlight_fet_enabled = true;
+		}
+
+	msleep(panel->panel_enable_delay);
+
+	if (gpio_is_valid(panel->led_en_gpio))
+		gpio_set_value(panel->led_en_gpio, 1);
+}
+
+static void bridge_panel_disable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (gpio_is_valid(panel->led_en_gpio))
+		gpio_set_value(panel->led_en_gpio, 0);
+
+	if (!IS_ERR_OR_NULL(panel->backlight_fet))
+		if (panel->backlight_fet_enabled) {
+			regulator_disable(panel->backlight_fet);
+			panel->backlight_fet_enabled = false;
+		}
+
+	msleep(panel->panel_disable_delay);
+}
+
+static void bridge_panel_post_disable(struct drm_bridge *bridge)
+{
+	struct bridge_panel *panel = bridge->driver_private;
+
+	if (gpio_is_valid(panel->lcd_en_gpio))
+		gpio_set_value(panel->lcd_en_gpio, 0);
+
+	if (!IS_ERR_OR_NULL(panel->lcd_fet))
+		if (panel->lcd_fet_enabled) {
+			regulator_disable(panel->lcd_fet);
+			panel->lcd_fet_enabled = false;
+		}
+
+	msleep(panel->panel_post_disable_delay);
+}
+
+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,
+};
+
+struct drm_bridge *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 NULL;
+	}
+
+	panel = devm_kzalloc(dev->dev, sizeof(*panel), GFP_KERNEL);
+	if (!panel) {
+		DRM_ERROR("Failed to allocate bridge panel\n");
+		return NULL;
+	}
+
+	panel->client = client;
+	panel->encoder = encoder;
+	panel->bridge = bridge;
+
+	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 NULL;
+
+	panel->backlight_fet = devm_regulator_get(dev->dev, "vcd_led");
+	if (IS_ERR(panel->backlight_fet))
+		return NULL;
+
+	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 NULL;
+		}
+	} 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 NULL;
+		}
+	} else {
+		panel->led_en_gpio = -ENODEV;
+	}
+
+	ret = drm_bridge_init(dev, bridge, &bridge_panel_funcs);
+	if (ret) {
+		DRM_ERROR("Failed to initialize bridge with drm\n");
+		goto err;
+	}
+
+	bridge->driver_private = panel;
+
+	if (!encoder->bridge)
+		/* First entry in the bridge chain */
+		encoder->bridge = bridge;
+
+	return bridge;
+
+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 NULL;
+}
+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..7f0d662
--- /dev/null
+++ b/include/drm/bridge/bridge_panel.h
@@ -0,0 +1,40 @@
+/*
+ * 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)
+
+struct drm_bridge *bridge_panel_init(struct drm_device *dev,
+					struct drm_encoder *encoder,
+					struct i2c_client *client,
+					struct device_node *node);
+#else
+
+static inline struct drm_bridge *bridge_panel_init(struct drm_device *dev,
+						struct drm_encoder *encoder,
+						struct i2c_client *client,
+						struct device_node *node)
+{
+	return 0;
+}
+
+#endif
+
+#endif
-- 
1.7.9.5

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

[Thierry Reding]

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

On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote:
> 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 ++++

Can we please stop adding files to this directory? Device tree bindings
are supposed to be OS agnostic, but DRM is specific to Linux and should
not be used when describing hardware.

> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
[...]
> +	-led-en-gpio:
> +		eDP panel LED enable GPIO.
> +			Indicates which GPIO needs to be powered up as output
> +			to enable the backlight.

Since this is used to control a backlight, then this should really be a
separate node to describe the backlight device (and use the
corresponding backlight driver) rather than duplicating a subset of that
functionality.

> +	-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.

What are panel_VCC or 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.

Similarily, what does BL_EN stand for?

> +	bridge-panel {
> +		compatible = "drm-bridge,panel";

Again, drm- doesn't mean anything outside of Linux (and maybe BSD),
therefore shouldn't be used to describe hardware in device tree.

> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
[...]

This duplicates much of the functionality that panels should provide. I
think a better solution would be to properly integrate panels with
bridges.

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 V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay kumar]

On Tue, May 13, 2014 at 1:35 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote:
>> 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 ++++
>
> Can we please stop adding files to this directory? Device tree bindings
> are supposed to be OS agnostic, but DRM is specific to Linux and should
> not be used when describing hardware.
One should not be adding a devicetree node if it is not describing the
actual hardware?

>> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
> [...]
>> +     -led-en-gpio:
>> +             eDP panel LED enable GPIO.
>> +                     Indicates which GPIO needs to be powered up as output
>> +                     to enable the backlight.
>
> Since this is used to control a backlight, then this should really be a
> separate node to describe the backlight device (and use the
> corresponding backlight driver) rather than duplicating a subset of that
> functionality.
I am stressing this point again!
Backlight should ideally be enabled only after:
1) Panel is powered up (LCD_VDD)
2) HPD is thrown.
3) Valid data starts coming from the encoder(DP in our case)
All the above 3 are controlled inside drm, but pwm-backlight is
independent of drm. So, we let the pwm_config happen in pwm-backlight driver
and enable backlight GPIO(BL_EN) inside drm, after valid video data starts
coming out of the encoder.
As you said, we can get this GPIO from a backlight device node, but since
this GPIO is related to panel also, I think conceptually its not wrong
to have this
GPIO binding defined in a panel node.

>> +     -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.
>
> What are panel_VCC or video_enable?
It is the time taken for the panel to throw a valid HPD from the point
we enabled LCD_VCC.
After HPD is thrown, we can start sending video data.
>> +     -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.
>
> Similarily, what does BL_EN stand for?
Backlight Enable, same as "enable-gpios" in pwm-backlight driver.

>> +     bridge-panel {
>> +             compatible = "drm-bridge,panel";
>
> Again, drm- doesn't mean anything outside of Linux (and maybe BSD),
> therefore shouldn't be used to describe hardware in device tree.
Agreed. :)

>> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
> [...]
>
> This duplicates much of the functionality that panels should provide. I
> think a better solution would be to properly integrate panels with
> bridges.
True, ideally I should be calling drm_panel functions from a simple dummy bridge
which sits at the end of the bridge chain. But, since you are not
convinced with having
pre_enable and post_disable calls for panels, I had no other way to do
this, than
stuffing these panel controls inside bridge! :(
See the discussion here. I have already explained this!
http://www.spinics.net/lists/dri-devel/msg57973.html

Ajay

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

[Thierry Reding]

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

On Tue, May 13, 2014 at 10:19:33PM +0530, Ajay kumar wrote:
> On Tue, May 13, 2014 at 1:35 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote:
> >> 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 ++++
> >
> > Can we please stop adding files to this directory? Device tree bindings
> > are supposed to be OS agnostic, but DRM is specific to Linux and should
> > not be used when describing hardware.
> One should not be adding a devicetree node if it is not describing the
> actual hardware?

Correct. But my point is that even if you describe actual hardware, then
the bindings don't belong in a drm subdirectory, because DRM does not
make any sense in a hardware context.

Something like video/bridge may be a better name for a directory.

> >> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
> > [...]
> >> +     -led-en-gpio:
> >> +             eDP panel LED enable GPIO.
> >> +                     Indicates which GPIO needs to be powered up as output
> >> +                     to enable the backlight.
> >
> > Since this is used to control a backlight, then this should really be a
> > separate node to describe the backlight device (and use the
> > corresponding backlight driver) rather than duplicating a subset of that
> > functionality.
> I am stressing this point again!
> Backlight should ideally be enabled only after:
> 1) Panel is powered up (LCD_VDD)
> 2) HPD is thrown.
> 3) Valid data starts coming from the encoder(DP in our case)
> All the above 3 are controlled inside drm, but pwm-backlight is
> independent of drm. So, we let the pwm_config happen in pwm-backlight driver
> and enable backlight GPIO(BL_EN) inside drm, after valid video data starts
> coming out of the encoder.

But that's completely the wrong way around. Why can't you simply control
the backlight from within DRM and only enable it at the right time?

> As you said, we can get this GPIO from a backlight device node, but since
> this GPIO is related to panel also, I think conceptually its not wrong
> to have this
> GPIO binding defined in a panel node.

It's not related to the panel. It's an enable for the backlight.
Backlight could be considered part of the panel, therefore it makes
sense to control the backlight from the panel driver.

> >> +     -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.
> >
> > What are panel_VCC or video_enable?
> It is the time taken for the panel to throw a valid HPD from the point
> we enabled LCD_VCC.
> After HPD is thrown, we can start sending video data.

What does "throw a valid HPD" mean? Is it an actual signal that can be
captured via software (GPIO, interrupt, ...)? If so then it would be
preferable to not use a delay at all but rather rely on that mechanism
to determine when it's safe to send a video signal.

> >> +     -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.
> >
> > Similarily, what does BL_EN stand for?
> Backlight Enable, same as "enable-gpios" in pwm-backlight driver.

When writing a binding it's useful to describe these things without
referring to signal names or abbreviations, since they may be something
else for different people.

> >> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
> > [...]
> >
> > This duplicates much of the functionality that panels should provide. I
> > think a better solution would be to properly integrate panels with
> > bridges.
> True, ideally I should be calling drm_panel functions from a simple dummy
> bridge which sits at the end of the bridge chain. But, since you are not
> convinced with having pre_enable and post_disable calls for panels, I had
> no other way to do this, than stuffing these panel controls inside
> bridge! :(

What makes you think that I would suddenly be any more convinced by this
solution than by your prior proposal? I didn't say outright no to what
you proposed earlier. What I said was that I didn't like it and that I
thought we could come up with a better solution. Part of getting to a
better solution is trying to understand the problems involved. You don't
solve a problem by simply moving code into a different driver.

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 V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay kumar]

On Wed, May 14, 2014 at 8:24 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, May 13, 2014 at 10:19:33PM +0530, Ajay kumar wrote:
>> On Tue, May 13, 2014 at 1:35 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Fri, May 09, 2014 at 08:23:01PM +0530, Ajay Kumar wrote:
>> >> 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 ++++
>> >
>> > Can we please stop adding files to this directory? Device tree bindings
>> > are supposed to be OS agnostic, but DRM is specific to Linux and should
>> > not be used when describing hardware.
>> One should not be adding a devicetree node if it is not describing the
>> actual hardware?
>
> Correct. But my point is that even if you describe actual hardware, then
> the bindings don't belong in a drm subdirectory, because DRM does not
> make any sense in a hardware context.
>
> Something like video/bridge may be a better name for a directory.
Ok, this is just about the name.

>> >> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>> > [...]
>> >> +     -led-en-gpio:
>> >> +             eDP panel LED enable GPIO.
>> >> +                     Indicates which GPIO needs to be powered up as output
>> >> +                     to enable the backlight.
>> >
>> > Since this is used to control a backlight, then this should really be a
>> > separate node to describe the backlight device (and use the
>> > corresponding backlight driver) rather than duplicating a subset of that
>> > functionality.
>> I am stressing this point again!
>> Backlight should ideally be enabled only after:
>> 1) Panel is powered up (LCD_VDD)
>> 2) HPD is thrown.
>> 3) Valid data starts coming from the encoder(DP in our case)
>> All the above 3 are controlled inside drm, but pwm-backlight is
>> independent of drm. So, we let the pwm_config happen in pwm-backlight driver
>> and enable backlight GPIO(BL_EN) inside drm, after valid video data starts
>> coming out of the encoder.
>
> But that's completely the wrong way around. Why can't you simply control
> the backlight from within DRM and only enable it at the right time?
I am trying to reuse existing code as much as possible. pwm-backlight driver
takes care of generating proper pwm signals for the given backlight value,
so I am reusing that part of it. Even though it also provides a way to handle
"backlight enable" pin(which is ofcourse optional) I wish to control it in the
panel drvier, because generally all panel datasheets say you should control
backlight enable along with panel controls.

>> As you said, we can get this GPIO from a backlight device node, but since
>> this GPIO is related to panel also, I think conceptually its not wrong
>> to have this
>> GPIO binding defined in a panel node.
>
> It's not related to the panel. It's an enable for the backlight.
> Backlight could be considered part of the panel, therefore it makes
> sense to control the backlight from the panel driver.
Ok. This GPIO goes from my AP to the backlight unit of the panel.
Basically, a board related GPIO which exists on all the boards.
Where should I be handling this?

>> >> +     -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.
>> >
>> > What are panel_VCC or video_enable?
>> It is the time taken for the panel to throw a valid HPD from the point
>> we enabled LCD_VCC.
>> After HPD is thrown, we can start sending video data.
>
> What does "throw a valid HPD" mean? Is it an actual signal that can be
> captured via software (GPIO, interrupt, ...)? If so then it would be
> preferable to not use a delay at all but rather rely on that mechanism
> to determine when it's safe to send a video signal.
Right, your explanation holds good, but only for the case of eDP panels.
But, when we use eDP-LVDS bridges(ps8622), its a different case!
the bridge takes care of sending HPD, so the encoder gets HPD interrupt
and tries to read edid from the panel, but the panel might not have powered up
enough to read edid. In such cases we would need delay.
So, having a delay field here would be really useful.
May be, while describing this particular DT binding I should elaborate more.

>> >> +     -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.
>> >
>> > Similarily, what does BL_EN stand for?
>> Backlight Enable, same as "enable-gpios" in pwm-backlight driver.
>
> When writing a binding it's useful to describe these things without
> referring to signal names or abbreviations, since they may be something
> else for different people.
Ok. Will take care of this from now on :)

>> >> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
>> > [...]
>> >
>> > This duplicates much of the functionality that panels should provide. I
>> > think a better solution would be to properly integrate panels with
>> > bridges.
>> True, ideally I should be calling drm_panel functions from a simple dummy
>> bridge which sits at the end of the bridge chain. But, since you are not
>> convinced with having pre_enable and post_disable calls for panels, I had
>> no other way to do this, than stuffing these panel controls inside
>> bridge! :(
>
> What makes you think that I would suddenly be any more convinced by this
> solution than by your prior proposal? I didn't say outright no to what
> you proposed earlier. What I said was that I didn't like it and that I
> thought we could come up with a better solution. Part of getting to a
> better solution is trying to understand the problems involved. You don't
> solve a problem by simply moving code into a different driver.
Ok. What is better solution according to you?
Handling the backlight enable GPIO in pwm-backlight driver is not
a better soultion "ALWAYS". Sometimes, we may just need to control it
along with video encoders. And, even the panel datasheets mention that
in "Power On/off sequence" chapter.
And, I have explained the problems and feasible solutions.
If drm_panel can support pre_enable/post_disable, we can easily
implement a generic bridge which sits at the end of bridge chain, and calls
corresponding drm_panel functions.

Please see the discussion happening here:
https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg30586.html


Ajay

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

[Thierry Reding]

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

On Wed, May 14, 2014 at 11:39:30PM +0530, Ajay kumar wrote:
[...]
> >> >> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
> >> > [...]
> >> >> +     -led-en-gpio:
> >> >> +             eDP panel LED enable GPIO.
> >> >> +                     Indicates which GPIO needs to be powered up as output
> >> >> +                     to enable the backlight.
> >> >
> >> > Since this is used to control a backlight, then this should really be a
> >> > separate node to describe the backlight device (and use the
> >> > corresponding backlight driver) rather than duplicating a subset of that
> >> > functionality.
> >> I am stressing this point again!
> >> Backlight should ideally be enabled only after:
> >> 1) Panel is powered up (LCD_VDD)
> >> 2) HPD is thrown.
> >> 3) Valid data starts coming from the encoder(DP in our case)
> >> All the above 3 are controlled inside drm, but pwm-backlight is
> >> independent of drm. So, we let the pwm_config happen in pwm-backlight driver
> >> and enable backlight GPIO(BL_EN) inside drm, after valid video data starts
> >> coming out of the encoder.
> >
> > But that's completely the wrong way around. Why can't you simply control
> > the backlight from within DRM and only enable it at the right time?
> I am trying to reuse existing code as much as possible. pwm-backlight driver
> takes care of generating proper pwm signals for the given backlight value,
> so I am reusing that part of it. Even though it also provides a way to handle
> "backlight enable" pin(which is ofcourse optional) I wish to control it in the
> panel drvier, because generally all panel datasheets say you should control
> backlight enable along with panel controls.

I still don't see how controlling the enable GPIO from the panel will be
in any way better, or different for that matter, from simply enabling
the backlight and let the backlight driver handle the enable GPIO. Have
you tried to do that and found that it doesn't work?

> >> As you said, we can get this GPIO from a backlight device node, but since
> >> this GPIO is related to panel also, I think conceptually its not wrong
> >> to have this
> >> GPIO binding defined in a panel node.
> >
> > It's not related to the panel. It's an enable for the backlight.
> > Backlight could be considered part of the panel, therefore it makes
> > sense to control the backlight from the panel driver.
> Ok. This GPIO goes from my AP to the backlight unit of the panel.
> Basically, a board related GPIO which exists on all the boards.
> Where should I be handling this?

In the driver that handles the backlight unit. That is: pwm-backlight.

> >> > What are panel_VCC or video_enable?
> >> It is the time taken for the panel to throw a valid HPD from the point
> >> we enabled LCD_VCC.
> >> After HPD is thrown, we can start sending video data.
> >
> > What does "throw a valid HPD" mean? Is it an actual signal that can be
> > captured via software (GPIO, interrupt, ...)? If so then it would be
> > preferable to not use a delay at all but rather rely on that mechanism
> > to determine when it's safe to send a video signal.
> Right, your explanation holds good, but only for the case of eDP panels.
> But, when we use eDP-LVDS bridges(ps8622), its a different case!
> the bridge takes care of sending HPD, so the encoder gets HPD interrupt
> and tries to read edid from the panel, but the panel might not have powered up
> enough to read edid. In such cases we would need delay.
> So, having a delay field here would be really useful.

Okay.

> May be, while describing this particular DT binding I should elaborate more.

Yes, something like the above explanation would be good to have in the
binding.

> >> >> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
> >> > [...]
> >> >
> >> > This duplicates much of the functionality that panels should provide. I
> >> > think a better solution would be to properly integrate panels with
> >> > bridges.
> >> True, ideally I should be calling drm_panel functions from a simple dummy
> >> bridge which sits at the end of the bridge chain. But, since you are not
> >> convinced with having pre_enable and post_disable calls for panels, I had
> >> no other way to do this, than stuffing these panel controls inside
> >> bridge! :(
> >
> > What makes you think that I would suddenly be any more convinced by this
> > solution than by your prior proposal? I didn't say outright no to what
> > you proposed earlier. What I said was that I didn't like it and that I
> > thought we could come up with a better solution. Part of getting to a
> > better solution is trying to understand the problems involved. You don't
> > solve a problem by simply moving code into a different driver.
> Ok. What is better solution according to you?
> Handling the backlight enable GPIO in pwm-backlight driver is not
> a better soultion "ALWAYS". Sometimes, we may just need to control it
> along with video encoders.

You keep saying that, but you haven't said *why* you need to do that.

> And, even the panel datasheets mention that  in "Power On/off sequence"
> chapter.

Can you point me to such a datasheet?

> And, I have explained the problems and feasible solutions.

You have explained the solutions that you've found to the problem. I'm
trying to understand the problem fully so that perhaps we can find a
more generic solution.

> If drm_panel can support pre_enable/post_disable, we can easily
> implement a generic bridge which sits at the end of bridge chain, and
> calls corresponding drm_panel functions.

The problem with blindly adding .pre_enable() and .post_disable()
without thinking this through is that it can easily introduce
incompatibilities between various drivers and panels. There is a risk
that some of the code that goes into a panel's .pre_enable() will be
specific to a given setup (and perhaps not even related to the panel
at all). If that panel is reused on a different board where the
restrictions may be different the panel may not work.

So before we go and add any new functions to DRM panel I'd like to see
them properly documented. It needs to be defined precisely what the
effect of these functions should be so that both panel driver writers
know what to implement and display driver writers need to know when to
call which function.

Also, please let's not call them .pre_enable() and .post_disable(). The
names should be something more specific to reflect what they are meant
to do. Even something like .prepare() and .unprepare() would be better
choices.

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 V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay kumar]

On Thu, May 15, 2014 at 1:43 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, May 14, 2014 at 11:39:30PM +0530, Ajay kumar wrote:
> [...]
>> >> >> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>> >> > [...]
>> >> >> +     -led-en-gpio:
>> >> >> +             eDP panel LED enable GPIO.
>> >> >> +                     Indicates which GPIO needs to be powered up as output
>> >> >> +                     to enable the backlight.
>> >> >
>> >> > Since this is used to control a backlight, then this should really be a
>> >> > separate node to describe the backlight device (and use the
>> >> > corresponding backlight driver) rather than duplicating a subset of that
>> >> > functionality.
>> >> I am stressing this point again!
>> >> Backlight should ideally be enabled only after:
>> >> 1) Panel is powered up (LCD_VDD)
>> >> 2) HPD is thrown.
>> >> 3) Valid data starts coming from the encoder(DP in our case)
>> >> All the above 3 are controlled inside drm, but pwm-backlight is
>> >> independent of drm. So, we let the pwm_config happen in pwm-backlight driver
>> >> and enable backlight GPIO(BL_EN) inside drm, after valid video data starts
>> >> coming out of the encoder.
>> >
>> > But that's completely the wrong way around. Why can't you simply control
>> > the backlight from within DRM and only enable it at the right time?
>> I am trying to reuse existing code as much as possible. pwm-backlight driver
>> takes care of generating proper pwm signals for the given backlight value,
>> so I am reusing that part of it. Even though it also provides a way to handle
>> "backlight enable" pin(which is ofcourse optional) I wish to control it in the
>> panel drvier, because generally all panel datasheets say you should control
>> backlight enable along with panel controls.
>
> I still don't see how controlling the enable GPIO from the panel will be
> in any way better, or different for that matter, from simply enabling
> the backlight and let the backlight driver handle the enable GPIO. Have
> you tried to do that and found that it doesn't work?
It works, but it gives me glitch when I try to configure video.
This is because backlight_on(pwm-backlight) happens much before
I configure video(inside drm), and while configuring video there would
be a glitch,
and that glitch would be visible to the user since backlight is already enabled.

On the other hand, if I choose to delay enabling backlight till I am sure enough
that video is properly configured, then even though the glitch comes up, the
user cannot make it out since backlight if off - a better user experience!

>> >> As you said, we can get this GPIO from a backlight device node, but since
>> >> this GPIO is related to panel also, I think conceptually its not wrong
>> >> to have this
>> >> GPIO binding defined in a panel node.
>> >
>> > It's not related to the panel. It's an enable for the backlight.
>> > Backlight could be considered part of the panel, therefore it makes
>> > sense to control the backlight from the panel driver.
>> Ok. This GPIO goes from my AP to the backlight unit of the panel.
>> Basically, a board related GPIO which exists on all the boards.
>> Where should I be handling this?
>
> In the driver that handles the backlight unit. That is: pwm-backlight.
>
>> >> > What are panel_VCC or video_enable?
>> >> It is the time taken for the panel to throw a valid HPD from the point
>> >> we enabled LCD_VCC.
>> >> After HPD is thrown, we can start sending video data.
>> >
>> > What does "throw a valid HPD" mean? Is it an actual signal that can be
>> > captured via software (GPIO, interrupt, ...)? If so then it would be
>> > preferable to not use a delay at all but rather rely on that mechanism
>> > to determine when it's safe to send a video signal.
>> Right, your explanation holds good, but only for the case of eDP panels.
>> But, when we use eDP-LVDS bridges(ps8622), its a different case!
>> the bridge takes care of sending HPD, so the encoder gets HPD interrupt
>> and tries to read edid from the panel, but the panel might not have powered up
>> enough to read edid. In such cases we would need delay.
>> So, having a delay field here would be really useful.
>
> Okay.
>
>> May be, while describing this particular DT binding I should elaborate more.
>
> Yes, something like the above explanation would be good to have in the
> binding.
Ok, will do that.

>> >> >> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
>> >> > [...]
>> >> >
>> >> > This duplicates much of the functionality that panels should provide. I
>> >> > think a better solution would be to properly integrate panels with
>> >> > bridges.
>> >> True, ideally I should be calling drm_panel functions from a simple dummy
>> >> bridge which sits at the end of the bridge chain. But, since you are not
>> >> convinced with having pre_enable and post_disable calls for panels, I had
>> >> no other way to do this, than stuffing these panel controls inside
>> >> bridge! :(
>> >
>> > What makes you think that I would suddenly be any more convinced by this
>> > solution than by your prior proposal? I didn't say outright no to what
>> > you proposed earlier. What I said was that I didn't like it and that I
>> > thought we could come up with a better solution. Part of getting to a
>> > better solution is trying to understand the problems involved. You don't
>> > solve a problem by simply moving code into a different driver.
>> Ok. What is better solution according to you?
>> Handling the backlight enable GPIO in pwm-backlight driver is not
>> a better soultion "ALWAYS". Sometimes, we may just need to control it
>> along with video encoders.
>
> You keep saying that, but you haven't said *why* you need to do that.
I have explained above. Also see chapter 6.5 in [1]

>> And, even the panel datasheets mention that  in "Power On/off sequence"
>> chapter.
>
> Can you point me to such a datasheet?
[1] : http://www.yslcd.com.tw/docs/product/B116XW03%20V.0.pdf
chapter 6.5

>> And, I have explained the problems and feasible solutions.
>
> You have explained the solutions that you've found to the problem. I'm
> trying to understand the problem fully so that perhaps we can find a
> more generic solution.
>
>> If drm_panel can support pre_enable/post_disable, we can easily
>> implement a generic bridge which sits at the end of bridge chain, and
>> calls corresponding drm_panel functions.
>
> The problem with blindly adding .pre_enable() and .post_disable()
> without thinking this through is that it can easily introduce
> incompatibilities between various drivers and panels. There is a risk
> that some of the code that goes into a panel's .pre_enable() will be
> specific to a given setup (and perhaps not even related to the panel
> at all). If that panel is reused on a different board where the
> restrictions may be different the panel may not work.
Ok, I understand. That's why I am pointing out to the LVDS datasheet above.
I have a similar datasheet for eDP panel B133HTN01, but its private and I am
not supposed to share it! :(

> So before we go and add any new functions to DRM panel I'd like to see
> them properly documented. It needs to be defined precisely what the
> effect of these functions should be so that both panel driver writers
> know what to implement and display driver writers need to know when to
> call which function.
Agreed, we should put in precise explanation in bindings/comments.

> Also, please let's not call them .pre_enable() and .post_disable(). The
> names should be something more specific to reflect what they are meant
> to do. Even something like .prepare() and .unprepare() would be better
> choices.
.prepare() and .unprepare() also sound good and meaningful.
powering up the LCD unit goes into prepare()
powering up the LED unit goes into enable()

Regards,
Ajay

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

[Ajay kumar]

ping.

On Thu, May 15, 2014 at 5:10 PM, Ajay kumar <ajaynumb@gmail.com> wrote:
> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Wed, May 14, 2014 at 11:39:30PM +0530, Ajay kumar wrote:
>> [...]
>>> >> >> diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt
>>> >> > [...]
>>> >> >> +     -led-en-gpio:
>>> >> >> +             eDP panel LED enable GPIO.
>>> >> >> +                     Indicates which GPIO needs to be powered up as output
>>> >> >> +                     to enable the backlight.
>>> >> >
>>> >> > Since this is used to control a backlight, then this should really be a
>>> >> > separate node to describe the backlight device (and use the
>>> >> > corresponding backlight driver) rather than duplicating a subset of that
>>> >> > functionality.
>>> >> I am stressing this point again!
>>> >> Backlight should ideally be enabled only after:
>>> >> 1) Panel is powered up (LCD_VDD)
>>> >> 2) HPD is thrown.
>>> >> 3) Valid data starts coming from the encoder(DP in our case)
>>> >> All the above 3 are controlled inside drm, but pwm-backlight is
>>> >> independent of drm. So, we let the pwm_config happen in pwm-backlight driver
>>> >> and enable backlight GPIO(BL_EN) inside drm, after valid video data starts
>>> >> coming out of the encoder.
>>> >
>>> > But that's completely the wrong way around. Why can't you simply control
>>> > the backlight from within DRM and only enable it at the right time?
>>> I am trying to reuse existing code as much as possible. pwm-backlight driver
>>> takes care of generating proper pwm signals for the given backlight value,
>>> so I am reusing that part of it. Even though it also provides a way to handle
>>> "backlight enable" pin(which is ofcourse optional) I wish to control it in the
>>> panel drvier, because generally all panel datasheets say you should control
>>> backlight enable along with panel controls.
>>
>> I still don't see how controlling the enable GPIO from the panel will be
>> in any way better, or different for that matter, from simply enabling
>> the backlight and let the backlight driver handle the enable GPIO. Have
>> you tried to do that and found that it doesn't work?
> It works, but it gives me glitch when I try to configure video.
> This is because backlight_on(pwm-backlight) happens much before
> I configure video(inside drm), and while configuring video there would
> be a glitch,
> and that glitch would be visible to the user since backlight is already enabled.
>
> On the other hand, if I choose to delay enabling backlight till I am sure enough
> that video is properly configured, then even though the glitch comes up, the
> user cannot make it out since backlight if off - a better user experience!
>
>>> >> As you said, we can get this GPIO from a backlight device node, but since
>>> >> this GPIO is related to panel also, I think conceptually its not wrong
>>> >> to have this
>>> >> GPIO binding defined in a panel node.
>>> >
>>> > It's not related to the panel. It's an enable for the backlight.
>>> > Backlight could be considered part of the panel, therefore it makes
>>> > sense to control the backlight from the panel driver.
>>> Ok. This GPIO goes from my AP to the backlight unit of the panel.
>>> Basically, a board related GPIO which exists on all the boards.
>>> Where should I be handling this?
>>
>> In the driver that handles the backlight unit. That is: pwm-backlight.
>>
>>> >> > What are panel_VCC or video_enable?
>>> >> It is the time taken for the panel to throw a valid HPD from the point
>>> >> we enabled LCD_VCC.
>>> >> After HPD is thrown, we can start sending video data.
>>> >
>>> > What does "throw a valid HPD" mean? Is it an actual signal that can be
>>> > captured via software (GPIO, interrupt, ...)? If so then it would be
>>> > preferable to not use a delay at all but rather rely on that mechanism
>>> > to determine when it's safe to send a video signal.
>>> Right, your explanation holds good, but only for the case of eDP panels.
>>> But, when we use eDP-LVDS bridges(ps8622), its a different case!
>>> the bridge takes care of sending HPD, so the encoder gets HPD interrupt
>>> and tries to read edid from the panel, but the panel might not have powered up
>>> enough to read edid. In such cases we would need delay.
>>> So, having a delay field here would be really useful.
>>
>> Okay.
>>
>>> May be, while describing this particular DT binding I should elaborate more.
>>
>> Yes, something like the above explanation would be good to have in the
>> binding.
> Ok, will do that.
>
>>> >> >> diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c
>>> >> > [...]
>>> >> >
>>> >> > This duplicates much of the functionality that panels should provide. I
>>> >> > think a better solution would be to properly integrate panels with
>>> >> > bridges.
>>> >> True, ideally I should be calling drm_panel functions from a simple dummy
>>> >> bridge which sits at the end of the bridge chain. But, since you are not
>>> >> convinced with having pre_enable and post_disable calls for panels, I had
>>> >> no other way to do this, than stuffing these panel controls inside
>>> >> bridge! :(
>>> >
>>> > What makes you think that I would suddenly be any more convinced by this
>>> > solution than by your prior proposal? I didn't say outright no to what
>>> > you proposed earlier. What I said was that I didn't like it and that I
>>> > thought we could come up with a better solution. Part of getting to a
>>> > better solution is trying to understand the problems involved. You don't
>>> > solve a problem by simply moving code into a different driver.
>>> Ok. What is better solution according to you?
>>> Handling the backlight enable GPIO in pwm-backlight driver is not
>>> a better soultion "ALWAYS". Sometimes, we may just need to control it
>>> along with video encoders.
>>
>> You keep saying that, but you haven't said *why* you need to do that.
> I have explained above. Also see chapter 6.5 in [1]
>
>>> And, even the panel datasheets mention that  in "Power On/off sequence"
>>> chapter.
>>
>> Can you point me to such a datasheet?
> [1] : http://www.yslcd.com.tw/docs/product/B116XW03%20V.0.pdf
> chapter 6.5
>
>>> And, I have explained the problems and feasible solutions.
>>
>> You have explained the solutions that you've found to the problem. I'm
>> trying to understand the problem fully so that perhaps we can find a
>> more generic solution.
>>
>>> If drm_panel can support pre_enable/post_disable, we can easily
>>> implement a generic bridge which sits at the end of bridge chain, and
>>> calls corresponding drm_panel functions.
>>
>> The problem with blindly adding .pre_enable() and .post_disable()
>> without thinking this through is that it can easily introduce
>> incompatibilities between various drivers and panels. There is a risk
>> that some of the code that goes into a panel's .pre_enable() will be
>> specific to a given setup (and perhaps not even related to the panel
>> at all). If that panel is reused on a different board where the
>> restrictions may be different the panel may not work.
> Ok, I understand. That's why I am pointing out to the LVDS datasheet above.
> I have a similar datasheet for eDP panel B133HTN01, but its private and I am
> not supposed to share it! :(
>
>> So before we go and add any new functions to DRM panel I'd like to see
>> them properly documented. It needs to be defined precisely what the
>> effect of these functions should be so that both panel driver writers
>> know what to implement and display driver writers need to know when to
>> call which function.
> Agreed, we should put in precise explanation in bindings/comments.
>
>> Also, please let's not call them .pre_enable() and .post_disable(). The
>> names should be something more specific to reflect what they are meant
>> to do. Even something like .prepare() and .unprepare() would be better
>> choices.
> .prepare() and .unprepare() also sound good and meaningful.
> powering up the LCD unit goes into prepare()
> powering up the LED unit goes into enable()
>
> Regards,
> Ajay

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

[Thierry Reding]

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

On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote:
> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > I still don't see how controlling the enable GPIO from the panel will be
> > in any way better, or different for that matter, from simply enabling
> > the backlight and let the backlight driver handle the enable GPIO. Have
> > you tried to do that and found that it doesn't work?
> It works, but it gives me glitch when I try to configure video.
> This is because backlight_on(pwm-backlight) happens much before
> I configure video(inside drm), and while configuring video there would
> be a glitch,
> and that glitch would be visible to the user since backlight is already enabled.

Okay, so this means that your backlight is turned on too early. Instead
of working around that problem by moving control of the backlight enable
GPIO from the backlight driver into the panel driver, the correct way to
fix it is to make sure the backlight stays off until video is ready.

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 V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay kumar]

On Sun, May 18, 2014 at 2:44 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote:
>> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> [...]
>> > I still don't see how controlling the enable GPIO from the panel will be
>> > in any way better, or different for that matter, from simply enabling
>> > the backlight and let the backlight driver handle the enable GPIO. Have
>> > you tried to do that and found that it doesn't work?
>> It works, but it gives me glitch when I try to configure video.
>> This is because backlight_on(pwm-backlight) happens much before
>> I configure video(inside drm), and while configuring video there would
>> be a glitch,
>> and that glitch would be visible to the user since backlight is already enabled.
>
> Okay, so this means that your backlight is turned on too early. Instead
> of working around that problem by moving control of the backlight enable
> GPIO from the backlight driver into the panel driver, the correct way to
> fix it is to make sure the backlight stays off until video is ready.
Ok. Please suggest an idea how to do the same!

I have already suggested a simple idea which conforms to a valid spec.
Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel
like we are forcing people to handle enable_gpio inside pwm_bl.c.
Note that, pwm_bl can still work properly without enabling the backlight GPIO.
And, I did point out to a valid datasheet from AUO, which clearly indicates why
backlight enable GPIO should be a part of panel driver and not pwm_bl driver.
I am not trying to say we should remove enable_gpio from pwm_bl.
Provided that its already an optional property, and if someone wants
to control it in a panel driver, what is wrong in doing so?

Regards,
Ajay

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

[Thierry Reding]

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

On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote:
> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote:
> >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > [...]
> >> > I still don't see how controlling the enable GPIO from the panel will be
> >> > in any way better, or different for that matter, from simply enabling
> >> > the backlight and let the backlight driver handle the enable GPIO. Have
> >> > you tried to do that and found that it doesn't work?
> >> It works, but it gives me glitch when I try to configure video.
> >> This is because backlight_on(pwm-backlight) happens much before
> >> I configure video(inside drm), and while configuring video there would
> >> be a glitch,
> >> and that glitch would be visible to the user since backlight is already enabled.
> >
> > Okay, so this means that your backlight is turned on too early. Instead
> > of working around that problem by moving control of the backlight enable
> > GPIO from the backlight driver into the panel driver, the correct way to
> > fix it is to make sure the backlight stays off until video is ready.
> Ok. Please suggest an idea how to do the same!

I did post a patch[0] a long time ago that added a way to fix this for
pwm-backlight at least.

> I have already suggested a simple idea which conforms to a valid spec.
> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel
> like we are forcing people to handle enable_gpio inside pwm_bl.c.

And that's a good thing. That's what people will expect. Backlights are
exposed via sysfs, which is currently also the only way to control the
backlight from userspace. If the driver for that doesn't have everything
required to control the backlight how can userspace control it?

> Note that, pwm_bl can still work properly without enabling the backlight GPIO.
> And, I did point out to a valid datasheet from AUO, which clearly indicates why
> backlight enable GPIO should be a part of panel driver and not pwm_bl driver.

Just because some spec mentions the backlight enable pin in some panel
power up sequence diagram that doesn't mean we have to implement it as
part of the panel driver.

> I am not trying to say we should remove enable_gpio from pwm_bl.
> Provided that its already an optional property, and if someone wants
> to control it in a panel driver, what is wrong in doing so?

See above.

Thierry

[0]: https://lkml.org/lkml/2013/10/7/188

[-- 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 V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay kumar]

On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote:
>> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote:
>> >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding
>> >> <thierry.reding@gmail.com> wrote:
>> > [...]
>> >> > I still don't see how controlling the enable GPIO from the panel will
>> >> > be
>> >> > in any way better, or different for that matter, from simply
>> >> > enabling
>> >> > the backlight and let the backlight driver handle the enable GPIO.
>> >> > Have
>> >> > you tried to do that and found that it doesn't work?
>> >> It works, but it gives me glitch when I try to configure video.
>> >> This is because backlight_on(pwm-backlight) happens much before
>> >> I configure video(inside drm), and while configuring video there would
>> >> be a glitch,
>> >> and that glitch would be visible to the user since backlight is already
>> >> enabled.
>> >
>> > Okay, so this means that your backlight is turned on too early. Instead
>> > of working around that problem by moving control of the backlight
>> > enable
>> > GPIO from the backlight driver into the panel driver, the correct way
>> > to
>> > fix it is to make sure the backlight stays off until video is ready.
>> Ok. Please suggest an idea how to do the same!
>
> I did post a patch[0] a long time ago that added a way to fix this for
> pwm-backlight at least.
I have tried this. We have to wait till the userspace to switch the
backlight on again :(

>> I have already suggested a simple idea which conforms to a valid spec.
>> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel
>> like we are forcing people to handle enable_gpio inside pwm_bl.c.
>
> And that's a good thing. That's what people will expect. Backlights are
> exposed via sysfs, which is currently also the only way to control the
> backlight from userspace. If the driver for that doesn't have everything
> required to control the backlight how can userspace control it?
This is a valid point, only at the hardware level.
But if you consider a user experience perspective,
user still will not be able to see the backlight even
though enable_gpio is controlled elsewhere, since
pwm is disabled anyhow.
Now lets talk about backlight_on case. Even though user tries to turn
on the backlight, and the driver turns on backlight supply, pwm and
enable_gpio, the driver cannot always guarantee him that backlight is
"really on" since it also depends on panel power.
Such ambiguities exist for few panels. And, for such panels why can't
we keep enable_gpio in another driver, and let the pwm_bl control only
backlight levels?

>> Note that, pwm_bl can still work properly without enabling the backlight
>> GPIO.
>> And, I did point out to a valid datasheet from AUO, which clearly
>> indicates why
>> backlight enable GPIO should be a part of panel driver and not pwm_bl
>> driver.
>
> Just because some spec mentions the backlight enable pin in some panel
> power up sequence diagram that doesn't mean we have to implement it as
> part of the panel driver.
That is not "some spec". I believe all the AUO panels share the same sequence!

>> I am not trying to say we should remove enable_gpio from pwm_bl.
>> Provided that its already an optional property, and if someone wants
>> to control it in a panel driver, what is wrong in doing so?
>
> See above.
>
> Thierry
>
> [0]: https://lkml.org/lkml/2013/10/7/188
>

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

[Thierry Reding]

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

On Tue, May 20, 2014 at 12:37:38AM +0530, Ajay kumar wrote:
> On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote:
> >> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >> > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote:
> >> >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding
> >> >> <thierry.reding@gmail.com> wrote:
> >> > [...]
> >> >> > I still don't see how controlling the enable GPIO from the panel will
> >> >> > be
> >> >> > in any way better, or different for that matter, from simply
> >> >> > enabling
> >> >> > the backlight and let the backlight driver handle the enable GPIO.
> >> >> > Have
> >> >> > you tried to do that and found that it doesn't work?
> >> >> It works, but it gives me glitch when I try to configure video.
> >> >> This is because backlight_on(pwm-backlight) happens much before
> >> >> I configure video(inside drm), and while configuring video there would
> >> >> be a glitch,
> >> >> and that glitch would be visible to the user since backlight is already
> >> >> enabled.
> >> >
> >> > Okay, so this means that your backlight is turned on too early. Instead
> >> > of working around that problem by moving control of the backlight
> >> > enable
> >> > GPIO from the backlight driver into the panel driver, the correct way
> >> > to
> >> > fix it is to make sure the backlight stays off until video is ready.
> >> Ok. Please suggest an idea how to do the same!
> >
> > I did post a patch[0] a long time ago that added a way to fix this for
> > pwm-backlight at least.
> I have tried this. We have to wait till the userspace to switch the
> backlight on again :(

Erm... why?

> >> I have already suggested a simple idea which conforms to a valid spec.
> >> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel
> >> like we are forcing people to handle enable_gpio inside pwm_bl.c.
> >
> > And that's a good thing. That's what people will expect. Backlights are
> > exposed via sysfs, which is currently also the only way to control the
> > backlight from userspace. If the driver for that doesn't have everything
> > required to control the backlight how can userspace control it?
> This is a valid point, only at the hardware level.
> But if you consider a user experience perspective,
> user still will not be able to see the backlight even
> though enable_gpio is controlled elsewhere, since
> pwm is disabled anyhow.

But that's precisely my point. If the enable_gpio is controlled
elsewhere there will be no way for the userspace interface to enable the
backlight.

> Now lets talk about backlight_on case. Even though user tries to turn
> on the backlight, and the driver turns on backlight supply, pwm and
> enable_gpio, the driver cannot always guarantee him that backlight is
> "really on" since it also depends on panel power.

No. If the backlight is properly hooked up to all resources, then
turning it on will *really turn it on*.

> >> Note that, pwm_bl can still work properly without enabling the backlight
> >> GPIO.
> >> And, I did point out to a valid datasheet from AUO, which clearly
> >> indicates why
> >> backlight enable GPIO should be a part of panel driver and not pwm_bl
> >> driver.
> >
> > Just because some spec mentions the backlight enable pin in some panel
> > power up sequence diagram that doesn't mean we have to implement it as
> > part of the panel driver.
> That is not "some spec". I believe all the AUO panels share the same
> sequence!

Just because some specs mention the backlight enable pin in some panel
power up sequence diagram that doesn't mean we have to implement it as
part of the panel driver.

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 V3 2/3] drm/bridge: add a dummy panel driver to support lvds bridges

[Ajay kumar]

On Tue, May 20, 2014 at 1:40 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, May 20, 2014 at 12:37:38AM +0530, Ajay kumar wrote:
>> On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote:
>> >> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding
>> >> <thierry.reding@gmail.com> wrote:
>> >> > On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote:
>> >> >> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding
>> >> >> <thierry.reding@gmail.com> wrote:
>> >> > [...]
>> >> >> > I still don't see how controlling the enable GPIO from the panel will
>> >> >> > be
>> >> >> > in any way better, or different for that matter, from simply
>> >> >> > enabling
>> >> >> > the backlight and let the backlight driver handle the enable GPIO.
>> >> >> > Have
>> >> >> > you tried to do that and found that it doesn't work?
>> >> >> It works, but it gives me glitch when I try to configure video.
>> >> >> This is because backlight_on(pwm-backlight) happens much before
>> >> >> I configure video(inside drm), and while configuring video there would
>> >> >> be a glitch,
>> >> >> and that glitch would be visible to the user since backlight is already
>> >> >> enabled.
>> >> >
>> >> > Okay, so this means that your backlight is turned on too early. Instead
>> >> > of working around that problem by moving control of the backlight
>> >> > enable
>> >> > GPIO from the backlight driver into the panel driver, the correct way
>> >> > to
>> >> > fix it is to make sure the backlight stays off until video is ready.
>> >> Ok. Please suggest an idea how to do the same!
>> >
>> > I did post a patch[0] a long time ago that added a way to fix this for
>> > pwm-backlight at least.
>> I have tried this. We have to wait till the userspace to switch the
>> backlight on again :(
>
> Erm... why?
Because, backlight remains in powerdown state till the userspace startup scripts
turn it back on!

>> >> I have already suggested a simple idea which conforms to a valid spec.
>> >> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel
>> >> like we are forcing people to handle enable_gpio inside pwm_bl.c.
>> >
>> > And that's a good thing. That's what people will expect. Backlights are
>> > exposed via sysfs, which is currently also the only way to control the
>> > backlight from userspace. If the driver for that doesn't have everything
>> > required to control the backlight how can userspace control it?
>> This is a valid point, only at the hardware level.
>> But if you consider a user experience perspective,
>> user still will not be able to see the backlight even
>> though enable_gpio is controlled elsewhere, since
>> pwm is disabled anyhow.
>
> But that's precisely my point. If the enable_gpio is controlled
> elsewhere there will be no way for the userspace interface to enable the
> backlight.
Hmm, this is a valid point. Still, see my explanation below.[1]

>> Now lets talk about backlight_on case. Even though user tries to turn
>> on the backlight, and the driver turns on backlight supply, pwm and
>> enable_gpio, the driver cannot always guarantee him that backlight is
>> "really on" since it also depends on panel power.
>
> No. If the backlight is properly hooked up to all resources, then
> turning it on will *really turn it on*.
[1] : This is what I am trying to elaborate.
Backlight really does depend on panel power also, and
you cannot tie LCD resources to backlight driver.
Now, consider that panel/LCD is off and user wishes to control backlight,
it will have no effect.
So, ideally backlight drivers are not "completely independent" in generating
backlight in most of the cases.
LCD driver and backlight driver should both co ordinate at user/kernel level
to make things work properly.

Lets consider this also as a similar case - "backlight enable being handled in
panel driver."
Here also, things will work if LCD and backlight drivers work in tandem,
at user space.

>> >> Note that, pwm_bl can still work properly without enabling the backlight
>> >> GPIO.
>> >> And, I did point out to a valid datasheet from AUO, which clearly
>> >> indicates why
>> >> backlight enable GPIO should be a part of panel driver and not pwm_bl
>> >> driver.
>> >
>> > Just because some spec mentions the backlight enable pin in some panel
>> > power up sequence diagram that doesn't mean we have to implement it as
>> > part of the panel driver.
>> That is not "some spec". I believe all the AUO panels share the same
>> sequence!
>
> Just because some specs mention the backlight enable pin in some panel
> power up sequence diagram that doesn't mean we have to implement it as
> part of the panel driver.
> Thierry

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

[Andrzej Hajda]

On 06/03/2014 11:58 AM, Ajay kumar wrote:
> On Tue, May 20, 2014 at 1:40 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Tue, May 20, 2014 at 12:37:38AM +0530, Ajay kumar wrote:
>>> On 5/19/14, Thierry Reding <thierry.reding@gmail.com> wrote:
>>>> On Sun, May 18, 2014 at 01:50:36PM +0530, Ajay kumar wrote:
>>>>> On Sun, May 18, 2014 at 2:44 AM, Thierry Reding
>>>>> <thierry.reding@gmail.com> wrote:
>>>>>> On Thu, May 15, 2014 at 05:10:16PM +0530, Ajay kumar wrote:
>>>>>>> On Thu, May 15, 2014 at 1:43 PM, Thierry Reding
>>>>>>> <thierry.reding@gmail.com> wrote:
>>>>>> [...]
>>>>>>>> I still don't see how controlling the enable GPIO from the panel will
>>>>>>>> be
>>>>>>>> in any way better, or different for that matter, from simply
>>>>>>>> enabling
>>>>>>>> the backlight and let the backlight driver handle the enable GPIO.
>>>>>>>> Have
>>>>>>>> you tried to do that and found that it doesn't work?
>>>>>>> It works, but it gives me glitch when I try to configure video.
>>>>>>> This is because backlight_on(pwm-backlight) happens much before
>>>>>>> I configure video(inside drm), and while configuring video there would
>>>>>>> be a glitch,
>>>>>>> and that glitch would be visible to the user since backlight is already
>>>>>>> enabled.
>>>>>> Okay, so this means that your backlight is turned on too early. Instead
>>>>>> of working around that problem by moving control of the backlight
>>>>>> enable
>>>>>> GPIO from the backlight driver into the panel driver, the correct way
>>>>>> to
>>>>>> fix it is to make sure the backlight stays off until video is ready.
>>>>> Ok. Please suggest an idea how to do the same!
>>>> I did post a patch[0] a long time ago that added a way to fix this for
>>>> pwm-backlight at least.
>>> I have tried this. We have to wait till the userspace to switch the
>>> backlight on again :(
>> Erm... why?
> Because, backlight remains in powerdown state till the userspace startup scripts
> turn it back on!
>
>>>>> I have already suggested a simple idea which conforms to a valid spec.
>>>>> Just because enable_gpio is already a part of pwm_bl.c, I somewhat feel
>>>>> like we are forcing people to handle enable_gpio inside pwm_bl.c.
>>>> And that's a good thing. That's what people will expect. Backlights are
>>>> exposed via sysfs, which is currently also the only way to control the
>>>> backlight from userspace. If the driver for that doesn't have everything
>>>> required to control the backlight how can userspace control it?
>>> This is a valid point, only at the hardware level.
>>> But if you consider a user experience perspective,
>>> user still will not be able to see the backlight even
>>> though enable_gpio is controlled elsewhere, since
>>> pwm is disabled anyhow.
>> But that's precisely my point. If the enable_gpio is controlled
>> elsewhere there will be no way for the userspace interface to enable the
>> backlight.
> Hmm, this is a valid point. Still, see my explanation below.[1]
>
>>> Now lets talk about backlight_on case. Even though user tries to turn
>>> on the backlight, and the driver turns on backlight supply, pwm and
>>> enable_gpio, the driver cannot always guarantee him that backlight is
>>> "really on" since it also depends on panel power.
>> No. If the backlight is properly hooked up to all resources, then
>> turning it on will *really turn it on*.
> [1] : This is what I am trying to elaborate.
> Backlight really does depend on panel power also, and
> you cannot tie LCD resources to backlight driver.
> Now, consider that panel/LCD is off and user wishes to control backlight,
> it will have no effect.
> So, ideally backlight drivers are not "completely independent" in generating
> backlight in most of the cases.
> LCD driver and backlight driver should both co ordinate at user/kernel level
> to make things work properly.
>
> Lets consider this also as a similar case - "backlight enable being handled in
> panel driver."
> Here also, things will work if LCD and backlight drivers work in tandem,
> at user space.
>
>>>>> Note that, pwm_bl can still work properly without enabling the backlight
>>>>> GPIO.
>>>>> And, I did point out to a valid datasheet from AUO, which clearly
>>>>> indicates why
>>>>> backlight enable GPIO should be a part of panel driver and not pwm_bl
>>>>> driver.
>>>> Just because some spec mentions the backlight enable pin in some panel
>>>> power up sequence diagram that doesn't mean we have to implement it as
>>>> part of the panel driver.
>>> That is not "some spec". I believe all the AUO panels share the same
>>> sequence!
>> Just because some specs mention the backlight enable pin in some panel
>> power up sequence diagram that doesn't mean we have to implement it as
>> part of the panel driver.
>> Thierry

My two cents. I have took some random panel specs with separate backlight:
- LD070WX3 - implemented in Linux as simple panel [1],
- LP129QE1 - again implemented as simple panel [2].
Both panels requires that backlight should be turned on after 200ms of
valid video data and
turned off 200ms before end of video data.

[1]: http://www.displayalliance.com/storage/1-spec-sheets/LD070WX3-SL01.pdf
[2]:
http://www.revointeractive.com/prodimages/LCD-Display-Panel/LG-12.9-LP129QE1-SPA1-LVDS-2560-1600-400-NITS.pdf

If I understand correctly currently there is no communication between
video data provider (encoder) and backlight
device driver, so it is possible to violate panel specifications. I
guess usually it is harmless but I think it would be good
thing to solve it somehow.

IMHO it could be done this way:
1. Panel should be notified about starting of video data from its video
source, for example by some drm_panel callback.
2. Panel should have possibility to activate/deactivate backlight
device, as I understand there is no such mechanism at the moment,
unless we try to play with backlight registration/unregistration.

Any opinions?

Regards
Andrzej

[RFC V3 3/3] drm: create and demonstrate bridge chaining using exynos_dp

[Ajay Kumar]

use drm_bridge helpers to demonstrate chaining of bridges(ptn3460 and
bridge_panel) with exynos_dp encoder.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/bridge/ptn3460.c        |   60 +++++++++++++++++++++++--------
 drivers/gpu/drm/exynos/exynos_dp_core.c |   25 ++++++++-----
 include/drm/bridge/ptn3460.h            |   15 ++++----
 3 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
index b171901..31ab8b3 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);
 	}
 
+	drm_next_bridge_pre_enable(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,7 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 
 static void ptn3460_enable(struct drm_bridge *bridge)
 {
+	drm_next_bridge_enable(bridge);
 }
 
 static void ptn3460_disable(struct drm_bridge *bridge)
@@ -153,6 +156,8 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 
 	ptn_bridge->enabled = false;
 
+	drm_next_bridge_disable(bridge);
+
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
 
@@ -162,6 +167,7 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 
 static void ptn3460_post_disable(struct drm_bridge *bridge)
 {
+	drm_next_bridge_post_disable(bridge);
 }
 
 void ptn3460_bridge_destroy(struct drm_bridge *bridge)
@@ -173,6 +179,9 @@ void ptn3460_bridge_destroy(struct drm_bridge *bridge)
 		gpio_free(ptn_bridge->gpio_pd_n);
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_free(ptn_bridge->gpio_rst_n);
+
+	drm_next_bridge_destroy(bridge);
+
 	/* Nothing else to free, we've got devm allocated memory */
 }
 
@@ -189,15 +198,28 @@ int ptn3460_get_modes(struct drm_connector *connector)
 	struct ptn3460_bridge *ptn_bridge;
 	u8 *edid;
 	int ret, num_modes;
-	bool power_off;
 
 	ptn_bridge = container_of(connector, struct ptn3460_bridge, connector);
 
 	if (ptn_bridge->edid)
 		return drm_add_edid_modes(connector, ptn_bridge->edid);
 
-	power_off = !ptn_bridge->enabled;
-	ptn3460_pre_enable(ptn_bridge->bridge);
+	if (!ptn_bridge->enabled) {
+		if (gpio_is_valid(ptn_bridge->gpio_pd_n))
+			gpio_set_value(ptn_bridge->gpio_pd_n, 1);
+
+		if (gpio_is_valid(ptn_bridge->gpio_rst_n)) {
+			gpio_set_value(ptn_bridge->gpio_rst_n, 0);
+			udelay(10);
+			gpio_set_value(ptn_bridge->gpio_rst_n, 1);
+		}
+
+		msleep(90);
+
+		ret = ptn3460_select_edid(ptn_bridge);
+		if (ret)
+			DRM_ERROR("Select edid failed ret=%d\n", ret);
+	}
 
 	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
 	if (!edid) {
@@ -219,8 +241,13 @@ int ptn3460_get_modes(struct drm_connector *connector)
 	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
 
 out:
-	if (power_off)
-		ptn3460_disable(ptn_bridge->bridge);
+	if (!ptn_bridge->enabled) {
+		if (gpio_is_valid(ptn_bridge->gpio_rst_n))
+			gpio_set_value(ptn_bridge->gpio_rst_n, 1);
+
+		if (gpio_is_valid(ptn_bridge->gpio_pd_n))
+			gpio_set_value(ptn_bridge->gpio_pd_n, 0);
+	}
 
 	return num_modes;
 }
@@ -264,8 +291,10 @@ struct drm_connector_funcs ptn3460_connector_funcs = {
 	.destroy = ptn3460_connector_destroy,
 };
 
-int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
-		struct i2c_client *client, struct device_node *node)
+struct drm_bridge *ptn3460_init(struct drm_device *dev,
+				struct drm_encoder *encoder,
+				struct i2c_client *client,
+				struct device_node *node)
 {
 	int ret;
 	struct drm_bridge *bridge;
@@ -274,13 +303,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 	bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL);
 	if (!bridge) {
 		DRM_ERROR("Failed to allocate drm bridge\n");
-		return -ENOMEM;
+		return NULL;
 	}
 
 	ptn_bridge = devm_kzalloc(dev->dev, sizeof(*ptn_bridge), GFP_KERNEL);
 	if (!ptn_bridge) {
 		DRM_ERROR("Failed to allocate ptn bridge\n");
-		return -ENOMEM;
+		return NULL;
 	}
 
 	ptn_bridge->client = client;
@@ -292,7 +321,7 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 				GPIOF_OUT_INIT_HIGH, "PTN3460_PD_N");
 		if (ret) {
 			DRM_ERROR("Request powerdown-gpio failed (%d)\n", ret);
-			return ret;
+			return NULL;
 		}
 	}
 
@@ -307,7 +336,7 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 		if (ret) {
 			DRM_ERROR("Request reset-gpio failed (%d)\n", ret);
 			gpio_free(ptn_bridge->gpio_pd_n);
-			return ret;
+			return NULL;
 		}
 	}
 
@@ -325,7 +354,10 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 	}
 
 	bridge->driver_private = ptn_bridge;
-	encoder->bridge = bridge;
+
+	if (!encoder->bridge)
+		/* First entry in the bridge chain */
+		encoder->bridge = bridge;
 
 	ret = drm_connector_init(dev, &ptn_bridge->connector,
 			&ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
@@ -338,13 +370,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 	drm_sysfs_connector_add(&ptn_bridge->connector);
 	drm_mode_connector_attach_encoder(&ptn_bridge->connector, encoder);
 
-	return 0;
+	return bridge;
 
 err:
 	if (gpio_is_valid(ptn_bridge->gpio_pd_n))
 		gpio_free(ptn_bridge->gpio_pd_n);
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_free(ptn_bridge->gpio_rst_n);
-	return ret;
+	return NULL;
 }
 EXPORT_SYMBOL(ptn3460_init);
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 1cc3981..2cc31e1 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -27,6 +27,7 @@
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/bridge/ptn3460.h>
+#include <drm/bridge/bridge_panel.h>
 
 #include "exynos_drm_drv.h"
 #include "exynos_dp_core.h"
@@ -979,24 +980,32 @@ static bool find_bridge(const char *compat, struct bridge_init *bridge)
 
 	bridge->client = of_find_i2c_device_by_node(bridge->node);
 	if (!bridge->client)
-		return false;
+		DRM_INFO("bridge is not an i2c device\n");
 
 	return true;
 }
 
-/* returns the number of bridges attached */
-static int exynos_drm_attach_lcd_bridge(struct drm_device *dev,
+static bool exynos_drm_attach_lcd_bridge(struct drm_device *dev,
 		struct drm_encoder *encoder)
 {
 	struct bridge_init bridge;
-	int ret;
+	struct drm_bridge *bridge_chain = NULL, *next = NULL;
+	bool connector_created = false;
 
 	if (find_bridge("nxp,ptn3460", &bridge)) {
-		ret = ptn3460_init(dev, encoder, bridge.client, bridge.node);
-		if (!ret)
-			return 1;
+		bridge_chain = ptn3460_init(dev, encoder, bridge.client,
+								bridge.node);
+		if (bridge_chain)
+			connector_created = true;
 	}
-	return 0;
+
+	if (find_bridge("drm-bridge,panel", &bridge)) {
+		next = bridge_panel_init(dev, encoder, bridge.client,
+							bridge.node);
+		drm_bridge_add_to_chain(bridge_chain, next);
+	}
+
+	return connector_created;
 }
 
 static int exynos_dp_create_connector(struct exynos_drm_display *display,
diff --git a/include/drm/bridge/ptn3460.h b/include/drm/bridge/ptn3460.h
index ff62344..f612b9b 100644
--- a/include/drm/bridge/ptn3460.h
+++ b/include/drm/bridge/ptn3460.h
@@ -21,15 +21,18 @@ struct device_node;
 
 #if defined(CONFIG_DRM_PTN3460) || defined(CONFIG_DRM_PTN3460_MODULE)
 
-int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
-		struct i2c_client *client, struct device_node *node);
+struct drm_bridge *ptn3460_init(struct drm_device *dev,
+				struct drm_encoder *encoder,
+				struct i2c_client *client,
+				struct device_node *node);
 #else
 
-static inline int ptn3460_init(struct drm_device *dev,
-		struct drm_encoder *encoder, struct i2c_client *client,
-		struct device_node *node)
+static inline struct drm_bridge *ptn3460_init(struct drm_device *dev,
+						struct drm_encoder *encoder,
+						struct i2c_client *client,
+						struct device_node *node)
 {
-	return 0;
+	return NULL;
 }
 
 #endif
-- 
1.7.9.5