drm/vc4: fix two phase errors in the HVS scaler, and the chroma siting default

drivers/gpu/drm/vc4/vc4_hvs.c

@@ -409,30 +409,53 @@ static int vc4_hvs_debugfs_lbm_allocs(struct seq_file *m, void *data)
 	 (((c1) & 0x1ff) << 9) |				\
 	 (((c2) & 0x1ff) << 18))
 
-/* The whole filter kernel is arranged as the coefficients 0-16 going
+/* The PPF is a 4-tap filter with 64 phases, of which only 8 key phases
+ * (at 0, 1/8, ..., 7/8 of a source pixel) are stored; the hardware
+ * linearly interpolates the rest.  The 32 coefficients are indexed
+ * tap * 8 + phase, taps ordered +2, +1, 0, -1 from the sample point.
+ *
+ * The whole filter kernel is arranged as the coefficients 0-16 going
  * up, then a pad, then 17-31 going down and reversed within the
- * dwords.  This means that a linear phase kernel (where it's
- * symmetrical at the boundary between 15 and 16) has the last 5
- * dwords matching the first 5, but reversed.
+ * dwords.
  */
-#define VC4_LINEAR_PHASE_KERNEL(c0, c1, c2, c3, c4, c5, c6, c7, c8,	\
-				c9, c10, c11, c12, c13, c14, c15)	\
+#define VC4_KERNEL(c0, c1, c2, c3, c4, c5, c6, c7, c8, c9, c10, c11,	\
+		   c12, c13, c14, c15, c16, c17, c18, c19, c20, c21,	\
+		   c22, c23, c24, c25, c26, c27, c28, c29, c30, c31)	\
 	{VC4_PPF_FILTER_WORD(c0, c1, c2),				\
 	 VC4_PPF_FILTER_WORD(c3, c4, c5),				\
 	 VC4_PPF_FILTER_WORD(c6, c7, c8),				\
 	 VC4_PPF_FILTER_WORD(c9, c10, c11),				\
 	 VC4_PPF_FILTER_WORD(c12, c13, c14),				\
-	 VC4_PPF_FILTER_WORD(c15, c15, 0)}
+	 VC4_PPF_FILTER_WORD(c15, c16, 0),				\
+	 VC4_PPF_FILTER_WORD(c19, c18, c17),				\
+	 VC4_PPF_FILTER_WORD(c22, c21, c20),				\
+	 VC4_PPF_FILTER_WORD(c25, c24, c23),				\
+	 VC4_PPF_FILTER_WORD(c28, c27, c26),				\
+	 VC4_PPF_FILTER_WORD(c31, c30, c29)}
+
+/* A linear phase kernel is symmetrical at the boundary between 15 and
+ * 16, so only the first half need be given.
+ */
+#define VC4_LINEAR_PHASE_KERNEL(c0, c1, c2, c3, c4, c5, c6, c7, c8,	\
+				c9, c10, c11, c12, c13, c14, c15)	\
+	VC4_KERNEL(c0, c1, c2, c3, c4, c5, c6, c7, c8, c9, c10, c11,	\
+		   c12, c13, c14, c15, c15, c14, c13, c12, c11, c10,	\
+		   c9, c8, c7, c6, c5, c4, c3, c2, c1, c0)
 
-#define VC4_LINEAR_PHASE_KERNEL_DWORDS 6
-#define VC4_KERNEL_DWORDS (VC4_LINEAR_PHASE_KERNEL_DWORDS * 2 - 1)
+#define VC4_KERNEL_DWORDS 11
 
 /* Recommended B=1/3, C=1/3 filter choice from Mitchell/Netravali.
  * http://www.cs.utexas.edu/~fussell/courses/cs384g/lectures/mitchell/Mitchell.pdf
+ *
+ * Tabulated at the key phases the hardware actually uses, so it is not
+ * linear phase: a symmetric table would have to put the key phases at
+ * (p + 1/2) / 8, half a key phase late.
  */
 static const u32 mitchell_netravali_1_3_1_3_kernel[] =
-	VC4_LINEAR_PHASE_KERNEL(0, -2, -6, -8, -10, -8, -3, 2, 18,
-				50, 82, 119, 155, 187, 213, 227);
+	VC4_KERNEL(0, -1, -4, -7, -9, -9, -6, 1,
+		   14, 36, 66, 101, 137, 171, 200, 220,
+		   228, 220, 200, 171, 137, 101, 66, 36,
+		   14, 1, -6, -9, -9, -7, -4, -1);
 static const u32 nearest_neighbour_kernel[] =
 	VC4_LINEAR_PHASE_KERNEL(0, 0, 0, 0, 0, 0, 0, 0,
 				1, 1, 1, 1, 255, 255, 255, 255);
@@ -458,14 +481,8 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs,
 
 	dst_kernel = hvs->dlist + space->start;
 
-	for (i = 0; i < VC4_KERNEL_DWORDS; i++) {
-		if (i < VC4_LINEAR_PHASE_KERNEL_DWORDS)
-			writel(kernel[i], &dst_kernel[i]);
-		else {
-			writel(kernel[VC4_KERNEL_DWORDS - i - 1],
-			       &dst_kernel[i]);
-		}
-	}
+	for (i = 0; i < VC4_KERNEL_DWORDS; i++)
+		writel(kernel[i], &dst_kernel[i]);
 
 	return 0;
 }

drivers/gpu/drm/vc4/vc4_plane.c

@@ -15,6 +15,8 @@
  * into the region of the HVS that it has allocated for us.
  */
 
+#include <linux/log2.h>
+
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_atomic_uapi.h>
@@ -688,36 +690,18 @@ static void vc4_write_tpz(struct vc4_plane_state *vc4_state, u32 src, u32 dst)
 #define PHASE_BITS 6
 
 static void vc4_write_ppf(struct vc4_plane_state *vc4_state, u32 src, u32 dst,
-			  u32 xy, int channel, int chroma_offset,
-			  bool no_interpolate)
+			  u32 xy, int channel, unsigned int subsample,
+			  int chroma_offset, bool no_interpolate)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(vc4_state->base.plane->dev);
+	unsigned int sub_shift = ilog2(subsample);
 	u32 scale = src / dst;
 	s32 offset, offset2;
 	s32 phase;
 
 	WARN_ON_ONCE(vc4->gen > VC4_GEN_6_D);
 
-	/*
-	 * Start the phase at 1/2 pixel from the 1st pixel at src_x.
-	 * 1/4 pixel for YUV, plus the offset for chroma siting.
-	 */
-	if (channel) {
-		/*
-		 * The phase is relative to scale_src->x, so shift it for
-		 * display list's x value
-		 */
-		offset = (xy & 0x1ffff) >> (16 - PHASE_BITS) >> 1;
-		offset -= chroma_offset >> (17 - PHASE_BITS);
-		offset += -(1 << PHASE_BITS >> 2);
-	} else {
-		/*
-		 * The phase is relative to scale_src->x, so shift it for
-		 * display list's x value
-		 */
-		offset = (xy & 0xffff) >> (16 - PHASE_BITS);
-		offset += -(1 << PHASE_BITS >> 1);
-
+	if (!channel) {
 		/*
 		 * This is a kludge to make sure the scaling factors are
 		 * consistent with YUV's luma scaling. We lose 1-bit precision
@@ -726,6 +710,25 @@ static void vc4_write_ppf(struct vc4_plane_state *vc4_state, u32 src, u32 dst,
 		scale &= ~1;
 	}
 
+	/*
+	 * Start the phase at 1/2 pixel from the 1st pixel at src_x, less the
+	 * chroma siting offset. The phase is relative to scale_src->x, so
+	 * shift it for the display list's x value. Everything is computed in
+	 * luma pixels and then converted to this channel's pixels, so that a
+	 * subsampled chroma channel lands on the same position as the luma.
+	 */
+	offset = (xy & ((0x10000 << sub_shift) - 1)) >> (16 - PHASE_BITS);
+	offset -= chroma_offset >> (16 - PHASE_BITS);
+	offset -= 1 << PHASE_BITS >> 1;
+	offset >>= sub_shift;
+
+	/*
+	 * Output pixel r samples the source at (r + 1/2) * scale - 1/2, so the
+	 * phase the first output pixel starts at needs half a destination
+	 * pixel's worth of source added to it.
+	 */
+	offset += (s32)(scale >> (17 - PHASE_BITS));
+
 	/*
 	 * There may be a also small error introduced by precision of scale.
 	 * Add half of that as a compromise
@@ -930,7 +933,13 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state,
 {
 	struct vc4_dev *vc4 = to_vc4_dev(state->plane->dev);
 	struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
+	const struct drm_format_info *info = state->fb->format;
 	bool no_interpolate = state->scaling_filter == DRM_SCALING_FILTER_NEAREST_NEIGHBOR;
+	unsigned int hsub = channel ? info->hsub : 1;
+	unsigned int vsub = channel ? info->vsub : 1;
+	/* Chroma siting only has any meaning on a subsampled axis */
+	int siting_h = hsub > 1 ? state->chroma_siting_h : 0;
+	int siting_v = vsub > 1 ? state->chroma_siting_v : 0;
 
 	if (vc4_state->is_yuv444_unity)
 		no_interpolate = 1;
@@ -941,16 +950,14 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state,
 	if (vc4_state->x_scaling[channel] == VC4_SCALING_PPF) {
 		vc4_write_ppf(vc4_state, vc4_state->src_w[channel],
 			      vc4_state->crtc_w, vc4_state->src_x, channel,
-			      state->chroma_siting_h,
-			      no_interpolate);
+			      hsub, siting_h, no_interpolate);
 	}
 
 	/* Ch0 V-PPF Words 0-1: Scaling Parameters, Context */
 	if (vc4_state->y_scaling[channel] == VC4_SCALING_PPF) {
 		vc4_write_ppf(vc4_state, vc4_state->src_h[channel],
 			      vc4_state->crtc_h, vc4_state->src_y, channel,
-			      state->chroma_siting_v,
-			      no_interpolate);
+			      vsub, siting_v, no_interpolate);
 		vc4_dlist_write(vc4_state, 0xc0c0c0c0);
 	}
 
@@ -2838,7 +2845,8 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
 						 BIT(DRM_SCALING_FILTER_DEFAULT) |
 						 BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR));
 
-	drm_plane_create_chroma_siting_properties(plane, 0, 0);
+	/* MPEG-2 / H.264 / HEVC 4:2:0 siting: H cosited, V interstitial */
+	drm_plane_create_chroma_siting_properties(plane, 0, 0x8000);
 
 	if (type == DRM_PLANE_TYPE_PRIMARY)
 		drm_plane_create_zpos_immutable_property(plane, 0);