[Intel-gfx] [PATCH] drm/i915/tgl: Fix REVID macros for TGL to fetch correct stepping

[Lucas De Marchi]

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 at intel.com> 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 at intel.com>
>>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Signed-off-by: Aditya Swarup <aditya.swarup at intel.com>
>>>> ---
>>>>  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
>>
>