From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH 06/19] drm/i915: Add intel_pipe_wm and
 prepare for watermark pre-compute
Date: Mon, 16 Sep 2013 22:36:02 +0200
Message-ID: <20130916203601.GD32145@phenom.ffwll.local>
References: <1377862239-17052-1-git-send-email-ville.syrjala@linux.intel.com>
	<1377862239-17052-7-git-send-email-ville.syrjala@linux.intel.com>
	<CA+gsUGR1-JNVZqJKn1VLVSPBohJxfvJwNpMSdkXKKCv1+ow1CQ@mail.gmail.com>
	<20130916162914.GK4531@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from mail-ea0-f172.google.com (mail-ea0-f172.google.com
	[209.85.215.172])
	by gabe.freedesktop.org (Postfix) with ESMTP id 45658E5DD9
	for <intel-gfx@lists.freedesktop.org>;
	Mon, 16 Sep 2013 13:35:47 -0700 (PDT)
Received: by mail-ea0-f172.google.com with SMTP id r16so2303924ead.3
	for <intel-gfx@lists.freedesktop.org>;
	Mon, 16 Sep 2013 13:35:46 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20130916162914.GK4531@intel.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=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: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
List-Id: intel-gfx@lists.freedesktop.org

On Mon, Sep 16, 2013 at 07:29:14PM +0300, Ville Syrj=E4l=E4 wrote:
> On Mon, Sep 16, 2013 at 12:07:32PM -0300, Paulo Zanoni wrote:
> > 2013/8/30  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrj=E4l=E4 <ville.syrjala@linux.intel.com>
> > >
> > > Introduce a new struct intel_pipe_wm which contains all the
> > > watermarks for a single pipe. Use it to unify the LP0 and LP1+
> > > watermark computations so that we can just iterate through the
> > > watermark levels neatly and call ilk_compute_wm_level() for each.
> > >
> > > Also add another tool ilk_wm_merge() that merges the LP1+ watermarks
> > > from all pipes. For that, embed one intel_pipe_wm inside intel_crtc t=
hat
> > > contains the currently valid watermarks for each pipe.
> > >
> > > This is mainly preparatory work for pre-computing the watermarks for
> > > each pipe and merging them at a later time. For now the merging still
> > > happens immediately.
> > =

> > >From the commit message, it seems this patch could be split in 4 or 5
> > sub-patches.
> =

> I think if I split it up any more then then the indiviual patches might
> stop making sense. Then you're going to be asking "why are you doing this
> change here?" and I have to answer "see patch n+2" or something.

Yeah, I think the patch is ok. It mixes in a few steps, but when
introducing new concepts I think smashing a few really simple things into
the first patch (if the patch is still pretty small like this one here)
helps the reader to understand how it fits together. Then with that
baseline more complex stuff can be converted over ...

Just my 2 uninformed cents ;-)

Cheers, Daniel
-- =

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch