From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 45/89] drm/i915/skl: Definition of SKL WM param structs for pipe/plane Date: Wed, 17 Sep 2014 17:59:24 +0200 Message-ID: <20140917155924.GL31703@phenom.ffwll.local> References: <1409830075-11139-1-git-send-email-damien.lespiau@intel.com> <1409830075-11139-46-git-send-email-damien.lespiau@intel.com> <20140910183953.GE4193@intel.com> <20140917135900.GE17120@strange.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wg0-f44.google.com (mail-wg0-f44.google.com [74.125.82.44]) by gabe.freedesktop.org (Postfix) with ESMTP id C134B6E1B7 for ; Wed, 17 Sep 2014 08:58:57 -0700 (PDT) Received: by mail-wg0-f44.google.com with SMTP id b13so772580wgh.3 for ; Wed, 17 Sep 2014 08:58:56 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140917135900.GE17120@strange.ger.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 17, 2014 at 02:59:00PM +0100, Damien Lespiau wrote: > On Wed, Sep 10, 2014 at 09:39:53PM +0300, Ville Syrj=E4l=E4 wrote: > > > +struct skl_wm_values { > > > + bool dirty[I915_MAX_PIPES]; > > > + uint32_t wm_linetime[I915_MAX_PIPES]; > > > + uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; > > > + uint32_t cursor[I915_MAX_PIPES][8]; > > > + uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES]; > > > + uint32_t cursor_trans[I915_MAX_PIPES]; > > > +}; > > = > > These multi dimensional arrays hurt my eyes. Maybe we should restructure > > this a bit to eg: > > = > > struct skl_wm_values { > > struct { > > wm_linetime; > > plane[MAX_PLANES][8]; > > ... > > } pipe[MAX_PIPES]; > > }; > > = > > The two dimensional plane[][] array is still a bit nasty, but maybe we > > can live with it. > > = > > We could also do the same operatiob for the ilk version to keep stuff > > similar. > > = > > > + > > > +struct skl_wm_level { > > > + bool plane_en[I915_MAX_PLANES]; > > > + uint16_t plane_res_b[I915_MAX_PLANES]; > > > + uint8_t plane_res_l[I915_MAX_PLANES]; > > = > > This stuff could also look better as an array of struct of some sort. > > Also should probably put the bool and uint8_t next to each other in case > > gcc is smart enough to pack things more tightly. > > = > > > + bool cursor_en; > > > + uint16_t cursor_res_b; > > > + uint8_t cursor_res_l; > > = > > And this could also be an instance of the same struct use for the proper > > planes. > > = > > > +struct skl_pipe_wm_parameters { > > > + bool active; > > > + uint32_t pipe_htotal; > > > + uint32_t pixel_rate; /* in KHz */ > > > + struct intel_plane_wm_parameters plane[I915_MAX_PLANES]; > > > + struct intel_plane_wm_parameters cursor; > > > +}; > > = > > I suppose we just need to start using some kind of named indexes for the > > planes on all platforms so we can unify all this stuff. But that can be > > done when we have all the code merged so we can better see how to unify > > things. > = > For all those comments, the issue here is that changing something in > those definitions has consequences in 10/15 patches that will need to be > changed. Rather painful. It'd be much easier to do those change once we > have that code upstream, on top. As far as I can see there are minor-ish > improvements over what's here. > = > I've added an item in my post-merge plan. Sounds like an acceptable > plan? Sounds good to me, atm all the plane config tracking is seriously all in-flight anyway ... -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch