[Mesa-dev] [PATCH 11/12] i965/cnl: Properly handle l3 configuration

Tue Apr 25 16:41:54 UTC 2017

On Mon, Apr 24, 2017 at 9:15 PM, Ben Widawsky <ben at 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.phogat at gmail.com> writes:
>>
>> From: Ben Widawsky <benjamin.widawsky at intel.com>
>>>
>>> V2: Squash the changes in one patch and rebased on master (Anuj).
>>>
>>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>>> Signed-off-by: Anuj Phogat <anuj.phogat at 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
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170425/86aba555/attachment.html>