Re: [Intel-gfx] [PATCH v7 01/13] drm/i915/guc: Update GuC ADS size for error capture lists

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

On 3/9/2022 9:30 PM, Matthew Brost wrote:
> On Sat, Feb 26, 2022 at 01:55:29AM -0800, Alan Previn wrote:
> +intel_guc_capture_getlist(struct intel_guc *guc, u32 owner, u32 type, 
> u32 classid,
>> +			  struct file **fileoutptr)
>> +{
>> +	struct __guc_state_capture_priv *gc = guc->capture.priv;
>> +	struct __guc_capture_ads_cache *cache = &gc->ads_cache[owner][type][classid];
>> +	struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>> +	struct guc_debug_capture_list *listnode;
>> +	struct file *file;
>> +	u8 *caplist, *tmp;
>> +	size_t size = 0;
>> +	int ret, num_regs;
>> +
>> +	if (!gc->reglists)
>> +		return -ENODEV;
>> +
>> +	if (cache->is_valid) {
>> +		*fileoutptr = cache->file;
>> +		return cache->status;
>> +	}
>> +
>> +	ret = intel_guc_capture_getlistsize(guc, owner, type, classid, &size);
>> +	if (ret) {
>> +		cache->is_valid = true;
>> +		cache->file = NULL;
>> +		cache->size = 0;
>> +		cache->status = ret;
>> +		return ret;
>> +	}
>> +
>> +	caplist = kzalloc(size, GFP_KERNEL);
>> +	if (!caplist)
>> +		return -ENOMEM;
>> +
>> +	/* populate capture list header */
>> +	tmp = caplist;
>> +	num_regs = guc_cap_list_num_regs(guc->capture.priv, owner, type, classid);
>> +	listnode = (struct guc_debug_capture_list *)tmp;
>> +	listnode->header.info = FIELD_PREP(GUC_CAPTURELISTHDR_NUMDESCR, (u32)num_regs);
>> +
>> +	/* populate list of register descriptor */
>> +	tmp += sizeof(struct guc_debug_capture_list);
>> +	guc_capture_list_init(guc, owner, type, classid, (struct guc_mmio_reg *)tmp, num_regs);
>> +
>> +	/* ADS only takes file handles, so we keep that as our cache */
>> +	file = shmem_create_from_data("guc-err-cap", caplist, size);
> I don't think you need a file here, why can't this just be a peice of
> kalloc'd memory that is populated and copied via IOSYS map calls?
Agreed on above.
> The memory will need to be persistent (allocated at driver load, free'd
> during unload) as can't allocate memory in the reset path tho.
Of course (as per prior rev comments)
> Beyond that, you are parsing a static table to populate memory. That
> almost certainly means you could just have static memory which we
> directly copy into the ADS.

On this, we dont actually copy the static tables from the top of this 
src file as-is directly into ADS because the format of the static tables 
is different from what ADS expects - i need both formats since the 
latter is lean (just register info) but the former also contains the 
name of registers (strings) to be used at printout time.

> Matt
>