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

Anuj Phogat anuj.phogat at gmail.com
Mon Jun 12 23:25:44 UTC 2017


On Mon, Jun 12, 2017 at 3:28 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> Anuj Phogat <anuj.phogat at gmail.com> writes:
>
>> 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.
>
> They match the hardware spec's tables exactly up to a scale factor,
> which is documented in the doxygen comment of gen_l3_config.
and understanding how we come up with that scale factor is a useful
information for the developer. That's not documented in there.

> Understanding the relationship between L3 bank count and way size may be
> useful insight, but it's not directly useful for interpreting the BXT
> 2x6 table (the actual unit in KB is), and it's no more relevant for BXT
> 2x6 than it is for any other platform, but it wouldn't make sense to
> duplicate the same comment on top of each one of the tables.
>
I can take care not duplicating it.

>> 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.
>>
>
> I didn't necessarily want you to drop the comment, I suggested an (IMO)
> more straightforward alternative, see below.
>
>>> |   /**
>>> |    * 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


More information about the mesa-stable mailing list