[Intel-gfx] [PATCH 04/21 v2] drm/i915: skylake scaler structure definitions

[Daniel Vetter]

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