From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH 12/26] drm/i915: Page table helpers,
 and define renames
Date: Tue, 18 Mar 2014 09:05:58 +0000
Message-ID: <20140318090558.GD18530@nuc-i3427.alporthouse.com>
References: <1395121738-29126-1-git-send-email-benjamin.widawsky@intel.com>
 <1395121738-29126-13-git-send-email-benjamin.widawsky@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces@lists.freedesktop.org>
Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118])
 by gabe.freedesktop.org (Postfix) with ESMTP id 5A71A6E538
 for <intel-gfx@lists.freedesktop.org>; Tue, 18 Mar 2014 02:06:02 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <1395121738-29126-13-git-send-email-benjamin.widawsky@intel.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
 <mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
 <mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
To: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
List-Id: intel-gfx@lists.freedesktop.org

On Mon, Mar 17, 2014 at 10:48:44PM -0700, Ben Widawsky wrote:
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -1,8 +1,11 @@
>  #ifndef _I915_GEM_GTT_H
>  #define _I915_GEM_GTT_H
>  
> -#define GEN6_PPGTT_PD_ENTRIES 512
> -#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> +/* GEN Agnostic defines */
> +#define I915_PDES_PER_PD		512
> +#define I915_PTE_MASK			(PAGE_SHIFT-1)

That looks decidely fishy.

PAGE_SHIFT is 12 -> PTE_MASK = 0xb

> +#define I915_PDE_MASK			(I915_PDES_PER_PD-1)
> +
>  typedef uint32_t gen6_gtt_pte_t;
>  typedef uint64_t gen8_gtt_pte_t;
>  typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
> @@ -23,6 +26,98 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>  #define GEN6_PTE_ADDR_ENCODE(addr)	GEN6_GTT_ADDR_ENCODE(addr)
>  #define HSW_PTE_ADDR_ENCODE(addr)	HSW_GTT_ADDR_ENCODE(addr)
>  
> +
> +/* GEN6 PPGTT resembles a 2 level page table:
> + * 31:22 | 21:12 |  11:0
> + *  PDE  |  PTE  | offset
> + */
> +#define GEN6_PDE_SHIFT			22
> +#define GEN6_PTES_PER_PT		(PAGE_SIZE / sizeof(gen6_gtt_pte_t))
> +
> +static inline uint32_t i915_pte_index(uint64_t address, uint32_t pde_shift)
> +{
> +	const uint32_t mask = (1 << (pde_shift - PAGE_SHIFT)) - 1;
> +	return (address >> PAGE_SHIFT) & mask;
> +}
> +
> +/* Helper to counts the number of PTEs within the given length. This count does
> + * not cross a page table boundary, so the max value would be
> + * GEN6_PTES_PER_PT for GEN6, and GEN8_PTES_PER_PT for GEN8.
> + */
> +static inline size_t i915_pte_count(uint64_t addr, size_t length,
> +				    uint32_t pde_shift)
> +{
> +	const uint64_t pd_mask = ~((1 << pde_shift) - 1);
> +	uint64_t end;
> +
> +	if (WARN_ON(!length))
> +		return 0;
> +
> +	if (WARN_ON(addr % PAGE_SIZE))
> +		addr = round_down(addr, PAGE_SIZE);
> +
> +	if (WARN_ON(length % PAGE_SIZE))
> +		length = round_up(length, PAGE_SIZE);

Oh oh. I think these fixups are very suspect, so just
BUG_ON(length == 0);
BUG_ON(offset_in_page(addr|length));

> +
> +	end = addr + length;
> +
> +	if ((addr & pd_mask) != (end & pd_mask))
> +		return (1 << (pde_shift - PAGE_SHIFT)) -

#define NUM_PTE(pde_shift) (1 << (pde_shift - PAGE_SHIFT))
here and for computing the pd_mask.

> +			i915_pte_index(addr, pde_shift);
> +
> +	return i915_pte_index(end, pde_shift) - i915_pte_index(addr, pde_shift);
> +}

Otherwise the helpers look a useful improvement in readability.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre