[Mesa-dev] [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-dev
mailing list