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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx