On Tue, Mar 24, 2015 at 10:13:54PM -0700, Matt Roper wrote:
> On Fri, Mar 20, 2015 at 05:04:25PM -0700, Chandra Konduru wrote:
> > +struct intel_scaler {
> > + int id;
> > + int in_use;
> > + uint32_t mode;
>
> If I'm reading later patches correctly, this looks like this is always
> PS_SCALER_MODE_HQ if one scaler is needed, or PS_SCALER_MODE_DYN if
> multiple scalers are needed. So the values for each of a CRTC's scalers
> doesn't actually vary; should this just be a single value in
> intel_crtc_scalar_state rather than being duplicated for each scaler?
Or we can just compute it at runtime with hweight of the inuse scaler
bitmask.
> > + uint32_t filter;
>
> Is filter a constant? Unless I missed something in later patches, it
> looks like it's set to PS_FILTER_MEDIUM and then never changed. Can we
> drop the field and just use the constant itself where appropriate?
Concured. I prefer that we add struct members only when there's a need -
all that indirection just makes it harder to follow the code logic.
I also agree with all of the other comments from Matt cut out here.
[snip]
> > + struct intel_crtc_scaler_state scaler_state;
>
> If we can kill off a bunch of the fields above, then we may be able to
> put the remaining few fields directly in intel_crtc_state and eliminate
> a level of structure nesting, which might make things a bit simpler.
Imo substructures for separate things make sense - otherwise we'll just
have a pile of members with the same prefix, which semantically is exactly
the same thing. But I agree that reducing dynamic state to only the bits
that are indeed dynamic (and not trivially derived from existing dynamic
state) is a good goal.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
