[PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[Archit Taneja]

In the case of an error on calling dsi_update_screen_l4(), a
successful framedone callback is still sent to panel-taal. An
error should be returned to taal_update() instead.

Signed-off-by: Archit Taneja <archit@ti.com>
---
v2: This fixes a compilation warning seen after
builing with v1.

v1:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31715.html

 drivers/video/omap2/dss/dsi.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index a6e0f64..b3fa3a7
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -2920,7 +2920,12 @@ int omap_dsi_update(struct omap_dss_device *dssdev,
 
 		dsi_update_screen_dispc(dssdev, x, y, w, h);
 	} else {
-		dsi_update_screen_l4(dssdev, x, y, w, h);
+		int r;
+
+		r = dsi_update_screen_l4(dssdev, x, y, w, h);
+		if (r)
+			return r;
+
 		dsi_perf_show("L4");
 		callback(0, data);
 	}
-- 
1.5.4.7

Re: [PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[Nishanth Menon]

Taneja, Archit had written, on 07/14/2010 07:11 AM, the following:
> In the case of an error on calling dsi_update_screen_l4(), a
> successful framedone callback is still sent to panel-taal. An
> error should be returned to taal_update() instead.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> v2: This fixes a compilation warning seen after
> builing with v1.
> 
> v1:
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31715.html
> 
>  drivers/video/omap2/dss/dsi.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index a6e0f64..b3fa3a7
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -2920,7 +2920,12 @@ int omap_dsi_update(struct omap_dss_device *dssdev,
>  
>  		dsi_update_screen_dispc(dssdev, x, y, w, h);
dont need the error check here?
>  	} else {
> -		dsi_update_screen_l4(dssdev, x, y, w, h);
> +		int r;
> +
> +		r = dsi_update_screen_l4(dssdev, x, y, w, h);
> +		if (r)
no print for warning developer/user? is this error expected?
> +			return r;
> +
>  		dsi_perf_show("L4");
>  		callback(0, data);
>  	}


-- 
Regards,
Nishanth Menon

RE: [PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[Taneja, Archit]

> Taneja, Archit had written, on 07/14/2010 07:11 AM, the following:
> > In the case of an error on calling dsi_update_screen_l4(), a 
> > successful framedone callback is still sent to panel-taal. An error 
> > should be returned to taal_update() instead.
> > 
> > Signed-off-by: Archit Taneja <archit@ti.com>
> > ---
> > v2: This fixes a compilation warning seen after builing with v1.
> > 
> > v1:
> > http://www.mail-archive.com/linux-omap@vger.kernel.org/msg31715.html
> > 
> >  drivers/video/omap2/dss/dsi.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/video/omap2/dss/dsi.c 
> > b/drivers/video/omap2/dss/dsi.c index a6e0f64..b3fa3a7
> > --- a/drivers/video/omap2/dss/dsi.c
> > +++ b/drivers/video/omap2/dss/dsi.c
> > @@ -2920,7 +2920,12 @@ int omap_dsi_update(struct omap_dss_device 
> > *dssdev,
> >  
> >  		dsi_update_screen_dispc(dssdev, x, y, w, h);
> dont need the error check here?

No, there is a scheduled workqueue which notifies a failure if
this function doesn't do what it was supposed to.

> >  	} else {
> > -		dsi_update_screen_l4(dssdev, x, y, w, h);
> > +		int r;
> > +
> > +		r = dsi_update_screen_l4(dssdev, x, y, w, h);
> > +		if (r)
> no print for warning developer/user? is this error expected?

The failure or success of omap_dsi_update() is notified to the display
panel driver(panel-taal) by a callback , if this error check isn't put
here, the panel driver will receive the callback without any error. Hence
it is needed. The error arrives when we don't get the supported pixel size
and color mode.

I am not sure if a print is needed, I will wait for Tomi's comment for that,
this returned value will be checked by upper functuions though. It is the
responsibility of the caller of taal_update to handle the returned value
correctly.

Regards,

Archit

Re: [PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[Tomi Valkeinen]

On Wed, 2010-07-14 at 14:11 +0200, ext Archit Taneja wrote:
> In the case of an error on calling dsi_update_screen_l4(), a
> successful framedone callback is still sent to panel-taal. An
> error should be returned to taal_update() instead.
> 

Looks ok to me, I'll add it to my tree. Are you actually using
dsi_update_screen_l4?

 Tomi

RE: [PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[Taneja, Archit]

> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com] 
> Sent: Tuesday, July 27, 2010 12:50 PM
> To: Taneja, Archit
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH v2] OMAP: DSS2: Fix error path in 
> omap_dsi_update()
> 
> On Wed, 2010-07-14 at 14:11 +0200, ext Archit Taneja wrote:
> > In the case of an error on calling dsi_update_screen_l4(), a 
> > successful framedone callback is still sent to panel-taal. An error 
> > should be returned to taal_update() instead.
> > 
> 
> Looks ok to me, I'll add it to my tree. Are you actually 
> using dsi_update_screen_l4?
> 
>  Tomi

There is a usecase in OMAP4 where we can run 4 displays in
parallel, 3 can be run using the 3 dispc channels, and another DSI
panel can be run using L4 updates.

It would probably be better if we can connect 2 or more peripherals to
one DSI PHY, but with l4 updates we don't need to introduce any hardware changes
on the boards.

So with l4 update we can free one dispc channel. I am not sure how much the
4 display usecase is going to be used. But having l4 updates is still a handy feature,
especially if one wants to run both a DPI and a DSI display on OMAP3 :)

Archit

RE: [PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[Tomi Valkeinen]

On Tue, 2010-07-27 at 09:34 +0200, ext Taneja, Archit wrote:
> >
> > Looks ok to me, I'll add it to my tree. Are you actually 
> > using dsi_update_screen_l4?
> > 
> >  Tomi
> 
> There is a usecase in OMAP4 where we can run 4 displays in
> parallel, 3 can be run using the 3 dispc channels, and another DSI
> panel can be run using L4 updates.

Ok.

> It would probably be better if we can connect 2 or more peripherals to
> one DSI PHY, but with l4 updates we don't need to introduce any hardware changes
> on the boards.
> 
> So with l4 update we can free one dispc channel. I am not sure how much the
> 4 display usecase is going to be used. But having l4 updates is still a handy feature,

The current implementation of dsi_update_screen_l4() is very bad
performance-wise, for throughput and for being basically a busy loop.

Much better would be to use system DMA for the transfer. In theory this
is possible, and I've made some tests with it, but I wasn't able to
solve the problems how to inject DSI commands (write_memory_start and
write_memory_continue), and how to get unpacked 24bit pixels from the
memory changed into packed 24bit pixels for the DSI transfer.

> especially if one wants to run both a DPI and a DSI display on OMAP3 :)

Yes, that's the original reason I implemented it =).

 Tomi

RE: [PATCH v2] OMAP: DSS2: Fix error path in omap_dsi_update()

[Taneja, Archit]

 

> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com] 
> Sent: Tuesday, July 27, 2010 1:14 PM
> To: Taneja, Archit
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH v2] OMAP: DSS2: Fix error path in 
> omap_dsi_update()
> 
> On Tue, 2010-07-27 at 09:34 +0200, ext Taneja, Archit wrote:
> > >
> > > Looks ok to me, I'll add it to my tree. Are you actually using 
> > > dsi_update_screen_l4?
> > > 
> > >  Tomi
> > 
> > There is a usecase in OMAP4 where we can run 4 displays in 
> parallel, 3 
> > can be run using the 3 dispc channels, and another DSI panel can be 
> > run using L4 updates.
> 
> Ok.
> 
> > It would probably be better if we can connect 2 or more 
> peripherals to 
> > one DSI PHY, but with l4 updates we don't need to introduce any 
> > hardware changes on the boards.
> > 
> > So with l4 update we can free one dispc channel. I am not sure how 
> > much the
> > 4 display usecase is going to be used. But having l4 
> updates is still 
> > a handy feature,
> 
> The current implementation of dsi_update_screen_l4() is very 
> bad performance-wise, for throughput and for being basically 
> a busy loop.
> 
> Much better would be to use system DMA for the transfer. In 
> theory this is possible, and I've made some tests with it, 
> but I wasn't able to solve the problems how to inject DSI 
> commands (write_memory_start and write_memory_continue), and 
> how to get unpacked 24bit pixels from the memory changed into 
> packed 24bit pixels for the DSI transfer.

I can try to look up more into this, I have no idea about it's
improvement in terms of performance numbers, but preventing the cpu
load makes it worth it.

Archit