From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 2/5] drm/i915: preserve swizzle settings if necessary v3 Date: Wed, 11 Jun 2014 08:13:45 -0700 Message-ID: <20140611081345.2d805e8c@jbarnes-desktop> References: <1401992671-2548-1-git-send-email-jbarnes@virtuousgeek.org> <1401992671-2548-2-git-send-email-jbarnes@virtuousgeek.org> <20140610140251.GP5821@phenom.ffwll.local> <20140610102711.171104dc@jbarnes-desktop> <20140610193326.GB5821@phenom.ffwll.local> <20140610124538.350e86ba@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: intel-gfx , dri-devel List-Id: dri-devel@lists.freedesktop.org On Wed, 11 Jun 2014 11:23:26 +0200 Daniel Vetter wrote: > On Tue, Jun 10, 2014 at 12:45:38PM -0700, Jesse Barnes wrote: > > On Tue, 10 Jun 2014 21:33:27 +0200 > > Daniel Vetter wrote: > > > > > On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes wrote: > > > > Yes, that's what I do below... I even added it to the changelog: > > > > http://patchwork.freedesktop.org/patch/27223/ > > > > > > > > Did you miss the later hunk in intel_display.c? > > > > > > > > What we try to do here is enable swizzling if possible, which we can do > > > > if no inherited fbs are tiled. > > > > > > > > So I think I've done exactly what you repeated above, and documented > > > > it. So you're going to need to repeat it with different words so I can > > > > understand, if I'm still missing something. > > > > > > In swizzle_detect: > > > > > > ... > > > > > > if (GEN6+) { > > > if (preserve_bios_swizzle) { > > > if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) { > > > swizzle_x = I915_BIT_6_SWIZZLE_9_10; > > > ... > > > } else { > > > swizzle_x = I915_BIT_6_SWIZZLE_NONE; > > > ... > > > } > > > } else { > > > /* existing/old logic to decide about swizzling */ > > > } > > > } > > > > > > ... > > > > > > Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use > > > a small helper function to compute preserve_bios_swizzle instead of > > > storing it in dev_priv (since we will only use it at exactly one place), > > > but that's a pure style preference. > > > > Doesn't this amount to the same thing? I.e. we enable it if possible, > > otherwise just report it as unswizzled? So you're just wanting a style > > change here? > > So presuming I read your code correctly there's two issues: > > - The first thing you check in swizzle_detect is the hw swizzle bit in > DSP_ARB. If that's not set you force unswizzled. Since most BIOS don't > bother to set this (they use an untiled buffer after all) this means > you've effectively disabled swizzling on almost all machines. The goal > of keeping the old logic was that we actually want to enable swizzling > in some situations. Ah damn, I had been thinking that bit_6_swizzle was the runtime call, and that the init_swizzle call would go ahead and set it up properly. I see that's too late though. > - If you have a machine which uses tiled framebuffers and enables > swizzling in the BIOS your code will a) drop the swizzle setup in > gem_init_hw, breaking resume b) not set the swizzle settings correctly > in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such > a machine exists, but still. This would affect krh's MBA, which is why I wanted testing here... anyway I'll spin a new one and ask krh to test again. -- Jesse Barnes, Intel Open Source Technology Center