On Fri, Apr 13, 2012 at 18:11, Chris Wilson wrote: > On Fri, 13 Apr 2012 17:08:57 -0300, Eugeni Dodonov < > eugeni.dodonov@intel.com> wrote: > > - if (IS_HASWELL(dev)) > > + if (IS_HASWELL(dev)) { > > intel_init_power_wells(dev); > > + intel_hsw_prepare_ddi_buffers(dev); > Give this a much more generic, more grandiose name and move the > generation specific routines down a level: > intel_init_ddi() [but make it more verbose and self-documenting!] > Will do. > When you do that you'll might consider it to actually a part of the > display initialisation and so move it down below intel_init_display(). > Remember it is vital for our sanity that we be able to concisely describe > the sequence of steps required to initialise the GPU. > My initial idea here was to re-use the IS_HASWELL check to do all the platform-specific stuff in one place, but with the suggestion above it makes it much easier to move the intel_ddi_init into more logical position. The specs ask to make sure that the DDI buffers translations are configured correctly before attempting to enable them, so I thought on doing them early in the process, so the further step assume that everything is up and ready to use already. But yes, I believe that it could be done from within intel_init_display as well, and it will be more logical indeed. In both cases, I agree that it will make the driver code much more self-explainable and easier to maintain. Huge thanks! I'll send a new version with those comments addressed later. -- Eugeni Dodonov