Dynamic clock cause hard lockup on Radeon 7000 + SMP

[Alex Deucher]

On Tue, 30 Nov 2004 00:33:42 -0500, Kristian Høgsberg <krh at bitplanet.net> wrote:
> Kristian Høgsberg wrote:
> 
> 
> > Alex Deucher wrote:
> >
> >> On Tue, 23 Nov 2004 18:27:44 -0500, Alex Deucher
> >> <alexdeucher at gmail.com> wrote:
> >>
> >>> On Tue, 23 Nov 2004 17:46:24 -0500, Kristian Høgsberg
> >>> <krh at bitplanet.net> wrote:
> >>>
> >>>
> >>>
> >>>>
> >>>> Alex Deucher wrote:
> >>>>
> >>>>> On Tue, 23 Nov 2004 17:25:00 -0500, Kristian Høgsberg
> >>>>> <krh at bitplanet.net> wrote:
> >>>>>
> >>>>>
> >>>>>> Alex Deucher wrote:
> >>>>>>
> >>>>>>
> >>>>>>> On Tue, 23 Nov 2004 16:26:14 -0500, Michel Dänzer
> >>>>>>> <michel at daenzer.net> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> On Tue, 2004-11-23 at 15:06 -0500, Alex Deucher wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> On Tue, 23 Nov 2004 15:01:08 -0500, Alex Deucher
> >>>>>>>>> <alexdeucher at gmail.com> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> On Tue, 23 Nov 2004 14:21:18 -0500, Kristian Høgsberg
> >>>>>>>>>> <krh at bitplanet.net> wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> -            if ( !info->HasCRTC2 ) {
> >>>>>>>>> +           if ((info->ChipFamily == CHIP_FAMILY_RV100) &&
> >>>>>>>>> !(info->IsMobility)) {
> >>>>>>>>> +               break;
> >>>>>>>>> +           } else if ( !info->HasCRTC2 ) {
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> It should probably simply be enabled only on Mobility chips.
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> right now it's not enabled for _any_ chips by default, however, the
> >>>>>>> not enabled case which sets the clocks explicitly to "on" rather
> >>>>>>> than
> >>>>>>> "dynamic" seems to cause lockups for Kristian.  Kristian, does it
> >>>>>>> lock
> >>>>>>> up when you turn dynamicclocks on (sets clocks for dynamic) on your
> >>>>>>> board?
> >>>>>>
> >>>>>>
> >>>>>> Hmm, no, enabling dynamic clocks also makes the crash go away.  I was
> >>>>>> wondering why not enabling dynamic clocks (ie no "DynamicClocks"
> >>>>>> option)
> >>>>>> also need to change the PLL bits. I was expecting that in that
> >>>>>> case it
> >>>>>> could just leave it as is (i.e. 6.7.0 behaviour).  Or in code:
> >>>>>>
> >>>>>>    if (xf86ReturnOptValBool(info->Options, OPTION_DYNAMIC_CLOCKS,
> >>>>>> FALSE)) {
> >>>>>>        RADEONSetDynamicClock(pScrn, 1);
> >>>>>>    }
> >>>>>>
> >>>>>> instead of
> >>>>>>
> >>>>>>    if (xf86ReturnOptValBool(info->Options, OPTION_DYNAMIC_CLOCKS,
> >>>>>> FALSE)) {
> >>>>>>        RADEONSetDynamicClock(pScrn, 1);
> >>>>>>    } else {
> >>>>>>        RADEONSetDynamicClock(pScrn, 0);
> >>>>>>    }
> >>>>>
> >>>>>
> >>>>> the problem was that certain asics apparently need to have their
> >>>>> clocks forced on rather than left in their default configuration.  the
> >>>>> old 6.7 code forced on several clocks in the accel init code in radeon
> >>>>> _accel.c.  that code was removed or commented out in favor of the code
> >>>>> here.
> >>>>
> >>>>
> >>>> OK, so the right fix would be to port that clock configuration from 6.7
> >>>> radeon_accel.c into the 'case 0:' part of RADEONSetDynamicClock()?
> >>>>
> >>>
> >>> it seems that way.  I'll try and take a loot at it this week at some
> >>> point and come up with a better patch unless someone beats me to it.
> >>>
> >>> Alex
> >>
> >>
> >>
> >> See if this works for you (also added to bugzilla).
> >
> >
> > Alex, this is awesome, thanks a lot.  I'll give it a try Monday after
> > thanksgiving.
> 
> OK, it seems to work too.  I wasn't able to crash the box with the
> latest version of the patch.  I updated the bugzilla bug, but I didn't
> close the bug just yet.  I think we should add this to HEAD and 6.8.2.
> 

If no one has any objections I'll apply this to HEAD tonight.

Alex

> cheers,
> Kristian
> 
>