Re: [PATCH 12/13] drm/i915: remove in_dbg_master check from intel_fbc.c

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx