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

[Damien Lespiau]

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?

Thanks,

-- 
Damien