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

[Daniel Vetter]

On Fri, Feb 27, 2015 at 12:49:11PM +0000, John Harrison wrote:
> 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 at Intel.com wrote:
> >>>>>>From: John Harrison <John.C.Harrison at 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 */'.

Just remove it entirely, together with all the other ironlake rc6 stuff
plus functions only called by it. All the ironlake_*_rc6 stuff
essentially. ironlake_*_drps is the turbo support, that part needs to
stay.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch