Re: [RFC][PATCH 3/5] drm/i915: Enable scaling filter for plane and pipe

[Ville Syrjälä]

On Tue, Feb 25, 2020 at 12:35:43PM +0530, Pankaj Bharadiya wrote:
> Attach scaling filter property for crtc and plane and program the
> scaler control register for the selected filter type.
> 
> This is preparatory patch to enable Nearest-neighbor integer scaling.
> 
> Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@xxxxxxxxx>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 17 +++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_sprite.c  | 12 +++++++++++-
>  drivers/gpu/drm/i915/i915_reg.h              |  1 +
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3031e64ee518..b5903ef3c5a0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6242,6 +6242,8 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
> +	const struct drm_crtc_state *state = &crtc_state->uapi;
> +	u32 scaling_filter = PS_FILTER_MEDIUM;
>  	const struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
>  
> @@ -6258,6 +6260,11 @@ static void skl_pfit_enable(const struct intel_crtc_state *crtc_state)
>  		pfit_w = (crtc_state->pch_pfit.size >> 16) & 0xFFFF;
>  		pfit_h = crtc_state->pch_pfit.size & 0xFFFF;
>  
> +		if (state->scaling_filter ==
> +		    DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
> +			scaling_filter = PS_FILTER_PROGRAMMED;
> +		}

Just make that a function that can be used all over.
skl_scaler_filter(scaling_filter) or something.

> +
>  		hscale = (crtc_state->pipe_src_w << 16) / pfit_w;
>  		vscale = (crtc_state->pipe_src_h << 16) / pfit_h;
>  
> @@ -6268,8 +6275,10 @@ 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);
> +		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> +				  PS_SCALER_EN |
> +				  scaling_filter |
> +				  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),
> @@ -16695,6 +16704,10 @@ 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) >= 11)

gen >= 10 actually. Even glk seems to have it but bspec says not to
use it on glk. Supposedly not validated.

ilk/snb/ivb pfits also has programmable coefficients actually. So
IMO we should enable this on those as well.

The bigger problem will be how is userspace supposed to use this if it's
a crtc property? Those will not get automagically exposed via xrandr.

> +		drm_crtc_enable_scaling_filter(&crtc->base);
> +
>  	intel_color_init(crtc);
>  
>  	drm_WARN_ON(&dev_priv->drm, drm_crtc_index(&crtc->base) != crtc->pipe);
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 7abeefe8dce5..fd7b31a21723 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -414,6 +414,12 @@ skl_program_scaler(struct intel_plane *plane,
>  	u16 y_hphase, uv_rgb_hphase;
>  	u16 y_vphase, uv_rgb_vphase;
>  	int hscale, vscale;
> +	const struct drm_plane_state *state = &plane_state->uapi;
> +	u32 scaling_filter = PS_FILTER_MEDIUM;
> +
> +	if (state->scaling_filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
> +		scaling_filter = PS_FILTER_PROGRAMMED;
> +	}
>  
>  	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
>  				      &plane_state->uapi.dst,
> @@ -441,7 +447,8 @@ skl_program_scaler(struct intel_plane *plane,
>  	}
>  
>  	intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, scaler_id),
> -			  PS_SCALER_EN | PS_PLANE_SEL(plane->id) | scaler->mode);
> +			  scaling_filter | PS_SCALER_EN |
> +			  PS_PLANE_SEL(plane->id) | scaler->mode);
>  	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),
> @@ -3104,6 +3111,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) >= 11)

also gen>=10

Also this patch breaks things as we don't yet have the code to program
the coefficients. So the series needs to be reordered.

> +		drm_plane_enable_scaling_filter(&plane->base);
> +
>  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
>  
>  	return plane;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f45b5e86ec63..34923b1c284c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7212,6 +7212,7 @@ enum {
>  #define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
>  #define PS_FILTER_MASK         (3 << 23)
>  #define PS_FILTER_MEDIUM       (0 << 23)
> +#define PS_FILTER_PROGRAMMED   (1 << 23)
>  #define PS_FILTER_EDGE_ENHANCE (2 << 23)
>  #define PS_FILTER_BILINEAR     (3 << 23)
>  #define PS_VERT3TAP            (1 << 21)
> -- 
> 2.23.0

-- 
Ville Syrjälä
Intel