Re: kernel panic with latest DSS

From: Tomi Valkeinen <tomi.valkeinen@nokia.com>
To: ext Steve Sakoman <sakoman@gmail.com>
Cc: "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>
Subject: Re: kernel panic with latest DSS
Date: Fri, 12 Mar 2010 12:51:33 +0200	[thread overview]
Message-ID: <1268391093.28571.137.camel@tubuntu.research.nokia.com> (raw)
In-Reply-To: <5e088bd91003101148o82a07e2j4df803d23ee78d83@mail.gmail.com>

Can you try this patch?

I haven't run into this earlier as I always have DSI compiled in. It
possibly doesn't behave correctly in that case either, but doesn't
crash.

 Tomi


>From c065bb82e1358105d57d80974ce71d3e5aeabf35 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@nokia.com>
Date: Fri, 12 Mar 2010 12:46:05 +0200
Subject: [PATCH] OMAP: DSS2: initialize dss clk sources properly

Clk sources were not initialized, leading to kernel crash, or possibly to
strange behaviour if DSI was compiled in.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@nokia.com>
---
 drivers/video/omap2/dss/dss.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 8254a42..5434418 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -590,6 +590,9 @@ int dss_init(bool skip_init)
 		}
 	}
 
+	dss.dsi_clk_source = DSS_SRC_DSS1_ALWON_FCLK;
+	dss.dispc_clk_source = DSS_SRC_DSS1_ALWON_FCLK;
+
 	dss_save_context();
 
 	rev = dss_read_reg(DSS_REVISION);
-- 
1.6.5






On Wed, 2010-03-10 at 20:48 +0100, ext Steve Sakoman wrote:
> On Wed, Mar 10, 2010 at 10:58 AM,  <Tomi.Valkeinen@nokia.com> wrote:
> 
> > I haven't had time to look at this, but I don't think your analysis is correct.
> 
> You are correct of course.  My sleepy eyes neglected to notice that
> double = and that makes a big difference :-)
> 
> > What first comes to my mind is that dss.dispc_clk_source is, for some reason, uninitialized. Could you put prints to dss_select_dispc_clk_source to see if it's called, and if so, what is assigned to dss.dispc_clk_source?
> 
> I did a build with that change and it does seem that
> dss_select_dispc_clk_source is never called.
> 
> Steve
> 
> >  Tomi
> >
> > ________________________________________
> > From: ext Steve Sakoman [sakoman@gmail.com]
> > Sent: Wednesday, March 10, 2010 20:42
> > To: linux-omap@vger.kernel.org; Valkeinen Tomi (Nokia-D/Helsinki)
> > Subject: Re: kernel panic with latest DSS
> >
> > On Wed, Mar 10, 2010 at 10:16 AM, Steve Sakoman <sakoman@gmail.com> wrote:
> >
> >> The issue occurs in the following section of code in
> >> drivers/video/omap2/dss/dispc.c:
> >>
> >> unsigned long dispc_fclk_rate(void)
> >> {
> >>        unsigned long r = 0;
> >>
> >>        if (dss_get_dispc_clk_source() == DSS_SRC_DSS1_ALWON_FCLK)
> >>                r = dss_clk_get_rate(DSS_CLK_FCK1);
> >>        else
> >> #ifdef CONFIG_OMAP2_DSS_DSI
> >>                r = dsi_get_dsi1_pll_rate();
> >> #else
> >>        BUG();
> >> #endif
> >>        return r;
> >> }
> >>
> >> Since my machines don't have DSI displays CONFIG_OMAP2_DSS_DSI is not
> >> defined and the BUG() call is triggered.
> >
> > The issue seems to be the test for DSS_SRC_DSS1_ALWON_FCLK.
> >
> > Looking at  drivers/video/omap2/dss/dispc.c I see that
> > DSS_SRC_DSS1_ALWON_FCLK will never be returned since clk_src is always
> > set to 0 or 1 based on a test of DSS_SRC_DSS1_ALWON_FCLK :
> >
> > void dss_select_dispc_clk_source(enum dss_clk_source clk_src)
> > {
> >        int b;
> >
> >        BUG_ON(clk_src != DSS_SRC_DSI1_PLL_FCLK &&
> >                        clk_src != DSS_SRC_DSS1_ALWON_FCLK);
> >
> >        b = clk_src == DSS_SRC_DSS1_ALWON_FCLK ? 0 : 1;
> >
> >        REG_FLD_MOD(DSS_CONTROL, b, 0, 0);      /* DISPC_CLK_SWITCH */
> >
> >        dss.dispc_clk_source = clk_src;
> > }
> >
> > It seems to me that the test should return to the way it was in the
> > previous DSS version, i.e. a test for 0:
> >
> > diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> > index e777e35..be9b3de 100644
> > --- a/drivers/video/omap2/dss/dispc.c
> > +++ b/drivers/video/omap2/dss/dispc.c
> > @@ -2218,7 +2218,7 @@ unsigned long dispc_fclk_rate(void)
> >  {
> >        unsigned long r = 0;
> >
> > -       if (dss_get_dispc_clk_source() == DSS_SRC_DSS1_ALWON_FCLK)
> > +       if (dss_get_dispc_clk_source() == 0)
> >                r = dss_clk_get_rate(DSS_CLK_FCK1);
> >        else
> >  #ifdef CONFIG_OMAP2_DSS_DSI
> >
> > A test build confirms that this change restores my ability to play
> > DVD's and display webcam streams.  My understanding of this code is
> > superficial, so hopefully Tomi can comment on whether this is a good
> > fix or not.
> >
> > Steve


