On Fri, Apr 13, 2012 at 18:11, Chris Wilson <chris at chris-wilson.co.uk>wrote:
> On Fri, 13 Apr 2012 17:08:57 -0300, Eugeni Dodonov <
> eugeni.dodonov at 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
<http://eugeni.dodonov.net/>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20120413/e022c839/attachment-0001.htm>
