diff mbox series

[v3] drm/i915/guc: Simplify/extend platform check for Wa_14018913170

Message ID 20240216183841.497768-1-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series [v3] drm/i915/guc: Simplify/extend platform check for Wa_14018913170 | expand

Commit Message

John Harrison Feb. 16, 2024, 6:38 p.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

The above w/a is required for every platform that the i915 driver
supports. It is fixed on the latest platforms but they are only
supported by Xe instead of i915. So just remove the platform check
completely and keep the code simple.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Rodrigo Vivi Feb. 19, 2024, 8:28 p.m. UTC | #1 
On Fri, Feb 16, 2024 at 10:38:41AM -0800, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The above w/a is required for every platform that the i915 driver
> supports. It is fixed on the latest platforms but they are only
> supported by Xe instead of i915. So just remove the platform check
> completely and keep the code simple.

Well, I was going to say that I would prefer a GMD version greater-than
check to be future proof. However if this code gets used in some other
new platform a new specific guc support would likely need to be added
as well right?

Perhaps at least adding a comment in the code?

with that
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 2b450c43bbd7f..a3662edb42032 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -321,8 +321,7 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc)
>  
>  	/* Wa_14018913170 */
>  	if (GUC_FIRMWARE_VER(guc) >= MAKE_GUC_VER(70, 7, 0)) {
> -		if (IS_DG2(gt->i915) || IS_METEORLAKE(gt->i915) || IS_PONTEVECCHIO(gt->i915))
> -			flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
> +		flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
>  	}
>  
>  	return flags;
> -- 
> 2.43.0
>
John Harrison Feb. 20, 2024, 7:52 p.m. UTC | #2 
On 2/19/2024 12:28, Rodrigo Vivi wrote:
> On Fri, Feb 16, 2024 at 10:38:41AM -0800, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The above w/a is required for every platform that the i915 driver
>> supports. It is fixed on the latest platforms but they are only
>> supported by Xe instead of i915. So just remove the platform check
>> completely and keep the code simple.
> Well, I was going to say that I would prefer a GMD version greater-than
> check to be future proof. However if this code gets used in some other
> new platform a new specific guc support would likely need to be added
> as well right?
There is no future for i915. That's the point. The only platforms that 
have the hardware fix are all ones that will only ever be supported by 
the Xe driver. So if such a platform were to be backported to i915 then 
there would be a lot more work than just adding a new GuC firmware platform.

And going backwards, the bug affects all platforms that have a GuC. So 
if any GuC code is being executed at all, then this w/a is applicable.

>
> Perhaps at least adding a comment in the code?
Such as this?
         /*
          * Wa_14018913170: Applicable to all platforms supported by i915 so
          * don't bother testing for all X/Y/Z platforms explicitly.
          */

John.


>
> with that
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_guc.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> index 2b450c43bbd7f..a3662edb42032 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> @@ -321,8 +321,7 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc)
>>   
>>   	/* Wa_14018913170 */
>>   	if (GUC_FIRMWARE_VER(guc) >= MAKE_GUC_VER(70, 7, 0)) {
>> -		if (IS_DG2(gt->i915) || IS_METEORLAKE(gt->i915) || IS_PONTEVECCHIO(gt->i915))
>> -			flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
>> +		flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
>>   	}
>>   
>>   	return flags;
>> -- 
>> 2.43.0
>>
Rodrigo Vivi Feb. 20, 2024, 10:03 p.m. UTC | #3 
On Tue, Feb 20, 2024 at 11:52:04AM -0800, John Harrison wrote:
> On 2/19/2024 12:28, Rodrigo Vivi wrote:
> > On Fri, Feb 16, 2024 at 10:38:41AM -0800, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@Intel.com>
> > > 
> > > The above w/a is required for every platform that the i915 driver
> > > supports. It is fixed on the latest platforms but they are only
> > > supported by Xe instead of i915. So just remove the platform check
> > > completely and keep the code simple.
> > Well, I was going to say that I would prefer a GMD version greater-than
> > check to be future proof. However if this code gets used in some other
> > new platform a new specific guc support would likely need to be added
> > as well right?
> There is no future for i915. That's the point. The only platforms that have
> the hardware fix are all ones that will only ever be supported by the Xe
> driver. So if such a platform were to be backported to i915 then there would
> be a lot more work than just adding a new GuC firmware platform.
> 
> And going backwards, the bug affects all platforms that have a GuC. So if
> any GuC code is being executed at all, then this w/a is applicable.
> 
> > 
> > Perhaps at least adding a comment in the code?
> Such as this?
>         /*
>          * Wa_14018913170: Applicable to all platforms supported by i915.

I believe only this first line would be enough. but up to you and feel free
to put my rv-b with whatever version you get.

 so
>          * don't bother testing for all X/Y/Z platforms explicitly.
>          */
> 
> John.
> 
> 
> > 
> > with that
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > 
> > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gt/uc/intel_guc.c | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > index 2b450c43bbd7f..a3662edb42032 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> > > @@ -321,8 +321,7 @@ static u32 guc_ctl_wa_flags(struct intel_guc *guc)
> > >   	/* Wa_14018913170 */
> > >   	if (GUC_FIRMWARE_VER(guc) >= MAKE_GUC_VER(70, 7, 0)) {
> > > -		if (IS_DG2(gt->i915) || IS_METEORLAKE(gt->i915) || IS_PONTEVECCHIO(gt->i915))
> > > -			flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
> > > +		flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
> > >   	}
> > >   	return flags;
> > > -- 
> > > 2.43.0
> > > 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 2b450c43bbd7f..a3662edb42032 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -321,8 +321,7 @@  static u32 guc_ctl_wa_flags(struct intel_guc *guc)
 
 	/* Wa_14018913170 */
 	if (GUC_FIRMWARE_VER(guc) >= MAKE_GUC_VER(70, 7, 0)) {
-		if (IS_DG2(gt->i915) || IS_METEORLAKE(gt->i915) || IS_PONTEVECCHIO(gt->i915))
-			flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
+		flags |= GUC_WA_ENABLE_TSC_CHECK_ON_RC6;
 	}
 
 	return flags;