| Message ID | 20210120151611.132591-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915: Nuke not needed members of dram_info | expand |
On Wed, 2021-01-20 at 10:52 -0800, Lucas De Marchi wrote: > On Wed, Jan 20, 2021 at 10:42:46AM -0800, Jose Souza wrote: > > On Wed, 2021-01-20 at 10:31 -0800, Lucas De Marchi wrote: > > > On Wed, Jan 20, 2021 at 07:16:08AM -0800, Jose Souza wrote: > > > > Valid, ranks and bandwidth_kbps are set into dram_info but are not > > > > used anywhere else so nuking it. > > > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_drv.c | 4 +-- > > > > drivers/gpu/drm/i915/i915_drv.h | 3 -- > > > > drivers/gpu/drm/i915/intel_dram.c | 47 +++++++------------------------ > > > > 3 files changed, 12 insertions(+), 42 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > > index f5666b44ea9d..a1cc60de99f0 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -609,8 +609,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) > > > > > > > > intel_opregion_setup(dev_priv); > > > > /* > > > > - * Fill the dram structure to get the system raw bandwidth and > > > > - * dram info. This will be used for memory latency calculation. > > > > + * Fill the dram structure to get the system dram info. This will be > > > > + * used for memory latency calculation. > > > > */ > > > > intel_dram_detect(dev_priv); > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > > > index 8376cff5ba86..250e92910fa1 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -1134,11 +1134,8 @@ struct drm_i915_private { > > > > } wm; > > > > > > > > struct dram_info { > > > > - bool valid; > > > > bool is_16gb_dimm; > > > > u8 num_channels; > > > > - u8 ranks; > > > > - u32 bandwidth_kbps; > > > > bool symmetric_memory; > > > > enum intel_dram_type { > > > > INTEL_DRAM_UNKNOWN, > > > > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c > > > > index 4754296a250e..694fbd8c9cd4 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dram.c > > > > +++ b/drivers/gpu/drm/i915/intel_dram.c > > > > @@ -201,17 +201,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915) > > > > return -EINVAL; > > > > } > > > > > > > > - /* > > > > - * If any of the channel is single rank channel, worst case output > > > > - * will be same as if single rank memory, so consider single rank > > > > - * memory. > > > > - */ > > > > - if (ch0.ranks == 1 || ch1.ranks == 1) > > > > - dram_info->ranks = 1; > > > > - else > > > > - dram_info->ranks = max(ch0.ranks, ch1.ranks); > > > > - > > > > - if (dram_info->ranks == 0) { > > > > + if (ch0.ranks == 0 && ch1.ranks == 0) { > > > > > > previously if any of them were != 0, we would not fall here. > > > > This is the same behavior. > > indeed, I misread the condition > > > > > > > > > > > > > drm_info(&i915->drm, "couldn't get memory rank information\n"); > > > > return -EINVAL; > > > > } > > > > @@ -269,16 +259,12 @@ skl_get_dram_info(struct drm_i915_private *i915) > > > > mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * > > > > SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); > > > > > > > > - dram_info->bandwidth_kbps = dram_info->num_channels * > > > > - mem_freq_khz * 8; > > > > - > > > > - if (dram_info->bandwidth_kbps == 0) { > > > > + if (dram_info->num_channels * mem_freq_khz == 0) { > > > > drm_info(&i915->drm, > > > > "Couldn't get system memory bandwidth\n"); > > > > return -EINVAL; > > > > } > > > > > > > > - dram_info->valid = true; > > > > return 0; > > > > } > > > > > > > > @@ -365,7 +351,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) > > > > struct dram_info *dram_info = &i915->dram_info; > > > > u32 dram_channels; > > > > u32 mem_freq_khz, val; > > > > - u8 num_active_channels; > > > > + u8 num_active_channels, valid_ranks = 0; > > > > int i; > > > > > > > > val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); > > > > @@ -375,10 +361,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) > > > > dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; > > > > num_active_channels = hweight32(dram_channels); > > > > > > > > - /* Each active bit represents 4-byte channel */ > > > > - dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4); > > > > - > > > > - if (dram_info->bandwidth_kbps == 0) { > > > > + if (mem_freq_khz * num_active_channels == 0) { > > > > > > maybe better to replace with a local var? > > > > > > bandwidth_kbps = mem_freq_khz * num_active_channels; > > > > > > and then check it where needed. > > > > The only place it is used is in this if to return -EINVAL, same for the SKL function. > > The multiplication fits under the 80 col limit so don't see why add a local var. > > ok... maybe `if (!mem_freq_khz || !num_active_channels)` then? That works too but I still prefer keep it as close as possible from the previous check. > > > LUcas De Marchi > > > > > > > > > Lucas De Marchi > > > > > > > drm_info(&i915->drm, > > > > "Couldn't get system memory bandwidth\n"); > > > > return -EINVAL; > > > > @@ -410,27 +393,18 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) > > > > dimm.size, dimm.width, dimm.ranks, > > > > intel_dram_type_str(type)); > > > > > > > > - /* > > > > - * If any of the channel is single rank channel, > > > > - * worst case output will be same as if single rank > > > > - * memory, so consider single rank memory. > > > > - */ > > > > - if (dram_info->ranks == 0) > > > > - dram_info->ranks = dimm.ranks; > > > > - else if (dimm.ranks == 1) > > > > - dram_info->ranks = 1; > > > > + if (valid_ranks == 0) > > > > + valid_ranks = dimm.ranks; > > > > > > > > if (type != INTEL_DRAM_UNKNOWN) > > > > dram_info->type = type; > > > > } > > > > > > > > - if (dram_info->type == INTEL_DRAM_UNKNOWN || dram_info->ranks == 0) { > > > > + if (dram_info->type == INTEL_DRAM_UNKNOWN || valid_ranks == 0) { > > > > drm_info(&i915->drm, "couldn't get memory information\n"); > > > > return -EINVAL; > > > > } > > > > > > > > - dram_info->valid = true; > > > > - > > > > return 0; > > > > } > > > > > > > > @@ -456,11 +430,10 @@ void intel_dram_detect(struct drm_i915_private *i915) > > > > if (ret) > > > > return; > > > > > > > > - drm_dbg_kms(&i915->drm, "DRAM bandwidth: %u kBps, channels: %u\n", > > > > - dram_info->bandwidth_kbps, dram_info->num_channels); > > > > + drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels); > > > > > > > > - drm_dbg_kms(&i915->drm, "DRAM ranks: %u, 16Gb DIMMs: %s\n", > > > > - dram_info->ranks, yesno(dram_info->is_16gb_dimm)); > > > > + drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n", > > > > + yesno(dram_info->is_16gb_dimm)); > > > > } > > > > > > > > static u32 gen9_edram_size_mb(struct drm_i915_private *i915, u32 cap) > > > > -- > > > > 2.30.0 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Wed, Jan 20, 2021 at 07:29:37PM +0000, Jose Souza wrote: >On Wed, 2021-01-20 at 10:52 -0800, Lucas De Marchi wrote: >> On Wed, Jan 20, 2021 at 10:42:46AM -0800, Jose Souza wrote: >> > On Wed, 2021-01-20 at 10:31 -0800, Lucas De Marchi wrote: >> > > On Wed, Jan 20, 2021 at 07:16:08AM -0800, Jose Souza wrote: >> > > > Valid, ranks and bandwidth_kbps are set into dram_info but are not >> > > > used anywhere else so nuking it. >> > > > >> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >> > > > --- >> > > > drivers/gpu/drm/i915/i915_drv.c | 4 +-- >> > > > drivers/gpu/drm/i915/i915_drv.h | 3 -- >> > > > drivers/gpu/drm/i915/intel_dram.c | 47 +++++++------------------------ >> > > > 3 files changed, 12 insertions(+), 42 deletions(-) >> > > > >> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> > > > index f5666b44ea9d..a1cc60de99f0 100644 >> > > > --- a/drivers/gpu/drm/i915/i915_drv.c >> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c >> > > > @@ -609,8 +609,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) >> > > > >> > > > intel_opregion_setup(dev_priv); >> > > > /* >> > > > - * Fill the dram structure to get the system raw bandwidth and >> > > > - * dram info. This will be used for memory latency calculation. >> > > > + * Fill the dram structure to get the system dram info. This will be >> > > > + * used for memory latency calculation. >> > > > */ >> > > > intel_dram_detect(dev_priv); >> > > > >> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> > > > index 8376cff5ba86..250e92910fa1 100644 >> > > > --- a/drivers/gpu/drm/i915/i915_drv.h >> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > > > @@ -1134,11 +1134,8 @@ struct drm_i915_private { >> > > > } wm; >> > > > >> > > > struct dram_info { >> > > > - bool valid; >> > > > bool is_16gb_dimm; >> > > > u8 num_channels; >> > > > - u8 ranks; >> > > > - u32 bandwidth_kbps; >> > > > bool symmetric_memory; >> > > > enum intel_dram_type { >> > > > INTEL_DRAM_UNKNOWN, >> > > > diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c >> > > > index 4754296a250e..694fbd8c9cd4 100644 >> > > > --- a/drivers/gpu/drm/i915/intel_dram.c >> > > > +++ b/drivers/gpu/drm/i915/intel_dram.c >> > > > @@ -201,17 +201,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915) >> > > > return -EINVAL; >> > > > } >> > > > >> > > > - /* >> > > > - * If any of the channel is single rank channel, worst case output >> > > > - * will be same as if single rank memory, so consider single rank >> > > > - * memory. >> > > > - */ >> > > > - if (ch0.ranks == 1 || ch1.ranks == 1) >> > > > - dram_info->ranks = 1; >> > > > - else >> > > > - dram_info->ranks = max(ch0.ranks, ch1.ranks); >> > > > - >> > > > - if (dram_info->ranks == 0) { >> > > > + if (ch0.ranks == 0 && ch1.ranks == 0) { >> > > >> > > previously if any of them were != 0, we would not fall here. >> > >> > This is the same behavior. >> >> indeed, I misread the condition >> >> > >> > > >> > > >> > > > drm_info(&i915->drm, "couldn't get memory rank information\n"); >> > > > return -EINVAL; >> > > > } >> > > > @@ -269,16 +259,12 @@ skl_get_dram_info(struct drm_i915_private *i915) >> > > > mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * >> > > > SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); >> > > > >> > > > - dram_info->bandwidth_kbps = dram_info->num_channels * >> > > > - mem_freq_khz * 8; >> > > > - >> > > > - if (dram_info->bandwidth_kbps == 0) { >> > > > + if (dram_info->num_channels * mem_freq_khz == 0) { >> > > > drm_info(&i915->drm, >> > > > "Couldn't get system memory bandwidth\n"); >> > > > return -EINVAL; >> > > > } >> > > > >> > > > - dram_info->valid = true; >> > > > return 0; >> > > > } >> > > > >> > > > @@ -365,7 +351,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) >> > > > struct dram_info *dram_info = &i915->dram_info; >> > > > u32 dram_channels; >> > > > u32 mem_freq_khz, val; >> > > > - u8 num_active_channels; >> > > > + u8 num_active_channels, valid_ranks = 0; >> > > > int i; >> > > > >> > > > val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); >> > > > @@ -375,10 +361,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) >> > > > dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; >> > > > num_active_channels = hweight32(dram_channels); >> > > > >> > > > - /* Each active bit represents 4-byte channel */ >> > > > - dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4); >> > > > - >> > > > - if (dram_info->bandwidth_kbps == 0) { >> > > > + if (mem_freq_khz * num_active_channels == 0) { >> > > >> > > maybe better to replace with a local var? >> > > >> > > bandwidth_kbps = mem_freq_khz * num_active_channels; >> > > >> > > and then check it where needed. >> > >> > The only place it is used is in this if to return -EINVAL, same for the SKL function. >> > The multiplication fits under the 80 col limit so don't see why add a local var. >> >> ok... maybe `if (!mem_freq_khz || !num_active_channels)` then? > >That works too but I still prefer keep it as close as possible from the previous check. Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Lucas De Marchi > >> >> >> LUcas De Marchi >> >> > >> > > >> > > Lucas De Marchi >> > > >> > > > drm_info(&i915->drm, >> > > > "Couldn't get system memory bandwidth\n"); >> > > > return -EINVAL; >> > > > @@ -410,27 +393,18 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) >> > > > dimm.size, dimm.width, dimm.ranks, >> > > > intel_dram_type_str(type)); >> > > > >> > > > - /* >> > > > - * If any of the channel is single rank channel, >> > > > - * worst case output will be same as if single rank >> > > > - * memory, so consider single rank memory. >> > > > - */ >> > > > - if (dram_info->ranks == 0) >> > > > - dram_info->ranks = dimm.ranks; >> > > > - else if (dimm.ranks == 1) >> > > > - dram_info->ranks = 1; >> > > > + if (valid_ranks == 0) >> > > > + valid_ranks = dimm.ranks; >> > > > >> > > > if (type != INTEL_DRAM_UNKNOWN) >> > > > dram_info->type = type; >> > > > } >> > > > >> > > > - if (dram_info->type == INTEL_DRAM_UNKNOWN || dram_info->ranks == 0) { >> > > > + if (dram_info->type == INTEL_DRAM_UNKNOWN || valid_ranks == 0) { >> > > > drm_info(&i915->drm, "couldn't get memory information\n"); >> > > > return -EINVAL; >> > > > } >> > > > >> > > > - dram_info->valid = true; >> > > > - >> > > > return 0; >> > > > } >> > > > >> > > > @@ -456,11 +430,10 @@ void intel_dram_detect(struct drm_i915_private *i915) >> > > > if (ret) >> > > > return; >> > > > >> > > > - drm_dbg_kms(&i915->drm, "DRAM bandwidth: %u kBps, channels: %u\n", >> > > > - dram_info->bandwidth_kbps, dram_info->num_channels); >> > > > + drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels); >> > > > >> > > > - drm_dbg_kms(&i915->drm, "DRAM ranks: %u, 16Gb DIMMs: %s\n", >> > > > - dram_info->ranks, yesno(dram_info->is_16gb_dimm)); >> > > > + drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n", >> > > > + yesno(dram_info->is_16gb_dimm)); >> > > > } >> > > > >> > > > static u32 gen9_edram_size_mb(struct drm_i915_private *i915, u32 cap) >> > > > -- >> > > > 2.30.0 >> > > > >> > > > _______________________________________________ >> > > > Intel-gfx mailing list >> > > > Intel-gfx@lists.freedesktop.org >> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index f5666b44ea9d..a1cc60de99f0 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -609,8 +609,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv) intel_opregion_setup(dev_priv); /* - * Fill the dram structure to get the system raw bandwidth and - * dram info. This will be used for memory latency calculation. + * Fill the dram structure to get the system dram info. This will be + * used for memory latency calculation. */ intel_dram_detect(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8376cff5ba86..250e92910fa1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1134,11 +1134,8 @@ struct drm_i915_private { } wm; struct dram_info { - bool valid; bool is_16gb_dimm; u8 num_channels; - u8 ranks; - u32 bandwidth_kbps; bool symmetric_memory; enum intel_dram_type { INTEL_DRAM_UNKNOWN, diff --git a/drivers/gpu/drm/i915/intel_dram.c b/drivers/gpu/drm/i915/intel_dram.c index 4754296a250e..694fbd8c9cd4 100644 --- a/drivers/gpu/drm/i915/intel_dram.c +++ b/drivers/gpu/drm/i915/intel_dram.c @@ -201,17 +201,7 @@ skl_dram_get_channels_info(struct drm_i915_private *i915) return -EINVAL; } - /* - * If any of the channel is single rank channel, worst case output - * will be same as if single rank memory, so consider single rank - * memory. - */ - if (ch0.ranks == 1 || ch1.ranks == 1) - dram_info->ranks = 1; - else - dram_info->ranks = max(ch0.ranks, ch1.ranks); - - if (dram_info->ranks == 0) { + if (ch0.ranks == 0 && ch1.ranks == 0) { drm_info(&i915->drm, "couldn't get memory rank information\n"); return -EINVAL; } @@ -269,16 +259,12 @@ skl_get_dram_info(struct drm_i915_private *i915) mem_freq_khz = DIV_ROUND_UP((val & SKL_REQ_DATA_MASK) * SKL_MEMORY_FREQ_MULTIPLIER_HZ, 1000); - dram_info->bandwidth_kbps = dram_info->num_channels * - mem_freq_khz * 8; - - if (dram_info->bandwidth_kbps == 0) { + if (dram_info->num_channels * mem_freq_khz == 0) { drm_info(&i915->drm, "Couldn't get system memory bandwidth\n"); return -EINVAL; } - dram_info->valid = true; return 0; } @@ -365,7 +351,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) struct dram_info *dram_info = &i915->dram_info; u32 dram_channels; u32 mem_freq_khz, val; - u8 num_active_channels; + u8 num_active_channels, valid_ranks = 0; int i; val = intel_uncore_read(&i915->uncore, BXT_P_CR_MC_BIOS_REQ_0_0_0); @@ -375,10 +361,7 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) dram_channels = val & BXT_DRAM_CHANNEL_ACTIVE_MASK; num_active_channels = hweight32(dram_channels); - /* Each active bit represents 4-byte channel */ - dram_info->bandwidth_kbps = (mem_freq_khz * num_active_channels * 4); - - if (dram_info->bandwidth_kbps == 0) { + if (mem_freq_khz * num_active_channels == 0) { drm_info(&i915->drm, "Couldn't get system memory bandwidth\n"); return -EINVAL; @@ -410,27 +393,18 @@ static int bxt_get_dram_info(struct drm_i915_private *i915) dimm.size, dimm.width, dimm.ranks, intel_dram_type_str(type)); - /* - * If any of the channel is single rank channel, - * worst case output will be same as if single rank - * memory, so consider single rank memory. - */ - if (dram_info->ranks == 0) - dram_info->ranks = dimm.ranks; - else if (dimm.ranks == 1) - dram_info->ranks = 1; + if (valid_ranks == 0) + valid_ranks = dimm.ranks; if (type != INTEL_DRAM_UNKNOWN) dram_info->type = type; } - if (dram_info->type == INTEL_DRAM_UNKNOWN || dram_info->ranks == 0) { + if (dram_info->type == INTEL_DRAM_UNKNOWN || valid_ranks == 0) { drm_info(&i915->drm, "couldn't get memory information\n"); return -EINVAL; } - dram_info->valid = true; - return 0; } @@ -456,11 +430,10 @@ void intel_dram_detect(struct drm_i915_private *i915) if (ret) return; - drm_dbg_kms(&i915->drm, "DRAM bandwidth: %u kBps, channels: %u\n", - dram_info->bandwidth_kbps, dram_info->num_channels); + drm_dbg_kms(&i915->drm, "DRAM channels: %u\n", dram_info->num_channels); - drm_dbg_kms(&i915->drm, "DRAM ranks: %u, 16Gb DIMMs: %s\n", - dram_info->ranks, yesno(dram_info->is_16gb_dimm)); + drm_dbg_kms(&i915->drm, "DRAM 16Gb DIMMs: %s\n", + yesno(dram_info->is_16gb_dimm)); } static u32 gen9_edram_size_mb(struct drm_i915_private *i915, u32 cap)
Valid, ranks and bandwidth_kbps are set into dram_info but are not used anywhere else so nuking it. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 4 +-- drivers/gpu/drm/i915/i915_drv.h | 3 -- drivers/gpu/drm/i915/intel_dram.c | 47 +++++++------------------------ 3 files changed, 12 insertions(+), 42 deletions(-)