[Mesa-dev] [PATCH V2 2/2] i965: Add a variable for size increment per bank in get_l3_way_size()

[Francisco Jerez]

Anuj Phogat <anuj.phogat at gmail.com> writes:

> On Mon, Jun 19, 2017 at 2:18 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Anuj Phogat <anuj.phogat at gmail.com> writes:
>>
>>> Adding min_size_increment_per_bank variable better explains the
>>> computation of L3 way size in the function.
>>>
>>> V2: Use const variable for min_size_increment_per_bank.
>>>
>>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>>> Cc: Francisco Jerez <currojerez at riseup.net>
>>> ---
>>>  src/intel/common/gen_l3_config.c | 9 ++++-----
>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/intel/common/gen_l3_config.c b/src/intel/common/gen_l3_config.c
>>> index 44a4b24..9a7771a 100644
>>> --- a/src/intel/common/gen_l3_config.c
>>> +++ b/src/intel/common/gen_l3_config.c
>>> @@ -271,12 +271,11 @@ gen_get_l3_config(const struct gen_device_info *devinfo,
>>>  static unsigned
>>>  get_l3_way_size(const struct gen_device_info *devinfo)
>>>  {
>>> -   assert(devinfo->l3_banks);
>>> -
>>> -   if (devinfo->is_broxton)
>>> -      return 4;
>>> +   const unsigned min_size_increment_per_bank =
>>
>> I think the name you used in your previous revision (way_size_per_bank)
>> was more descriptive.
>>
> I changed the variable name to better match with what we have in the docs
> after seeing your comment  on [PATCH 1/2]:

The BSpec using a misleading name for something doesn't make it a good
idea for us to use it.  If you want to make it easy for people to find
this information on the hardware docs it would be more useful to add a
reference to the right BSpec page where you got this information.

> "The way size per bank is not really documented in the BSpec so it
> doesn't really give the reader any additional useful information."
>

The point I was trying to make is that the way size per bank is not
documented consistently across generations (when at all) and you can
only reconstruct it after pulling together information scattered over
the whole BSpec.  OTOH the total way size is readily available so
arguing about the former only seemed like a way to confuse the reader,
particularly in the context of BXT 2x6 that has a single L3 bank.

> I don't feel strongly about either of the names. So, I'll just make the
> change and push the patch upstream.
>
>>> +      (devinfo->gen >= 9 && devinfo->l3_banks == 1) ? 4 : 2;
>>
>> Redundant parenthesis.  With my (cosmetic) suggestions taken into
>> account patch is:
>>
>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>>
>>>
>>> -   return 2 * devinfo->l3_banks;
>>> +   assert(devinfo->l3_banks);
>>> +   return min_size_increment_per_bank * devinfo->l3_banks;
>>>  }
>>>
>>>  /**
>>> --
>>> 2.9.4
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170619/0c91de4f/attachment.sig>