[Intel-gfx] [PATCH] drm/i915: don't double-restore ARB mode bits for kms

[Daniel Vetter]

On Wed, Feb 5, 2014 at 2:01 PM, Jani Nikula <jani.nikula at linux.intel.com> wrote:
> On Thu, 16 Jan 2014, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>> On Thu, Jan 16, 2014 at 5:54 PM, Paulo Zanoni <przanoni at gmail.com> wrote:
>>> 2014/1/16 Daniel Vetter <daniel.vetter at ffwll.ch>:
>>>> Our init_clock_gating functions and related code should already take
>>>> care of this. And if they don't we'd better know.
>>>
>>> For both registers, I see functions applying specific workarounds, but
>>> they only do the read-write-modify through _MASKED_BIT_ENABLE(). I
>>> don't see anybody explicitly fully initializing the registers
>>> anywhere. So we go with whatever the BIOS gives us at boot + our
>>> changes to a few specific registers. At resume, we don't know so far
>>> what we'll get, so I fear this patch may in fact cause a regression.
>>
>> That's part of the risk of it, but fixing those is a simple matter of
>> comparing register dumps. These two bits here are one of the very few
>> holdouts we have that depend upon the register save/restore code, and
>> I want to remove them. Since thus far this was a really good way to
>> hide random bugs.
>
> I'm in favour of the patch in general, but I spotted at least
>
>         /* On GEN3 we really need to make sure the ARB C3 LP bit is set */
>         if (IS_GEN3(dev)) {
>                 I915_WRITE(MI_ARB_STATE,
>                            _MASKED_BIT_ENABLE(MI_ARB_C3_LP_WRITE_ENABLE));
>         }
>
> in i915_gem_load() that won't be set on a suspend/resume cycle. Is that
> going to be problem?

Yes, good catch. Somehow I've missed this when grepping.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch