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