Re: [PATCH 03/12] drm/i915: Stage scaler request for NV12 as src format

Ville Syrjälä
2015-05-21T16:49:11Z

On Thu, May 21, 2015 at 04:24:00PM +0000, Konduru, Chandra wrote:
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4499,9 +4499,11 @@ skl_update_scaler_users(
> > > rotation = DRM_ROTATE_0;
> > > }
> > >
> > > - need_scaling = intel_rotation_90_or_270(rotation) ?
> > > - (src_h != dst_w || src_w != dst_h):
> > > - (src_w != dst_w || src_h != dst_h);
> > > + /* scaling is required when src dst sizes doesn't match or format is NV12
> > */
> > > + need_scaling = (src_w != dst_w || src_h != dst_h ||
> > > + (intel_rotation_90_or_270(rotation) &&
> > > + (src_h != dst_w || src_w != dst_h)) ||
> >
> > That doesn't look right.
> It is evaluating scaling needed by comparing
> 1) src != dst
> 2) format == nv12
> Can you pls point what doesn't look right here?

It'll do these checks before even checking the rotation:
src_w != dst_w || src_h != dst_h

>
> > Maybe add a small helper function that has these scaling
> > checks so that we don't need to have them all in the same if statement.
>
> Thought about doing that but have to pass around 6 params to helper
> and do the same evaluation there which seems unnecessary.

Just figured it'll look a bit less convoluted if you split it up into a
few separate if statements. And doing that via a helper avoids
polluting the main codepath with the details. I tend to like small
helper functions like that (maybe a bit too much sometimes :)

>
> >
> > > + (fb && fb->pixel_format == DRM_FORMAT_NV12));
> > >

--
Ville Syrjälä
Intel OTC