On Tue, Jul 13, 2010 at 5:45 AM, Jerome Glisse glisse@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.
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.
No. not again, move on and fix something more important, this is just busy work at this point.
This is like the 4-5th attempt at making these codepaths work, and everyone of them has not worked and you've redesigned it, and I've spent months fixing regressions upstream on crappy hw.
So lets not, we have codepaths that work, the only way anything like this is interesting is if it
a) solves lots of bugs are seeing (i.e. you write it all, get 15 tested-by, fixes lines). b) it actually makes gpu reset work this time.
Also the whole two codepaths thing was a broken mess the last time we did this, and it makes shit impossible to bisect across, as the bisects all ended at the enable new codepaths for rs400 commit.
If you are going to do this, you need to do it in a step by step fashion, add one unit, convert all GPUs to use it, finish it all, get it tested by lots of people, and then maybe I'll put it in d-r-t, I'd much rather we got past the kernel code and worked on the important stuff first.
Dave.