From 6f99ae0f0c7a2561cd3fb93bf42ba8837f9b1c42 Mon Sep 17 00:00:00 2001 From: Dom Cobley Date: Mon, 14 Sep 2026 16:30:40 +0100 Subject: [PATCH 1/5] drm/vc4: Don't assume 4:2:0 subsampling in the scaler phase calculation vc4_write_ppf() treated anything on channel 1 as being subsampled 2x in the axis being programmed: it halved the source coordinate, applied a 1/4 chroma-pixel centring offset instead of 1/2, and scaled the chroma siting by 17 bits rather than 16. That only holds for 4:2:0. For 4:2:2 the chroma plane is full height - src_h[1] == src_h[0], and the vertical PPF is force-enabled so that unity-scaled planes render at all - and for 4:4:4 the chroma plane is full resolution in both axes. In those cases the chroma phase came out at half the luma phase, displacing chroma by a quarter of a pixel against the luma. Take the subsampling for the axis being programmed from the format and derive the phase from it, rather than from the channel number. The calculation is now done in luma pixels throughout and converted to the channel's pixels at the end, which for 4:2:0 gives a bit-identical phase to before. Fixes: ac69c01fa8250 ("drm/vc4: plane: Handle fractional coordinates using the phase field") Signed-off-by: Dom Cobley --- drivers/gpu/drm/vc4/vc4_plane.c | 49 ++++++++++++++++----------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index faee4198f4da0c..9df1429b51818e 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -15,6 +15,8 @@ * into the region of the HVS that it has allocated for us. */ +#include + #include #include #include @@ -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,18 @@ 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; + /* * There may be a also small error introduced by precision of scale. * Add half of that as a compromise @@ -930,7 +926,10 @@ 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; if (vc4_state->is_yuv444_unity) no_interpolate = 1; @@ -941,16 +940,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, state->chroma_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, state->chroma_siting_v, no_interpolate); vc4_dlist_write(vc4_state, 0xc0c0c0c0); } From f8f55c928db6e54d1fc48577158ec5aa5db7dc39 Mon Sep 17 00:00:00 2001 From: Dom Cobley Date: Mon, 14 Sep 2026 16:30:57 +0100 Subject: [PATCH 2/5] drm/vc4: Ignore chroma siting on a non-subsampled axis Chroma siting describes where the chroma samples sit relative to the luma samples, so it only says anything on an axis that is actually subsampled. On a full-resolution axis - vertically for 4:2:2, both axes for 4:4:4 - the chroma is by definition cosited with the luma, but the property was applied there regardless, shifting the chroma away from the luma. Fixes: b7fcc0d961dd6 ("vc4/drm:plane: Make use of chroma siting parameter") Signed-off-by: Dom Cobley --- drivers/gpu/drm/vc4/vc4_plane.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 9df1429b51818e..a29d9046baf9e7 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -930,6 +930,9 @@ static void vc4_write_scaling_parameters(struct drm_plane_state *state, 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; @@ -940,14 +943,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, - hsub, 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, - vsub, state->chroma_siting_v, no_interpolate); + vsub, siting_v, no_interpolate); vc4_dlist_write(vc4_state, 0xc0c0c0c0); } From e8444b19383e01fa18a4fc2e3092f46b9d50c3c5 Mon Sep 17 00:00:00 2001 From: Dom Cobley Date: Tue, 15 Sep 2026 16:43:50 +0100 Subject: [PATCH 3/5] drm/vc4: Add the missing half-destination-pixel term to the PPF phase vc4_write_ppf() starts the phase at (src_x - siting - 1/2) / subsample but output pixel r samples the source at (r + 1/2) * scale - 1/2, so the initial phase also needs half a destination pixel's worth of source: (src_x - siting - 1/2) / subsample + scale/2 Without it everything the PPF touches is displaced by half a destination pixel's worth of source. For 4:2:0 chroma composited 1:1 that is 0.25 chroma pixels, i.e. half a luma line, and since the luma at 1:1 goes through SCALING_NONE rather than the PPF, the chroma ends up half a line adrift of the luma it belongs to. Measured on a Pi 5 by compositing a synthetic 4:2:0 frame 1:1 and capturing it through the writeback connector: with cosited chroma and CHROMA_SITING_V=0 the chroma came back 0.371 luma rows early (0.378 rows on the horizontal axis), against a luma control that measured 0.000. The siting properties themselves scale correctly at -1.00 luma pixels per unit. Fixes: ac69c01fa8250 ("drm/vc4: plane: Handle fractional coordinates using the phase field") Signed-off-by: Dom Cobley --- drivers/gpu/drm/vc4/vc4_plane.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index a29d9046baf9e7..ffb74d8d68d8ac 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -722,6 +722,13 @@ static void vc4_write_ppf(struct vc4_plane_state *vc4_state, u32 src, u32 dst, 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 From 431e5499fe42688a964972b525910f6b657fd074 Mon Sep 17 00:00:00 2001 From: Dom Cobley Date: Tue, 15 Sep 2026 17:57:34 +0100 Subject: [PATCH 4/5] drm/vc4: Default vertical chroma siting to interstitial The chroma siting properties defaulted to (0, 0), placing the chroma samples on the top-left luma sample. MPEG-2, H.264 and HEVC 4:2:0 are horizontally cosited but vertically interstitial, so the default left typical video content half a luma line out unless userspace knew about this property - which is downstream-only - and set it. Default to (0, 0x8000) instead, which is right for the common case. This is only an improvement once the PPF phase is right. Measured on a Pi 5 by compositing a synthetic 4:2:0 frame 1:1 and capturing it through the writeback connector, PSNR against an ideal reference, whole frame: SITING_V=0 SITING_V=0.5 before the scale/2 fix 21.01 dB 20.67 dB after 20.21 dB 21.20 dB Before that fix the pipeline sampled chroma 0.38 luma rows early, which a siting of 0 partly cancelled and 0.5 made worse - so this change on its own would have been a regression. Signed-off-by: Dom Cobley --- drivers/gpu/drm/vc4/vc4_plane.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index ffb74d8d68d8ac..f36cf96c51b6f1 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -2845,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); From 0c128f149d2d8d56c82523330fd606de92a97672 Mon Sep 17 00:00:00 2001 From: Dom Cobley Date: Tue, 15 Sep 2026 17:59:21 +0100 Subject: [PATCH 5/5] drm/vc4: Tabulate the PPF filter kernel at the phases the hardware uses The PPF is a 4-tap filter with 64 phases. Only 8 key phases are stored - 32 coefficients, indexed tap * 8 + phase - and the hardware linearly interpolates the other 56 from them. The key phases sit at 0, 1/8, ..., 7/8 of a source pixel. The Mitchell-Netravali table was tabulated at (p + 1/2) / 8 instead: 1/16, 3/16, ..., 15/16. Every stored phase, and therefore every interpolated one, was half a key phase - 1/16 of a source pixel - late. For 4:2:0 chroma composited 1:1 that is an eighth of a luma line, on both axes, and it does not respond to CHROMA_SITING because the siting moves the requested phase and this moved the realised one. It was tabulated that way because vc4_hvs_upload_linear_kernel() only took the first 16 coefficients and mirrored them, which requires f(p) + f(7 - p) == 1, and (p + 1/2) / 8 is the only spacing that gives that. The hardware stores all 32 coefficients and has no such requirement, so write all 32 and put the key phases where they belong. The nearest-neighbour kernel is unchanged: the new macro emits exactly the dwords the old mirroring did for a symmetric table. While at it the coefficients are rounded so that every phase sums to exactly 256 and is within 0.63 counts of the ideal kernel. Phases 0 and 7 were out by up to 5.7 counts and summed to 247, and SCALER_PPF_AGC fixes a bad sum by rewriting the largest coefficient, so those two phases had their dominant tap inflated from 227 to 236 and smoothed noticeably less than the other six. Measured on a Pi 5 through the writeback connector, chroma sampling offset against the source's own siting, in luma pixels: vertical horizontal old table +0.125 +0.123 this table +0.011 +0.021 luma control 0.000 0.000 The old table's own centroids predicted +0.129 and this one's 0.000. 4:2:2 vertical chroma, which the old table put at +0.037, now measures 0.000. PSNR of an interstitially-sited 4:2:0 test frame against its ideal now peaks with CHROMA_SITING_V at 0.5, where it should; with the old table the peak was at 0.625. Fixes: 21af94cf1a4c2 ("drm/vc4: Add support for scaling of display planes.") Signed-off-by: Dom Cobley --- drivers/gpu/drm/vc4/vc4_hvs.c | 55 +++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index aa51aba723a38d..8e0c64ff64729a 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/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; }