From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 2/3] drm/i915: Fix page table entries for Bay Trail. Date: Fri, 19 Apr 2013 09:08:16 +0300 Message-ID: <87vc7j129b.fsf@intel.com> References: <1366316916-4164-1-git-send-email-kenneth@whitecape.org> <1366316916-4164-3-git-send-email-kenneth@whitecape.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id B6775E6725 for ; Thu, 18 Apr 2013 23:09:33 -0700 (PDT) In-Reply-To: <1366316916-4164-3-git-send-email-kenneth@whitecape.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Kenneth Graunke , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 18 Apr 2013, Kenneth Graunke 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 > --- > 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 > } > > 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