Re: [PATCH v3] drm/i915: Added write-enable pte bit support

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, May 18, 2014 at 11:27:00AM +0530, Akash Goel wrote:
> On Wed, 2014-05-14 at 10:14 +0200, Daniel Vetter wrote:
> > On Tue, May 13, 2014 at 03:43:12PM -0700, Jesse Barnes wrote:
> > > On Wed, 14 May 2014 00:30:34 +0200
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> > > > On Tue, May 13, 2014 at 03:05:24PM -0700, Jesse Barnes wrote:
> > > > > On Tue, 11 Feb 2014 14:19:03 +0530
> > > > > akash.goel@intel.com wrote:
> > > > > 
> > > > > > @@ -810,6 +815,7 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
> > > > > >  		pt_vaddr[act_pte] =
> > > > > >  			vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
> > > > > >  				       cache_level, true);
> > > > > > +
> > > > > >  		if (++act_pte == I915_PPGTT_PT_ENTRIES) {
> > > > > >  			kunmap_atomic(pt_vaddr);
> > > > > >  			pt_vaddr = NULL;
> > > > > 
> > > > > Some extra whitespace here.
> 
> Thanks, will remove this.
> 
> > > > > 
> > > > > Otherwise:
> > > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > 
> > > > Hm, looking at the patch again encoding this into the cache_level enum is
> > > > fraught with fun. And due to IS_VLV aliasing chv this will blow up on chv
> > > > very likely. My old idea was to eventually add a pte_flags param all over
> > > > for this stuff with additional bits.
> > > 
> > > That works too; and yeah CHV is all different here isn't it.  But won't
> > > it go through the gen8 paths anyway?
> > 
> > Yes, but we have a switch on the cache_level in the gen8 pte encode
> > function. Since the bit31 gets or'ed in for VLV, it'll hit also chv and
> > wreak havoc in that specific switch - we'll hit the default case when we
> > don't expect to.
> > 
> > cache_level = functional behaviour relevant for the kernel's clflushing
> > code
> > 
> 
> As the 'IS_VALLEYVIEW' check will alias with CHV also, can I just update
> the condition here, to include 'GEN7' also (IS_GEN7) in the check.

Adding new random bits to an enum which is used all over the place (and
not just in the pte encoding functions) makes the code much harder to
read. Also the code that deals with enum cache_level is all about cache
coherency, which has rather tricky logic.

Hence I expect this choice to cause further bugs down the road, which is
why I don't really like it. My apologies for not spotting this earlier.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch