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

[Damien Lespiau]

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

Is this patch worthy of your r-b tag then Ville?

Thanks,

-- 
Damien