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

[Francisco Jerez]

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.

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

> + */
> +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.

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

> +   /* 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.

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

>  }
>  
>  /**
> @@ -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.

>     return (devinfo->gen >= 8 ? devinfo->num_slices : 1);
>  }
>  
> -- 
> 2.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170418/0a2c183d/attachment.sig>