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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Nov 05, 2026 at 02:12:40PM +0000, sashiko-bot@kernel.org wrote:
> 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, 

This ^. And please also adjust intel_dsb_align() the same way
so that the two functions will remain nearly identical.

> or if
> unconditional cacheline alignment before GOSUB is intended, should
> intel_dsb_gosub() call intel_dsb_align_tail() directly instead of
> duplicating it?

-- 
Ville Syrjälä
Intel