[Intel-gfx] [PATCH 2/2] drm/i915: Simplify scaler init during CRTC HW readout

[Imre Deak]

On Fri, Jul 21, 2017 at 07:36:22PM +0530, Mahesh Kumar wrote:
> Hi,
> 
> 
> On Friday 21 July 2017 06:56 PM, Imre Deak wrote:
> > On Fri, Jul 21, 2017 at 06:44:24PM +0530, Mahesh Kumar wrote:
> > > Hi,
> > > 
> > > 
> > > On Thursday 20 July 2017 04:20 AM, Imre Deak wrote:
> > > > The crtc state starts out being bzero'd, so no need to clear
> > > > scaler_users. Also intel_crtc_init_scalers() knows already which
> > > > platforms have scalers, so no need for the platform check here.
> > > > Similarly intel_crtc_init_scalers() will init scaler_id as required,
> > > > so no need to do it here separately.
> > > > 
> > > > Cc: Chandra Konduru <chandra.konduru at intel.com>
> > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > ---
> > > >    drivers/gpu/drm/i915/intel_display.c | 7 +------
> > > >    1 file changed, 1 insertion(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 8a38e64b1931..7210f418e9c0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -9132,12 +9132,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> > > >    	u64 power_domain_mask;
> > > >    	bool active;
> > > > -	if (INTEL_GEN(dev_priv) >= 9) {
> > > > -		intel_crtc_init_scalers(crtc, pipe_config);
> > > > -
> > > > -		pipe_config->scaler_state.scaler_id = -1;
> > > > -		pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX);
> > > > -	}
> > > > +	intel_crtc_init_scalers(crtc, pipe_config);
> > >
> > > If we are removing gen check, then IMHO we should initialize "scaler_id =
> > > -1" even before !crtc->num_scalers check in intel_crtc_init_scalers
> > > function, just to make sure scaler state doesn't have wrong value in
> > > platforms where we don't have any scalers.
> >
> > I agree that setting scaler_id to -1 on all platforms is a good idea,
> > but it's unrelated to removing here the GEN check. intel_crtc_init_scalers()
> > is already called for all platforms in intel_crtc_init(). So this change won't
> > make a difference for platforms without scalers: we leave scaler_id zeroed
> > on those already now (they have num_scalers set to 0). This isn't either
> > a problem since we allocate scalers only on GEN9+.
> > 
> > So we could do what you suggest but imo as a follow-up.
>
> sounds good, But if you are going to write a cleanup patch, will that
> be possible for you to do following change also.  0 indicates no
> scaler is used & non-zero +ve value indicates used scaler id.

That's one possibility, or we could just use a helper to initialize the
intel specific part of the state; there could be other fields there with
non-zero default values. This helper could be then used in the few
places where this is done now (or done partially) open-coded.

Also patches are welcome, feel free to follow-up:)

> in any case
> 
> Reviewed-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> 
> -Mahesh
> > 
> > --Imre
>