On Wed, Nov 25, 2020 at 09:51:04AM -0800, Aditya Swarup wrote:
On 11/25/20 7:33 AM, Chris Wilson wrote:
Quoting Jani Nikula (2020-11-25 11:45:56)
On Tue, 24 Nov 2020, Aditya Swarup <aditya.swarup@xxxxxxxxx> wrote:
Fix TGL REVID macros to fetch correct display/gt stepping based
on SOC rev id from INTEL_REVID() macro. Previously, we were just
returning the first element of the revid array instead of using
the correct index based on SOC rev id.
Also, add array bound checks for TGL REV ID array. Since, there
might be a possibility of using older kernels on latest platform
revision, resulting in out of bounds access for rev ID array.
In this scenario, print message for unsupported rev ID and apply
settings for latest rev ID available.
Fixes: ("drm/i915/tgl: Fix stepping WA matching")
Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Signed-off-by: Aditya Swarup <aditya.swarup@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 35 +++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 15be8debae54..29d55b7017be 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1572,16 +1572,37 @@ enum {
TGL_REVID_D0,
};
-extern const struct i915_rev_steppings tgl_uy_revids[];
-extern const struct i915_rev_steppings tgl_revids[];
+extern const struct i915_rev_steppings tgl_uy_revids[4];
+extern const struct i915_rev_steppings tgl_revids[2];
Just a quick note, the compiler does not check that the size in the
extern declaration matches the size in the array definition. So you
might end up with a mismatch without noticing.
Yes.. We will have to take care of it if we are adding rev id to array table(which mostly
should remain a const once we decide to go upstream). Without this declaration, I cannot
use ARRAY_SIZE() macro with revid arrays as the sizeof() operator complains about not
knowing the size of the array in question as it is an extern declaration.
So, I don't know what other approach you want to suggest? If we move all the array tables to i915_drv.h(which
I feel would be a better approach rather than having it in intel_workarounds.c), Matt
Roper's KBL patch says that compiler complains about unused variables.
adding the table in the header means that each compilation unit (.o)
will get a copy of the table when it includes the header (it will end up
being trimmed out if not used though). This is not what you want.
As I said in the other reply, sizeof does actually work here:
$ cat /tmp/a.c
#include <stdio.h>
#include "b.h"
int main(int argc, const char *argv[])
{
printf("%zu", sizeof(tgl_uy_revids));
return 0;
}
$ cat /tmp/b.h
#pragma once
struct i915_rev_steppings { int a; };
extern const struct i915_rev_steppings tgl_uy_revids[4];
$ cat /tmp/b.c
#include "b.h"
const struct i915_rev_steppings tgl_uy_revids[] = {
{ 10 },
{ 20 },
{ 30 },
{ 40 },
};
And compiler also warns if in the *definition* of tgl_uy_revids it goes
over the amount of space of the declaration. For clarity, you may
however want to add a define to tell the size:
-extern const struct i915_rev_steppings tgl_uy_revids[4];
+#define TGL_UY_REVIDS_SIZE 4
+extern const struct i915_rev_steppings tgl_uy_revids[TGL_UY_REVIDS_SIZE];
and do the same in the .c
We are anyhow going to correct the whole thing with your stepping series anyway. This is supposed
to be a stop gap fix. Revids shouldn't be changing for TGL anymore.
What surprised me is that this defeated the __must_be_array() check.
I thought these were just pointers to C
+#define TGL_UY_REVID_RANGE(revid) \
+ ((revid) < ARRAY_SIZE(tgl_uy_revids))
+
+#define TGL_REVID_RANGE(revid) \
+ ((revid) < ARRAY_SIZE(tgl_revids))
static inline const struct i915_rev_steppings *
tgl_revids_get(struct drm_i915_private *dev_priv)
{
- if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv))
- return tgl_uy_revids;
- else
- return tgl_revids;
+ const u8 revid = INTEL_REVID(dev_priv);
+
+ if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv)) {
+ if (TGL_UY_REVID_RANGE(revid)) {
+ return tgl_uy_revids + revid;
+ } else {
+ drm_dbg_kms(&dev_priv->drm,
+ "Unsupported SOC stepping found %u, using %lu instead\n",
+ revid, ARRAY_SIZE(tgl_uy_revids) - 1);
Also please don't have a dbg for every single IS_TGL_*_REVID
invocation. And this is not _kms, but driver; better yet, don't bother
with a drm_dbg_kms here at all.
If you want to actually check, add something like
intel_detect_preproduction_hw() and warn about unknown future revids.
Or include the info when we print the revid in the caps.
So, what you are suggesting is add an info print in that function intel_detect_preproduction_hw() right?
Or something else?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
