[Intel-gfx] [PATCH 08/21] drm/i915: Add helper function to update scaler_users in crtc_state

[Konduru, Chandra]

> -----Original Message-----
> From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, March 18, 2015 1:16 AM
> To: Konduru, Chandra
> Cc: intel-gfx; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 08/21] drm/i915: Add helper function to update
> scaler_users in crtc_state
> 
> On Sun, Mar 15, 2015 at 6:55 AM, Chandra Konduru
> <chandra.konduru at intel.com> wrote:
> > +       /*
> > +        * check for rect size:
> > +        *     min sizes in case of scaling involved
> > +        *     max sizes in all cases
> > +        */
> > +       if ((need_scaling &&
> > +               (src_w < scaler->min_src_w || src_h < scaler->min_src_h ||
> > +                dst_w < scaler->min_dst_w || dst_h <
> > + scaler->min_dst_h)) ||
> > +
> > +                src_w > scaler->max_src_w || src_h > scaler->max_src_h ||
> > +                dst_w > scaler->max_dst_w || dst_h >
> > + scaler->max_dst_h) {
> 
> Ok let's hope I've traced all the min/max stuff in your patches correctly. It looks
> like we only ever initialized them to fixed values, never changed them and only
> use them here. Spreading out the values like that without having a real need for
> this flexibility makes review really hard imo.
> 
> What about instead adding a skl_check_scale_limits functions which does all
> these checks here and uses hardcoded values? That way you could move the
> commits about the various values (e.g. only 34% scaling and the other easier-to-
> understand limits) right next to the code that checks these limits?

I agree that some of limits are fixed, ratios aren't fixed. So kept them
in state and updated during modeset. There isn't much complexity with current 
approach.

> 
> There's also some confusion with the overly generic (imo) old sprite code and its
> scaling limit checks. Imo we can look at that later on.

Agree. 

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch