Re: Mesa >= 23.3.x and python 2.6 ...

[Jordan Justen]

On 2024-01-19 12:24:24, Stefan Dirsch wrote:
> Hi Jordan
> 
> Thanks for digging into this!
> 
> On Fri, Jan 19, 2024 at 12:10:37PM -0800, Jordan Justen wrote:
> > On 2024-01-18 04:37:52, Stefan Dirsch wrote:
> > > Hi
> > > 
> > > I noticed that with version 23.3.x Mesa no longer can be built with python
> > > 2.6. It still worked with Mesa 23.2.1.
> > 
> > As mentioned in other emails, this was typo where 3.6 was intended.
> > 
> > > 
> > > It fails with
> > > 
> > > [   95s] Traceback (most recent call last):
> > > [   95s]   File "../src/intel/genxml/gen_bits_header.py", line 23, in 
> > > 
> > > [   95s] import intel_genxml
> > > [   95s]   File 
> > > "/home/abuild/rpmbuild/BUILD/mesa-23.3.3/src/intel/genxml/intel_
> > > genxml.py", line 5
> > > [   95s] from __future__ import annotations
> > > [   95s] ^
> > > [   95s] SyntaxError: future feature annotations is not defined
> > > 
> > 
> > I guess this code first appeared in Dylan's:
> > 
> > 4fd2e15855d ("intel/genxml: add type annotations to gen_sort_tags.py")
> > 
> > and then became part of the standard tests a few commits later in:
> > 
> > 1f0a0a46d97 ("meson: run genxml sort tests")
> > 
> > back in Oct 2022. So, I guess at that point 'ninja test' would have
> > failed with Python 3.6.
> > 
> > Then, I suppose I propagated this to being used on every build in:
> > 
> > 0495f952d48 ("intel/genxml: Add genxml_import.py script")
> > 
> > in Sept 2023.
> 
> Thanks. This explains why I've found this code already in older releases, but
> it didn't fail for me yet. You said tests. Is this just a test, I could
> disable (as dirty hack)? I was assuming it would generate code ...

In 0495f952d48, I moved in to a common file, and essentially, now it's
used by our script that runs during the build in addition to the test.

It was "fun" finding a way to get python 3.6 :), but after that, I
think I found a way to make Python 3.6 work. I guess you can try it
out:

https://gitlab.freedesktop.org/jljusten/mesa/-/commits/intel-genxml-python3.6

In my light testing, Python 3.6 through 3.13 seemed to work. Python
3.5 did *not* work.

-Jordan

Re: Mesa >= 23.3.x and python 2.6 ...

[Jordan Justen]

On 2024-01-18 04:37:52, Stefan Dirsch wrote:
> Hi
> 
> I noticed that with version 23.3.x Mesa no longer can be built with python
> 2.6. It still worked with Mesa 23.2.1.

As mentioned in other emails, this was typo where 3.6 was intended.

> 
> It fails with
> 
> [   95s] Traceback (most recent call last):
> [   95s]   File "../src/intel/genxml/gen_bits_header.py", line 23, in 
> [   95s] import intel_genxml
> [   95s]   File 
> "/home/abuild/rpmbuild/BUILD/mesa-23.3.3/src/intel/genxml/intel_
> genxml.py", line 5
> [   95s] from __future__ import annotations
> [   95s] ^
> [   95s] SyntaxError: future feature annotations is not defined
> 

I guess this code first appeared in Dylan's:

4fd2e15855d ("intel/genxml: add type annotations to gen_sort_tags.py")

and then became part of the standard tests a few commits later in:

1f0a0a46d97 ("meson: run genxml sort tests")

back in Oct 2022. So, I guess at that point 'ninja test' would have
failed with Python 3.6.

Then, I suppose I propagated this to being used on every build in:

0495f952d48 ("intel/genxml: Add genxml_import.py script")

in Sept 2023.

Maybe Dylan knows how we might make this compatible with Python 3.6,
assuming we want to. :)

https://devguide.python.org/versions/

-Jordan

Re: [PATCH v10 2/2] drm/i915: Allow user to set cache at BO creation

[Jordan Justen]

On 2023-05-22 04:52:56, Andi Shyti wrote:
> Hi,
> 
> On Thu, May 18, 2023 at 10:11:03PM -0700, fei.y...@intel.com wrote:
> > From: Fei Yang 
> > 
> > To comply with the design that buffer objects shall have immutable
> > cache setting through out their life cycle, {set, get}_caching ioctl's
> > are no longer supported from MTL onward. With that change caching
> > policy can only be set at object creation time. The current code
> > applies a default (platform dependent) cache setting for all objects.
> > However this is not optimal for performance tuning. The patch extends
> > the existing gem_create uAPI to let user set PAT index for the object
> > at creation time.
> > The new extension is platform independent, so UMD's can switch to using
> > this extension for older platforms as well, while {set, get}_caching are
> > still supported on these legacy paltforms for compatibility reason.
> > 
> > Test igt@gem_create@create_ext_set_pat posted at
> > https://patchwork.freedesktop.org/series/117695/
> > 
> > Tested with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> > 
> > Signed-off-by: Fei Yang 
> > Cc: Chris Wilson 
> > Cc: Matt Roper 
> > Cc: Andi Shyti 
> > Reviewed-by: Andi Shyti 
> > Acked-by: Jordan Justen 
> > Tested-by: Jordan Justen 
> 
> last call for comments and reviews on this patch. If nothing
> comes I am going to push it tomorrow morning (Europe).
> 
> There is also a merge request from Mesa pending on this so that I
> don't want to keep it hanging any longer.

No need to wait any longer with regard to feedback from Mesa.

I definitely was hoping for more consideration of the userspace
request, but it's been decisively rejected. My ack was not readily
given, but it stands.

-Jordan

Re: [PATCH v10 2/2] drm/i915: Allow user to set cache at BO creation

[Jordan Justen]

On 2023-05-18 22:11:03,  wrote:
> From: Fei Yang 
> 
> To comply with the design that buffer objects shall have immutable
> cache setting through out their life cycle, {set, get}_caching ioctl's
> are no longer supported from MTL onward. With that change caching
> policy can only be set at object creation time. The current code
> applies a default (platform dependent) cache setting for all objects.
> However this is not optimal for performance tuning. The patch extends
> the existing gem_create uAPI to let user set PAT index for the object
> at creation time.
> The new extension is platform independent, so UMD's can switch to using
> this extension for older platforms as well, while {set, get}_caching are
> still supported on these legacy paltforms for compatibility reason.
> 
> Test igt@gem_create@create_ext_set_pat posted at
> https://patchwork.freedesktop.org/series/117695/
> 
> Tested with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
> 
> Signed-off-by: Fei Yang 
> Cc: Chris Wilson 
> Cc: Matt Roper 
> Cc: Andi Shyti 
> Reviewed-by: Andi Shyti 
> Acked-by: Jordan Justen 

Nevertheless, I'm still disappointed my suggestion was so quickly shot
down.

I tried to look over our usage Mesa of i915 extensions, and found
this:

I915_GEM_CREATE_EXT_MEMORY_REGIONS:

 * If DRM_I915_QUERY_MEMORY_REGIONS is found

I915_GEM_CREATE_EXT_PROTECTED_CONTENT:

 * Probed via the current "robust" method. Resulted in 8s driver
   startup delay in some bad scenarios.

 * Will be guarded by I915_PARAM_PXP_STATUS when available in future

I915_CONTEXT_CREATE_EXT_SETPARAM (I915_CONTEXT_PARAM_ENGINES):

 * If DRM_I915_QUERY_ENGINE_INFO is found

I915_GEM_CREATE_EXT_SET_PAT:

 * When platform is mtl or newer

I think we will continue to try to find workarounds that imply the
extension's existence, but it could be nice to have a generic way to
find out what extensions the kernel knows about.

-Jordan

Re: [PATCH v7 4/4] drm/i915: Allow user to set cache at BO creation

[Jordan Justen]

On 2023-05-10 15:14:16, Andi Shyti wrote:
> Hi,
> 
> On Tue, May 09, 2023 at 09:59:42AM -0700, fei.y...@intel.com wrote:
> > From: Fei Yang 
> > 
> > To comply with the design that buffer objects shall have immutable
> > cache setting through out their life cycle, {set, get}_caching ioctl's
> > are no longer supported from MTL onward. With that change caching
> > policy can only be set at object creation time. The current code
> > applies a default (platform dependent) cache setting for all objects.
> > However this is not optimal for performance tuning. The patch extends
> > the existing gem_create uAPI to let user set PAT index for the object
> > at creation time.
> > The new extension is platform independent, so UMD's can switch to using
> > this extension for older platforms as well, while {set, get}_caching are
> > still supported on these legacy paltforms for compatibility reason.
> > 
> > Cc: Chris Wilson 
> > Cc: Matt Roper 
> > Cc: Andi Shyti 
> > Signed-off-by: Fei Yang 
> > Reviewed-by: Andi Shyti 
> 
> just for a matter of completeness, this is new uapi is tested
> through the "create-ext-set-pat" test case from the "gem_create"
> igt test[1]. Can any of the igt maintainers give it a look,
> comment and ack?
> 
> The mesa merge request is here [2]. As there is a merge request
> in progress, would anyone from mesa be so kind to give an ack to
> this patch, as well?
> 
> With the mesa ack in place this patch should be ready to go and
> I'm looking forward to having it in.

I tested my MR [2] in our CI. There was some bad news, but I don't
think it needs to block these patches.

The good news was that I found that OpenGL testing with our iris
driver appeared to have ok results when using this interface.

But, our Vulkan Anvil driver was not stable with the current patches
in the Mesa MR. We will need to debug this further before using the
interface on Vulkan.

I don't suspect that this is an issue with the kernel interface, so
you can add:

Tested-by: Jordan Justen 

-Jordan

> 
> Thanks,
> Andi
> 
> [1] https://patchwork.freedesktop.org/patch/534955/?series=117185=1
> [2] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/22878
>

Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP

[Jordan Justen]

On 2023-05-10 15:00:03, Teres Alexis, Alan Previn wrote:
> 
> alan:snip
> 
> > This is why I asked if it was it was "basically certain that in a
> > production environment, then it will eventually return 1 meaning it's
> > ready". Alan's response was a little ambiguous on this point.
> alan: if we get a '2' and never transition to '1' - thats a kernel bug or
> firmware / system issue.
> 
> > Arguably a transition from 2 to -ENODEV could be considered a kernel
> > bug, but it doesn't sound like Alan would agree. :) Maybe Alan would
> > agree to saying it's either a kernel, or system integration bug.
> alan: agreed - that would be a kernel bug or a system integration bug.
> 
> I also agreed on the init-flow vs app-usage thoughts Jordan had.
> That said MESA has many ways it can use this UAPI and we can discuss
> that on the MESA patch.
> 
> 
> hmmm.. so... ack anyone? [insert big hopeful smiley here]
> apologies if I am asking too often.

Assuming that:

  2 = PXP feature is supported but should be ready soon (pending
  initialization of non-i915 system dependencies).

really means, "it'll be ready soon or there is a bug somewhere",

Acked-by: Jordan Justen 

If Mesa finds that it can't really rely on that, we may have to decide
on a different approach in how to use that return value.

Is it possible to hold on for another ~12 hours to see if Lionel wants
to Ack as well?

-Jordan

Re: [Intel-gfx] [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP

[Jordan Justen]

...fixing some Cc email addresses I somehow mangled.

On 2023-05-10 12:40:07, Souza, Jose wrote:
> On Mon, 2023-05-08 at 17:49 +, Teres Alexis, Alan Previn wrote:
> > On Fri, 2023-05-05 at 00:39 -0700, Justen, Jordan L wrote:
> > > On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote:
> > > > On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote:
> > > > > Because of the additional firmware, component-driver and
> > > > > initialization depedencies required on MTL platform before a
> > > > > PXP context can be created, UMD calling for PXP creation as a
> > > > > way to get-caps can take a long time. An actual real world
> > > > > customer stack has seen this happen in the 4-to-8 second range
> > > > > after the kernel starts (which sees MESA's init appear in the
> > > > > middle of this range as the compositor comes up). To avoid
> > > > > unncessary delays experienced by the UMD for get-caps purposes,
> > > > > add a GET_PARAM for I915_PARAM_PXP_SUPPORT.
> > > > > 
> > > > alan:snip.
> > > > Progress update on the UMD side - I'm working on patch for PR here: 
> > > > https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57
> > > > but taking time to verify certain code paths
> > > 
> > > Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready
> > > soon"), then it is basically certain that in a production environment,
> > > then it will eventually return 1 meaning it's ready, right?
> > alan: yes - but not 100%. non-platform-state-machine could still
> > cause unexpected failures for example, [1] if hw was fused in a way
> > that doesnt permit PXP or [2] enabling certain BIOS debug knobs
> > doesnt allow PXP. However at the moment, there is no way for us
> > to know for sure without actually creating a protected context.
> > Of course having hw-fusing + bios-config that supports PXP have
> > always 100% succeeded for me.
> 
> In my opinion it is problematic mark that we support protected
> context but then it fails to create protected context.

This is why I asked if it was it was "basically certain that in a
production environment, then it will eventually return 1 meaning it's
ready". Alan's response was a little ambiguous on this point.

But, considering the number of applications that will need this
feature is low, combined with an extremely low chance that the kernel
will end up going from 2 (will be ready soon) to -ENODEV (will never
work), well, it seems worth considering advertising it with no delay
even if it later fails if used.

Arguably a transition from 2 to -ENODEV could be considered a kernel
bug, but it doesn't sound like Alan would agree. :) Maybe Alan would
agree to saying it's either a kernel, or system integration bug.

I think it'd also be ok if we didn't advertise support if an
application starts when the kernel is still in the 2 (will be ready
soon) state.

But, some environments might prefer to wait, so I think the kernel
uapi should stay as Alan has it now so we have the flexibility to
potentially accommodate this. (Perhaps with driconf, or a build flag,
or an env-var.)

-Jordan

Re: [PATCH v9 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP

[Jordan Justen]

On 2023-05-04 22:30:07, Teres Alexis, Alan Previn wrote:
> On Thu, 2023-04-27 at 16:48 -0700, Teres Alexis, Alan Previn wrote:
> > Because of the additional firmware, component-driver and
> > initialization depedencies required on MTL platform before a
> > PXP context can be created, UMD calling for PXP creation as a
> > way to get-caps can take a long time. An actual real world
> > customer stack has seen this happen in the 4-to-8 second range
> > after the kernel starts (which sees MESA's init appear in the
> > middle of this range as the compositor comes up). To avoid
> > unncessary delays experienced by the UMD for get-caps purposes,
> > add a GET_PARAM for I915_PARAM_PXP_SUPPORT.
> > 
> alan:snip.
> Progress update on the UMD side - I'm working on patch for PR here: 
> https://gitlab.freedesktop.org/alan_previn_intel/mesa-alan-previn-features/-/commit/fb9d4fbfbef7dfd3f41df335cd31549fd39ddb57
> but taking time to verify certain code paths

Just to confirm, if I915_PARAM_PXP_STATUS returns 2 ("will be ready
soon"), then it is basically certain that in a production environment,
then it will eventually return 1 meaning it's ready, right?

If this is correct, then I think that the change in
i915_gem_supports_protected_context() is good, and probably we can
skip the change in iris_create_hw_context().

Basically, we're timing out for multiple seconds either way. And, I'm
hoping that the kernel will eventually get the PXP init done and
create the context.

I think there's 2 cases of interest here.

First, and most likely, the application running doesn't care about
protected content. In this case we can quickly advertise the support,
but there will be no consequence because the application won't use the
feature.

Second, the application does care about protected content. In this
case we can quickly advertise the support, but if the feature is used
too quickly, then the context create might take a long time.

If I915_PARAM_PXP_STATUS returning 2 has a reasonable chance in a
production environment of eventually finding out that pxp can't work,
then perhaps more disussion is needed. I guess the worst case scenario
is no worse than today though. (Except it is still somewhat better,
because the common case would not involve protected content being
used by the application.)

Another option besides from the timeout loop in
iris_create_hw_context() might be to check I915_PARAM_PXP_STATUS after
the context create fails to tweak the debug message.

-Jordan

Re: [PATCH v8 6/8] drm/i915/uapi/pxp: Add a GET_PARAM for PXP

[Jordan Justen]

On 2023-04-26 11:17:16, Teres Alexis, Alan Previn wrote:
> alan: Hi Jordan, Tvrtko, Daniel Vetter and Lionel,... 
> how to proceed on this series (which have required Rb/Ack's) in light of
> the recent discussion on the other series here:
> https://patchwork.freedesktop.org/patch/532994/?series=115980=8
> it sounds like:
> - Jordan still wants the extension query

Yes, I do, but so far it doesn't appear that any kernel devs think
it's a reasonable request.

As I read through your emails about this pxp situation, it seems like
a separate issue. When I encountered the 8s delay, it was on MTL, and
apparently some firmware issue meant it was never going to work. So, I
thought this was a case of it either being supported, or never being
supported.

Now I'm seeing from your emails that in some cases it might be
supported, but just not ready yet. In that case a status which is
directly tied to pxp seems valuable. (But, yuck, how did we get into
this situation? :)

Can you tell that pxp is in progress, but not ready yet, as a separate
state from 'it will never work on this platform'? If so, maybe the
status could return something like:

0: It's never going to work
1: It's ready to use
2: It's starting and should work soon

I could see an argument for treating that as a case where we could
still advertise protected content support, but if we try to use it we
might be in for a nasty delay.

Maybe Lionel would have a different opinion on whether it would be a
good idea to go this route.

Regarding the extensions list I was requesting, it might be easiest
for the kernel if it just replies with all the extensions it knows
about regardless of whether they are usable right now.

-Jordan

Re: IOCTL feature detection (Was: Re: [Intel-gfx] [PATCH 8/8] drm/i915: Allow user to set cache at BO creation)

[Jordan Justen]

On 2023-04-26 04:52:43, Daniel Vetter wrote:
> 
> Joonas asked me to put my thoughts here:
> 
> - in general the "feature discovery by trying it out" approach is most
>   robust and hence preferred, but it's also not something that's required
>   when there's good reasons against it

More robust in what sense?

Userspace has to set up some scenario to use the interface, which
hopefully is not too complex. It's difficult to predict what it may
look like in the future since we are talking about undefined
extensions.

Userspace has to rely on the kernel making creating and destroying
this thing essentially free. For
I915_GEM_CREATE_EXT_PROTECTED_CONTENT, that did not work out. For
I915_GEM_CREATE_EXT_SET_PAT, just wondering, since the PAT indices are
platform specific, what might happen if we tried to choose some common
index value to detect the extension in a generic manner? Could it
potentially lead to unexpected behavior, or maybe just an error. I
guess it's really extension specific what kind of issues potentially
could arise.

> tldr; prefer discovery, don't sweat it if not, definitely don't
> overengineer this with some magic "give me all extensions" thing because
> that results in guaranteed cross-component backporting pains sooner or
> later. Or inconsistency, which defeats the point.

I guess I don't know the full context of your concerns here.

For returning the gem-create extensions, isn't this just a matter of
returning the valid indices to the create_extensions array in
i915_gem_create.c? Is that an over-engineered query?

It seems weird that there's a reasonably well defined "extension"
mechanism here, but no way for the kernel to just tell us what
extensions it knows about.

-Jordan

Re: IOCTL feature detection (Was: Re: [Intel-gfx] [PATCH 8/8] drm/i915: Allow user to set cache at BO creation)

[Jordan Justen]

On 2023-04-25 06:41:54, Joonas Lahtinen wrote:
> (+ Faith and Daniel as they have been involved in previous discussions)
> 
> Quoting Jordan Justen (2023-04-24 20:13:00)
> > On 2023-04-24 02:08:43, Tvrtko Ursulin wrote:
> > > 
> > > Being able to "list" supported extensions sounds like a reasonable
> > > principle, albeit a departure from the design direction to date.
> > > Which means there are probably no quick solutions. Also, AFAIU, only
> > > PXP context create is the problematic one, right? Everything else is
> > > pretty much instant or delayed allocation so super cheap to probe by
> > > attempting to use.
> > > 
> > > If I got that right and given this series is about
> > > drm_i915_gem_create_ext I don't think this side discussion should be
> > > blocking it.
> > 
> > This still leaves the issue of no reasonable detection mechanism for
> > the extension.
> 
> I remember this exact discussion being repeated at least a few times in
> the past 5 years. Previously the conclusion was always that detection by
> trying to use the extension leads to least amount of uAPI surface. There
> is also no concern of having mismatch in backporting of the query and the
> functionality patches.
> 
> Why exactly do you think it is more reasonable to do the following?
> 
> check_if_extension_query_is_supported();
> 
> check_if_extension_xyz_is_supported();
> 

As I've mentioned a couple times, I'm asking for query item that
returns it all the extensions that conceivably could work.

In theory it could be made a single query item which somehow then
enumerates if something is a context extension or bo extension. But,
it seems simpler for all if we just have a couple query items
returning a simple u64 array for the few classes of extensions.

> VS
> 
> create_[context,buffer,whatever]_with_extension();
> 
> destroy_[context,buffer,whatever]();
> 
> For contexts and buffers there's no overhead,

There's no-overhead to creating and destroying things? I think the 8s
timeout when trying create a protected content context shows it's not
always quite that simple.

Over time userspace will have to continue growing this set of
create/destroy tests as new extensions are added. Simply so we can
discover what extensions are supported.

This doesn't seem like a very robust way to advertise extensions for
an api.

Another point ... say you need to debug why some simple app is failing
to start the driver. One tool might be strace. But now you have a
bunch of noise of calls from the driver creating and destroying things
just to discover what extensions the kernel supports. It would be nice
if all this was handled with a query item vs a bunch of
create/destroys.

> and we're keeping the uAPI surface smaller (less bugs, less
> validation effort).

I understand wanting to keep the kernel uapi and implementation
simple.

Is it too complicated to return a u64 array for a query item
indicating the extensions supported for the various classes of
extensions? I think in most cases it'll just be essentially shuffling
across an array of u64 items. (Since most known extensions will always
be supported by the kernel.)

> Additionally we support checking combinations of extensions A, B and
> C without extra effort.

Regarding combinations of extensions, is that really something
userspace needs to probe? I would think if userspace knows about the
possible extensions, then it's on userspace to use combinations
appropriately.

But, in detecting extensions, it is possible that, say extension B
relies on extension A. Now userspace may have to probe to see if
extension A is supported, and then probe for extension B while using
extension A. Essentially, probing for an extension could become a bit
complicated. Vs the kernel just giving us a u64 array of known
extensions...

-Jordan

RE: [Intel-gfx] [PATCH 0/8] drm/i915/mtl: Define MOCS and PAT tables for MTL

[Jordan Justen]

On 2023-04-16 23:43:20, Yang, Fei wrote:
> > fei.y...@intel.com kirjoitti 17.4.2023 klo 9.24:
> >> From: Fei Yang 
> >> 
> >> The series includes patches needed to enable MTL.
> >> Also add new extension for GEM_CREATE uAPI to let user space set cache 
> >> policy for buffer objects.
> >
> > if I'm counting right, this would be version 5 of the series, yet
> > that is not shown anywhere nor the changes between series..
> 
> Yes, mostly addressing minor issues, just want to keep the commit
> message clean as it's the enablement patch set for new platform.

One thing that could help is to use the --subject-prefix parameter to
git format-patch so "PATCH v5" would be in the subject of the emails.

Additionally, it's helpful to include full a version history in the
cover-letter (0/8) patch so it's clear what has changed in each
version of the patches sent out.

-Jordan

Re: [Intel-gfx] [PATCH 8/8] drm/i915: Allow user to set cache at BO creation

[Jordan Justen]

On 2023-04-24 02:08:43, Tvrtko Ursulin wrote:
> 
> Being able to "list" supported extensions sounds like a reasonable
> principle, albeit a departure from the design direction to date.
> Which means there are probably no quick solutions. Also, AFAIU, only
> PXP context create is the problematic one, right? Everything else is
> pretty much instant or delayed allocation so super cheap to probe by
> attempting to use.
> 
> If I got that right and given this series is about
> drm_i915_gem_create_ext I don't think this side discussion should be
> blocking it.

This still leaves the issue of no reasonable detection mechanism for
the extension. If the discussion gets too complicated, then can we add
a GET_PARAM for the SET_PAT extension? I'm hoping we could either come
up with something better reasonably quickly, or i915/Xe can add a new
param for each new extensions until a better approach is available.

> Furthermore the PXP context create story is even more complicated,
> in a way that it is not just about querying whether the extension is
> supported, but the expensive check is something more complicated.
> 
> Going back to implementation details for this proposed new feature,
> one alternative to query could be something like:
> 
>drm_i915_gem_create_ext.flags |= I915_GEM_CREATE_EXT_FLAG_PROBE_EXTENSIONS;
> 
> That would be somewhat more light weight to implement that the
> i915_query route. And it appears it would work for all ioctls which
> support extensions apart for i915_context_param_engines.

This seems little better than the "try it, and if it works then it's
supported".

I'm not suggesting that userspace should be able to check that
scenario x+y+z will work, but more a list of extensions that
conceivably could work. Normally this should just a matter of the
kernel unconditionally adding the newly implemented extension to the
list returned in the query call.

If a GET_PARAM can be made for the PXP case, then it seems like a
query item returning CONTEXT_CREATE extensions could conditionally
omit that extension just as easily as implementing the proposed new
GET_PARAM.

-Jordan

Re: [Intel-gfx] [PATCH 8/8] drm/i915: Allow user to set cache at BO creation

[Jordan Justen]

On 2023-04-20 09:11:18, Yang, Fei wrote:
> > On 20/04/2023 12:39, Andi Shyti wrote:
> >> Hi Fei,
> >>
> >>> To comply with the design that buffer objects shall have immutable
> >>> cache setting through out their life cycle, {set, get}_caching ioctl's
> >>> are no longer supported from MTL onward. With that change caching
> >>> policy can only be set at object creation time. The current code
> >>> applies a default (platform dependent) cache setting for all objects.
> >>> However this is not optimal for performance tuning. The patch extends
> >>> the existing gem_create uAPI to let user set PAT index for the object
> >>> at creation time.
> >>> The new extension is platform independent, so UMD's can switch to using
> >>> this extension for older platforms as well, while {set, get}_caching are
> >>> still supported on these legacy paltforms for compatibility reason.
> >>>
> >>> Cc: Chris Wilson 
> >>> Cc: Matt Roper 
> >>> Cc: Andi Shyti 
> >>> Signed-off-by: Fei Yang 
> >>> Reviewed-by: Andi Shyti 
> >>
> >> because this is an API change, we need some more information
> >> here.
> >>
> >> First of all you need to CC the userspace guys that have been
> >> working on top of your series and get their ack's.
> >
> > Yes, and a link to a Mesa merge request which uses the uapi should be
> > included.
> 
> Working with Mesa team on this, stay tuned.
> 

I would like to see the extension detection issue is handled before
ack'ing this.

How about a new DRM_I915_QUERY_GEM_CREATE_EXTENSIONS item, that
returns a u64 array of usable extension names for
DRM_IOCTL_I915_GEM_CREATE_EXT?

A similar DRM_I915_QUERY_GEM_CONTEXT_CREATE_EXTENSIONS could also
provide an alternative to Alan's "drm/i915/uapi/pxp: Add a GET_PARAM
for PXP", and more easily allow advertising future new extensions for
context/buffer creation.

-Jordan

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Allow user to set cache at BO creation

[Jordan Justen]

On 2023-04-05 13:26:43, Jordan Justen wrote:
> On 2023-04-05 00:45:24, Lionel Landwerlin wrote:
> > On 04/04/2023 19:04, Yang, Fei wrote:
> > >> Subject: Re: [Intel-gfx] [PATCH 7/7] drm/i915: Allow user to set cache 
> > >> at BO creation
> > >>
> > >> Just like the protected content uAPI, there is no way for userspace to 
> > >> tell
> > >> this feature is available other than trying using it.
> > >>
> > >> Given the issues with protected content, is it not thing we could want 
> > >> to add?
> > > Sorry I'm not aware of the issues with protected content, could you 
> > > elaborate?
> > > There was a long discussion on teams uAPI channel, could you comment 
> > > there if
> > > any concerns?
> > >
> > 
> > We wanted to have a getparam to detect protected support and were told 
> > to detect it by trying to create a context with it.
> > 
> 
> An extensions system where the detection mechanism is "just try it",
> and assume it's not supported if it fails. ??
> 

I guess no one wants to discuss the issues with this so-called
detection mechanism for i915 extensions. (Just try it and if it fails,
it must not be supported.)

I wonder how many ioctls we will be making a couple years down the
road just to see what the kernel supports.

Maybe we'll get more fun 8 second timeouts to deal with. Maybe these
probing ioctls failing or succeeding will alter the kmd's state in
some unexpected way.

It'll also be fun to debug cases where the driver is not starting up
with the noise of a bunch of probing ioctls flying by.

I thought about suggesting at least something like
I915_PARAM_CMD_PARSER_VERSION, but I don't know if that could have
prevented this 8 second timeout for creating a protected content
context. Maybe it's better than nothing though.

Of course, there was also the vague idea I threw out below for getting
a list of supported extentions.

-Jordan

> 
> This seem likely to get more and more problematic as a detection
> mechanism as more extensions are added.
> 
> > 
> > Now it appears trying to create a protected context can block for 
> > several seconds.
> > 
> > Since we have to report capabilities to the user even before it creates 
> > protected contexts, any app is at risk of blocking.
> > 
> 
> This failure path is not causing any re-thinking about using this as
> the extension detection mechanism?
> 
> Doesn't the ioctl# + input-struct-size + u64-extension# identify the
> extension such that the kernel could indicate if it is supported or
> not. (Or, perhaps return an array of the supported extensions so the
> umd doesn't have to potentially make many ioctls for each extension of
> interest.)
> 
> -Jordan

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Allow user to set cache at BO creation

[Jordan Justen]

On 2023-04-05 00:45:24, Lionel Landwerlin wrote:
> On 04/04/2023 19:04, Yang, Fei wrote:
> >> Subject: Re: [Intel-gfx] [PATCH 7/7] drm/i915: Allow user to set cache at 
> >> BO creation
> >>
> >> Just like the protected content uAPI, there is no way for userspace to tell
> >> this feature is available other than trying using it.
> >>
> >> Given the issues with protected content, is it not thing we could want to 
> >> add?
> > Sorry I'm not aware of the issues with protected content, could you 
> > elaborate?
> > There was a long discussion on teams uAPI channel, could you comment there 
> > if
> > any concerns?
> >
> 
> We wanted to have a getparam to detect protected support and were told 
> to detect it by trying to create a context with it.
> 

An extensions system where the detection mechanism is "just try it",
and assume it's not supported if it fails. ??

This seem likely to get more and more problematic as a detection
mechanism as more extensions are added.

> 
> Now it appears trying to create a protected context can block for 
> several seconds.
> 
> Since we have to report capabilities to the user even before it creates 
> protected contexts, any app is at risk of blocking.
> 

This failure path is not causing any re-thinking about using this as
the extension detection mechanism?

Doesn't the ioctl# + input-struct-size + u64-extension# identify the
extension such that the kernel could indicate if it is supported or
not. (Or, perhaps return an array of the supported extensions so the
umd doesn't have to potentially make many ioctls for each extension of
interest.)

-Jordan

Re: [PATCH v3 2/2] drm/i915/uapi: expose GTT alignment

[Jordan Justen]

On 2022-10-14 03:58:12, Matthew Auld wrote:
> On 14/10/2022 08:20, Jordan Justen wrote:
> > Acked-by: Jordan Justen 
> 
> Thanks. Can I take that as ack for merging the series from Mesa POV? I 
> think Lionel was going to test this, but I think keeps getting swamped 
> with other stuff. We kind of urgently need to land this series.

Speaking from the uapi perspective, I would would say, yes, it looks
seems good to merge. I looked over your igt patches, and they seem to
test the uapi well. The uapi change is pretty simple, and the related
documentation changes look good.

No, we haven't gotten the chance to test the uapi implementation with
Mesa, but I expect we will soon, once it's in drm-tip.

-Jordan

Re: [PATCH v3 2/2] drm/i915/uapi: expose GTT alignment

[Jordan Justen]

Acked-by: Jordan Justen 

On 2022-10-04 04:49:15, Matthew Auld wrote:
> On some platforms we potentially have different alignment restrictions
> depending on the memory type. We also now have different alignment
> restrictions for the same region across different kernel versions.
> Extend the region query to return the minimum required GTT alignment.
> 
> Testcase: igt@gem_create@create-ext-placement-alignment
> Testcase: igt@i915_query@query-regions-sanity-check
> Suggested-by: Lionel Landwerlin 
> Signed-off-by: Matthew Auld 
> Cc: Michal Mrozek 
> Cc: Thomas Hellström 
> Cc: Stuart Summers 
> Cc: Jordan Justen 
> Cc: Yang A Shi 
> Cc: Nirmoy Das 
> Cc: Niranjana Vishwanathapura 
> ---
>  drivers/gpu/drm/i915/i915_query.c |  1 +
>  include/uapi/drm/i915_drm.h   | 29 +++--
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_query.c 
> b/drivers/gpu/drm/i915/i915_query.c
> index 6ec9c9fb7b0d..111377f210ed 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -498,6 +498,7 @@ static int query_memregion_info(struct drm_i915_private 
> *i915,
> info.region.memory_class = mr->type;
> info.region.memory_instance = mr->instance;
> info.probed_size = mr->total;
> +   info.gtt_alignment = mr->min_page_size;
>  
> if (mr->type == INTEL_MEMORY_LOCAL)
> info.probed_cpu_visible_size = mr->io_size;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 08d69e36fb66..2e613109356b 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -3346,8 +3346,33 @@ struct drm_i915_memory_region_info {
> /** @region: The class:instance pair encoding */
> struct drm_i915_gem_memory_class_instance region;
>  
> -   /** @rsvd0: MBZ */
> -   __u32 rsvd0;
> +   union {
> +   /** @rsvd0: MBZ */
> +   __u32 rsvd0;
> +   /**
> +* @gtt_alignment:
> +*
> +* The minimum required GTT alignment for this type of memory.
> +* When allocating a GTT address it must be aligned to this
> +* value or larger. On some platforms the kernel might opt to
> +* using 64K pages for I915_MEMORY_CLASS_DEVICE, where 64K GTT
> +* pages can then be used if we also use 64K GTT alignment.
> +*
> +* NOTE: If this is zero then this must be an older
> +* kernel which lacks support for this field.
> +*
> +* Side note: For larger objects (especially for
> +* I915_MEMORY_CLASS_DEVICE), like 2M+ in size, userspace 
> should
> +* consider potentially bumping the GTT alignment to say 2M,
> +* which could potentially increase the likelihood of the 
> kernel
> +* being able to utilise 2M GTT pages underneath, if the 
> layout
> +* of the physical pages allows it.  On some configurations we
> +* can then also use a more efficient page-table layout, if we
> +* can't use the more desirable 2M GTT page, so long as we 
> know
> +* that the entire page-table will be used by this object.
> +*/
> +   __u32 gtt_alignment;
> +   };
>  
> /**
>  * @probed_size: Memory probed by the driver
> -- 
> 2.37.3
>

Re: [Intel-gfx] [PATCH 1/1] drm/i915/guc: Update to GuC version 70.1.1

[Jordan Justen]

On 2022-07-14 16:08:51, Dave Airlie wrote:
> On Fri, 15 Apr 2022 at 10:15, Matt Roper  wrote:
> >
> > On Tue, Apr 12, 2022 at 03:59:55PM -0700, john.c.harri...@intel.com wrote:
> > > From: John Harrison 
> > >
> > > The latest GuC firmware drops the context descriptor pool in favour of
> > > passing all creation data in the create H2G. It also greatly simplifies
> > > the work queue and removes the process descriptor used for multi-LRC
> > > submission. So, remove all mention of LRC and process descriptors and
> > > update the registration code accordingly.
> > >
> > > Unfortunately, the new API also removes the ability to set default
> > > values for the scheduling policies at context registration time.
> > > Instead, a follow up H2G must be sent. The individual scheduling
> > > policy update H2G commands are also dropped in favour of a single KLV
> > > based H2G. So, change the update wrappers accordingly and call this
> > > during context registration..
> > >
> > > Of course, this second H2G per registration might fail due to being
> > > backed up. The registration code has a complicated state machine to
> > > cope with the actual registration call failing. However, if that works
> > > then there is no support for unwinding if a further call should fail.
> > > Unwinding would require sending a H2G to de-register - but that can't
> > > be done because the CTB is already backed up.
> > >
> > > So instead, add a new flag to say whether the context has a pending
> > > policy update. This is set if the policy H2G fails at registration
> > > time. The submission code checks for this flag and retries the policy
> > > update if set. If that call fails, the submission path early exists
> > > with a retry error. This is something that is already supported for
> > > other reasons.
> > >
> > > Signed-off-by: John Harrison 
> > > Reviewed-by: Daniele Ceraolo Spurio 
> >
> > Applied to drm-intel-gt-next.  Thanks for the patch and review.
> >
> 
> (cc'ing Linus and danvet, as a headsup, there is also a phoronix
> article where this was discovered).
> 
> Okay WTF.
> 
> This is in no way acceptable. This needs to be fixed in 5.19-rc ASAP.
> 
> Once hardware is released and we remove the gate flag by default, you
> cannot just bump firmware versions blindly.
> 
> The kernel needs to retain compatibility with all released firmwares
> since a device was declared supported.
> 
> This needs to be reverted, and then 70 should be introduced with a
> fallback to 69 versions.
> 
> Very disappointing, I expect this to get dealt with v.quickly.

This reminds me of something. A distant memory, really. But, if you
can believe it, i915 used to actually be able to *do something*
without the *closed source* guc. Crazy, right?

Anyway, that's all ancient history now. I mean, you have to go back
pretty far for that. Let me check my notes. Yeah, you'd probably have
to go all the way back to 2021 for that. I guess a lot of things were
much simpler back then though.

Anyway... Always fun to reminisce.

-Jordan

Re: [Intel-gfx] [PATCH] drm/fourcc: Document the Intel CCS modifiers' CC plane expected pitch

[Jordan Justen]

On 2022-06-24 14:38:50, Chery, Nanley G wrote:
> +Jordan (FYI)
> 
> I think the commit message has an extra "color" next to "CC". 
> With or without that dropped,
> 
> Reviewed-by: Nanley Chery 

Reviewed-by: Jordan Justen 

Re: [PATCH v2 01/12] drm/doc: add rfc section for small BAR uapi

[Jordan Justen]

On 2022-06-21 11:31:39, Lionel Landwerlin wrote:
> On 21/06/2022 13:44, Matthew Auld wrote:
> > Add an entry for the new uapi needed for small BAR on DG2+.
> >
> > v2:
> >- Some spelling fixes and other small tweaks. (Akeem & Thomas)
> >- Rework error capture interactions, including no longer needing
> >  NEEDS_CPU_ACCESS for objects marked for capture. (Thomas)
> >- Add probed_cpu_visible_size. (Lionel)
> > v3:
> >- Drop the vma query for now.
> >- Add unallocated_cpu_visible_size as part of the region query.
> >- Improve the docs some more, including documenting the expected
> >  behaviour on older kernels, since this came up in some offline
> >  discussion.
> > v4:
> >- Various improvements all over. (Tvrtko)
> >
> > v5:
> >- Include newer integrated platforms when applying the non-recoverable
> >  context and error capture restriction. (Thomas)
> >
> > Signed-off-by: Matthew Auld 
> > Cc: Thomas Hellström 
> > Cc: Lionel Landwerlin 
> > Cc: Tvrtko Ursulin 
> > Cc: Jon Bloomfield 
> > Cc: Daniel Vetter 
> > Cc: Jordan Justen 
> > Cc: Kenneth Graunke 
> > Cc: Akeem G Abodunrin 
> > Cc: mesa-...@lists.freedesktop.org
> > Acked-by: Tvrtko Ursulin 
> > Acked-by: Akeem G Abodunrin 
> 
> 
> With Jordan with have changes for Anv/Iris : 
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/16739
> 
> Acked-by: Lionel Landwerlin 
> 

Acked-by: Jordan Justen 

Re: [PATCH v3] uapi/drm/i915: Document memory residency and Flat-CCS capability of obj

[Jordan Justen]

On 2022-05-16 00:47:43, Lionel Landwerlin wrote:
> On 14/05/2022 00:06, Jordan Justen wrote:
>> 
>> Acked-by: Jordan Justen 
>> 
>> I think Nanley has accounted for this on iris with:
>> 
>> 
>> https://gitlab.freedesktop.org/mesa/mesa/-/commit/42a865730ef72574e179b56a314f30fdccc6cba8
>> 
> 
> Thanks Jordan,
> 
> We might want to through in an additional : assert((flags & BO_ALLOC_SMEM) ==
> 0); in the CCS case

Yeah. I noticed this potential for concern when looking at the
small-bar uapi on iris. I added an assert, and I haven't seen it get
triggered yet.

-Jordan

Re: [PATCH v3] uapi/drm/i915: Document memory residency and Flat-CCS capability of obj

[Jordan Justen]

On 2022-05-13 05:31:00, Lionel Landwerlin wrote:
> On 02/05/2022 17:15, Ramalingam C wrote:
> > Capture the impact of memory region preference list of the objects, on
> > their memory residency and Flat-CCS capability.
> >
> > v2:
> >Fix the Flat-CCS capability of an obj with {lmem, smem} preference
> >list [Thomas]
> > v3:
> >Reworded the doc [Matt]
> >
> > Signed-off-by: Ramalingam C 
> > cc: Matthew Auld 
> > cc: Thomas Hellstrom 
> > cc: Daniel Vetter 
> > cc: Jon Bloomfield 
> > cc: Lionel Landwerlin 
> > cc: Kenneth Graunke 
> > cc: mesa-...@lists.freedesktop.org
> > cc: Jordan Justen 
> > cc: Tony Ye 
> > Reviewed-by: Matthew Auld 
> > ---
> >   include/uapi/drm/i915_drm.h | 16 
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index a2def7b27009..b7e1c2fe08dc 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -3443,6 +3443,22 @@ struct drm_i915_gem_create_ext {
> >* At which point we get the object handle in 
> > _i915_gem_create_ext.handle,
> >* along with the final object size in _i915_gem_create_ext.size, 
> > which
> >* should account for any rounding up, if required.
> > + *
> > + * Note that userspace has no means of knowing the current backing region
> > + * for objects where @num_regions is larger than one. The kernel will only
> > + * ensure that the priority order of the @regions array is honoured, either
> > + * when initially placing the object, or when moving memory around due to
> > + * memory pressure
> > + *
> > + * On Flat-CCS capable HW, compression is supported for the objects 
> > residing
> > + * in I915_MEMORY_CLASS_DEVICE. When such objects (compressed) has other
> > + * memory class in @regions and migrated (by I915, due to memory
> > + * constrain) to the non I915_MEMORY_CLASS_DEVICE region, then I915 needs 
> > to
> > + * decompress the content. But I915 dosen't have the required information 
> > to
> > + * decompress the userspace compressed objects.
> > + *
> > + * So I915 supports Flat-CCS, only on the objects which can reside only on
> > + * I915_MEMORY_CLASS_DEVICE regions.
> 
> I think it's fine to assume Flat-CSS surface will always be in lmem.
> 
> I see no issue for the Anv Vulkan driver.
> 
> Maybe Nanley or Ken can speak for the Iris GL driver?
> 

Acked-by: Jordan Justen 

I think Nanley has accounted for this on iris with:

https://gitlab.freedesktop.org/mesa/mesa/-/commit/42a865730ef72574e179b56a314f30fdccc6cba8

-Jordan

Re: [Intel-gfx] [PATCH v2 0/4] i915: Turn on compute engine support

[Jordan Justen]

I did some light testing with our anvil (Vulkan) and iris (OpenGL)
Mesa drivers after applying these patches on top of drm-tip tagged
intel/CI_DRM_11574. All the unit tests that I tried passed. I also ran
the gl_manhattan31 benchmark which used the compute engine for iris
compute shader ops.

Series:

Reviewed-by: Jordan Justen 
Tested-by: Jordan Justen 

-Jordan

On 2022-04-27 21:19:22, Matt Roper wrote:
> Now that the necessary GuC-based hardware workarounds have landed, we're
> finally ready to actually enable compute engines for use by userspace.
> All of the "under-the-hood" heavy lifting already landed a while back in
> other series so all that remains now is to add I915_ENGINE_CLASS_COMPUTE
> to the uapi enum and add the CCS engines to the engine lists for the
> Xe_HP SDV and DG2.
> 
> Userspace (Mesa) is linked in the ABI patch.  Existing IGT tests (e.g.,
> i915_hangman) provide test coverage for general engine behavior since compute
> engines should follow the same general rules as other engines.  We've also
> recently added some additional subtests like
> igt@gem_reset_stats@shared-reset-domain to cover the user-visible impacts of
> the compute engines sharing the same hardware reset domain as the render
> engine.
> 
> v2:
>  - Update TLB invalidation register for compute engines and move it to a
>separate patch since it isn't related to the new uapi.  (Tvrtko,
>Prathap)
>  - Move new kerneldoc for pre-existing engine classes to a separate
>patch.  (Andi)
>  - Drop the compute UMD merge request link for now because it also
>included some additional multi-tile uapi that we're not ready to
>upstream just yet.  Even if they don't have a disentangled MR ready
>for reference, we still have the Mesa MR as a key userspace consumer.
>(Tvrtko)
> 
> Cc: Lucas De Marchi 
> Cc: Tvrtko Ursulin 
> 
> Daniele Ceraolo Spurio (1):
>   drm/i915: Xe_HP SDV and DG2 have up to 4 CCS engines
> 
> Matt Roper (3):
>   drm/i915/uapi: Add kerneldoc for engine class enum
>   drm/i915/xehp: Add register for compute engine's MMIO-based TLB
> invalidation
>   drm/i915/xehp: Add compute engine ABI
> 
>  drivers/gpu/drm/i915/gt/intel_engine_user.c |  2 +-
>  drivers/gpu/drm/i915/gt/intel_gt.c  |  1 +
>  drivers/gpu/drm/i915/gt/intel_gt_regs.h |  1 +
>  drivers/gpu/drm/i915/i915_drm_client.c  |  1 +
>  drivers/gpu/drm/i915/i915_drm_client.h  |  2 +-
>  drivers/gpu/drm/i915/i915_pci.c |  6 +-
>  include/uapi/drm/i915_drm.h | 62 +++--
>  7 files changed, 65 insertions(+), 10 deletions(-)
> 
> -- 
> 2.35.1
>

[PATCH v7 1/2] drm/i915/guc: Add fetch of hwconfig blob

[Jordan Justen]

From: John Harrison 

Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.

The table is stored in the GT structure so that it can be fetched once
at driver load time. Keeping inside a GuC structure would mean it
would be release and reloaded on a GuC reset (part of a full GT
reset). However, the table does not change just because the GT has been
reset and the GuC reloaded. Also, dynamic memory allocations inside
the reset path are a problem.

Note that the table is only available on ADL-P and later platforms.

v2 (John's v2 patch):
 * Move to GT level to avoid memory allocation during reset path (and
   unnecessary re-read of the table on a reset).

v5 (of Jordan's posting):
 * Various changes made by Jordan and recommended by Michal
   - Makefile ordering
   - Adjust "struct intel_guc_hwconfig hwconfig" comment
   - Set Copyright year to 2022 in intel_guc_hwconfig.c/.h
   - Drop inline from hwconfig_to_guc()
   - Replace hwconfig param with guc in __guc_action_get_hwconfig()
   - Move zero size check into guc_hwconfig_discover_size()
   - Change comment to say zero size offset/size is needed to get size
   - Add has_guc_hwconfig to devinfo and drop has_table()
   - Change drm_err to notice in __uc_init_hw() and use %pe

v6 (of Jordan's posting):
 * Added a couple more small changes recommended by Michal
 * Merge in John's v2 patch, but note:
   - Using drm_notice as recommended by Michal
   - Reverted Michal's suggestion of using devinfo

v7 (of Jordan's posting):
 * Change back to drm_err as preferred by John

Cc: Michal Wajdeczko 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jon Bloomfield 
Signed-off-by: Jordan Justen 
Reviewed-by: Michal Wajdeczko 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/gt/intel_gt.c|   6 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h  |   4 +
 drivers/gpu/drm/i915/gt/intel_hwconfig.h  |  21 +++
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 162 ++
 7 files changed, 199 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/intel_hwconfig.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 9d588d936e3d..61b078bd1b32 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -187,6 +187,7 @@ i915-y += gt/uc/intel_uc.o \
  gt/uc/intel_guc_ct.o \
  gt/uc/intel_guc_debugfs.o \
  gt/uc/intel_guc_fw.o \
+ gt/uc/intel_guc_hwconfig.o \
  gt/uc/intel_guc_log.o \
  gt/uc/intel_guc_log_debugfs.o \
  gt/uc/intel_guc_rc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 8a2483ccbfb9..d731110ce3e6 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -712,6 +712,11 @@ int intel_gt_init(struct intel_gt *gt)
if (err)
goto err_uc_init;
 
+   err = intel_gt_init_hwconfig(gt);
+   if (err)
+   drm_err(>i915->drm, "Failed to retrieve hwconfig table: 
%pe\n",
+   ERR_PTR(err));
+
err = __engines_record_defaults(gt);
if (err)
goto err_gt;
@@ -793,6 +798,7 @@ void intel_gt_driver_release(struct intel_gt *gt)
intel_gt_pm_fini(gt);
intel_gt_fini_scratch(gt);
intel_gt_fini_buffer_pool(gt);
+   intel_gt_fini_hwconfig(gt);
 }
 
 void intel_gt_driver_late_release(struct intel_gt *gt)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h 
b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index f20687796490..514b92cff9b0 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -20,6 +20,7 @@
 #include "i915_vma.h"
 #include "intel_engine_types.h"
 #include "intel_gt_buffer_pool_types.h"
+#include "intel_hwconfig.h"
 #include "intel_llc_types.h"
 #include "intel_reset_types.h"
 #include "intel_rc6_types.h"
@@ -199,6 +200,9 @@ struct intel_gt {
struct sseu_dev_info sseu;
 
unsigned long mslice_mask;
+
+   /** @hwconfig: hardware configuration data */
+   struct intel_hwconfig hwconfig;
} info;
 
struct {
diff --git a/drivers/gpu/drm/i915/gt/intel_hwconfig.h 
b/drivers/gpu/drm/i915/gt/intel_hwconfig.h
new file mode 100644
index ..322290780b67
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_hwconfig.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_HWCONFIG_H_
+#define

[PATCH v7 2/2] drm/i915/uapi: Add query for hwconfig blob

[Jordan Justen]

From: Rodrigo Vivi 

In this interface i915 is returning a blob of data which it receives
from the guc software. This blob provides some useful data about the
hardware for drivers. The format of this blob will be documented in
the Programmer Reference Manuals when released.

Cc: Tvrtko Ursulin 
Cc: Kenneth Graunke 
Cc: Michal Wajdeczko 
Cc: Slawomir Milczarek 
Cc: Joonas Lahtinen 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jordan Justen 
Tested-by: Jordan Justen 
Acked-by: Jon Bloomfield 
---
 drivers/gpu/drm/i915/i915_query.c | 23 +++
 include/uapi/drm/i915_drm.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..b5ca00cb6cf6 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
*i915,
return total_length;
 }
 
+static int query_hwconfig_blob(struct drm_i915_private *i915,
+  struct drm_i915_query_item *query_item)
+{
+   struct intel_gt *gt = to_gt(i915);
+   struct intel_hwconfig *hwconfig = >info.hwconfig;
+
+   if (!hwconfig->size || !hwconfig->ptr)
+   return -ENODEV;
+
+   if (query_item->length == 0)
+   return hwconfig->size;
+
+   if (query_item->length < hwconfig->size)
+   return -EINVAL;
+
+   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+   return -EFAULT;
+
+   return hwconfig->size;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) 
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+   query_hwconfig_blob,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 05c3642aaece..071ffd9d51f1 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2691,6 +2691,7 @@ struct drm_i915_query_item {
 #define DRM_I915_QUERY_ENGINE_INFO 2
 #define DRM_I915_QUERY_PERF_CONFIG  3
 #define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_HWCONFIG_BLOB   5
 /* Must be kept compact -- no holes and well documented */
 
/**
-- 
2.34.1

[PATCH v7 0/2] GuC HWCONFIG with documentation

[Jordan Justen]

Now with 100% less documentation. (See v7 notes.)

This is John/Rodrigo's 2 patches with some changes. There are various
changes suggested by Michal in John's "fetch" patch. The table
terminology was also changed to "blob" as requested by Joonas.

Dropped these 2 patches (which were in v1-v6):
 * "drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item"
 * "drm/i915/guc: Verify hwconfig blob matches supported format"

v2:
 * Fix -Werror errors.
 * Rebase to drm-intel/for-linux-next instead of
   drm-intel/for-linux-next-gt, as this seems to be what CI wants.
 * Fix u32 -> __u32.
 * Add commit message for "Verify hwconfig blob" patch as requested by
   Tvrtko.
 * Reword text added to i915_drm.h as requested by Tvrtko. (Attempting
   to indicate the overall blob ends right at the last blob item.)

v3:
 * Add several changes suggested by Tvrtko in the "Verify hwconfig
   blob", along with some tweaks to i915_drm.h from the feedback for
   the same patch.

v4:
 * Rewrite verify_hwconfig_blob() to hopefully be clearer without
   relying on comments so much, and add various suggestions from
   Michal.
 * Michal also had some suggestions in John's "drm/i915/guc: Add fetch
   of hwconfig table" patch. I held off on making any of these changes
   in this version.

v5:
 * Add many changes suggested by Michal in John's "drm/i915/guc: Add
   fetch of hwconfig table" patch.
 * Fix documenation formatting as suggested by Daniel in
   "drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item"

v6:
 * Updated "drm/i915/guc: Add fetch of hwconfig table" by merging in
   John's v2 patch which saves the hwconfig blob at the GT level. I
   also added a few changes requested by Michal on the v5 posting.
 * Tvrtko requested an example of UMD using the i915 hwconfig
   interface. Mesa support for this interface can be seen in this MR:
   https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14511

v7:
 * Jon took back his Acked-by for my documentation patches, so I had
   to drop those 2 patches. Jon and John feel very strongly that i915
   should not document or guarantee the format of the blob returned by
   this uapi. They say it should only be documented in the Programmer
   Reference Manuals whenever they are released, and userspace can
   verify it.
 * The "query" patch commit message no longer documents the uapi, and
   refers to the eventual release of PRMs.
 * I changed the "fetch" patch back to reporting drm_err after
   reviewing the discussion between Michal and John on this. (John
   preferred error, and Michal said this was ok.)

John Harrison (1):
  drm/i915/guc: Add fetch of hwconfig blob

Rodrigo Vivi (1):
  drm/i915/uapi: Add query for hwconfig blob

 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/gt/intel_gt.c|   6 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h  |   4 +
 drivers/gpu/drm/i915/gt/intel_hwconfig.h  |  21 +++
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 162 ++
 drivers/gpu/drm/i915/i915_query.c |  23 +++
 include/uapi/drm/i915_drm.h   |   1 +
 9 files changed, 223 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/intel_hwconfig.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c

-- 
2.34.1

[PATCH v6 2/4] drm/i915/uapi: Add query for hwconfig blob

[Jordan Justen]

From: Rodrigo Vivi 

The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data
which it receives from the GuC software. This blob provides some
useful data about the hardware for drivers.

Although the blob is not fully documented at this time, the basic
format is an array of u32 values. The array is a simple and flexible
KLV (Key/Length/Value) formatted table. For example, it could be just:
enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, };

  static const u32 hwconfig[] = {
  ATTR_SOME_VALUE,
  1, // Value Length in DWords
  8, // Value

  ATTR_SOME_MASK,
  3,
  0x00, 0x, 0xFF00,
  };

The attribute ids and meaning of the values will be documented in the
Programmer Reference Manuals when released.

Cc: Tvrtko Ursulin 
Cc: Kenneth Graunke 
Cc: Michal Wajdeczko 
Cc: Slawomir Milczarek 
Cc: Joonas Lahtinen 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jordan Justen 
Tested-by: Jordan Justen 
Acked-by: Jon Bloomfield 
---
 drivers/gpu/drm/i915/i915_query.c | 23 +++
 include/uapi/drm/i915_drm.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..b5ca00cb6cf6 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
*i915,
return total_length;
 }
 
+static int query_hwconfig_blob(struct drm_i915_private *i915,
+  struct drm_i915_query_item *query_item)
+{
+   struct intel_gt *gt = to_gt(i915);
+   struct intel_hwconfig *hwconfig = >info.hwconfig;
+
+   if (!hwconfig->size || !hwconfig->ptr)
+   return -ENODEV;
+
+   if (query_item->length == 0)
+   return hwconfig->size;
+
+   if (query_item->length < hwconfig->size)
+   return -EINVAL;
+
+   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+   return -EFAULT;
+
+   return hwconfig->size;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) 
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+   query_hwconfig_blob,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..069d2fadfbd9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
 #define DRM_I915_QUERY_ENGINE_INFO 2
 #define DRM_I915_QUERY_PERF_CONFIG  3
 #define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_HWCONFIG_BLOB   5
 /* Must be kept compact -- no holes and well documented */
 
/**
-- 
2.34.1

[PATCH v6 3/4] drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item

[Jordan Justen]

Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.

v3:
 * Add various changes suggested by Tvrtko

v5:
 * Fix documenation formatting and verified with `make htmldocs` as
   suggested by Daniel

Cc: Daniel Vetter 
Signed-off-by: Jordan Justen 
Acked-by: Jon Bloomfield 
Acked-by: Daniel Vetter 
---
 include/uapi/drm/i915_drm.h | 43 +
 1 file changed, 43 insertions(+)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 069d2fadfbd9..d033211cb862 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3279,6 +3279,49 @@ struct drm_i915_gem_create_ext_protected_content {
 /* ID of the protected content session managed by i915 when PXP is active */
 #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
 
+/**
+ * DOC: GuC HWCONFIG blob uAPI
+ *
+ * The GuC produces a blob with information about the current device.
+ * i915 reads this blob from GuC and makes it available via this uAPI.
+ *
+ * The returned blob is a sequence of items of variable length
+ * described by struct drm_i915_query_hwconfig_blob_item.
+ *
+ * The overall blob returned by DRM_I915_QUERY_HWCONFIG_BLOB will end
+ * at the same location as the end of the final struct
+ * drm_i915_query_hwconfig_blob_item. In other words, walking through
+ * the individual items is guaranteed to eventually arrive at the
+ * exact end of the entire blob.
+ */
+
+/**
+ * struct drm_i915_query_hwconfig_blob_item - A single hwconfig item
+ * within the sequence of hwconfig items returned by
+ * DRM_I915_QUERY_HWCONFIG_BLOB.
+ *
+ * The length field gives the length of the data[] array. The length
+ * is the number of u32 items in the data[] array, and *not* the
+ * number of bytes.
+ *
+ * The key and length fields are required, so the minimum item size is
+ * 2 x u32, or 8 bytes, when the length field is 0. If the length
+ * field is 1, then the item's size is 12 bytes.
+ *
+ * The meaning of the key field and the data values are documented in
+ * the Programmer's Reference Manual.
+ */
+struct drm_i915_query_hwconfig_blob_item {
+   /** @key: Enum which defines how to interpret @data values. */
+   __u32 key;
+
+   /** @length: The number of u32 values in the @data array. */
+   __u32 length;
+
+   /** @data: Array of values with meaning defined by @key. */
+   __u32 data[];
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.34.1

[PATCH v6 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Jordan Justen]

i915_drm.h now defines the format of the returned
DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from
the black box GuC software, it should verify that the data matches
that format before sending it to user-space.

The verification makes a single simple pass through the blob contents,
so this verification step should not add a significant amount of init
time to i915.

v3:
 * Add various changes suggested by Tvrtko

v4:
 * Rewrite verify_hwconfig_blob() to hopefully be clearer without
   relying on comments so much, and add various suggestions from
   Michal.

v6:
 * Rework based on John's updated "drm/i915/guc: Add fetch of hwconfig
   table" v2 which stores the hwconfig blob at the GT level.

Signed-off-by: Jordan Justen 
Acked-by: Jon Bloomfield 
Reviewed-by: Tvrtko Ursulin 
---
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 43 ++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
index e0f65bdd1c84..1a3134d3d434 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -68,8 +68,44 @@ static int guc_hwconfig_discover_size(struct intel_guc *guc, 
struct intel_hwconf
return 0;
 }
 
+static int verify_hwconfig_blob(struct intel_guc *guc, struct intel_hwconfig 
*hwconfig)
+{
+   struct drm_device *drm = _to_gt(guc)->i915->drm;
+   struct drm_i915_query_hwconfig_blob_item *item = hwconfig->ptr;
+   u64 offset = 0;
+   u64 remaining = hwconfig->size;
+   /* Everything before the data field is required */
+   u64 min_item_size = offsetof(struct drm_i915_query_hwconfig_blob_item, 
data);
+   u64 item_size;
+
+   if (!IS_ALIGNED(hwconfig->size, sizeof(u32))) {
+   drm_err(drm, "hwconfig blob size (%d) is not u32 aligned\n", 
hwconfig->size);
+   return -EINVAL;
+   }
+
+   while (offset < hwconfig->size) {
+   if (remaining < min_item_size) {
+   drm_err(drm, "hwconfig blob invalid (no room for item 
required fields at offset %lld)\n",
+   offset);
+   return -EINVAL;
+   }
+   item_size = min_item_size + sizeof(u32) * item->length;
+   if (item_size > remaining) {
+   drm_err(drm, "hwconfig blob invalid (no room for data 
array of item at offset %lld)\n",
+   offset);
+   return -EINVAL;
+   }
+   offset += item_size;
+   remaining -= item_size;
+   item = (void *)>data[item->length];
+   }
+
+   return 0;
+}
+
 static int guc_hwconfig_fill_buffer(struct intel_guc *guc, struct 
intel_hwconfig *hwconfig)
 {
+   struct drm_device *drm = _to_gt(guc)->i915->drm;
struct i915_vma *vma;
u32 ggtt_offset;
void *vaddr;
@@ -84,8 +120,13 @@ static int guc_hwconfig_fill_buffer(struct intel_guc *guc, 
struct intel_hwconfig
ggtt_offset = intel_guc_ggtt_offset(guc, vma);
 
ret = __guc_action_get_hwconfig(guc, ggtt_offset, hwconfig->size);
-   if (ret >= 0)
+   if (ret >= 0) {
memcpy(hwconfig->ptr, vaddr, hwconfig->size);
+   if (verify_hwconfig_blob(guc, hwconfig)) {
+   drm_err(drm, "Ignoring invalid hwconfig blob received 
from GuC!\n");
+   ret = -EINVAL;
+   }
+   }
 
i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP);
 
-- 
2.34.1

[PATCH v6 1/4] drm/i915/guc: Add fetch of hwconfig table

[Jordan Justen]

From: John Harrison 

Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.

The table is stored in the GT structure so that it can be fetched once
at driver load time. Keeping inside a GuC structure would mean it
would be release and reloaded on a GuC reset (part of a full GT
reset). However, the table does not change just because the GT has been
reset and the GuC reloaded. Also, dynamic memory allocations inside
the reset path are a problem.

Note that the table is only available on ADL-P and later platforms.

v2 (John's v2 patch):
 * Move to GT level to avoid memory allocation during reset path (and
   unnecessary re-read of the table on a reset).

v5 (of Jordan's posting):
 * Various changes made by Jordan and recommended by Michal
   - Makefile ordering
   - Adjust "struct intel_guc_hwconfig hwconfig" comment
   - Set Copyright year to 2022 in intel_guc_hwconfig.c/.h
   - Drop inline from hwconfig_to_guc()
   - Replace hwconfig param with guc in __guc_action_get_hwconfig()
   - Move zero size check into guc_hwconfig_discover_size()
   - Change comment to say zero size offset/size is needed to get size
   - Add has_guc_hwconfig to devinfo and drop has_table()
   - Change drm_err to notice in __uc_init_hw() and use %pe

v6 (of Jordan's posting):
 * Added a couple more small changes recommended by Michal
 * Merge in John's v2 patch, but note:
   - Using drm_notice as recommended by Michal
   - Reverted Michal's suggestion of using devinfo

Cc: Michal Wajdeczko 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jon Bloomfield 
Signed-off-by: Jordan Justen 
Reviewed-by: Michal Wajdeczko 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/gt/intel_gt.c|   6 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h  |   4 +
 drivers/gpu/drm/i915/gt/intel_hwconfig.h  |  21 +++
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 162 ++
 7 files changed, 199 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/intel_hwconfig.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 9d588d936e3d..61b078bd1b32 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -187,6 +187,7 @@ i915-y += gt/uc/intel_uc.o \
  gt/uc/intel_guc_ct.o \
  gt/uc/intel_guc_debugfs.o \
  gt/uc/intel_guc_fw.o \
+ gt/uc/intel_guc_hwconfig.o \
  gt/uc/intel_guc_log.o \
  gt/uc/intel_guc_log_debugfs.o \
  gt/uc/intel_guc_rc.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c 
b/drivers/gpu/drm/i915/gt/intel_gt.c
index e8403fa53909..bf02fb28562a 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -712,6 +712,11 @@ int intel_gt_init(struct intel_gt *gt)
if (err)
goto err_uc_init;
 
+   err = intel_gt_init_hwconfig(gt);
+   if (err)
+   drm_notice(>i915->drm, "Failed to retrieve hwconfig table: 
%pe\n",
+  ERR_PTR(err));
+
err = __engines_record_defaults(gt);
if (err)
goto err_gt;
@@ -793,6 +798,7 @@ void intel_gt_driver_release(struct intel_gt *gt)
intel_gt_pm_fini(gt);
intel_gt_fini_scratch(gt);
intel_gt_fini_buffer_pool(gt);
+   intel_gt_fini_hwconfig(gt);
 }
 
 void intel_gt_driver_late_release(struct intel_gt *gt)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h 
b/drivers/gpu/drm/i915/gt/intel_gt_types.h
index f20687796490..514b92cff9b0 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
@@ -20,6 +20,7 @@
 #include "i915_vma.h"
 #include "intel_engine_types.h"
 #include "intel_gt_buffer_pool_types.h"
+#include "intel_hwconfig.h"
 #include "intel_llc_types.h"
 #include "intel_reset_types.h"
 #include "intel_rc6_types.h"
@@ -199,6 +200,9 @@ struct intel_gt {
struct sseu_dev_info sseu;
 
unsigned long mslice_mask;
+
+   /** @hwconfig: hardware configuration data */
+   struct intel_hwconfig hwconfig;
} info;
 
struct {
diff --git a/drivers/gpu/drm/i915/gt/intel_hwconfig.h 
b/drivers/gpu/drm/i915/gt/intel_hwconfig.h
new file mode 100644
index ..322290780b67
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/intel_hwconfig.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_HWCONFIG_H_
+#define _INTEL_HWCONFIG_H_
+
+#include 
+
+struct intel_gt;
+
+struct intel_h

[PATCH v6 0/4] GuC HWCONFIG with documentation

[Jordan Justen]

This is John/Rodrigo's 2 patches with some minor changes, and I added
2 patches.

"drm/i915/uapi: Add query for hwconfig blob" was changed:

 * Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB
   as requested by Joonas.

 * Reword commit message

 * I added Acked-by to this patch, but this Acked-by only applies in
   the context of this version of the patchset.

"drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a
struct to the uAPI and documents the return value for
DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still
deferred to the PRM.)

"drm/i915/guc: Verify hwconfig blob matches supported format" does the
simple verification of the blob to make sure it matches what the uAPI
documents.

v2:
 * Fix -Werror errors.
 * Rebase to drm-intel/for-linux-next instead of
   drm-intel/for-linux-next-gt, as this seems to be what CI wants.
 * Fix u32 -> __u32.
 * Add commit message for "Verify hwconfig blob" patch as requested by
   Tvrtko.
 * Reword text added to i915_drm.h as requested by Tvrtko. (Attempting
   to indicate the overall blob ends right at the last blob item.)

v3:
 * Add several changes suggested by Tvrtko in the "Verify hwconfig
   blob", along with some tweaks to i915_drm.h from the feedback for
   the same patch.

v4:
 * Rewrite verify_hwconfig_blob() to hopefully be clearer without
   relying on comments so much, and add various suggestions from
   Michal.
 * Michal also had some suggestions in John's "drm/i915/guc: Add fetch
   of hwconfig table" patch. I held off on making any of these changes
   in this version.

v5:
 * Add many changes suggested by Michal in John's "drm/i915/guc: Add
   fetch of hwconfig table" patch.
 * Fix documenation formatting as suggested by Daniel in
   "drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item"

v6:
 * Updated "drm/i915/guc: Add fetch of hwconfig table" by merging in
   John's v2 patch which saves the hwconfig blob at the GT level. I
   also added a few changes requested by Michal on the v5 posting.
 * Tvrtko requested an example of UMD using the i915 hwconfig
   interface. Mesa support for this interface can be seen in this MR:
   https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14511

John Harrison (1):
  drm/i915/guc: Add fetch of hwconfig table

Jordan Justen (2):
  drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item
  drm/i915/guc: Verify hwconfig blob matches supported format

Rodrigo Vivi (1):
  drm/i915/uapi: Add query for hwconfig blob

 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/gt/intel_gt.c|   6 +
 drivers/gpu/drm/i915/gt/intel_gt_types.h  |   4 +
 drivers/gpu/drm/i915/gt/intel_hwconfig.h  |  21 ++
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 203 ++
 drivers/gpu/drm/i915/i915_query.c |  23 ++
 include/uapi/drm/i915_drm.h   |  44 
 9 files changed, 307 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/intel_hwconfig.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c

-- 
2.34.1

Re: [PATCH v5 1/4] drm/i915/guc: Add fetch of hwconfig table

[Jordan Justen]

John Harrison  writes:

> On 2/22/2022 02:36, Jordan Justen wrote:
>> From: John Harrison 
>>
>> Implement support for fetching the hardware description table from the
>> GuC. The call is made twice - once without a destination buffer to
>> query the size and then a second time to fill in the buffer.
>>
>> Note that the table is only available on ADL-P and later platforms.
>>
>> v5 (of Jordan's posting):
>>   * Various changes made by Jordan and recommended by Michal
>> - Makefile ordering
>> - Adjust "struct intel_guc_hwconfig hwconfig" comment
>> - Set Copyright year to 2022 in intel_guc_hwconfig.c/.h
>> - Drop inline from hwconfig_to_guc()
>> - Replace hwconfig param with guc in __guc_action_get_hwconfig()
>> - Move zero size check into guc_hwconfig_discover_size()
>> - Change comment to say zero size offset/size is needed to get size
>> - Add has_guc_hwconfig to devinfo and drop has_table()
>> - Change drm_err to notice in __uc_init_hw() and use %pe
>>
>> Cc: Michal Wajdeczko 
>> Signed-off-by: Rodrigo Vivi 
>> Signed-off-by: John Harrison 
>> Reviewed-by: Matthew Brost 
>> Acked-by: Jon Bloomfield 
>> Signed-off-by: Jordan Justen 
>> ---
>>   
>> +ret = intel_guc_hwconfig_init(>hwconfig);
>> +if (ret)
>> +drm_notice(>drm, "Failed to retrieve hwconfig table: 
>> %pe\n",
> Why only drm_notice? As you are keen to point out, the UMDs won't work 
> if the table is not available. All the failure paths in your own 
> verification function are 'drm_err'. So why is it only a 'notice' if 
> there is no table at all?

This was requested by Michal in my v3 posting:

https://patchwork.freedesktop.org/patch/472936/?series=99787=3

I don't think that it should be a failure for i915 if it is unable to
read the table, or if the table read is invalid. I think it should be up
to the UMD to react to the missing hwconfig however they think is
appropriate, but I would like the i915 to guarantee & document the
format returned to userspace to whatever extent is feasible.

As you point out there is a discrepancy, and I think I should be
consistent with whatever is used here in my "drm/i915/guc: Verify
hwconfig blob matches supported format" patch.

I guess I'd tend to agree with Michal that "maybe drm_notice since we
continue probe", but I would go along with either if you two want to
discuss further.

> Note that this function is called as part of the reset path. The reset 
> path is not allowed to allocate memory. The table is stored in a 
> dynamically allocated object. Hence the IGT test failure. The table 
> query has to be done elsewhere at driver init time only.

Thanks for clearing this up. I did notice on dg2 that gpu resets were
causing a re-read of the hwconfig from GuC, but it definitely was not
clear to me that there would be a connection to the IGT failure that you
pointed out.

>
>> +   ERR_PTR(ret));
>> +
>>  ret = guc_enable_communication(guc);
>>  if (ret)
>>  goto err_log_capture;
>> @@ -562,6 +567,8 @@ static void __uc_fini_hw(struct intel_uc *uc)
>>  if (intel_uc_uses_guc_submission(uc))
>>  intel_guc_submission_disable(guc);
>>   
>> +intel_guc_hwconfig_fini(>hwconfig);
>> +
>>  __uc_sanitize(uc);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c 
>> b/drivers/gpu/drm/i915/i915_pci.c
>> index 76e590fcb903..1d31e35a5154 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -990,6 +990,7 @@ static const struct intel_device_info adl_p_info = {
>>  BIT(RCS0) | BIT(BCS0) | BIT(VECS0) | BIT(VCS0) | BIT(VCS2),
>>  .ppgtt_size = 48,
>>  .dma_mask_size = 39,
>> +.has_guc_hwconfig = 1,
> Who requested this change? It was previously done this way but the 
> instruction was that i915_pci.c is for hardware features only but that 
> this, as you seem extremely keen about pointing out at every 
> opportunity, is a software feature.

This was requested by Michal as well. I definitely agree it is a
software feature, but I was not aware that "i915_pci.c is for hardware
features only".

Michal, do you agree with this and returning to the previous method for
enabling the feature?

-Jordan

[PATCH v5 3/4] drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item

[Jordan Justen]

Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.

v3:
 * Add various changes suggested by Tvrtko

v5:
 * Fix documenation formatting and verified with `make htmldocs` as
   suggested by Daniel

Cc: Daniel Vetter 
Signed-off-by: Jordan Justen 
Acked-by: Jon Bloomfield 
Acked-by: Daniel Vetter 
---
 include/uapi/drm/i915_drm.h | 43 +
 1 file changed, 43 insertions(+)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 069d2fadfbd9..e44902ce8e64 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3276,6 +3276,49 @@ struct drm_i915_gem_create_ext_protected_content {
__u32 flags;
 };
 
+/**
+ * DOC: GuC HWCONFIG blob uAPI
+ *
+ * The GuC produces a blob with information about the current device.
+ * i915 reads this blob from GuC and makes it available via this uAPI.
+ *
+ * The returned blob is a sequence of items of variable length
+ * described by struct drm_i915_query_hwconfig_blob_item.
+ *
+ * The overall blob returned by DRM_I915_QUERY_HWCONFIG_BLOB will end
+ * at the same location as the end of the final struct
+ * drm_i915_query_hwconfig_blob_item. In other words, walking through
+ * the individual items is guaranteed to eventually arrive at the
+ * exact end of the entire blob.
+ */
+
+/**
+ * struct drm_i915_query_hwconfig_blob_item - A single hwconfig item
+ * within the sequence of hwconfig items returned by
+ * DRM_I915_QUERY_HWCONFIG_BLOB.
+ *
+ * The length field gives the length of the data[] array. The length
+ * is the number of u32 items in the data[] array, and *not* the
+ * number of bytes.
+ *
+ * The key and length fields are required, so the minimum item size is
+ * 2 x u32, or 8 bytes, when the length field is 0. If the length
+ * field is 1, then the item's size is 12 bytes.
+ *
+ * The meaning of the key field and the data values are documented in
+ * the Programmer's Reference Manual.
+ */
+struct drm_i915_query_hwconfig_blob_item {
+   /** @key: Enum which defines how to interpret @data values. */
+   __u32 key;
+
+   /** @length: The number of u32 values in the @data array. */
+   __u32 length;
+
+   /** @data: Array of values with meaning defined by @key. */
+   __u32 data[];
+};
+
 /* ID of the protected content session managed by i915 when PXP is active */
 #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
 
-- 
2.34.1

[PATCH v5 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Jordan Justen]

i915_drm.h now defines the format of the returned
DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from
the black box GuC software, it should verify that the data matches
that format before sending it to user-space.

The verification makes a single simple pass through the blob contents,
so this verification step should not add a significant amount of init
time to i915.

v3:
 * Add various changes suggested by Tvrtko

v4:
 * Rewrite verify_hwconfig_blob() to hopefully be clearer without
   relying on comments so much, and add various suggestions from
   Michal.

Signed-off-by: Jordan Justen 
Acked-by: Jon Bloomfield 
---
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 44 ++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
index ad289603460c..a844b880cbdb 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -73,9 +73,46 @@ static int guc_hwconfig_discover_size(struct 
intel_guc_hwconfig *hwconfig)
return 0;
 }
 
+static int verify_hwconfig_blob(struct intel_guc_hwconfig *hwconfig)
+{
+   struct intel_guc *guc = hwconfig_to_guc(hwconfig);
+   struct drm_device *drm = _to_gt(guc)->i915->drm;
+   struct drm_i915_query_hwconfig_blob_item *item = hwconfig->ptr;
+   u64 offset = 0;
+   u64 remaining = hwconfig->size;
+   /* Everything before the data field is required */
+   u64 min_item_size = offsetof(struct drm_i915_query_hwconfig_blob_item, 
data);
+   u64 item_size;
+
+   if (!IS_ALIGNED(hwconfig->size, sizeof(u32))) {
+   drm_err(drm, "hwconfig blob size (%d) is not u32 aligned\n", 
hwconfig->size);
+   return -EINVAL;
+   }
+
+   while (offset < hwconfig->size) {
+   if (remaining < min_item_size) {
+   drm_err(drm, "hwconfig blob invalid (no room for item 
required fields at offset %lld)\n",
+   offset);
+   return -EINVAL;
+   }
+   item_size = min_item_size + sizeof(u32) * item->length;
+   if (item_size > remaining) {
+   drm_err(drm, "hwconfig blob invalid (no room for data 
array of item at offset %lld)\n",
+   offset);
+   return -EINVAL;
+   }
+   offset += item_size;
+   remaining -= item_size;
+   item = (void *)>data[item->length];
+   }
+
+   return 0;
+}
+
 static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
 {
struct intel_guc *guc = hwconfig_to_guc(hwconfig);
+   struct drm_device *drm = _to_gt(guc)->i915->drm;
struct i915_vma *vma;
u32 ggtt_offset;
void *vaddr;
@@ -90,8 +127,13 @@ static int guc_hwconfig_fill_buffer(struct 
intel_guc_hwconfig *hwconfig)
ggtt_offset = intel_guc_ggtt_offset(guc, vma);
 
ret = __guc_action_get_hwconfig(guc, ggtt_offset, hwconfig->size);
-   if (ret >= 0)
+   if (ret >= 0) {
memcpy(hwconfig->ptr, vaddr, hwconfig->size);
+   if (verify_hwconfig_blob(hwconfig)) {
+   drm_err(drm, "Ignoring invalid hwconfig blob received 
from GuC!\n");
+   ret = -EINVAL;
+   }
+   }
 
i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP);
 
-- 
2.34.1

[PATCH v5 2/4] drm/i915/uapi: Add query for hwconfig blob

[Jordan Justen]

From: Rodrigo Vivi 

The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data
which it receives from the GuC software. This blob provides some
useful data about the hardware for drivers.

Although the blob is not fully documented at this time, the basic
format is an array of u32 values. The array is a simple and flexible
KLV (Key/Length/Value) formatted table. For example, it could be just:
enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, };

  static const u32 hwconfig[] = {
  ATTR_SOME_VALUE,
  1, // Value Length in DWords
  8, // Value

  ATTR_SOME_MASK,
  3,
  0x00, 0x, 0xFF00,
  };

The attribute ids and meaning of the values will be documented in the
Programmer Reference Manuals when released.

Cc: Tvrtko Ursulin 
Cc: Kenneth Graunke 
Cc: Michal Wajdeczko 
Cc: Slawomir Milczarek 
Cc: Joonas Lahtinen 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jordan Justen 
Tested-by: Jordan Justen 
Acked-by: Jon Bloomfield 
---
 drivers/gpu/drm/i915/i915_query.c | 23 +++
 include/uapi/drm/i915_drm.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..195524e9a369 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
*i915,
return total_length;
 }
 
+static int query_hwconfig_blob(struct drm_i915_private *i915,
+  struct drm_i915_query_item *query_item)
+{
+   struct intel_gt *gt = to_gt(i915);
+   struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
+
+   if (!hwconfig->size || !hwconfig->ptr)
+   return -ENODEV;
+
+   if (query_item->length == 0)
+   return hwconfig->size;
+
+   if (query_item->length < hwconfig->size)
+   return -EINVAL;
+
+   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+   return -EFAULT;
+
+   return hwconfig->size;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) 
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+   query_hwconfig_blob,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..069d2fadfbd9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
 #define DRM_I915_QUERY_ENGINE_INFO 2
 #define DRM_I915_QUERY_PERF_CONFIG  3
 #define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_HWCONFIG_BLOB   5
 /* Must be kept compact -- no holes and well documented */
 
/**
-- 
2.34.1

[PATCH v5 1/4] drm/i915/guc: Add fetch of hwconfig table

[Jordan Justen]

From: John Harrison 

Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.

Note that the table is only available on ADL-P and later platforms.

v5 (of Jordan's posting):
 * Various changes made by Jordan and recommended by Michal
   - Makefile ordering
   - Adjust "struct intel_guc_hwconfig hwconfig" comment
   - Set Copyright year to 2022 in intel_guc_hwconfig.c/.h
   - Drop inline from hwconfig_to_guc()
   - Replace hwconfig param with guc in __guc_action_get_hwconfig()
   - Move zero size check into guc_hwconfig_discover_size()
   - Change comment to say zero size offset/size is needed to get size
   - Add has_guc_hwconfig to devinfo and drop has_table()
   - Change drm_err to notice in __uc_init_hw() and use %pe

Cc: Michal Wajdeczko 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jon Bloomfield 
Signed-off-by: Jordan Justen 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 145 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 +++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   7 +
 drivers/gpu/drm/i915/i915_pci.c   |   1 +
 drivers/gpu/drm/i915/intel_device_info.h  |   1 +
 9 files changed, 182 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index e9ce09620eb5..661f1afb51d7 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -188,6 +188,7 @@ i915-y += gt/uc/intel_uc.o \
  gt/uc/intel_guc_ct.o \
  gt/uc/intel_guc_debugfs.o \
  gt/uc/intel_guc_fw.o \
+ gt/uc/intel_guc_hwconfig.o \
  gt/uc/intel_guc_log.o \
  gt/uc/intel_guc_log_debugfs.o \
  gt/uc/intel_guc_rc.o \
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
index fe5d7d261797..4a61c819f32b 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -137,6 +137,7 @@ enum intel_guc_action {
INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004,
INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
+   INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100,
INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502,
INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503,
INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
index 488b6061ee89..f9e2a6aaef4a 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -8,6 +8,10 @@
 
 enum intel_guc_response_status {
INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
+   INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20,
+   INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201,
+   INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202,
+   INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204,
INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
 };
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index f9240d4baa69..2058eb8c3d0c 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -13,6 +13,7 @@
 #include "intel_guc_fw.h"
 #include "intel_guc_fwif.h"
 #include "intel_guc_ct.h"
+#include "intel_guc_hwconfig.h"
 #include "intel_guc_log.h"
 #include "intel_guc_reg.h"
 #include "intel_guc_slpc_types.h"
@@ -37,6 +38,8 @@ struct intel_guc {
struct intel_guc_ct ct;
/** @slpc: sub-structure containing SLPC related data and objects */
struct intel_guc_slpc slpc;
+   /** @hwconfig: data related to hardware configuration KLV blob */
+   struct intel_guc_hwconfig hwconfig;
 
/** @sched_engine: Global engine used to submit requests to GuC */
struct i915_sched_engine *sched_engine;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
new file mode 100644
index ..ad289603460c
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include "gt/intel_gt.h"
+#include "i915_drv.h"
+#include "i915_memcpy.h"
+#include "intel_guc_hwconfig.h"
+
+stati

[PATCH v5 0/4] GuC HWCONFIG with documentation

[Jordan Justen]

This is John/Rodrigo's 2 patches with some minor changes, and I added
2 patches.

"drm/i915/uapi: Add query for hwconfig blob" was changed:

 * Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB
   as requested by Joonas.

 * Reword commit message

 * I added Acked-by to this patch, but this only applies in the
   context of this version of the patchset. If my changes are
   rejected, then please *do not* add my Acked-by to the other series.

   In particular, I do not want my Acked-by on the patch if the patch
   mentions the HWCONFIG format, but is not willing to add that to the
   actual uAPI.

   I also do not want my Acked-by on it if it mentions "consolidation"
   of this data. Since we are dealing with open source projects (aside
   from GuC), this doesn't seem appropriate.

"drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a
struct to the uAPI and documents the return value for
DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still
deferred to the PRM.)

"drm/i915/guc: Verify hwconfig blob matches supported format" does the
simple verification of the blob to make sure it matches what the uAPI
documents.

v2:
 * Fix -Werror errors.
 * Rebase to drm-intel/for-linux-next instead of
   drm-intel/for-linux-next-gt, as this seems to be what CI wants.
 * Fix u32 -> __u32.
 * Add commit message for "Verify hwconfig blob" patch as requested by
   Tvrtko.
 * Reword text added to i915_drm.h as requested by Tvrtko. (Attempting
   to indicate the overall blob ends right at the last blob item.)

v3:
 * Add several changes suggested by Tvrtko in the "Verify hwconfig
   blob", along with some tweaks to i915_drm.h from the feedback for
   the same patch.

v4:
 * Rewrite verify_hwconfig_blob() to hopefully be clearer without
   relying on comments so much, and add various suggestions from
   Michal.
 * Michal also had some suggestions in John's "drm/i915/guc: Add fetch
   of hwconfig table" patch. I held off on making any of these changes
   in this version.

v5:
 * Add many changes suggested by Michal in John's "drm/i915/guc: Add
   fetch of hwconfig table" patch.
 * Fix documenation formatting as suggested by Daniel in
   "drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item"

John Harrison (1):
  drm/i915/guc: Add fetch of hwconfig table

Jordan Justen (2):
  drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item
  drm/i915/guc: Verify hwconfig blob matches supported format

Rodrigo Vivi (1):
  drm/i915/uapi: Add query for hwconfig blob

 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 187 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   7 +
 drivers/gpu/drm/i915/i915_pci.c   |   1 +
 drivers/gpu/drm/i915/i915_query.c |  23 +++
 drivers/gpu/drm/i915/intel_device_info.h  |   1 +
 include/uapi/drm/i915_drm.h   |  44 +
 11 files changed, 291 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

-- 
2.34.1

Re: [PATCH v8 5/5] drm/i915/uapi: document behaviour for DG2 64K support

[Jordan Justen]

Ramalingam C  writes:

> On 2022-02-18 at 18:06:00 +, Robert Beckett wrote:
>> 
>> If desired, we can make the wording clearer, maybe something like:
>> 
>> "To keep things simple for userland, we mandate that any GTT mappings
>> must be aligned to 2MB. The kernel will internally pad them out to the next
>> 2MB boundary"
>
> Added the extra information in next version @
> https://patchwork.freedesktop.org/patch/475166/?series=100419=1
>
> Jordan, hope this explanation clears your doubt.

Ok. It sounds like what we are doing in Mesa matches what is required by
hardware and the kernel. Thanks.

-Jordan

Re: [PATCH v8 5/5] drm/i915/uapi: document behaviour for DG2 64K support

[Jordan Justen]

Robert Beckett  writes:

> From: Matthew Auld 
>
> On discrete platforms like DG2, we need to support a minimum page size
> of 64K when dealing with device local-memory. This is quite tricky for
> various reasons, so try to document the new implicit uapi for this.
>
> v3: fix typos and less emphasis
> v2: Fixed suggestions on formatting [Daniel]
>
> Signed-off-by: Matthew Auld 
> Signed-off-by: Ramalingam C 
> Signed-off-by: Robert Beckett 
> Acked-by: Jordan Justen 
> Reviewed-by: Ramalingam C 
> Reviewed-by: Thomas Hellström 
> cc: Simon Ser 
> cc: Pekka Paalanen 
> Cc: Jordan Justen 
> Cc: Kenneth Graunke 
> Cc: mesa-...@lists.freedesktop.org
> Cc: Tony Ye 
> Cc: Slawomir Milczarek 
> ---
>  include/uapi/drm/i915_drm.h | 44 -
>  1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 5e678917da70..77e5e74c32c1 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1118,10 +1118,16 @@ struct drm_i915_gem_exec_object2 {
>   /**
>* When the EXEC_OBJECT_PINNED flag is specified this is populated by
>* the user with the GTT offset at which this object will be pinned.
> +  *
>* When the I915_EXEC_NO_RELOC flag is specified this must contain the
>* presumed_offset of the object.
> +  *
>* During execbuffer2 the kernel populates it with the value of the
>* current GTT offset of the object, for future presumed_offset writes.
> +  *
> +  * See struct drm_i915_gem_create_ext for the rules when dealing with
> +  * alignment restrictions with I915_MEMORY_CLASS_DEVICE, on devices with
> +  * minimum page sizes, like DG2.
>*/
>   __u64 offset;
>  
> @@ -3145,11 +3151,39 @@ struct drm_i915_gem_create_ext {
>*
>* The (page-aligned) allocated size for the object will be returned.
>*
> -  * Note that for some devices we have might have further minimum
> -  * page-size restrictions(larger than 4K), like for device local-memory.
> -  * However in general the final size here should always reflect any
> -  * rounding up, if for example using the 
> I915_GEM_CREATE_EXT_MEMORY_REGIONS
> -  * extension to place the object in device local-memory.
> +  *
> +  * DG2 64K min page size implications:
> +  *
> +  * On discrete platforms, starting from DG2, we have to contend with GTT
> +  * page size restrictions when dealing with I915_MEMORY_CLASS_DEVICE
> +  * objects.  Specifically the hardware only supports 64K or larger GTT
> +  * page sizes for such memory. The kernel will already ensure that all
> +  * I915_MEMORY_CLASS_DEVICE memory is allocated using 64K or larger page
> +  * sizes underneath.
> +  *
> +  * Note that the returned size here will always reflect any required
> +  * rounding up done by the kernel, i.e 4K will now become 64K on devices
> +  * such as DG2.
> +  *
> +  * Special DG2 GTT address alignment requirement:
> +  *
> +  * The GTT alignment will also need to be at least 2M for such objects.
> +  *
> +  * Note that due to how the hardware implements 64K GTT page support, we
> +  * have some further complications:
> +  *
> +  *   1) The entire PDE (which covers a 2MB virtual address range), must
> +  *   contain only 64K PTEs, i.e mixing 4K and 64K PTEs in the same
> +  *   PDE is forbidden by the hardware.
> +  *
> +  *   2) We still need to support 4K PTEs for I915_MEMORY_CLASS_SYSTEM
> +  *   objects.
> +  *
> +  * To keep things simple for userland, we mandate that any GTT mappings
> +  * must be aligned to and rounded up to 2MB.

Could I get a clarification about this "rounded up" part.

Currently Mesa is aligning the start of each and every buffer VMA to be
2MiB aligned. But, we are *not* taking any steps to "round up" the size
of buffers to 2MiB alignment.

Bob's Mesa MR from a while ago,

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14599

was trying to add this "round up" size for buffers. We didn't accept
this MR because we thought if we have ensured that no other buffer will
use the same 2MiB VMA range, then it should not be required.

If what we are doing is ok, then maybe this "round up" language should
be dropped? Or, perhaps the "round up" mentioned here isn't implying we
must align the size of buffers that we create, and I'm misinterpreting
this.

-Jordan

> As this only wastes virtual
> +  * address space and avoids userland having to copy any needlessly
> +  * complicated PDE sharing scheme (coloring) and only affects DG2, this
> +  * is deemed to be a good compromise.
>*/
>   __u64 size;
>   /**
> -- 
> 2.25.1

[PATCH v4 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Jordan Justen]

i915_drm.h now defines the format of the returned
DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from
the black box GuC software, it should verify that the data matches
that format before sending it to user-space.

The verification makes a single simple pass through the blob contents,
so this verification step should not add a significant amount of init
time to i915.

v3:
 * Add various changes suggested by Tvrtko

v4:
 * Rewrite verify_hwconfig_blob() to hopefully be clearer without
   relying on comments so much, and add various suggestions from
   Michal.

Signed-off-by: Jordan Justen 
Acked-by: Jon Bloomfield 
---
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 44 ++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
index ce6088f112d4..7d5569d8d1e3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -71,9 +71,46 @@ static int guc_hwconfig_discover_size(struct 
intel_guc_hwconfig *hwconfig)
return 0;
 }
 
+static int verify_hwconfig_blob(struct intel_guc_hwconfig *hwconfig)
+{
+   struct intel_guc *guc = hwconfig_to_guc(hwconfig);
+   struct drm_device *drm = _to_gt(guc)->i915->drm;
+   struct drm_i915_query_hwconfig_blob_item *item = hwconfig->ptr;
+   u64 offset = 0;
+   u64 remaining = hwconfig->size;
+   /* Everything before the data field is required */
+   u64 min_item_size = offsetof(struct drm_i915_query_hwconfig_blob_item, 
data);
+   u64 item_size;
+
+   if (!IS_ALIGNED(hwconfig->size, sizeof(u32))) {
+   drm_err(drm, "hwconfig blob size (%d) is not u32 aligned\n", 
hwconfig->size);
+   return -EINVAL;
+   }
+
+   while (offset < hwconfig->size) {
+   if (remaining < min_item_size) {
+   drm_err(drm, "hwconfig blob invalid (no room for item 
required fields at offset %lld)\n",
+   offset);
+   return -EINVAL;
+   }
+   item_size = min_item_size + sizeof(u32) * item->length;
+   if (item_size > remaining) {
+   drm_err(drm, "hwconfig blob invalid (no room for data 
array of item at offset %lld)\n",
+   offset);
+   return -EINVAL;
+   }
+   offset += item_size;
+   remaining -= item_size;
+   item = (void *)>data[item->length];
+   }
+
+   return 0;
+}
+
 static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
 {
struct intel_guc *guc = hwconfig_to_guc(hwconfig);
+   struct drm_device *drm = _to_gt(guc)->i915->drm;
struct i915_vma *vma;
u32 ggtt_offset;
void *vaddr;
@@ -88,8 +125,13 @@ static int guc_hwconfig_fill_buffer(struct 
intel_guc_hwconfig *hwconfig)
ggtt_offset = intel_guc_ggtt_offset(guc, vma);
 
ret = __guc_action_get_hwconfig(hwconfig, ggtt_offset, hwconfig->size);
-   if (ret >= 0)
+   if (ret >= 0) {
memcpy(hwconfig->ptr, vaddr, hwconfig->size);
+   if (verify_hwconfig_blob(hwconfig)) {
+   drm_err(drm, "Ignoring invalid hwconfig blob received 
from GuC!\n");
+   ret = -EINVAL;
+   }
+   }
 
i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP);
 
-- 
2.34.1

[PATCH v4 3/4] drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item

[Jordan Justen]

Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.

v3:
 * Add various changes suggested by Tvrtko

Cc: Daniel Vetter 
Signed-off-by: Jordan Justen 
Acked-by: Jon Bloomfield 
---
 include/uapi/drm/i915_drm.h | 35 +++
 1 file changed, 35 insertions(+)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 069d2fadfbd9..8279515ae2ce 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3276,6 +3276,41 @@ struct drm_i915_gem_create_ext_protected_content {
__u32 flags;
 };
 
+/**
+ * DOC: GuC HWCONFIG blob uAPI
+ *
+ * The GuC produces a blob with information about the current device.
+ * i915 reads this blob from GuC and makes it available via this uAPI.
+ *
+ * The returned blob is a sequence of items of variable length
+ * described by struct drm_i915_query_hwconfig_blob_item. The
+ * drm_i915_query_hwconfig_blob_item length field gives the length of
+ * the drm_i915_query_hwconfig_blob_item data[] array. The length is
+ * the number of u32 items in the data[] array, and *not* the number
+ * of bytes.
+ *
+ * The key and length fields are required, so the minimum item size is
+ * 2 x u32, or 8 bytes, when the length field is 0. If the length
+ * field is 1, then the item's size is 12 bytes.
+ *
+ * The overall blob returned by DRM_I915_QUERY_HWCONFIG_BLOB will end
+ * at the same location as the end of the final
+ * drm_i915_query_hwconfig_blob_item. In other words, walking through
+ * the individual items is guaranteed to eventually arrive at the
+ * exact end of the entire blob.
+ *
+ * The meaning of the key field and the data values are documented in
+ * the Programmer's Reference Manual.
+ */
+struct drm_i915_query_hwconfig_blob_item {
+   /** @key: Enum which defines how to interpret @data values. */
+   __u32 key;
+   /** @length: The number of u32 values in the @data array. */
+   __u32 length;
+   /** @key: Array of values with meaning defined by @key */
+   __u32 data[];
+};
+
 /* ID of the protected content session managed by i915 when PXP is active */
 #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
 
-- 
2.34.1

[PATCH v4 2/4] drm/i915/uapi: Add query for hwconfig blob

[Jordan Justen]

From: Rodrigo Vivi 

The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data
which it receives from the GuC software. This blob provides some
useful data about the hardware for drivers.

Although the blob is not fully documented at this time, the basic
format is an array of u32 values. The array is a simple and flexible
KLV (Key/Length/Value) formatted table. For example, it could be just:
enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, };

  static const u32 hwconfig[] = {
  ATTR_SOME_VALUE,
  1, // Value Length in DWords
  8, // Value

  ATTR_SOME_MASK,
  3,
  0x00, 0x, 0xFF00,
  };

The attribute ids and meaning of the values will be documented in the
Programmer Reference Manuals when released.

Cc: Tvrtko Ursulin 
Cc: Kenneth Graunke 
Cc: Michal Wajdeczko 
Cc: Slawomir Milczarek 
Cc: Joonas Lahtinen 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jordan Justen 
Tested-by: Jordan Justen 
Acked-by: Jon Bloomfield 
---
 drivers/gpu/drm/i915/i915_query.c | 23 +++
 include/uapi/drm/i915_drm.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..195524e9a369 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
*i915,
return total_length;
 }
 
+static int query_hwconfig_blob(struct drm_i915_private *i915,
+  struct drm_i915_query_item *query_item)
+{
+   struct intel_gt *gt = to_gt(i915);
+   struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
+
+   if (!hwconfig->size || !hwconfig->ptr)
+   return -ENODEV;
+
+   if (query_item->length == 0)
+   return hwconfig->size;
+
+   if (query_item->length < hwconfig->size)
+   return -EINVAL;
+
+   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+   return -EFAULT;
+
+   return hwconfig->size;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) 
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+   query_hwconfig_blob,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..069d2fadfbd9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
 #define DRM_I915_QUERY_ENGINE_INFO 2
 #define DRM_I915_QUERY_PERF_CONFIG  3
 #define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_HWCONFIG_BLOB   5
 /* Must be kept compact -- no holes and well documented */
 
/**
-- 
2.34.1

[PATCH v4 1/4] drm/i915/guc: Add fetch of hwconfig table

[Jordan Justen]

From: John Harrison 

Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.

Note that the table is only available on ADL-P and later platforms.

Cc: Michal Wajdeczko 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jon Bloomfield 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 151 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 +++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 7 files changed, 185 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 16cab8db012a..784949070d6c 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -192,6 +192,7 @@ i915-y += gt/uc/intel_uc.o \
  gt/uc/intel_guc_rc.o \
  gt/uc/intel_guc_slpc.o \
  gt/uc/intel_guc_submission.o \
+ gt/uc/intel_guc_hwconfig.o \
  gt/uc/intel_huc.o \
  gt/uc/intel_huc_debugfs.o \
  gt/uc/intel_huc_fw.o
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
index fe5d7d261797..4a61c819f32b 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -137,6 +137,7 @@ enum intel_guc_action {
INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004,
INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
+   INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100,
INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502,
INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503,
INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
index 488b6061ee89..f9e2a6aaef4a 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -8,6 +8,10 @@
 
 enum intel_guc_response_status {
INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
+   INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20,
+   INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201,
+   INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202,
+   INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204,
INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
 };
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index f9240d4baa69..ce2ff4bb0fd5 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -13,6 +13,7 @@
 #include "intel_guc_fw.h"
 #include "intel_guc_fwif.h"
 #include "intel_guc_ct.h"
+#include "intel_guc_hwconfig.h"
 #include "intel_guc_log.h"
 #include "intel_guc_reg.h"
 #include "intel_guc_slpc_types.h"
@@ -37,6 +38,8 @@ struct intel_guc {
struct intel_guc_ct ct;
/** @slpc: sub-structure containing SLPC related data and objects */
struct intel_guc_slpc slpc;
+   /** @hwconfig: hardware configuration KLV table */
+   struct intel_guc_hwconfig hwconfig;
 
/** @sched_engine: Global engine used to submit requests to GuC */
struct i915_sched_engine *sched_engine;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
new file mode 100644
index ..ce6088f112d4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "gt/intel_gt.h"
+#include "i915_drv.h"
+#include "i915_memcpy.h"
+#include "intel_guc_hwconfig.h"
+
+static inline struct intel_guc *hwconfig_to_guc(struct intel_guc_hwconfig 
*hwconfig)
+{
+   return container_of(hwconfig, struct intel_guc, hwconfig);
+}
+
+/*
+ * GuC has a blob containing hardware configuration information (HWConfig).
+ * This is formatted as a simple and flexible KLV (Key/Length/Value) table.
+ *
+ * For example, a minimal version could be:
+ *   enum device_attr {
+ * ATTR_SOME_VALUE = 0,
+ * ATTR_SOME_MASK  = 1,
+ *   };
+ *
+ *   static const u32 hwconfig[] = {
+ * ATTR_SOME_VALUE,
+ * 1,  // Value Length in DWords
+ * 8,  // Value
+ *
+ * ATTR_SOME_MASK,
+ * 3,
+ * 0x00, 0x, 0xFF00,
+ *   };
+ *
+ * The attribute ids are defined in a hardware spec.
+ */
+
+static int __guc_action_get_hwconfig(struct intel_guc_hwconfig *hwconfig,
+  

[PATCH v4 0/4] GuC HWCONFIG with documentation

[Jordan Justen]

This is John/Rodrigo's 2 patches with some minor changes, and I added
2 patches.

"drm/i915/uapi: Add query for hwconfig blob" was changed:

 * Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB
   as requested by Joonas.

 * Reword commit message

 * I added Acked-by to this patch, but this only applies in the
   context of this version of the patchset. If my changes are
   rejected, then please *do not* add my Acked-by to the other series.

   In particular, I do not want my Acked-by on the patch if the patch
   mentions the HWCONFIG format, but is not willing to add that to the
   actual uAPI.

   I also do not want my Acked-by on it if it mentions "consolidation"
   of this data. Since we are dealing with open source projects (aside
   from GuC), this doesn't seem appropriate.

"drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a
struct to the uAPI and documents the return value for
DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still
deferred to the PRM.)

"drm/i915/guc: Verify hwconfig blob matches supported format" does the
simple verification of the blob to make sure it matches what the uAPI
documents.

v2:
 * Fix -Werror errors.
 * Rebase to drm-intel/for-linux-next instead of
   drm-intel/for-linux-next-gt, as this seems to be what CI wants.
 * Fix u32 -> __u32.
 * Add commit message for "Verify hwconfig blob" patch as requested by
   Tvrtko.
 * Reword text added to i915_drm.h as requested by Tvrtko. (Attempting
   to indicate the overall blob ends right at the last blob item.)

v3:
 * Add several changes suggested by Tvrtko in the "Verify hwconfig
   blob", along with some tweaks to i915_drm.h from the feedback for
   the same patch.

v4:
 * Rewrite verify_hwconfig_blob() to hopefully be clearer without
   relying on comments so much, and add various suggestions from
   Michal.
 * Michal also had some suggestions in John's "drm/i915/guc: Add fetch
   of hwconfig table" patch. I held off on making any of these changes
   in this version.

John Harrison (1):
  drm/i915/guc: Add fetch of hwconfig table

Jordan Justen (2):
  drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item
  drm/i915/guc: Verify hwconfig blob matches supported format

Rodrigo Vivi (1):
  drm/i915/uapi: Add query for hwconfig blob

 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 193 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 drivers/gpu/drm/i915/i915_query.c |  23 +++
 include/uapi/drm/i915_drm.h   |  36 
 9 files changed, 286 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

-- 
2.34.1

Re: [Intel-gfx] [PATCH 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Jordan Justen]

Tvrtko Ursulin  writes:

> Will GuC folks be reviewing this work?

I don't know. If you mean the i915 devs interfacing with the GuC, I know
John/Rodrigo seemed a bit resistant writing the patches to give
userspace this trivial format guarantee on the blob.

So, I hoped they would write the patches (3 & 4 in my series), but now
I'm hoping they will at least accept the patches.

> Quick sanity check maybe makes sense, given data is being "sent" to 
> userspace directly, I am just not sure if it is worth having in 
> non-debug builds of i915. Though I will agree not having it in 
> production then largely defeats the purpose so dunno.

The check seems fairly trivial, and it seems like i915 should provide
whatever guidance/guarantee is possible to userspace. (Thus, do it once
per boot even on release builds.) See also, the commit message I added.

> Effective difference if GuC load fails versus userspace libraries
> failing to parse hwconfig?

Lots of potential things to consider. Personally, I think for internal
Intel builds, if this check fails, then it ought to cause the GuC to
always fail to load, which today means the device will be wedged.

For external builds, I think it should still load the GuC but not send
the blob to userspace. This is what should happen with the patches in
this series. (I really hope this never happens, which it why I think the
internal builds should be so harsh.)

Now ... if i915 ever regains the ability to drive the device without the
closed source GuC, well... No reason to go off on unrealistic tangents. :)

Also, later down the road for released products, userspace drivers may
choose to bypass the hwconfig to limit the dependence on GuC. That is a
related, but different topic.

-Jordan

[PATCH v3 3/4] drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item

[Jordan Justen]

Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.

v3:
 * Add various changes suggested by Tvrtko

Cc: Daniel Vetter 
Signed-off-by: Jordan Justen 
---
 include/uapi/drm/i915_drm.h | 32 
 1 file changed, 32 insertions(+)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 069d2fadfbd9..c3faee3b3f70 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3276,6 +3276,38 @@ struct drm_i915_gem_create_ext_protected_content {
__u32 flags;
 };
 
+/**
+ * DOC: GuC HWCONFIG blob uAPI
+ *
+ * The GuC produces a blob with information about the current device.
+ * i915 reads this blob from GuC and makes it available via this uAPI.
+ *
+ * The returned blob is a sequence of items of variable length
+ * described by struct drm_i915_query_hwconfig_blob_item. The
+ * drm_i915_query_hwconfig_blob_item length field gives the length of
+ * the drm_i915_query_hwconfig_blob_item data[] array for the item and
+ * thereby determines the length of that item. The key and length
+ * fields are required, so the minimum item size is 2 x __u32, or 8
+ * bytes.
+ *
+ * The overall blob returned by DRM_I915_QUERY_HWCONFIG_BLOB will end
+ * at the same location as the end of the final
+ * drm_i915_query_hwconfig_blob_item. In other words, walking through
+ * the individual items is guaranteed to eventually arrive at the
+ * exact end of the entire blob.
+ *
+ * The meaning of the key field and the data values are documented in
+ * the Programmer's Reference Manual.
+ */
+struct drm_i915_query_hwconfig_blob_item {
+   /** @key: Enum which defines how to interpret @data values. */
+   __u32 key;
+   /** @length: Length of the @data array. */
+   __u32 length;
+   /** @key: Array of values with meaning defined by @key */
+   __u32 data[];
+};
+
 /* ID of the protected content session managed by i915 when PXP is active */
 #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
 
-- 
2.34.1

[PATCH v3 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Jordan Justen]

i915_drm.h now defines the format of the returned
DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from
the black box GuC software, it should verify that the data matches
that format before sending it to user-space.

The verification makes a single simple pass through the blob contents,
so this verification step should not add a significant amount of init
time to i915.

v3:
 * Add various changes suggested by Tvrtko

Signed-off-by: Jordan Justen 
---
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 56 ++-
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
index ce6088f112d4..350a0517b9f0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -71,7 +71,52 @@ static int guc_hwconfig_discover_size(struct 
intel_guc_hwconfig *hwconfig)
return 0;
 }
 
-static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
+static int verify_hwconfig_blob(struct drm_device *drm,
+   const struct intel_guc_hwconfig *hwconfig)
+{
+   struct drm_i915_query_hwconfig_blob_item *pos;
+   u32 remaining;
+
+   if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
+   return -EINVAL;
+
+   pos = hwconfig->ptr;
+   /* The number of dwords in the blob to validate. Each loop
+* pass will process at least 2 dwords corresponding to the
+* key and length fields of the item. In addition, the length
+* field of the item indicates the length of the data array,
+* and that number of dwords will be processed (skipped) as
+* well.
+*/
+   remaining = hwconfig->size / 4;
+
+   while (remaining > 0) {
+   /* Each item requires at least 2 dwords for the key
+* and length fields. If the length field is 0, then
+* the data array would be of length 0.
+*/
+   if (remaining < 2)
+   return -EINVAL;
+   /* remaining >= 2, so subtracting 2 is ok, whereas
+* adding 2 to pos->length could overflow.
+*/
+   if (pos->length > remaining - 2)
+   return -EINVAL;
+   /* The length check above ensures that the adjustment
+* of the remaining variable will not underflow, and
+* that the adjustment of the pos variable will not
+* pass the end of the blob data.
+*/
+   remaining -= 2 + pos->length;
+   pos = (void *)>data[pos->length];
+   }
+
+   drm_dbg(drm, "hwconfig blob format is valid\n");
+   return 0;
+}
+
+static int guc_hwconfig_fill_buffer(struct drm_device *drm,
+   struct intel_guc_hwconfig *hwconfig)
 {
struct intel_guc *guc = hwconfig_to_guc(hwconfig);
struct i915_vma *vma;
@@ -88,8 +133,13 @@ static int guc_hwconfig_fill_buffer(struct 
intel_guc_hwconfig *hwconfig)
ggtt_offset = intel_guc_ggtt_offset(guc, vma);
 
ret = __guc_action_get_hwconfig(hwconfig, ggtt_offset, hwconfig->size);
-   if (ret >= 0)
+   if (ret >= 0) {
memcpy(hwconfig->ptr, vaddr, hwconfig->size);
+   if (verify_hwconfig_blob(drm, hwconfig)) {
+   drm_err(drm, "Ignoring invalid hwconfig blob received 
from GuC!\n");
+   ret = -EINVAL;
+   }
+   }
 
i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP);
 
@@ -141,7 +191,7 @@ int intel_guc_hwconfig_init(struct intel_guc_hwconfig 
*hwconfig)
return -ENOMEM;
}
 
-   ret = guc_hwconfig_fill_buffer(hwconfig);
+   ret = guc_hwconfig_fill_buffer(>drm, hwconfig);
if (ret < 0) {
intel_guc_hwconfig_fini(hwconfig);
return ret;
-- 
2.34.1

[PATCH v3 2/4] drm/i915/uapi: Add query for hwconfig blob

[Jordan Justen]

From: Rodrigo Vivi 

The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data
which it receives from the GuC software. This blob provides some
useful data about the hardware for drivers.

Although the blob is not fully documented at this time, the basic
format is an array of u32 values. The array is a simple and flexible
KLV (Key/Length/Value) formatted table. For example, it could be just:
enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, };

  static const u32 hwconfig[] = {
  ATTR_SOME_VALUE,
  1, // Value Length in DWords
  8, // Value

  ATTR_SOME_MASK,
  3,
  0x00, 0x, 0xFF00,
  };

The attribute ids and meaning of the values will be documented in the
Programmer Reference Manuals when released.

Cc: Tvrtko Ursulin 
Cc: Kenneth Graunke 
Cc: Michal Wajdeczko 
Cc: Slawomir Milczarek 
Cc: Joonas Lahtinen 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jordan Justen 
Tested-by: Jordan Justen 
---
 drivers/gpu/drm/i915/i915_query.c | 23 +++
 include/uapi/drm/i915_drm.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..195524e9a369 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
*i915,
return total_length;
 }
 
+static int query_hwconfig_blob(struct drm_i915_private *i915,
+  struct drm_i915_query_item *query_item)
+{
+   struct intel_gt *gt = to_gt(i915);
+   struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
+
+   if (!hwconfig->size || !hwconfig->ptr)
+   return -ENODEV;
+
+   if (query_item->length == 0)
+   return hwconfig->size;
+
+   if (query_item->length < hwconfig->size)
+   return -EINVAL;
+
+   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+   return -EFAULT;
+
+   return hwconfig->size;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) 
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+   query_hwconfig_blob,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..069d2fadfbd9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
 #define DRM_I915_QUERY_ENGINE_INFO 2
 #define DRM_I915_QUERY_PERF_CONFIG  3
 #define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_HWCONFIG_BLOB   5
 /* Must be kept compact -- no holes and well documented */
 
/**
-- 
2.34.1

[PATCH v3 0/4] GuC HWCONFIG with documentation

[Jordan Justen]

This is John/Rodrigo's 2 patches with some minor changes, and I added
2 patches.

"drm/i915/uapi: Add query for hwconfig blob" was changed:

 * Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB
   as requested by Joonas.

 * Reword commit message

 * I added Acked-by to this patch, but this only applies in the
   context of this version of the patchset. If my changes are
   rejected, then please *do not* add my Acked-by to the other series.

   In particular, I do not want my Acked-by on the patch if the patch
   mentions the HWCONFIG format, but is not willing to add that to the
   actual uAPI.

   I also do not want my Acked-by on it if it mentions "consolidation"
   of this data. Since we are dealing with open source projects (aside
   from GuC), this doesn't seem appropriate.

"drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a
struct to the uAPI and documents the return value for
DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still
deferred to the PRM.)

"drm/i915/guc: Verify hwconfig blob matches supported format" does the
simple verification of the blob to make sure it matches what the uAPI
documents.

v2:
 * Fix -Werror errors.
 * Rebase to drm-intel/for-linux-next instead of
   drm-intel/for-linux-next-gt, as this seems to be what CI wants.
 * Fix u32 -> __u32. (Sorry, I was first testing in Mesa tree.)
 * Add commit message for "Verify hwconfig blob" patch as requested by
   Tvrtko.
 * Reword text added to i915_drm.h as requested by Tvrtko. (Attempting
   to indicate the overall blob ends right at the last blob item.)

v3:
 * Add several changes suggested by Tvrtko in the "Verify hwconfig
   blob", along with some tweaks to i915_drm.h from the feedback for
   the same patch.

John Harrison (1):
  drm/i915/guc: Add fetch of hwconfig table

Jordan Justen (2):
  drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item
  drm/i915/guc: Verify hwconfig blob matches supported format

Rodrigo Vivi (1):
  drm/i915/uapi: Add query for hwconfig blob

 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 201 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 drivers/gpu/drm/i915/i915_query.c |  23 ++
 include/uapi/drm/i915_drm.h   |  33 +++
 9 files changed, 291 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

-- 
2.34.1

[PATCH v3 1/4] drm/i915/guc: Add fetch of hwconfig table

[Jordan Justen]

From: John Harrison 

Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.

Note that the table is only available on ADL-P and later platforms.

Cc: Michal Wajdeczko 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 151 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 +++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 7 files changed, 185 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 6836b020a5be..ba9b6557d59d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -192,6 +192,7 @@ i915-y += gt/uc/intel_uc.o \
  gt/uc/intel_guc_rc.o \
  gt/uc/intel_guc_slpc.o \
  gt/uc/intel_guc_submission.o \
+ gt/uc/intel_guc_hwconfig.o \
  gt/uc/intel_huc.o \
  gt/uc/intel_huc_debugfs.o \
  gt/uc/intel_huc_fw.o
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
index fe5d7d261797..4a61c819f32b 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -137,6 +137,7 @@ enum intel_guc_action {
INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004,
INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
+   INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100,
INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502,
INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503,
INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
index 488b6061ee89..f9e2a6aaef4a 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -8,6 +8,10 @@
 
 enum intel_guc_response_status {
INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
+   INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20,
+   INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201,
+   INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202,
+   INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204,
INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
 };
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index f9240d4baa69..ce2ff4bb0fd5 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -13,6 +13,7 @@
 #include "intel_guc_fw.h"
 #include "intel_guc_fwif.h"
 #include "intel_guc_ct.h"
+#include "intel_guc_hwconfig.h"
 #include "intel_guc_log.h"
 #include "intel_guc_reg.h"
 #include "intel_guc_slpc_types.h"
@@ -37,6 +38,8 @@ struct intel_guc {
struct intel_guc_ct ct;
/** @slpc: sub-structure containing SLPC related data and objects */
struct intel_guc_slpc slpc;
+   /** @hwconfig: hardware configuration KLV table */
+   struct intel_guc_hwconfig hwconfig;
 
/** @sched_engine: Global engine used to submit requests to GuC */
struct i915_sched_engine *sched_engine;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
new file mode 100644
index ..ce6088f112d4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "gt/intel_gt.h"
+#include "i915_drv.h"
+#include "i915_memcpy.h"
+#include "intel_guc_hwconfig.h"
+
+static inline struct intel_guc *hwconfig_to_guc(struct intel_guc_hwconfig 
*hwconfig)
+{
+   return container_of(hwconfig, struct intel_guc, hwconfig);
+}
+
+/*
+ * GuC has a blob containing hardware configuration information (HWConfig).
+ * This is formatted as a simple and flexible KLV (Key/Length/Value) table.
+ *
+ * For example, a minimal version could be:
+ *   enum device_attr {
+ * ATTR_SOME_VALUE = 0,
+ * ATTR_SOME_MASK  = 1,
+ *   };
+ *
+ *   static const u32 hwconfig[] = {
+ * ATTR_SOME_VALUE,
+ * 1,  // Value Length in DWords
+ * 8,  // Value
+ *
+ * ATTR_SOME_MASK,
+ * 3,
+ * 0x00, 0x, 0xFF00,
+ *   };
+ *
+ * The attribute ids are defined in a hardware spec.
+ */
+
+static int __guc_action_get_hwconfig(struct intel_guc_hwconfig *hwconfig,
+u32 

[PATCH v2 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Jordan Justen]

i915_drm.h now defines the format of the returned
DRM_I915_QUERY_HWCONFIG_BLOB query item. Since i915 receives this from
the black box GuC software, it should verify that the data matches
that format before sending it to user-space.

The verification makes a single simple pass through the blob contents,
so this verification step should not add a significant amount of init
time to i915.

Signed-off-by: Jordan Justen 
---
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 29 +++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
index ce6088f112d4..6208246d4209 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -71,6 +71,29 @@ static int guc_hwconfig_discover_size(struct 
intel_guc_hwconfig *hwconfig)
return 0;
 }
 
+static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
+{
+   struct drm_i915_query_hwconfig_blob_item *pos;
+   u32 remaining;
+
+   if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
+   return -EINVAL;
+
+   pos = hwconfig->ptr;
+   remaining = (hwconfig->size / 4);
+   while (remaining > 0) {
+   if (remaining < 2)
+   return -EINVAL;
+   if (pos->length > remaining - 2)
+   return -EINVAL;
+   remaining -= 2 + pos->length;
+   pos = (void *)>data[pos->length];
+   }
+
+   DRM_INFO("hwconfig blob format appears valid\n");
+   return 0;
+}
+
 static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
 {
struct intel_guc *guc = hwconfig_to_guc(hwconfig);
@@ -91,6 +114,12 @@ static int guc_hwconfig_fill_buffer(struct 
intel_guc_hwconfig *hwconfig)
if (ret >= 0)
memcpy(hwconfig->ptr, vaddr, hwconfig->size);
 
+   if (verify_hwconfig_blob(hwconfig)) {
+   DRM_ERROR("Ignoring invalid hwconfig blob received from "
+ "GuC!\n");
+   return -EINVAL;
+   }
+
i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP);
 
return ret;
-- 
2.34.1

[PATCH v2 2/4] drm/i915/uapi: Add query for hwconfig blob

[Jordan Justen]

From: Rodrigo Vivi 

The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data
which it receives from the GuC software. This blob provides some
useful data about the hardware for drivers.

Although the blob is not fully documented at this time, the basic
format is an array of u32 values. The array is a simple and flexible
KLV (Key/Length/Value) formatted table. For example, it could be just:
enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, };

  static const u32 hwconfig[] = {
  ATTR_SOME_VALUE,
  1, // Value Length in DWords
  8, // Value

  ATTR_SOME_MASK,
  3,
  0x00, 0x, 0xFF00,
  };

The attribute ids and meaning of the values will be documented in the
Programmer Reference Manuals when released.

Cc: Tvrtko Ursulin 
Cc: Kenneth Graunke 
Cc: Michal Wajdeczko 
Cc: Slawomir Milczarek 
Cc: Joonas Lahtinen 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jordan Justen 
Tested-by: Jordan Justen 
---
 drivers/gpu/drm/i915/i915_query.c | 23 +++
 include/uapi/drm/i915_drm.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..195524e9a369 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
*i915,
return total_length;
 }
 
+static int query_hwconfig_blob(struct drm_i915_private *i915,
+  struct drm_i915_query_item *query_item)
+{
+   struct intel_gt *gt = to_gt(i915);
+   struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
+
+   if (!hwconfig->size || !hwconfig->ptr)
+   return -ENODEV;
+
+   if (query_item->length == 0)
+   return hwconfig->size;
+
+   if (query_item->length < hwconfig->size)
+   return -EINVAL;
+
+   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+   return -EFAULT;
+
+   return hwconfig->size;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) 
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+   query_hwconfig_blob,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..069d2fadfbd9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
 #define DRM_I915_QUERY_ENGINE_INFO 2
 #define DRM_I915_QUERY_PERF_CONFIG  3
 #define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_HWCONFIG_BLOB   5
 /* Must be kept compact -- no holes and well documented */
 
/**
-- 
2.34.1

[PATCH v2 1/4] drm/i915/guc: Add fetch of hwconfig table

[Jordan Justen]

From: John Harrison 

Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.

Note that the table is only available on ADL-P and later platforms.

Cc: Michal Wajdeczko 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 151 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 +++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 7 files changed, 185 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 6836b020a5be..ba9b6557d59d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -192,6 +192,7 @@ i915-y += gt/uc/intel_uc.o \
  gt/uc/intel_guc_rc.o \
  gt/uc/intel_guc_slpc.o \
  gt/uc/intel_guc_submission.o \
+ gt/uc/intel_guc_hwconfig.o \
  gt/uc/intel_huc.o \
  gt/uc/intel_huc_debugfs.o \
  gt/uc/intel_huc_fw.o
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
index fe5d7d261797..4a61c819f32b 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -137,6 +137,7 @@ enum intel_guc_action {
INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004,
INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
+   INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100,
INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502,
INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503,
INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
index 488b6061ee89..f9e2a6aaef4a 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -8,6 +8,10 @@
 
 enum intel_guc_response_status {
INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
+   INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20,
+   INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201,
+   INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202,
+   INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204,
INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
 };
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index f9240d4baa69..ce2ff4bb0fd5 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -13,6 +13,7 @@
 #include "intel_guc_fw.h"
 #include "intel_guc_fwif.h"
 #include "intel_guc_ct.h"
+#include "intel_guc_hwconfig.h"
 #include "intel_guc_log.h"
 #include "intel_guc_reg.h"
 #include "intel_guc_slpc_types.h"
@@ -37,6 +38,8 @@ struct intel_guc {
struct intel_guc_ct ct;
/** @slpc: sub-structure containing SLPC related data and objects */
struct intel_guc_slpc slpc;
+   /** @hwconfig: hardware configuration KLV table */
+   struct intel_guc_hwconfig hwconfig;
 
/** @sched_engine: Global engine used to submit requests to GuC */
struct i915_sched_engine *sched_engine;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
new file mode 100644
index ..ce6088f112d4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "gt/intel_gt.h"
+#include "i915_drv.h"
+#include "i915_memcpy.h"
+#include "intel_guc_hwconfig.h"
+
+static inline struct intel_guc *hwconfig_to_guc(struct intel_guc_hwconfig 
*hwconfig)
+{
+   return container_of(hwconfig, struct intel_guc, hwconfig);
+}
+
+/*
+ * GuC has a blob containing hardware configuration information (HWConfig).
+ * This is formatted as a simple and flexible KLV (Key/Length/Value) table.
+ *
+ * For example, a minimal version could be:
+ *   enum device_attr {
+ * ATTR_SOME_VALUE = 0,
+ * ATTR_SOME_MASK  = 1,
+ *   };
+ *
+ *   static const u32 hwconfig[] = {
+ * ATTR_SOME_VALUE,
+ * 1,  // Value Length in DWords
+ * 8,  // Value
+ *
+ * ATTR_SOME_MASK,
+ * 3,
+ * 0x00, 0x, 0xFF00,
+ *   };
+ *
+ * The attribute ids are defined in a hardware spec.
+ */
+
+static int __guc_action_get_hwconfig(struct intel_guc_hwconfig *hwconfig,
+u32 

[PATCH v2 3/4] drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item

[Jordan Justen]

Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.

Cc: Daniel Vetter 
Signed-off-by: Jordan Justen 
---
 include/uapi/drm/i915_drm.h | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 069d2fadfbd9..280e7e2e04d8 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3276,6 +3276,36 @@ struct drm_i915_gem_create_ext_protected_content {
__u32 flags;
 };
 
+/**
+ * DOC: GuC HWCONFIG blob uAPI
+ *
+ * The GuC produces a blob with information about the current device.
+ * i915 reads this blob from GuC and makes it available via this uAPI.
+ *
+ * The returned blob is an sequence of items of variable length
+ * described by struct drm_i915_query_hwconfig_blob_item. The
+ * drm_i915_query_hwconfig_blob_item length field gives the length of
+ * the drm_i915_query_hwconfig_blob_item data[] array for the item and
+ * thereby determines the length of that item.
+ *
+ * The overall blob returned by DRM_I915_QUERY_HWCONFIG_BLOB will end
+ * at the same location as the end of the final
+ * drm_i915_query_hwconfig_blob_item. In other words, walking through
+ * the individual items is guaranteed to eventually arrive at the
+ * exact end of the entire blob.
+ *
+ * The meaning of the key field and the data values are documented in
+ * the Programmer's Reference Manual.
+ */
+struct drm_i915_query_hwconfig_blob_item {
+   /** @key: Enum which defines how to interpret @data values. */
+   __u32 key;
+   /** @length: Length of the @data array. */
+   __u32 length;
+   /** @key: Array of values with meaning defined by @key */
+   __u32 data[];
+};
+
 /* ID of the protected content session managed by i915 when PXP is active */
 #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
 
-- 
2.34.1

[PATCH v2 0/4] GuC HWCONFIG with documentation

[Jordan Justen]

This is John/Rodrigo's 2 patches with some minor changes, and I added
2 patches.

"drm/i915/uapi: Add query for hwconfig blob" was changed:

 * Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB
   as requested by Joonas.

 * Reword commit message

 * I added Acked-by to this patch, but this only applies in the
   context of this version of the patchset. If my changes are
   rejected, then please *do not* add my Acked-by to the other series.

   In particular, I do not want my Acked-by on the patch if the patch
   mentions the HWCONFIG format, but is not willing to add that to the
   actual uAPI.

   I also do not want my Acked-by on it if it mentions "consolidation"
   of this data. Since we are dealing with open source projects (aside
   from GuC), this doesn't seem appropriate.

"drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a
struct to the uAPI and documents the return value for
DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still
deferred to the PRM.)

"drm/i915/guc: Verify hwconfig blob matches supported format" does the
simple verification of the blob to make sure it matches what the uAPI
documents.

v2:
 * Fix -Werror errors.
 * Rebase to drm-intel/for-linux-next instead of
   drm-intel/for-linux-next-gt, as this seems to be what CI wants.
 * Fix u32 -> __u32. (Sorry, I was first testing in Mesa tree.)
 * Add commit message for "Verify hwconfig blob" patch as requested by
   Tvrtko.
 * Reword text added to i915_drm.h as requested by Tvrtko. (Attempting
   to indicate the overall blob ends right at the last blob item.)

John Harrison (1):
  drm/i915/guc: Add fetch of hwconfig table

Jordan Justen (2):
  drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item
  drm/i915/guc: Verify hwconfig blob matches supported format

Rodrigo Vivi (1):
  drm/i915/uapi: Add query for hwconfig blob

 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 180 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 drivers/gpu/drm/i915/i915_query.c |  23 +++
 include/uapi/drm/i915_drm.h   |  31 +++
 9 files changed, 268 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

-- 
2.34.1

[PATCH 4/4] drm/i915/guc: Verify hwconfig blob matches supported format

[Jordan Justen]

Signed-off-by: Jordan Justen 
---
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 26 +++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
index ce6088f112d4..695ef7a8f519 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -71,6 +71,26 @@ static int guc_hwconfig_discover_size(struct 
intel_guc_hwconfig *hwconfig)
return 0;
 }
 
+static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
+{
+   if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
+   return -EINVAL;
+
+   struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
+   u32 remaining = (hwconfig->size / 4);
+   while (remaining > 0) {
+   if (remaining < 2)
+   return -EINVAL;
+   if (pos->length > remaining - 2)
+   return -EINVAL;
+   remaining -= 2 + pos->length;
+   pos = (void *)>data[pos->length];
+   }
+
+   DRM_INFO("hwconfig blob format appears valid\n");
+   return 0;
+}
+
 static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
 {
struct intel_guc *guc = hwconfig_to_guc(hwconfig);
@@ -91,6 +111,12 @@ static int guc_hwconfig_fill_buffer(struct 
intel_guc_hwconfig *hwconfig)
if (ret >= 0)
memcpy(hwconfig->ptr, vaddr, hwconfig->size);
 
+   if (verify_hwconfig_blob(hwconfig)) {
+   DRM_ERROR("Ignoring invalid hwconfig blob received from "
+ "GuC!\n");
+   return -EINVAL;
+   }
+
i915_vma_unpin_and_release(, I915_VMA_RELEASE_MAP);
 
return ret;
-- 
2.34.1

[PATCH 3/4] drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item

[Jordan Justen]

Also, document DRM_I915_QUERY_HWCONFIG_BLOB with this struct.

Cc: Daniel Vetter 
Signed-off-by: Jordan Justen 
---
 include/uapi/drm/i915_drm.h | 24 
 1 file changed, 24 insertions(+)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 069d2fadfbd9..38b8c11e91f0 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3276,6 +3276,30 @@ struct drm_i915_gem_create_ext_protected_content {
__u32 flags;
 };
 
+/**
+ * DOC: GuC HWCONFIG blob uAPI
+ *
+ * The GuC produces a blob with information about the current device.
+ * i915 reads this blob from GuC and makes it available via this uAPI.
+ *
+ * The returned blob is an array of items described by struct
+ * drm_i915_query_hwconfig_blob_item. The
+ * drm_i915_query_hwconfig_blob_item length field gives the length of
+ * the drm_i915_query_hwconfig_blob_item data[] array for the item.
+ *
+ * The length of the query data returned by
+ * DRM_I915_QUERY_HWCONFIG_BLOB will align with the end at the final
+ * drm_i915_query_hwconfig_blob_item entry.
+ *
+ * The meaning of the key field and the data values are documented in
+ * the Programmer's Reference Manual.
+ */
+struct drm_i915_query_hwconfig_blob_item {
+   u32 key;
+   u32 length;
+   u32 data[];
+};
+
 /* ID of the protected content session managed by i915 when PXP is active */
 #define I915_PROTECTED_CONTENT_DEFAULT_SESSION 0xf
 
-- 
2.34.1

[PATCH 1/4] drm/i915/guc: Add fetch of hwconfig table

[Jordan Justen]

From: John Harrison 

Implement support for fetching the hardware description table from the
GuC. The call is made twice - once without a destination buffer to
query the size and then a second time to fill in the buffer.

Note that the table is only available on ADL-P and later platforms.

Cc: Michal Wajdeczko 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 151 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 +++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 7 files changed, 185 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 451df10e3a36..f6e4a699495e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -190,6 +190,7 @@ i915-y += gt/uc/intel_uc.o \
  gt/uc/intel_guc_rc.o \
  gt/uc/intel_guc_slpc.o \
  gt/uc/intel_guc_submission.o \
+ gt/uc/intel_guc_hwconfig.o \
  gt/uc/intel_huc.o \
  gt/uc/intel_huc_debugfs.o \
  gt/uc/intel_huc_fw.o
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
index 7afdadc7656f..a9a329e53c35 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -129,6 +129,7 @@ enum intel_guc_action {
INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
INTEL_GUC_ACTION_SETUP_PC_GUCRC = 0x3004,
INTEL_GUC_ACTION_AUTHENTICATE_HUC = 0x4000,
+   INTEL_GUC_ACTION_GET_HWCONFIG = 0x4100,
INTEL_GUC_ACTION_REGISTER_CONTEXT = 0x4502,
INTEL_GUC_ACTION_DEREGISTER_CONTEXT = 0x4503,
INTEL_GUC_ACTION_REGISTER_COMMAND_TRANSPORT_BUFFER = 0x4505,
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h 
b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
index c20658ee85a5..8085fb181274 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -8,6 +8,10 @@
 
 enum intel_guc_response_status {
INTEL_GUC_RESPONSE_STATUS_SUCCESS = 0x0,
+   INTEL_GUC_RESPONSE_NOT_SUPPORTED = 0x20,
+   INTEL_GUC_RESPONSE_NO_ATTRIBUTE_TABLE = 0x201,
+   INTEL_GUC_RESPONSE_NO_DECRYPTION_KEY = 0x202,
+   INTEL_GUC_RESPONSE_DECRYPTION_FAILED = 0x204,
INTEL_GUC_RESPONSE_STATUS_GENERIC_FAIL = 0xF000,
 };
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 697d9d66acef..309bc8d5447b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -13,6 +13,7 @@
 #include "intel_guc_fw.h"
 #include "intel_guc_fwif.h"
 #include "intel_guc_ct.h"
+#include "intel_guc_hwconfig.h"
 #include "intel_guc_log.h"
 #include "intel_guc_reg.h"
 #include "intel_guc_slpc_types.h"
@@ -37,6 +38,8 @@ struct intel_guc {
struct intel_guc_ct ct;
/** @slpc: sub-structure containing SLPC related data and objects */
struct intel_guc_slpc slpc;
+   /** @hwconfig: hardware configuration KLV table */
+   struct intel_guc_hwconfig hwconfig;
 
/** @sched_engine: Global engine used to submit requests to GuC */
struct i915_sched_engine *sched_engine;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
new file mode 100644
index ..ce6088f112d4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "gt/intel_gt.h"
+#include "i915_drv.h"
+#include "i915_memcpy.h"
+#include "intel_guc_hwconfig.h"
+
+static inline struct intel_guc *hwconfig_to_guc(struct intel_guc_hwconfig 
*hwconfig)
+{
+   return container_of(hwconfig, struct intel_guc, hwconfig);
+}
+
+/*
+ * GuC has a blob containing hardware configuration information (HWConfig).
+ * This is formatted as a simple and flexible KLV (Key/Length/Value) table.
+ *
+ * For example, a minimal version could be:
+ *   enum device_attr {
+ * ATTR_SOME_VALUE = 0,
+ * ATTR_SOME_MASK  = 1,
+ *   };
+ *
+ *   static const u32 hwconfig[] = {
+ * ATTR_SOME_VALUE,
+ * 1,  // Value Length in DWords
+ * 8,  // Value
+ *
+ * ATTR_SOME_MASK,
+ * 3,
+ * 0x00, 0x, 0xFF00,
+ *   };
+ *
+ * The attribute ids are defined in a hardware spec.
+ */
+
+static int __guc_action_get_hwconfig(struct intel_guc_hwconfig *hwconfig,
+u32 

[PATCH 2/4] drm/i915/uapi: Add query for hwconfig blob

[Jordan Justen]

From: Rodrigo Vivi 

The DRM_I915_QUERY_HWCONFIG_BLOB query item returns a blob of data
which it receives from the GuC software. This blob provides some
useful data about the hardware for drivers.

Although the blob is not fully documented at this time, the basic
format is an array of u32 values. The array is a simple and flexible
KLV (Key/Length/Value) formatted table. For example, it could be just:
enum device_attr { ATTR_SOME_VALUE = 0, ATTR_SOME_MASK = 1, };

  static const u32 hwconfig[] = {
  ATTR_SOME_VALUE,
  1, // Value Length in DWords
  8, // Value

  ATTR_SOME_MASK,
  3,
  0x00, 0x, 0xFF00,
  };

The attribute ids and meaning of the values will be documented in the
Programmer Reference Manuals when released.

Cc: Tvrtko Ursulin 
Cc: Kenneth Graunke 
Cc: Michal Wajdeczko 
Cc: Slawomir Milczarek 
Cc: Joonas Lahtinen 
Signed-off-by: Rodrigo Vivi 
Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
Acked-by: Jordan Justen 
Tested-by: Jordan Justen 
---
 drivers/gpu/drm/i915/i915_query.c | 23 +++
 include/uapi/drm/i915_drm.h   |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_query.c 
b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..195524e9a369 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
*i915,
return total_length;
 }
 
+static int query_hwconfig_blob(struct drm_i915_private *i915,
+  struct drm_i915_query_item *query_item)
+{
+   struct intel_gt *gt = to_gt(i915);
+   struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
+
+   if (!hwconfig->size || !hwconfig->ptr)
+   return -ENODEV;
+
+   if (query_item->length == 0)
+   return hwconfig->size;
+
+   if (query_item->length < hwconfig->size)
+   return -EINVAL;
+
+   if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+hwconfig->ptr, hwconfig->size))
+   return -EFAULT;
+
+   return hwconfig->size;
+}
+
 static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) 
= {
query_topology_info,
query_engine_info,
query_perf_config,
query_memregion_info,
+   query_hwconfig_blob,
 };
 
 int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 914ebd9290e5..069d2fadfbd9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
 #define DRM_I915_QUERY_ENGINE_INFO 2
 #define DRM_I915_QUERY_PERF_CONFIG  3
 #define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_HWCONFIG_BLOB   5
 /* Must be kept compact -- no holes and well documented */
 
/**
-- 
2.34.1

[PATCH 0/4] GuC HWCONFIG with documentation

[Jordan Justen]

This is John/Rodrigo's 2 patches with some minor changes, and I added
2 patches.

"drm/i915/uapi: Add query for hwconfig blob" was changed:

 * Rename DRM_I915_QUERY_HWCONFIG_TABLE to DRM_I915_QUERY_HWCONFIG_BLOB
   as requested by Joonas.

 * Reword commit message

 * I added Acked-by to this patch, but this only applies in the
   context of this version of the patchset. If my changes are
   rejected, then please *do not* add my Acked-by to the other series.

   In particular, I do not want my Acked-by on the patch if the patch
   mentions the HWCONFIG format, but is not willing to add that to the
   actual uAPI.

   I also do not want my Acked-by on it if it mentions "consolidation"
   of this data. Since we are dealing with open source projects (aside
   from GuC), this doesn't seem appropriate.

"drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item" adds a
struct to the uAPI and documents the return value for
DRM_I915_QUERY_HWCONFIG_BLOB. (Except, keys / values are still
deferred to the PRM.)

"drm/i915/guc: Verify hwconfig blob matches supported format" does the
simple verification of the blob to make sure it matches what the uAPI
documents.

John Harrison (1):
  drm/i915/guc: Add fetch of hwconfig table

Jordan Justen (2):
  drm/i915/uapi: Add struct drm_i915_query_hwconfig_blob_item
  drm/i915/guc: Verify hwconfig blob matches supported format

Rodrigo Vivi (1):
  drm/i915/uapi: Add query for hwconfig blob

 drivers/gpu/drm/i915/Makefile |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h  |   1 +
 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   4 +
 drivers/gpu/drm/i915/gt/uc/intel_guc.h|   3 +
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 177 ++
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.h   |  19 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c |   6 +
 drivers/gpu/drm/i915/i915_query.c |  23 +++
 include/uapi/drm/i915_drm.h   |  25 +++
 9 files changed, 259 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.h

-- 
2.34.1

Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

Jordan Justen  writes:

> John, Rodrigo,
>
> It is now clear to me just how dependent i915 is going to be on the
> closed source guc software, and that's just a fact of life for our
> graphics stack going forward.
>
> In that context, it seems kind of pointless for me to make a big deal
> out of this peripheral "query item" commit message. I still think
> something as simple and to the point as:
>
> "In this interface i915 is returning a blob of data which it receives
> from the guc software. This blob provides some useful data about the
> hardware for drivers. The format of this blob will be documented in the
> Programmer Reference Manuals when released."

As I said on the internal email thread, *if you use my commit message
suggestion*, then, begrudgingly, you can add my:

Acked-by: Jordan Justen 

to the patch.

Regardless of the commit message, I think you can add:

Tested-by: Jordan Justen 

In truth, I've only tested this via the "prelim" i915 Linux uapi fork on
an internal kernel tree, but I think that probably is close enough.

In case you find it helpful, maybe:

Ref: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14511

-Jordan

>
> ... would be better, but obviously this is really just down in the noise
> in terms of concerns about the greater issue. So, feel free (to
> continue) to ignore my suggestion.
>
> Sorry for having wasted your time,
>
> -Jordan
>
> john.c.harri...@intel.com writes:
>
>> From: Rodrigo Vivi 
>>
>> GuC contains a consolidated table with a bunch of information about the
>> current device.
>>
>> Previously, this information was spread and hardcoded to all the components
>> including GuC, i915 and various UMDs. The goal here is to consolidate
>> the data into GuC in a way that all interested components can grab the
>> very latest and synchronized information using a simple query.
>>
>> As per most of the other queries, this one can be called twice.
>> Once with item.length=0 to determine the exact buffer size, then
>> allocate the user memory and call it again for to retrieve the
>> table data. For example:
>>   struct drm_i915_query_item item = {
>> .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>>   };
>>   query.items_ptr = (int64_t) 
>>   query.num_items = 1;
>>
>>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>
>>   if (item.length <= 0)
>> return -ENOENT;
>>
>>   data = malloc(item.length);
>>   item.data_ptr = (int64_t) 
>>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>
>>   // Parse the data as appropriate...
>>
>> The returned array is a simple and flexible KLV (Key/Length/Value)
>> formatted table. For example, it could be just:
>>   enum device_attr {
>>  ATTR_SOME_VALUE = 0,
>>  ATTR_SOME_MASK  = 1,
>>   };
>>
>>   static const u32 hwconfig[] = {
>>   ATTR_SOME_VALUE,
>>   1, // Value Length in DWords
>>   8, // Value
>>
>>   ATTR_SOME_MASK,
>>   3,
>>   0x00, 0x, 0xFF00,
>>   };
>>
>> The attribute ids are defined in a hardware spec.
>>
>> Cc: Tvrtko Ursulin 
>> Cc: Kenneth Graunke 
>> Cc: Michal Wajdeczko 
>> Cc: Slawomir Milczarek 
>> Signed-off-by: Rodrigo Vivi 
>> Signed-off-by: John Harrison 
>> Reviewed-by: Matthew Brost 
>> ---
>>  drivers/gpu/drm/i915/i915_query.c | 23 +++
>>  include/uapi/drm/i915_drm.h   |  1 +
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>> b/drivers/gpu/drm/i915/i915_query.c
>> index 2dfbc22857a3..609e64d5f395 100644
>> --- a/drivers/gpu/drm/i915/i915_query.c
>> +++ b/drivers/gpu/drm/i915/i915_query.c
>> @@ -479,12 +479,35 @@ static int query_memregion_info(struct 
>> drm_i915_private *i915,
>>  return total_length;
>>  }
>>  
>> +static int query_hwconfig_table(struct drm_i915_private *i915,
>> +struct drm_i915_query_item *query_item)
>> +{
>> +struct intel_gt *gt = to_gt(i915);
>> +struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
>> +
>> +if (!hwconfig->size || !hwconfig->ptr)
>> +return -ENODEV;
>> +
>> +if (query_item->length == 0)
>> +return hwconfig->size;
>> +
>> +if (query_item->length < hwconfig->size)
>> +return -EINVAL;
>> +
>> +if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),

Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

John, Rodrigo,

It is now clear to me just how dependent i915 is going to be on the
closed source guc software, and that's just a fact of life for our
graphics stack going forward.

In that context, it seems kind of pointless for me to make a big deal
out of this peripheral "query item" commit message. I still think
something as simple and to the point as:

"In this interface i915 is returning a blob of data which it receives
from the guc software. This blob provides some useful data about the
hardware for drivers. The format of this blob will be documented in the
Programmer Reference Manuals when released."

... would be better, but obviously this is really just down in the noise
in terms of concerns about the greater issue. So, feel free (to
continue) to ignore my suggestion.

Sorry for having wasted your time,

-Jordan

john.c.harri...@intel.com writes:

> From: Rodrigo Vivi 
>
> GuC contains a consolidated table with a bunch of information about the
> current device.
>
> Previously, this information was spread and hardcoded to all the components
> including GuC, i915 and various UMDs. The goal here is to consolidate
> the data into GuC in a way that all interested components can grab the
> very latest and synchronized information using a simple query.
>
> As per most of the other queries, this one can be called twice.
> Once with item.length=0 to determine the exact buffer size, then
> allocate the user memory and call it again for to retrieve the
> table data. For example:
>   struct drm_i915_query_item item = {
> .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>   };
>   query.items_ptr = (int64_t) 
>   query.num_items = 1;
>
>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>
>   if (item.length <= 0)
> return -ENOENT;
>
>   data = malloc(item.length);
>   item.data_ptr = (int64_t) 
>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>
>   // Parse the data as appropriate...
>
> The returned array is a simple and flexible KLV (Key/Length/Value)
> formatted table. For example, it could be just:
>   enum device_attr {
>  ATTR_SOME_VALUE = 0,
>  ATTR_SOME_MASK  = 1,
>   };
>
>   static const u32 hwconfig[] = {
>   ATTR_SOME_VALUE,
>   1, // Value Length in DWords
>   8, // Value
>
>   ATTR_SOME_MASK,
>   3,
>   0x00, 0x, 0xFF00,
>   };
>
> The attribute ids are defined in a hardware spec.
>
> Cc: Tvrtko Ursulin 
> Cc: Kenneth Graunke 
> Cc: Michal Wajdeczko 
> Cc: Slawomir Milczarek 
> Signed-off-by: Rodrigo Vivi 
> Signed-off-by: John Harrison 
> Reviewed-by: Matthew Brost 
> ---
>  drivers/gpu/drm/i915/i915_query.c | 23 +++
>  include/uapi/drm/i915_drm.h   |  1 +
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c 
> b/drivers/gpu/drm/i915/i915_query.c
> index 2dfbc22857a3..609e64d5f395 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
> *i915,
>   return total_length;
>  }
>  
> +static int query_hwconfig_table(struct drm_i915_private *i915,
> + struct drm_i915_query_item *query_item)
> +{
> + struct intel_gt *gt = to_gt(i915);
> + struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
> +
> + if (!hwconfig->size || !hwconfig->ptr)
> + return -ENODEV;
> +
> + if (query_item->length == 0)
> + return hwconfig->size;
> +
> + if (query_item->length < hwconfig->size)
> + return -EINVAL;
> +
> + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
> +  hwconfig->ptr, hwconfig->size))
> + return -EFAULT;
> +
> + return hwconfig->size;
> +}
> +
>  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
>   struct drm_i915_query_item *query_item) 
> = {
>   query_topology_info,
>   query_engine_info,
>   query_perf_config,
>   query_memregion_info,
> + query_hwconfig_table,
>  };
>  
>  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file 
> *file)
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 914ebd9290e5..132515199f27 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
>  #define DRM_I915_QUERY_ENGINE_INFO   2
>  #define DRM_I915_QUERY_PERF_CONFIG  3
>  #define DRM_I915_QUERY_MEMORY_REGIONS   4
> +#define DRM_I915_QUERY_HWCONFIG_TABLE   5
>  /* Must be kept compact -- no holes and well documented */
>  
>   /**
> -- 
> 2.25.1

Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

John Harrison  writes:

> On 1/27/2022 16:48, Jordan Justen wrote:
>> john.c.harri...@intel.com writes:
>>
>>> From: Rodrigo Vivi 
>>>
>>> GuC contains a consolidated table with a bunch of information about the
>>> current device.
>>>
>>> Previously, this information was spread and hardcoded to all the components
>>> including GuC, i915 and various UMDs. The goal here is to consolidate
>>> the data into GuC in a way that all interested components can grab the
>>> very latest and synchronized information using a simple query.
>> This "consolidate" goal is not what I was told for the purpose of this.
>> I don't think these paragraphs are the true.
> The intention is to remove multiple hardcoded tables spread across a 
> bunch of different drivers and replace them with a single table 
> retrieved from the hardware itself. That sounds like consolidation to me.

That is not what I was told. That is apparently what someone is trying
to sell here.

Mesa would prefer to "hardcode" info rather than depend on the closed
source guc software.

>>
>>> As per most of the other queries, this one can be called twice.
>>> Once with item.length=0 to determine the exact buffer size, then
>>> allocate the user memory and call it again for to retrieve the
>>> table data. For example:
>>>struct drm_i915_query_item item = {
>>>  .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>>>};
>>>query.items_ptr = (int64_t) 
>>>query.num_items = 1;
>>>
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>if (item.length <= 0)
>>>  return -ENOENT;
>>>
>>>data = malloc(item.length);
>>>item.data_ptr = (int64_t) 
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>// Parse the data as appropriate...
>>>
>>> The returned array is a simple and flexible KLV (Key/Length/Value)
>>> formatted table. For example, it could be just:
>>>enum device_attr {
>>>   ATTR_SOME_VALUE = 0,
>>>   ATTR_SOME_MASK  = 1,
>>>};
>>>
>>>static const u32 hwconfig[] = {
>>>ATTR_SOME_VALUE,
>>>1, // Value Length in DWords
>>>8, // Value
>>>
>>>ATTR_SOME_MASK,
>>>3,
>>>0x00, 0x, 0xFF00,
>>>};
>> You said on 03 Nov 2021 that you would remove the parts of this commit
>> message that document the format. Why? Because i915 will not make any
>> guarantees as to the format of what is returned. Thus, i915 should not
>> comment on the format.
> And you replied that you would prefer to keep it.

No, I did not.

You said, "Sure. Can remove comments." to which, I replied, "Obviously
not what should be done, but apparently all i915 is willing to do."

So, i915 should document and stand behind this blob's format. But, if
they are not willing to, they shouldn't half-heartedly put some text in
a commit message.

>>
>> Can you Cc me on future postings of this patch?
>>
>>> The attribute ids are defined in a hardware spec.
>> As this spec is not published, it's hard to verify or refute this claim.
>>
>> Think this is a more accurate commit message for this patch:
>>
>>  In this interface i915 is returning a currently undocumented blob of
>>  data which it receives from the closed source guc software. The
>>  format of this blob *might* be defined in a hardware spec in the
>>  future.
>>
>> I'm sure you will prefer to replace "might" with "is planned to". I
>> think "might" is more accurate, but I suppose the other would be
>> acceptable.
>>
>> -Jordan
> Getting brand new spec documents published is not a fast process.

Heh.

Have you learned anything new about the status of it in the past 3
months?

> That doesn't mean it isn't going to happen.

It also doesn't mean it is going to happen either. Maybe you want to add
some text wherein Intel guarantees that it will be released in a spec by
some date?

> Also, just because a document is currently confidential and private
> doesn't mean that it doesn't exist.

Should we add "This is documented in a private spec, so it really does
exist!"?

-Jordan

Re: [Intel-gfx] [PATCH v3 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

john.c.harri...@intel.com writes:

> From: Rodrigo Vivi 
>
> GuC contains a consolidated table with a bunch of information about the
> current device.
>
> Previously, this information was spread and hardcoded to all the components
> including GuC, i915 and various UMDs. The goal here is to consolidate
> the data into GuC in a way that all interested components can grab the
> very latest and synchronized information using a simple query.

This "consolidate" goal is not what I was told for the purpose of this.
I don't think these paragraphs are the true.

> As per most of the other queries, this one can be called twice.
> Once with item.length=0 to determine the exact buffer size, then
> allocate the user memory and call it again for to retrieve the
> table data. For example:
>   struct drm_i915_query_item item = {
> .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>   };
>   query.items_ptr = (int64_t) 
>   query.num_items = 1;
>
>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>
>   if (item.length <= 0)
> return -ENOENT;
>
>   data = malloc(item.length);
>   item.data_ptr = (int64_t) 
>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>
>   // Parse the data as appropriate...
>
> The returned array is a simple and flexible KLV (Key/Length/Value)
> formatted table. For example, it could be just:
>   enum device_attr {
>  ATTR_SOME_VALUE = 0,
>  ATTR_SOME_MASK  = 1,
>   };
>
>   static const u32 hwconfig[] = {
>   ATTR_SOME_VALUE,
>   1, // Value Length in DWords
>   8, // Value
>
>   ATTR_SOME_MASK,
>   3,
>   0x00, 0x, 0xFF00,
>   };

You said on 03 Nov 2021 that you would remove the parts of this commit
message that document the format. Why? Because i915 will not make any
guarantees as to the format of what is returned. Thus, i915 should not
comment on the format.

Can you Cc me on future postings of this patch?

> The attribute ids are defined in a hardware spec.

As this spec is not published, it's hard to verify or refute this claim.

Think this is a more accurate commit message for this patch:

In this interface i915 is returning a currently undocumented blob of
data which it receives from the closed source guc software. The
format of this blob *might* be defined in a hardware spec in the
future.

I'm sure you will prefer to replace "might" with "is planned to". I
think "might" is more accurate, but I suppose the other would be
acceptable.

-Jordan

>
> Cc: Tvrtko Ursulin 
> Cc: Kenneth Graunke 
> Cc: Michal Wajdeczko 
> Cc: Slawomir Milczarek 
> Signed-off-by: Rodrigo Vivi 
> Signed-off-by: John Harrison 
> Reviewed-by: Matthew Brost 
> ---
>  drivers/gpu/drm/i915/i915_query.c | 23 +++
>  include/uapi/drm/i915_drm.h   |  1 +
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_query.c 
> b/drivers/gpu/drm/i915/i915_query.c
> index 2dfbc22857a3..609e64d5f395 100644
> --- a/drivers/gpu/drm/i915/i915_query.c
> +++ b/drivers/gpu/drm/i915/i915_query.c
> @@ -479,12 +479,35 @@ static int query_memregion_info(struct drm_i915_private 
> *i915,
>   return total_length;
>  }
>  
> +static int query_hwconfig_table(struct drm_i915_private *i915,
> + struct drm_i915_query_item *query_item)
> +{
> + struct intel_gt *gt = to_gt(i915);
> + struct intel_guc_hwconfig *hwconfig = >uc.guc.hwconfig;
> +
> + if (!hwconfig->size || !hwconfig->ptr)
> + return -ENODEV;
> +
> + if (query_item->length == 0)
> + return hwconfig->size;
> +
> + if (query_item->length < hwconfig->size)
> + return -EINVAL;
> +
> + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
> +  hwconfig->ptr, hwconfig->size))
> + return -EFAULT;
> +
> + return hwconfig->size;
> +}
> +
>  static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
>   struct drm_i915_query_item *query_item) 
> = {
>   query_topology_info,
>   query_engine_info,
>   query_perf_config,
>   query_memregion_info,
> + query_hwconfig_table,
>  };
>  
>  int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file 
> *file)
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 914ebd9290e5..132515199f27 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2685,6 +2685,7 @@ struct drm_i915_query_item {
>  #define DRM_I915_QUERY_ENGINE_INFO   2
>  #define DRM_I915_QUERY_PERF_CONFIG  3
>  #define DRM_I915_QUERY_MEMORY_REGIONS   4
> +#define DRM_I915_QUERY_HWCONFIG_TABLE   5
>  /* Must be kept compact -- no holes and well documented */
>  
>   /**
> -- 
> 2.25.1

Re: [PATCH v2 4/4] drm/i915/uapi: document behaviour for DG2 64K support

[Jordan Justen]

Robert Beckett  writes:

> From: Matthew Auld 
>
> On discrete platforms like DG2, we need to support a minimum page size
> of 64K when dealing with device local-memory. This is quite tricky for
> various reasons, so try to document the new implicit uapi for this.
>
> v2: Fixed suggestions on formatting [Daniel]
>
> Signed-off-by: Matthew Auld 
> Signed-off-by: Ramalingam C 
> Signed-off-by: Robert Beckett 
> cc: Simon Ser 
> cc: Pekka Paalanen 
> Cc: Jordan Justen 
> Cc: Kenneth Graunke 
> Cc: mesa-...@lists.freedesktop.org
> Cc: Tony Ye 
> Cc: Slawomir Milczarek 
> ---
>  include/uapi/drm/i915_drm.h | 44 -
>  1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 5e678917da70..486b7b96291e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1118,10 +1118,16 @@ struct drm_i915_gem_exec_object2 {
>   /**
>* When the EXEC_OBJECT_PINNED flag is specified this is populated by
>* the user with the GTT offset at which this object will be pinned.
> +  *
>* When the I915_EXEC_NO_RELOC flag is specified this must contain the
>* presumed_offset of the object.
> +  *
>* During execbuffer2 the kernel populates it with the value of the
>* current GTT offset of the object, for future presumed_offset writes.
> +  *
> +  * See struct drm_i915_gem_create_ext for the rules when dealing with
> +  * alignment restrictions with I915_MEMORY_CLASS_DEVICE, on devices with
> +  * minimum page sizes, like DG2.
>*/
>   __u64 offset;
>  
> @@ -3145,11 +3151,39 @@ struct drm_i915_gem_create_ext {
>*
>* The (page-aligned) allocated size for the object will be returned.
>*
> -  * Note that for some devices we have might have further minimum
> -  * page-size restrictions(larger than 4K), like for device local-memory.
> -  * However in general the final size here should always reflect any
> -  * rounding up, if for example using the 
> I915_GEM_CREATE_EXT_MEMORY_REGIONS
> -  * extension to place the object in device local-memory.
> +  *
> +  * **DG2 64K min page size implications:**

Long term, I'm not sure that the "**" (for emphasis) is needed here or
below. It's interesting at the moment, but will be just another thing
baked into the kernel/user code in a month from now. :)

> +  *
> +  * On discrete platforms, starting from DG2, we have to contend with GTT
> +  * page size restrictions when dealing with I915_MEMORY_CLASS_DEVICE
> +  * objects.  Specifically the hardware only supports 64K or larger GTT
> +  * page sizes for such memory. The kernel will already ensure that all
> +  * I915_MEMORY_CLASS_DEVICE memory is allocated using 64K or larger page
> +  * sizes underneath.
> +  *
> +  * Note that the returned size here will always reflect any required
> +  * rounding up done by the kernel, i.e 4K will now become 64K on devices
> +  * such as DG2.
> +  *
> +  * **Special DG2 GTT address alignment requirement:**
> +  *
> +  * The GTT alignment will also need be at least 2M for  such objects.
> +  *
> +  * Note that due to how the hardware implements 64K GTT page support, we
> +  * have some further complications:
> +  *
> +  *   1) The entire PDE(which covers a 2MB virtual address range), must
> +  *   contain only 64K PTEs, i.e mixing 4K and 64K PTEs in the same
> +  *   PDE is forbidden by the hardware.
> +  *
> +  *   2) We still need to support 4K PTEs for I915_MEMORY_CLASS_SYSTEM
> +  *   objects.
> +  *
> +  * To keep things simple for userland, we mandate that any GTT mappings
> +  * must be aligned to and rounded up to 2MB. As this only wastes virtual
> +  * address space and avoids userland having to copy any needlessly
> +  * complicated PDE sharing scheme (coloring) and only affects GD2, this
> +  * id deemed to be a good compromise.

typos: GD2, id

Isn't much of this more relavent to the vma offset at exec time? Is
there actually any new restriction on the size field during buffer
creation?

I see Matthew references these notes from the offset comments, so if the
kernel devs prefer it here, then you can add my Acked-by on this patch.

-Jordan

>*/
>   __u64 size;
>   /**
> -- 
> 2.25.1

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

John Harrison  writes:

> On 11/3/2021 14:38, Jordan Justen wrote:
>> So, i915 wants to wash it's hands completely of the format? There is
>> obviously a difference between hardware features and a blob coming from
>> closed source software. (Which i915 just happens to be passing along.)
>> The hardware is a lot more difficult to change...
> Actually, no. The table is not "coming from closed source software". The 
> table is defined by hardware specs. It is a table of hardware specific 
> values.

So, guc literally reads this info from the hardware verbatim? Then gives
it to i915 so i915 can give it to UMDs? Otherwise, it seems like a table
in software. Anyway...

>> It seems like these details should be dropped from the i915 patch commit
>> message since i915 wants nothing to do with it.
> Sure. Can remove comments.

Obviously not what should be done, but apparently all i915 is willing to
do.

>> I would think it'd be preferable for i915 to stand behind the basic blob
>> format as is (even if the keys/values can't be defined), and make a new
>> query item if the closed source software changes the format.
> Close source software is not allowed to change the format because closed 
> source software has no say in defining the format.

So, why can't i915 define this extremely simple (apparently
unchangeable) blob format, and thereby guarantee that it will have to
insulate UMDs if the format changes by making a different query item? It
ought to be made painful for everyone if this blob format changes.
Hopefully the format will basically never change. (Even if new
keys/values might be added.)

Further, it seems there is an implication that the keys will always be
backward compatible. Is that true, and if so, how can there be harm in
i915 enumerating the "known" ones?

>> Of course, it'd be even better if i915 could define some keys/values as
>> well. (Or if a spec could be released to help document / tie down the
>> format.)
> See the corresponding IGT test that details all the currently defined keys.

i915 can't/won't say anything about it, but look at this unmerged IGT
test? In the next sentence you'll say, but don't count on that because
IGT really has no control over it. :)

>>>>> The attribute ids are defined in a hardware spec.
>>>> Which spec?
>>> Unfortunately, it is not one that is currently public. We are pushing
>>> the relevant people to get it included in the public bspec / HRM. It is
>>> a slow process :(.
>>>
>> Right. I take it no progress has been made in the 1.5 months since you
>> posted this, so it'll probably finally be documented 6~12 months after
>> hardware is available? :)

Apparently all the data for this spec is "available" (in an unmerged IGT
patch), but am I correct in assuming that no actual spec timeframe is
defined?

-Jordan

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

John Harrison  writes:

> On 11/1/2021 08:39, Jordan Justen wrote:
>>  writes:
>>
>>> From: Rodrigo Vivi 
>>>
>>> GuC contains a consolidated table with a bunch of information about the
>>> current device.
>>>
>>> Previously, this information was spread and hardcoded to all the components
>>> including GuC, i915 and various UMDs. The goal here is to consolidate
>>> the data into GuC in a way that all interested components can grab the
>>> very latest and synchronized information using a simple query.
>>>
>>> As per most of the other queries, this one can be called twice.
>>> Once with item.length=0 to determine the exact buffer size, then
>>> allocate the user memory and call it again for to retrieve the
>>> table data. For example:
>>>struct drm_i915_query_item item = {
>>>  .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>>>};
>>>query.items_ptr = (int64_t) 
>>>query.num_items = 1;
>>>
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>if (item.length <= 0)
>>>  return -ENOENT;
>>>
>>>data = malloc(item.length);
>>>item.data_ptr = (int64_t) 
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>// Parse the data as appropriate...
>>>
>>> The returned array is a simple and flexible KLV (Key/Length/Value)
>>> formatted table. For example, it could be just:
>>>enum device_attr {
>>>   ATTR_SOME_VALUE = 0,
>>>   ATTR_SOME_MASK  = 1,
>>>};
>>>
>>>static const u32 hwconfig[] = {
>>>ATTR_SOME_VALUE,
>>>1, // Value Length in DWords
>>>8, // Value
>>>
>>>ATTR_SOME_MASK,
>>>3,
>>>0x00, 0x, 0xFF00,
>>>};
>> Seems simple enough, so why doesn't i915 define the format of the
>> returned hwconfig blob in i915_drm.h?
> Because the definition is nothing to do with i915. This table comes from 
> the hardware spec. It is not defined by the KMD and it is not currently 
> used by the KMD. So there is no reason for the KMD to be creating 
> structures for it in the same way that the KMD does not document, 
> define, struct, etc. every other feature of the hardware that the UMDs 
> might use.

So, i915 wants to wash it's hands completely of the format? There is
obviously a difference between hardware features and a blob coming from
closed source software. (Which i915 just happens to be passing along.)
The hardware is a lot more difficult to change...

It seems like these details should be dropped from the i915 patch commit
message since i915 wants nothing to do with it.

I would think it'd be preferable for i915 to stand behind the basic blob
format as is (even if the keys/values can't be defined), and make a new
query item if the closed source software changes the format.

Of course, it'd be even better if i915 could define some keys/values as
well. (Or if a spec could be released to help document / tie down the
format.)

>>
>> struct drm_i915_hwconfig {
>>  uint32_t key;
>>  uint32_t length;
>>  uint32_t values[];
>> };
>>
>> It sounds like the kernel depends on the closed source guc being loaded
>> to return this information. Is that right? Will i915 also become
>> dependent on some of this data such that it won't be able to initialize
>> without the firmware being loaded?
> At the moment, the KMD does not use the table at all. We merely provide 
> a mechanism for the UMDs to retrieve it from the hardware.
>
> In terms of future direction, that is something you need to take up with 
> the hardware architects.
>

Why do you keep saying hardware, when only software is involved?

>
>>> The attribute ids are defined in a hardware spec.
>> Which spec?
>
> Unfortunately, it is not one that is currently public. We are pushing 
> the relevant people to get it included in the public bspec / HRM. It is 
> a slow process :(.
>

Right. I take it no progress has been made in the 1.5 months since you
posted this, so it'll probably finally be documented 6~12 months after
hardware is available? :)

-Jordan

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

 writes:

> From: Rodrigo Vivi 
>
> GuC contains a consolidated table with a bunch of information about the
> current device.
>
> Previously, this information was spread and hardcoded to all the components
> including GuC, i915 and various UMDs. The goal here is to consolidate
> the data into GuC in a way that all interested components can grab the
> very latest and synchronized information using a simple query.
>
> As per most of the other queries, this one can be called twice.
> Once with item.length=0 to determine the exact buffer size, then
> allocate the user memory and call it again for to retrieve the
> table data. For example:
>   struct drm_i915_query_item item = {
> .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>   };
>   query.items_ptr = (int64_t) 
>   query.num_items = 1;
>
>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>
>   if (item.length <= 0)
> return -ENOENT;
>
>   data = malloc(item.length);
>   item.data_ptr = (int64_t) 
>   ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>
>   // Parse the data as appropriate...
>
> The returned array is a simple and flexible KLV (Key/Length/Value)
> formatted table. For example, it could be just:
>   enum device_attr {
>  ATTR_SOME_VALUE = 0,
>  ATTR_SOME_MASK  = 1,
>   };
>
>   static const u32 hwconfig[] = {
>   ATTR_SOME_VALUE,
>   1, // Value Length in DWords
>   8, // Value
>
>   ATTR_SOME_MASK,
>   3,
>   0x00, 0x, 0xFF00,
>   };

Seems simple enough, so why doesn't i915 define the format of the
returned hwconfig blob in i915_drm.h?

struct drm_i915_hwconfig {
uint32_t key;
uint32_t length;
uint32_t values[];
};

It sounds like the kernel depends on the closed source guc being loaded
to return this information. Is that right? Will i915 also become
dependent on some of this data such that it won't be able to initialize
without the firmware being loaded?

> The attribute ids are defined in a hardware spec.

Which spec?

-Jordan

[Mesa-dev] [PATCH 1/2] intel: Add SKL GT4 PCI IDs

[Jordan Justen]

Series Reviewed-by: Jordan Justen 

For the 2de4e8fdbae1e1909ce35f8ba15608a124686fb0 version on your drm
gt4 branch.

On 2015-10-23 10:56:33, Ben Widawsky wrote:
> Cc: Kristian Høgsberg 
> Cc: Damien Lespiau 
> Signed-off-by: Ben Widawsky 
> ---
>  intel/intel_chipset.h | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
> index 253ea71..6c8dc73 100644
> --- a/intel/intel_chipset.h
> +++ b/intel/intel_chipset.h
> @@ -180,6 +180,10 @@
>  #define PCI_CHIP_SKYLAKE_SRV_GT3   0x192A
>  #define PCI_CHIP_SKYLAKE_SRV_GT1   0x190A
>  #define PCI_CHIP_SKYLAKE_WKS_GT2   0x191D
> +#define PCI_CHIP_SKYLAKE_DT_GT40x1932
> +#define PCI_CHIP_SKYLAKE_SRV_GT4   0x193A
> +#define PCI_CHIP_SKYLAKE_H_GT4 0x193B
> +#define PCI_CHIP_SKYLAKE_WKS_GT4   0x193D
>  
>  #define PCI_CHIP_BROXTON_0 0x0A84
>  #define PCI_CHIP_BROXTON_1 0x1A84
> @@ -362,9 +366,15 @@
>  (devid) == PCI_CHIP_SKYLAKE_HALO_GT3   || \
>  (devid) == PCI_CHIP_SKYLAKE_SRV_GT3)
>  
> +#define IS_SKL_GT4(devid)  ((devid) == PCI_CHIP_SKYLAKE_DT_GT4 || \
> +(devid) == PCI_CHIP_SKYLAKE_SRV_GT4|| \
> +(devid) == PCI_CHIP_SKYLAKE_H_GT4  || \
> +(devid) == PCI_CHIP_SKYLAKE_WKS_GT4)
> +
>  #define IS_SKYLAKE(devid)  (IS_SKL_GT1(devid) || \
>  IS_SKL_GT2(devid) || \
> -IS_SKL_GT3(devid))
> +IS_SKL_GT3(devid) || \
> +IS_SKL_GT4(devid))
>  
>  #define IS_BROXTON(devid)  ((devid) == PCI_CHIP_BROXTON_0  || \
>  (devid) == PCI_CHIP_BROXTON_1  || \
> -- 
> 2.6.1
> 
> ___
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[PATCH 5/8] dri/common: Add functions mapping MESA_FORMAT_* <-> __DRI_IMAGE_FORMAT_*

[Jordan Justen]

On Mon, Nov 4, 2013 at 8:11 PM, Keith Packard  wrote:
> Jordan Justen  writes:
>> After patch 6, this will add SARGB8, right? So, maybe add this to the
>> commit message, or separate out adding SARGB8 into a separate commit?
>
> I added the SARGB8 define in patch 4; is there some other separation you
> think would be warranted?

I was just noting that a side effect of patch five is adding support
for SARGB8. When you remove the code in patch 6, and use these, you've
now added support for this new format.

Not important, but I thought it might be worth noting in the commit
message. Actually probably noting it in the commit message for patch 6
is better since the change happens at that point.

-Jordan

[PATCH 5/8] dri/common: Add functions mapping MESA_FORMAT_* <-> __DRI_IMAGE_FORMAT_*

[Jordan Justen]

On Mon, Nov 4, 2013 at 6:23 PM, Keith Packard  wrote:
> The __DRI_IMAGE_FORMAT codes are used by the image extension, drivers need to
> be able to translate between them. Instead of duplicating this translation in
> each driver, create a shared version.
>
> Signed-off-by: Keith Packard 
> ---
>  src/mesa/drivers/dri/common/dri_util.c | 62 
> ++
>  src/mesa/drivers/dri/common/dri_util.h |  6 
>  2 files changed, 68 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/common/dri_util.c 
> b/src/mesa/drivers/dri/common/dri_util.c
> index 95c8b41..76c8ae5 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -792,3 +792,65 @@ driUpdateFramebufferSize(struct gl_context *ctx, const 
> __DRIdrawable *dPriv)
>assert(fb->Height == dPriv->h);
> }
>  }
> +
> +uint32_t
> +driGLFormatToImageFormat(gl_format format)
> +{
> +   switch (format) {
> +   case MESA_FORMAT_RGB565:
> +  return __DRI_IMAGE_FORMAT_RGB565;
> +   case MESA_FORMAT_XRGB:
> +  return __DRI_IMAGE_FORMAT_XRGB;
> +   case MESA_FORMAT_ARGB2101010:
> +  return __DRI_IMAGE_FORMAT_ARGB2101010;
> +   case MESA_FORMAT_XRGB2101010_UNORM:
> +  return __DRI_IMAGE_FORMAT_XRGB2101010;
> +   case MESA_FORMAT_ARGB:
> +  return __DRI_IMAGE_FORMAT_ARGB;
> +   case MESA_FORMAT_RGBA_REV:
> +  return __DRI_IMAGE_FORMAT_ABGR;
> +   case MESA_FORMAT_RGBX_REV:
> +  return __DRI_IMAGE_FORMAT_XBGR;
> +   case MESA_FORMAT_R8:
> +  return __DRI_IMAGE_FORMAT_R8;
> +   case MESA_FORMAT_GR88:
> +  return __DRI_IMAGE_FORMAT_GR88;
> +   case MESA_FORMAT_NONE:
> +  return __DRI_IMAGE_FORMAT_NONE;
> +   case MESA_FORMAT_SARGB8:
> +  return __DRI_IMAGE_FORMAT_SARGB8;

After patch 6, this will add SARGB8, right? So, maybe add this to the
commit message, or separate out adding SARGB8 into a separate commit?

Patches 1-6: Reviewed-by: Jordan Justen 

-Jordan

> +   default:
> +  return 0;
> +   }
> +}
> +
> +gl_format
> +driImageFormatToGLFormat(uint32_t image_format)
> +{
> +   switch (image_format) {
> +   case __DRI_IMAGE_FORMAT_RGB565:
> +  return MESA_FORMAT_RGB565;
> +   case __DRI_IMAGE_FORMAT_XRGB:
> +  return MESA_FORMAT_XRGB;
> +   case __DRI_IMAGE_FORMAT_ARGB2101010:
> +  return MESA_FORMAT_ARGB2101010;
> +   case __DRI_IMAGE_FORMAT_XRGB2101010:
> +  return MESA_FORMAT_XRGB2101010_UNORM;
> +   case __DRI_IMAGE_FORMAT_ARGB:
> +  return MESA_FORMAT_ARGB;
> +   case __DRI_IMAGE_FORMAT_ABGR:
> +  return MESA_FORMAT_RGBA_REV;
> +   case __DRI_IMAGE_FORMAT_XBGR:
> +  return MESA_FORMAT_RGBX_REV;
> +   case __DRI_IMAGE_FORMAT_R8:
> +  return MESA_FORMAT_R8;
> +   case __DRI_IMAGE_FORMAT_GR88:
> +  return MESA_FORMAT_GR88;
> +   case __DRI_IMAGE_FORMAT_SARGB8:
> +  return MESA_FORMAT_SARGB8;
> +   case __DRI_IMAGE_FORMAT_NONE:
> +  return MESA_FORMAT_NONE;
> +   default:
> +  return MESA_FORMAT_NONE;
> +   }
> +}
> diff --git a/src/mesa/drivers/dri/common/dri_util.h 
> b/src/mesa/drivers/dri/common/dri_util.h
> index 5b56061..fd40769 100644
> --- a/src/mesa/drivers/dri/common/dri_util.h
> +++ b/src/mesa/drivers/dri/common/dri_util.h
> @@ -271,6 +271,12 @@ struct __DRIdrawableRec {
>  } dri2;
>  };
>
> +extern uint32_t
> +driGLFormatToImageFormat(gl_format format);
> +
> +extern gl_format
> +driImageFormatToGLFormat(uint32_t image_format);
> +
>  extern void
>  dri2InvalidateDrawable(__DRIdrawable *drawable);
>
> --
> 1.8.4.2
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm: Changes to 'debian-experimental'

[Jordan Justen]

On Mon, Sep 10, 2012 at 1:27 AM, Michel Dänzer mic...@daenzer.net wrote:
 On Son, 2012-09-09 at 15:33 -0700, Jordan Justen wrote:
 New branch 'debian-experimental' available with the following commits:

 I think you pushed this to the wrong repository?

My apologies. I had the hook script mis-configured under my personal
repo. I don't think the main drm repo was modified.

-Jordan

--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
--
___
Dri-devel mailing list
dri-de...@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

drm: Changes to 'debian-experimental'

[Jordan Justen]

On Mon, Sep 10, 2012 at 1:27 AM, Michel D?nzer  wrote:
> On Son, 2012-09-09 at 15:33 -0700, Jordan Justen wrote:
>> New branch 'debian-experimental' available with the following commits:
>
> I think you pushed this to the wrong repository?

My apologies. I had the hook script mis-configured under my personal
repo. I don't think the main drm repo was modified.

-Jordan

--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
--
___
Dri-devel mailing list
Dri-devel at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel