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