From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH] drm/i915: don't double-restore ARB mode bits for kms Date: Wed, 05 Feb 2014 15:01:25 +0200 Message-ID: <878utp3e8q.fsf@intel.com> References: <1389887373-3547-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 6949F106B71 for ; Wed, 5 Feb 2014 04:57:46 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter , Paulo Zanoni Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, 16 Jan 2014, Daniel Vetter wrote: > On Thu, Jan 16, 2014 at 5:54 PM, Paulo Zanoni wrote: >> 2014/1/16 Daniel Vetter : >>> 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? BR, Jani. -- Jani Nikula, Intel Open Source Technology Center