diff mbox series

drm/i915: Skip Bit12 fw domain reset for gen12+

Message ID 20220817224304.255767-1-radhakrishna.sripada@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Skip Bit12 fw domain reset for gen12+ | expand

Commit Message

Sripada, Radhakrishna Aug. 17, 2022, 10:43 p.m. UTC 
Bit12 of the Forcewake request register should not be cleared post
gen12. Do not touch this bit while clearing during fw domain reset.

Bspec: 52542

Signed-off-by: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Matt Roper Aug. 18, 2022, 6 a.m. UTC | #1 
On Wed, Aug 17, 2022 at 03:43:04PM -0700, Radhakrishna Sripada wrote:
> Bit12 of the Forcewake request register should not be cleared post
> gen12. Do not touch this bit while clearing during fw domain reset.
> 
> Bspec: 52542
> 
> Signed-off-by: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index a852c471d1b3..c85e2b686c95 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -113,7 +113,10 @@ fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
>  	 * off in ICL+), so no waiting for acks
>  	 */
>  	/* WaRsClearFWBitsAtReset:bdw,skl */

While we're at it, let's remove the "bdw,skl" from this comment since
it's misleading and doesn't match the code.  We do still apply this
workaround on other pre-gen12 platforms than just those two.

Aside from the comment tweak,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> -	fw_clear(d, 0xffff);
> +	if (GRAPHICS_VER(d->uncore->i915) >= 12)
> +		fw_clear(d, 0xefff);
> +	else
> +		fw_clear(d, 0xffff);
>  }
>  
>  static inline void
> -- 
> 2.25.1
>
Gwan-gyeong Mun Aug. 24, 2022, 6:14 a.m. UTC | #2 
On 8/18/22 3:00 PM, Matt Roper wrote:
> On Wed, Aug 17, 2022 at 03:43:04PM -0700, Radhakrishna Sripada wrote:
>> Bit12 of the Forcewake request register should not be cleared post
>> gen12. Do not touch this bit while clearing during fw domain reset.
>>
>> Bspec: 52542
>>
>> Signed-off-by: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index a852c471d1b3..c85e2b686c95 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -113,7 +113,10 @@ fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
>>   	 * off in ICL+), so no waiting for acks
>>   	 */
>>   	/* WaRsClearFWBitsAtReset:bdw,skl */
> 
> While we're at it, let's remove the "bdw,skl" from this comment since
> it's misleading and doesn't match the code.  We do still apply this
> workaround on other pre-gen12 platforms than just those two.
> 
> Aside from the comment tweak,
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
>> -	fw_clear(d, 0xffff);
>> +	if (GRAPHICS_VER(d->uncore->i915) >= 12)
Hi Radhakrishna Sripada,

In bspec 52542, there is an explanation that BIT12 should not be set for 
address 0xA188 corresponding to FORCEWAKE_MT/FORCEWAKE_GT_GEN9, but in 
bspec 52466, there is no explanation that BIT12 should not be set for 
address 0xA278, address of FORCEWAKE_RENDER_GEN9.

I ask if fw_domain_reset() should perform fw_clear() by comparing not 
only GRAPHICS_VER() >= 12 but also checking of FW_DOMAIN_ID_RENDER and 
FW_DOMAIN_ID_GT values.

Br,
G.G.
>> +		fw_clear(d, 0xefff);
>> +	else
>> +		fw_clear(d, 0xffff);
>>   }
>>   
>>   static inline void
>> -- 
>> 2.25.1
>>
>
Sripada, Radhakrishna Aug. 25, 2022, 5:49 p.m. UTC | #3 
Hi G.G,

> -----Original Message-----
> From: Mun, Gwan-gyeong <gwan-gyeong.mun@intel.com>
> Sent: Tuesday, August 23, 2022 11:14 PM
> To: Roper, Matthew D <matthew.d.roper@intel.com>; Sripada, Radhakrishna
> <radhakrishna.sripada@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Skip Bit12 fw domain reset for gen12+
> 
> 
> 
> On 8/18/22 3:00 PM, Matt Roper wrote:
> > On Wed, Aug 17, 2022 at 03:43:04PM -0700, Radhakrishna Sripada wrote:
> >> Bit12 of the Forcewake request register should not be cleared post
> >> gen12. Do not touch this bit while clearing during fw domain reset.
> >>
> >> Bspec: 52542
> >>
> >> Signed-off-by: Sushma Venkatesh Reddy
> <sushma.venkatesh.reddy@intel.com>
> >> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
> >>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> >> index a852c471d1b3..c85e2b686c95 100644
> >> --- a/drivers/gpu/drm/i915/intel_uncore.c
> >> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> >> @@ -113,7 +113,10 @@ fw_domain_reset(const struct
> intel_uncore_forcewake_domain *d)
> >>   	 * off in ICL+), so no waiting for acks
> >>   	 */
> >>   	/* WaRsClearFWBitsAtReset:bdw,skl */
> >
> > While we're at it, let's remove the "bdw,skl" from this comment since
> > it's misleading and doesn't match the code.  We do still apply this
> > workaround on other pre-gen12 platforms than just those two.
> >
> > Aside from the comment tweak,
> >
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> >
> >> -	fw_clear(d, 0xffff);
> >> +	if (GRAPHICS_VER(d->uncore->i915) >= 12)
> Hi Radhakrishna Sripada,
> 
> In bspec 52542, there is an explanation that BIT12 should not be set for
> address 0xA188 corresponding to FORCEWAKE_MT/FORCEWAKE_GT_GEN9, but
> in
> bspec 52466, there is no explanation that BIT12 should not be set for
> address 0xA278, address of FORCEWAKE_RENDER_GEN9.
> 
> I ask if fw_domain_reset() should perform fw_clear() by comparing not
> only GRAPHICS_VER() >= 12 but also checking of FW_DOMAIN_ID_RENDER and
> FW_DOMAIN_ID_GT values.
Based on the note in 52542, all other WA notes are overridden by the comment. So unless stated
otherwise, it should apply to this register as well.

Created a bspec issue to request for additional clarification just to be safe. Will send an additional
patch if the comment contradicts our understanding. 

Thanks,
RK
> 
> Br,
> G.G.
> >> +		fw_clear(d, 0xefff);
> >> +	else
> >> +		fw_clear(d, 0xffff);
> >>   }
> >>
> >>   static inline void
> >> --
> >> 2.25.1
> >>
> >
Tvrtko Ursulin Dec. 15, 2022, 5:38 p.m. UTC | #4 
On 25/08/2022 18:49, Sripada, Radhakrishna wrote:
> Hi G.G,
> 
>> -----Original Message-----
>> From: Mun, Gwan-gyeong <gwan-gyeong.mun@intel.com>
>> Sent: Tuesday, August 23, 2022 11:14 PM
>> To: Roper, Matthew D <matthew.d.roper@intel.com>; Sripada, Radhakrishna
>> <radhakrishna.sripada@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Skip Bit12 fw domain reset for gen12+
>>
>>
>>
>> On 8/18/22 3:00 PM, Matt Roper wrote:
>>> On Wed, Aug 17, 2022 at 03:43:04PM -0700, Radhakrishna Sripada wrote:
>>>> Bit12 of the Forcewake request register should not be cleared post
>>>> gen12. Do not touch this bit while clearing during fw domain reset.
>>>>
>>>> Bspec: 52542
>>>>
>>>> Signed-off-by: Sushma Venkatesh Reddy
>> <sushma.venkatesh.reddy@intel.com>
>>>> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>> b/drivers/gpu/drm/i915/intel_uncore.c
>>>> index a852c471d1b3..c85e2b686c95 100644
>>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>>> @@ -113,7 +113,10 @@ fw_domain_reset(const struct
>> intel_uncore_forcewake_domain *d)
>>>>    	 * off in ICL+), so no waiting for acks
>>>>    	 */
>>>>    	/* WaRsClearFWBitsAtReset:bdw,skl */
>>>
>>> While we're at it, let's remove the "bdw,skl" from this comment since
>>> it's misleading and doesn't match the code.  We do still apply this
>>> workaround on other pre-gen12 platforms than just those two.
>>>
>>> Aside from the comment tweak,
>>>
>>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>>
>>>> -	fw_clear(d, 0xffff);
>>>> +	if (GRAPHICS_VER(d->uncore->i915) >= 12)
>> Hi Radhakrishna Sripada,
>>
>> In bspec 52542, there is an explanation that BIT12 should not be set for
>> address 0xA188 corresponding to FORCEWAKE_MT/FORCEWAKE_GT_GEN9, but
>> in
>> bspec 52466, there is no explanation that BIT12 should not be set for
>> address 0xA278, address of FORCEWAKE_RENDER_GEN9.
>>
>> I ask if fw_domain_reset() should perform fw_clear() by comparing not
>> only GRAPHICS_VER() >= 12 but also checking of FW_DOMAIN_ID_RENDER and
>> FW_DOMAIN_ID_GT values.
> Based on the note in 52542, all other WA notes are overridden by the comment. So unless stated
> otherwise, it should apply to this register as well.
> 
> Created a bspec issue to request for additional clarification just to be safe. Will send an additional
> patch if the comment contradicts our understanding.

How important was this patch - should it be sent to stable?

Regards,

Tvrtko
Matt Roper Dec. 15, 2022, 5:57 p.m. UTC | #5 
On Thu, Dec 15, 2022 at 05:38:22PM +0000, Tvrtko Ursulin wrote:
> 
> On 25/08/2022 18:49, Sripada, Radhakrishna wrote:
> > Hi G.G,
> > 
> > > -----Original Message-----
> > > From: Mun, Gwan-gyeong <gwan-gyeong.mun@intel.com>
> > > Sent: Tuesday, August 23, 2022 11:14 PM
> > > To: Roper, Matthew D <matthew.d.roper@intel.com>; Sripada, Radhakrishna
> > > <radhakrishna.sripada@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Skip Bit12 fw domain reset for gen12+
> > > 
> > > 
> > > 
> > > On 8/18/22 3:00 PM, Matt Roper wrote:
> > > > On Wed, Aug 17, 2022 at 03:43:04PM -0700, Radhakrishna Sripada wrote:
> > > > > Bit12 of the Forcewake request register should not be cleared post
> > > > > gen12. Do not touch this bit while clearing during fw domain reset.
> > > > > 
> > > > > Bspec: 52542
> > > > > 
> > > > > Signed-off-by: Sushma Venkatesh Reddy
> > > <sushma.venkatesh.reddy@intel.com>
> > > > > Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
> > > > >    1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > > b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > index a852c471d1b3..c85e2b686c95 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > @@ -113,7 +113,10 @@ fw_domain_reset(const struct
> > > intel_uncore_forcewake_domain *d)
> > > > >    	 * off in ICL+), so no waiting for acks
> > > > >    	 */
> > > > >    	/* WaRsClearFWBitsAtReset:bdw,skl */
> > > > 
> > > > While we're at it, let's remove the "bdw,skl" from this comment since
> > > > it's misleading and doesn't match the code.  We do still apply this
> > > > workaround on other pre-gen12 platforms than just those two.
> > > > 
> > > > Aside from the comment tweak,
> > > > 
> > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > > 
> > > > > -	fw_clear(d, 0xffff);
> > > > > +	if (GRAPHICS_VER(d->uncore->i915) >= 12)
> > > Hi Radhakrishna Sripada,
> > > 
> > > In bspec 52542, there is an explanation that BIT12 should not be set for
> > > address 0xA188 corresponding to FORCEWAKE_MT/FORCEWAKE_GT_GEN9, but
> > > in
> > > bspec 52466, there is no explanation that BIT12 should not be set for
> > > address 0xA278, address of FORCEWAKE_RENDER_GEN9.
> > > 
> > > I ask if fw_domain_reset() should perform fw_clear() by comparing not
> > > only GRAPHICS_VER() >= 12 but also checking of FW_DOMAIN_ID_RENDER and
> > > FW_DOMAIN_ID_GT values.
> > Based on the note in 52542, all other WA notes are overridden by the comment. So unless stated
> > otherwise, it should apply to this register as well.
> > 
> > Created a bspec issue to request for additional clarification just to be safe. Will send an additional
> > patch if the comment contradicts our understanding.
> 
> How important was this patch - should it be sent to stable?

It shouldn't be needed for stable; clearing the bit isn't necessary from
gen12 onward, but as far as we know it doesn't cause any problems either
(except maybe on MTL, which is still under force_probe and not supported
on stable kernels).  So according to stable-kernel-rules.rst, this patch
would not qualify.


Matt

> 
> Regards,
> 
> Tvrtko
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index a852c471d1b3..c85e2b686c95 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -113,7 +113,10 @@  fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
 	 * off in ICL+), so no waiting for acks
 	 */
 	/* WaRsClearFWBitsAtReset:bdw,skl */
-	fw_clear(d, 0xffff);
+	if (GRAPHICS_VER(d->uncore->i915) >= 12)
+		fw_clear(d, 0xefff);
+	else
+		fw_clear(d, 0xffff);
 }
 
 static inline void