Re: [PATCH] [RFC]: drm/i915/display: Use ceiling division for NV12 UV surface offset calculation

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sat, 11 Apr 2026, Vidya Srinivas <vidya.srinivas@intel.com> wrote:
> For LNL+, odd source size and panning for YUV 422/420 surfaces is
> supported. However, it requires the UV (chroma) surface Start X/Y and
> width/height to be calculated as ceiling(half of Y plane value) rather
> than floor. The current code uses (>> 17) which is floor division. For
> odd Y plane values this produces an off-by-one error in the UV plane
> offset.
>
> On Android systems we see PLANE ATS fault when NV12 overlays are
> used with odd source dimensions:
>
> [  126.854200] xe 0000:00:02.0: [drm:intel_atomic_setup_scaler [xe]] [CRTC:148:pipe A] attached scaler id 0.0 to PLANE:33
> [  126.854617] xe 0000:00:02.0: [drm:skl_update_scaler [xe]] [CRTC:148:pipe A] scaler_user index 0.0: staged scaling request for 1279x719->1340x753
> [  126.854837] xe 0000:00:02.0: [drm:intel_plane_atomic_check [xe]] UV plane [PLANE:33:plane 1A] using Y plane [PLANE:123:plane 4A]
> [  126.854926] xe 0000:00:02.0: [drm] *ERROR* [CRTC:148:pipe A] PLANE ATS fault
>
> With Y plane width 1279:
>   floor(1279/2) = 639 (current)
>   ceil(1279/2)  = 640 (required)
>
> Change the UV offset/size calculation to use ceiling division by adding
> (1 << 17) - 1 before shifting. This is a no-op for even values since
> ceiling and floor are equal when the dividend is even.
>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 7a9d494334b5..c455bf92ae99 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -2139,10 +2139,16 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
>  	int min_height = intel_plane_min_height(plane, fb, uv_plane, rotation);
>  	int max_width = intel_plane_max_width(plane, fb, uv_plane, rotation);
>  	int max_height = intel_plane_max_height(plane, fb, uv_plane, rotation);
> -	int x = plane_state->uapi.src.x1 >> 17;
> -	int y = plane_state->uapi.src.y1 >> 17;
> -	int w = drm_rect_width(&plane_state->uapi.src) >> 17;
> -	int h = drm_rect_height(&plane_state->uapi.src) >> 17;
> +
> +	/*
> +	 * LNL+ UV surface start/size =
> +	 * ceiling(half of Y plane start/size). Use ceiling division
> +	 * unconditionally; it is a no-op for even values.
> +	 */
> +	int x = (plane_state->uapi.src.x1 + (1 << 17) - 1) >> 17;
> +	int y = (plane_state->uapi.src.y1 + (1 << 17) - 1) >> 17;
> +	int w = (drm_rect_width(&plane_state->uapi.src) + (1 << 17) - 1) >> 17;
> +	int h = (drm_rect_height(&plane_state->uapi.src) + (1 << 17) - 1) >> 17;

The problem I have with this is that the original >> 17 is already too
magic. It divides a U16.16 fixed point in half, and this is completely
non-obvious.

The commit message doesn't even mention this.

I think this needs a clean separation between the division and the
conversion to int.


BR,
Jani.


>  	u32 offset;
>  
>  	/* FIXME not quite sure how/if these apply to the chroma plane */

-- 
Jani Nikula, Intel