On Mon, Apr 24, 2017 at 9:15 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
> On 17-04-18 18:18:39, Francisco Jerez wrote: > > Most, if not all of the unrelated changes that snuck in were due to rebase. > Anuj, would you mind fixing those? I tried my best to address the rest, > but I'm > admittedly stumbling my way through some of the l3 programming. Yes, unrelated changes are due to bad rebase. I'll fix them in V2. > > > Anuj Phogat <anuj.pho...@gmail.com> writes: >> >> From: Ben Widawsky <benjamin.widaw...@intel.com> >>> >>> V2: Squash the changes in one patch and rebased on master (Anuj). >>> >>> Signed-off-by: Ben Widawsky <b...@bwidawsk.net> >>> Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com> >>> --- >>> src/intel/common/gen_l3_config.c | 43 ++++++++++++++++++++++++++++++ >>> ++++------ >>> 1 file changed, 37 insertions(+), 6 deletions(-) >>> >>> diff --git a/src/intel/common/gen_l3_config.c >>> b/src/intel/common/gen_l3_config.c >>> index 4fe3503..f3e8793 100644 >>> --- a/src/intel/common/gen_l3_config.c >>> +++ b/src/intel/common/gen_l3_config.c >>> @@ -102,6 +102,26 @@ static const struct gen_l3_config chv_l3_configs[] >>> = { >>> }; >>> >>> /** >>> + * On CNL, RO clients are merged and shared with read/write space. As a >>> result >>> + * we have fewer allocation parameters. >>> >> >> The two sentences above make it sound like RO clients haven't been part >> of the same partition until CNL. They have. I'd drop this. >> >> > So the difference I was trying to spell out is that the previous "IS" "C" > and > "T" fields do not exist in a programmable way. > > Also, programming does not require any >>> + * back scaling. Programming simply works in 2k increments and is >>> scaled by the >>> + * hardware. >>> >> >> That's basically the case (up to the specific scale factor) on all >> hardware, I'd drop this too. >> >> > I personally think the existing code isn't as self-documenting to me as it > is to > you, and so I was trying to spell it out. I was trying to document, not > show > differentiation. In either event, I don't care if we keep this or leave it. > > + */ >>> +static const struct gen_l3_config cnl_l3_configs[] = { >>> + /* SLM URB Rest DC RO */ >>> >> >> s/Rest/ALL/ (these are L3 partition enum labels), and align to the >> column boundaries below. >> >> > Sure. > > > + {{ 0, 64, 64, 0, 0 }}, >>> + {{ 0, 64, 0, 16, 48 }}, >>> + {{ 0, 48, 0, 16, 64 }}, >>> + {{ 0, 32, 0, 0, 96 }}, >>> + {{ 0, 32, 96, 0, 0 }}, >>> + {{ 0, 32, 0, 16, 80 }}, >>> + {{ 32, 16, 80, 0, 0 }}, >>> + {{ 32, 16, 0, 64, 16 }}, >>> + {{ 32, 0, 96, 0, 0 }}, >>> + {{ 0 }} >>> +}; >>> + >>> +/** >>> * Return a zero-terminated array of validated L3 configurations for the >>> * specified device. >>> */ >>> @@ -116,9 +136,11 @@ get_l3_configs(const struct gen_device_info >>> *devinfo) >>> return (devinfo->is_cherryview ? chv_l3_configs : bdw_l3_configs); >>> >>> case 9: >>> - case 10: >>> return chv_l3_configs; >>> >>> + case 10: >>> + return cnl_l3_configs; >>> + >>> default: >>> unreachable("Not implemented"); >>> } >>> @@ -258,13 +280,19 @@ get_l3_way_size(const struct gen_device_info >>> *devinfo) >>> if (devinfo->is_baytrail) >>> return 2; >>> >>> - else if (devinfo->gt == 1 || >>> - devinfo->is_cherryview || >>> - devinfo->is_broxton) >>> >> >> Unrelated change sneaked in. >> >> > See above reply (not sure how this got in other than rebase). > > + /* Way size is actually 6 * num_slices, because it's 2k per bank, and >>> + * normally 3 banks per slice. However, on CNL+ this information >>> isn't >>> + * needed to setup the URB/l3 configuration. We fudge the answer here >>> + * and then use the scaling to fix it up later. >>> + */ >>> >> >> The comment makes it sound like you're lying to the caller and returning >> a bogus way size you're going to fix up later. That's not the case >> though, the value you're returning below is accurate for all CNL >> configs. 6 * num_slices OTOH *would* be inaccurate. I'd drop the >> comment. >> >> > Anuj, would you mind doing what Curro asks? I'll drop the comment. > > > + if (devinfo->gen >= 10) >>> + return 2 * devinfo->l3_banks; >>> + >>> >> >> It would be nice if we could use the 'l3_banks' devinfo field you just >> added on previous gens too in order to simplify things. I'm okay if you >> don't feel like doing the clean up right away but maybe add a short >> FINISHME comment next to its definition in gen_device_info.h so we don't >> forget about this (hopefully temporary) inconsistency. >> >> + /* XXX: Cherryview and Broxton are always gt1 */ >>> + if (devinfo->gt == 1) >>> >> >> Unrelated change. >> >> return 4; >>> >>> - else >>> - return 8 * devinfo->num_slices; >>> + return 8 * devinfo->num_slices; >>> >> >> Unrelated change. >> >> > I think here too it must have been a rebase change. > > } >>> >>> /** >>> @@ -274,6 +302,9 @@ get_l3_way_size(const struct gen_device_info >>> *devinfo) >>> static unsigned >>> get_urb_size_scale(const struct gen_device_info *devinfo) >>> { >>> + if (devinfo->gen == 10) >>> + return devinfo->l3_banks; >>> + >>> >> >> This seems bogus, AFAICT URB programming is done in size per slice units >> (just like it was the case on previous gens), not in size per L3 bank >> units. >> >> > We have parts which have different l3 banks per slice, and those seemed to > need > to be programmed differently which is how I got to this. Tell us what you'd > recommend instead, and Anuj can try to run with that. > > Thanks. > > > return (devinfo->gen >= 8 ? devinfo->num_slices : 1); >>> } >>> >>> -- >>> 2.9.3 >>> >>>
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev