Re: [Intel-gfx] [PATCH 1/1] drm/i915: Export LMEM max memory bandwidth via sysfs

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 21/11/2022 10:01, Himal Prasad Ghimiray wrote:
> Export lmem maximum memory bandwidth to the userspace via sysfs
> 
> (v2)
> Add TODO comment to have client parts specific condition (Anshuman)
> Remove prelim prefix from the sysfs node name (Aravind)

Link to userspace consumer?

> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h   |  2 ++
>   drivers/gpu/drm/i915/i915_sysfs.c | 28 ++++++++++++++++++++++++++++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e1892d147741..1d59b84b86ad2 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6606,6 +6606,8 @@
>   #define	    POWER_SETUP_I1_WATTS		REG_BIT(31)
>   #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
>   #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
> +#define	  PCODE_MEMORY_CONFIG			0x70
> +#define	    MEMORY_CONFIG_SUBCOMMAND_READ_MAX_BANDWIDTH 0x0
>   #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
>   #define   XEHP_PCODE_FREQUENCY_CONFIG		0x6e	/* xehpsdv, pvc */
>   /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 595e8b5749907..69df2012bd10e 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -37,7 +37,10 @@
>   
>   #include "i915_drv.h"
>   #include "i915_sysfs.h"
> +#include "i915_reg.h"
>   #include "intel_pm.h"
> +#include "intel_pcode.h"
> +

Please don't do whitespace changes if there isn't a good reason.

>   
>   struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
>   {
> @@ -231,11 +234,36 @@ static void i915_setup_error_capture(struct device *kdev) {}
>   static void i915_teardown_error_capture(struct device *kdev) {}
>   #endif
>   
> +static ssize_t
> +lmem_max_bw_Mbps_show(struct device *dev, struct device_attribute *attr, char *buff)
> +{
> +	struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> +	u32 val;
> +	int err;
> +
> +	err = snb_pcode_read_p(&i915->uncore, PCODE_MEMORY_CONFIG,
> +			       MEMORY_CONFIG_SUBCOMMAND_READ_MAX_BANDWIDTH,
> +			       0x0, &val);
> +	if (err)
> +		return err;
> +
> +	return sysfs_emit(buff, "%u\n", val);
> +}
> +
> +static DEVICE_ATTR_RO(lmem_max_bw_Mbps);
> +
>   void i915_setup_sysfs(struct drm_i915_private *dev_priv)
>   {
>   	struct device *kdev = dev_priv->drm.primary->kdev;
>   	int ret;
>   
> +	/*TODO: Need to add client Parts condition check. */

What does this mean? Are DG1 and DG2 not client parts?

> +	if (IS_DG1(dev_priv) || IS_DG2(dev_priv)) {
> +		ret = sysfs_create_file(&kdev->kobj, &dev_attr_lmem_max_bw_Mbps.attr);
> +		if (ret)
> +			drm_err(&dev_priv->drm, "Setting up sysfs to read max B/W failed\n");

I suggest at most drm_warn since error is ignored.

I also suggest expanding B/W to memory bandwidth. Maybe "Failed to 
create maximum memory bandwidth sysfs file"?

Regards,

Tvrtko

> +	}
> +
>   	if (HAS_L3_DPF(dev_priv)) {
>   		ret = device_create_bin_file(kdev, &dpf_attrs);
>   		if (ret)