Re: [PATCH 2/3] drm/i915: Fix page table entries for Bay Trail.

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 18 Apr 2013, Kenneth Graunke <kenneth@whitecape.org> wrote:
> On Bay Trail, bit 1 means "writeable by the GPU."  Failing to set that
> means basically anything using the GPU will cause hangs.
>
> Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9db65c1..db5c654 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -75,6 +75,27 @@ static inline gen6_gtt_pte_t gen6_pte_encode(struct drm_device *dev,
>  	return pte;
>  }
>  
> +#define BYT_PTE_WRITEABLE		(1 << 1)
> +#define BYT_PTE_SNOOPED_BY_CPU_CACHES	(1 << 2)
> +
> +static inline gen6_gtt_pte_t byt_pte_encode(struct drm_device *dev,

inline doesn't really make sense for functions used through
pointers... I wonder if the compiler should warn about taking address of
inline functions, or whether it just happily non-inlines. Too lazy to
check. ;)

Oh, this applies for patch 1 too; please drop inline from
gen6_pte_encode() there.

> +					    dma_addr_t addr,
> +					    enum i915_cache_level level)
> +{
> +	gen6_gtt_pte_t pte = GEN6_PTE_VALID;
> +	pte |= GEN6_PTE_ADDR_ENCODE(addr);
> +
> +	/* Mark the page as writeable.  Other platforms don't have a
> +	 * setting for read-only/writable, so this matches that behavior.
> +	 */
> +	pte |= BYT_PTE_WRITEABLE;
> +
> +	if (level != I915_CACHE_NONE)
> +		pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES;
> +
> +	return pte;
> +}
> +
>  static int gen6_ppgtt_enable(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> @@ -236,7 +257,11 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	first_pd_entry_in_global_pt =
>  		gtt_total_entries(dev_priv->gtt) - I915_PPGTT_PD_ENTRIES;
>  
> -	ppgtt->pte_encode = gen6_pte_encode;
> +	if (IS_VALLEYVIEW(dev)) {
> +		ppgtt->pte_encode = byt_pte_encode;
> +	} else {
> +		ppgtt->pte_encode = gen6_pte_encode;
> +	}

HAS_ALIASING_PPGTT(dev) returns false for VLV. Do we need this in
preparation for future, or can we just drop this?

>  	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
>  	ppgtt->enable = gen6_ppgtt_enable;
>  	ppgtt->clear_range = gen6_ppgtt_clear_range;
> @@ -826,7 +851,11 @@ int i915_gem_gtt_init(struct drm_device *dev)
>  	} else {
>  		dev_priv->gtt.gtt_probe = gen6_gmch_probe;
>  		dev_priv->gtt.gtt_remove = gen6_gmch_remove;
> -		dev_priv->gtt.pte_encode = gen6_pte_encode;
> +		if (IS_VALLEYVIEW(dev)) {
> +			dev_priv->gtt.pte_encode = byt_pte_encode;
> +		} else {
> +			dev_priv->gtt.pte_encode = gen6_pte_encode;
> +		}

The rest looks good. With the bikesheds addressed (by patches or
counter-arguments ;)

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  	}
>  
>  	ret = dev_priv->gtt.gtt_probe(dev, &dev_priv->gtt.total,
> -- 
> 1.8.2.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx