diff mbox

drm/i915/chv: WaDisablePwrmtrEvent:chv on CHV only

Message ID 1402333609-5782-1-git-send-email-Tom.O'Rourke@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tom.O'Rourke@intel.com June 9, 2014, 5:06 p.m. UTC 
From: Tom O'Rourke <Tom.O'Rourke@intel.com>

In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.

Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Lespiau, Damien June 9, 2014, 5:32 p.m. UTC | #1 
On Mon, Jun 09, 2014 at 10:06:49AM -0700, Tom.O'Rourke@intel.com wrote:
> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> 
> In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.
> 
> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>

A lovely catch.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Jani Nikula June 10, 2014, 3:30 p.m. UTC | #2 
On Mon, 09 Jun 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Mon, Jun 09, 2014 at 10:06:49AM -0700, Tom.O'Rourke@intel.com wrote:
>> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
>> 
>> In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.
>> 
>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
>
> A lovely catch.

Sadly gen8_enable_rps does not get called on chv, so the fix is wrong.

BR,
Jani.

>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>
> -- 
> Damien
>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index d9c5918..3d3e402 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3522,8 +3522,10 @@ static void gen8_enable_rps(struct drm_device *dev)
>>  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
>>  
>>  	/* WaDisablePwrmtrEvent:chv (pre-production hw) */
>> -	I915_WRITE(0xA80C, I915_READ(0xA80C) & 0x00ffffff);
>> -	I915_WRITE(0xA810, I915_READ(0xA810) & 0xffffff00);
>> +	if (IS_CHERRYVIEW(dev)) {
>> +		I915_WRITE(0xA80C, I915_READ(0xA80C) & 0x00ffffff);
>> +		I915_WRITE(0xA810, I915_READ(0xA810) & 0xffffff00);
>> +	}
>>  
>>  	/* 5: Enable RPS */
>>  	I915_WRITE(GEN6_RP_CONTROL,
>> -- 
>> 1.7.9.5
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula June 10, 2014, 3:34 p.m. UTC | #3 
On Tue, 10 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 09 Jun 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
>> On Mon, Jun 09, 2014 at 10:06:49AM -0700, Tom.O'Rourke@intel.com wrote:
>>> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
>>> 
>>> In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.
>>> 
>>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
>>
>> A lovely catch.
>
> Sadly gen8_enable_rps does not get called on chv, so the fix is wrong.

To elaborate, I think we need a patch dropping the wa altogether (which
we can queue for 3.15 through stable because the change affects
broadwell) and another patch, if needed, adding the wa in the chv
specific function.

Thanks,
Jani.


>
> BR,
> Jani.
>
>>
>> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>>
>> -- 
>> Damien
>>
>>> ---
>>>  drivers/gpu/drm/i915/intel_pm.c |    6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index d9c5918..3d3e402 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -3522,8 +3522,10 @@ static void gen8_enable_rps(struct drm_device *dev)
>>>  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
>>>  
>>>  	/* WaDisablePwrmtrEvent:chv (pre-production hw) */
>>> -	I915_WRITE(0xA80C, I915_READ(0xA80C) & 0x00ffffff);
>>> -	I915_WRITE(0xA810, I915_READ(0xA810) & 0xffffff00);
>>> +	if (IS_CHERRYVIEW(dev)) {
>>> +		I915_WRITE(0xA80C, I915_READ(0xA80C) & 0x00ffffff);
>>> +		I915_WRITE(0xA810, I915_READ(0xA810) & 0xffffff00);
>>> +	}
>>>  
>>>  	/* 5: Enable RPS */
>>>  	I915_WRITE(GEN6_RP_CONTROL,
>>> -- 
>>> 1.7.9.5
>>> 
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Jani Nikula, Intel Open Source Technology Center
Ville Syrjala June 10, 2014, 5:16 p.m. UTC | #4 
On Tue, Jun 10, 2014 at 06:34:18PM +0300, Jani Nikula wrote:
> On Tue, 10 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Mon, 09 Jun 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> >> On Mon, Jun 09, 2014 at 10:06:49AM -0700, Tom.O'Rourke@intel.com wrote:
> >>> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> >>> 
> >>> In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.
> >>> 
> >>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> >>
> >> A lovely catch.
> >
> > Sadly gen8_enable_rps does not get called on chv, so the fix is wrong.
> 
> To elaborate, I think we need a patch dropping the wa altogether (which
> we can queue for 3.15 through stable because the change affects
> broadwell) and another patch, if needed, adding the wa in the chv
> specific function.

This is just a merge mishap in one the chv patches. Someone just
needs to send a patch that moves the misapplied stuff to the
appropriate chv function.
Jani Nikula June 10, 2014, 6:17 p.m. UTC | #5 
On Tue, 10 Jun 2014, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Jun 10, 2014 at 06:34:18PM +0300, Jani Nikula wrote:
>> On Tue, 10 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > On Mon, 09 Jun 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
>> >> On Mon, Jun 09, 2014 at 10:06:49AM -0700, Tom.O'Rourke@intel.com wrote:
>> >>> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
>> >>> 
>> >>> In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.
>> >>> 
>> >>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
>> >>
>> >> A lovely catch.
>> >
>> > Sadly gen8_enable_rps does not get called on chv, so the fix is wrong.
>> 
>> To elaborate, I think we need a patch dropping the wa altogether (which
>> we can queue for 3.15 through stable because the change affects
>> broadwell) and another patch, if needed, adding the wa in the chv
>> specific function.
>
> This is just a merge mishap in one the chv patches. Someone just
> needs to send a patch that moves the misapplied stuff to the
> appropriate chv function.

Right. So my first comment was correct, and my elaboration total
bullcrap. This is not present in 3.15, but we've queued the screwup for
3.16. Thanks for the correction Ville.

BR,
Jani.



>
> -- 
> Ville Syrjälä
> Intel OTC
Tom.O'Rourke@intel.com June 10, 2014, 7:03 p.m. UTC | #6 
>> This is just a merge mishap in one the chv patches. Someone just needs

>> to send a patch that moves the misapplied stuff to the appropriate chv

>> function.

>

>Right. So my first comment was correct, and my elaboration total bullcrap. This

>is not present in 3.15, but we've queued the screwup for 3.16. Thanks for the

>correction Ville.

>

>BR,

>Jani.

>

>

>

>>

>> --

>> Ville Syrjälä

>> Intel OTC

>

>--

>Jani Nikula, Intel Open Source Technology Center

[TOR:] Hi Ville,
Do you want to be the someone to send the patch that moves the misapplied stuff?
I could guess where it goes but I do not have a CHV for testing the fix.
Thanks,
Tom
Ville Syrjala June 10, 2014, 7:15 p.m. UTC | #7 
On Tue, Jun 10, 2014 at 07:03:48PM +0000, O'Rourke, Tom wrote:
> >> This is just a merge mishap in one the chv patches. Someone just needs
> >> to send a patch that moves the misapplied stuff to the appropriate chv
> >> function.
> >
> >Right. So my first comment was correct, and my elaboration total bullcrap. This
> >is not present in 3.15, but we've queued the screwup for 3.16. Thanks for the
> >correction Ville.
> >
> >BR,
> >Jani.
> >
> >
> >
> >>
> >> --
> >> Ville Syrjälä
> >> Intel OTC
> >
> >--
> >Jani Nikula, Intel Open Source Technology Center
> [TOR:] Hi Ville,
> Do you want to be the someone to send the patch that moves the misapplied stuff?

Go ahead if you want to do it. Easy +1 to your patch statistics ;)

> I could guess where it goes

Or just look at the original patch:
http://patchwork.freedesktop.org/patch/23974/

> but I do not have a CHV for testing the fix.\

You're not alone in that. In any case the patch was never really
tested, except by the compiler :P
Jani Nikula June 12, 2014, 10:33 a.m. UTC | #8 
On Tue, 10 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 10 Jun 2014, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Tue, Jun 10, 2014 at 06:34:18PM +0300, Jani Nikula wrote:
>>> On Tue, 10 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>> > On Mon, 09 Jun 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
>>> >> On Mon, Jun 09, 2014 at 10:06:49AM -0700, Tom.O'Rourke@intel.com wrote:
>>> >>> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
>>> >>> 
>>> >>> In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.
>>> >>> 
>>> >>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
>>> >>
>>> >> A lovely catch.
>>> >
>>> > Sadly gen8_enable_rps does not get called on chv, so the fix is wrong.
>>> 
>>> To elaborate, I think we need a patch dropping the wa altogether (which
>>> we can queue for 3.15 through stable because the change affects
>>> broadwell) and another patch, if needed, adding the wa in the chv
>>> specific function.
>>
>> This is just a merge mishap in one the chv patches. Someone just
>> needs to send a patch that moves the misapplied stuff to the
>> appropriate chv function.
>
> Right. So my first comment was correct, and my elaboration total
> bullcrap. This is not present in 3.15, but we've queued the screwup for
> 3.16. Thanks for the correction Ville.

Argh. I'm really confusing myself and others here. Please bear with me.

So we've added

commit e4443e459ccf43f2c139358400365fd6a839d40d
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Wed Apr 9 13:28:41 2014 +0300

    drm/i915/chv: Add a bunch of pre production workarounds

which contains the chv specific w/a in bdw code. This is now going to
3.16, and we need to fix this for 3.16 through
drm-intel-fixes. Effectively the hunk touching gen8_enable_rps() from
Tom's new patch [1]. Right?

However the new patch from Tom moves those bits to
cherryview_enable_rps(), which is only present since

commit 38807746fa2ce44b79957ff07813d10fcaf3d311
Author: Deepak S <deepak.s@linux.intel.com>
Date:   Fri May 23 21:00:15 2014 +0530

    drm/i915/chv: Enable Render Standby (RC6) for Cherryview

and queued for 3.17. So we need another patch adding the bits to
cherryview_enable_rps() on top of drm-intel-next-queued, effectively the
second hunk from Tom's new patch. Right?

So Tom, please split your patch in two, one on top of drm-intel-fixes,
and another on top of drm-intel-next-queued, and I think we'll be fine.

BR,
Jani.


[1] http://mid.gmane.org/1402442794-166797-1-git-send-email-Tom.O'Rourke@intel.com


>
> BR,
> Jani.
>
>
>
>>
>> -- 
>> Ville Syrjälä
>> Intel OTC
>
> -- 
> Jani Nikula, Intel Open Source Technology Center
Daniel Vetter June 12, 2014, 12:42 p.m. UTC | #9 
On Thu, Jun 12, 2014 at 01:33:32PM +0300, Jani Nikula wrote:
> On Tue, 10 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Tue, 10 Jun 2014, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> On Tue, Jun 10, 2014 at 06:34:18PM +0300, Jani Nikula wrote:
> >>> On Tue, 10 Jun 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>> > On Mon, 09 Jun 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> >>> >> On Mon, Jun 09, 2014 at 10:06:49AM -0700, Tom.O'Rourke@intel.com wrote:
> >>> >>> From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> >>> >>> 
> >>> >>> In gen8_enable_rps, don't write CHV registers unless IS_CHERRYVIEW.
> >>> >>> 
> >>> >>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> >>> >>
> >>> >> A lovely catch.
> >>> >
> >>> > Sadly gen8_enable_rps does not get called on chv, so the fix is wrong.
> >>> 
> >>> To elaborate, I think we need a patch dropping the wa altogether (which
> >>> we can queue for 3.15 through stable because the change affects
> >>> broadwell) and another patch, if needed, adding the wa in the chv
> >>> specific function.
> >>
> >> This is just a merge mishap in one the chv patches. Someone just
> >> needs to send a patch that moves the misapplied stuff to the
> >> appropriate chv function.
> >
> > Right. So my first comment was correct, and my elaboration total
> > bullcrap. This is not present in 3.15, but we've queued the screwup for
> > 3.16. Thanks for the correction Ville.
> 
> Argh. I'm really confusing myself and others here. Please bear with me.
> 
> So we've added
> 
> commit e4443e459ccf43f2c139358400365fd6a839d40d
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Wed Apr 9 13:28:41 2014 +0300
> 
>     drm/i915/chv: Add a bunch of pre production workarounds
> 
> which contains the chv specific w/a in bdw code. This is now going to
> 3.16, and we need to fix this for 3.16 through
> drm-intel-fixes. Effectively the hunk touching gen8_enable_rps() from
> Tom's new patch [1]. Right?
> 
> However the new patch from Tom moves those bits to
> cherryview_enable_rps(), which is only present since
> 
> commit 38807746fa2ce44b79957ff07813d10fcaf3d311
> Author: Deepak S <deepak.s@linux.intel.com>
> Date:   Fri May 23 21:00:15 2014 +0530
> 
>     drm/i915/chv: Enable Render Standby (RC6) for Cherryview
> 
> and queued for 3.17. So we need another patch adding the bits to
> cherryview_enable_rps() on top of drm-intel-next-queued, effectively the
> second hunk from Tom's new patch. Right?
> 
> So Tom, please split your patch in two, one on top of drm-intel-fixes,
> and another on top of drm-intel-next-queued, and I think we'll be fine.

I've already merged Tom's patch to dinq since it's fully needed there. So
I think we just need a new patch version for -fixes only and resolve the
mess in a merge (shouldn't cause one). If we split the patch also for dinq
then I need to rebase/merge again. I guess you could simply apply Tom's
fix to -fixes and drop the unecessary hunk yourself.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d9c5918..3d3e402 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3522,8 +3522,10 @@  static void gen8_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
 
 	/* WaDisablePwrmtrEvent:chv (pre-production hw) */
-	I915_WRITE(0xA80C, I915_READ(0xA80C) & 0x00ffffff);
-	I915_WRITE(0xA810, I915_READ(0xA810) & 0xffffff00);
+	if (IS_CHERRYVIEW(dev)) {
+		I915_WRITE(0xA80C, I915_READ(0xA80C) & 0x00ffffff);
+		I915_WRITE(0xA810, I915_READ(0xA810) & 0xffffff00);
+	}
 
 	/* 5: Enable RPS */
 	I915_WRITE(GEN6_RP_CONTROL,