Thanks for the review! Updated the patch BR Vinod
On Tue, 2026-06-16 at 20:49 +0300, Ville Syrjälä wrote: > On Tue, Jun 16, 2026 at 01:15:53PM +0300, Vinod Govindapillai wrote: > > Bspec has been updated for xe2_lpd+ platforms on how to handle > > the bw buddy prgramming in case no matching memory configuration > > is found w.r.t the current page mask table. The recommendation > > is to keep the default settings for the related registers as it > > is without explicitly disabling the bw buddy. > > > > Bspec: 68871 > > Suggested-by: Ville Syrjala <[email protected]> > > Signed-off-by: Vinod Govindapillai <[email protected]> > > --- > > .../drm/i915/display/intel_display_power.c | 43 +++++++++++++-- > > ---- > > 1 file changed, 30 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c > > b/drivers/gpu/drm/i915/display/intel_display_power.c > > index 2e51dfcd5dce..dcfd78f48622 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > @@ -1631,23 +1631,40 @@ static void tgl_bw_buddy_init(struct > > intel_display *display) > > table[config].type == dram_info->type) > > break; > > > > + /* > > + * If we don't recognize the memory configuration, > > explicitly disable > > + * the address buddy logic in pre-xe2_lpd platforms as it > > was before. > > + * In xe2_lpd+ cases, page masks must be set to 0 if no > > matching > > + * configuration is found in the table. So keep the > > default settings > > + * as it is. By default, BW_BUDDY_CTL bit 31 is 0 (bw > > buddy enabled) > > + * and BW_BUDDY_PAGE_MASK is 0x0 > > This seems to mostly just repeat what the code already says. > > > + * > > + * TODO: Revisit the buddy page masks table when bspec > > updates the > > + * table with the correct number of channels for each dram > > type. > > + */ > > That TODO is unrelated to the changes here, and also seems misplaced. > > > if (table[config].page_mask == 0) { > > drm_dbg_kms(display->drm, > > "Unknown memory configuration; > > disabling address buddy logic.\n"); > > - for_each_set_bit(i, &abox_mask, > > BITS_PER_TYPE(abox_mask)) > > - intel_de_write(display, BW_BUDDY_CTL(i), > > - BW_BUDDY_DISABLE); > > - } else { > > - for_each_set_bit(i, &abox_mask, > > BITS_PER_TYPE(abox_mask)) { > > - intel_de_write(display, > > BW_BUDDY_PAGE_MASK(i), > > - table[config].page_mask); > > - > > - /* Wa_22010178259:tgl,dg1,rkl,adl-s */ > > - if (intel_display_wa(display, > > INTEL_DISPLAY_WA_22010178259)) > > - intel_de_rmw(display, > > BW_BUDDY_CTL(i), > > - > > BW_BUDDY_TLB_REQ_TIMER_MASK, > > - > > BW_BUDDY_TLB_REQ_TIMER(0x8)); > > + > > + if (DISPLAY_VER(display) < 20) { > > + for_each_set_bit(i, &abox_mask, > > BITS_PER_TYPE(abox_mask)) > > + intel_de_write(display, > > BW_BUDDY_CTL(i), > > + BW_BUDDY_DISABLE); > > } > > + > > + return; > > + } > > + > > + /* We found a matching configuration. Program the BW_BUDDY > > registers. */ > > Seems obvious. The comment is redundant. > > > + for_each_set_bit(i, &abox_mask, BITS_PER_TYPE(abox_mask)) > > { > > + intel_de_write(display, BW_BUDDY_PAGE_MASK(i), > > + table[config].page_mask); > > Please split code restructuring from functional changes. This diff > is too hard to read. > > > + > > + /* Wa_22010178259:tgl,dg1,rkl,adl-s */ > > + if (intel_display_wa(display, > > INTEL_DISPLAY_WA_22010178259)) > > + intel_de_rmw(display, BW_BUDDY_CTL(i), > > + BW_BUDDY_TLB_REQ_TIMER_MASK, > > + BW_BUDDY_TLB_REQ_TIMER(0x8)); > > } > > } > > > > -- > > 2.43.0 >
