On Mon, Jun 12, 2017 at 4:25 PM, Anuj Phogat <anuj.pho...@gmail.com> wrote: > On Mon, Jun 12, 2017 at 3:28 PM, Francisco Jerez <curroje...@riseup.net> > wrote: >> Anuj Phogat <anuj.pho...@gmail.com> writes: >> >>> On Mon, Jun 12, 2017 at 12:22 PM, Francisco Jerez <curroje...@riseup.net> >>> wrote: >>>> Anuj Phogat <anuj.pho...@gmail.com> writes: >>>> >>>>> On Mon, Jun 12, 2017 at 11:10 AM, Francisco Jerez <curroje...@riseup.net> >>>>> wrote: >>>>>> Anuj Phogat <anuj.pho...@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.pho...@gmail.com> >>>>>>> Cc: Francisco Jerez <curroje...@riseup.net> >>>>>>> Cc: "17.1" <mesa-sta...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev