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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
