Re: [PATCH 06/19] drm/i915: Add intel_pipe_wm and prepare for watermark pre-compute

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Sep 16, 2013 at 07:29:14PM +0300, Ville Syrjälä 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älä <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 that
> > > 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