From mboxrd@z Thu Jan 1 00:00:00 1970 From: Damien Lespiau Subject: Re: [PATCH 45/89] drm/i915/skl: Definition of SKL WM param structs for pipe/plane Date: Mon, 22 Sep 2014 15:00:08 +0100 Message-ID: <20140922140008.GL10951@strange.ger.corp.intel.com> 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> <20140917155924.GL31703@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 465286E41E for ; Mon, 22 Sep 2014 07:00:42 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140917155924.GL31703@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 17, 2014 at 05:59:24PM +0200, Daniel Vetter wrote: > 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 restruct= ure > > > 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 c= ase > > > 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 pro= per > > > 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 uni= fy > > > 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 ... Is this patch worthy of your r-b tag then Ville? Thanks, -- = Damien