Unit initialization infrastructure

[Alex Deucher]

On Mon, Jul 12, 2010 at 3:45 PM, Jerome Glisse <glisse at freedesktop.org> wrote:
> So here is something i have been thinking on for a while and never
> get around to actually code it until recently. I needed|wanted it
> as i was working on improved GPU reset. Issue i faced is that despite
> trying hard to make clean code reusable btw asic we ended up with some
> messy code path (especially in error path) and you basicly have to
> write for each asic a special function handling initialization+
> finalization add 2 other for suspend/resume and finaly add one for
> reset and one for power management.
>

I haven't really had a chance to process this yet, but while we are
thinking about this sort of thing, I think we should also fix up the
radeon_asic handling.  Rather than having a static set of radeon_asic
pointer tables in the driver, we really sure just allocate one per
card when loading the driver.  Right now, we edit some of the tables
at load time to work around certain quirks on some asics which could
break things when multiple cards are installed.

The other issue is the debugfs support is not multi-card aware.

Alex

> So the idea of unit is to give each unit a set of function :
> init,fini,suspend,resume,hold,unhold
>
> Unit are things like : CP, MC, MemoryManager, GEM, CLK, ...
> Now instead of writing one init/fini/suspend/resume function
> you just provide for each asic the following :
>
> struct radeon_unit_funcs *r100_units[] = {
> ปทททททท&r100_unit_errata,
> ปทททททท&r100_unit_vga,
> ปทททททท&r100_unit_common,
> ปทททททท&radeon_unit_scratch,
> ปทททททท&radeon_unit_sr,
> ปทททททท&radeon_unit_surface,
> ปทททททท&radeon_unit_reset,
> ปทททททท&radeon_unit_bios,
> ปทททททท&r100_unit_clock,
> ปทททททท&r100_unit_mc,
> ปทททททท&radeon_unit_fence,
> ปทททททท&radeon_unit_irq,
> ปทททททท&radeon_unit_mm,
> ปทททททท&radeon_unit_agp,
> ปทททททท&r100_unit_gart,
> ปทททททท&r100_unit_irq,
> ปทททททท&radeon_unit_gem,
> ปทททททท&r100_unit_cs,
> ปทททททท&r100_unit_cp,
> ปทททททท&r100_unit_ib,
> ปททททททNULL
> };
>
> and a common helper function just go through the list
> and init each unit, for fini it goes in reverse order.
> resume/suspend are like init/fini, suspend goes in
> reverse order while resume goes in normal order
> radeon_unit_sr is a fake helper unit just to know
> from which point you start/end suspend/resume,
> same for radeon_unit_reset
>
> So with that approach i think it's lot easier to
> figure out dependency btw unit and in which order
> things get call.
>
> I think it will help to untangle some of the code
> path that are tricky today to follow. Thought there
> is a downside any change like this is likely to
> introduce regression and i hate those like others,
> anyway i think the longterm benefit are greater than
> the temporary disturbance we can even have the unit
> path coexist with current code and allow it to
> stabilize on its own before switching over and
> removing old path.
>
> So before i go any further what is your opinion
> on this ? Attached is a patch which shows the
> approach for r100/r200 asic (tested and it
> works) as you can see it can be done without
> touching the old path and we can also add an
> option to select btw old/new path.
>
> Cheers,
> Jerome
>