Re: [PATCH 47/51] drm/i915: Update ironlake_enable_rc6() to do explicit request management

[John Harrison <John.C.Harrison@Intel.com>]

On 25/02/2015 21:31, Daniel Vetter wrote:
> On Wed, Feb 18, 2015 at 02:28:16PM +0000, John Harrison wrote:
>> On 13/02/2015 17:03, Chris Wilson wrote:
>>> On Fri, Feb 13, 2015 at 04:58:24PM +0000, John Harrison wrote:
>>>> On 13/02/2015 12:19, Chris Wilson wrote:
>>>>> On Fri, Feb 13, 2015 at 11:48:56AM +0000, John.C.Harrison@Intel.com wrote:
>>>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>>>
>>>>>> Updated ironlake_enable_rc6() to do explicit request creation and submission.
>>>>> If you merged the context here with the common context switching code,
>>>>> we don't even need to touch the ring here.
>>>>> -Chris
>>>>>
>>>> It would certainly be preferable to not have any ring commands
>>>> written from deep within the power management code. However, I
>>>> didn't want to change anything I didn't really need to, especially
>>>> in code that I'm not at all sure about. Plus I don't have an
>>>> ironlake to test any significant change on.
>>> I did and tested extensively.
>>> -Chris
>> Do you have a patch that just does the move of this from PM code to context
>> switch code? Something that I can drop into this series would be great. If
>> not, exactly where about in the context switch code should it go? Should it
>> be in the start of day initialisation, in the per context intialisation,
>> every context switch, only the first switch after a resume, ...?
>>
>> Tracing back to where/when this code is currently executed seems to be quite
>> complicated. The _enable_rc6() function is called during ring reset but only
>> for Gen6+ because Ironlake is broken according to the comment. It is also
>> called by a system power management callback but it is unclear when that
>> would occur. Finally, it is also called from the display code in
>> intel_modeset_init_hw().
> ilk rc6 is disabled by default because it crashes machines hard and
> doesn't seem to be all that useful really. Compared to gen6+ rc6 which has
> a massive impact on idle power consumption, more with each generation.
>
> I've also never heard of anyone managing to make this work reliably. We
> could also just rip the entire code out I think, at least I wouldn't be
> surprised if it has bitrot completely by now.
> -Daniel

So just remove the ironlake_enable_rc6() function completely? Or keep it 
as a stub that just says '/* this has never worked so has been removed */'.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx