On Thu, 18 Apr 2013, Kenneth Graunke <kenneth at 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 at 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 at intel.com>
> }
>
> ret = dev_priv->gtt.gtt_probe(dev, &dev_priv->gtt.total,
> --
> 1.8.2.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
