Linux Framebuffer Layer development
 help / color / mirror / Atom feed

* [RFC 06/11] OMAPDSS: APPLY: configure channel_in for writeback
From: Archit Taneja @ 2012-11-07 14:56 UTC (permalink / raw)
  To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1352299469-17609-1-git-send-email-archit@ti.com>

writeback's input is configured by the CHANNELIN field in DISPC_WB_ATTRIBUTES.
It's an immediate write register field. We need to change writeback's channel
in whenever the manager to which it is connected changes.

When changing managers for overlays, we needed to be certain that the overlay
was disabled. We added some extra waits for that. However, such a thing isn't
required for writeback as it's disabled for sure after we call dss_wb_disable.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c     |   31 +++++++++++++++++++++++++++++++
 drivers/video/omap2/dss/dss.h       |    9 +++++++++
 drivers/video/omap2/dss/writeback.c |   23 +++++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 1ab1755..a205ed4 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -1175,6 +1175,22 @@ int dss_mgr_set_output(struct omap_overlay_manager *mgr,
 		goto err;
 	}
 
+	if (output_is_wb(output)) {
+		enum dss_writeback_channel channel;
+
+		r = dss_writeback_calc_channel_in(mgr, &channel);
+		if (r)
+			goto err;
+
+		r = dispc_runtime_get();
+		if (r)
+			goto err;
+
+		dispc_wb_set_channel_in(channel);
+
+		dispc_runtime_put();
+	}
+
 	output->manager = mgr;
 	mgr->output = output;
 
@@ -1210,6 +1226,21 @@ int dss_mgr_unset_output(struct omap_overlay_manager *mgr)
 
 	spin_unlock_irqrestore(&data_lock, flags);
 
+	if (output_is_wb(mgr->output)) {
+		struct omap_dss_output *wb = mgr->output;
+		struct wb_priv_data *wp = get_wb_priv(wb);
+
+		spin_lock_irqsave(&data_lock, flags);
+
+		if (wp->enabled) {
+			DSSERR("WB output can't be unset when enabled\n");
+			r = -EINVAL;
+			goto err1;
+		}
+
+		spin_unlock_irqrestore(&data_lock, flags);
+	}
+
 	mgr->output->manager = NULL;
 	mgr->output = NULL;
 
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 6238895..f9e7074 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -540,6 +540,15 @@ int hdmi_audio_config(struct omap_dss_audio *audio);
 #ifdef CONFIG_OMAP4_DSS_WRITEBACK
 int writeback_init_platform_driver(void) __init;
 void writeback_uninit_platform_driver(void) __exit;
+int dss_writeback_calc_channel_in(struct omap_overlay_manager *mgr,
+		enum dss_writeback_channel *channel);
+#else
+static inline int dss_writeback_calc_channel_in(struct omap_overlay_manager *mgr,
+		enum dss_writeback_channel *channel)
+{
+	WARN("%s: WB not compiled in, returning 0\n", __func__);
+	return 0;
+}
 #endif
 
 /* RFBI */
diff --git a/drivers/video/omap2/dss/writeback.c b/drivers/video/omap2/dss/writeback.c
index 598defd..09394fb 100644
--- a/drivers/video/omap2/dss/writeback.c
+++ b/drivers/video/omap2/dss/writeback.c
@@ -48,6 +48,29 @@ static inline struct platform_device *writeback_get_wbdev_from_output(struct oma
 	return out->pdev;
 }
 
+int dss_writeback_calc_channel_in(struct omap_overlay_manager *mgr,
+		enum dss_writeback_channel *channel)
+{
+	/*
+	 * Add a case for dummy writeback manager once we support connecting
+	 * writeback directly to an overlay.
+	 */
+	switch (mgr->id) {
+	case OMAP_DSS_CHANNEL_DIGIT:
+		*channel = DSS_WB_TV_MGR;
+		break;
+	case OMAP_DSS_CHANNEL_LCD2:
+		*channel = DSS_WB_LCD2_MGR;
+		break;
+	case OMAP_DSS_CHANNEL_LCD:
+	default:
+		*channel = DSS_WB_LCD1_MGR;
+		break;
+	}
+
+	return 0;
+}
+
 void omapdss_writeback_set_input_size(struct omap_dss_output *wb, u16 w, u16 h)
 {
 	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
-- 
1.7.9.5


^ permalink raw reply related

* [RFC 07/11] OMAPDSS: writeback: add mechanism to do mem to mem updates
From: Archit Taneja @ 2012-11-07 14:56 UTC (permalink / raw)
  To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1352299469-17609-1-git-send-email-archit@ti.com>

writeback in mem to mem mode works like an overlay manager connected to a
display in stallmode. When we set ENABLE in DISPC_WB_ATTRIBUTES writeback, it
starts a mem to mem transfer. On completion, we get a FRAMEDONEWB interrupt and
the ENABLE bit is cleared by HW.

In APPLY, we add dss_wb_start_update, this function is similar to
dss_mgr_start_update, but is responsible for configuring both the manager(and
the overlays connected to it) and the writeback registers. We add an updating
field in the writeback private data which tells APPLY when we are busy and
when we are done with a mem to mem transfer. We register to the framedone isr
when needed and we update the updating field in dss_apply_irq_handler when we
are done with the transfer.

In the writeback output driver, we add a semaphore based way to lock the
writeback resource as we do in DSI command mode. The writeback user is expected
to lock the bus, call omapdss_writeback_update and mention a callback along with
it, and unlock the bus in the callback. omapdss_writeback_update registers to
the FRAMEDONE interrupt to trigger the callback and starts the update by calling
the corresponding APPLY function.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c     |   83 +++++++++++++++++++++++++++
 drivers/video/omap2/dss/dss.h       |    1 +
 drivers/video/omap2/dss/writeback.c |  106 +++++++++++++++++++++++++++++++++++
 include/video/omapdss.h             |    4 ++
 4 files changed, 194 insertions(+)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index a205ed4..2fd08d8 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -114,6 +114,9 @@ struct wb_priv_data {
 	bool info_dirty;
 	struct omap_dss_writeback_info info;
 
+	/* If true, writeback output is enabled */
+	bool updating;
+
 	/*
 	 * If true in memory to memory mode, a manager is connected to it.
 	 * However, it may not be active.
@@ -285,6 +288,7 @@ static int dss_check_settings_apply(struct omap_overlay_manager *mgr)
 static bool need_isr(void)
 {
 	const int num_mgrs = dss_feat_get_num_mgrs();
+	const int num_wbs = dss_feat_get_num_wbs();
 	int i;
 
 	for (i = 0; i < num_mgrs; ++i) {
@@ -362,6 +366,20 @@ static bool need_isr(void)
 		}
 	}
 
+	for (i = 0; i < num_wbs; i++) {
+		struct omap_dss_output *wb = omap_dss_get_writeback();
+		struct wb_priv_data *wp = get_wb_priv(wb);
+
+		if (!wp->enabled)
+			continue;
+
+		if (wb_manual_update(wb)) {
+			/* to catch FRAMEDONEWB */
+			if (wp->updating)
+				return true;
+		}
+	}
+
 	return false;
 }
 
@@ -841,6 +859,7 @@ static void dss_apply_irq_handler(void *data, u32 mask);
 static void dss_register_vsync_isr(void)
 {
 	const int num_mgrs = dss_feat_get_num_mgrs();
+	const int num_wbs = dss_feat_get_num_wbs();
 	u32 mask;
 	int r, i;
 
@@ -851,6 +870,9 @@ static void dss_register_vsync_isr(void)
 	for (i = 0; i < num_mgrs; ++i)
 		mask |= dispc_mgr_get_framedone_irq(i);
 
+	for (i = 0; i < num_wbs; i++)
+		mask |= dispc_wb_get_framedone_irq();
+
 	r = omap_dispc_register_isr(dss_apply_irq_handler, NULL, mask);
 	WARN_ON(r);
 
@@ -860,6 +882,7 @@ static void dss_register_vsync_isr(void)
 static void dss_unregister_vsync_isr(void)
 {
 	const int num_mgrs = dss_feat_get_num_mgrs();
+	const int num_wbs = dss_feat_get_num_wbs();
 	u32 mask;
 	int r, i;
 
@@ -870,6 +893,9 @@ static void dss_unregister_vsync_isr(void)
 	for (i = 0; i < num_mgrs; ++i)
 		mask |= dispc_mgr_get_framedone_irq(i);
 
+	for (i = 0; i < num_wbs; i++)
+		mask |= dispc_wb_get_framedone_irq();
+
 	r = omap_dispc_unregister_isr(dss_apply_irq_handler, NULL, mask);
 	WARN_ON(r);
 
@@ -879,6 +905,7 @@ static void dss_unregister_vsync_isr(void)
 static void dss_apply_irq_handler(void *data, u32 mask)
 {
 	const int num_mgrs = dss_feat_get_num_mgrs();
+	const int num_wbs = dss_feat_get_num_wbs();
 	int i;
 	bool extra_updating;
 
@@ -906,6 +933,16 @@ static void dss_apply_irq_handler(void *data, u32 mask)
 		}
 	}
 
+	for (i = 0; i < num_wbs; i++) {
+		struct omap_dss_output *wb = omap_dss_get_writeback();
+		struct wb_priv_data *wp = get_wb_priv(wb);
+
+		if (!wp->enabled)
+			continue;
+
+		wp->updating = dispc_wb_is_enabled();
+	}
+
 	dss_write_regs();
 	dss_set_go_bits();
 
@@ -1685,6 +1722,11 @@ void dss_wb_disable(struct omap_dss_output *wb)
 	if (!wp->enabled)
 		goto out;
 
+	if (wp->updating) {
+		DSSERR("can't disable writeback in the middle of an update\n");
+		goto out;
+	}
+
 	spin_lock_irqsave(&data_lock, flags);
 
 	wp->updating = false;
@@ -1694,3 +1736,44 @@ void dss_wb_disable(struct omap_dss_output *wb)
 out:
 	mutex_unlock(&apply_lock);
 }
+
+/*
+ * When doing a writeback mem to mem update, the updating field for writeback
+ * private data is true, but for manager private data the updating field is
+ * false. This is because a manager isn't really 'enabled' in HW, it's the
+ * writeback pipeline which initiates the transfer.
+ */
+void dss_wb_start_update(struct omap_dss_output *wb)
+{
+	struct omap_overlay_manager *mgr = wb->manager;
+	struct wb_priv_data *wp = get_wb_priv(wb);
+	unsigned long flags;
+	int r;
+
+	spin_lock_irqsave(&data_lock, flags);
+
+	WARN_ON(wp->updating);
+
+	/* add some similar check for writeback later */
+
+	r = dss_check_settings(mgr);
+	if (r) {
+		DSSERR("cannot start manual update: illegal configuration\n");
+		spin_unlock_irqrestore(&data_lock, flags);
+		return;
+	}
+
+	dss_wb_write_regs(wb);
+
+	dss_mgr_write_regs(mgr);
+	dss_mgr_write_regs_extra(mgr);
+
+	wp->updating = true;
+
+	if (!dss_data.irq_enabled && need_isr())
+		dss_register_vsync_isr();
+
+	dispc_wb_enable(true);
+
+	spin_unlock_irqrestore(&data_lock, flags);
+}
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index f9e7074..8c70b08 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -217,6 +217,7 @@ void dss_wb_get_info(struct omap_dss_output *wb,
 int omap_dss_wb_apply(struct omap_dss_output *wb);
 int dss_wb_enable(struct omap_dss_output *wb);
 void dss_wb_disable(struct omap_dss_output *wb);
+void dss_wb_start_update(struct omap_dss_output *wb);
 
 /* output */
 void dss_register_output(struct omap_dss_output *out);
diff --git a/drivers/video/omap2/dss/writeback.c b/drivers/video/omap2/dss/writeback.c
index 09394fb..7a25e99 100644
--- a/drivers/video/omap2/dss/writeback.c
+++ b/drivers/video/omap2/dss/writeback.c
@@ -20,6 +20,8 @@
 #include <linux/kernel.h>
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/semaphore.h>
+#include <linux/workqueue.h>
 #include <linux/platform_device.h>
 
 #include <video/omapdss.h>
@@ -27,7 +29,15 @@
 #include "dss.h"
 
 struct writeback_data {
+	struct platform_device *pdev;
+
 	struct mutex lock;
+	struct semaphore bus_lock;
+
+	void (*framedone_callback)(int, void *);
+	void *framedone_data;
+
+	struct delayed_work framedone_timeout_work;
 
 	struct omap_dss_output output;
 	/*
@@ -71,6 +81,32 @@ int dss_writeback_calc_channel_in(struct omap_overlay_manager *mgr,
 	return 0;
 }
 
+void omapdss_writeback_bus_lock(struct omap_dss_output *wb)
+{
+	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
+	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
+
+	down(&wb_data->bus_lock);
+}
+EXPORT_SYMBOL(omapdss_writeback_bus_lock);
+
+void omapdss_writeback_bus_unlock(struct omap_dss_output *wb)
+{
+	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
+	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
+
+	up(&wb_data->bus_lock);
+}
+EXPORT_SYMBOL(omapdss_writeback_bus_unlock);
+
+static bool writeback_bus_is_locked(struct omap_dss_output *wb)
+{
+	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
+	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
+
+	return wb_data->bus_lock.count = 0;
+}
+
 void omapdss_writeback_set_input_size(struct omap_dss_output *wb, u16 w, u16 h)
 {
 	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
@@ -88,11 +124,39 @@ void omapdss_writeback_set_input_size(struct omap_dss_output *wb, u16 w, u16 h)
 }
 EXPORT_SYMBOL(omapdss_writeback_set_input_size);
 
+static void writeback_handle_framedone(struct platform_device *wbdev, int error)
+{
+	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
+
+	wb_data->framedone_callback(error, wb_data->framedone_data);
+}
+
+static void writeback_framedone_timeout_work_callback(struct work_struct *work)
+{
+	struct writeback_data *wb_data = container_of(work,
+			struct writeback_data, framedone_timeout_work.work);
+
+	DSSERR("FRAMEDONE WB not received for 100ms\n");
+
+	writeback_handle_framedone(wb_data->pdev, -ETIMEDOUT);
+}
+
+static void writeback_framedone_irq_callback(void *data, u32 mask)
+{
+	struct platform_device *wbdev = (struct platform_device *) data;
+	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
+
+	cancel_delayed_work(&wb_data->framedone_timeout_work);
+
+	writeback_handle_framedone(wb_data->pdev, 0);
+}
+
 static void writeback_config_manager(struct omap_dss_output *wb)
 {
 	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
 	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
 	struct dss_lcd_mgr_config lcd_config;
+	int r;
 
 	dss_mgr_set_timings(wb->manager, &wb_data->input_timings);
 
@@ -110,6 +174,11 @@ static void writeback_config_manager(struct omap_dss_output *wb)
 	 */
 	dss_mgr_set_lcd_config(wb->manager, &lcd_config);
 
+	r = omap_dispc_register_isr(writeback_framedone_irq_callback,
+		(void *) wbdev, DISPC_IRQ_FRAMEDONEWB);
+	if (r)
+		DSSERR("Failed to register for FRAMEDONEWB irq\n");
+
 	dss_mgr_enable(wb->manager);
 }
 
@@ -119,6 +188,8 @@ int omapdss_writeback_enable(struct omap_dss_output *wb)
 	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
 	int r;
 
+	WARN_ON(!writeback_bus_is_locked(wb));
+
 	mutex_lock(&wb_data->lock);
 
 	r = dispc_runtime_get();
@@ -139,18 +210,48 @@ void omapdss_writeback_disable(struct omap_dss_output *wb)
 {
 	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
 	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
+	int r;
+
+	WARN_ON(!writeback_bus_is_locked(wb));
 
 	mutex_lock(&wb_data->lock);
 
 	dss_wb_disable(wb);
 	dss_mgr_disable(wb->manager);
 
+	r = omap_dispc_unregister_isr(writeback_framedone_irq_callback,
+		(void *) wbdev, DISPC_IRQ_FRAMEDONEWB);
+	if (r)
+		DSSERR("Failed to unregister FRAMEDONEWB irq\n");
+
 	dispc_runtime_put();
 
 	mutex_unlock(&wb_data->lock);
 }
 EXPORT_SYMBOL(omapdss_writeback_disable);
 
+int omapdss_writeback_update(struct omap_dss_output *wb,
+		void (*callback)(int, void *), void *data)
+{
+	struct platform_device *wbdev = writeback_get_wbdev_from_output(wb);
+	struct writeback_data *wb_data = writeback_get_drv_data(wbdev);
+	int r;
+
+	WARN_ON(!writeback_bus_is_locked(wb));
+
+	wb_data->framedone_callback = callback;
+	wb_data->framedone_data = data;
+
+	r = schedule_delayed_work(&wb_data->framedone_timeout_work,
+		msecs_to_jiffies(100));
+	BUG_ON(r = 0);
+
+	dss_wb_start_update(wb);
+
+	return 0;
+}
+EXPORT_SYMBOL(omapdss_writeback_update);
+
 int omapdss_writeback_apply(struct omap_dss_output *wb)
 {
 	return omap_dss_wb_apply(wb);
@@ -199,7 +300,12 @@ static int __init omap_writeback_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(&pdev->dev, wb_data);
 
+	wb_data->pdev = pdev;
 	mutex_init(&wb_data->lock);
+	sema_init(&wb_data->bus_lock, 1);
+
+	INIT_DEFERRABLE_WORK(&wb_data->framedone_timeout_work,
+			writeback_framedone_timeout_work_callback);
 
 	/* initialize with dummy timings */
 	wb_data->input_timings = (struct omap_video_timings)
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index f63c0cb..c7f3ac7 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -837,9 +837,13 @@ void omapdss_rfbi_set_data_lines(struct omap_dss_device *dssdev,
 void omapdss_rfbi_set_interface_timings(struct omap_dss_device *dssdev,
 		struct rfbi_timings *timings);
 
+void omapdss_writeback_bus_lock(struct omap_dss_output *wb);
+void omapdss_writeback_bus_unlock(struct omap_dss_output *wb);
 int omapdss_writeback_enable(struct omap_dss_output *wb);
 void omapdss_writeback_disable(struct omap_dss_output *wb);
 void omapdss_writeback_set_input_size(struct omap_dss_output *wb, u16 w, u16 h);
+int omapdss_writeback_update(struct omap_dss_output *wb,
+		void (*callback)(int, void *), void *data);
 int omapdss_writeback_apply(struct omap_dss_output *wb);
 int omapdss_writeback_set_info(struct omap_dss_output *wb,
 		struct omap_dss_writeback_info *info);
-- 
1.7.9.5


^ permalink raw reply related

* [RFC 08/11] OMAPDSS: APPLY: Check if overlay is connected in mem to mem mode
From: Archit Taneja @ 2012-11-07 14:56 UTC (permalink / raw)
  To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1352299469-17609-1-git-send-email-archit@ti.com>

When a manager is connected to writeback in mem to mem mode, all the connected
fetch and process data at rate suitable for the scalar to perform the required
downscaling. This is because there isn't any display connected here, and hence
no real time constraints. When calling dispc_ovl_setup, pass the correct value
of mem_to_mem parameter so that it can discard pixel clock related scaling
limitations.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/apply.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
index 2fd08d8..e1c589c 100644
--- a/drivers/video/omap2/dss/apply.c
+++ b/drivers/video/omap2/dss/apply.c
@@ -615,10 +615,13 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
 
 static void dss_ovl_write_regs(struct omap_overlay *ovl)
 {
+	struct omap_overlay_manager *mgr = ovl->manager;
+	struct omap_dss_output *out = mgr->output;
 	struct ovl_priv_data *op = get_ovl_priv(ovl);
 	struct omap_overlay_info *oi;
 	bool replication;
 	struct mgr_priv_data *mp;
+	bool mem_to_mem;
 	int r;
 
 	DSSDBG("writing ovl %d regs", ovl->id);
@@ -628,11 +631,13 @@ static void dss_ovl_write_regs(struct omap_overlay *ovl)
 
 	oi = &op->info;
 
-	mp = get_mgr_priv(ovl->manager);
+	mp = get_mgr_priv(mgr);
 
 	replication = dss_ovl_use_replication(mp->lcd_config, oi->color_mode);
 
-	r = dispc_ovl_setup(ovl->id, oi, replication, &mp->timings, false);
+	mem_to_mem = output_is_wb(out) ? wb_manual_update(out) : false;
+
+	r = dispc_ovl_setup(ovl->id, oi, replication, &mp->timings, mem_to_mem);
 	if (r) {
 		/*
 		 * We can't do much here, as this function can be called from
-- 
1.7.9.5


^ permalink raw reply related

* [RFC 09/11] OMAPDSS: FEATURES: Add writeback as supported outputs for OMAP4 managers
From: Archit Taneja @ 2012-11-07 14:56 UTC (permalink / raw)
  To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1352299469-17609-1-git-send-email-archit@ti.com>

The writeback pipeline can connect to any of the OMAP4 overlay managers. Add it
as a supported output in dss features.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/dss/dss_features.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index 3e8287c..e8d10cb 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -202,15 +202,15 @@ static const enum omap_dss_output_id omap3630_dss_supported_outputs[] = {
 static const enum omap_dss_output_id omap4_dss_supported_outputs[] = {
 	/* OMAP_DSS_CHANNEL_LCD */
 	OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI |
-	OMAP_DSS_OUTPUT_DSI1,
+	OMAP_DSS_OUTPUT_DSI1 | OMAP_DSS_OUTPUT_WB,
 
 	/* OMAP_DSS_CHANNEL_DIGIT */
 	OMAP_DSS_OUTPUT_VENC | OMAP_DSS_OUTPUT_HDMI |
-	OMAP_DSS_OUTPUT_DPI,
+	OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_WB,
 
 	/* OMAP_DSS_CHANNEL_LCD2 */
 	OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI |
-	OMAP_DSS_OUTPUT_DSI2,
+	OMAP_DSS_OUTPUT_DSI2 | OMAP_DSS_OUTPUT_WB,
 };
 
 static const enum omap_dss_output_id omap5_dss_supported_outputs[] = {
-- 
1.7.9.5


^ permalink raw reply related

* [RFC 10/11] ARCH: ARM: OMAP: Create a platform device for writeback
From: Archit Taneja @ 2012-11-07 14:56 UTC (permalink / raw)
  To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1352299469-17609-1-git-send-email-archit@ti.com>

writeback is a like DPI in the sense that it's completelty a part of DISPC.
Hence it doesn't have it's own resource, or a hwmod entity tied to it.

Create a simple platfrom device for writeback in omap_display_init. Set the
parent to the omapdss_dss platform device like done for other DSS submodules.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 arch/arm/mach-omap2/display.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 282c814e..2e01b61 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -414,6 +414,15 @@ int __init omap_display_init(struct omap_dss_board_info *board_data)
 		}
 	}
 
+	if (cpu_is_omap44xx()) {
+		pdev = create_simple_dss_pdev("omapdss_writeback", -1,
+				NULL, 0, dss_pdev);
+		if (IS_ERR(pdev)) {
+			pr_err("Could not build platform_device for omapdss_writeback\n");
+			return PTR_ERR(pdev);
+		}
+	}
+
 	return 0;
 }
 
-- 
1.7.9.5


^ permalink raw reply related

* [RFC 11/11] Example: OMAPFB: clear framebuffers using writeback
From: Archit Taneja @ 2012-11-07 14:56 UTC (permalink / raw)
  To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1352299469-17609-1-git-send-email-archit@ti.com>

This is an example to demonstrate how writeback is used to clear framebuffers.

The function omapfb_clear_fb_writeback is added as an alternative to the MPU
intensive function cfb_fillrect.

The writeback is attached to a free manager which has no overlays connected to
it, the manager's default color is set to black, and the size of both writeback
and manager are set to the framebuffer size. writeback_info is configured to
write the manager's output to the framebuffer address, and a mem to me update is
done.

This currently isn't full proof as it the logic of getting a free manager isn't
optimal yet and has a few corner cases.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-main.c |  132 +++++++++++++++++++++++++++++-
 1 file changed, 131 insertions(+), 1 deletion(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index be9096c..680f1eb 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -1590,8 +1590,131 @@ static int omapfb_allocate_all_fbs(struct omapfb2_device *fbdev)
 	return 0;
 }
 
+static struct omap_overlay_manager *get_free_manager(struct fb_info *fbi)
+{
+	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
+	struct omap_overlay *ovl;
+	struct omap_overlay_manager *mgr, *def_mgr;
+	int i;
+
+	ovl = omap_dss_get_overlay(0);
+	def_mgr = ovl->manager;
+
+	for (i = 0; i < fbdev->num_managers; i++) {
+		mgr = fbdev->managers[i];
+		if (mgr != def_mgr)
+			return mgr;
+	}
+
+	return NULL;
+}
+
+static void wb_callback(int err, void *data)
+{
+	struct omap_dss_output *wb = (struct omap_dss_output *) data;
+
+	omapdss_writeback_bus_unlock(wb);
+}
+
+static int omapfb_clear_fb_writeback(struct fb_info *fbi)
+{
+	struct fb_var_screeninfo *var = &fbi->var;
+	struct fb_fix_screeninfo *fix = &fbi->fix;
+	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omap_overlay_manager *mgr = NULL;
+	struct omap_overlay_manager_info mgr_info;
+	struct omap_dss_output *wb, *orig_out = NULL;
+	struct omap_dss_writeback_info wb_info;
+	enum omap_color_mode mode;
+	u32 data_start_p = 0;
+	int r;
+
+	wb = omap_dss_get_writeback();
+	if (!wb)
+		return -ENODEV;
+
+	/* find the first unused overlay manager */
+	mgr = get_free_manager(fbi);
+	if (!mgr)
+		return -ENODEV;
+
+	/* free the manager output */
+	if (mgr->output) {
+		orig_out = mgr->output;
+		mgr->unset_output(mgr);
+	}
+
+	/* get framebuffer color mode */
+	r = fb_mode_to_dss_mode(var, &mode);
+	if (r)
+		return r;
+
+	/* calculate framebuffer buffer address */
+	if (ofbi->region->size)
+		omapfb_calc_addr(ofbi, var, fix, 0, &data_start_p);
+
+	/* link the free overlay manager to writeback */
+	mgr->set_output(mgr, wb);
+
+	omapdss_writeback_bus_lock(wb);
+
+	/* enable writeback to configure writeback and overlay manager params */
+	omapdss_writeback_enable(wb);
+
+	/* configure and apply manager info to set default color to zero */
+	mgr->get_manager_info(mgr, &mgr_info);
+
+	mgr_info.default_color = 0x0;
+
+	mgr->set_manager_info(mgr, &mgr_info);
+
+	mgr->apply(mgr);
+
+	/*
+	 * configure writeback parameters to write manager output to
+	 * framebuffer
+	 */
+	omapdss_writeback_set_input_size(wb, fbi->var.xres_virtual,
+		fbi->var.yres_virtual);
+
+	omapdss_writeback_get_info(wb, &wb_info);
+
+	wb_info.paddr = data_start_p;
+	wb_info.rotation_type = OMAP_DSS_ROT_DMA;
+	wb_info.rotation = 0;
+	wb_info.mirror = 0;
+	wb_info.width = fbi->var.xres_virtual;
+	wb_info.buf_width = fbi->var.xres_virtual;
+	wb_info.height = fbi->var.yres_virtual;
+	wb_info.color_mode = mode;
+
+	omapdss_writeback_set_info(wb, &wb_info);
+
+	omapdss_writeback_apply(wb);
+
+	/* start writeback update */
+	omapdss_writeback_update(wb, wb_callback, wb);
+
+	omapdss_writeback_bus_lock(wb);
+
+	/* disable writeback(and the connected manager) */
+	omapdss_writeback_disable(wb);
+
+	omapdss_writeback_bus_unlock(wb);
+
+	/* restore manager back to it's old state */
+	mgr->unset_output(mgr);
+
+	if (orig_out)
+		mgr->set_output(mgr, orig_out);
+
+	return 0;
+}
+
 static void omapfb_clear_fb(struct fb_info *fbi)
 {
+	int r;
 	const struct fb_fillrect rect = {
 		.dx = 0,
 		.dy = 0,
@@ -1601,7 +1724,14 @@ static void omapfb_clear_fb(struct fb_info *fbi)
 		.rop = ROP_COPY,
 	};
 
-	cfb_fillrect(fbi, &rect);
+	r = omapfb_clear_fb_writeback(fbi);
+
+	/*
+	 * if clearing through writeback failed, revert to clearing the
+	 * framebuffer through MPU
+	 */
+	if (r)
+		cfb_fillrect(fbi, &rect);
 }
 
 int omapfb_realloc_fbmem(struct fb_info *fbi, unsigned long size, int type)
-- 
1.7.9.5


^ permalink raw reply related

* Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available
From: Tomi Valkeinen @ 2012-11-07 15:13 UTC (permalink / raw)
  To: Rob Clark
  Cc: Tomi Valkeinen, Archit Taneja, linux-omap, linux-fbdev, dri-devel
In-Reply-To: <CAF6AEGvJqmvkvkbnegVvnt+SxrWajB=Vq-LsY=k1c96eeyQEXA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 6815 bytes --]

On 2012-11-07 16:32, Rob Clark wrote:
> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

>> Hotplugging is not some abstract future scenario, we already have
>> hardware that could use it. For example, omap3 SDP board has a
>> switchable output to DVI or LCD panel. In this case we know what the two
>> options are, but the disabled component is still effectually removed
>> from the system, and plugged back in when it's enabled.
> 
> I would look at this as two different connectors which can not be used
> at the same time.  You have this scenario with desktop graphics cards.

Yes, that's an option with fixed amount of display devices. But doesn't
work for capes.

>> Hotplug is not a high priority item, but I do wish we get it supported
>> in common panel framework. Then it's at least possible to extend drm in
>> the future to support it.
>>
>>
>>
>> Anyway, this makes me wonder... omapdrm currently maps the elements of
>> the whole video pipeline to drm elements (encoder, connector, etc).
>> Would it make more sense to just map the DISPC to these drm elements?
>> Connector would then be the output from DISPC.
> 
> I think:
> 
>   plane->overlay
>   crtc->manager
> 
> is pretty clear.  And really
> 
>   encoder->output
> 
> should be the way it is..  on the branch w/ omapdss/omapdrm kms

I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
chain. The primary "output" is in the DISPC module, the overlay manager.
That's where the timings, pixel clock, etc. are programmed. The second
step, our output, is really a converter IP. It receives the primary
output, converts it and outputs something else. Just like an external
converter chip would do.

And the output can be quite a bit anything. For example, with DBI or DSI
command mode outputs we don't have any of the conventional video
timings. It doesn't make sense to program, say, video blanking periods
to those outputs. But even with DBI and DSI we do have video blanking
periods in the DISPC's output, the ovl mgr.

Of course, at the end of the chain we have a panel that uses normal
video timings (well, most likely but not necessarily), and so we could
program those timings at the end of the chain, in the block before the
panel. But even then the encoder doesn't really map to the DSS's output
block, as the DSS's output block may not have the conventional timings
(like DBI), or they may be something totally different than what we get
in the end of the chain to the panel.

So I think mapping encoder to output will not work with multiple display
blocks in a chain. Thus I'd see the encoder would better match the
DISPC's output, or alternatively perhaps the block which is just before
the panel (whatever that is, sometimes it can be OMAP's DSI/HDMI/etc).
However, the latter may be a bit strange as the block could be an
external component, possibly hotpluggable.

> re-write, this is how it is for plane/crtc, except for now:
> 
>   encoder+connector->dssdev
> 
> Basically the encoder is doing the "control" stuff (power on/off, set
> timings, etc), and the connector is only doing non control stuff
> (detect, reading edid, etc).
> 
> But I think this will probably change a bit as CFP comes into the
> picture.  Currently the drm connector is somewhat a "passive" element,
> but I think this will have to change a bit w/ CFP.
> 
>> This would map the drm elements to the static hardware blocks, and the
>> meaning of those blocks would be quite similar to what they are in the
>> desktop world (I guess).
>>
>> The panel driver, the external chips, and the DSS internal output blocks
>> (dsi, dpi, ...) would be handled separately from those drm elements. The
>> DSS internal blocks are static, of course, but they can be effectively
>> considered the same way as external chips.
> 
> I think dsi/dpi/etc map to encoder.  The big question is where the
> panel's fit.  But to userspace somehow this should look like
> connectors.  I think:
> 
>   encoder->output
>   connector->panel
> 
> could work.. although connector is less passive than KMS currently
> assumes.  And "panel" could really be a whole chain in the case of
> bridge chips, etc.  I don't know, maybe there are better ways.  But I
> think userspace really just wants to know "which monitor" which is
> basically connector.

Hmm yes. Well, even if we map encoder and connector to the ovl manager,
the userspace could see which monitor there is. Obviously we need to
make changes for that to work, but as a model it feels a lot more
natural to me than using output and panel for encoder and connector.

Perhaps it's wrong to say "map connector to ovl mgr". It would be more
like "this connector observes the chain connected to this ovl mgr", even
though the connector wouldn't observe any block in the chain directly.
Just observing the plug in/out status, etc.

But I think encoder really maps quite well directly to the output side
of overlay manager.

>> The omapdrm driver needs of course to access those separate elements
>> also, but that shouldn't be a problem. If omapdrm needs to call a
>> function in the panel driver, all it needs to do is go through the chain
>> to find the panel. Well, except if one output connected two two panels
>> via a bridge chip...
> 
> yeah, that is a really ugly case in our hw since it is quite
> non-transparent (ie. implications about use of planes, etc).

Not really in this case. You're perhaps thinking about connecting two
outputs to a single panel. Which is problematic also.

We don't have in sights a board that splits one output to two panels, so
I think we should just ignore that for now. But two outputs for one
panel is on the table.

>> And if drm is at some point extended to support panel drivers, or chains
>> of external display entities, it would be easier to add that support.
>>
>> What would it require the manage the elements like that? Would it help?
>> It sounds to me that this would simplify the model.
> 
> I'm not really entirely sure..  other than at least other drivers
> supporting CFP will have the same requirements ;-)
> 
> I guess the two best options are either bury some sort of chain of
> panel drivers in the connector, or introduce some internal elements in
> DRM which are not necessarily visible to userspace.  (Or at least
> userspace should have the option to ignore it for backwards
> compatibility.  For atomic pageflip/modeset, the converting of
> everything to properties makes it easier to think about exposing new
> KMS mode object types to userspace.)

Yes, I don't think we should or need to expose these new elements to
userspace, at least in the first place.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply

* Re: tty, vt: lockdep warnings
From: Alan Cox @ 2012-11-07 15:57 UTC (permalink / raw)
  To: Sasha Levin
  Cc: Dave Jones, Hugh Dickins, Sasha Levin, Daniel Vetter,
	Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-fbdev,
	florianSchandinat
In-Reply-To: <509A665C.3030603@gmail.com>

On Wed, 07 Nov 2012 08:47:08 -0500
Sasha Levin <levinsasha928@gmail.com> wrote:

> On 11/06/2012 12:38 PM, Alan Cox wrote:
> >>  > The root
> >>  > cause is loading two different framebuffers with one taking over from
> >>  > another - that should be an obscure corner case and once the fuzz testing
> >>  > can avoid.
> >>  > 
> >>  > I had a semi-informed poke at this and came up with a possible patch (not very tested)
> >>
> >> If this fixes the real problems we've been seeing, I'll dance a jig.
> > 
> > Youtube...
> 
> +1
> 
> > At this point my bigger concern is that it'll just make something else
> > warn instead. The underlying problem is that fbcon layer implements a
> > single threaded notifier whose locking semantics are at best random. It's
> > not calld with a specific set of locks each time. Possibly it sohuld be
> > two notifiers (one for fb stuff, one for console layer stuff) but the
> > entire layer is horrible. I live in home the KMS guys will rip out the
> > useful bits and build a straight kms fb layer with refcounting and the
> > like 8)
> > 
> > Testing certainly needed and if it's still blowing up then hopefully
> > further traces will help fix up the other cases we don't know about.
> 
> So the good news are that the original lockdep splat I've reported is gone.
> 
> The semi-bad news are that there's a new one. It happens less frequently
> but I assume it's not a new splat either, but was well hidden behind the
> other splat.

Doesn't look too bad (fingers crossed)

try

commit 8f965b0816d98ed535fdfccc3e50d84818e9c43b
Author: Alan Cox <alan@linux.intel.com>
Date:   Wed Nov 7 15:48:48 2012 +0000

    fb: Rework locking to fix lock ordering on takeover
    
    Adjust the console layer to allow a take over call where the caller already
    holds the locks. Make the fb layer lock in order.
    
    This s partly a band aid, the fb layer is terminally confused about the
    locking rules it uses for its notifiers it seems.
    
    Signed-off-by: Alan Cox <alan@linux.intel.com>

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index f87d7e8..ea57f27 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2984,7 +2984,7 @@ int __init vty_init(const struct file_operations *console_fops)
 
 static struct class *vtconsole_class;
 
-static int bind_con_driver(const struct consw *csw, int first, int last,
+static int do_bind_con_driver(const struct consw *csw, int first, int last,
 			   int deflt)
 {
 	struct module *owner = csw->owner;
@@ -2995,7 +2995,7 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
 	if (!try_module_get(owner))
 		return -ENODEV;
 
-	console_lock();
+	WARN_CONSOLE_UNLOCKED();
 
 	/* check if driver is registered */
 	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
@@ -3080,11 +3080,22 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
 
 	retval = 0;
 err:
-	console_unlock();
 	module_put(owner);
 	return retval;
 };
 
+
+static int bind_con_driver(const struct consw *csw, int first, int last,
+			   int deflt)
+{
+	int ret;
+	
+	console_unlock();
+	ret = do_bind_con_driver(csw, first, last, deflt);
+	console_unlock();
+	return ret;
+}
+	
 #ifdef CONFIG_VT_HW_CONSOLE_BINDING
 static int con_is_graphics(const struct consw *csw, int first, int last)
 {
@@ -3196,9 +3207,9 @@ int unbind_con_driver(const struct consw *csw, int first, int last, int deflt)
 	if (!con_is_bound(csw))
 		con_driver->flag &= ~CON_DRIVER_FLAG_INIT;
 
-	console_unlock();
 	/* ignore return value, binding should not fail */
-	bind_con_driver(defcsw, first, last, deflt);
+	do_bind_con_driver(defcsw, first, last, deflt);
+	console_unlock();
 err:
 	module_put(owner);
 	return retval;
@@ -3489,28 +3500,18 @@ int con_debug_leave(void)
 }
 EXPORT_SYMBOL_GPL(con_debug_leave);
 
-/**
- * register_con_driver - register console driver to console layer
- * @csw: console driver
- * @first: the first console to take over, minimum value is 0
- * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
- *
- * DESCRIPTION: This function registers a console driver which can later
- * bind to a range of consoles specified by @first and @last. It will
- * also initialize the console driver by calling con_startup().
- */
-int register_con_driver(const struct consw *csw, int first, int last)
+static int do_register_con_driver(const struct consw *csw, int first, int last)
 {
 	struct module *owner = csw->owner;
 	struct con_driver *con_driver;
 	const char *desc;
 	int i, retval = 0;
 
+	WARN_CONSOLE_UNLOCKED();
+
 	if (!try_module_get(owner))
 		return -ENODEV;
 
-	console_lock();
-
 	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
 		con_driver = &registered_con_driver[i];
 
@@ -3563,10 +3564,29 @@ int register_con_driver(const struct consw *csw, int first, int last)
 	}
 
 err:
-	console_unlock();
 	module_put(owner);
 	return retval;
 }
+
+/**
+ * register_con_driver - register console driver to console layer
+ * @csw: console driver
+ * @first: the first console to take over, minimum value is 0
+ * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
+ *
+ * DESCRIPTION: This function registers a console driver which can later
+ * bind to a range of consoles specified by @first and @last. It will
+ * also initialize the console driver by calling con_startup().
+ */
+int register_con_driver(const struct consw *csw, int first, int last)
+{
+	int retval;
+	
+	console_lock();
+	retval = do_register_con_driver(csw, first, last);
+	console_unlock();
+	return retval;
+}
 EXPORT_SYMBOL(register_con_driver);
 
 /**
@@ -3622,6 +3642,29 @@ EXPORT_SYMBOL(unregister_con_driver);
  *
  *      take_over_console is basically a register followed by unbind
  */
+int do_take_over_console(const struct consw *csw, int first, int last, int deflt)
+{
+	int err;
+
+	err = do_register_con_driver(csw, first, last);
+	/* if we get an busy error we still want to bind the console driver
+	 * and return success, as we may have unbound the console driver
+	 * but not unregistered it.
+	*/
+	if (err = -EBUSY)
+		err = 0;
+	if (!err)
+		do_bind_con_driver(csw, first, last, deflt);
+
+	return err;
+}
+/*
+ *	If we support more console drivers, this function is used
+ *	when a driver wants to take over some existing consoles
+ *	and become default driver for newly opened ones.
+ *
+ *      take_over_console is basically a register followed by unbind
+ */
 int take_over_console(const struct consw *csw, int first, int last, int deflt)
 {
 	int err;
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index fdefa8f..c75f8ce 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -529,6 +529,34 @@ static int search_for_mapped_con(void)
 	return retval;
 }
 
+static int do_fbcon_takeover(int show_logo)
+{
+	int err, i;
+
+	if (!num_registered_fb)
+		return -ENODEV;
+
+	if (!show_logo)
+		logo_shown = FBCON_LOGO_DONTSHOW;
+
+	for (i = first_fb_vc; i <= last_fb_vc; i++)
+		con2fb_map[i] = info_idx;
+
+	err = do_take_over_console(&fb_con, first_fb_vc, last_fb_vc,
+				fbcon_is_default);
+
+	if (err) {
+		for (i = first_fb_vc; i <= last_fb_vc; i++) {
+			con2fb_map[i] = -1;
+		}
+		info_idx = -1;
+	} else {
+		fbcon_has_console_bind = 1;
+	}
+
+	return err;
+}
+
 static int fbcon_takeover(int show_logo)
 {
 	int err, i;
@@ -3115,7 +3143,7 @@ static int fbcon_fb_registered(struct fb_info *info)
 		}
 
 		if (info_idx != -1)
-			ret = fbcon_takeover(1);
+			ret = do_fbcon_takeover(1);
 	} else {
 		for (i = first_fb_vc; i <= last_fb_vc; i++) {
 			if (con2fb_map_boot[i] = idx)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 3ff0105..564ebe9 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1650,7 +1650,9 @@ static int do_register_framebuffer(struct fb_info *fb_info)
 	event.info = fb_info;
 	if (!lock_fb_info(fb_info))
 		return -ENODEV;
+        console_lock();
 	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
+        console_unlock();
 	unlock_fb_info(fb_info);
 	return 0;
 }
@@ -1853,11 +1855,8 @@ int fb_new_modelist(struct fb_info *info)
 	err = 1;
 
 	if (!list_empty(&info->modelist)) {
-		if (!lock_fb_info(info))
-			return -ENODEV;
 		event.info = info;
 		err = fb_notifier_call_chain(FB_EVENT_NEW_MODELIST, &event);
-		unlock_fb_info(info);
 	}
 
 	return err;
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index a55e366..ef476b0 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -177,6 +177,8 @@ static ssize_t store_modes(struct device *device,
 	if (i * sizeof(struct fb_videomode) != count)
 		return -EINVAL;
 
+	if (!lock_fb_info(fb_info))
+		return -ENODEV;
 	console_lock();
 	list_splice(&fb_info->modelist, &old_list);
 	fb_videomode_to_modelist((const struct fb_videomode *)buf, i,
@@ -188,6 +190,7 @@ static ssize_t store_modes(struct device *device,
 		fb_destroy_modelist(&old_list);
 
 	console_unlock();
+	unlock_fb_info(fb_info);
 
 	return 0;
 }
diff --git a/include/linux/console.h b/include/linux/console.h
index dedb082..4ef4307 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -78,6 +78,7 @@ int con_is_bound(const struct consw *csw);
 int register_con_driver(const struct consw *csw, int first, int last);
 int unregister_con_driver(const struct consw *csw);
 int take_over_console(const struct consw *sw, int first, int last, int deflt);
+int do_take_over_console(const struct consw *sw, int first, int last, int deflt);
 void give_up_console(const struct consw *sw);
 #ifdef CONFIG_HW_CONSOLE
 int con_debug_enter(struct vc_data *vc);

^ permalink raw reply related

* Re: tty, vt: lockdep warnings
From: Bjørn Mork @ 2012-11-07 16:15 UTC (permalink / raw)
  To: Alan Cox
  Cc: Sasha Levin, Dave Jones, Hugh Dickins, Sasha Levin, Daniel Vetter,
	Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-fbdev,
	florianSchandinat
In-Reply-To: <20121107160232.18e83ee9@pyramind.ukuu.org.uk>

Alan Cox <alan@lxorguk.ukuu.org.uk> writes:

> +
> +static int bind_con_driver(const struct consw *csw, int first, int last,
> +			   int deflt)
> +{
> +	int ret;
> +	
> +	console_unlock();

console_lock() maybe...

> +	ret = do_bind_con_driver(csw, first, last, deflt);
> +	console_unlock();
> +	return ret;
> +}
> +	


Bjørn

^ permalink raw reply

* Re: tty, vt: lockdep warnings
From: Alan Cox @ 2012-11-07 16:56 UTC (permalink / raw)
  To: Bjørn Mork
  Cc: Sasha Levin, Dave Jones, Hugh Dickins, Sasha Levin, Daniel Vetter,
	Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-fbdev,
	florianSchandinat
In-Reply-To: <87625h9yoe.fsf@nemi.mork.no>

On Wed, 07 Nov 2012 17:15:45 +0100
Bjørn Mork <bjorn@mork.no> wrote:

> Alan Cox <alan@lxorguk.ukuu.org.uk> writes:
> 
> > +
> > +static int bind_con_driver(const struct consw *csw, int first, int last,
> > +			   int deflt)
> > +{
> > +	int ret;
> > +	
> > +	console_unlock();
> 
> console_lock() maybe...

Thanks I'll fix that path in v3

^ permalink raw reply

* Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available
From: Rob Clark @ 2012-11-07 19:18 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Tomi Valkeinen, Archit Taneja, linux-omap, linux-fbdev, dri-devel
In-Reply-To: <509A7A95.804@ti.com>

On Wed, Nov 7, 2012 at 9:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 2012-11-07 16:32, Rob Clark wrote:
>> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>>> Hotplugging is not some abstract future scenario, we already have
>>> hardware that could use it. For example, omap3 SDP board has a
>>> switchable output to DVI or LCD panel. In this case we know what the two
>>> options are, but the disabled component is still effectually removed
>>> from the system, and plugged back in when it's enabled.
>>
>> I would look at this as two different connectors which can not be used
>> at the same time.  You have this scenario with desktop graphics cards.
>
> Yes, that's an option with fixed amount of display devices. But doesn't
> work for capes.

Only if capes are hotpluggable.. otherwise probe what cape(s?) are
present at boot time (is this possible to detect a cape from sw?), and
create the associated connector(s).

Anyways, I think we are stretching a bit hard for use cases for
hot-pluggable panels..  I just prefer to ignore hotplug for now and
come back to it when there is a more legitimate use-case.

>>> Hotplug is not a high priority item, but I do wish we get it supported
>>> in common panel framework. Then it's at least possible to extend drm in
>>> the future to support it.
>>>
>>>
>>>
>>> Anyway, this makes me wonder... omapdrm currently maps the elements of
>>> the whole video pipeline to drm elements (encoder, connector, etc).
>>> Would it make more sense to just map the DISPC to these drm elements?
>>> Connector would then be the output from DISPC.
>>
>> I think:
>>
>>   plane->overlay
>>   crtc->manager
>>
>> is pretty clear.  And really
>>
>>   encoder->output
>>
>> should be the way it is..  on the branch w/ omapdss/omapdrm kms
>
> I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
> chain. The primary "output" is in the DISPC module, the overlay manager.
> That's where the timings, pixel clock, etc. are programmed. The second
> step, our output, is really a converter IP. It receives the primary
> output, converts it and outputs something else. Just like an external
> converter chip would do.
>

the timings, pixel clock, vblank/framedone irqs, that all maps to crtc.

encoder = converter, so I think this fits.  "An encoder takes pixel
data from a CRTC and converts it to a format suitable for any attached
connectors" (from drm docbook)

> And the output can be quite a bit anything. For example, with DBI or DSI
> command mode outputs we don't have any of the conventional video
> timings. It doesn't make sense to program, say, video blanking periods
> to those outputs. But even with DBI and DSI we do have video blanking
> periods in the DISPC's output, the ovl mgr.

the encoder has mode_fixup() which can alter the timings that end up
getting set, which might be a way to account for this.  I guess really
it should be the panel driver that is telling the encoder what
adjusted timings to give to the crtc.. so the panel driver doesn't
quite map to connector.

> Of course, at the end of the chain we have a panel that uses normal
> video timings (well, most likely but not necessarily), and so we could
> program those timings at the end of the chain, in the block before the
> panel. But even then the encoder doesn't really map to the DSS's output
> block, as the DSS's output block may not have the conventional timings
> (like DBI), or they may be something totally different than what we get
> in the end of the chain to the panel.
>
> So I think mapping encoder to output will not work with multiple display
> blocks in a chain. Thus I'd see the encoder would better match the
> DISPC's output, or alternatively perhaps the block which is just before
> the panel (whatever that is, sometimes it can be OMAP's DSI/HDMI/etc).
> However, the latter may be a bit strange as the block could be an
> external component, possibly hotpluggable.
>
>> re-write, this is how it is for plane/crtc, except for now:
>>
>>   encoder+connector->dssdev
>>
>> Basically the encoder is doing the "control" stuff (power on/off, set
>> timings, etc), and the connector is only doing non control stuff
>> (detect, reading edid, etc).
>>
>> But I think this will probably change a bit as CFP comes into the
>> picture.  Currently the drm connector is somewhat a "passive" element,
>> but I think this will have to change a bit w/ CFP.
>>
>>> This would map the drm elements to the static hardware blocks, and the
>>> meaning of those blocks would be quite similar to what they are in the
>>> desktop world (I guess).
>>>
>>> The panel driver, the external chips, and the DSS internal output blocks
>>> (dsi, dpi, ...) would be handled separately from those drm elements. The
>>> DSS internal blocks are static, of course, but they can be effectively
>>> considered the same way as external chips.
>>
>> I think dsi/dpi/etc map to encoder.  The big question is where the
>> panel's fit.  But to userspace somehow this should look like
>> connectors.  I think:
>>
>>   encoder->output
>>   connector->panel
>>
>> could work.. although connector is less passive than KMS currently
>> assumes.  And "panel" could really be a whole chain in the case of
>> bridge chips, etc.  I don't know, maybe there are better ways.  But I
>> think userspace really just wants to know "which monitor" which is
>> basically connector.
>
> Hmm yes. Well, even if we map encoder and connector to the ovl manager,
> the userspace could see which monitor there is. Obviously we need to
> make changes for that to work, but as a model it feels a lot more
> natural to me than using output and panel for encoder and connector.

well, what we call 'ovl mgr' is really where the (vblank) interrupt
generation is.  This really should be the crtc.  I tried it
differently initially, and it wasn't really working out.

> Perhaps it's wrong to say "map connector to ovl mgr". It would be more
> like "this connector observes the chain connected to this ovl mgr", even
> though the connector wouldn't observe any block in the chain directly.
> Just observing the plug in/out status, etc.
>
> But I think encoder really maps quite well directly to the output side
> of overlay manager.
>
>>> The omapdrm driver needs of course to access those separate elements
>>> also, but that shouldn't be a problem. If omapdrm needs to call a
>>> function in the panel driver, all it needs to do is go through the chain
>>> to find the panel. Well, except if one output connected two two panels
>>> via a bridge chip...
>>
>> yeah, that is a really ugly case in our hw since it is quite
>> non-transparent (ie. implications about use of planes, etc).
>
> Not really in this case. You're perhaps thinking about connecting two
> outputs to a single panel. Which is problematic also.

yes, sorry, I read that backwards..

> We don't have in sights a board that splits one output to two panels, so
> I think we should just ignore that for now. But two outputs for one
> panel is on the table.
>
>>> And if drm is at some point extended to support panel drivers, or chains
>>> of external display entities, it would be easier to add that support.
>>>
>>> What would it require the manage the elements like that? Would it help?
>>> It sounds to me that this would simplify the model.
>>
>> I'm not really entirely sure..  other than at least other drivers
>> supporting CFP will have the same requirements ;-)
>>
>> I guess the two best options are either bury some sort of chain of
>> panel drivers in the connector, or introduce some internal elements in
>> DRM which are not necessarily visible to userspace.  (Or at least
>> userspace should have the option to ignore it for backwards
>> compatibility.  For atomic pageflip/modeset, the converting of
>> everything to properties makes it easier to think about exposing new
>> KMS mode object types to userspace.)
>
> Yes, I don't think we should or need to expose these new elements to
> userspace, at least in the first place.

It *might* be useful someday, if there are ever any settings for a
bridge chip, etc, which we'd want to expose to userspace as
properties..  but I think we can come back to that later.

BR,
-R

>  Tomi
>
>

^ permalink raw reply

* Re: i.MX 257 ARM-CPU: framebuffer error "division by zero"
From: Stefan Koch @ 2012-11-07 20:33 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <509A6964.5090305@gmail.com>

The error snipped in first post is not from the beginning it's from the end.
This is from the beginning.
And there is named "imxfb", too.

For this error "video=imxfb:SVGA-16@60" is set in kernel command line:

The full messages are here: http://paste.debian.net/hidden/3d16ba27/

[<80246f78>] (register_framebuffer+0x17c/0x230) from [<80586ab8>] 
(imxfb_probe+0x3c8/0x5a8)
[<80586ab8>] (imxfb_probe+0x3c8/0x5a8) from [<80286ba8>] 
(platform_drv_probe+0x14/0x18)
[<80286ba8>] (platform_drv_probe+0x14/0x18) from [<802858b0>] 
(driver_probe_device+0x80/0x20c)
[<802858b0>] (driver_probe_device+0x80/0x20c) from [<80285ac8>] 
(__driver_attach+0x8c/0x90)
[<80285ac8>] (__driver_attach+0x8c/0x90) from [<80284230>] 
(bus_for_each_dev+0x64/0x8c)
[<80284230>] (bus_for_each_dev+0x64/0x8c) from [<802849b0>] 
(bus_add_driver+0xa4/0x230)
[<802849b0>] (bus_add_driver+0xa4/0x230) from [<80286098>] 
(driver_register+0x78/0x140)
[<80286098>] (driver_register+0x78/0x140) from [<80286ef4>] 
(platform_driver_probe+0x18/0x9c)
[<80286ef4>] (platform_driver_probe+0x18/0x9c) from [<805866d8>] 
(imxfb_init+0x88/0xa0)
[<805866d8>] (imxfb_init+0x88/0xa0) from [<800087e4>] 
(do_one_initcall+0x30/0x178)
[<800087e4>] (do_one_initcall+0x30/0x178) from [<80576310>] 
(kernel_init+0xe4/0x1b0)
[<80576310>] (kernel_init+0xe4/0x1b0) from [<80015268>] 
(kernel_thread_exit+0x0/0x8)
Division by zero in kernel.
[<80019ba4>] (unwind_backtrace+0x0/0xf4) from [<8022accc>] (Ldiv0+0x8/0x10)
[<8022accc>] (Ldiv0+0x8/0x10) from [<8022ac9c>] (__aeabi_uidivmod+0x8/0x18)
[<8022ac9c>] (__aeabi_uidivmod+0x8/0x18) from [<80254198>] 
(cfb_imageblit+0x1fc/0x4b0)
[<80254198>] (cfb_imageblit+0x1fc/0x4b0) from [<80251964>] 
(bit_putcs+0x2e4/0x420)
[<80251964>] (bit_putcs+0x2e4/0x420) from [<8024bde0>] 
(fbcon_putcs+0xec/0x128)
[<8024bde0>] (fbcon_putcs+0xec/0x128) from [<802741f8>] 
(do_update_region+0x108/0x18c)
[<802741f8>] (do_update_region+0x108/0x18c) from [<80275970>] 
(update_region+0x70/0x88)
[<80275970>] (update_region+0x70/0x88) from [<8024dd08>] 
(fbcon_switch+0x468/0x4bc)
[<8024dd08>] (fbcon_switch+0x468/0x4bc) from [<80276c84>] 
(redraw_screen+0x15c/0x278)
[<80276c84>] (redraw_screen+0x15c/0x278) from [<80279e90>] 
(take_over_console+0x2cc/0x37c)
[<80279e90>] (take_over_console+0x2cc/0x37c) from [<8024cacc>] 
(fbcon_takeover+0x70/0xd4)
[<8024cacc>] (fbcon_takeover+0x70/0xd4) from [<8042541c>] 
(notifier_call_chain+0x44/0x84)
[<8042541c>] (notifier_call_chain+0x44/0x84) from [<8004a2a0>] 
(__blocking_notifier_call_chain+0x4c/0x64)
[<8004a2a0>] (__blocking_notifier_call_chain+0x4c/0x64) from 
[<8004a2d0>] (blocking_notifier_call_chain+0x18/0x20)
[<8004a2d0>] (blocking_notifier_call_chain+0x18/0x20) from [<80246f78>] 
(register_framebuffer+0x17c/0x230)
[<80246f78>] (register_framebuffer+0x17c/0x230) from [<80586ab8>] 
(imxfb_probe+0x3c8/0x5a8)
[<80586ab8>] (imxfb_probe+0x3c8/0x5a8) from [<80286ba8>] 
(platform_drv_probe+0x14/0x18)
[<80286ba8>] (platform_drv_probe+0x14/0x18) from [<802858b0>] 
(driver_probe_device+0x80/0x20c)
[<802858b0>] (driver_probe_device+0x80/0x20c) from [<80285ac8>] 
(__driver_attach+0x8c/0x90)
[<80285ac8>] (__driver_attach+0x8c/0x90) from [<80284230>] 
(bus_for_each_dev+0x64/0x8c)
[<80284230>] (bus_for_each_dev+0x64/0x8c) from [<802849b0>] 
(bus_add_driver+0xa4/0x230)
[<802849b0>] (bus_add_driver+0xa4/0x230) from [<80286098>] 
(driver_register+0x78/0x140)
[<80286098>] (driver_register+0x78/0x140) from [<80286ef4>] 
(platform_driver_probe+0x18/0x9c)
[<80286ef4>] (platform_driver_probe+0x18/0x9c) from [<805866d8>] 
(imxfb_init+0x88/0xa0)
[<805866d8>] (imxfb_init+0x88/0xa0) from [<800087e4>] 
(do_one_initcall+0x30/0x178)
[<800087e4>] (do_one_initcall+0x30/0x178) from [<80576310>] 
(kernel_init+0xe4/0x1b0)
[<80576310>] (kernel_init+0xe4/0x1b0) from [<80015268>] 
(kernel_thread_exit+0x0/0x8)


Without this setting the CPU does stop working (for JTAG debugger it has 
the same effect as board is in power off state):

177772 bytes written at address 0x811ffc00
downloaded 177772 bytes in 19.548666s (8.881 KiB/s)
WARNING: unknown debug reason: 0xf
ThumbEE -- incomplete support
target state: halted
target halted in ThumbEE state due to debug-request, current mode: System
cpsr: 0xffffffff pc: 0xfffffff9
MMU: enabled, D-Cache: enabled, I-Cache: enabled




^ permalink raw reply

* [PATCH] backlight: Add of_find_backlight_by_node() function
From: Thierry Reding @ 2012-11-07 22:08 UTC (permalink / raw)
  To: Florian Tobias Schandinat
  Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

This function finds the struct backlight_device for a given device tree
node. A dummy function is provided so that it safely compiles out if OF
support is disabled.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/video/backlight/backlight.c | 17 +++++++++++++++++
 include/linux/backlight.h           | 10 ++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 297db2f..0d1ed4f 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -370,6 +370,23 @@ void backlight_device_unregister(struct backlight_device *bd)
 }
 EXPORT_SYMBOL(backlight_device_unregister);
 
+#if IS_ENABLED(CONFIG_OF)
+static int of_parent_match(struct device *dev, void *data)
+{
+	return dev->parent && dev->parent->of_node = data;
+}
+
+struct backlight_device *of_find_backlight_by_node(struct device_node *node)
+{
+	struct device *dev;
+
+	dev = class_find_device(backlight_class, NULL, node, of_parent_match);
+
+	return dev ? to_backlight_device(dev) : NULL;
+}
+EXPORT_SYMBOL(of_find_backlight_by_node);
+#endif
+
 static void __exit backlight_class_exit(void)
 {
 	class_destroy(backlight_class);
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 5ffc6dd..11840e9 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -134,4 +134,14 @@ struct generic_bl_info {
 	void (*kick_battery)(void);
 };
 
+#if IS_ENABLED(CONFIG_OF)
+struct backlight_device *of_find_backlight_by_node(struct device_node *node);
+#else
+static inline struct backlight_device *
+of_find_backlight_by_node(struct device_node *node)
+{
+	return NULL;
+}
+#endif
+
 #endif
-- 
1.8.0


^ permalink raw reply related

* Re: [PATCH 1/2] video: exynos_dp: remove redundant parameters
From: Jingoo Han @ 2012-11-08  0:54 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <1352286744-22588-1-git-send-email-ajaykumar.rs@samsung.com>

On Wednesday, November 07, 2012 8:12 PM Ajay Kumar wrote
> 
> This patch cleans up few redundant parameters keeping
> the same functionality intact.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>


Acked-by: Jingoo Han <jg1.han@samsung.com>


> ---
>  drivers/video/exynos/exynos_dp_core.c |   14 +++++---------
>  drivers/video/exynos/exynos_dp_core.h |    9 ++-------
>  drivers/video/exynos/exynos_dp_reg.c  |   23 +++++++++--------------
>  3 files changed, 16 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index d55470e..f62778c 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -752,19 +752,15 @@ static int exynos_dp_set_link_train(struct exynos_dp_device *dp,
>  	return retval;
>  }
> 
> -static int exynos_dp_config_video(struct exynos_dp_device *dp,
> -			struct video_info *video_info)
> +static int exynos_dp_config_video(struct exynos_dp_device *dp)
>  {
>  	int retval = 0;
>  	int timeout_loop = 0;
>  	int done_count = 0;
> 
> -	exynos_dp_config_video_slave_mode(dp, video_info);
> +	exynos_dp_config_video_slave_mode(dp);
> 
> -	exynos_dp_set_video_color_format(dp, video_info->color_depth,
> -			video_info->color_space,
> -			video_info->dynamic_range,
> -			video_info->ycbcr_coeff);
> +	exynos_dp_set_video_color_format(dp);
> 
>  	if (exynos_dp_get_pll_lock_status(dp) = PLL_UNLOCKED) {
>  		dev_err(dp->dev, "PLL is not locked yet.\n");
> @@ -937,7 +933,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>  	exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
> 
>  	exynos_dp_init_video(dp);
> -	ret = exynos_dp_config_video(dp, dp->video_info);
> +	ret = exynos_dp_config_video(dp);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to config video\n");
>  		return ret;
> @@ -1003,7 +999,7 @@ static int exynos_dp_resume(struct device *dev)
>  	exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
> 
>  	exynos_dp_init_video(dp);
> -	exynos_dp_config_video(dp, dp->video_info);
> +	exynos_dp_config_video(dp);
> 
>  	return 0;
>  }
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 57b8a65..1e646d7 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -107,11 +107,7 @@ u32 exynos_dp_get_lane3_link_training(struct exynos_dp_device *dp);
>  void exynos_dp_reset_macro(struct exynos_dp_device *dp);
>  void exynos_dp_init_video(struct exynos_dp_device *dp);
> 
> -void exynos_dp_set_video_color_format(struct exynos_dp_device *dp,
> -				u32 color_depth,
> -				u32 color_space,
> -				u32 dynamic_range,
> -				u32 ycbcr_coeff);
> +void exynos_dp_set_video_color_format(struct exynos_dp_device *dp);
>  int exynos_dp_is_slave_video_stream_clock_on(struct exynos_dp_device *dp);
>  void exynos_dp_set_video_cr_mn(struct exynos_dp_device *dp,
>  			enum clock_recovery_m_value_type type,
> @@ -121,8 +117,7 @@ void exynos_dp_set_video_timing_mode(struct exynos_dp_device *dp, u32 type);
>  void exynos_dp_enable_video_master(struct exynos_dp_device *dp, bool enable);
>  void exynos_dp_start_video(struct exynos_dp_device *dp);
>  int exynos_dp_is_video_stream_on(struct exynos_dp_device *dp);
> -void exynos_dp_config_video_slave_mode(struct exynos_dp_device *dp,
> -			struct video_info *video_info);
> +void exynos_dp_config_video_slave_mode(struct exynos_dp_device *dp);
>  void exynos_dp_enable_scrambling(struct exynos_dp_device *dp);
>  void exynos_dp_disable_scrambling(struct exynos_dp_device *dp);
> 
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index 3f5ca8a..db4975d 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -1034,24 +1034,20 @@ void exynos_dp_init_video(struct exynos_dp_device *dp)
>  	writel(reg, dp->reg_base + EXYNOS_DP_VIDEO_CTL_8);
>  }
> 
> -void exynos_dp_set_video_color_format(struct exynos_dp_device *dp,
> -			u32 color_depth,
> -			u32 color_space,
> -			u32 dynamic_range,
> -			u32 ycbcr_coeff)
> +void exynos_dp_set_video_color_format(struct exynos_dp_device *dp)
>  {
>  	u32 reg;
> 
>  	/* Configure the input color depth, color space, dynamic range */
> -	reg = (dynamic_range << IN_D_RANGE_SHIFT) |
> -		(color_depth << IN_BPC_SHIFT) |
> -		(color_space << IN_COLOR_F_SHIFT);
> +	reg = (dp->video_info->dynamic_range << IN_D_RANGE_SHIFT) |
> +		(dp->video_info->color_depth << IN_BPC_SHIFT) |
> +		(dp->video_info->color_space << IN_COLOR_F_SHIFT);
>  	writel(reg, dp->reg_base + EXYNOS_DP_VIDEO_CTL_2);
> 
>  	/* Set Input Color YCbCr Coefficients to ITU601 or ITU709 */
>  	reg = readl(dp->reg_base + EXYNOS_DP_VIDEO_CTL_3);
>  	reg &= ~IN_YC_COEFFI_MASK;
> -	if (ycbcr_coeff)
> +	if (dp->video_info->ycbcr_coeff)
>  		reg |= IN_YC_COEFFI_ITU709;
>  	else
>  		reg |= IN_YC_COEFFI_ITU601;
> @@ -1178,8 +1174,7 @@ int exynos_dp_is_video_stream_on(struct exynos_dp_device *dp)
>  	return 0;
>  }
> 
> -void exynos_dp_config_video_slave_mode(struct exynos_dp_device *dp,
> -			struct video_info *video_info)
> +void exynos_dp_config_video_slave_mode(struct exynos_dp_device *dp)
>  {
>  	u32 reg;
> 
> @@ -1190,17 +1185,17 @@ void exynos_dp_config_video_slave_mode(struct exynos_dp_device *dp,
> 
>  	reg = readl(dp->reg_base + EXYNOS_DP_VIDEO_CTL_10);
>  	reg &= ~INTERACE_SCAN_CFG;
> -	reg |= (video_info->interlaced << 2);
> +	reg |= (dp->video_info->interlaced << 2);
>  	writel(reg, dp->reg_base + EXYNOS_DP_VIDEO_CTL_10);
> 
>  	reg = readl(dp->reg_base + EXYNOS_DP_VIDEO_CTL_10);
>  	reg &= ~VSYNC_POLARITY_CFG;
> -	reg |= (video_info->v_sync_polarity << 1);
> +	reg |= (dp->video_info->v_sync_polarity << 1);
>  	writel(reg, dp->reg_base + EXYNOS_DP_VIDEO_CTL_10);
> 
>  	reg = readl(dp->reg_base + EXYNOS_DP_VIDEO_CTL_10);
>  	reg &= ~HSYNC_POLARITY_CFG;
> -	reg |= (video_info->h_sync_polarity << 0);
> +	reg |= (dp->video_info->h_sync_polarity << 0);
>  	writel(reg, dp->reg_base + EXYNOS_DP_VIDEO_CTL_10);
> 
>  	reg = AUDIO_MODE_SPDIF_MODE | VIDEO_MODE_SLAVE_MODE;
> --
> 1.7.0.4


^ permalink raw reply

* Re: [PATCH 2/2] video: exynos_dp: move exynos_dp_config_video to a workqueue
From: Jingoo Han @ 2012-11-08  0:58 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <1352286744-22588-2-git-send-email-ajaykumar.rs@samsung.com>

On Wednesday, November 07, 2012 8:12 PM Ajay Kumar wrote
> 
> This speeds up boot significantly, since it can take up to a second to
> get the display up and going, and we want to keep on booting (through
> some userspace) while that happens.

This patch is not necessary.
The exynos_dp_config_video was already moved by Sean Paul's patch.
(http://www.spinics.net/lists/linux-fbdev/msg08555.html)
This patch including other Sean Paul's patches, will be merged to 3.8-rc1.


> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  drivers/video/exynos/exynos_dp_core.c |   30 ++++++++++++++++++------------
>  drivers/video/exynos/exynos_dp_core.h |    1 +
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index f62778c..ecae6f4 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -18,6 +18,7 @@
>  #include <linux/io.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> +#include <linux/workqueue.h>
> 
>  #include <video/exynos_dp.h>
> 
> @@ -752,19 +753,22 @@ static int exynos_dp_set_link_train(struct exynos_dp_device *dp,
>  	return retval;
>  }
> 
> -static int exynos_dp_config_video(struct exynos_dp_device *dp)
> +static void exynos_dp_config_video(struct work_struct *work)
>  {
> +	struct exynos_dp_device *dp;
>  	int retval = 0;
>  	int timeout_loop = 0;
>  	int done_count = 0;
> 
> +	dp = container_of(work, struct exynos_dp_device, config_work);
> +
>  	exynos_dp_config_video_slave_mode(dp);
> 
>  	exynos_dp_set_video_color_format(dp);
> 
>  	if (exynos_dp_get_pll_lock_status(dp) = PLL_UNLOCKED) {
>  		dev_err(dp->dev, "PLL is not locked yet.\n");
> -		return -EINVAL;
> +		return;
>  	}
> 
>  	for (;;) {
> @@ -773,7 +777,7 @@ static int exynos_dp_config_video(struct exynos_dp_device *dp)
>  			break;
>  		if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>  			dev_err(dp->dev, "Timeout of video streamclk ok\n");
> -			return -ETIMEDOUT;
> +			return;
>  		}
> 
>  		usleep_range(1, 2);
> @@ -807,7 +811,7 @@ static int exynos_dp_config_video(struct exynos_dp_device *dp)
>  		}
>  		if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>  			dev_err(dp->dev, "Timeout of video streamclk ok\n");
> -			return -ETIMEDOUT;
> +			return;
>  		}
> 
>  		usleep_range(1000, 1001);
> @@ -815,8 +819,6 @@ static int exynos_dp_config_video(struct exynos_dp_device *dp)
> 
>  	if (retval != 0)
>  		dev_err(dp->dev, "Video stream is not detected!\n");
> -
> -	return retval;
>  }
> 
>  static void exynos_dp_enable_scramble(struct exynos_dp_device *dp, bool enable)
> @@ -933,11 +935,9 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>  	exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
> 
>  	exynos_dp_init_video(dp);
> -	ret = exynos_dp_config_video(dp);
> -	if (ret) {
> -		dev_err(&pdev->dev, "unable to config video\n");
> -		return ret;
> -	}
> +
> +	INIT_WORK(&dp->config_work, exynos_dp_config_video);
> +	schedule_work(&dp->config_work);
> 
>  	platform_set_drvdata(pdev, dp);
> 
> @@ -949,6 +949,9 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>  	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>  	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> 
> +	if (work_pending(&dp->config_work))
> +		flush_work_sync(&dp->config_work);
> +
>  	if (pdata && pdata->phy_exit)
>  		pdata->phy_exit();
> 
> @@ -964,6 +967,9 @@ static int exynos_dp_suspend(struct device *dev)
>  	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>  	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> 
> +	if (work_pending(&dp->config_work))
> +		flush_work_sync(&dp->config_work);
> +
>  	if (pdata && pdata->phy_exit)
>  		pdata->phy_exit();
> 
> @@ -999,7 +1005,7 @@ static int exynos_dp_resume(struct device *dev)
>  	exynos_dp_set_link_bandwidth(dp, dp->video_info->link_rate);
> 
>  	exynos_dp_init_video(dp);
> -	exynos_dp_config_video(dp);
> +	schedule_work(&dp->config_work);
> 
>  	return 0;
>  }
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 1e646d7..303831d 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -32,6 +32,7 @@ struct exynos_dp_device {
> 
>  	struct video_info	*video_info;
>  	struct link_train	link_train;
> +	struct work_struct	config_work;
>  };
> 
>  /* exynos_dp_reg.c */
> --
> 1.7.0.4


^ permalink raw reply

* Re: [PATCH 12/12] OMAPDSS: DPI: always use DSI PLL if available
From: Tomi Valkeinen @ 2012-11-08  7:39 UTC (permalink / raw)
  To: Rob Clark
  Cc: Tomi Valkeinen, Archit Taneja, linux-omap, linux-fbdev, dri-devel
In-Reply-To: <CAF6AEGsvRV=ez_aqz_tkb+azNMy4jQuSVdd7oKg6=nTnguX-Kw@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 4910 bytes --]

On 2012-11-07 21:18, Rob Clark wrote:
> On Wed, Nov 7, 2012 at 9:13 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On 2012-11-07 16:32, Rob Clark wrote:
>>> On Wed, Nov 7, 2012 at 4:01 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>>>> Hotplugging is not some abstract future scenario, we already have
>>>> hardware that could use it. For example, omap3 SDP board has a
>>>> switchable output to DVI or LCD panel. In this case we know what the two
>>>> options are, but the disabled component is still effectually removed
>>>> from the system, and plugged back in when it's enabled.
>>>
>>> I would look at this as two different connectors which can not be used
>>> at the same time.  You have this scenario with desktop graphics cards.
>>
>> Yes, that's an option with fixed amount of display devices. But doesn't
>> work for capes.
> 
> Only if capes are hotpluggable.. otherwise probe what cape(s?) are
> present at boot time (is this possible to detect a cape from sw?), and
> create the associated connector(s).

Well, a cape can be anything. For beaglebone they have capes with
eeprom, and you can detect it.

The reason I'd like to have hotplug is that it would simplify panel
drivers in the case where we have multiple possible panels for the same
output, like the DVI/LCD case above for omap3 SDP.

If we don't have hotplug, then both DVI and LCD panel devices are
present at the same time, and they will share resources. In the minimum
they are sharing the video output, but more often than not they share
gpios/powers/etc.

It's normal that a driver will acquire resources for its device in its
probe, and thus we would have two drivers acquiring the same resources
at boot time, leading to the other driver failing. We currently manage
this by acquiring the resources late, only when the panel is being
enabled. But I think that's rather ugly.

It would be much cleaner if the panel device does not exist at all if
the panel is disconnected, and is created only when it is connected.
This of course creates the problem of who is responsible for creating
the panel device, and what triggers it. I think that's case specific,
and for capes, it'd be the cape driver.

But then again, I guess it's acceptable that we don't allow changing the
plugged-in panels at runtime. The user would have to select them with
kernel parameters or such. I guess this would be ok for capes and
development boards. I'm not aware of a production board that would
switch panels at runtime, although I know these were on the table in Nokia.

> Anyways, I think we are stretching a bit hard for use cases for
> hot-pluggable panels..  I just prefer to ignore hotplug for now and
> come back to it when there is a more legitimate use-case.

Ok, fair enough. But let's keep hotplug in mind, and if we're going to
create code that would make hotplug impossible to implement, let's stop
for a moment and think if we can do that in some other way.

>> I'm not so sure. The output (dpi/dsi/hdmi...) is the second step in our
>> chain. The primary "output" is in the DISPC module, the overlay manager.
>> That's where the timings, pixel clock, etc. are programmed. The second
>> step, our output, is really a converter IP. It receives the primary
>> output, converts it and outputs something else. Just like an external
>> converter chip would do.
>>
> 
> the timings, pixel clock, vblank/framedone irqs, that all maps to crtc.
> 
> encoder == converter, so I think this fits.  "An encoder takes pixel
> data from a CRTC and converts it to a format suitable for any attached
> connectors" (from drm docbook)

Oh, ok. Then what you say makes sense. I thought encoder contains the
timings, as you said previously: "Basically the encoder is doing the
"control" stuff (power on/off, set timings, etc)".

In the case we have a long chain of display blocks, encoder could cover
all of them, not just the output block of OMAP? I don't know where the
panel goes, though.

>> And the output can be quite a bit anything. For example, with DBI or DSI
>> command mode outputs we don't have any of the conventional video
>> timings. It doesn't make sense to program, say, video blanking periods
>> to those outputs. But even with DBI and DSI we do have video blanking
>> periods in the DISPC's output, the ovl mgr.
> 
> the encoder has mode_fixup() which can alter the timings that end up
> getting set, which might be a way to account for this.  I guess really
> it should be the panel driver that is telling the encoder what
> adjusted timings to give to the crtc.. so the panel driver doesn't
> quite map to connector.

The panel can't say what timings to give to crtc, it depends on what's
between the crtc and the panel. In case of OMAP DSI, the dsi driver
needs to specify the timings for crtc, based on the panel.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

^ permalink raw reply

* Re: tty, vt: lockdep warnings (Patch v3)
From: Alan Cox @ 2012-11-08 14:34 UTC (permalink / raw)
  To: Bjørn Mork
  Cc: Sasha Levin, Dave Jones, Hugh Dickins, Sasha Levin, Daniel Vetter,
	Greg Kroah-Hartman, Jiri Slaby, linux-kernel, linux-fbdev,
	florianSchandinat
In-Reply-To: <87625h9yoe.fsf@nemi.mork.no>

commit f35b3fbf24c4e4debb6a7a864b09854ccc2a22e7
Author: Alan Cox <alan@linux.intel.com>
Date:   Wed Nov 7 16:35:08 2012 +0000

    fb: Rework locking to fix lock ordering on takeover
    
    Adjust the console layer to allow a take over call where the caller already
    holds the locks. Make the fb layer lock in order.
    
    This s partly a band aid, the fb layer is terminally confused about the
    locking rules it uses for its notifiers it seems.
    
    Signed-off-by: Alan Cox <alan@linux.intel.com>

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index f87d7e8..77bf205 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2984,7 +2984,7 @@ int __init vty_init(const struct file_operations *console_fops)
 
 static struct class *vtconsole_class;
 
-static int bind_con_driver(const struct consw *csw, int first, int last,
+static int do_bind_con_driver(const struct consw *csw, int first, int last,
 			   int deflt)
 {
 	struct module *owner = csw->owner;
@@ -2995,7 +2995,7 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
 	if (!try_module_get(owner))
 		return -ENODEV;
 
-	console_lock();
+	WARN_CONSOLE_UNLOCKED();
 
 	/* check if driver is registered */
 	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
@@ -3080,11 +3080,22 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
 
 	retval = 0;
 err:
-	console_unlock();
 	module_put(owner);
 	return retval;
 };
 
+
+static int bind_con_driver(const struct consw *csw, int first, int last,
+			   int deflt)
+{
+	int ret;
+	
+	console_lock();
+	ret = do_bind_con_driver(csw, first, last, deflt);
+	console_unlock();
+	return ret;
+}
+	
 #ifdef CONFIG_VT_HW_CONSOLE_BINDING
 static int con_is_graphics(const struct consw *csw, int first, int last)
 {
@@ -3196,9 +3207,9 @@ int unbind_con_driver(const struct consw *csw, int first, int last, int deflt)
 	if (!con_is_bound(csw))
 		con_driver->flag &= ~CON_DRIVER_FLAG_INIT;
 
-	console_unlock();
 	/* ignore return value, binding should not fail */
-	bind_con_driver(defcsw, first, last, deflt);
+	do_bind_con_driver(defcsw, first, last, deflt);
+	console_unlock();
 err:
 	module_put(owner);
 	return retval;
@@ -3489,28 +3500,18 @@ int con_debug_leave(void)
 }
 EXPORT_SYMBOL_GPL(con_debug_leave);
 
-/**
- * register_con_driver - register console driver to console layer
- * @csw: console driver
- * @first: the first console to take over, minimum value is 0
- * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
- *
- * DESCRIPTION: This function registers a console driver which can later
- * bind to a range of consoles specified by @first and @last. It will
- * also initialize the console driver by calling con_startup().
- */
-int register_con_driver(const struct consw *csw, int first, int last)
+static int do_register_con_driver(const struct consw *csw, int first, int last)
 {
 	struct module *owner = csw->owner;
 	struct con_driver *con_driver;
 	const char *desc;
 	int i, retval = 0;
 
+	WARN_CONSOLE_UNLOCKED();
+
 	if (!try_module_get(owner))
 		return -ENODEV;
 
-	console_lock();
-
 	for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
 		con_driver = &registered_con_driver[i];
 
@@ -3563,10 +3564,29 @@ int register_con_driver(const struct consw *csw, int first, int last)
 	}
 
 err:
-	console_unlock();
 	module_put(owner);
 	return retval;
 }
+
+/**
+ * register_con_driver - register console driver to console layer
+ * @csw: console driver
+ * @first: the first console to take over, minimum value is 0
+ * @last: the last console to take over, maximum value is MAX_NR_CONSOLES -1
+ *
+ * DESCRIPTION: This function registers a console driver which can later
+ * bind to a range of consoles specified by @first and @last. It will
+ * also initialize the console driver by calling con_startup().
+ */
+int register_con_driver(const struct consw *csw, int first, int last)
+{
+	int retval;
+	
+	console_lock();
+	retval = do_register_con_driver(csw, first, last);
+	console_unlock();
+	return retval;
+}
 EXPORT_SYMBOL(register_con_driver);
 
 /**
@@ -3622,6 +3642,29 @@ EXPORT_SYMBOL(unregister_con_driver);
  *
  *      take_over_console is basically a register followed by unbind
  */
+int do_take_over_console(const struct consw *csw, int first, int last, int deflt)
+{
+	int err;
+
+	err = do_register_con_driver(csw, first, last);
+	/* if we get an busy error we still want to bind the console driver
+	 * and return success, as we may have unbound the console driver
+	 * but not unregistered it.
+	*/
+	if (err = -EBUSY)
+		err = 0;
+	if (!err)
+		do_bind_con_driver(csw, first, last, deflt);
+
+	return err;
+}
+/*
+ *	If we support more console drivers, this function is used
+ *	when a driver wants to take over some existing consoles
+ *	and become default driver for newly opened ones.
+ *
+ *      take_over_console is basically a register followed by unbind
+ */
 int take_over_console(const struct consw *csw, int first, int last, int deflt)
 {
 	int err;
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index fdefa8f..c75f8ce 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -529,6 +529,34 @@ static int search_for_mapped_con(void)
 	return retval;
 }
 
+static int do_fbcon_takeover(int show_logo)
+{
+	int err, i;
+
+	if (!num_registered_fb)
+		return -ENODEV;
+
+	if (!show_logo)
+		logo_shown = FBCON_LOGO_DONTSHOW;
+
+	for (i = first_fb_vc; i <= last_fb_vc; i++)
+		con2fb_map[i] = info_idx;
+
+	err = do_take_over_console(&fb_con, first_fb_vc, last_fb_vc,
+				fbcon_is_default);
+
+	if (err) {
+		for (i = first_fb_vc; i <= last_fb_vc; i++) {
+			con2fb_map[i] = -1;
+		}
+		info_idx = -1;
+	} else {
+		fbcon_has_console_bind = 1;
+	}
+
+	return err;
+}
+
 static int fbcon_takeover(int show_logo)
 {
 	int err, i;
@@ -3115,7 +3143,7 @@ static int fbcon_fb_registered(struct fb_info *info)
 		}
 
 		if (info_idx != -1)
-			ret = fbcon_takeover(1);
+			ret = do_fbcon_takeover(1);
 	} else {
 		for (i = first_fb_vc; i <= last_fb_vc; i++) {
 			if (con2fb_map_boot[i] = idx)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 3ff0105..564ebe9 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1650,7 +1650,9 @@ static int do_register_framebuffer(struct fb_info *fb_info)
 	event.info = fb_info;
 	if (!lock_fb_info(fb_info))
 		return -ENODEV;
+        console_lock();
 	fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
+        console_unlock();
 	unlock_fb_info(fb_info);
 	return 0;
 }
@@ -1853,11 +1855,8 @@ int fb_new_modelist(struct fb_info *info)
 	err = 1;
 
 	if (!list_empty(&info->modelist)) {
-		if (!lock_fb_info(info))
-			return -ENODEV;
 		event.info = info;
 		err = fb_notifier_call_chain(FB_EVENT_NEW_MODELIST, &event);
-		unlock_fb_info(info);
 	}
 
 	return err;
diff --git a/drivers/video/fbsysfs.c b/drivers/video/fbsysfs.c
index a55e366..ef476b0 100644
--- a/drivers/video/fbsysfs.c
+++ b/drivers/video/fbsysfs.c
@@ -177,6 +177,8 @@ static ssize_t store_modes(struct device *device,
 	if (i * sizeof(struct fb_videomode) != count)
 		return -EINVAL;
 
+	if (!lock_fb_info(fb_info))
+		return -ENODEV;
 	console_lock();
 	list_splice(&fb_info->modelist, &old_list);
 	fb_videomode_to_modelist((const struct fb_videomode *)buf, i,
@@ -188,6 +190,7 @@ static ssize_t store_modes(struct device *device,
 		fb_destroy_modelist(&old_list);
 
 	console_unlock();
+	unlock_fb_info(fb_info);
 
 	return 0;
 }
diff --git a/include/linux/console.h b/include/linux/console.h
index dedb082..4ef4307 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -78,6 +78,7 @@ int con_is_bound(const struct consw *csw);
 int register_con_driver(const struct consw *csw, int first, int last);
 int unregister_con_driver(const struct consw *csw);
 int take_over_console(const struct consw *sw, int first, int last, int deflt);
+int do_take_over_console(const struct consw *sw, int first, int last, int deflt);
 void give_up_console(const struct consw *sw);
 #ifdef CONFIG_HW_CONSOLE
 int con_debug_enter(struct vc_data *vc);

^ permalink raw reply related

* Re: [PATCH] video: exynos_dp: Clean up SW link training
From: Sean Paul @ 2012-11-08 21:02 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <1351702475-31324-1-git-send-email-seanpaul@chromium.org>

On Fri, Nov 2, 2012 at 10:45 AM, Sean Paul <seanpaul@chromium.org> wrote:
> On Thu, Nov 1, 2012 at 8:48 PM, Jingoo Han <jg1.han@samsung.com> wrote:
>> On Friday, November 02, 2012 1:15 AM Sean Paul wrote
>>>
>>> On Thu, Nov 1, 2012 at 1:35 AM, Jingoo Han <jg1.han@samsung.com> wrote:
>>> > On Thursday, November 01, 2012 1:55 AM Sean Paul wrote
>>> >>
>>> >> Clean up some of the SW training code to make it more clear and reduce
>>> >> duplicate code.
>>> >>
>>> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> >> ---
>>> >>  drivers/video/exynos/exynos_dp_core.c |  279 +++++++++++++--------------------
>>> >>  1 files changed, 112 insertions(+), 167 deletions(-)
>>> >>
>>> >> Thanks for the pointer. There are still places where the code can be either
>>> >> simplified, or duplication removed.
>>> >
>>> > Removing duplication is good, but don't change the Link training sequence.
>>> > Link training sequence is very sensitive and tricky.
>>> >
>>>
>>> I definitely appreciate how tricky it is :) I didn't actually change
>>> any of the functionality from the original code.
>>
>> No, you changed the procedure when exynos_dp_clock_recovery_ok() fails.
>> It is not the same with exynos_dp_get_adjust_training_lane().
>> So, the else path at exynos_dp_process_clock_recovery() should not be
>> changed.
>>
>>>
>>> I noticed you made a couple of functional changes in your clean-up
>>> patch (http://www.spinics.net/lists/linux-fbdev/msg06849.html). I
>>> assumed that these functional changes were no-ops since bug fixes
>>> would have gone in separate patches.
>>>
>>> I've also done a fair bit of testing to ensure it works.
>>
>> Yes, I know.
>> But, most panels does NOT make the problem,
>> even though there is a bug.
>>
>> With your panel, exynos_dp_clock_recovery_ok() does not fail,
>> so, the problem does NOT happen.
>>
>>>
>>> > I will modify your patch and I will submit new patch.
>>> >
>>>
>>> More comments below.
>>>
>>> > Best regards,
>>> > Jingoo Han
>>> >
>>> >>
>>> >> Below is a rebased patch for your review.
>>> >>
>>> >> Sean
>>> >>
>>> >>
>>> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>>> >> index 44820f2..b126e8a 100644
>>> >> --- a/drivers/video/exynos/exynos_dp_core.c
>>> >> +++ b/drivers/video/exynos/exynos_dp_core.c
>>> >> @@ -276,7 +276,7 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
>>> >>
>>> >>       /* Set sink to D0 (Sink Not Ready) mode. */
>>> >>       retval = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
>>> >> -                             DPCD_SET_POWER_STATE_D0);
>>> >> +                     DPCD_SET_POWER_STATE_D0);
>>> >>       if (retval)
>>> >>               return retval;
>>> >>
>>> >> @@ -301,17 +301,18 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
>>> >>       exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
>>> >>
>>> >>       /* Set RX training pattern */
>>> >> -     exynos_dp_write_byte_to_dpcd(dp,
>>> >> -             DPCD_ADDR_TRAINING_PATTERN_SET,
>>> >> -             DPCD_SCRAMBLING_DISABLED |
>>> >> -             DPCD_TRAINING_PATTERN_1);
>>> >> +     retval = exynos_dp_write_byte_to_dpcd(dp,
>>> >> +                     DPCD_ADDR_TRAINING_PATTERN_SET,
>>> >> +                     DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_1);
>>> >> +     if (retval)
>>> >> +             return retval;
>>> >>
>>> >>       for (lane = 0; lane < lane_count; lane++)
>>> >>               buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
>>> >>                           DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
>>> >> -     retval = exynos_dp_write_bytes_to_dpcd(dp,
>>> >> -             DPCD_ADDR_TRAINING_LANE0_SET,
>>> >> -             lane_count, buf);
>>> >> +
>>> >> +     retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
>>> >> +                     lane_count, buf);
>>> >>
>>> >>       return retval;
>>> >>  }
>>> >> @@ -337,18 +338,17 @@ static int exynos_dp_clock_recovery_ok(u8 link_status[2], int lane_count)
>>> >>       return 0;
>>> >>  }
>>> >>
>>> >> -static int exynos_dp_channel_eq_ok(u8 link_align[3], int lane_count)
>>> >> +static int exynos_dp_channel_eq_ok(u8 link_status[2], u8 link_align,
>>> >> +             int lane_count)
>>> >>  {
>>> >>       int lane;
>>> >> -     u8 lane_align;
>>> >>       u8 lane_status;
>>> >>
>>> >> -     lane_align = link_align[2];
>>> >> -     if ((lane_align & DPCD_INTERLANE_ALIGN_DONE) = 0)
>>> >> +     if ((link_align & DPCD_INTERLANE_ALIGN_DONE) = 0)
>>> >>               return -EINVAL;
>>> >>
>>> >>       for (lane = 0; lane < lane_count; lane++) {
>>> >> -             lane_status = exynos_dp_get_lane_status(link_align, lane);
>>> >> +             lane_status = exynos_dp_get_lane_status(link_status, lane);
>>> >>               lane_status &= DPCD_CHANNEL_EQ_BITS;
>>> >>               if (lane_status != DPCD_CHANNEL_EQ_BITS)
>>> >>                       return -EINVAL;
>>> >> @@ -432,22 +432,47 @@ static void exynos_dp_reduce_link_rate(struct exynos_dp_device *dp)
>>> >>       dp->link_train.lt_state = FAILED;
>>> >>  }
>>> >>
>>> >> +static void exynos_dp_get_adjust_training_lane(struct exynos_dp_device *dp,
>>> >> +                             u8 adjust_request[2])
>>> >> +{
>>> >> +     int lane, lane_count;
>>> >> +     u8 voltage_swing, pre_emphasis, training_lane;
>>> >> +
>>> >> +     lane_count = dp->link_train.lane_count;
>>> >> +     for (lane = 0; lane < lane_count; lane++) {
>>> >> +             voltage_swing = exynos_dp_get_adjust_request_voltage(
>>> >> +                                             adjust_request, lane);
>>> >> +             pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
>>> >> +                                             adjust_request, lane);
>>> >> +             training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
>>> >> +                             DPCD_PRE_EMPHASIS_SET(pre_emphasis);
>>> >> +
>>> >> +             if (voltage_swing = VOLTAGE_LEVEL_3)
>>> >> +                     training_lane |= DPCD_MAX_SWING_REACHED;
>>> >> +             if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
>>> >> +                     training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
>>> >> +
>>> >> +             dp->link_train.training_lane[lane] = training_lane;
>>> >> +     }
>>> >> +}
>>> >> +
>>> >>  static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
>>> >>  {
>>> >> -     u8 link_status[2];
>>> >>       int lane, lane_count, retval;
>>> >> -
>>> >> -     u8 adjust_request[2];
>>> >> -     u8 voltage_swing;
>>> >> -     u8 pre_emphasis;
>>> >> -     u8 training_lane;
>>> >> +     u8 voltage_swing, pre_emphasis, training_lane;
>>> >> +     u8 link_status[2], adjust_request[2];
>>> >>
>>> >>       usleep_range(100, 101);
>>> >>
>>> >>       lane_count = dp->link_train.lane_count;
>>> >>
>>> >>       retval =  exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
>>> >> -                             2, link_status);
>>> >> +                     2, link_status);
>>> >> +     if (retval)
>>> >> +             return retval;
>>> >> +
>>> >> +     retval =  exynos_dp_read_bytes_from_dpcd(dp,
>>> >> +                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
>>> >>       if (retval)
>>> >>               return retval;
>>> >>
>>> >> @@ -455,43 +480,9 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
>>> >>               /* set training pattern 2 for EQ */
>>> >>               exynos_dp_set_training_pattern(dp, TRAINING_PTN2);
>>> >>
>>> >> -             for (lane = 0; lane < lane_count; lane++) {
>>> >> -                     retval = exynos_dp_read_bytes_from_dpcd(dp,
>>> >> -                                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
>>> >> -                                     2, adjust_request);
>>> >> -                     if (retval)
>>> >> -                             return retval;
>>> >> -
>>> >> -                     voltage_swing = exynos_dp_get_adjust_request_voltage(
>>> >> -                                                     adjust_request, lane);
>>> >> -                     pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
>>> >> -                                                     adjust_request, lane);
>>> >> -                     training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
>>> >> -                                     DPCD_PRE_EMPHASIS_SET(pre_emphasis);
>>> >> -
>>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3)
>>> >> -                             training_lane |= DPCD_MAX_SWING_REACHED;
>>> >> -                     if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
>>> >> -                             training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
>>> >> -
>>> >> -                     dp->link_train.training_lane[lane] = training_lane;
>>> >> -
>>> >> -                     exynos_dp_set_lane_link_training(dp,
>>> >> -                             dp->link_train.training_lane[lane],
>>> >> -                             lane);
>>> >> -             }
>>> >> -
>>> >
>>> > Please don't move it to back.
>>> >
>>>
>>> I assume you're talking about the adjust_request read here? I noticed
>>> this was changed in your original clean-up patch
>>> (http://www.spinics.net/lists/linux-fbdev/msg06849.html), but assumed
>>> it was a no-op. What bug does it fix? According to the flowcharts in
>>> the exynos5250 datasheet (figure 49-10 & 49-11), this should be done
>>> *before* setting training pattern 2. Your alteration to my patch will
>>> read it after.
>>>
>>> I also noticed that you added back exynos_dp_get_adjust_training_lane
>>> call here, along with setting DPCD_ADDR_TRAINING_LANE0_SET. You'll
>>> notice that this same code is run in the else path of this function.
>>
>> No, it is not same.
>>
>> 1) your patch
>>         exynos_dp_read_bytes_from_dpcd(dp,
>>                 DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
>>         if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
>>                 ...
>>         } else {
>>                 for (lane = 0; lane < lane_count; lane++) {
>>                         training_lane = exynos_dp_get_lane_link_training()
>>                         voltage_swing = exynos_dp_get_adjust_request_voltage()
>>                         pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis()
>>                         ...
>>
>> 2) my patch
>>         if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
>>                 ...
>>         } else {
>>                 for (lane = 0; lane < lane_count; lane++) {
>>                         training_lane = exynos_dp_get_lane_link_training()
>>                         exynos_dp_read_bytes_from_dpcd(dp,
>>                                 DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
>>                         voltage_swing = exynos_dp_get_adjust_request_voltage()
>>                         pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis()
>>                         ...
>>
>>
>> When the place of reading DPCD_ADDR_ADJUST_REQUEST_LANE0_1 is changed,
>> it makes the Link Fail problem when exynos_dp_clock_recovery_ok() fails.
>> In the case of most panels, exynos_dp_clock_recovery_ok() does not fail.
>> But, some panels failed at exynos_dp_clock_recovery_ok(), and makes the problem
>> if your patch is used.
>>
>
> I must be missing something. exynos_dp_clock_recovery_ok just
> interprets link_status, which is read in the same place in both
> patches. exynos_dp_get_lane_link_training reads from an exynos
> register. I fail to see how either of those operations could affect
> the adjust_request DPCD read.
>
> The following is the order of operations for
> exynos_dp_clock_recovery_ok failure with my patch:
>
> (1) Read lane_status from DPCD
> (2) Read adjust_request from DPCD
> (3) clock_recovery_ok fails
> (4) Get training_lane 0, parse voltage_swing 0 from adjust_request,
> parse pre_emphasis 0 from adjust_request
> (5) Check voltage_swing and pre_emphasis against training_lane,
> increment loop count if nothing changed, exit earlyif max met
> (6) Repeat (4),(5) for lane 1, 2, 3
> (7) Change the value of training_lane to match adjust request read in step (2)
> (8) Write training_ctl to exynos DP register (all lanes)
> (9) Write training_lane back out to DPCD (all lanes)
>
> With your patch (http://www.spinics.net/lists/linux-fbdev/msg08548.html):
>
> (1) Read lane_status from DPCD
> (2) clock_recovery_ok fails
> (3) Get training_lane 0
> (4) Read adjust_request from DPCD
> (5) Parse voltage_swing 0 from adjust_request, parse pre_emphasis 0
> from adjust_request
> (6) Exit early if max met, check voltage_swing and pre_emphasis
> against training_lane, increment loop count if nothing changed
> (7) Change the value of training_lane to match adjust request read in step (4)
> (8) Write training_ctl to exynos DP register for lane 0
> (9) Repeat (3), (4), (5), (6), (7), (8) for lane 1, 2, 3
> (9) Write training_lane back out to DPCD (all lanes)
>
> So unless reading the training lane from DPCD or
> exynos_dp_set_lane_link_training (which writes an exynos register)
> changes the value of the adjust_request read from DPCD, our patches
> are the same :)
>
>> Also, your patch calls exynos_dp_get_adjust_request_voltage() and
>> exynos_dp_get_adjust_request_pre_emphasis() twice,
>> when exynos_dp_clock_recovery_ok() fails. Previously, exynos_dp_clock_recovery_ok()
>> and exynos_dp_get_adjust_request_pre_emphasis() are called only onetime
>> when exynos_dp_clock_recovery_ok() fails.
>> There is no need to call exynos_dp_get_adjust_request_voltage() and
>> exynos_dp_get_adjust_request_pre_emphasis() 'TWICE'.
>>
>
> Yes, it does, but it's just a shift and bitwise-AND... not really
> heavy weight. It would be nice to refactor that code a bit to remove
> the nasty, but that's a different patch.
>
>
>>
>> Anyway, your patch is good and readability is improved.
>> So, I went the extra mile to accept your original patch and
>> add it to v3 patch that I sent.
>> (http://www.spinics.net/lists/linux-fbdev/msg08548.html)
>>
>> But, it is necessary to be careful with some error paths,
>> that is not used at most eDP panels.
>>
>
> I'm still not convinced, but I think we're converging :)
>

Ping.


> Cheers,
>
> Sean
>
>
>
>> Best regards,
>> Jingoo Han
>>
>>> Hence, I removed the duplication and put it all at the bottom. This
>>> improves readability, matches the flowchart more closely, and removes
>>> duplication.
>>>
>>> I'd urge you to please read my patch more carefully and ask questions
>>> if you have any.
>>>
>>> Thanks!
>>>
>>> Sean
>>>
>>> >>               retval = exynos_dp_write_byte_to_dpcd(dp,
>>> >>                       DPCD_ADDR_TRAINING_PATTERN_SET,
>>> >> -                     DPCD_SCRAMBLING_DISABLED |
>>> >> -                     DPCD_TRAINING_PATTERN_2);
>>> >> -             if (retval)
>>> >> -                     return retval;
>>> >> -
>>> >> -             retval = exynos_dp_write_bytes_to_dpcd(dp,
>>> >> -                     DPCD_ADDR_TRAINING_LANE0_SET,
>>> >> -                     lane_count,
>>> >> -                     dp->link_train.training_lane);
>>> >> +                     DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_2);
>>> >>               if (retval)
>>> >>                       return retval;
>>> >>
>>> >> @@ -501,73 +492,49 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
>>> >>               for (lane = 0; lane < lane_count; lane++) {
>>> >>                       training_lane = exynos_dp_get_lane_link_training(
>>> >>                                                       dp, lane);
>>> >> -                     retval = exynos_dp_read_bytes_from_dpcd(dp,
>>> >> -                                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
>>> >> -                                     2, adjust_request);
>>> >> -                     if (retval)
>>> >> -                             return retval;
>>> >> -
>>> >>                       voltage_swing = exynos_dp_get_adjust_request_voltage(
>>> >>                                                       adjust_request, lane);
>>> >>                       pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
>>> >>                                                       adjust_request, lane);
>>> >>
>>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3 ||
>>> >> -                         pre_emphasis = PRE_EMPHASIS_LEVEL_3) {
>>> >> -                             dev_err(dp->dev, "voltage or pre emphasis reached max level\n");
>>> >> -                             goto reduce_link_rate;
>>> >> -                     }
>>> >> -
>>> >> -                     if ((DPCD_VOLTAGE_SWING_GET(training_lane) =
>>> >> -                                     voltage_swing) &&
>>> >> -                        (DPCD_PRE_EMPHASIS_GET(training_lane) =
>>> >> -                                     pre_emphasis)) {
>>> >> +                     if (DPCD_VOLTAGE_SWING_GET(training_lane) =
>>> >> +                                     voltage_swing &&
>>> >> +                         DPCD_PRE_EMPHASIS_GET(training_lane) =
>>> >> +                                     pre_emphasis)
>>> >>                               dp->link_train.cr_loop[lane]++;
>>> >> -                             if (dp->link_train.cr_loop[lane] = MAX_CR_LOOP) {
>>> >> -                                     dev_err(dp->dev, "CR Max loop\n");
>>> >> -                                     goto reduce_link_rate;
>>> >> -                             }
>>> >> -                     }
>>> >>
>>> >> -                     training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
>>> >> -                                     DPCD_PRE_EMPHASIS_SET(pre_emphasis);
>>> >> -
>>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3)
>>> >> -                             training_lane |= DPCD_MAX_SWING_REACHED;
>>> >> -                     if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
>>> >> -                             training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
>>> >> -
>>> >> -                     dp->link_train.training_lane[lane] = training_lane;
>>> >> -
>>> >> -                     exynos_dp_set_lane_link_training(dp,
>>> >> -                             dp->link_train.training_lane[lane], lane);
>>> >> +                     if (dp->link_train.cr_loop[lane] = MAX_CR_LOOP ||
>>> >> +                         voltage_swing = VOLTAGE_LEVEL_3 ||
>>> >> +                         pre_emphasis = PRE_EMPHASIS_LEVEL_3) {
>>> >> +                             dev_err(dp->dev, "CR Max reached (%d,%d,%d)\n",
>>> >> +                                     dp->link_train.cr_loop[lane],
>>> >> +                                     voltage_swing, pre_emphasis);
>>> >> +                             exynos_dp_reduce_link_rate(dp);
>>> >> +                             return -EIO;
>>> >> +                     }
>>> >>               }
>>> >> +     }
>>> >> +
>>> >> +     exynos_dp_get_adjust_training_lane(dp, adjust_request);
>>> >>
>>> >> -             retval = exynos_dp_write_bytes_to_dpcd(dp,
>>> >> -                             DPCD_ADDR_TRAINING_LANE0_SET, lane_count,
>>> >> -                             dp->link_train.training_lane);
>>> >> +     for (lane = 0; lane < lane_count; lane++) {
>>> >> +             exynos_dp_set_lane_link_training(dp,
>>> >> +                     dp->link_train.training_lane[lane], lane);
>>> >> +             retval = exynos_dp_write_byte_to_dpcd(dp,
>>> >> +                     DPCD_ADDR_TRAINING_LANE0_SET + lane,
>>> >> +                     dp->link_train.training_lane[lane]);
>>> >
>>> > The following would be better.
>>> > byte's'_to_dpcd is faster than byte_to_dpcd x 4 times.
>>> >
>>> >         for (lane = 0; lane < lane_count; lane++) {
>>> >                 exynos_dp_set_lane_link_training(dp,
>>> >                         dp->link_train.training_lane[lane], lane);
>>> >         }
>>> >
>>> >         retval = exynos_dp_write_bytes_to_dpcd(dp,
>>> >                         DPCD_ADDR_TRAINING_LANE0_SET, lane_count,
>>> >                         dp->link_train.training_lane);
>>> >
>>>
>>>
>>> Makes sense, that's a good change.
>>>
>>>
>>> >>               if (retval)
>>> >>                       return retval;
>>> >>       }
>>> >>
>>> >>       return retval;
>>> >> -
>>> >> -reduce_link_rate:
>>> >> -     exynos_dp_reduce_link_rate(dp);
>>> >> -     return -EIO;
>>> >>  }
>>> >>
>>> >>  static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
>>> >>  {
>>> >> -     u8 link_status[2];
>>> >> -     u8 link_align[3];
>>> >>       int lane, lane_count, retval;
>>> >>       u32 reg;
>>> >> -
>>> >> -     u8 adjust_request[2];
>>> >> -     u8 voltage_swing;
>>> >> -     u8 pre_emphasis;
>>> >> -     u8 training_lane;
>>> >> +     u8 link_align, link_status[2], adjust_request[2];
>>> >>
>>> >>       usleep_range(400, 401);
>>> >>
>>> >> @@ -578,85 +545,63 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
>>> >>       if (retval)
>>> >>               return retval;
>>> >>
>>> >> -     if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
>>> >> -             link_align[0] = link_status[0];
>>> >> -             link_align[1] = link_status[1];
>>> >> -
>>> >> -             exynos_dp_read_byte_from_dpcd(dp,
>>> >> -                     DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED,
>>> >> -                     &link_align[2]);
>>> >> -
>>> >> -             for (lane = 0; lane < lane_count; lane++) {
>>> >> -                     retval = exynos_dp_read_bytes_from_dpcd(dp,
>>> >> -                                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
>>> >> -                                     2, adjust_request);
>>> >> -                     if (retval)
>>> >> -                             return retval;
>>> >> +     if (exynos_dp_clock_recovery_ok(link_status, lane_count)) {
>>> >> +             exynos_dp_reduce_link_rate(dp);
>>> >> +             return -EIO;
>>> >> +     }
>>> >>
>>> >> -                     voltage_swing = exynos_dp_get_adjust_request_voltage(
>>> >> -                                                     adjust_request, lane);
>>> >> -                     pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
>>> >> -                                                     adjust_request, lane);
>>> >> -                     training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
>>> >> -                                     DPCD_PRE_EMPHASIS_SET(pre_emphasis);
>>> >> +     retval = exynos_dp_read_bytes_from_dpcd(dp,
>>> >> +                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
>>> >> +     if (retval)
>>> >> +             return retval;
>>> >>
>>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3)
>>> >> -                             training_lane |= DPCD_MAX_SWING_REACHED;
>>> >> -                     if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
>>> >> -                             training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
>>> >> +     retval = exynos_dp_read_byte_from_dpcd(dp,
>>> >> +                     DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED, &link_align);
>>> >> +     if (retval)
>>> >> +             return retval;
>>> >>
>>> >> -                     dp->link_train.training_lane[lane] = training_lane;
>>> >> -             }
>>> >> +     exynos_dp_get_adjust_training_lane(dp, adjust_request);
>>> >>
>>> >> -             if (exynos_dp_channel_eq_ok(link_align, lane_count) = 0) {
>>> >> -                     /* traing pattern Set to Normal */
>>> >> -                     exynos_dp_training_pattern_dis(dp);
>>> >> +     if (!exynos_dp_channel_eq_ok(link_status, link_align, lane_count)) {
>>> >> +             /* traing pattern Set to Normal */
>>> >> +             exynos_dp_training_pattern_dis(dp);
>>> >>
>>> >> -                     dev_info(dp->dev, "Link Training success!\n");
>>> >> +             dev_info(dp->dev, "Link Training success!\n");
>>> >>
>>> >> -                     exynos_dp_get_link_bandwidth(dp, &reg);
>>> >> -                     dp->link_train.link_rate = reg;
>>> >> -                     dev_dbg(dp->dev, "final bandwidth = %.2x\n",
>>> >> -                             dp->link_train.link_rate);
>>> >> +             exynos_dp_get_link_bandwidth(dp, &reg);
>>> >> +             dp->link_train.link_rate = reg;
>>> >> +             dev_dbg(dp->dev, "final bandwidth = %.2x\n",
>>> >> +                     dp->link_train.link_rate);
>>> >>
>>> >> -                     exynos_dp_get_lane_count(dp, &reg);
>>> >> -                     dp->link_train.lane_count = reg;
>>> >> -                     dev_dbg(dp->dev, "final lane count = %.2x\n",
>>> >> -                             dp->link_train.lane_count);
>>> >> +             exynos_dp_get_lane_count(dp, &reg);
>>> >> +             dp->link_train.lane_count = reg;
>>> >> +             dev_dbg(dp->dev, "final lane count = %.2x\n",
>>> >> +                     dp->link_train.lane_count);
>>> >>
>>> >> -                     /* set enhanced mode if available */
>>> >> -                     exynos_dp_set_enhanced_mode(dp);
>>> >> -                     dp->link_train.lt_state = FINISHED;
>>> >> -             } else {
>>> >> -                     /* not all locked */
>>> >> -                     dp->link_train.eq_loop++;
>>> >> +             /* set enhanced mode if available */
>>> >> +             exynos_dp_set_enhanced_mode(dp);
>>> >> +             dp->link_train.lt_state = FINISHED;
>>> >>
>>> >> -                     if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
>>> >> -                             dev_err(dp->dev, "EQ Max loop\n");
>>> >> -                             goto reduce_link_rate;
>>> >> -                     }
>>> >> +             return 0;
>>> >> +     }
>>> >>
>>> >> -                     for (lane = 0; lane < lane_count; lane++)
>>> >> -                             exynos_dp_set_lane_link_training(dp,
>>> >> -                                     dp->link_train.training_lane[lane],
>>> >> -                                     lane);
>>> >> +     /* not all locked */
>>> >> +     dp->link_train.eq_loop++;
>>> >>
>>> >> -                     retval = exynos_dp_write_bytes_to_dpcd(dp,
>>> >> -                                     DPCD_ADDR_TRAINING_LANE0_SET,
>>> >> -                                     lane_count,
>>> >> -                                     dp->link_train.training_lane);
>>> >> -                     if (retval)
>>> >> -                             return retval;
>>> >> -             }
>>> >> -     } else {
>>> >> -             goto reduce_link_rate;
>>> >> +     if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
>>> >> +             dev_err(dp->dev, "EQ Max loop\n");
>>> >> +             exynos_dp_reduce_link_rate(dp);
>>> >> +             return -EIO;
>>> >>       }
>>> >>
>>> >> -     return 0;
>>> >> +     for (lane = 0; lane < lane_count; lane++)
>>> >> +             exynos_dp_set_lane_link_training(dp,
>>> >> +                     dp->link_train.training_lane[lane], lane);
>>> >>
>>> >> -reduce_link_rate:
>>> >> -     exynos_dp_reduce_link_rate(dp);
>>> >> -     return -EIO;
>>> >> +     retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
>>> >> +                     lane_count, dp->link_train.training_lane);
>>> >> +
>>> >> +     return retval;
>>> >>  }
>>> >>
>>> >>  static void exynos_dp_get_max_rx_bandwidth(struct exynos_dp_device *dp,
>>> >> --
>>> >> 1.7.7.3
>>> >
>>

^ permalink raw reply

* Re: [PATCH v7 5/8] fbmon: add videomode helpers
From: Steffen Trumtrar @ 2012-11-08 21:25 UTC (permalink / raw)
  To: Manjunathappa, Prakash
  Cc: devicetree-discuss@lists.ozlabs.org, Rob Herring,
	linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
	linux-media@vger.kernel.org, Valkeinen, Tomi, Stephen Warren,
	kernel@pengutronix.de
In-Reply-To: <A73F36158E33644199EB82C5EC81C7BC3E9E1B39@DBDE01.ent.ti.com>

Hi!

On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
> Hi Steffen,
> 
> On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
> > +#if IS_ENABLED(CONFIG_VIDEOMODE)
> > +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode)
> > +{
> > +	fbmode->xres = vm->hactive;
> > +	fbmode->left_margin = vm->hback_porch;
> > +	fbmode->right_margin = vm->hfront_porch;
> > +	fbmode->hsync_len = vm->hsync_len;
> > +
> > +	fbmode->yres = vm->vactive;
> > +	fbmode->upper_margin = vm->vback_porch;
> > +	fbmode->lower_margin = vm->vfront_porch;
> > +	fbmode->vsync_len = vm->vsync_len;
> > +
> > +	fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> > +
> > +	fbmode->sync = 0;
> > +	fbmode->vmode = 0;
> > +	if (vm->hah)
> > +		fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > +	if (vm->vah)
> > +		fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > +	if (vm->interlaced)
> > +		fbmode->vmode |= FB_VMODE_INTERLACED;
> > +	if (vm->doublescan)
> > +		fbmode->vmode |= FB_VMODE_DOUBLE;
> > +
> 
> "pixelclk-inverted" property of the panel is not percolated fb_videomode.
> Please let me know if I am missing something.
> 

The next version is almost finished. Only thing I'm missing is this.
And I actually do not know which flag would represent an inverted pixelclock
in fb_videomode. Does anybody have any idea what I have to do here?

	if (vm->pixelclk_pol)
		fbmode->sync = ???

That's as far as I have come and I don't see a flag that seems right.
Is this even a valid property of fb_videomode?

Regards,
Steffen

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply

* Re: [PATCH v7 0/8] of: add display helper
From: Rob Herring @ 2012-11-08 21:35 UTC (permalink / raw)
  To: Steffen Trumtrar
  Cc: devicetree-discuss, linux-fbdev, dri-devel, Laurent Pinchart,
	Thierry Reding, Guennady Liakhovetski, linux-media,
	Tomi Valkeinen, Stephen Warren, kernel
In-Reply-To: <1351675689-26814-1-git-send-email-s.trumtrar@pengutronix.de>

On 10/31/2012 04:28 AM, Steffen Trumtrar wrote:
> Hi!
> 
> Finally, v7 of the series.
> 
> Changes since v6:
> 	- get rid of some empty lines etc.
> 	- move functions to their subsystems
> 	- split of_ from non-of_ functions
> 	- add at least some kerneldoc to some functions
> 
> Regards,
> Steffen
> 
> 
> Steffen Trumtrar (8):
>   video: add display_timing struct and helpers
>   of: add helper to parse display timings
>   of: add generic videomode description
>   video: add videomode helpers
>   fbmon: add videomode helpers
>   fbmon: add of_videomode helpers
>   drm_modes: add videomode helpers
>   drm_modes: add of_videomode helpers
> 
>  .../devicetree/bindings/video/display-timings.txt  |  139 +++++++++++++++
>  drivers/gpu/drm/drm_modes.c                        |   78 +++++++++
>  drivers/of/Kconfig                                 |   12 ++
>  drivers/of/Makefile                                |    2 +
>  drivers/of/of_display_timings.c                    |  185 ++++++++++++++++++++
>  drivers/of/of_videomode.c                          |   47 +++++

Not sure why you moved this, but please move this back to drivers/video.
We're trying to move subsystem specific pieces out of drivers/of.

Rob

>  drivers/video/Kconfig                              |   11 ++
>  drivers/video/Makefile                             |    2 +
>  drivers/video/display_timing.c                     |   24 +++
>  drivers/video/fbmon.c                              |   76 ++++++++
>  drivers/video/videomode.c                          |   44 +++++
>  include/drm/drmP.h                                 |    8 +
>  include/linux/display_timing.h                     |   69 ++++++++
>  include/linux/fb.h                                 |    5 +
>  include/linux/of_display_timings.h                 |   20 +++
>  include/linux/of_videomode.h                       |   15 ++
>  include/linux/videomode.h                          |   36 ++++
>  17 files changed, 773 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
>  create mode 100644 drivers/of/of_display_timings.c
>  create mode 100644 drivers/of/of_videomode.c
>  create mode 100644 drivers/video/display_timing.c
>  create mode 100644 drivers/video/videomode.c
>  create mode 100644 include/linux/display_timing.h
>  create mode 100644 include/linux/of_display_timings.h
>  create mode 100644 include/linux/of_videomode.h
>  create mode 100644 include/linux/videomode.h
> 


^ permalink raw reply

* Re: [PATCH] video: exynos_dp: Clean up SW link training
From: Jingoo Han @ 2012-11-09  2:41 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <1351702475-31324-1-git-send-email-seanpaul@chromium.org>

On Friday, November 09, 2012 6:03 AM Sean Paul wrote
> 
> On Fri, Nov 2, 2012 at 10:45 AM, Sean Paul <seanpaul@chromium.org> wrote:
> > On Thu, Nov 1, 2012 at 8:48 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> >> On Friday, November 02, 2012 1:15 AM Sean Paul wrote
> >>>
> >>> On Thu, Nov 1, 2012 at 1:35 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> >>> > On Thursday, November 01, 2012 1:55 AM Sean Paul wrote
> >>> >>
> >>> >> Clean up some of the SW training code to make it more clear and reduce
> >>> >> duplicate code.
> >>> >>
> >>> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> >>> >> ---
> >>> >>  drivers/video/exynos/exynos_dp_core.c |  279 +++++++++++++--------------------
> >>> >>  1 files changed, 112 insertions(+), 167 deletions(-)
> >>> >>
> >>> >> Thanks for the pointer. There are still places where the code can be either
> >>> >> simplified, or duplication removed.
> >>> >
> >>> > Removing duplication is good, but don't change the Link training sequence.
> >>> > Link training sequence is very sensitive and tricky.
> >>> >
> >>>
> >>> I definitely appreciate how tricky it is :) I didn't actually change
> >>> any of the functionality from the original code.
> >>
> >> No, you changed the procedure when exynos_dp_clock_recovery_ok() fails.
> >> It is not the same with exynos_dp_get_adjust_training_lane().
> >> So, the else path at exynos_dp_process_clock_recovery() should not be
> >> changed.
> >>
> >>>
> >>> I noticed you made a couple of functional changes in your clean-up
> >>> patch (http://www.spinics.net/lists/linux-fbdev/msg06849.html). I
> >>> assumed that these functional changes were no-ops since bug fixes
> >>> would have gone in separate patches.
> >>>
> >>> I've also done a fair bit of testing to ensure it works.
> >>
> >> Yes, I know.
> >> But, most panels does NOT make the problem,
> >> even though there is a bug.
> >>
> >> With your panel, exynos_dp_clock_recovery_ok() does not fail,
> >> so, the problem does NOT happen.
> >>
> >>>
> >>> > I will modify your patch and I will submit new patch.
> >>> >
> >>>
> >>> More comments below.
> >>>
> >>> > Best regards,
> >>> > Jingoo Han
> >>> >
> >>> >>
> >>> >> Below is a rebased patch for your review.
> >>> >>
> >>> >> Sean
> >>> >>
> >>> >>
> >>> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> >>> >> index 44820f2..b126e8a 100644
> >>> >> --- a/drivers/video/exynos/exynos_dp_core.c
> >>> >> +++ b/drivers/video/exynos/exynos_dp_core.c
> >>> >> @@ -276,7 +276,7 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
> >>> >>
> >>> >>       /* Set sink to D0 (Sink Not Ready) mode. */
> >>> >>       retval = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
> >>> >> -                             DPCD_SET_POWER_STATE_D0);
> >>> >> +                     DPCD_SET_POWER_STATE_D0);
> >>> >>       if (retval)
> >>> >>               return retval;
> >>> >>
> >>> >> @@ -301,17 +301,18 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
> >>> >>       exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
> >>> >>
> >>> >>       /* Set RX training pattern */
> >>> >> -     exynos_dp_write_byte_to_dpcd(dp,
> >>> >> -             DPCD_ADDR_TRAINING_PATTERN_SET,
> >>> >> -             DPCD_SCRAMBLING_DISABLED |
> >>> >> -             DPCD_TRAINING_PATTERN_1);
> >>> >> +     retval = exynos_dp_write_byte_to_dpcd(dp,
> >>> >> +                     DPCD_ADDR_TRAINING_PATTERN_SET,
> >>> >> +                     DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_1);
> >>> >> +     if (retval)
> >>> >> +             return retval;
> >>> >>
> >>> >>       for (lane = 0; lane < lane_count; lane++)
> >>> >>               buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
> >>> >>                           DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
> >>> >> -     retval = exynos_dp_write_bytes_to_dpcd(dp,
> >>> >> -             DPCD_ADDR_TRAINING_LANE0_SET,
> >>> >> -             lane_count, buf);
> >>> >> +
> >>> >> +     retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
> >>> >> +                     lane_count, buf);
> >>> >>
> >>> >>       return retval;
> >>> >>  }
> >>> >> @@ -337,18 +338,17 @@ static int exynos_dp_clock_recovery_ok(u8 link_status[2], int lane_count)
> >>> >>       return 0;
> >>> >>  }
> >>> >>
> >>> >> -static int exynos_dp_channel_eq_ok(u8 link_align[3], int lane_count)
> >>> >> +static int exynos_dp_channel_eq_ok(u8 link_status[2], u8 link_align,
> >>> >> +             int lane_count)
> >>> >>  {
> >>> >>       int lane;
> >>> >> -     u8 lane_align;
> >>> >>       u8 lane_status;
> >>> >>
> >>> >> -     lane_align = link_align[2];
> >>> >> -     if ((lane_align & DPCD_INTERLANE_ALIGN_DONE) = 0)
> >>> >> +     if ((link_align & DPCD_INTERLANE_ALIGN_DONE) = 0)
> >>> >>               return -EINVAL;
> >>> >>
> >>> >>       for (lane = 0; lane < lane_count; lane++) {
> >>> >> -             lane_status = exynos_dp_get_lane_status(link_align, lane);
> >>> >> +             lane_status = exynos_dp_get_lane_status(link_status, lane);
> >>> >>               lane_status &= DPCD_CHANNEL_EQ_BITS;
> >>> >>               if (lane_status != DPCD_CHANNEL_EQ_BITS)
> >>> >>                       return -EINVAL;
> >>> >> @@ -432,22 +432,47 @@ static void exynos_dp_reduce_link_rate(struct exynos_dp_device *dp)
> >>> >>       dp->link_train.lt_state = FAILED;
> >>> >>  }
> >>> >>
> >>> >> +static void exynos_dp_get_adjust_training_lane(struct exynos_dp_device *dp,
> >>> >> +                             u8 adjust_request[2])
> >>> >> +{
> >>> >> +     int lane, lane_count;
> >>> >> +     u8 voltage_swing, pre_emphasis, training_lane;
> >>> >> +
> >>> >> +     lane_count = dp->link_train.lane_count;
> >>> >> +     for (lane = 0; lane < lane_count; lane++) {
> >>> >> +             voltage_swing = exynos_dp_get_adjust_request_voltage(
> >>> >> +                                             adjust_request, lane);
> >>> >> +             pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
> >>> >> +                                             adjust_request, lane);
> >>> >> +             training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
> >>> >> +                             DPCD_PRE_EMPHASIS_SET(pre_emphasis);
> >>> >> +
> >>> >> +             if (voltage_swing = VOLTAGE_LEVEL_3)
> >>> >> +                     training_lane |= DPCD_MAX_SWING_REACHED;
> >>> >> +             if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
> >>> >> +                     training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
> >>> >> +
> >>> >> +             dp->link_train.training_lane[lane] = training_lane;
> >>> >> +     }
> >>> >> +}
> >>> >> +
> >>> >>  static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
> >>> >>  {
> >>> >> -     u8 link_status[2];
> >>> >>       int lane, lane_count, retval;
> >>> >> -
> >>> >> -     u8 adjust_request[2];
> >>> >> -     u8 voltage_swing;
> >>> >> -     u8 pre_emphasis;
> >>> >> -     u8 training_lane;
> >>> >> +     u8 voltage_swing, pre_emphasis, training_lane;
> >>> >> +     u8 link_status[2], adjust_request[2];
> >>> >>
> >>> >>       usleep_range(100, 101);
> >>> >>
> >>> >>       lane_count = dp->link_train.lane_count;
> >>> >>
> >>> >>       retval =  exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
> >>> >> -                             2, link_status);
> >>> >> +                     2, link_status);
> >>> >> +     if (retval)
> >>> >> +             return retval;
> >>> >> +
> >>> >> +     retval =  exynos_dp_read_bytes_from_dpcd(dp,
> >>> >> +                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
> >>> >>       if (retval)
> >>> >>               return retval;
> >>> >>
> >>> >> @@ -455,43 +480,9 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
> >>> >>               /* set training pattern 2 for EQ */
> >>> >>               exynos_dp_set_training_pattern(dp, TRAINING_PTN2);
> >>> >>
> >>> >> -             for (lane = 0; lane < lane_count; lane++) {
> >>> >> -                     retval = exynos_dp_read_bytes_from_dpcd(dp,
> >>> >> -                                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
> >>> >> -                                     2, adjust_request);
> >>> >> -                     if (retval)
> >>> >> -                             return retval;
> >>> >> -
> >>> >> -                     voltage_swing = exynos_dp_get_adjust_request_voltage(
> >>> >> -                                                     adjust_request, lane);
> >>> >> -                     pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
> >>> >> -                                                     adjust_request, lane);
> >>> >> -                     training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
> >>> >> -                                     DPCD_PRE_EMPHASIS_SET(pre_emphasis);
> >>> >> -
> >>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3)
> >>> >> -                             training_lane |= DPCD_MAX_SWING_REACHED;
> >>> >> -                     if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
> >>> >> -                             training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
> >>> >> -
> >>> >> -                     dp->link_train.training_lane[lane] = training_lane;
> >>> >> -
> >>> >> -                     exynos_dp_set_lane_link_training(dp,
> >>> >> -                             dp->link_train.training_lane[lane],
> >>> >> -                             lane);
> >>> >> -             }
> >>> >> -
> >>> >
> >>> > Please don't move it to back.
> >>> >
> >>>
> >>> I assume you're talking about the adjust_request read here? I noticed
> >>> this was changed in your original clean-up patch
> >>> (http://www.spinics.net/lists/linux-fbdev/msg06849.html), but assumed
> >>> it was a no-op. What bug does it fix? According to the flowcharts in
> >>> the exynos5250 datasheet (figure 49-10 & 49-11), this should be done
> >>> *before* setting training pattern 2. Your alteration to my patch will
> >>> read it after.
> >>>
> >>> I also noticed that you added back exynos_dp_get_adjust_training_lane
> >>> call here, along with setting DPCD_ADDR_TRAINING_LANE0_SET. You'll
> >>> notice that this same code is run in the else path of this function.
> >>
> >> No, it is not same.
> >>
> >> 1) your patch
> >>         exynos_dp_read_bytes_from_dpcd(dp,
> >>                 DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
> >>         if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
> >>                 ...
> >>         } else {
> >>                 for (lane = 0; lane < lane_count; lane++) {
> >>                         training_lane = exynos_dp_get_lane_link_training()
> >>                         voltage_swing = exynos_dp_get_adjust_request_voltage()
> >>                         pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis()
> >>                         ...
> >>
> >> 2) my patch
> >>         if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
> >>                 ...
> >>         } else {
> >>                 for (lane = 0; lane < lane_count; lane++) {
> >>                         training_lane = exynos_dp_get_lane_link_training()
> >>                         exynos_dp_read_bytes_from_dpcd(dp,
> >>                                 DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
> >>                         voltage_swing = exynos_dp_get_adjust_request_voltage()
> >>                         pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis()
> >>                         ...
> >>
> >>
> >> When the place of reading DPCD_ADDR_ADJUST_REQUEST_LANE0_1 is changed,
> >> it makes the Link Fail problem when exynos_dp_clock_recovery_ok() fails.
> >> In the case of most panels, exynos_dp_clock_recovery_ok() does not fail.
> >> But, some panels failed at exynos_dp_clock_recovery_ok(), and makes the problem
> >> if your patch is used.
> >>
> >
> > I must be missing something. exynos_dp_clock_recovery_ok just
> > interprets link_status, which is read in the same place in both
> > patches. exynos_dp_get_lane_link_training reads from an exynos
> > register. I fail to see how either of those operations could affect
> > the adjust_request DPCD read.
> >
> > The following is the order of operations for
> > exynos_dp_clock_recovery_ok failure with my patch:
> >
> > (1) Read lane_status from DPCD
> > (2) Read adjust_request from DPCD
> > (3) clock_recovery_ok fails
> > (4) Get training_lane 0, parse voltage_swing 0 from adjust_request,
> > parse pre_emphasis 0 from adjust_request
> > (5) Check voltage_swing and pre_emphasis against training_lane,
> > increment loop count if nothing changed, exit earlyif max met
> > (6) Repeat (4),(5) for lane 1, 2, 3
> > (7) Change the value of training_lane to match adjust request read in step (2)
> > (8) Write training_ctl to exynos DP register (all lanes)
> > (9) Write training_lane back out to DPCD (all lanes)
> >
> > With your patch (http://www.spinics.net/lists/linux-fbdev/msg08548.html):
> >
> > (1) Read lane_status from DPCD
> > (2) clock_recovery_ok fails
> > (3) Get training_lane 0
> > (4) Read adjust_request from DPCD
> > (5) Parse voltage_swing 0 from adjust_request, parse pre_emphasis 0
> > from adjust_request
> > (6) Exit early if max met, check voltage_swing and pre_emphasis
> > against training_lane, increment loop count if nothing changed
> > (7) Change the value of training_lane to match adjust request read in step (4)
> > (8) Write training_ctl to exynos DP register for lane 0
> > (9) Repeat (3), (4), (5), (6), (7), (8) for lane 1, 2, 3
> > (9) Write training_lane back out to DPCD (all lanes)
> >
> > So unless reading the training lane from DPCD or
> > exynos_dp_set_lane_link_training (which writes an exynos register)
> > changes the value of the adjust_request read from DPCD, our patches
> > are the same :)
> >
> >> Also, your patch calls exynos_dp_get_adjust_request_voltage() and
> >> exynos_dp_get_adjust_request_pre_emphasis() twice,
> >> when exynos_dp_clock_recovery_ok() fails. Previously, exynos_dp_clock_recovery_ok()
> >> and exynos_dp_get_adjust_request_pre_emphasis() are called only onetime
> >> when exynos_dp_clock_recovery_ok() fails.
> >> There is no need to call exynos_dp_get_adjust_request_voltage() and
> >> exynos_dp_get_adjust_request_pre_emphasis() 'TWICE'.
> >>
> >
> > Yes, it does, but it's just a shift and bitwise-AND... not really
> > heavy weight. It would be nice to refactor that code a bit to remove
> > the nasty, but that's a different patch.
> >
> >
> >>
> >> Anyway, your patch is good and readability is improved.
> >> So, I went the extra mile to accept your original patch and
> >> add it to v3 patch that I sent.
> >> (http://www.spinics.net/lists/linux-fbdev/msg08548.html)
> >>
> >> But, it is necessary to be careful with some error paths,
> >> that is not used at most eDP panels.
> >>
> >
> > I'm still not convinced, but I think we're converging :)
> >
> 
> Ping.
> 

OK, I will accept your original patch.
Your original patch seems to be follow the Link Training procedure
of DP standard, more.
But, I will change exynos_dp_write_byte_to_dpcd() x 4 times
to exynos_dp_write_bytes_to_dpcd(), as I mentioned.

I will post v4 patches.
Thank you.


Best regards,
Jingoo Han

> 
> > Cheers,
> >
> > Sean
> >
> >
> >
> >> Best regards,
> >> Jingoo Han
> >>
> >>> Hence, I removed the duplication and put it all at the bottom. This
> >>> improves readability, matches the flowchart more closely, and removes
> >>> duplication.
> >>>
> >>> I'd urge you to please read my patch more carefully and ask questions
> >>> if you have any.
> >>>
> >>> Thanks!
> >>>
> >>> Sean
> >>>
> >>> >>               retval = exynos_dp_write_byte_to_dpcd(dp,
> >>> >>                       DPCD_ADDR_TRAINING_PATTERN_SET,
> >>> >> -                     DPCD_SCRAMBLING_DISABLED |
> >>> >> -                     DPCD_TRAINING_PATTERN_2);
> >>> >> -             if (retval)
> >>> >> -                     return retval;
> >>> >> -
> >>> >> -             retval = exynos_dp_write_bytes_to_dpcd(dp,
> >>> >> -                     DPCD_ADDR_TRAINING_LANE0_SET,
> >>> >> -                     lane_count,
> >>> >> -                     dp->link_train.training_lane);
> >>> >> +                     DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_2);
> >>> >>               if (retval)
> >>> >>                       return retval;
> >>> >>
> >>> >> @@ -501,73 +492,49 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
> >>> >>               for (lane = 0; lane < lane_count; lane++) {
> >>> >>                       training_lane = exynos_dp_get_lane_link_training(
> >>> >>                                                       dp, lane);
> >>> >> -                     retval = exynos_dp_read_bytes_from_dpcd(dp,
> >>> >> -                                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
> >>> >> -                                     2, adjust_request);
> >>> >> -                     if (retval)
> >>> >> -                             return retval;
> >>> >> -
> >>> >>                       voltage_swing = exynos_dp_get_adjust_request_voltage(
> >>> >>                                                       adjust_request, lane);
> >>> >>                       pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
> >>> >>                                                       adjust_request, lane);
> >>> >>
> >>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3 ||
> >>> >> -                         pre_emphasis = PRE_EMPHASIS_LEVEL_3) {
> >>> >> -                             dev_err(dp->dev, "voltage or pre emphasis reached max level\n");
> >>> >> -                             goto reduce_link_rate;
> >>> >> -                     }
> >>> >> -
> >>> >> -                     if ((DPCD_VOLTAGE_SWING_GET(training_lane) =
> >>> >> -                                     voltage_swing) &&
> >>> >> -                        (DPCD_PRE_EMPHASIS_GET(training_lane) =
> >>> >> -                                     pre_emphasis)) {
> >>> >> +                     if (DPCD_VOLTAGE_SWING_GET(training_lane) =
> >>> >> +                                     voltage_swing &&
> >>> >> +                         DPCD_PRE_EMPHASIS_GET(training_lane) =
> >>> >> +                                     pre_emphasis)
> >>> >>                               dp->link_train.cr_loop[lane]++;
> >>> >> -                             if (dp->link_train.cr_loop[lane] = MAX_CR_LOOP) {
> >>> >> -                                     dev_err(dp->dev, "CR Max loop\n");
> >>> >> -                                     goto reduce_link_rate;
> >>> >> -                             }
> >>> >> -                     }
> >>> >>
> >>> >> -                     training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
> >>> >> -                                     DPCD_PRE_EMPHASIS_SET(pre_emphasis);
> >>> >> -
> >>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3)
> >>> >> -                             training_lane |= DPCD_MAX_SWING_REACHED;
> >>> >> -                     if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
> >>> >> -                             training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
> >>> >> -
> >>> >> -                     dp->link_train.training_lane[lane] = training_lane;
> >>> >> -
> >>> >> -                     exynos_dp_set_lane_link_training(dp,
> >>> >> -                             dp->link_train.training_lane[lane], lane);
> >>> >> +                     if (dp->link_train.cr_loop[lane] = MAX_CR_LOOP ||
> >>> >> +                         voltage_swing = VOLTAGE_LEVEL_3 ||
> >>> >> +                         pre_emphasis = PRE_EMPHASIS_LEVEL_3) {
> >>> >> +                             dev_err(dp->dev, "CR Max reached (%d,%d,%d)\n",
> >>> >> +                                     dp->link_train.cr_loop[lane],
> >>> >> +                                     voltage_swing, pre_emphasis);
> >>> >> +                             exynos_dp_reduce_link_rate(dp);
> >>> >> +                             return -EIO;
> >>> >> +                     }
> >>> >>               }
> >>> >> +     }
> >>> >> +
> >>> >> +     exynos_dp_get_adjust_training_lane(dp, adjust_request);
> >>> >>
> >>> >> -             retval = exynos_dp_write_bytes_to_dpcd(dp,
> >>> >> -                             DPCD_ADDR_TRAINING_LANE0_SET, lane_count,
> >>> >> -                             dp->link_train.training_lane);
> >>> >> +     for (lane = 0; lane < lane_count; lane++) {
> >>> >> +             exynos_dp_set_lane_link_training(dp,
> >>> >> +                     dp->link_train.training_lane[lane], lane);
> >>> >> +             retval = exynos_dp_write_byte_to_dpcd(dp,
> >>> >> +                     DPCD_ADDR_TRAINING_LANE0_SET + lane,
> >>> >> +                     dp->link_train.training_lane[lane]);
> >>> >
> >>> > The following would be better.
> >>> > byte's'_to_dpcd is faster than byte_to_dpcd x 4 times.
> >>> >
> >>> >         for (lane = 0; lane < lane_count; lane++) {
> >>> >                 exynos_dp_set_lane_link_training(dp,
> >>> >                         dp->link_train.training_lane[lane], lane);
> >>> >         }
> >>> >
> >>> >         retval = exynos_dp_write_bytes_to_dpcd(dp,
> >>> >                         DPCD_ADDR_TRAINING_LANE0_SET, lane_count,
> >>> >                         dp->link_train.training_lane);
> >>> >
> >>>
> >>>
> >>> Makes sense, that's a good change.
> >>>
> >>>
> >>> >>               if (retval)
> >>> >>                       return retval;
> >>> >>       }
> >>> >>
> >>> >>       return retval;
> >>> >> -
> >>> >> -reduce_link_rate:
> >>> >> -     exynos_dp_reduce_link_rate(dp);
> >>> >> -     return -EIO;
> >>> >>  }
> >>> >>
> >>> >>  static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
> >>> >>  {
> >>> >> -     u8 link_status[2];
> >>> >> -     u8 link_align[3];
> >>> >>       int lane, lane_count, retval;
> >>> >>       u32 reg;
> >>> >> -
> >>> >> -     u8 adjust_request[2];
> >>> >> -     u8 voltage_swing;
> >>> >> -     u8 pre_emphasis;
> >>> >> -     u8 training_lane;
> >>> >> +     u8 link_align, link_status[2], adjust_request[2];
> >>> >>
> >>> >>       usleep_range(400, 401);
> >>> >>
> >>> >> @@ -578,85 +545,63 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
> >>> >>       if (retval)
> >>> >>               return retval;
> >>> >>
> >>> >> -     if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
> >>> >> -             link_align[0] = link_status[0];
> >>> >> -             link_align[1] = link_status[1];
> >>> >> -
> >>> >> -             exynos_dp_read_byte_from_dpcd(dp,
> >>> >> -                     DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED,
> >>> >> -                     &link_align[2]);
> >>> >> -
> >>> >> -             for (lane = 0; lane < lane_count; lane++) {
> >>> >> -                     retval = exynos_dp_read_bytes_from_dpcd(dp,
> >>> >> -                                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
> >>> >> -                                     2, adjust_request);
> >>> >> -                     if (retval)
> >>> >> -                             return retval;
> >>> >> +     if (exynos_dp_clock_recovery_ok(link_status, lane_count)) {
> >>> >> +             exynos_dp_reduce_link_rate(dp);
> >>> >> +             return -EIO;
> >>> >> +     }
> >>> >>
> >>> >> -                     voltage_swing = exynos_dp_get_adjust_request_voltage(
> >>> >> -                                                     adjust_request, lane);
> >>> >> -                     pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
> >>> >> -                                                     adjust_request, lane);
> >>> >> -                     training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
> >>> >> -                                     DPCD_PRE_EMPHASIS_SET(pre_emphasis);
> >>> >> +     retval = exynos_dp_read_bytes_from_dpcd(dp,
> >>> >> +                     DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
> >>> >> +     if (retval)
> >>> >> +             return retval;
> >>> >>
> >>> >> -                     if (voltage_swing = VOLTAGE_LEVEL_3)
> >>> >> -                             training_lane |= DPCD_MAX_SWING_REACHED;
> >>> >> -                     if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
> >>> >> -                             training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
> >>> >> +     retval = exynos_dp_read_byte_from_dpcd(dp,
> >>> >> +                     DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED, &link_align);
> >>> >> +     if (retval)
> >>> >> +             return retval;
> >>> >>
> >>> >> -                     dp->link_train.training_lane[lane] = training_lane;
> >>> >> -             }
> >>> >> +     exynos_dp_get_adjust_training_lane(dp, adjust_request);
> >>> >>
> >>> >> -             if (exynos_dp_channel_eq_ok(link_align, lane_count) = 0) {
> >>> >> -                     /* traing pattern Set to Normal */
> >>> >> -                     exynos_dp_training_pattern_dis(dp);
> >>> >> +     if (!exynos_dp_channel_eq_ok(link_status, link_align, lane_count)) {
> >>> >> +             /* traing pattern Set to Normal */
> >>> >> +             exynos_dp_training_pattern_dis(dp);
> >>> >>
> >>> >> -                     dev_info(dp->dev, "Link Training success!\n");
> >>> >> +             dev_info(dp->dev, "Link Training success!\n");
> >>> >>
> >>> >> -                     exynos_dp_get_link_bandwidth(dp, &reg);
> >>> >> -                     dp->link_train.link_rate = reg;
> >>> >> -                     dev_dbg(dp->dev, "final bandwidth = %.2x\n",
> >>> >> -                             dp->link_train.link_rate);
> >>> >> +             exynos_dp_get_link_bandwidth(dp, &reg);
> >>> >> +             dp->link_train.link_rate = reg;
> >>> >> +             dev_dbg(dp->dev, "final bandwidth = %.2x\n",
> >>> >> +                     dp->link_train.link_rate);
> >>> >>
> >>> >> -                     exynos_dp_get_lane_count(dp, &reg);
> >>> >> -                     dp->link_train.lane_count = reg;
> >>> >> -                     dev_dbg(dp->dev, "final lane count = %.2x\n",
> >>> >> -                             dp->link_train.lane_count);
> >>> >> +             exynos_dp_get_lane_count(dp, &reg);
> >>> >> +             dp->link_train.lane_count = reg;
> >>> >> +             dev_dbg(dp->dev, "final lane count = %.2x\n",
> >>> >> +                     dp->link_train.lane_count);
> >>> >>
> >>> >> -                     /* set enhanced mode if available */
> >>> >> -                     exynos_dp_set_enhanced_mode(dp);
> >>> >> -                     dp->link_train.lt_state = FINISHED;
> >>> >> -             } else {
> >>> >> -                     /* not all locked */
> >>> >> -                     dp->link_train.eq_loop++;
> >>> >> +             /* set enhanced mode if available */
> >>> >> +             exynos_dp_set_enhanced_mode(dp);
> >>> >> +             dp->link_train.lt_state = FINISHED;
> >>> >>
> >>> >> -                     if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
> >>> >> -                             dev_err(dp->dev, "EQ Max loop\n");
> >>> >> -                             goto reduce_link_rate;
> >>> >> -                     }
> >>> >> +             return 0;
> >>> >> +     }
> >>> >>
> >>> >> -                     for (lane = 0; lane < lane_count; lane++)
> >>> >> -                             exynos_dp_set_lane_link_training(dp,
> >>> >> -                                     dp->link_train.training_lane[lane],
> >>> >> -                                     lane);
> >>> >> +     /* not all locked */
> >>> >> +     dp->link_train.eq_loop++;
> >>> >>
> >>> >> -                     retval = exynos_dp_write_bytes_to_dpcd(dp,
> >>> >> -                                     DPCD_ADDR_TRAINING_LANE0_SET,
> >>> >> -                                     lane_count,
> >>> >> -                                     dp->link_train.training_lane);
> >>> >> -                     if (retval)
> >>> >> -                             return retval;
> >>> >> -             }
> >>> >> -     } else {
> >>> >> -             goto reduce_link_rate;
> >>> >> +     if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
> >>> >> +             dev_err(dp->dev, "EQ Max loop\n");
> >>> >> +             exynos_dp_reduce_link_rate(dp);
> >>> >> +             return -EIO;
> >>> >>       }
> >>> >>
> >>> >> -     return 0;
> >>> >> +     for (lane = 0; lane < lane_count; lane++)
> >>> >> +             exynos_dp_set_lane_link_training(dp,
> >>> >> +                     dp->link_train.training_lane[lane], lane);
> >>> >>
> >>> >> -reduce_link_rate:
> >>> >> -     exynos_dp_reduce_link_rate(dp);
> >>> >> -     return -EIO;
> >>> >> +     retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
> >>> >> +                     lane_count, dp->link_train.training_lane);
> >>> >> +
> >>> >> +     return retval;
> >>> >>  }
> >>> >>
> >>> >>  static void exynos_dp_get_max_rx_bandwidth(struct exynos_dp_device *dp,
> >>> >> --
> >>> >> 1.7.7.3
> >>> >
> >>


^ permalink raw reply

* [PATCH v4 1/8] video: exynos_dp: Check DPCD return codes
From: Jingoo Han @ 2012-11-09  5:48 UTC (permalink / raw)
  To: linux-fbdev

From: Sean Paul <seanpaul@chromium.org>

Add return code checks to the DPCD transactions in the SW link training

Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/exynos/exynos_dp_core.c |   86 +++++++++++++++++++++-----------
 1 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index d55470e..44820f2 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -261,11 +261,10 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
 	}
 }
 
-static void exynos_dp_link_start(struct exynos_dp_device *dp)
+static int exynos_dp_link_start(struct exynos_dp_device *dp)
 {
 	u8 buf[4];
-	int lane;
-	int lane_count;
+	int lane, lane_count, retval;
 
 	lane_count = dp->link_train.lane_count;
 
@@ -276,8 +275,10 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
 		dp->link_train.cr_loop[lane] = 0;
 
 	/* Set sink to D0 (Sink Not Ready) mode. */
-	exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
+	retval = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
 				DPCD_SET_POWER_STATE_D0);
+	if (retval)
+		return retval;
 
 	/* Set link rate and count as you want to establish*/
 	exynos_dp_set_link_bandwidth(dp, dp->link_train.link_rate);
@@ -286,8 +287,10 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
 	/* Setup RX configuration */
 	buf[0] = dp->link_train.link_rate;
 	buf[1] = dp->link_train.lane_count;
-	exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
+	retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_LINK_BW_SET,
 				2, buf);
+	if (retval)
+		return retval;
 
 	/* Set TX pre-emphasis to minimum */
 	for (lane = 0; lane < lane_count; lane++)
@@ -306,9 +309,11 @@ static void exynos_dp_link_start(struct exynos_dp_device *dp)
 	for (lane = 0; lane < lane_count; lane++)
 		buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
 			    DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
-	exynos_dp_write_bytes_to_dpcd(dp,
+	retval = exynos_dp_write_bytes_to_dpcd(dp,
 		DPCD_ADDR_TRAINING_LANE0_SET,
 		lane_count, buf);
+
+	return retval;
 }
 
 static unsigned char exynos_dp_get_lane_status(u8 link_status[2], int lane)
@@ -430,8 +435,7 @@ static void exynos_dp_reduce_link_rate(struct exynos_dp_device *dp)
 static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 {
 	u8 link_status[2];
-	int lane;
-	int lane_count;
+	int lane, lane_count, retval;
 
 	u8 adjust_request[2];
 	u8 voltage_swing;
@@ -442,17 +446,22 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 
 	lane_count = dp->link_train.lane_count;
 
-	exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
+	retval =  exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
 				2, link_status);
+	if (retval)
+		return retval;
 
 	if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
 		/* set training pattern 2 for EQ */
 		exynos_dp_set_training_pattern(dp, TRAINING_PTN2);
 
 		for (lane = 0; lane < lane_count; lane++) {
-			exynos_dp_read_bytes_from_dpcd(dp,
+			retval = exynos_dp_read_bytes_from_dpcd(dp,
 					DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
 					2, adjust_request);
+			if (retval)
+				return retval;
+
 			voltage_swing = exynos_dp_get_adjust_request_voltage(
 							adjust_request, lane);
 			pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
@@ -472,15 +481,19 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 				lane);
 		}
 
-		exynos_dp_write_byte_to_dpcd(dp,
+		retval = exynos_dp_write_byte_to_dpcd(dp,
 			DPCD_ADDR_TRAINING_PATTERN_SET,
 			DPCD_SCRAMBLING_DISABLED |
 			DPCD_TRAINING_PATTERN_2);
+		if (retval)
+			return retval;
 
-		exynos_dp_write_bytes_to_dpcd(dp,
+		retval = exynos_dp_write_bytes_to_dpcd(dp,
 			DPCD_ADDR_TRAINING_LANE0_SET,
 			lane_count,
 			dp->link_train.training_lane);
+		if (retval)
+			return retval;
 
 		dev_info(dp->dev, "Link Training Clock Recovery success\n");
 		dp->link_train.lt_state = EQUALIZER_TRAINING;
@@ -488,9 +501,12 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 		for (lane = 0; lane < lane_count; lane++) {
 			training_lane = exynos_dp_get_lane_link_training(
 							dp, lane);
-			exynos_dp_read_bytes_from_dpcd(dp,
+			retval = exynos_dp_read_bytes_from_dpcd(dp,
 					DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
 					2, adjust_request);
+			if (retval)
+				return retval;
+
 			voltage_swing = exynos_dp_get_adjust_request_voltage(
 							adjust_request, lane);
 			pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
@@ -527,13 +543,14 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 				dp->link_train.training_lane[lane], lane);
 		}
 
-		exynos_dp_write_bytes_to_dpcd(dp,
-			DPCD_ADDR_TRAINING_LANE0_SET,
-			lane_count,
-			dp->link_train.training_lane);
+		retval = exynos_dp_write_bytes_to_dpcd(dp,
+				DPCD_ADDR_TRAINING_LANE0_SET, lane_count,
+				dp->link_train.training_lane);
+		if (retval)
+			return retval;
 	}
 
-	return 0;
+	return retval;
 
 reduce_link_rate:
 	exynos_dp_reduce_link_rate(dp);
@@ -544,8 +561,7 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
 {
 	u8 link_status[2];
 	u8 link_align[3];
-	int lane;
-	int lane_count;
+	int lane, lane_count, retval;
 	u32 reg;
 
 	u8 adjust_request[2];
@@ -557,8 +573,10 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
 
 	lane_count = dp->link_train.lane_count;
 
-	exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
+	retval = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
 				2, link_status);
+	if (retval)
+		return retval;
 
 	if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
 		link_align[0] = link_status[0];
@@ -569,9 +587,12 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
 			&link_align[2]);
 
 		for (lane = 0; lane < lane_count; lane++) {
-			exynos_dp_read_bytes_from_dpcd(dp,
+			retval = exynos_dp_read_bytes_from_dpcd(dp,
 					DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
 					2, adjust_request);
+			if (retval)
+				return retval;
+
 			voltage_swing = exynos_dp_get_adjust_request_voltage(
 							adjust_request, lane);
 			pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
@@ -620,10 +641,12 @@ static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
 					dp->link_train.training_lane[lane],
 					lane);
 
-			exynos_dp_write_bytes_to_dpcd(dp,
-				DPCD_ADDR_TRAINING_LANE0_SET,
-				lane_count,
-				dp->link_train.training_lane);
+			retval = exynos_dp_write_bytes_to_dpcd(dp,
+					DPCD_ADDR_TRAINING_LANE0_SET,
+					lane_count,
+					dp->link_train.training_lane);
+			if (retval)
+				return retval;
 		}
 	} else {
 		goto reduce_link_rate;
@@ -701,16 +724,17 @@ static void exynos_dp_init_training(struct exynos_dp_device *dp,
 
 static int exynos_dp_sw_link_training(struct exynos_dp_device *dp)
 {
-	int retval = 0;
-	int training_finished = 0;
+	int retval = 0, training_finished = 0;
 
 	dp->link_train.lt_state = START;
 
 	/* Process here */
-	while (!training_finished) {
+	while (!retval && !training_finished) {
 		switch (dp->link_train.lt_state) {
 		case START:
-			exynos_dp_link_start(dp);
+			retval = exynos_dp_link_start(dp);
+			if (retval)
+				dev_err(dp->dev, "LT link start failed!\n");
 			break;
 		case CLOCK_RECOVERY:
 			retval = exynos_dp_process_clock_recovery(dp);
@@ -729,6 +753,8 @@ static int exynos_dp_sw_link_training(struct exynos_dp_device *dp)
 			return -EREMOTEIO;
 		}
 	}
+	if (retval)
+		dev_err(dp->dev, "eDP link training failed (%d)\n", retval);
 
 	return retval;
 }
-- 
1.7.1



^ permalink raw reply related

* [PATCH v4 2/8] video: exynos_dp: Clean up SW link training
From: Jingoo Han @ 2012-11-09  5:49 UTC (permalink / raw)
  To: linux-fbdev

From: Sean Paul <seanpaul@chromium.org>

Clean up some of the SW training code to make it more clear and reduce
duplicate code.

[jg1.han@samsung.com: used exynos_dp_write_bytes_to_dpcd()]
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/exynos/exynos_dp_core.c |  292 +++++++++++++-------------------
 1 files changed, 119 insertions(+), 173 deletions(-)

diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 44820f2..8c9bcb9 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -276,7 +276,7 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
 
 	/* Set sink to D0 (Sink Not Ready) mode. */
 	retval = exynos_dp_write_byte_to_dpcd(dp, DPCD_ADDR_SINK_POWER_STATE,
-				DPCD_SET_POWER_STATE_D0);
+			DPCD_SET_POWER_STATE_D0);
 	if (retval)
 		return retval;
 
@@ -301,17 +301,18 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
 	exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
 
 	/* Set RX training pattern */
-	exynos_dp_write_byte_to_dpcd(dp,
-		DPCD_ADDR_TRAINING_PATTERN_SET,
-		DPCD_SCRAMBLING_DISABLED |
-		DPCD_TRAINING_PATTERN_1);
+	retval = exynos_dp_write_byte_to_dpcd(dp,
+			DPCD_ADDR_TRAINING_PATTERN_SET,
+			DPCD_SCRAMBLING_DISABLED | DPCD_TRAINING_PATTERN_1);
+	if (retval)
+		return retval;
 
 	for (lane = 0; lane < lane_count; lane++)
 		buf[lane] = DPCD_PRE_EMPHASIS_PATTERN2_LEVEL0 |
 			    DPCD_VOLTAGE_SWING_PATTERN1_LEVEL0;
-	retval = exynos_dp_write_bytes_to_dpcd(dp,
-		DPCD_ADDR_TRAINING_LANE0_SET,
-		lane_count, buf);
+
+	retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
+			lane_count, buf);
 
 	return retval;
 }
@@ -337,18 +338,17 @@ static int exynos_dp_clock_recovery_ok(u8 link_status[2], int lane_count)
 	return 0;
 }
 
-static int exynos_dp_channel_eq_ok(u8 link_align[3], int lane_count)
+static int exynos_dp_channel_eq_ok(u8 link_status[2], u8 link_align,
+				int lane_count)
 {
 	int lane;
-	u8 lane_align;
 	u8 lane_status;
 
-	lane_align = link_align[2];
-	if ((lane_align & DPCD_INTERLANE_ALIGN_DONE) = 0)
+	if ((link_align & DPCD_INTERLANE_ALIGN_DONE) = 0)
 		return -EINVAL;
 
 	for (lane = 0; lane < lane_count; lane++) {
-		lane_status = exynos_dp_get_lane_status(link_align, lane);
+		lane_status = exynos_dp_get_lane_status(link_status, lane);
 		lane_status &= DPCD_CHANNEL_EQ_BITS;
 		if (lane_status != DPCD_CHANNEL_EQ_BITS)
 			return -EINVAL;
@@ -432,22 +432,47 @@ static void exynos_dp_reduce_link_rate(struct exynos_dp_device *dp)
 	dp->link_train.lt_state = FAILED;
 }
 
+static void exynos_dp_get_adjust_training_lane(struct exynos_dp_device *dp,
+					u8 adjust_request[2])
+{
+	int lane, lane_count;
+	u8 voltage_swing, pre_emphasis, training_lane;
+
+	lane_count = dp->link_train.lane_count;
+	for (lane = 0; lane < lane_count; lane++) {
+		voltage_swing = exynos_dp_get_adjust_request_voltage(
+						adjust_request, lane);
+		pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
+						adjust_request, lane);
+		training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
+				DPCD_PRE_EMPHASIS_SET(pre_emphasis);
+
+		if (voltage_swing = VOLTAGE_LEVEL_3)
+			training_lane |= DPCD_MAX_SWING_REACHED;
+		if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
+			training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
+
+		dp->link_train.training_lane[lane] = training_lane;
+	}
+}
+
 static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 {
-	u8 link_status[2];
 	int lane, lane_count, retval;
-
-	u8 adjust_request[2];
-	u8 voltage_swing;
-	u8 pre_emphasis;
-	u8 training_lane;
+	u8 voltage_swing, pre_emphasis, training_lane;
+	u8 link_status[2], adjust_request[2];
 
 	usleep_range(100, 101);
 
 	lane_count = dp->link_train.lane_count;
 
-	retval =  exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
-				2, link_status);
+	retval =  exynos_dp_read_bytes_from_dpcd(dp,
+			DPCD_ADDR_LANE0_1_STATUS, 2, link_status);
+	if (retval)
+		return retval;
+
+	retval =  exynos_dp_read_bytes_from_dpcd(dp,
+			DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
 	if (retval)
 		return retval;
 
@@ -455,43 +480,10 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 		/* set training pattern 2 for EQ */
 		exynos_dp_set_training_pattern(dp, TRAINING_PTN2);
 
-		for (lane = 0; lane < lane_count; lane++) {
-			retval = exynos_dp_read_bytes_from_dpcd(dp,
-					DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
-					2, adjust_request);
-			if (retval)
-				return retval;
-
-			voltage_swing = exynos_dp_get_adjust_request_voltage(
-							adjust_request, lane);
-			pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
-							adjust_request, lane);
-			training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
-					DPCD_PRE_EMPHASIS_SET(pre_emphasis);
-
-			if (voltage_swing = VOLTAGE_LEVEL_3)
-				training_lane |= DPCD_MAX_SWING_REACHED;
-			if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
-				training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
-
-			dp->link_train.training_lane[lane] = training_lane;
-
-			exynos_dp_set_lane_link_training(dp,
-				dp->link_train.training_lane[lane],
-				lane);
-		}
-
 		retval = exynos_dp_write_byte_to_dpcd(dp,
-			DPCD_ADDR_TRAINING_PATTERN_SET,
-			DPCD_SCRAMBLING_DISABLED |
-			DPCD_TRAINING_PATTERN_2);
-		if (retval)
-			return retval;
-
-		retval = exynos_dp_write_bytes_to_dpcd(dp,
-			DPCD_ADDR_TRAINING_LANE0_SET,
-			lane_count,
-			dp->link_train.training_lane);
+				DPCD_ADDR_TRAINING_PATTERN_SET,
+				DPCD_SCRAMBLING_DISABLED |
+				DPCD_TRAINING_PATTERN_2);
 		if (retval)
 			return retval;
 
@@ -501,162 +493,116 @@ static int exynos_dp_process_clock_recovery(struct exynos_dp_device *dp)
 		for (lane = 0; lane < lane_count; lane++) {
 			training_lane = exynos_dp_get_lane_link_training(
 							dp, lane);
-			retval = exynos_dp_read_bytes_from_dpcd(dp,
-					DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
-					2, adjust_request);
-			if (retval)
-				return retval;
-
 			voltage_swing = exynos_dp_get_adjust_request_voltage(
 							adjust_request, lane);
 			pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
 							adjust_request, lane);
 
-			if (voltage_swing = VOLTAGE_LEVEL_3 ||
-			    pre_emphasis = PRE_EMPHASIS_LEVEL_3) {
-				dev_err(dp->dev, "voltage or pre emphasis reached max level\n");
-				goto reduce_link_rate;
-			}
-
-			if ((DPCD_VOLTAGE_SWING_GET(training_lane) =
-					voltage_swing) &&
-			   (DPCD_PRE_EMPHASIS_GET(training_lane) =
-					pre_emphasis)) {
+			if (DPCD_VOLTAGE_SWING_GET(training_lane) =
+					voltage_swing &&
+			    DPCD_PRE_EMPHASIS_GET(training_lane) =
+					pre_emphasis)
 				dp->link_train.cr_loop[lane]++;
-				if (dp->link_train.cr_loop[lane] = MAX_CR_LOOP) {
-					dev_err(dp->dev, "CR Max loop\n");
-					goto reduce_link_rate;
-				}
-			}
-
-			training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
-					DPCD_PRE_EMPHASIS_SET(pre_emphasis);
 
-			if (voltage_swing = VOLTAGE_LEVEL_3)
-				training_lane |= DPCD_MAX_SWING_REACHED;
-			if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
-				training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
+			if (dp->link_train.cr_loop[lane] = MAX_CR_LOOP ||
+			    voltage_swing = VOLTAGE_LEVEL_3 ||
+			    pre_emphasis = PRE_EMPHASIS_LEVEL_3) {
+				dev_err(dp->dev, "CR Max reached (%d,%d,%d)\n",
+					dp->link_train.cr_loop[lane],
+					voltage_swing, pre_emphasis);
+				exynos_dp_reduce_link_rate(dp);
+				return -EIO;
+			}
+		}
+	}
 
-			dp->link_train.training_lane[lane] = training_lane;
+	exynos_dp_get_adjust_training_lane(dp, adjust_request);
 
-			exynos_dp_set_lane_link_training(dp,
-				dp->link_train.training_lane[lane], lane);
-		}
+	for (lane = 0; lane < lane_count; lane++)
+		exynos_dp_set_lane_link_training(dp,
+			dp->link_train.training_lane[lane], lane);
 
-		retval = exynos_dp_write_bytes_to_dpcd(dp,
-				DPCD_ADDR_TRAINING_LANE0_SET, lane_count,
-				dp->link_train.training_lane);
-		if (retval)
-			return retval;
-	}
+	retval = exynos_dp_write_bytes_to_dpcd(dp,
+			DPCD_ADDR_TRAINING_LANE0_SET, lane_count,
+			dp->link_train.training_lane);
+	if (retval)
+		return retval;
 
 	return retval;
-
-reduce_link_rate:
-	exynos_dp_reduce_link_rate(dp);
-	return -EIO;
 }
 
 static int exynos_dp_process_equalizer_training(struct exynos_dp_device *dp)
 {
-	u8 link_status[2];
-	u8 link_align[3];
 	int lane, lane_count, retval;
 	u32 reg;
-
-	u8 adjust_request[2];
-	u8 voltage_swing;
-	u8 pre_emphasis;
-	u8 training_lane;
+	u8 link_align, link_status[2], adjust_request[2];
 
 	usleep_range(400, 401);
 
 	lane_count = dp->link_train.lane_count;
 
-	retval = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_LANE0_1_STATUS,
-				2, link_status);
+	retval = exynos_dp_read_bytes_from_dpcd(dp,
+			DPCD_ADDR_LANE0_1_STATUS, 2, link_status);
 	if (retval)
 		return retval;
 
-	if (exynos_dp_clock_recovery_ok(link_status, lane_count) = 0) {
-		link_align[0] = link_status[0];
-		link_align[1] = link_status[1];
-
-		exynos_dp_read_byte_from_dpcd(dp,
-			DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED,
-			&link_align[2]);
-
-		for (lane = 0; lane < lane_count; lane++) {
-			retval = exynos_dp_read_bytes_from_dpcd(dp,
-					DPCD_ADDR_ADJUST_REQUEST_LANE0_1,
-					2, adjust_request);
-			if (retval)
-				return retval;
+	if (exynos_dp_clock_recovery_ok(link_status, lane_count)) {
+		exynos_dp_reduce_link_rate(dp);
+		return -EIO;
+	}
 
-			voltage_swing = exynos_dp_get_adjust_request_voltage(
-							adjust_request, lane);
-			pre_emphasis = exynos_dp_get_adjust_request_pre_emphasis(
-							adjust_request, lane);
-			training_lane = DPCD_VOLTAGE_SWING_SET(voltage_swing) |
-					DPCD_PRE_EMPHASIS_SET(pre_emphasis);
+	retval = exynos_dp_read_bytes_from_dpcd(dp,
+			DPCD_ADDR_ADJUST_REQUEST_LANE0_1, 2, adjust_request);
+	if (retval)
+		return retval;
 
-			if (voltage_swing = VOLTAGE_LEVEL_3)
-				training_lane |= DPCD_MAX_SWING_REACHED;
-			if (pre_emphasis = PRE_EMPHASIS_LEVEL_3)
-				training_lane |= DPCD_MAX_PRE_EMPHASIS_REACHED;
+	retval = exynos_dp_read_byte_from_dpcd(dp,
+			DPCD_ADDR_LANE_ALIGN_STATUS_UPDATED, &link_align);
+	if (retval)
+		return retval;
 
-			dp->link_train.training_lane[lane] = training_lane;
-		}
+	exynos_dp_get_adjust_training_lane(dp, adjust_request);
 
-		if (exynos_dp_channel_eq_ok(link_align, lane_count) = 0) {
-			/* traing pattern Set to Normal */
-			exynos_dp_training_pattern_dis(dp);
+	if (!exynos_dp_channel_eq_ok(link_status, link_align, lane_count)) {
+		/* traing pattern Set to Normal */
+		exynos_dp_training_pattern_dis(dp);
 
-			dev_info(dp->dev, "Link Training success!\n");
+		dev_info(dp->dev, "Link Training success!\n");
 
-			exynos_dp_get_link_bandwidth(dp, &reg);
-			dp->link_train.link_rate = reg;
-			dev_dbg(dp->dev, "final bandwidth = %.2x\n",
-				dp->link_train.link_rate);
+		exynos_dp_get_link_bandwidth(dp, &reg);
+		dp->link_train.link_rate = reg;
+		dev_dbg(dp->dev, "final bandwidth = %.2x\n",
+			dp->link_train.link_rate);
 
-			exynos_dp_get_lane_count(dp, &reg);
-			dp->link_train.lane_count = reg;
-			dev_dbg(dp->dev, "final lane count = %.2x\n",
-				dp->link_train.lane_count);
+		exynos_dp_get_lane_count(dp, &reg);
+		dp->link_train.lane_count = reg;
+		dev_dbg(dp->dev, "final lane count = %.2x\n",
+			dp->link_train.lane_count);
 
-			/* set enhanced mode if available */
-			exynos_dp_set_enhanced_mode(dp);
-			dp->link_train.lt_state = FINISHED;
-		} else {
-			/* not all locked */
-			dp->link_train.eq_loop++;
+		/* set enhanced mode if available */
+		exynos_dp_set_enhanced_mode(dp);
+		dp->link_train.lt_state = FINISHED;
 
-			if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
-				dev_err(dp->dev, "EQ Max loop\n");
-				goto reduce_link_rate;
-			}
+		return 0;
+	}
 
-			for (lane = 0; lane < lane_count; lane++)
-				exynos_dp_set_lane_link_training(dp,
-					dp->link_train.training_lane[lane],
-					lane);
+	/* not all locked */
+	dp->link_train.eq_loop++;
 
-			retval = exynos_dp_write_bytes_to_dpcd(dp,
-					DPCD_ADDR_TRAINING_LANE0_SET,
-					lane_count,
-					dp->link_train.training_lane);
-			if (retval)
-				return retval;
-		}
-	} else {
-		goto reduce_link_rate;
+	if (dp->link_train.eq_loop > MAX_EQ_LOOP) {
+		dev_err(dp->dev, "EQ Max loop\n");
+		exynos_dp_reduce_link_rate(dp);
+		return -EIO;
 	}
 
-	return 0;
+	for (lane = 0; lane < lane_count; lane++)
+		exynos_dp_set_lane_link_training(dp,
+			dp->link_train.training_lane[lane], lane);
 
-reduce_link_rate:
-	exynos_dp_reduce_link_rate(dp);
-	return -EIO;
+	retval = exynos_dp_write_bytes_to_dpcd(dp, DPCD_ADDR_TRAINING_LANE0_SET,
+			lane_count, dp->link_train.training_lane);
+
+	return retval;
 }
 
 static void exynos_dp_get_max_rx_bandwidth(struct exynos_dp_device *dp,
-- 
1.7.1



^ permalink raw reply related

* [PATCH v4 3/8] video: exynos_dp: Get pll lock before pattern set
From: Jingoo Han @ 2012-11-09  5:49 UTC (permalink / raw)
  To: linux-fbdev

From: Sean Paul <seanpaul@chromium.org>

According to the exynos datasheet (Figure 49-10), we should wait for PLL
lock before programming the training pattern when doing software eDP
link training.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/exynos/exynos_dp_core.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 8c9bcb9..6e54a18 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -264,7 +264,7 @@ static void exynos_dp_set_lane_lane_pre_emphasis(struct exynos_dp_device *dp,
 static int exynos_dp_link_start(struct exynos_dp_device *dp)
 {
 	u8 buf[4];
-	int lane, lane_count, retval;
+	int lane, lane_count, pll_tries, retval;
 
 	lane_count = dp->link_train.lane_count;
 
@@ -297,6 +297,18 @@ static int exynos_dp_link_start(struct exynos_dp_device *dp)
 		exynos_dp_set_lane_lane_pre_emphasis(dp,
 			PRE_EMPHASIS_LEVEL_0, lane);
 
+	/* Wait for PLL lock */
+	pll_tries = 0;
+	while (exynos_dp_get_pll_lock_status(dp) = PLL_UNLOCKED) {
+		if (pll_tries = DP_TIMEOUT_LOOP_COUNT) {
+			dev_err(dp->dev, "Wait for PLL lock timed out\n");
+			return -ETIMEDOUT;
+		}
+
+		pll_tries++;
+		usleep_range(90, 120);
+	}
+
 	/* Set training pattern 1 */
 	exynos_dp_set_training_pattern(dp, TRAINING_PTN1);
 
-- 
1.7.1



^ permalink raw reply related

* [PATCH v4 4/8] video: exynos_dp: Improve EDID error handling
From: Jingoo Han @ 2012-11-09  5:50 UTC (permalink / raw)
  To: linux-fbdev

From: Sean Paul <seanpaul@chromium.org>

EDID error handling has 2 problems:
 - It doesn't fail as early as it can
 - The retry counts for i2c and aux transactions are huge

This patch fails if the initial i2c transaction fails, and reduces the
aux and i2c retry counts down to 3.

[jg1.han@samsung.com: reduced the retry count of exynos_dp_read_byte_from_dpcd()]
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/exynos/exynos_dp_core.c |   13 ++++++++-----
 drivers/video/exynos/exynos_dp_reg.c  |   14 ++++++--------
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 6e54a18..4a6ed2f 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -90,9 +90,11 @@ static int exynos_dp_read_edid(struct exynos_dp_device *dp)
 	 */
 
 	/* Read Extension Flag, Number of 128-byte EDID extension blocks */
-	exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
+	retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
 				EDID_EXTENSION_FLAG,
 				&extend_block);
+	if (retval)
+		return retval;
 
 	if (extend_block > 0) {
 		dev_dbg(dp->dev, "EDID data includes a single extension!\n");
@@ -181,14 +183,15 @@ static int exynos_dp_handle_edid(struct exynos_dp_device *dp)
 	int retval;
 
 	/* Read DPCD DPCD_ADDR_DPCD_REV~RECEIVE_PORT1_CAP_1 */
-	exynos_dp_read_bytes_from_dpcd(dp,
-		DPCD_ADDR_DPCD_REV,
-		12, buf);
+	retval = exynos_dp_read_bytes_from_dpcd(dp, DPCD_ADDR_DPCD_REV,
+				12, buf);
+	if (retval)
+		return retval;
 
 	/* Read EDID */
 	for (i = 0; i < 3; i++) {
 		retval = exynos_dp_read_edid(dp);
-		if (retval = 0)
+		if (!retval)
 			break;
 	}
 
diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
index 3f5ca8a..fc19ef6 100644
--- a/drivers/video/exynos/exynos_dp_reg.c
+++ b/drivers/video/exynos/exynos_dp_reg.c
@@ -491,7 +491,7 @@ int exynos_dp_read_byte_from_dpcd(struct exynos_dp_device *dp,
 	int i;
 	int retval;
 
-	for (i = 0; i < 10; i++) {
+	for (i = 0; i < 3; i++) {
 		/* Clear AUX CH data buffer */
 		reg = BUF_CLR;
 		writel(reg, dp->reg_base + EXYNOS_DP_BUFFER_DATA_CTL);
@@ -552,7 +552,7 @@ int exynos_dp_write_bytes_to_dpcd(struct exynos_dp_device *dp,
 		else
 			cur_data_count = count - start_offset;
 
-		for (i = 0; i < 10; i++) {
+		for (i = 0; i < 3; i++) {
 			/* Select DPCD device address */
 			reg = AUX_ADDR_7_0(reg_addr + start_offset);
 			writel(reg, dp->reg_base + EXYNOS_DP_AUX_ADDR_7_0);
@@ -617,7 +617,7 @@ int exynos_dp_read_bytes_from_dpcd(struct exynos_dp_device *dp,
 			cur_data_count = count - start_offset;
 
 		/* AUX CH Request Transaction process */
-		for (i = 0; i < 10; i++) {
+		for (i = 0; i < 3; i++) {
 			/* Select DPCD device address */
 			reg = AUX_ADDR_7_0(reg_addr + start_offset);
 			writel(reg, dp->reg_base + EXYNOS_DP_AUX_ADDR_7_0);
@@ -700,17 +700,15 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
 	int i;
 	int retval;
 
-	for (i = 0; i < 10; i++) {
+	for (i = 0; i < 3; i++) {
 		/* Clear AUX CH data buffer */
 		reg = BUF_CLR;
 		writel(reg, dp->reg_base + EXYNOS_DP_BUFFER_DATA_CTL);
 
 		/* Select EDID device */
 		retval = exynos_dp_select_i2c_device(dp, device_addr, reg_addr);
-		if (retval != 0) {
-			dev_err(dp->dev, "Select EDID device fail!\n");
+		if (retval != 0)
 			continue;
-		}
 
 		/*
 		 * Set I2C transaction and read data
@@ -750,7 +748,7 @@ int exynos_dp_read_bytes_from_i2c(struct exynos_dp_device *dp,
 	int retval = 0;
 
 	for (i = 0; i < count; i += 16) {
-		for (j = 0; j < 100; j++) {
+		for (j = 0; j < 3; j++) {
 			/* Clear AUX CH data buffer */
 			reg = BUF_CLR;
 			writel(reg, dp->reg_base + EXYNOS_DP_BUFFER_DATA_CTL);
-- 
1.7.1



^ permalink raw reply related

page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox