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

[Aditya Swarup <aditya.swarup@intel.com>]

On 11/25/20 11:18 AM, Lucas De Marchi wrote:
> 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@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@intel.com>
>>>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> Signed-off-by: Aditya Swarup <aditya.swarup@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:

The question is not about sizeof() not working but rather the usage of ARRAY_SIZE()
macro in i915_drv.h with just extern declaration without size specified.

> 
>     $ 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];

You are specifying the size in the extern declaration which will make the ARRAY_SIZE()
macro work if used in the header else it will complain.

> 
>     $ 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

I will go ahead with this approach.

Aditya 

> 
>>
>> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx