[PATCH] OMAP: DSS2: DSI: Introduce sync_vc functions

[PATCH] OMAP: DSS2: DSI: Introduce sync_vc functions

[archit]

From: Archit Taneja <archit@ti.com>

The DSI protocol engine has no interrupt for signalling the end of a Frame
transfer. The present approach is to send a BTA after DISPC generates a
FRAMEDONE interrupt, and unlock the dsi bus only when the BTA Ack is received.

The assumption made with this approach was that OMAP will send a BTA only after
the long packet corresponding to the last line is sent. However, it is possible
that on the DISPC FRAMEDONE interrupt there are 2 (or more) lines of pixel data
in the DSI line buffer. Hence, the BTA Ack could be received for the long packet
corresponding to the second last line (or the third last and so on..).
Therefore, the current method doesn't ensure that the complete frame data is
sent before we start a new transfer. A similar explanation holds valid if we
send a BTA in between multiple short/long command packets from the slave port.

Introduce dsi_sync_vc functions, based on Tomi Valkeinen's idea, which ensure
that the DSI Virtual Channel in use(update_channel) completes its previous work
before proceeding to the next Frame/Command.

For a frame update, the DSI driver now sends a callback to the Panel Driver
on the FRAMEDONE interrupt itself. The callback in the panel driver then unlocks
the bus. dsi_sync_vc() functions are placed in dsi_vc_config_l4() and
dsi_vc_config_vp() to ensure that the previous task of the Virtual Channel is
completed.

Signed-off-by: Archit Taneja <archit@ti.com>
---
Note:
Applies over the master branch of the tree:
http://gitorious.org/linux-omap-dss2/linux/commits/master

 drivers/video/omap2/dss/dsi.c |  180 ++++++++++++++++++++++++++--------------
 1 files changed, 117 insertions(+), 63 deletions(-)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 5614164..cfbb772 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -2033,6 +2033,114 @@ static int dsi_force_tx_stop_mode_io(void)
 	return 0;
 }
 
+static bool dsi_vc_is_enabled(int channel)
+{
+	return REG_GET(DSI_VC_CTRL(channel), 0, 0);
+}
+
+static void dsi_packet_sent_handler_vp(void *data, u32 mask)
+{
+	const int channel = dsi.update_channel;
+	u8 bit = dsi.te_enabled ? 30 : 31;
+
+	if (REG_GET(DSI_VC_TE(channel), bit, bit) == 0)
+		complete((struct completion *)data);
+}
+
+static int dsi_sync_vc_vp(int channel)
+{
+	int r = 0;
+	u8 bit;
+
+	DECLARE_COMPLETION_ONSTACK(completion);
+
+	bit = dsi.te_enabled ? 30 : 31;
+
+	r = dsi_register_isr_vc(channel, dsi_packet_sent_handler_vp,
+		&completion, DSI_VC_IRQ_PACKET_SENT);
+	if (r)
+		goto err0;
+
+	/* Wait for completion only if TE_EN/TE_START is still set */
+	if (REG_GET(DSI_VC_TE(channel), bit, bit)) {
+		if (wait_for_completion_timeout(&completion,
+				msecs_to_jiffies(10)) == 0) {
+			DSSERR("Failed to complete previous frame transfer\n");
+			r = -EIO;
+			goto err1;
+		}
+	}
+
+	dsi_unregister_isr_vc(channel, dsi_packet_sent_handler_vp,
+		&completion, DSI_VC_IRQ_PACKET_SENT);
+
+	return 0;
+err1:
+	dsi_unregister_isr_vc(channel, dsi_packet_sent_handler_vp, &completion,
+		DSI_VC_IRQ_PACKET_SENT);
+err0:
+	return r;
+}
+
+static void dsi_packet_sent_handler_l4(void *data, u32 mask)
+{
+	const int channel = dsi.update_channel;
+
+	if (REG_GET(DSI_VC_CTRL(channel), 5, 5) == 0)
+		complete((struct completion *)data);
+}
+
+static int dsi_sync_vc_l4(int channel)
+{
+	int r = 0;
+
+	DECLARE_COMPLETION_ONSTACK(completion);
+
+	r = dsi_register_isr_vc(channel, dsi_packet_sent_handler_l4,
+		&completion, DSI_VC_IRQ_PACKET_SENT);
+	if (r)
+		goto err0;
+
+	/* Wait for completion only if TX_FIFO_NOT_EMPTY is still set */
+	if (REG_GET(DSI_VC_CTRL(channel), 5, 5)) {
+		if (wait_for_completion_timeout(&completion,
+				msecs_to_jiffies(10)) == 0) {
+			DSSERR("Failed to complete previous l4 transfer\n");
+			r = -EIO;
+			goto err1;
+		}
+	}
+
+	dsi_unregister_isr_vc(channel, dsi_packet_sent_handler_l4,
+		&completion, DSI_VC_IRQ_PACKET_SENT);
+
+	return 0;
+err1:
+	dsi_unregister_isr_vc(channel, dsi_packet_sent_handler_l4,
+		&completion, DSI_VC_IRQ_PACKET_SENT);
+err0:
+	return r;
+}
+
+static int dsi_sync_vc(int channel)
+{
+	WARN_ON(!dsi_bus_is_locked());
+
+	WARN_ON(in_interrupt());
+
+	if (!dsi_vc_is_enabled(channel))
+		return 0;
+
+	switch (dsi.vc[channel].mode) {
+	case DSI_VC_MODE_VP:
+		return dsi_sync_vc_vp(channel);
+	case DSI_VC_MODE_L4:
+		return dsi_sync_vc_l4(channel);
+	default:
+		BUG();
+	}
+}
+
 static int dsi_vc_enable(int channel, bool enable)
 {
 	DSSDBG("dsi_vc_enable channel %d, enable %d\n",
@@ -2085,6 +2193,8 @@ static int dsi_vc_config_l4(int channel)
 
 	DSSDBGF("%d", channel);
 
+	dsi_sync_vc(channel);
+
 	dsi_vc_enable(channel, 0);
 
 	/* VC_BUSY */
@@ -2113,6 +2223,8 @@ static int dsi_vc_config_vp(int channel)
 
 	DSSDBGF("%d", channel);
 
+	dsi_sync_vc(channel);
+
 	dsi_vc_enable(channel, 0);
 
 	/* VC_BUSY */
@@ -3105,17 +3217,8 @@ static void dsi_te_timeout(unsigned long arg)
 }
 #endif
 
-static void dsi_framedone_bta_callback(void *data, u32 mask);
-
 static void dsi_handle_framedone(int error)
 {
-	const int channel = dsi.update_channel;
-
-	dsi_unregister_isr_vc(channel, dsi_framedone_bta_callback,
-			NULL, DSI_VC_IRQ_BTA);
-
-	cancel_delayed_work(&dsi.framedone_timeout_work);
-
 	/* SIDLEMODE back to smart-idle */
 	dispc_enable_sidle();
 
@@ -3124,14 +3227,6 @@ static void dsi_handle_framedone(int error)
 		REG_FLD_MOD(DSI_TIMING2, 1, 15, 15); /* LP_RX_TO */
 	}
 
-	/* RX_FIFO_NOT_EMPTY */
-	if (REG_GET(DSI_VC_CTRL(channel), 20, 20)) {
-		DSSERR("Received error during frame transfer:\n");
-		dsi_vc_flush_receive_data(channel);
-		if (!error)
-			error = -EIO;
-	}
-
 	dsi.framedone_callback(error, dsi.framedone_data);
 
 	if (!error)
@@ -3152,61 +3247,20 @@ static void dsi_framedone_timeout_work_callback(struct work_struct *work)
 	dsi_handle_framedone(-ETIMEDOUT);
 }
 
-static void dsi_framedone_bta_callback(void *data, u32 mask)
-{
-	dsi_handle_framedone(0);
-
-#ifdef CONFIG_OMAP2_DSS_FAKE_VSYNC
-	dispc_fake_vsync_irq();
-#endif
-}
-
 static void dsi_framedone_irq_callback(void *data, u32 mask)
 {
-	const int channel = dsi.update_channel;
-	int r;
-
 	/* Note: We get FRAMEDONE when DISPC has finished sending pixels and
 	 * turns itself off. However, DSI still has the pixels in its buffers,
 	 * and is sending the data.
 	 */
 
-	if (dsi.te_enabled) {
-		/* enable LP_RX_TO again after the TE */
-		REG_FLD_MOD(DSI_TIMING2, 1, 15, 15); /* LP_RX_TO */
-	}
+	__cancel_delayed_work(&dsi.framedone_timeout_work);
 
-	/* Send BTA after the frame. We need this for the TE to work, as TE
-	 * trigger is only sent for BTAs without preceding packet. Thus we need
-	 * to BTA after the pixel packets so that next BTA will cause TE
-	 * trigger.
-	 *
-	 * This is not needed when TE is not in use, but we do it anyway to
-	 * make sure that the transfer has been completed. It would be more
-	 * optimal, but more complex, to wait only just before starting next
-	 * transfer.
-	 *
-	 * Also, as there's no interrupt telling when the transfer has been
-	 * done and the channel could be reconfigured, the only way is to
-	 * busyloop until TE_SIZE is zero. With BTA we can do this
-	 * asynchronously.
-	 * */
-
-	r = dsi_register_isr_vc(channel, dsi_framedone_bta_callback,
-			NULL, DSI_VC_IRQ_BTA);
-	if (r) {
-		DSSERR("Failed to register BTA ISR\n");
-		dsi_handle_framedone(-EIO);
-		return;
-	}
+	dsi_handle_framedone(0);
 
-	r = dsi_vc_send_bta(channel);
-	if (r) {
-		DSSERR("BTA after framedone failed\n");
-		dsi_unregister_isr_vc(channel, dsi_framedone_bta_callback,
-				NULL, DSI_VC_IRQ_BTA);
-		dsi_handle_framedone(-EIO);
-	}
+#ifdef CONFIG_OMAP2_DSS_FAKE_VSYNC
+	dispc_fake_vsync_irq();
+#endif
 }
 
 int omap_dsi_prepare_update(struct omap_dss_device *dssdev,
-- 
1.7.1

Re: [PATCH] OMAP: DSS2: DSI: Introduce sync_vc functions

[Tomi Valkeinen]

On Wed, 2011-03-23 at 04:59 -0500, Taneja, Archit wrote:
> From: Archit Taneja <archit@ti.com>
> 
> The DSI protocol engine has no interrupt for signalling the end of a Frame
> transfer. The present approach is to send a BTA after DISPC generates a
> FRAMEDONE interrupt, and unlock the dsi bus only when the BTA Ack is received.
> 
> The assumption made with this approach was that OMAP will send a BTA only after
> the long packet corresponding to the last line is sent. However, it is possible
> that on the DISPC FRAMEDONE interrupt there are 2 (or more) lines of pixel data
> in the DSI line buffer. Hence, the BTA Ack could be received for the long packet
> corresponding to the second last line (or the third last and so on..).
> Therefore, the current method doesn't ensure that the complete frame data is
> sent before we start a new transfer. A similar explanation holds valid if we
> send a BTA in between multiple short/long command packets from the slave port.
> 
> Introduce dsi_sync_vc functions, based on Tomi Valkeinen's idea, which ensure
> that the DSI Virtual Channel in use(update_channel) completes its previous work
> before proceeding to the next Frame/Command.
> 
> For a frame update, the DSI driver now sends a callback to the Panel Driver
> on the FRAMEDONE interrupt itself. The callback in the panel driver then unlocks
> the bus. dsi_sync_vc() functions are placed in dsi_vc_config_l4() and
> dsi_vc_config_vp() to ensure that the previous task of the Virtual Channel is
> completed.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> Note:
> Applies over the master branch of the tree:
> http://gitorious.org/linux-omap-dss2/linux/commits/master
> 
>  drivers/video/omap2/dss/dsi.c |  180 ++++++++++++++++++++++++++--------------
>  1 files changed, 117 insertions(+), 63 deletions(-)

Looks good, but this patch introduces one problem: handling the double
BTA for DSI TE.

Currently we aim DSI to be always in a state where one BTA has been
sent. So when the next frame transfer starts, we can just send one BTA
and we know we'll get two consecutive BTAs.

This patch removes the BTA which is sent after the frame transfer, thus
breaking the above system. This doesn't cause problems currently because
panel-taal.c always sends columns and rows addresses before starting the
frame update, and a BTA is sent after those configurations. But a simple
optimization would be to only send column/row config if they have
changed. And in that case we would not get a double BTA, and thus we'd
never get the DSI TE.

So how to solve that...

Two ways come to my mind:
- Track sent BTAs in dsi.c. Every time we send a packet, reset the
counter. Every time we send a BTA, increase the counter. Thus at frame
update we would know if we need to send an extra BTA.

- Always reset the BTA "status" from the panel at the beginning of frame
transfer. This could be done by sending a null packet.

The second one is probably simpler and more failsafe as there's no state
stored. The first one (as well as the current system) would go wrong if
something strange happens, like the panel resets. However, the second
one introduces some overhead, as we need to send a null packet and two
BTAs (versus one BTA) for every frame. It's probably negligible, though.

I think we should go with the second option for now. It's simpler and
works (should, at least). We can study the first option later if we need
to optimize this.

 Tomi

Re: [PATCH] OMAP: DSS2: DSI: Introduce sync_vc functions

[Archit Taneja]

Hi,

On Thursday 24 March 2011 01:18 PM, Valkeinen, Tomi wrote:
> On Wed, 2011-03-23 at 04:59 -0500, Taneja, Archit wrote:
>> From: Archit Taneja<archit@ti.com>
>>
>> The DSI protocol engine has no interrupt for signalling the end of a Frame
>> transfer. The present approach is to send a BTA after DISPC generates a
>> FRAMEDONE interrupt, and unlock the dsi bus only when the BTA Ack is received.
>>
>> The assumption made with this approach was that OMAP will send a BTA only after
>> the long packet corresponding to the last line is sent. However, it is possible
>> that on the DISPC FRAMEDONE interrupt there are 2 (or more) lines of pixel data
>> in the DSI line buffer. Hence, the BTA Ack could be received for the long packet
>> corresponding to the second last line (or the third last and so on..).
>> Therefore, the current method doesn't ensure that the complete frame data is
>> sent before we start a new transfer. A similar explanation holds valid if we
>> send a BTA in between multiple short/long command packets from the slave port.
>>
>> Introduce dsi_sync_vc functions, based on Tomi Valkeinen's idea, which ensure
>> that the DSI Virtual Channel in use(update_channel) completes its previous work
>> before proceeding to the next Frame/Command.
>>
>> For a frame update, the DSI driver now sends a callback to the Panel Driver
>> on the FRAMEDONE interrupt itself. The callback in the panel driver then unlocks
>> the bus. dsi_sync_vc() functions are placed in dsi_vc_config_l4() and
>> dsi_vc_config_vp() to ensure that the previous task of the Virtual Channel is
>> completed.
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>
>> ---
>> Note:
>> Applies over the master branch of the tree:
>> http://gitorious.org/linux-omap-dss2/linux/commits/master
>>
>>   drivers/video/omap2/dss/dsi.c |  180 ++++++++++++++++++++++++++--------------
>>   1 files changed, 117 insertions(+), 63 deletions(-)
>
> Looks good, but this patch introduces one problem: handling the double
> BTA for DSI TE.
>
> Currently we aim DSI to be always in a state where one BTA has been
> sent. So when the next frame transfer starts, we can just send one BTA
> and we know we'll get two consecutive BTAs.
>
> This patch removes the BTA which is sent after the frame transfer, thus
> breaking the above system. This doesn't cause problems currently because
> panel-taal.c always sends columns and rows addresses before starting the
> frame update, and a BTA is sent after those configurations. But a simple
> optimization would be to only send column/row config if they have
> changed. And in that case we would not get a double BTA, and thus we'd
> never get the DSI TE.
>
> So how to solve that...
>
> Two ways come to my mind:
> - Track sent BTAs in dsi.c. Every time we send a packet, reset the
> counter. Every time we send a BTA, increase the counter. Thus at frame
> update we would know if we need to send an extra BTA.
>
> - Always reset the BTA "status" from the panel at the beginning of frame
> transfer. This could be done by sending a null packet.
>
> The second one is probably simpler and more failsafe as there's no state
> stored. The first one (as well as the current system) would go wrong if
> something strange happens, like the panel resets. However, the second
> one introduces some overhead, as we need to send a null packet and two
> BTAs (versus one BTA) for every frame. It's probably negligible, though.

Okay. I agree the second one is a better option. I have a couple of 
queries though:
-The second BTA should be sent only after we get the Ack for the first 
one, i.e, we need to use bta_sync() for the first BTA, right?
-We shouldn't send null packets and the 2 BTAs at all if we aren't using 
Automatic TE mode, is this correct?
-Whose job should it be to send the null packet and the 2 BTAs, the dsi 
driver or the panel driver?

Archit

Re: [PATCH] OMAP: DSS2: DSI: Introduce sync_vc functions

[Tomi Valkeinen]

On Thu, 2011-03-24 at 03:36 -0500, Taneja, Archit wrote:
> Hi,
> 
> On Thursday 24 March 2011 01:18 PM, Valkeinen, Tomi wrote:

> > So how to solve that...
> >
> > Two ways come to my mind:
> > - Track sent BTAs in dsi.c. Every time we send a packet, reset the
> > counter. Every time we send a BTA, increase the counter. Thus at frame
> > update we would know if we need to send an extra BTA.
> >
> > - Always reset the BTA "status" from the panel at the beginning of frame
> > transfer. This could be done by sending a null packet.
> >
> > The second one is probably simpler and more failsafe as there's no state
> > stored. The first one (as well as the current system) would go wrong if
> > something strange happens, like the panel resets. However, the second
> > one introduces some overhead, as we need to send a null packet and two
> > BTAs (versus one BTA) for every frame. It's probably negligible, though.
> 
> Okay. I agree the second one is a better option. I have a couple of 
> queries though:
> -The second BTA should be sent only after we get the Ack for the first 
> one, i.e, we need to use bta_sync() for the first BTA, right?

True.

> -We shouldn't send null packets and the 2 BTAs at all if we aren't using 
> Automatic TE mode, is this correct?

If by automatic TE mode you mean the DSI TEE trigger, then yes. There's
currently check for the dsi.te_enabled in dsi_update_screen_dispc() for
that.

> -Whose job should it be to send the null packet and the 2 BTAs, the dsi 
> driver or the panel driver?

I'd say the dsi driver. It currently sends the one BTA in
dsi_update_screen_dispc(). Adding one null packet and a BTA there should
be quite simple.

 Tomi