From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eugeni Dodonov Subject: Re: [PATCH 21/29] drm/i915: initialize DDI buffer translations Date: Fri, 13 Apr 2012 22:32:37 -0300 Message-ID: References: <1334347745-11743-1-git-send-email-eugeni.dodonov@intel.com> <1334347745-11743-22-git-send-email-eugeni.dodonov@intel.com> <1334351495_415962@CP5-2952> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0454240234==" Return-path: Received: from mail-yx0-f177.google.com (mail-yx0-f177.google.com [209.85.213.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 130D59EB45 for ; Fri, 13 Apr 2012 18:33:18 -0700 (PDT) Received: by yenm10 with SMTP id m10so2189440yen.36 for ; Fri, 13 Apr 2012 18:33:18 -0700 (PDT) In-Reply-To: <1334351495_415962@CP5-2952> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, Eugeni Dodonov List-Id: intel-gfx@lists.freedesktop.org --===============0454240234== Content-Type: multipart/alternative; boundary=20cf303ddb90df567604bd9992c5 --20cf303ddb90df567604bd9992c5 Content-Type: text/plain; charset=ISO-8859-1 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 --20cf303ddb90df567604bd9992c5 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Fri, Apr 13, 2012 at 18:11, Chris Wilson <chris@chris-w= ilson.co.uk> wrote:
On Fri, 13 Apr 2012 17:08:57 -0300, Eugeni Dodonov <eugeni.dodonov@intel.com> = wrote:
> - =A0 =A0 if (IS_HASWELL(dev))
> + =A0 =A0 if (IS_HASWELL(dev)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 intel_init_power_wells(dev);
> + =A0 =A0 =A0 =A0 =A0 =A0 intel_hsw_prepare_ddi_buffers(dev);
Give this a much more generic, more grandiose name and move the
generation specific routines down a level:
=A0intel_init_ddi() [but make it more verbose and self-documenting!]

Will do.
=A0
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 d= o all the platform-specific stuff in one place, but with the suggestion abo= ve it makes it much easier to move the intel_ddi_init into more logical pos= ition.

The specs ask to make sure that the DDI buffers transla= tions are configured correctly before attempting to enable them, so I thoug= ht on doing them early in the process, so the further step assume that ever= ything 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 in= deed.

In both cases, I agree that it will make the driver cod= e much more self-explainable and easier to maintain. Huge thanks! I'll = send a new version with those comments addressed later.

--
Eugeni Dodonov

--20cf303ddb90df567604bd9992c5-- --===============0454240234== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0454240234==--