[Mesa-dev] [PATCH 1/2] i965: Fix broxton 2x6 l3 config
Anuj Phogat
anuj.phogat at gmail.com
Thu Jun 15 17:05:10 UTC 2017
On Mon, Jun 12, 2017 at 4:25 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> 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.
>
I'm just putting the formula in words what we already have in get_l3_way_size()
function.
>> 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-dev
mailing list