RE: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC

["Laxminarayan Bharadiya, Pankaj" <pankaj.laxminarayan.bharadiya@intel.com>]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: 23 March 2020 20:18
> To: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@intel.com>
> Cc: Lattannavar, Sameer <sameer.lattannavar@intel.com>;
> jani.nikula@linux.intel.com; daniel@ffwll.ch; intel-gfx@lists.freedesktop.org;
> dri-devel@lists.freedesktop.org; daniels@collabora.com; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> David Airlie <airlied@linux.ie>; Chris Wilson <chris@chris-wilson.co.uk>;
> Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Souza, Jose
> <jose.souza@intel.com>; Deak, Imre <imre.deak@intel.com>; Shankar, Uma
> <uma.shankar@intel.com>
> Subject: Re: [PATCH v2 5/5] drm/i915: Enable scaling filter for plane and CRTC
> 
> On Thu, Mar 19, 2020 at 03:51:03PM +0530, Pankaj Bharadiya wrote:
> > GEN >= 10 hardware supports the programmable scaler filter.
> >
> > Attach scaling filter property for CRTC and plane for GEN >= 10
> > hardwares and program scaler filter based on the selected filter type.
> >
> > changes since v1:
> > * None
> > Changes since RFC:
> > * Enable properties for GEN >= 10 platforms (Ville)
> > * Do not round off the crtc co-ordinate (Danial Stone, Ville)
> > * Add new functions to handle scaling filter setup (Ville)
> > * Remove coefficient set 0 hardcoding.
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > Signed-off-by: Pankaj Bharadiya
> > <pankaj.laxminarayan.bharadiya@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 32
> > ++++++++++++++++++--  drivers/gpu/drm/i915/display/intel_sprite.c  |
> > 31 ++++++++++++++++++-
> >  2 files changed, 60 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 791dd908aa89..4b3387ee332e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6309,6 +6309,25 @@ void
> skl_scaler_setup_nearest_neighbor_filter(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >
> > +static u32
> > +skl_scaler_crtc_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> pipe,
> > +			  int id, int set, enum drm_crtc_scaling_filter filter) {
> > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > +
> > +	if (filter == DRM_CRTC_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > +							 set);
> > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > +				PS_UV_VERT_FILTER_SELECT(set) |
> > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > +				PS_Y_VERT_FILTER_SELECT(set) |
> > +				PS_Y_HORZ_FILTER_SELECT(set);
> > +
> > +	}
> > +	return scaler_filter_ctl;
> 
> This function does too many things.

I was thinking to have a common function which configures the filter and also
provides the register bits (ps_ctrl) to select a desired filter so that we need
not have extra condition to figure out filter select register bits where this
function is being called.
How about renaming this function to some better name like  
skl_scaler_set_and_get_filter_select() or something else? 
Or shall I breakdown this function into multiple functions? 

Any suggestions?

> 
> > +}
> > +
> >  static void skl_pfit_enable(const struct intel_crtc_state
> > *crtc_state)  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -6316,12 +6335,14 @@ static void skl_pfit_enable(const struct
> intel_crtc_state *crtc_state)
> >  	enum pipe pipe = crtc->pipe;
> >  	const struct intel_crtc_scaler_state *scaler_state =
> >  		&crtc_state->scaler_state;
> > +	const struct drm_crtc_state *state = &crtc_state->uapi;
> 
> Pls don't add this kind of aliases. We're moving away from using the drm_ types
> as much as possible.

OK.

> 
> >
> >  	if (crtc_state->pch_pfit.enabled) {
> >  		u16 uv_rgb_hphase, uv_rgb_vphase;
> >  		int pfit_w, pfit_h, hscale, vscale;
> >  		unsigned long irqflags;
> >  		int id;
> > +		int scaler_filter_ctl;
> 
> It's a register value so u32. I'd also

Yes, I missed it. Thanks for pointing out.

> s/scaler_filter_ctl/filter_select/ or something like that.
> 
> Alternatively we could just call it ps_ctrl and have it contain the full register
> value for that particular register.

ps_ctrl sounds better, will use this name.

> 
> >
> >  		if (drm_WARN_ON(&dev_priv->drm,
> >  				crtc_state->scaler_state.scaler_id < 0)) @@ -
> 6340,8 +6361,12 @@
> > static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
> >
> >  		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > -		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> PS_SCALER_EN |
> > -				  PS_FILTER_MEDIUM | scaler_state-
> >scalers[id].mode);
> > +		scaler_filter_ctl =
> > +			skl_scaler_crtc_setup_filter(dev_priv, pipe, id, 0,
> > +						state->scaling_filter);
> > +		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> > +				  PS_SCALER_EN | scaler_filter_ctl |
> > +				  scaler_state->scalers[id].mode);
> >  		intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, id),
> >  				  PS_Y_PHASE(0) |
> PS_UV_RGB_PHASE(uv_rgb_vphase));
> >  		intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, id), @@ -
> 16777,6
> > +16802,9 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv,
> enum pipe pipe)
> >  		dev_priv->plane_to_crtc_mapping[i9xx_plane] = crtc;
> >  	}
> >
> > +	if (INTEL_GEN(dev_priv) >= 10)
> > +		drm_crtc_enable_scaling_filter(&crtc->base);
> > +
> >  	intel_color_init(crtc);
> >
> >  	intel_crtc_crc_init(crtc);
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index deda351719db..ac3fd9843ace 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -395,6 +395,26 @@ skl_plane_max_stride(struct intel_plane *plane,
> >  		return min(8192 * cpp, 32768);
> >  }
> >
> > +static u32
> > +skl_scaler_plane_setup_filter(struct drm_i915_private *dev_priv, enum pipe
> pipe,
> > +			      int id, int set,
> > +			      enum drm_plane_scaling_filter filter) {
> > +	u32 scaler_filter_ctl = PS_FILTER_MEDIUM;
> > +
> > +	if (filter == DRM_PLANE_SCALING_FILTER_NEAREST_NEIGHBOR) {
> > +		skl_scaler_setup_nearest_neighbor_filter(dev_priv, pipe, id,
> > +							 set);
> > +		scaler_filter_ctl = PS_FILTER_PROGRAMMED |
> > +				PS_UV_VERT_FILTER_SELECT(set) |
> > +				PS_UV_HORZ_FILTER_SELECT(set) |
> > +				PS_Y_VERT_FILTER_SELECT(set) |
> > +				PS_Y_HORZ_FILTER_SELECT(set);
> > +
> > +	}
> > +	return scaler_filter_ctl;
> > +}
> > +
> 
> We don't want such copy pasta between planes and crtcs.

Yeah, got it. 
Will add a common enum drm_scaling_filter and use it.

Thanks,
Pankaj
 
> 
> >  static void
> >  skl_program_scaler(struct intel_plane *plane,
> >  		   const struct intel_crtc_state *crtc_state, @@ -406,6 +426,7
> @@
> > skl_program_scaler(struct intel_plane *plane,
> >  	int scaler_id = plane_state->scaler_id;
> >  	const struct intel_scaler *scaler =
> >  		&crtc_state->scaler_state.scalers[scaler_id];
> > +	const struct drm_plane_state *state = &plane_state->uapi;
> >  	int crtc_x = plane_state->uapi.dst.x1;
> >  	int crtc_y = plane_state->uapi.dst.y1;
> >  	u32 crtc_w = drm_rect_width(&plane_state->uapi.dst);
> > @@ -413,6 +434,7 @@ skl_program_scaler(struct intel_plane *plane,
> >  	u16 y_hphase, uv_rgb_hphase;
> >  	u16 y_vphase, uv_rgb_vphase;
> >  	int hscale, vscale;
> > +	int scaler_filter_ctl;
> >
> >  	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
> >  				      &plane_state->uapi.dst,
> > @@ -439,8 +461,12 @@ skl_program_scaler(struct intel_plane *plane,
> >  		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> >  	}
> >
> > +	scaler_filter_ctl =
> > +		skl_scaler_plane_setup_filter(dev_priv, pipe, scaler_id, 0,
> > +					      state->scaling_filter);
> >  	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
> > -			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler-
> >mode);
> > +			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) |
> > +			  scaler->mode | scaler_filter_ctl);
> >  	intel_de_write_fw(dev_priv, SKL_PS_VPHASE(pipe, scaler_id),
> >  			  PS_Y_PHASE(y_vphase) |
> PS_UV_RGB_PHASE(uv_rgb_vphase));
> >  	intel_de_write_fw(dev_priv, SKL_PS_HPHASE(pipe, scaler_id), @@
> > -3121,6 +3147,9 @@ skl_universal_plane_create(struct drm_i915_private
> > *dev_priv,
> >
> >  	drm_plane_create_zpos_immutable_property(&plane->base, plane_id);
> >
> > +	if (INTEL_GEN(dev_priv) >= 10)
> > +		drm_plane_enable_scaling_filter(&plane->base);
> > +
> >  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
> >
> >  	return plane;
> > --
> > 2.23.0
> 
> --
> Ville Syrjälä
> Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel