RE: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor based integer scaling support

[Laxminarayan Bharadiya, Pankaj]

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Sent: 10 March 2020 21:47
> To: Laxminarayan Bharadiya, Pankaj
> <pankaj.laxminarayan.bharadiya@xxxxxxxxx>
> Cc: jani.nikula@xxxxxxxxxxxxxxx; daniel@xxxxxxxx; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; airlied@xxxxxxxx;
> maarten.lankhorst@xxxxxxxxxxxxxxx; tzimmermann@xxxxxxx;
> mripard@xxxxxxxxxx; mihail.atanassov@xxxxxxx; Joonas Lahtinen
> <joonas.lahtinen@xxxxxxxxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>;
> Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; Souza, Jose
> <jose.souza@xxxxxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>;
> Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; Deak, Imre
> <imre.deak@xxxxxxxxx>; Shankar, Uma <uma.shankar@xxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>
> Subject: Re: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor
> based integer scaling support
> 
> On Tue, Feb 25, 2020 at 12:35:45PM +0530, Pankaj Bharadiya wrote:
> > Integer scaling (IS) is a nearest-neighbor upscaling technique that
> > simply scales up the existing pixels by an integer (i.e., whole
> > number) multiplier.Nearest-neighbor (NN) interpolation works by
> > filling in the missing color values in the upscaled image with that of
> > the coordinate-mapped nearest source pixel value.
> >
> > Both IS and NN preserve the clarity of the original image. Integer
> > scaling is particularly useful for pixel art games that rely on sharp,
> > blocky images to deliver their distinctive look.
> >
> > Program the scaler filter coefficients to enable the NN filter if
> > scaling filter property is set to DRM_SCALING_FILTER_NEAREST_NEIGHBOR
> > and enable integer scaling.
> >
> > Bspec: 49247
> >
> > 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 | 83
> > +++++++++++++++++++-  drivers/gpu/drm/i915/display/intel_display.h |
> > 2 +  drivers/gpu/drm/i915/display/intel_sprite.c  | 20 +++--
> >  3 files changed, 97 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index b5903ef3c5a0..6d5f59203258 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -6237,6 +6237,73 @@ void skl_scaler_disable(const struct
> intel_crtc_state *old_crtc_state)
> >  		skl_detach_scaler(crtc, i);
> >  }
> >
> > +/**
> > + *  Theory behind setting nearest-neighbor integer scaling:
> > + *
> > + *  17 phase of 7 taps requires 119 coefficients in 60 dwords per set.
> > + *  The letter represents the filter tap (D is the center tap) and
> > +the number
> > + *  represents the coefficient set for a phase (0-16).
> > + *
> > + *         +------------+------------------------+------------------------+
> > + *         |Index value | Data value coeffient 1 | Data value coeffient 2 |
> > + *         +------------+------------------------+------------------------+
> > + *         |   00h      |          B0            |          A0            |
> > + *         +------------+------------------------+------------------------+
> > + *         |   01h      |          D0            |          C0            |
> > + *         +------------+------------------------+------------------------+
> > + *         |   02h      |          F0            |          E0            |
> > + *         +------------+------------------------+------------------------+
> > + *         |   03h      |          A1            |          G0            |
> > + *         +------------+------------------------+------------------------+
> > + *         |   04h      |          C1            |          B1            |
> > + *         +------------+------------------------+------------------------+
> > + *         |   ...      |          ...           |          ...           |
> > + *         +------------+------------------------+------------------------+
> > + *         |   38h      |          B16           |          A16           |
> > + *         +------------+------------------------+------------------------+
> > + *         |   39h      |          D16           |          C16           |
> > + *         +------------+------------------------+------------------------+
> > + *         |   3Ah      |          F16           |          C16           |
> > + *         +------------+------------------------+------------------------+
> > + *         |   3Bh      |        Reserved        |          G16           |
> > + *         +------------+------------------------+------------------------+
> > + *
> > + *  To enable nearest-neighbor scaling:  program scaler coefficents
> > +with
> > + *  the center tap (Dxx) values set to 1 and all other values set to
> > +0 as per
> > + *  SCALER_COEFFICIENT_FORMAT
> > + *
> > + */
> > +void skl_setup_nearest_neighbor_filter(struct drm_i915_private
> *dev_priv,
> > +				  enum pipe pipe, int scaler_id)
> 
> skl_scaler_...
> 
> > +{
> > +
> > +	int coeff = 0;
> > +	int phase = 0;
> > +	int tap;
> > +	int val = 0;
> 
> Needlessly wide scope for most of these.
> 
> > +
> > +	/*enable the index auto increment.*/
> > +	intel_de_write_fw(dev_priv, SKL_PS_COEF_INDEX_SET0(pipe,
> scaler_id),
> > +			  _PS_COEE_INDEX_AUTO_INC);
> > +
> > +	for (phase = 0; phase < 17; phase++) {
> > +		for (tap = 0; tap < 7; tap++) {
> > +			coeff++;
> 
> Can be part of the % check.

OK.

> 
> > +			if (tap == 3)
> > +				val = (phase % 2) ? (0x800) : (0x800 << 16);
> 
> Parens overload.

OK. Will remove.
> 
> > +
> > +			if (coeff % 2 == 0) {
> > +				intel_de_write_fw(dev_priv,
> SKL_PS_COEF_DATA_SET0(pipe, scaler_id), val);
> > +				val = 0;
> 
> Can drop this val=0 if you move the variable into tight scope and initialize
> there.

Moving val=0 initialization to the tight scope will not work here as we need
to retain "val" and write only when 2 coefficients are ready (since 2 
coefficients are packed in 1 dword).

e.g. for (12th , 11th)  coefficients, coefficient reg value should be ( (0 << 16) | 0x800).
If we initialize val = 0 in tight loop, 0 will be written to  coefficient register.

> 
> I was trying to think of a bit more generic way to do this, but couldn't really
> think of anything apart from pre-filling the entire coefficient set and the
> programming blindly. And that seems a bit wasteful if we only care about
> nearest neighbour.
> 
> > +			}
> > +
> > +		}
> > +
> > +	}
> > +
> > +	intel_de_write_fw(dev_priv, SKL_PS_COEF_DATA_SET0(pipe,
> scaler_id),
> > +0); }
> > +
> >  static void skl_pfit_enable(const struct intel_crtc_state
> > *crtc_state)  {
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -6260,9 +6327,23 @@ 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;
> >
> > +		id = scaler_state->scaler_id;
> > +
> >  		if (state->scaling_filter ==
> >  		    DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
> >  			scaling_filter = PS_FILTER_PROGRAMMED;
> > +			skl_setup_nearest_neighbor_filter(dev_priv, pipe,
> id);
> 
> This should be sitting alongside the other register writes.

I missed this, thanks for pointing out.

> 
> > +
> > +			/* Make the scaling window size to integer multiple
> of
> > +			 * source.
> > +			 *
> > +			 * TODO: Should userspace take desision to round
> > +			 * scaling window to integer multiple?
> 
> To give userspace actual control of the pfit window size we need the border
> props (or something along those lines). Step 1 is
> https://patchwork.freedesktop.org/series/68409/. There are further steps in
> my branch after that, but it's still missing the border props for eDP/LVDS/DSI
> since I was too lazy to think how they should interact with the existing scaling
> mode prop.
> 
> > +			 */
> > +			pfit_w = rounddown(pfit_w,
> > +					   (crtc_state->pipe_src_w << 16));
> > +			pfit_h = rounddown(pfit_h,
> > +					   (crtc_state->pipe_src_h << 16));
> >  		}
> 
> This part should be dropped as Daniel mentioned.

Will remove.

Thanks,
Pankaj

> 
> >
> >  		hscale = (crtc_state->pipe_src_w << 16) / pfit_w; @@ -
> 6271,8
> > +6352,6 @@ static void skl_pfit_enable(const struct intel_crtc_state
> *crtc_state)
> >  		uv_rgb_hphase = skl_scaler_calc_phase(1, hscale, false);
> >  		uv_rgb_vphase = skl_scaler_calc_phase(1, vscale, false);
> >
> > -		id = scaler_state->scaler_id;
> > -
> >  		spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> >  		intel_de_write_fw(dev_priv, SKL_PS_CTRL(pipe, id),
> 
> I think we should also explicitly indicate here which cofficient set(s) we're
> going to use, even if using set0 does mean those bits will be 0.
> 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index f92efbbec838..49f58d3c98fe 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -586,6 +586,8 @@ void intel_crtc_arm_fifo_underrun(struct
> > intel_crtc *crtc,
> >  u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center);
> > int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);  void
> > skl_scaler_disable(const struct intel_crtc_state *old_crtc_state);
> > +void skl_setup_nearest_neighbor_filter(struct drm_i915_private
> *dev_priv,
> > +				  enum pipe pipe, int scaler_id);
> >  void ilk_pfit_disable(const struct intel_crtc_state *old_crtc_state);
> >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> >  			const struct intel_plane_state *plane_state); diff --
> git
> > a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index fd7b31a21723..5bef5c031374 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -415,18 +415,26 @@ skl_program_scaler(struct intel_plane *plane,
> >  	u16 y_vphase, uv_rgb_vphase;
> >  	int hscale, vscale;
> >  	const struct drm_plane_state *state = &plane_state->uapi;
> > +	u32 src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> > +	u32 src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> >  	u32 scaling_filter = PS_FILTER_MEDIUM;
> > +	struct drm_rect dst;
> >
> >  	if (state->scaling_filter ==
> DRM_SCALING_FILTER_NEAREST_NEIGHBOR) {
> >  		scaling_filter = PS_FILTER_PROGRAMMED;
> > +		skl_setup_nearest_neighbor_filter(dev_priv, pipe,
> scaler_id);
> > +
> > +		/* Make the scaling window size to integer multiple of source
> > +		 * TODO: Should userspace take desision to round scaling
> window
> > +		 * to integer multiple?
> > +		 */
> > +		crtc_w = rounddown(crtc_w, src_w);
> > +		crtc_h = rounddown(crtc_h, src_h);
> >  	}
> >
> > -	hscale = drm_rect_calc_hscale(&plane_state->uapi.src,
> > -				      &plane_state->uapi.dst,
> > -				      0, INT_MAX);
> > -	vscale = drm_rect_calc_vscale(&plane_state->uapi.src,
> > -				      &plane_state->uapi.dst,
> > -				      0, INT_MAX);
> > +	drm_rect_init(&dst, crtc_x, crtc_y, crtc_w, crtc_h);
> 
> Drop as well.
> 
> > +	hscale = drm_rect_calc_hscale(&plane_state->uapi.src, &dst, 0,
> INT_MAX);
> > +	vscale = drm_rect_calc_vscale(&plane_state->uapi.src, &dst, 0,
> > +INT_MAX);
> >
> >  	/* TODO: handle sub-pixel coordinates */
> >  	if (intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier)
> &&
> > --
> > 2.23.0
> 
> --
> Ville Syrjälä
> Intel