[Mesa-dev] [PATCH 1/2] i965: Fix broxton 2x6 l3 config

[Anuj Phogat]

On Mon, Jun 12, 2017 at 12:22 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Anuj Phogat <anuj.phogat at gmail.com> writes:
>
>> On Mon, Jun 12, 2017 at 11:10 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Anuj Phogat <anuj.phogat at gmail.com> writes:
>>>
>>>> The new table added in this patch matches with the table
>>>> in gfxspecs. We were programming the wrong values earlier.
>>>>
>>>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>>>> Cc: Francisco Jerez <currojerez at riseup.net>
>>>> Cc: "17.1" <mesa-stable at lists.freedesktop.org>
>>>> ---
>>>>  src/intel/common/gen_device_info.c |  1 +
>>>>  src/intel/common/gen_device_info.h |  1 +
>>>>  src/intel/common/gen_l3_config.c   | 19 +++++++++++++++++++
>>>>  3 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/src/intel/common/gen_device_info.c b/src/intel/common/gen_device_info.c
>>>> index 75284a6..eccb464 100644
>>>> --- a/src/intel/common/gen_device_info.c
>>>> +++ b/src/intel/common/gen_device_info.c
>>>> @@ -502,6 +502,7 @@ static const struct gen_device_info gen_device_info_bxt = {
>>>>
>>>>  static const struct gen_device_info gen_device_info_bxt_2x6 = {
>>>>     GEN9_LP_FEATURES_2X6,
>>>> +   .is_broxton_2x6 = 1,
>>>>     .l3_banks = 1,
>>>>  };
>>>>  /*
>>>> diff --git a/src/intel/common/gen_device_info.h b/src/intel/common/gen_device_info.h
>>>> index 6207630..4fe1b21 100644
>>>> --- a/src/intel/common/gen_device_info.h
>>>> +++ b/src/intel/common/gen_device_info.h
>>>> @@ -41,6 +41,7 @@ struct gen_device_info
>>>>     bool is_haswell;
>>>>     bool is_cherryview;
>>>>     bool is_broxton;
>>>> +   bool is_broxton_2x6;
>>>
>>> I don't think this boolean flag is useful.  The hardware spec refers to
>>> the validated configurations that apply to BXT 2x6 as relevant to all
>>> products with a single bank, which means you should probably check for
>>> gen >= 9 and l3_banks == 1 instead.
>>>
>> Right. I'll drop is_broxton_2x6 variable in V2.
>>>>     bool is_kabylake;
>>>>
>>>>     bool has_hiz_and_separate_stencil;
>>>> diff --git a/src/intel/common/gen_l3_config.c b/src/intel/common/gen_l3_config.c
>>>> index ae31d08..e17994b 100644
>>>> --- a/src/intel/common/gen_l3_config.c
>>>> +++ b/src/intel/common/gen_l3_config.c
>>>> @@ -102,6 +102,23 @@ static const struct gen_l3_config chv_l3_configs[] = {
>>>>  };
>>>>
>>>>  /**
>>>> + * BXT 2x6 validated L3 configurations.  \sa ivb_l3_configs.
>>>> + * Number of ways =
>>>> + *    Allocation in KB for SKU / (Way size per bank * Number of banks).
>>>
>>> Is this formula relevant?  The way size per bank is not really
>>> documented in the BSpec so it doesn't really give the reader any
>>> additional useful information.
>>>
>> It is not documented with name "way size per bank" but we have enough
>> comments suggesting the "size increments per bank" for different intel
>> h/w.
>>
>> For CNL L3CNTLREG says:
>> "Increments of 2KB Per bank (L3 size needs to be calculated based on bank
>>   count per SKU)."
>>
>> For SKL "L3 Bank Configuration" section says:
>> "Each L3 bank is identical as described below."
>> For MultiBank SKUs:
>> "Up to 64 ways, up to 128KB, tagged for L3"
>> which computes to 2KB increments per bank.
>>
>> How about using Size increment per bank ? or any other name you suggest?
>> Number of ways =
>>    Allocation in KB for SKU / (Size increment per bank * Number of banks)
>>
>> I find above comment useful to explain the numbers in below table.
>> We have a single table applicable to multiple SKUs and searching the
>> documents every time you want to verify these tables is time
>> consuming. I would like to add the similar comment to other
>> gen_l3_config tables as well.
>>
>
> Why does expressing this in terms of the number of banks help understand
> anything?  The units the table is represented in are already documented
> in the doxygen docs of the gen_l3_config struct.  If you think that
> making it more explicit could save the reader the effort of looking up
> the docs of the structure being defined, how about the less convoluted:
>
Yes, we have the units (number of ways) documented in  gen_l3_config.h.
But what developer needs to know is how we came with the numbers in
these tables. They don't match up directly with what we have in the docs.
Expressing it in terms of number of banks helps the developer understand
how we came up with the values in gen_l3_config struct using tables in
the documents. We have table for multi bank SKL configurations but a
table for per bank configuration in CNL. Ben also had lot of problems
understanding this part of code. Adding him in Cc. I'm willing to drop this
comment if both of you find it incorrect or useless.

> |   /**
> |    * BXT single-bank validated L3 configurations.  For BXT 2x6 parts
> |    * this is effectively in 4 KB units.  \sa ivb_l3_configs.
> |    */
>
> Also, can you format the column headers consistently with the previous
> ones, like:
>
> |   /* SLM URB ALL DC  RO  IS   C   T */
>
ok
>>>> + * For BXT 2x6: Banks = 1, Way size per bank = 4.
>>>> + */
>>>> +static const struct gen_l3_config bxt_2x6_l3_configs[] = {
>>>> +   /*SLM URB  All DC  RO  IS   C   T */
>>>> +   {{  0, 32, 48,  0,  0,  0,  0,  0 }},
>>>> +   {{  0, 32,  0,  8, 40,  0,  0,  0 }},
>>>> +   {{  0, 32,  0, 32, 16,  0,  0,  0 }},
>>>> +   {{ 16, 16, 48,  0,  0,  0,  0,  0 }},
>>>> +   {{ 16, 16,  0, 40,  8,  0,  0,  0 }},
>>>> +   {{ 16, 16,  0, 16, 32,  0,  0,  0 }},
>>>> +   {{ 0 }}
>>>> +};
>>>> +
>>>> +/**
>>>>   * Return a zero-terminated array of validated L3 configurations for the
>>>>   * specified device.
>>>>   */
>>>> @@ -117,6 +134,8 @@ get_l3_configs(const struct gen_device_info *devinfo)
>>>>
>>>>     case 9:
>>>>     case 10:
>>>> +      if (devinfo->is_broxton_2x6)
>>>> +         return bxt_2x6_l3_configs;
>>>>        return chv_l3_configs;
>>>>
>>>>     default:
>>>> --
>>>> 2.9.3