Re: [PATCH 21/29] drm/i915: initialize DDI buffer translations

[Eugeni Dodonov <eugeni@dodonov.net>]

[-- Attachment #1.1: Type: text/plain, Size: 1616 bytes --]

On Fri, Apr 13, 2012 at 18:11, Chris Wilson <chris@chris-wilson.co.uk>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
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 2277 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx