| Message ID | 1446664257-32012-13-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/04/2015 11:10 AM, Paulo Zanoni wrote: > From our maintainer Daniel Vetter a few days ago: > "Oh dear this is dead code. kdbg uses the fbcon, which always uses > untiled, which means fbc will never be enabled. Also we have 0 users > and 0 test coverage for kdbg on top of i915 (Jesse implemented it > for fun years back). Imo just remove all this code." > > Adding to what Daniel said: for kgdboc's KMS support, > intel_pipe_set_base_atomic() already manually disables FBC, so we > won't do the in_dbg_master() check there. This is essentially a revert > of: > > commit c924b934d0cd14a4559611da91f28f59acebe32a > Author: Jason Wessel <jason.wessel@windriver.com> > Date: Thu Aug 5 09:22:32 2010 -0500 > i915: when kgdb is active display compression should be off > > Besides, it is not clear what is the exact problem caused by FBC, and > why other features such as PSR, DRRS, IPS and RPM are not also > checking for in_dbg_master(). IMHO we should either remove the code as > suggested by Daniel or we add some nice comments explaining why is FBC > so special. > > v2: Rebase due to new patch order. > > Cc: Jason Wessel <jason.wessel@windriver.com> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 8e806be..e496cb0 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) > goto out_disable; > } > > - /* If the kernel debugger is active, always disable compression */ > - if (in_dbg_master()) { > - set_no_fbc_reason(dev_priv, "Kernel debugger is active"); > - goto out_disable; > - } > - > /* WaFbcExceedCdClockThreshold:hsw,bdw */ > if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) && > ilk_pipe_pixel_rate(crtc->config) >= > Yeah looks fine. I haven't had any bug reports from the kdboc work, so I guess that means no one is using it. :) Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On 11/04/2015 02:13 PM, Jesse Barnes wrote: > On 11/04/2015 11:10 AM, Paulo Zanoni wrote: >> From our maintainer Daniel Vetter a few days ago: >> "Oh dear this is dead code. kdbg uses the fbcon, which always uses >> untiled, which means fbc will never be enabled. Also we have 0 users >> and 0 test coverage for kdbg on top of i915 (Jesse implemented it >> for fun years back). Imo just remove all this code." >> >> Adding to what Daniel said: for kgdboc's KMS support, >> intel_pipe_set_base_atomic() already manually disables FBC, so we >> won't do the in_dbg_master() check there. This is essentially a revert >> of: >> >> commit c924b934d0cd14a4559611da91f28f59acebe32a >> Author: Jason Wessel <jason.wessel@windriver.com> >> Date: Thu Aug 5 09:22:32 2010 -0500 >> i915: when kgdb is active display compression should be off >> >> Besides, it is not clear what is the exact problem caused by FBC, and >> why other features such as PSR, DRRS, IPS and RPM are not also >> checking for in_dbg_master(). IMHO we should either remove the code as >> suggested by Daniel or we add some nice comments explaining why is FBC >> so special. >> >> v2: Rebase due to new patch order. >> >> Cc: Jason Wessel <jason.wessel@windriver.com> >> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_fbc.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> index 8e806be..e496cb0 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) >> goto out_disable; >> } >> >> - /* If the kernel debugger is active, always disable compression */ >> - if (in_dbg_master()) { >> - set_no_fbc_reason(dev_priv, "Kernel debugger is active"); >> - goto out_disable; >> - } >> - >> /* WaFbcExceedCdClockThreshold:hsw,bdw */ >> if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) && >> ilk_pipe_pixel_rate(crtc->config) >= >> > Yeah looks fine. I haven't had any bug reports from the kdboc work, so > I guess that means no one is using it. :) > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> It was previously the case that the code here only got hit when you had a oops or a panic while running with the graphics console up. We would end up with fuzzy lines instead of a readable text console when we activated the atomic mode set. The kgdboc probably doesn't get used a whole lot but I have debugged a few strange WiFi and PM problems with it on a laptop and it worked well. :-) Cheers, Jason.
Em Qua, 2015-11-04 às 14:19 -0600, Jason Wessel escreveu: > On 11/04/2015 02:13 PM, Jesse Barnes wrote: > > On 11/04/2015 11:10 AM, Paulo Zanoni wrote: > > > From our maintainer Daniel Vetter a few days ago: > > > "Oh dear this is dead code. kdbg uses the fbcon, which always > > > uses > > > untiled, which means fbc will never be enabled. Also we have 0 > > > users > > > and 0 test coverage for kdbg on top of i915 (Jesse implemented > > > it > > > for fun years back). Imo just remove all this code." > > > > > > Adding to what Daniel said: for kgdboc's KMS support, > > > intel_pipe_set_base_atomic() already manually disables FBC, so we > > > won't do the in_dbg_master() check there. This is essentially a > > > revert > > > of: > > > > > > commit c924b934d0cd14a4559611da91f28f59acebe32a > > > Author: Jason Wessel <jason.wessel@windriver.com> > > > Date: Thu Aug 5 09:22:32 2010 -0500 > > > i915: when kgdb is active display compression should be off > > > > > > Besides, it is not clear what is the exact problem caused by FBC, > > > and > > > why other features such as PSR, DRRS, IPS and RPM are not also > > > checking for in_dbg_master(). IMHO we should either remove the > > > code as > > > suggested by Daniel or we add some nice comments explaining why > > > is FBC > > > so special. > > > > > > v2: Rebase due to new patch order. > > > > > > Cc: Jason Wessel <jason.wessel@windriver.com> > > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_fbc.c | 6 ------ > > > 1 file changed, 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > > > b/drivers/gpu/drm/i915/intel_fbc.c > > > index 8e806be..e496cb0 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct > > > drm_i915_private *dev_priv) > > > goto out_disable; > > > } > > > > > > - /* If the kernel debugger is active, always disable > > > compression */ > > > - if (in_dbg_master()) { > > > - set_no_fbc_reason(dev_priv, "Kernel debugger is > > > active"); > > > - goto out_disable; > > > - } > > > - > > > /* WaFbcExceedCdClockThreshold:hsw,bdw */ > > > if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) && > > > ilk_pipe_pixel_rate(crtc->config) >= > > > > > Yeah looks fine. I haven't had any bug reports from the kdboc > > work, so > > I guess that means no one is using it. :) > > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > It was previously the case that the code here only got hit when you > had a oops or a panic while running with the graphics console up. We > would end up with fuzzy lines instead of a readable text console when > we activated the atomic mode set. But on this case we'll call pipe_set_base_atomic(), which will disable FBC before doing the modeset. Maybe this was not the case in the past..? > > The kgdboc probably doesn't get used a whole lot but I have debugged > a few strange WiFi and PM problems with it on a laptop and it worked > well. :-) > Cheers, > Jason.
On 11/04/2015 12:26 PM, Zanoni, Paulo R wrote: > Em Qua, 2015-11-04 às 14:19 -0600, Jason Wessel escreveu: >> On 11/04/2015 02:13 PM, Jesse Barnes wrote: >>> On 11/04/2015 11:10 AM, Paulo Zanoni wrote: >>>> From our maintainer Daniel Vetter a few days ago: >>>> "Oh dear this is dead code. kdbg uses the fbcon, which always >>>> uses >>>> untiled, which means fbc will never be enabled. Also we have 0 >>>> users >>>> and 0 test coverage for kdbg on top of i915 (Jesse implemented >>>> it >>>> for fun years back). Imo just remove all this code." >>>> >>>> Adding to what Daniel said: for kgdboc's KMS support, >>>> intel_pipe_set_base_atomic() already manually disables FBC, so we >>>> won't do the in_dbg_master() check there. This is essentially a >>>> revert >>>> of: >>>> >>>> commit c924b934d0cd14a4559611da91f28f59acebe32a >>>> Author: Jason Wessel <jason.wessel@windriver.com> >>>> Date: Thu Aug 5 09:22:32 2010 -0500 >>>> i915: when kgdb is active display compression should be off >>>> >>>> Besides, it is not clear what is the exact problem caused by FBC, >>>> and >>>> why other features such as PSR, DRRS, IPS and RPM are not also >>>> checking for in_dbg_master(). IMHO we should either remove the >>>> code as >>>> suggested by Daniel or we add some nice comments explaining why >>>> is FBC >>>> so special. >>>> >>>> v2: Rebase due to new patch order. >>>> >>>> Cc: Jason Wessel <jason.wessel@windriver.com> >>>> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_fbc.c | 6 ------ >>>> 1 file changed, 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c >>>> b/drivers/gpu/drm/i915/intel_fbc.c >>>> index 8e806be..e496cb0 100644 >>>> --- a/drivers/gpu/drm/i915/intel_fbc.c >>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c >>>> @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct >>>> drm_i915_private *dev_priv) >>>> goto out_disable; >>>> } >>>> >>>> - /* If the kernel debugger is active, always disable >>>> compression */ >>>> - if (in_dbg_master()) { >>>> - set_no_fbc_reason(dev_priv, "Kernel debugger is >>>> active"); >>>> - goto out_disable; >>>> - } >>>> - >>>> /* WaFbcExceedCdClockThreshold:hsw,bdw */ >>>> if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) && >>>> ilk_pipe_pixel_rate(crtc->config) >= >>>> >>> Yeah looks fine. I haven't had any bug reports from the kdboc >>> work, so >>> I guess that means no one is using it. :) >>> >>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> >> >> It was previously the case that the code here only got hit when you >> had a oops or a panic while running with the graphics console up. We >> would end up with fuzzy lines instead of a readable text console when >> we activated the atomic mode set. > > But on this case we'll call pipe_set_base_atomic(), which will disable > FBC before doing the modeset. Maybe this was not the case in the > past..? Or some subsequent call re-enabled it somehow. You could replace it with a WARN and keep my r-b if you want, or just push as-is. But my guess is that kgdboc hasn't survived all the other modeset work we've done the past couple of years... I guess we need some igt tests and a bunch of fixups to keep it going. We could address this problem if/when that happens. Jesse
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 8e806be..e496cb0 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -890,12 +890,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) goto out_disable; } - /* If the kernel debugger is active, always disable compression */ - if (in_dbg_master()) { - set_no_fbc_reason(dev_priv, "Kernel debugger is active"); - goto out_disable; - } - /* WaFbcExceedCdClockThreshold:hsw,bdw */ if ((IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) && ilk_pipe_pixel_rate(crtc->config) >=
From our maintainer Daniel Vetter a few days ago: "Oh dear this is dead code. kdbg uses the fbcon, which always uses untiled, which means fbc will never be enabled. Also we have 0 users and 0 test coverage for kdbg on top of i915 (Jesse implemented it for fun years back). Imo just remove all this code." Adding to what Daniel said: for kgdboc's KMS support, intel_pipe_set_base_atomic() already manually disables FBC, so we won't do the in_dbg_master() check there. This is essentially a revert of: commit c924b934d0cd14a4559611da91f28f59acebe32a Author: Jason Wessel <jason.wessel@windriver.com> Date: Thu Aug 5 09:22:32 2010 -0500 i915: when kgdb is active display compression should be off Besides, it is not clear what is the exact problem caused by FBC, and why other features such as PSR, DRRS, IPS and RPM are not also checking for in_dbg_master(). IMHO we should either remove the code as suggested by Daniel or we add some nice comments explaining why is FBC so special. v2: Rebase due to new patch order. Cc: Jason Wessel <jason.wessel@windriver.com> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 6 ------ 1 file changed, 6 deletions(-)