[Intel-gfx] [PATCH 45/89] drm/i915/skl: Definition of SKL WM param structs for pipe/plane

[Daniel Vetter]

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älä 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