Re: ATI TV Wonder support

On Tuesday 23 November 2004 04:48 pm, John Baldwin wrote:
> On Tuesday 23 November 2004 04:36 pm, John Baldwin wrote:
> > On Tuesday 23 November 2004 03:07 pm, Anish Mistry wrote:
> > > On Tuesday 23 November 2004 02:51 pm, John Baldwin wrote:
> > > > On Tuesday 23 November 2004 02:43 pm, Anish Mistry wrote:
> > > > > On Tuesday 23 November 2004 11:34 am, you wrote:
> > > > > > On Monday 22 November 2004 11:00 pm, Justin Hibbits wrote:
> > > > > > > On Nov 22, 2004, at 22:56, Anish Mistry wrote:
> > > > > > > > On Monday 22 November 2004 09:29 pm, you wrote:
> > > > > > > >> On Nov 22, 2004, at 21:22, Anish Mistry wrote:
> > > > > > > >>> On Sunday 21 November 2004 10:00 pm, Justin Hibbits 
wrote:
> > > > > > > >>>> This patch gives more or less full ATI TV Wonder
> > > > > > > >>>> support to the bktr driver.
> > > > > > > >>>
> > > > > > > >>> Awesome.
> > > > > > > >>>
> > > > > > > >>>> The sound doesn't mute at close, but that might be
> > > > > > > >>>> xawtv's fault, but
> > > > > > > >>>> I
> > > > > > > >>>> don't know which to accuse or look at.  But, other than
> > > > > > > >>>> that, it seems pretty good.
> > > > > > > >>>
> > > > > > > >>> I've having some trouble using it though, the sound
> > > > > > > >>> doesn't seem to work
> > > > > > > >>> just as before.  Is there something extra I need to add
> > > > > > > >>> to my kernel besides bktr?
> > > > > > > >>> On boot:
> > > > > > > >>> bktr0: <BrookTree 878> mem 0xe3101000-0xe3101fff irq 11
> > > > > > > >>> at device 8.0 on
> > > > > > > >>> pci0
> > > > > > > >>> bktr0: [GIANT-LOCKED]
> > > > > > > >>> bktr0: Detected a MSP3445G-B8 at 0x80
> > > > > > > >>> bktr0: ATI TV Wonder, Philips NTSC tuner, msp3400c
> > > > > > > >>> stereo. pci0: <multimedia> at device 8.1 (no driver
> > > > > > > >>> attached) And when I start it:
> > > > > > > >>> bktr0: Detected a MSP3445G-B8 at 0x80
> > > > > > > >>>
> > > > > > > >>> You should also send-pr this if you haven't already.
> > > > > > > >>>
> > > > > > > >>> Thanks,
> > > > > > > >>>
> > > > > > > >>> --
> > > > > > > >>> Anish Mistry
> > > > > > > >>
> > > > > > > >> Oh, you need to build it with the msp3400c driver, so add
> > > > > > > >> the line:
> > > > > > > >>
> > > > > > > >> options BKTR_NEW_MSP34XX_DRIVER
> > > > > > > >>
> > > > > > > >> to your config file.  The card doesn't have any mux, only
> > > > > > > >> that chip, so
> > > > > > > >> you need the driver.  I've found a hackish solution to
> > > > > > > >> the sound problem, by resetting the card on exit.  It's
> > > > > > > >> horrible, so I won't include it in the patch, but it
> > > > > > > >> works for me.  If you want to use this too, add:
> > > > > > > >>
> > > > > > > >>  if (bktr->card.msp3400c )
> > > > > > > >>   msp_dpl_reset( bktr, bktr->msp_addr );
> > > > > > > >>
> > > > > > > >> to bktr_core.c, at line 1171.
> > > > > > > >>
> > > > > > > >>
> > > > > > > >> I'll resend my previous message as a send-pr later, when
> > > > > > > >> I have a little more time to test it.
> > > > > > > >
> > > > > > > > Thanks, but I'm getting a weird panic with 5-STABLE, which
> > > > > > > > I'm assuming you
> > > > > > > > aren't seeing.  Any ideas?
> > > > > > > >
> > > > > > > > panic: sleeping without a mutex
> > > > > > > > KDB: enter: panic
> > > > > > > > [thread 100012]
> > > > > > > > Stopped at kbd_enter+0x2c: leave
> > > > > > > > db> tr
> > > > > > > > kbd_enter(c0635ca1,100,c1b6c54c,c1b41320,0) at
> > > > > > > > kbd_enter+0x2c panic(c06363e5,0,0,0,c1b37c00) at
> > > > > > > > panic+0x10a
> > > > > > > > msleep(c1b6c54c,0,4c,c064007d,0) at msleep+0x2bf
> > > > > > > > msp3410d_thread(c1bd8000,d0181d48,c1db8000,c845e1d4,0) at
> > > > > > > > msp3410d_thread+0x5b
> > > > > > > > fork_exit(c045e1d4,c1db8000,d0181d48) at fork_exit+0x7e
> > > > > > > > fork_trampoline() at fork_trampoline+0x8
> > > > > > > > --- trap 0x1, eip=0, esp=0xd0181d7c, ebd=0 ---
> > > > > > > >
> > > > > > > > dmesg:
> > > > > > > > http://am-productions.biz/docs/bigguy.txt.gz
> > > > > > > > kernel config:
> > > > > > > > http://am-productions.biz/docs/BIGGUY.gz
> > > > > > > > --
> > > > > > > > Anish Mistry
> > > > > > >
> > > > > > > Oh yes, you need to compile it without witness support, or
> > > > > > > invariants (DDB is fine, though).  It, of course, cuts out a
> > > > > > > ton of useful debugging info for other parts, but it's the
> > > > > > > only way, unfortunately.
> > > > > >
> > > > > > That's a bug you'll need to fix or the driver can go to sleep
> > > > > > forever and never wake up.
> > > > >
> > > > > I figured something like.  Do you have any pointers of where to
> > > > > get started and examples of how things like this have been fixed
> > > > > in other drivers?
> > > >
> > > > Well, probably the code is not Giant-safe yet, so you'll need to
> > > > figure out why Giant isn't being held in the driver.
> > >
> > > Would it be better to first make the code Giant-safe, or just go
> > > ahead and add the locking to make it MP-safe?  or are these 2
> > > seperate things?
> >
> > Getting the code to be MP-safe and thus not need Giant is a larger
> > task than just adding some assertions and figuring out why Giant isn't
> > held when it should be.  It maybe that it simply needs D_NEEDSGIANT
> > added to the flags in its cdevsw.
>
> Actually, it looks like the patch changes the code to kick off a new
> kthread and that new kthread is going to need to acquire Giant at the
> start of its main() function and drop it before calling kthread_exit()
> and/or returning.
How about the attached patch?
http://am-productions.biz/docs/msp34xx-giant-locking.c.diff
-- 
Anish Mistry