diff mbox series

drm/i915/mtl: do not enable render power-gating on MTL

Message ID 20230517-mtl_disable_render_pg-v1-1-6495eebbfb24@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/mtl: do not enable render power-gating on MTL | expand

Commit Message

Andrzej Hajda May 17, 2023, 1:59 p.m. UTC 
Multiple CI tests fails with forcewake ack timeouts
if render power gating is enabled.
BSpec 52698 clearly states it should be 0 for MTL.

Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4983
Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)


---
base-commit: 01d3dd92d1b71421f6ee85e1bea829e0a917d979
change-id: 20230517-mtl_disable_render_pg-b9f9f1567f9e

Best regards,

Comments

Nirmoy Das May 17, 2023, 3:12 p.m. UTC | #1 
On 5/17/2023 3:59 PM, Andrzej Hajda wrote:
> Multiple CI tests fails with forcewake ack timeouts
> if render power gating is enabled.
> BSpec 52698 clearly states it should be 0 for MTL.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4983
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
> index 908a3d0f2343f4..ebb2373dd73640 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> @@ -117,8 +117,9 @@ static void gen11_rc6_enable(struct intel_rc6 *rc6)
>   			GEN6_RC_CTL_RC6_ENABLE |
>   			GEN6_RC_CTL_EI_MODE(1);
>   
> -	/* Wa_16011777198 - Render powergating must remain disabled */
> -	if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) ||
> +	/* Wa_16011777198 and BSpec 52698 - Render powergating must be off */

Nice catch! instead of bspec you could add Wa_14012655556.


> +	if (IS_METEORLAKE(gt->i915) ||
> +	    IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) ||
>   	    IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))
>   		pg_enable =
>   			GEN9_MEDIA_PG_ENABLE |
>
> ---
> base-commit: 01d3dd92d1b71421f6ee85e1bea829e0a917d979
> change-id: 20230517-mtl_disable_render_pg-b9f9f1567f9e

^ unwanted artifacts ?   Otherwise this looks good to me.

Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

>
> Best regards,
Rodrigo Vivi May 17, 2023, 3:25 p.m. UTC | #2 
On Wed, 2023-05-17 at 17:12 +0200, Das, Nirmoy wrote:
> 
> On 5/17/2023 3:59 PM, Andrzej Hajda wrote:
> > Multiple CI tests fails with forcewake ack timeouts
> > if render power gating is enabled.
> > BSpec 52698 clearly states it should be 0 for MTL.
> > 
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4983
> > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
> > b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > index 908a3d0f2343f4..ebb2373dd73640 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > @@ -117,8 +117,9 @@ static void gen11_rc6_enable(struct intel_rc6
> > *rc6)
> >                         GEN6_RC_CTL_RC6_ENABLE |
> >                         GEN6_RC_CTL_EI_MODE(1);
> >   
> > -       /* Wa_16011777198 - Render powergating must remain disabled
> > */
> > -       if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
> > ||
> > +       /* Wa_16011777198 and BSpec 52698 - Render powergating must
> > be off */
> 
> Nice catch!

Indeed! What a mess in the workaround database.
It is telling us that no_impact on MTL SKUs while we clearly needs
that. I tried to reopen that and get that fixed in the hsds.


>  instead of bspec you could add Wa_14012655556.

not actually.
16011777198 is the right lineage number for 14012655556.
Besides, 14012655556 is for DG2 anyway.

Let's keep the way Adrzej put with the BSPec reference besides the
lineage.

> 
> 
> > +       if (IS_METEORLAKE(gt->i915) ||
> > +           IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
> > ||
> >             IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))
> >                 pg_enable =
> >                         GEN9_MEDIA_PG_ENABLE |
> > 
> > ---
> > base-commit: 01d3dd92d1b71421f6ee85e1bea829e0a917d979
> > change-id: 20230517-mtl_disable_render_pg-b9f9f1567f9e
> 
> ^ unwanted artifacts ?   Otherwise this looks good to me.
> 
> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

with the artifacts removed:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> 
> > 
> > Best regards,
Andrzej Hajda May 17, 2023, 3:27 p.m. UTC | #3 
On 17.05.2023 17:12, Das, Nirmoy wrote:
>
> On 5/17/2023 3:59 PM, Andrzej Hajda wrote:
>> Multiple CI tests fails with forcewake ack timeouts
>> if render power gating is enabled.
>> BSpec 52698 clearly states it should be 0 for MTL.
>>
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4983
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c 
>> b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> index 908a3d0f2343f4..ebb2373dd73640 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> @@ -117,8 +117,9 @@ static void gen11_rc6_enable(struct intel_rc6 *rc6)
>>               GEN6_RC_CTL_RC6_ENABLE |
>>               GEN6_RC_CTL_EI_MODE(1);
>>   -    /* Wa_16011777198 - Render powergating must remain disabled */
>> -    if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) ||
>> +    /* Wa_16011777198 and BSpec 52698 - Render powergating must be 
>> off */
>
> Nice catch! instead of bspec you could add Wa_14012655556.

I put bspec because it is quite clear on the subject in contrast to WA 
:) but I can change it if this way is preferred.

>
>
>> +    if (IS_METEORLAKE(gt->i915) ||
>> +        IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) ||
>>           IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))
>>           pg_enable =
>>               GEN9_MEDIA_PG_ENABLE |
>>
>> ---
>> base-commit: 01d3dd92d1b71421f6ee85e1bea829e0a917d979
>> change-id: 20230517-mtl_disable_render_pg-b9f9f1567f9e
>
> ^ unwanted artifacts ?   Otherwise this looks good to me.

It is added by b4 tool, git deals with it correctly.

>
> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

Thx

Regards
Andrzej

>
>>
>> Best regards,
Nirmoy Das May 17, 2023, 3:36 p.m. UTC | #4 
On 5/17/2023 5:25 PM, Vivi, Rodrigo wrote:
> On Wed, 2023-05-17 at 17:12 +0200, Das, Nirmoy wrote:
>> On 5/17/2023 3:59 PM, Andrzej Hajda wrote:
>>> Multiple CI tests fails with forcewake ack timeouts
>>> if render power gating is enabled.
>>> BSpec 52698 clearly states it should be 0 for MTL.
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4983
>>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
>>> b/drivers/gpu/drm/i915/gt/intel_rc6.c
>>> index 908a3d0f2343f4..ebb2373dd73640 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
>>> @@ -117,8 +117,9 @@ static void gen11_rc6_enable(struct intel_rc6
>>> *rc6)
>>>                          GEN6_RC_CTL_RC6_ENABLE |
>>>                          GEN6_RC_CTL_EI_MODE(1);
>>>    
>>> -       /* Wa_16011777198 - Render powergating must remain disabled
>>> */
>>> -       if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
>>> ||
>>> +       /* Wa_16011777198 and BSpec 52698 - Render powergating must
>>> be off */
>> Nice catch!
> Indeed! What a mess in the workaround database.
> It is telling us that no_impact on MTL SKUs while we clearly needs
> that. I tried to reopen that and get that fixed in the hsds.
>
>
>>   instead of bspec you could add Wa_14012655556.
> not actually.
> 16011777198 is the right lineage number for 14012655556.
> Besides, 14012655556 is for DG2 anyway.
>
> Let's keep the way Adrzej put with the BSPec reference besides the
> lineage.

Makes sense, didn't realize 14012655556  is much older.

Thanks!

>
>>
>>> +       if (IS_METEORLAKE(gt->i915) ||
>>> +           IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
>>> ||
>>>              IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))
>>>                  pg_enable =
>>>                          GEN9_MEDIA_PG_ENABLE |
>>>
>>> ---
>>> base-commit: 01d3dd92d1b71421f6ee85e1bea829e0a917d979
>>> change-id: 20230517-mtl_disable_render_pg-b9f9f1567f9e
>> ^ unwanted artifacts ?   Otherwise this looks good to me.
>>
>> Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
> with the artifacts removed:
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>
>>> Best regards,
Rodrigo Vivi May 17, 2023, 8:09 p.m. UTC | #5 
On Wed, May 17, 2023 at 05:36:33PM +0200, Das, Nirmoy wrote:
> 
> On 5/17/2023 5:25 PM, Vivi, Rodrigo wrote:
> > On Wed, 2023-05-17 at 17:12 +0200, Das, Nirmoy wrote:
> > > On 5/17/2023 3:59 PM, Andrzej Hajda wrote:
> > > > Multiple CI tests fails with forcewake ack timeouts
> > > > if render power gating is enabled.
> > > > BSpec 52698 clearly states it should be 0 for MTL.
> > > > 
> > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4983
> > > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > > > ---
> > > >    drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +++--
> > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > index 908a3d0f2343f4..ebb2373dd73640 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > @@ -117,8 +117,9 @@ static void gen11_rc6_enable(struct intel_rc6
> > > > *rc6)
> > > >                          GEN6_RC_CTL_RC6_ENABLE |
> > > >                          GEN6_RC_CTL_EI_MODE(1);
> > > > -       /* Wa_16011777198 - Render powergating must remain disabled
> > > > */
> > > > -       if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
> > > > ||
> > > > +       /* Wa_16011777198 and BSpec 52698 - Render powergating must
> > > > be off */
> > > Nice catch!
> > Indeed! What a mess in the workaround database.
> > It is telling us that no_impact on MTL SKUs while we clearly needs
> > that. I tried to reopen that and get that fixed in the hsds.
> > 
> > 
> > >   instead of bspec you could add Wa_14012655556.
> > not actually.
> > 16011777198 is the right lineage number for 14012655556.
> > Besides, 14012655556 is for DG2 anyway.
> > 
> > Let's keep the way Adrzej put with the BSPec reference besides the
> > lineage.
> 
> Makes sense, didn't realize 14012655556  is much older.
> 
> Thanks!
> 
> > 
> > > 
> > > > +       if (IS_METEORLAKE(gt->i915) ||
> > > > +           IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
> > > > ||
> > > >              IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))
> > > >                  pg_enable =
> > > >                          GEN9_MEDIA_PG_ENABLE |
> > > > 
> > > > ---
> > > > base-commit: 01d3dd92d1b71421f6ee85e1bea829e0a917d979
> > > > change-id: 20230517-mtl_disable_render_pg-b9f9f1567f9e
> > > ^ unwanted artifacts ?   Otherwise this looks good to me.
> > > 
> > > Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
> > with the artifacts removed:
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Folks, please do not merge this patch.
At least not as it is right now...

We need to root cause this. The hw bug related to this workaround
was really fixed and this workaround should not be needed in MTL.

We need to find the root cause instead of masking it here.
Or at least merge as temporary FIXME/XXX and then work to
get the root cause...

The BSPec will get updated to remove the MTL mention there.

Sorry about that.

> > 
> > 
> > > > Best regards,
Andi Shyti May 18, 2023, 2:04 p.m. UTC | #6 
Hi Andrzej and Rodrigo,

> > On 5/17/2023 5:25 PM, Vivi, Rodrigo wrote:
> > > On Wed, 2023-05-17 at 17:12 +0200, Das, Nirmoy wrote:
> > > > On 5/17/2023 3:59 PM, Andrzej Hajda wrote:
> > > > > Multiple CI tests fails with forcewake ack timeouts
> > > > > if render power gating is enabled.
> > > > > BSpec 52698 clearly states it should be 0 for MTL.
> > > > > 
> > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4983
> > > > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +++--
> > > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > > b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > > index 908a3d0f2343f4..ebb2373dd73640 100644
> > > > > --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> > > > > @@ -117,8 +117,9 @@ static void gen11_rc6_enable(struct intel_rc6
> > > > > *rc6)
> > > > >                          GEN6_RC_CTL_RC6_ENABLE |
> > > > >                          GEN6_RC_CTL_EI_MODE(1);
> > > > > -       /* Wa_16011777198 - Render powergating must remain disabled
> > > > > */
> > > > > -       if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
> > > > > ||
> > > > > +       /* Wa_16011777198 and BSpec 52698 - Render powergating must
> > > > > be off */
> > > > Nice catch!
> > > Indeed! What a mess in the workaround database.
> > > It is telling us that no_impact on MTL SKUs while we clearly needs
> > > that. I tried to reopen that and get that fixed in the hsds.
> > > 
> > > 
> > > >   instead of bspec you could add Wa_14012655556.
> > > not actually.
> > > 16011777198 is the right lineage number for 14012655556.
> > > Besides, 14012655556 is for DG2 anyway.
> > > 
> > > Let's keep the way Adrzej put with the BSPec reference besides the
> > > lineage.
> > 
> > Makes sense, didn't realize 14012655556  is much older.
> > 
> > Thanks!
> > 
> > > 
> > > > 
> > > > > +       if (IS_METEORLAKE(gt->i915) ||
> > > > > +           IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0)
> > > > > ||
> > > > >              IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))
> > > > >                  pg_enable =
> > > > >                          GEN9_MEDIA_PG_ENABLE |
> > > > > 
> > > > > ---
> > > > > base-commit: 01d3dd92d1b71421f6ee85e1bea829e0a917d979
> > > > > change-id: 20230517-mtl_disable_render_pg-b9f9f1567f9e
> > > > ^ unwanted artifacts ?   Otherwise this looks good to me.
> > > > 
> > > > Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
> > > with the artifacts removed:
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Folks, please do not merge this patch.
> At least not as it is right now...
> 
> We need to root cause this. The hw bug related to this workaround
> was really fixed and this workaround should not be needed in MTL.
> 
> We need to find the root cause instead of masking it here.
> Or at least merge as temporary FIXME/XXX and then work to
> get the root cause...

I'm OK with merging this with the FIXME tag.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
index 908a3d0f2343f4..ebb2373dd73640 100644
--- a/drivers/gpu/drm/i915/gt/intel_rc6.c
+++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
@@ -117,8 +117,9 @@  static void gen11_rc6_enable(struct intel_rc6 *rc6)
 			GEN6_RC_CTL_RC6_ENABLE |
 			GEN6_RC_CTL_EI_MODE(1);
 
-	/* Wa_16011777198 - Render powergating must remain disabled */
-	if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) ||
+	/* Wa_16011777198 and BSpec 52698 - Render powergating must be off */
+	if (IS_METEORLAKE(gt->i915) ||
+	    IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_C0) ||
 	    IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0))
 		pg_enable =
 			GEN9_MEDIA_PG_ENABLE |