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

[Lucas De Marchi]

On Thu, Feb 03, 2022 at 11:03:24AM -0800, Matthew Brost wrote:
>On Wed, Jan 26, 2022 at 02:46:19PM -0800, Lucas De Marchi wrote:
>> On Wed, Jan 26, 2022 at 02:48:13AM -0800, Alan Previn wrote:
>> > Update GuC ADS size allocation to include space for
>> > the lists of error state capture register descriptors.
>> >
>> > Also, populate the lists of registers we want GuC to report back to
>> > Host on engine reset events. This list should include global,
>> > engine-class and engine-instance registers for every engine-class
>> > type on the current hardware.
>> >
>> > NOTE: Start with a sample table of register lists to layout the
>> > framework before adding real registers in subsequent patch.
>> >
>> > Signed-off-by: Alan Previn <alan.previn.teres.alexis at intel.com>
>> > ---
>>
>> ...
>>
>> > static void __guc_ads_init(struct intel_guc *guc)
>> > {
>> > 	struct intel_gt *gt = guc_to_gt(guc);
>> > @@ -573,9 +553,9 @@ static void __guc_ads_init(struct intel_guc *guc)
>> >
>> > 	base = intel_guc_ggtt_offset(guc, guc->ads_vma);
>> >
>> > -	/* Capture list for hang debug */
>> > -	guc_capture_list_init(guc, blob);
>> > -
>> > +	/* Lists for error capture debug */
>> > +	intel_guc_capture_prep_lists(guc, (struct guc_ads *)blob, base,
>>
>> no, please don't cast/export struct guc_ads like this. We can't really
>> dereference it since it may be in IO memory.
>>
>> See https://patchwork.freedesktop.org/series/99378/ with the huge
>> refactor in this file to make it conform to the rules of accessing IO
>> memory.
>>
>> Maybe this list could be appended in the same reglist buffer and we just
>> copy it once to its final location, like we are doing with the reglist?
>>
>
>Agree with Lucas here, let's create the capture list on probe and store
>it somewhere in the device data. On ADS population this more or less
>turns into a memcpy (or after Lucas's series a dma-buf-map call). And on
>fini, just free to stored data. The create capture list IMO is fine to
>be done in an external file and return the data to the ADS code but
>let's make sure everyone is on board with that. Maybe ping Lucas
>directly about this?

yeah, sounds good to me. My worry is exporting ADS struct layout to
other compilation units. Asking for the entire ads blob
(or what would be dma_buf_map in my patches, or iosys_map in the new
version I will send soon) could quickly spread too much knowledge about it to
the rest of the driver.

I think we should at most export the speficic offset inside the buffer
another compilation unit can fill out. In that case with the
iosys_buf API we would return a shallow copy of our guc->ads_map +
offset.

And the other alternative would be as you suggested: save the list in a
temporary buffer and when needed call intel_guc_ads() to populate that
into ads in the right place.

The integration with iosys-map I can figure out if my patch series lands
after this one, or I can help rebasing this otherwise. But IMO we
should not pass the plain blob pointer around regardless of iosys-map.


thanks
Lucas De Marchi

>
>Specific patch Lucas's is referencing:
>https://patchwork.freedesktop.org/patch/471051/?series=99378&rev=1
>
>Matt
>
>> Lucas De Marchi