<div dir="ltr"><div class="gmail_default" style="font-family:verdana,sans-serif;font-size:small"> </div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 24, 2017 at 9:15 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.</blockquote><div class="gmail_default" style="font-family:verdana,sans-serif;font-size:small">Yes, unrelated changes are due to bad rebase. I'll fix them in V2.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>> writes: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> From: Ben Widawsky <<a href="mailto:benjamin.widawsky@intel.com" target="_blank">benjamin.widawsky@intel.com</a>> V2: Squash the changes in one patch and rebased on master (Anuj). Signed-off-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>> --- 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. </blockquote> 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. </blockquote> 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. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Also, programming does not require any + * back scaling. Programming simply works in 2k increments and is scaled by the + * hardware. </blockquote> That's basically the case (up to the specific scale factor) on all hardware, I'd drop this too. </blockquote> 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. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + */ +static const struct gen_l3_config cnl_l3_configs[] = { + /* SLM URB Rest DC RO */ </blockquote> s/Rest/ALL/ (these are L3 partition enum labels), and align to the column boundaries below. </blockquote> Sure.<div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + {{ 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) </blockquote> Unrelated change sneaked in. </blockquote> </div></div> See above reply (not sure how this got in other than rebase). <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + /* 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. + */ </blockquote> 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. </blockquote> Anuj, would you mind doing what Curro asks?</blockquote><div class="gmail_default" style="font-family:verdana,sans-serif;font-size:small">I'll drop the comment.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + if (devinfo->gen >= 10) + return 2 * devinfo->l3_banks; + </blockquote> 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. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + /* XXX: Cherryview and Broxton are always gt1 */ + if (devinfo->gt == 1) </blockquote> Unrelated change. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> return 4; - else - return 8 * devinfo->num_slices; + return 8 * devinfo->num_slices; </blockquote> Unrelated change. </blockquote> I think here too it must have been a rebase change. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> } /** @@ -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; + </blockquote> 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. </blockquote> 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.<div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> return (devinfo->gen >= 8 ? devinfo->num_slices : 1); } -- 2.9.3 </blockquote></blockquote> </div></div></blockquote></div> </div></div>