Re: [Intel-gfx] [PATCH 1/4] drm/i915: Nuke not needed members of dram_info

["Souza, Jose" <jose.souza@intel.com>]

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
> > 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx