Re: [Intel-xe] [RFC 0/2] Dump + OOB workarounds

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Apr 12, 2023 at 01:20:52AM -0700, Lucas De Marchi wrote:
> This is 2 RFCs in one, since they are more or less related.
> 
> PATCH #1) Dump the "driver workaround database" to check what are the
> workarounds implemented. This is useful when checking against the spec
> what workarounds are implemented in the driver.  This can later be
> extended to mark what are the "active" workarounds on the current
> platform.  I think this second scenario is more useful for the debugfs
> usage.

It is actually useful for the w/a assessment to know if the wa is
already known and implemented by the driver.

But for this use case in particular, this information without the
platform that it is 'active' is kind of useless...
I would continue to use git-grep for that while we don't show the
platform info.

But thinking about other cases I also believe this is not that useful
without the active platform information.

> 
> Other alternatives I have considered:
> a) Reuse the kunit infra; with this it's possible to easily fake a
> platform and get what are the WAs considered "active" for a platform.
> This is nice because it doesn't need to run on real hardware. However
> this may be abusing what the kunit is for

First I also thought this was an abuse... but if you think on validation
and we have some way to cross check some expected table against the
implemented/active one then you might build a case.

> 
> b) Just write a tool in C or python that parses the section from
> the .ko and output the info need. Drawback is keeping in sync the
> declarations from xe_rtp with this additional tool. And probably also
> that it'd need to process the rules by itself.

why from the .ko and not a grep in the code then?

> 
> PATCH #2) "out-of-band" workarounds, i.e. those that are sprinkled
> around the driver. In the implementation here I tried to keep the caller
> similar to what it was before. With the addition of XE_WA() the caller
> "registers" the workaround so later we can access it. As example I
> converted 1 place in xe_guc.c to use that.

I liked that XE_WA declaration.

We could even have some tool that also checks if all the XE_WA declarations
are indeed the lineage number and not some random hsd number.

> 
> Note: The "condition" is checked each time it goes through that code
> path.  I was debating the ability to just re-use XE_RTP_RULE() instead
> since it appears all the conditions can be translated to rules.
> Consider this the alternative (a).
> 
> After writing the commit message for that patch I thought: but if we can
> transform those conditions in rules, we could very well keep them in an
> additional table in xe_wa.c, which is the alternative below:
> 
> b) keep an additional table in xe_wa.c whose action is to set a
> bit/byte(?)  in  xe->active_oob_workarounds[]. During probe
> the rules are processed and the ones active are marked in that array.
> The extra section created by XE_WA() is then used to just map
> the WA to the index value. Then the callers would only need something
> like `if (XE_WA(xe, "XXXXXX") { ... } ` since the condition would be
> executed once on init.  However I'm thinking that this may not scale to
> all workarounds we may have.

I like this idea and I believe that it does scale... It also helps
to push back on big ugly workarounds that doesn't fit in this.

> 
> Thoughts?
> 
> thanks
> Lucas De Marchi
> 
> Lucas De Marchi (2):
>   drm/xe: Add debugfs to dump all known workarounds
>   drm/xe: Register OOB workarounds
> 
>  drivers/gpu/drm/xe/Makefile       |  2 ++
>  drivers/gpu/drm/xe/xe.lds         |  8 +++++++
>  drivers/gpu/drm/xe/xe_debugfs.c   | 12 ++++++++++
>  drivers/gpu/drm/xe/xe_guc.c       |  6 ++---
>  drivers/gpu/drm/xe/xe_rtp_types.h |  8 +++++++
>  drivers/gpu/drm/xe/xe_wa.c        | 38 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/xe/xe_wa.h        | 14 ++++++++++++
>  7 files changed, 83 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/xe/xe.lds
> 
> -- 
> 2.39.0
>