> Subject: [PATCH] drm/i915/display/xe3p: Interpret 0xF populated-channel > count as 16 >
Hi, Thanks for the patch mostly looks good I have some comments you can find them below > The register MEM_SS_INFO_GLOBAL [Number of populated channels] field > definition is updated with an encoding for 16 channels. > > For 16-channel configuration, program 1111b. A programmed value of 1111b > must be interpreted as 16 channels for memory bandwidth calculations. > > WA: 16030862157 > Bspec: 69131, 68859 > Assisted-by: Claude:claude-opus-4-8 > Signed-off-by: Uma Shankar <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_bw.c | 18 +++++++++++++++--- > drivers/gpu/drm/i915/display/intel_dram.c | 11 +++++++++++ > 2 files changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c > b/drivers/gpu/drm/i915/display/intel_bw.c > index 41539fdfeac5..f1197ce2d9fa 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.c > +++ b/drivers/gpu/drm/i915/display/intel_bw.c > @@ -272,7 +272,13 @@ static int icl_get_qgv_points(struct intel_display > *display, > case INTEL_DRAM_LPDDR4: > case INTEL_DRAM_LPDDR5: > qi->t_bl = 16; > - qi->max_numchannels = 8; > + /* > + * Wa_16030862157 > + * Xe3p supports a fully-populated 16-channel LPDDR > + * config (4 memory controllers x 4 channels); earlier > + * D14+ platforms top out at 8. > + */ > + qi->max_numchannels = DISPLAY_VER(display) >= 35 ? Should this be == 35 and later be extended as and when WA is extended ? > 16 : 8; Since this is a WA lets use the intel_display_wa framework Which would mean defining intel_display_wa enum for this wa And updating the function intel_display_wa() Then we can call this function instead of DISPLAY_VER(display) >= 35 > qi->channel_width = 16; > qi->deinterleave = 4; > break; > @@ -624,10 +630,16 @@ static int tgl_get_bw_info(struct intel_display > *display, > > ipqdepth = min(ipqdepthpch, display_bw_params->displayrtids / > num_channels); > /* > + * Wa_16030862157 > * clperchgroup = 4kpagespermempage * clperchperblock, > - * clperchperblock = 8 / num_channels * interleave > + * clperchperblock = max(8 / num_channels, 1) * interleave > + * > + * The 8 / num_channels truncating divide collapses to 0 for > + * >8-channel configs (16-channel: 8 / 16 = 0); the max(..., 1) floor > + * keeps clperchperblock >= 1 there while preserving the literal > + * truncating divide for <=8-channel configs. > */ > - clperchgroup = 4 * (8 / num_channels) * qi.deinterleave; > + clperchgroup = 4 * max(8 / num_channels, 1) * qi.deinterleave; > > display->bw.num_qgv_points = qi.num_qgv_points; > display->bw.num_psf_gv_points = qi.num_psf_points; diff --git > a/drivers/gpu/drm/i915/display/intel_dram.c > b/drivers/gpu/drm/i915/display/intel_dram.c > index f103f7cba018..df58effca785 100644 > --- a/drivers/gpu/drm/i915/display/intel_dram.c > +++ b/drivers/gpu/drm/i915/display/intel_dram.c > @@ -796,6 +796,17 @@ static int xelpdp_get_dram_info(struct intel_display > *display, struct dram_info > dram_info->num_qgv_points = > REG_FIELD_GET(MTL_N_OF_ENABLED_QGV_POINTS_MASK, val); > /* PSF GV points not supported in D14+ */ > > + /* > + * Wa_16030862157 > + * MEM_SS_INFO_GLOBAL populated-channel field is only 4 bits and > + * cannot encode 16, so on Xe3p the BIOS programs the saturated field > + * value (0xf) to indicate the fully-populated 16-channel config (4 > + * memory controllers x 4 channels). Interpret it as 16. > + */ > + if (DISPLAY_VER(display) >= 35 && Same here I also think this patch series can be broken into two 1st patch which introduces the WA enum and function updating along with this change in dram The 2nd patch updating the bw side of things. Also the subject for 1st patch can be drm/i915/dram and 2nd patch can be drm/i915/bw Regards, Suraj Kandpal > + dram_info->num_channels == > REG_FIELD_MAX(MTL_N_OF_POPULATED_CH_MASK)) > + dram_info->num_channels = 16; > + > if (DISPLAY_VER(display) >= 35) > dram_info->ecc_impacting_de_bw = > REG_FIELD_GET(XE3P_ECC_IMPACTING_DE, val); > > -- > 2.50.1
