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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Mar 19, 2015 at 05:43:24PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@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@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.

Hm, where are the ratios? I haven't found that part in your series ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx