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, 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/[email protected]?part=1
