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.
> 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