On Thu, Nov 05, 2026 at 02:12:40PM +0000, [email protected] 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 <[email protected]> > > 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
