| Message ID | 1392108543-31552-1-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Otherwise: Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> Might be good to expose this as a param too, so userspace could use it for stuff that shouldn't be written...
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. > > 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. -Daniel
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. > > > > 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? Ack on the cache_level abuse though; it's an implementation detail that should be changed if/when this is properly exposed to userspace (or maybe we shouldn't bother and just use CHV/BDW stuff going forward, supporting a proper mprotect ioctl).
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. > > > > > > 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 > Ack on the cache_level abuse though; it's an implementation detail that > should be changed if/when this is properly exposed to userspace (or > maybe we shouldn't bother and just use CHV/BDW stuff going forward, > supporting a proper mprotect ioctl). Not talking about exposing to userspace, but adding pte_flags to the low-level platform pte encode functions to prevent the above bugs. I'm ok with keeping obj->gt_ro. -Daniel
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. > > Ack on the cache_level abuse though; it's an implementation detail that > > should be changed if/when this is properly exposed to userspace (or > > maybe we shouldn't bother and just use CHV/BDW stuff going forward, > > supporting a proper mprotect ioctl). > > Not talking about exposing to userspace, but adding pte_flags to the > low-level platform pte encode functions to prevent the above bugs. I'm ok > with keeping obj->gt_ro. > -Daniel
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
On Mon, 2014-05-19 at 08:56 +0200, Daniel Vetter wrote: > 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 Hi Daniel, I understand your concern, but actually (as of now) this bit is only VLV specific. As per an earlier suggestion from Chris, I decided to overload the cache_level enum itself, in lieu of adding a new parameter to all the respective 'xxx_insert_entries' & 'xxx_pte_encode' functions. And this is being done just before calling the 'xxx_insert_entries' function, which simply passes the flag as it is to 'xxx_pte_encode' function. So there may not be any risk here, if we use the appropriate VLV check. Best Regards Akash
On Mon, 2014-05-19 at 13:03 +0530, Akash Goel wrote: > On Mon, 2014-05-19 at 08:56 +0200, Daniel Vetter wrote: > > 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 > > Hi Daniel, > > I understand your concern, but actually (as of now) this bit is only VLV > specific. As per an earlier suggestion from Chris, I decided to > overload the cache_level enum itself, in lieu of adding a new parameter > to all the respective 'xxx_insert_entries' & 'xxx_pte_encode' functions. > > And this is being done just before calling the 'xxx_insert_entries' > function, which simply passes the flag as it is to 'xxx_pte_encode' > function. So there may not be any risk here, if we use the appropriate > VLV check. > Hi Daniel, Please can you provide your inputs here. Best regards Akash > Best Regards > Akash > > > >
On Sun, Jun 01, 2014 at 12:12:50PM +0530, Akash Goel wrote: > On Mon, 2014-05-19 at 13:03 +0530, Akash Goel wrote: > > On Mon, 2014-05-19 at 08:56 +0200, Daniel Vetter wrote: > > > 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 > > > > Hi Daniel, > > > > I understand your concern, but actually (as of now) this bit is only VLV > > specific. As per an earlier suggestion from Chris, I decided to > > overload the cache_level enum itself, in lieu of adding a new parameter > > to all the respective 'xxx_insert_entries' & 'xxx_pte_encode' functions. > > > > And this is being done just before calling the 'xxx_insert_entries' > > function, which simply passes the flag as it is to 'xxx_pte_encode' > > function. So there may not be any risk here, if we use the appropriate > > VLV check. > > Please can you provide your inputs here. I guess you've run into a case where Chris&I disagree. I still think wiring up a flags parameter to all the pte encode functions is the sane option to pick here. -Daniel
On Mon, 2014-06-02 at 10:19 +0200, Daniel Vetter wrote: > On Sun, Jun 01, 2014 at 12:12:50PM +0530, Akash Goel wrote: > > On Mon, 2014-05-19 at 13:03 +0530, Akash Goel wrote: > > > On Mon, 2014-05-19 at 08:56 +0200, Daniel Vetter wrote: > > > > 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 > > > > > > Hi Daniel, > > > > > > I understand your concern, but actually (as of now) this bit is only VLV > > > specific. As per an earlier suggestion from Chris, I decided to > > > overload the cache_level enum itself, in lieu of adding a new parameter > > > to all the respective 'xxx_insert_entries' & 'xxx_pte_encode' functions. > > > > > > And this is being done just before calling the 'xxx_insert_entries' > > > function, which simply passes the flag as it is to 'xxx_pte_encode' > > > function. So there may not be any risk here, if we use the appropriate > > > VLV check. > > > > Please can you provide your inputs here. > > I guess you've run into a case where Chris&I disagree. I still think > wiring up a flags parameter to all the pte encode functions is the sane > option to pick here. Hi Daniel, Yes I just followed Chris's suggestion, which I also felt was reasonable. But it's fine, will implement it as per your suggestion only. I hope that will be acceptable to all. Best regards Akash > -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 728b9c3..8dab62e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1699,6 +1699,12 @@ struct drm_i915_gem_object { unsigned int pin_display:1; /* + * Is the object to be mapped as read-only to the GPU + * Only honoured if hardware has relevant pte bit + */ + unsigned long gt_ro:1; + + /* * Is the GPU currently using a fence to access this buffer, */ unsigned int pending_fenced_gpu_access:1; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 6e858e1..1e02d44 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -151,6 +151,7 @@ static gen6_gtt_pte_t ivb_pte_encode(dma_addr_t addr, #define BYT_PTE_WRITEABLE (1 << 1) #define BYT_PTE_SNOOPED_BY_CPU_CACHES (1 << 2) +#define BYT_PTE_READ_ONLY (1 << 31) static gen6_gtt_pte_t byt_pte_encode(dma_addr_t addr, enum i915_cache_level level, @@ -167,6 +168,10 @@ static gen6_gtt_pte_t byt_pte_encode(dma_addr_t addr, if (level != I915_CACHE_NONE) pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES; + /* Handle read-only request */ + if (level & BYT_PTE_READ_ONLY) + pte &= ~BYT_PTE_WRITEABLE; + return pte; } @@ -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; @@ -999,6 +1005,10 @@ ppgtt_bind_vma(struct i915_vma *vma, WARN_ON(flags); + if (IS_VALLEYVIEW(vma->vm->dev)) + if (vma->obj->gt_ro) + cache_level |= BYT_PTE_READ_ONLY; + vma->vm->insert_entries(vma->vm, vma->obj->pages, entry, cache_level); } @@ -1336,6 +1346,10 @@ static void ggtt_bind_vma(struct i915_vma *vma, struct drm_i915_gem_object *obj = vma->obj; const unsigned long entry = vma->node.start >> PAGE_SHIFT; + if (IS_VALLEYVIEW(dev)) + if (obj->gt_ro) + cache_level |= BYT_PTE_READ_ONLY; + /* If there is no aliasing PPGTT, or the caller needs a global mapping, * or we have a global mapping already but the cacheability flags have * changed, set the global PTEs. diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index d897a19..2fee914 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1354,6 +1354,9 @@ static int intel_init_ring_buffer(struct drm_device *dev, goto err_hws; } + /* mark ring buffers as read-only from GPU side by default */ + obj->gt_ro = 1; + ring->obj = obj; ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, true, false);