[Mesa-dev] [PATCH 1/2] i965: Fix broxton 2x6 l3 config
Francisco Jerez
currojerez at riseup.net
Mon Jun 12 22:28:18 UTC 2017
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.
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.
> 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
-------------- 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/20170612/31ade7f7/attachment.sig>
More information about the mesa-dev
mailing list