Re: [PATCH] drm/i915/dsb: Fill all padding before GOSUB

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/i915/dsb: intel_dsb_gosub_align() duplicates intel_dsb_align_tail()
--

commit 9778ee8e90748be72ffe6e979a8d71a200d84540
Author: Hanson Wang <hanson.wang@ugreen.com>

drm/i915/dsb: Fill all padding before GOSUB

This commit updates intel_dsb_gosub_align() to zero all bytes skipped when
aligning free_pos to a cacheline boundary before emitting a GOSUB
instruction, rather than only zeroing the gap when at most two QW slots
remain.

> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 299d778520a22..83aeb3fc89425 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -608,8 +608,13 @@ static void intel_dsb_gosub_align(struct intel_dsb *dsb)
>  	/*
>  	 * Wa_16024917128
>  	 * "Ensure GOSUB is not placed in cacheline QW slot 6 or 7 (numbered 0-7)"
> +	 *
> +	 * free_pos always advances to the next cacheline, so the skipped
> +	 * bytes remain inside the programmed command range. Fill the whole
> +	 * gap with NOPs; clearing only slots 6 and 7 leaves a larger gap
> +	 * uninitialized.
>  	 */
> -	if (aligned_tail - tail <= 2 * 8)
> +	if (aligned_tail > tail)
>  		dsb_buffer_fill(dsb, dsb->free_pos, 0, aligned_tail - tail);
>  
>  	dsb->free_pos = aligned_tail / 4;

[Severity: Low]
Wa_16024917128 states that GOSUB must not be placed in cacheline QW slot 6
or 7 (where 0 < aligned_tail - tail <= 2 * 8). When tail is in QW slots 0
through 5, GOSUB is allowed at that slot, and the trailing
intel_dsb_align_tail(dsb) call at the end of intel_dsb_gosub() already pads
the instructions following GOSUB within that cacheline with NOPs:

drivers/gpu/drm/i915/display/intel_dsb.c:intel_dsb_gosub() {
	...
	intel_dsb_gosub_align(dsb);
	...
	intel_dsb_emit(dsb, lower_32_bits(head_tail),
		       (DSB_OPCODE_GOSUB << DSB_OPCODE_SHIFT) |
		       upper_32_bits(head_tail));

	/*
	 * "NOTE: the instructions within the cacheline
	 *  FOLLOWING the GOSUB instruction must be NOPs."
	 */
	intel_dsb_align_tail(dsb);
}

In commit 2c41d62f6fb1, intel_dsb_gosub_align() checked
if (aligned_tail - tail <= 2 * 8) to detect QW slots 6 and 7, but left
dsb->free_pos = aligned_tail / 4 outside the conditional block, which caused
free_pos to advance to the next cacheline without zeroing the skipped bytes
when tail was in QW slots 1 through 5.

With this change to if (aligned_tail > tail), intel_dsb_gosub_align()
becomes an exact duplicate of intel_dsb_align_tail() defined immediately
above it:

static void intel_dsb_align_tail(struct intel_dsb *dsb)
{
	u32 aligned_tail, tail;

	intel_dsb_ins_align(dsb);

	tail = dsb->free_pos * 4;
	aligned_tail = ALIGN(tail, CACHELINE_BYTES);

	if (aligned_tail > tail)
		dsb_buffer_fill(dsb, dsb->free_pos, 0, aligned_tail - tail);

	dsb->free_pos = aligned_tail / 4;
}

Should dsb->free_pos = aligned_tail / 4 instead be moved inside the
if (aligned_tail - tail <= 2 * 8) block so that free_pos only advances to
the next cacheline when GOSUB would land in QW slot 6 or 7, or if
unconditional cacheline alignment before GOSUB is intended, should
intel_dsb_gosub() call intel_dsb_align_tail() directly instead of
duplicating it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105134658.3724628-1-hanson.wang@ugreen.com?part=1