Re: [PATCH v9 15/16] drm/i915/gsc: allocate extended operational memory in LMEM

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

I had provided rb on vers 7 and i see the only difference here in v9 is the usage of I915_BO_ALLOC_CPU_CLEAR in
gsc_ext_om_alloc saving us a few lines for free. Thus:

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>



On Thu, 2022-09-08 at 00:51 +0300, Winkler, Tomas wrote:
> GSC requires more operational memory than available on chip.
> Reserve 4M of LMEM for GSC operation. The memory is provided to the
> GSC as struct resource to the auxiliary data of the child device.
> 
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
> V9: Use I915_BO_ALLOC_CPU_CLEAR to clear the allocated memory
>     instead of doing a manual memset (Matt)
> 
>  drivers/gpu/drm/i915/gt/intel_gsc.c | 79 ++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/gt/intel_gsc.h |  3 ++
>  2 files changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
> index e1040c8f2fd3..7af6db3194dd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -7,6 +7,7 @@
>  #include <linux/mei_aux.h>
>  #include "i915_drv.h"
>  #include "i915_reg.h"
> +#include "gem/i915_gem_region.h"
>  #include "gt/intel_gsc.h"
>  #include "gt/intel_gt.h"
>  
> @@ -36,12 +37,56 @@ static int gsc_irq_init(int irq)
>  	return irq_set_chip_data(irq, NULL);
>  }
>  
> +static int
> +gsc_ext_om_alloc(struct intel_gsc *gsc, struct intel_gsc_intf *intf, size_t size)
> +{
> +	struct intel_gt *gt = gsc_to_gt(gsc);
> +	struct drm_i915_gem_object *obj;
> +	int err;
> +
> +	obj = i915_gem_object_create_lmem(gt->i915, size,
> +					  I915_BO_ALLOC_CONTIGUOUS |
> +					  I915_BO_ALLOC_CPU_CLEAR);
> +	if (IS_ERR(obj)) {
> +		drm_err(&gt->i915->drm, "Failed to allocate gsc memory\n");
> +		return PTR_ERR(obj);
> +	}
> +
> +	err = i915_gem_object_pin_pages_unlocked(obj);
> +	if (err) {
> +		drm_err(&gt->i915->drm, "Failed to pin pages for gsc memory\n");
> +		goto out_put;
> +	}
> +
> +	intf->gem_obj = obj;
> +
> +	return 0;
> +
> +out_put:
> +	i915_gem_object_put(obj);
> +	return err;
> +}
> +
> +static void gsc_ext_om_destroy(struct intel_gsc_intf *intf)
> +{
> +	struct drm_i915_gem_object *obj = fetch_and_zero(&intf->gem_obj);
> +
> +	if (!obj)
> +		return;
> +
> +	if (i915_gem_object_has_pinned_pages(obj))
> +		i915_gem_object_unpin_pages(obj);
> +
> +	i915_gem_object_put(obj);
> +}
> +
>  struct gsc_def {
>  	const char *name;
>  	unsigned long bar;
>  	size_t bar_size;
>  	bool use_polling;
>  	bool slow_firmware;
> +	size_t lmem_size;
>  };
>  
>  /* gsc resources and definitions (HECI1 and HECI2) */
> @@ -74,6 +119,7 @@ static const struct gsc_def gsc_def_dg2[] = {
>  		.name = "mei-gsc",
>  		.bar = DG2_GSC_HECI1_BASE,
>  		.bar_size = GSC_BAR_LENGTH,
> +		.lmem_size = SZ_4M,
>  	},
>  	{
>  		.name = "mei-gscfi",
> @@ -90,26 +136,32 @@ static void gsc_release_dev(struct device *dev)
>  	kfree(adev);
>  }
>  
> -static void gsc_destroy_one(struct intel_gsc_intf *intf)
> +static void gsc_destroy_one(struct drm_i915_private *i915,
> +			    struct intel_gsc *gsc, unsigned int intf_id)
>  {
> +	struct intel_gsc_intf *intf = &gsc->intf[intf_id];
> +
>  	if (intf->adev) {
>  		auxiliary_device_delete(&intf->adev->aux_dev);
>  		auxiliary_device_uninit(&intf->adev->aux_dev);
>  		intf->adev = NULL;
>  	}
> +
>  	if (intf->irq >= 0)
>  		irq_free_desc(intf->irq);
>  	intf->irq = -1;
> +
> +	gsc_ext_om_destroy(intf);
>  }
>  
> -static void gsc_init_one(struct drm_i915_private *i915,
> -			 struct intel_gsc_intf *intf,
> +static void gsc_init_one(struct drm_i915_private *i915, struct intel_gsc *gsc,
>  			 unsigned int intf_id)
>  {
>  	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>  	struct mei_aux_device *adev;
>  	struct auxiliary_device *aux_dev;
>  	const struct gsc_def *def;
> +	struct intel_gsc_intf *intf = &gsc->intf[intf_id];
>  	int ret;
>  
>  	intf->irq = -1;
> @@ -141,7 +193,7 @@ static void gsc_init_one(struct drm_i915_private *i915,
>  	intf->irq = irq_alloc_desc(0);
>  	if (intf->irq < 0) {
>  		drm_err(&i915->drm, "gsc irq error %d\n", intf->irq);
> -		return;
> +		goto fail;
>  	}
>  
>  	ret = gsc_irq_init(intf->irq);
> @@ -155,6 +207,19 @@ static void gsc_init_one(struct drm_i915_private *i915,
>  	if (!adev)
>  		goto fail;
>  
> +	if (def->lmem_size) {
> +		drm_dbg(&i915->drm, "setting up GSC lmem\n");
> +
> +		if (gsc_ext_om_alloc(gsc, intf, def->lmem_size)) {
> +			drm_err(&i915->drm, "setting up gsc extended operational memory failed\n");
> +			kfree(adev);
> +			goto fail;
> +		}
> +
> +		adev->ext_op_mem.start = i915_gem_object_get_dma_address(intf->gem_obj, 0);
> +		adev->ext_op_mem.end = adev->ext_op_mem.start + def->lmem_size;
> +	}
> +
>  	adev->irq = intf->irq;
>  	adev->bar.parent = &pdev->resource[0];
>  	adev->bar.start = def->bar + pdev->resource[0].start;
> @@ -188,7 +253,7 @@ static void gsc_init_one(struct drm_i915_private *i915,
>  
>  	return;
>  fail:
> -	gsc_destroy_one(intf);
> +	gsc_destroy_one(i915, gsc, intf->id);
>  }
>  
>  static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
> @@ -229,7 +294,7 @@ void intel_gsc_init(struct intel_gsc *gsc, struct drm_i915_private *i915)
>  		return;
>  
>  	for (i = 0; i < INTEL_GSC_NUM_INTERFACES; i++)
> -		gsc_init_one(i915, &gsc->intf[i], i);
> +		gsc_init_one(i915, gsc, i);
>  }
>  
>  void intel_gsc_fini(struct intel_gsc *gsc)
> @@ -241,5 +306,5 @@ void intel_gsc_fini(struct intel_gsc *gsc)
>  		return;
>  
>  	for (i = 0; i < INTEL_GSC_NUM_INTERFACES; i++)
> -		gsc_destroy_one(&gsc->intf[i]);
> +		gsc_destroy_one(gt->i915, gsc, i);
>  }
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.h b/drivers/gpu/drm/i915/gt/intel_gsc.h
> index 68582f912b21..fcac1775e9c3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.h
> @@ -20,11 +20,14 @@ struct mei_aux_device;
>  
>  /**
>   * struct intel_gsc - graphics security controller
> + *
> + * @gem_obj: scratch memory GSC operations
>   * @intf : gsc interface
>   */
>  struct intel_gsc {
>  	struct intel_gsc_intf {
>  		struct mei_aux_device *adev;
> +		struct drm_i915_gem_object *gem_obj;
>  		int irq;
>  		unsigned int id;
>  	} intf[INTEL_GSC_NUM_INTERFACES];
> -- 
> 2.37.2
>