| Message ID | 1467299569-10920-2-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: > Precursor for fix to secure batch execution. We will need to be able to > retrieve the batch VMA (as well as the batch itself) from the eb list, > so this patch extracts that part of eb_get_batch() into a separate > function, and moves both parts to a more logical place in the file, near > where the eb list is created. > > Also, it may not be obvious, but the current execbuffer2 ioctl interface > requires that the buffer object containing the batch-to-be-executed be > the LAST entry in the exec2_list[] array (I expected it to be the first!). > > To clarify this, we can replace the rather obscure construct > "list_entry(eb->vmas.prev, ...)" > in the old version of eb_get_batch() with the equivalent but more explicit > "list_last_entry(&eb->vmas,...)" > in the new eb_get_batch_vma() and of course add an explanatory comment. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> I have no context on the secure batch fix you're talking about, but this here makes sense as an independent cleanup. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 49 ++++++++++++++++++------------ > 1 file changed, 29 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 608fdc4..eea8b1f 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -186,6 +186,35 @@ struct eb_vmas { > return ret; > } > > +static inline struct i915_vma * > +eb_get_batch_vma(struct eb_vmas *eb) > +{ > + /* The batch is always the LAST item in the VMA list */ > + struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list); > + > + return vma; > +} > + > +static struct drm_i915_gem_object * > +eb_get_batch(struct eb_vmas *eb) > +{ > + struct i915_vma *vma = eb_get_batch_vma(eb); > + > + /* > + * SNA is doing fancy tricks with compressing batch buffers, which leads > + * to negative relocation deltas. Usually that works out ok since the > + * relocate address is still positive, except when the batch is placed > + * very low in the GTT. Ensure this doesn't happen. > + * > + * Note that actual hangs have only been observed on gen7, but for > + * paranoia do it everywhere. > + */ > + if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0) > + vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS; > + > + return vma->obj; > +} > + > static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle) > { > if (eb->and < 0) { > @@ -1341,26 +1370,6 @@ static bool only_mappable_for_reloc(unsigned int flags) > return file_priv->bsd_ring; > } > > -static struct drm_i915_gem_object * > -eb_get_batch(struct eb_vmas *eb) > -{ > - struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma), exec_list); > - > - /* > - * SNA is doing fancy tricks with compressing batch buffers, which leads > - * to negative relocation deltas. Usually that works out ok since the > - * relocate address is still positive, except when the batch is placed > - * very low in the GTT. Ensure this doesn't happen. > - * > - * Note that actual hangs have only been observed on gen7, but for > - * paranoia do it everywhere. > - */ > - if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0) > - vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS; > - > - return vma->obj; > -} > - > #define I915_USER_RINGS (4) > > static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = { > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote: > On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: > > Precursor for fix to secure batch execution. We will need to be able to > > retrieve the batch VMA (as well as the batch itself) from the eb list, > > so this patch extracts that part of eb_get_batch() into a separate > > function, and moves both parts to a more logical place in the file, near > > where the eb list is created. > > > > Also, it may not be obvious, but the current execbuffer2 ioctl interface > > requires that the buffer object containing the batch-to-be-executed be > > the LAST entry in the exec2_list[] array (I expected it to be the first!). > > > > To clarify this, we can replace the rather obscure construct > > "list_entry(eb->vmas.prev, ...)" > > in the old version of eb_get_batch() with the equivalent but more explicit > > "list_last_entry(&eb->vmas,...)" > > in the new eb_get_batch_vma() and of course add an explanatory comment. > > > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > I have no context on the secure batch fix you're talking about, but this > here makes sense as an independent cleanup. It won't help though, so this is just churn for no purpose. -Chris
On 13/07/16 13:44, Chris Wilson wrote: > On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote: >> On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: >>> Precursor for fix to secure batch execution. We will need to be able to >>> retrieve the batch VMA (as well as the batch itself) from the eb list, >>> so this patch extracts that part of eb_get_batch() into a separate >>> function, and moves both parts to a more logical place in the file, near >>> where the eb list is created. >>> >>> Also, it may not be obvious, but the current execbuffer2 ioctl interface >>> requires that the buffer object containing the batch-to-be-executed be >>> the LAST entry in the exec2_list[] array (I expected it to be the first!). >>> >>> To clarify this, we can replace the rather obscure construct >>> "list_entry(eb->vmas.prev, ...)" >>> in the old version of eb_get_batch() with the equivalent but more explicit >>> "list_last_entry(&eb->vmas,...)" >>> in the new eb_get_batch_vma() and of course add an explanatory comment. >>> >>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >> >> I have no context on the secure batch fix you're talking about, but this >> here makes sense as an independent cleanup. > > It won't help though, so this is just churn for no purpose. > -Chris At the very least, it replaces a confusing construct with a comprehensible one annotated with an explanatory comment. Separating finding the VMA for the batch from finding the batch itself also improves clarity and costs nothing (compiler inlines it anyway). Comprehensibility -- and hence maintainability -- is always a worthwhile purpose :) BTW, do the comments in this code from patch d23db88 drm/i915: Prevent negative relocation deltas from wrapping still apply? 'Cos I think it's pretty ugly to be setting a flag on a VMA as a side-effect of a "lookup" type operation :( Surely cleaner to do that sort of think at the top level i.e. inside i915_gem_do_execbuffer() ? .Dave.
On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote: > On 13/07/16 13:44, Chris Wilson wrote: > >On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote: > >>On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: > >>>Precursor for fix to secure batch execution. We will need to be able to > >>>retrieve the batch VMA (as well as the batch itself) from the eb list, > >>>so this patch extracts that part of eb_get_batch() into a separate > >>>function, and moves both parts to a more logical place in the file, near > >>>where the eb list is created. > >>> > >>>Also, it may not be obvious, but the current execbuffer2 ioctl interface > >>>requires that the buffer object containing the batch-to-be-executed be > >>>the LAST entry in the exec2_list[] array (I expected it to be the first!). > >>> > >>>To clarify this, we can replace the rather obscure construct > >>> "list_entry(eb->vmas.prev, ...)" > >>>in the old version of eb_get_batch() with the equivalent but more explicit > >>> "list_last_entry(&eb->vmas,...)" > >>>in the new eb_get_batch_vma() and of course add an explanatory comment. > >>> > >>>Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > >> > >>I have no context on the secure batch fix you're talking about, but this > >>here makes sense as an independent cleanup. > > > >It won't help though, so this is just churn for no purpose. > >-Chris > > At the very least, it replaces a confusing construct with > a comprehensible one annotated with an explanatory comment. No. It deepens a confusion in the code that I've been trying to get removed over the last couple of years. > Separating finding the VMA for the batch from finding the batch itself > also improves clarity and costs nothing (compiler inlines it anyway). No. That's the confusion you have here. The object is irrelevant. > Comprehensibility -- and hence maintainability -- is always > a worthwhile purpose :) s/comprehensibility/greater confusion/ > BTW, do the comments in this code from patch > > d23db88 drm/i915: Prevent negative relocation deltas from wrapping > > still apply? 'Cos I think it's pretty ugly to be setting a flag > on a VMA as a side-effect of a "lookup" type operation :( Surely > cleaner to do that sort of think at the top level i.e. inside > i915_gem_do_execbuffer() ? The comment is wrong since the practice is more widespread and it is a particular hw bug on Ivybridge. -Chris
On 14/07/16 15:03, Chris Wilson wrote: > On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote: >> On 13/07/16 13:44, Chris Wilson wrote: >>> On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote: >>>> On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: >>>>> Precursor for fix to secure batch execution. We will need to be able to >>>>> retrieve the batch VMA (as well as the batch itself) from the eb list, >>>>> so this patch extracts that part of eb_get_batch() into a separate >>>>> function, and moves both parts to a more logical place in the file, near >>>>> where the eb list is created. >>>>> >>>>> Also, it may not be obvious, but the current execbuffer2 ioctl interface >>>>> requires that the buffer object containing the batch-to-be-executed be >>>>> the LAST entry in the exec2_list[] array (I expected it to be the first!). >>>>> >>>>> To clarify this, we can replace the rather obscure construct >>>>> "list_entry(eb->vmas.prev, ...)" >>>>> in the old version of eb_get_batch() with the equivalent but more explicit >>>>> "list_last_entry(&eb->vmas,...)" >>>>> in the new eb_get_batch_vma() and of course add an explanatory comment. >>>>> >>>>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>>> >>>> I have no context on the secure batch fix you're talking about, but this >>>> here makes sense as an independent cleanup. >>> >>> It won't help though, so this is just churn for no purpose. >>> -Chris >> >> At the very least, it replaces a confusing construct with >> a comprehensible one annotated with an explanatory comment. > > No. It deepens a confusion in the code that I've been trying to get > removed over the last couple of years. ? I was referring to the list_{last_}entry() change. That's definitely a clarification as to how things work now. Of course, if you're planning to make the batch the first object rather than the last, I won't object. But whichever it is, let's use the most-appropriately-named of the available list functions when we pick an item from a list. And comment why or what it's doing. >> Separating finding the VMA for the batch from finding the batch itself >> also improves clarity and costs nothing (compiler inlines it anyway). > > No. That's the confusion you have here. The object is irrelevant. Ah, so we have a function to return an irrelevant object. Let's just delete it then ;) Do you think we /should/ just get rid of eb_get_batch()? Maybe just have eb_get_batch_vma() return the VMA to the [single] caller i915_gem_do_execbuffer() instead, and then have /that/ do both the flag-setting ugliness and the indirection to the object (which evidently is not irrelevant to it) ? >> Comprehensibility -- and hence maintainability -- is always >> a worthwhile purpose :) > > s/comprehensibility/greater confusion/ Spoken like a true Discordian ;) > > BTW, do the comments in this code from patch >> >> d23db88 drm/i915: Prevent negative relocation deltas from wrapping >> >> still apply? 'Cos I think it's pretty ugly to be setting a flag >> on a VMA as a side-effect of a "lookup" type operation :( Surely >> cleaner to do that sort of think at the top level i.e. inside >> i915_gem_do_execbuffer() ? > > The comment is wrong since the practice is more widespread and it is > a particular hw bug on Ivybridge. > -Chris Another reason to move it out to the caller and update the comments in the process! .Dave.
On Fri, Jul 15, 2016 at 09:03:40AM +0100, Dave Gordon wrote: > On 14/07/16 15:03, Chris Wilson wrote: > >On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote: > >>On 13/07/16 13:44, Chris Wilson wrote: > >>>On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote: > >>>>On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: > >>>>>Precursor for fix to secure batch execution. We will need to be able to > >>>>>retrieve the batch VMA (as well as the batch itself) from the eb list, > >>>>>so this patch extracts that part of eb_get_batch() into a separate > >>>>>function, and moves both parts to a more logical place in the file, near > >>>>>where the eb list is created. > >>>>> > >>>>>Also, it may not be obvious, but the current execbuffer2 ioctl interface > >>>>>requires that the buffer object containing the batch-to-be-executed be > >>>>>the LAST entry in the exec2_list[] array (I expected it to be the first!). > >>>>> > >>>>>To clarify this, we can replace the rather obscure construct > >>>>> "list_entry(eb->vmas.prev, ...)" > >>>>>in the old version of eb_get_batch() with the equivalent but more explicit > >>>>> "list_last_entry(&eb->vmas,...)" > >>>>>in the new eb_get_batch_vma() and of course add an explanatory comment. > >>>>> > >>>>>Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > >>>> > >>>>I have no context on the secure batch fix you're talking about, but this > >>>>here makes sense as an independent cleanup. > >>> > >>>It won't help though, so this is just churn for no purpose. > >>>-Chris > >> > >>At the very least, it replaces a confusing construct with > >>a comprehensible one annotated with an explanatory comment. > > > >No. It deepens a confusion in the code that I've been trying to get > >removed over the last couple of years. > > ? > > I was referring to the list_{last_}entry() change. That's definitely > a clarification as to how things work now. Of course, if you're > planning to make the batch the first object rather than the last, I > won't object. But whichever it is, let's use the > most-appropriately-named of the available list functions when we > pick an item from a list. And comment why or what it's doing. > > >>Separating finding the VMA for the batch from finding the batch itself > >>also improves clarity and costs nothing (compiler inlines it anyway). > > > >No. That's the confusion you have here. The object is irrelevant. > > Ah, so we have a function to return an irrelevant object. Let's just > delete it then ;) > > Do you think we /should/ just get rid of eb_get_batch()? Maybe just > have eb_get_batch_vma() return the VMA to the [single] caller > i915_gem_do_execbuffer() instead, and then have /that/ do both > the flag-setting ugliness and the indirection to the object (which > evidently is not irrelevant to it) ? > > >>Comprehensibility -- and hence maintainability -- is always > >>a worthwhile purpose :) > > > >s/comprehensibility/greater confusion/ > > Spoken like a true Discordian ;) > > >> BTW, do the comments in this code from patch > >> > >>d23db88 drm/i915: Prevent negative relocation deltas from wrapping > >> > >>still apply? 'Cos I think it's pretty ugly to be setting a flag > >>on a VMA as a side-effect of a "lookup" type operation :( Surely > >>cleaner to do that sort of think at the top level i.e. inside > >>i915_gem_do_execbuffer() ? > > > >The comment is wrong since the practice is more widespread and it is > >a particular hw bug on Ivybridge. > >-Chris > > Another reason to move it out to the caller and update the comments > in the process! Then review the patches that have already been posted several times on the list to do everything above. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 608fdc4..eea8b1f 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -186,6 +186,35 @@ struct eb_vmas { return ret; } +static inline struct i915_vma * +eb_get_batch_vma(struct eb_vmas *eb) +{ + /* The batch is always the LAST item in the VMA list */ + struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma), exec_list); + + return vma; +} + +static struct drm_i915_gem_object * +eb_get_batch(struct eb_vmas *eb) +{ + struct i915_vma *vma = eb_get_batch_vma(eb); + + /* + * SNA is doing fancy tricks with compressing batch buffers, which leads + * to negative relocation deltas. Usually that works out ok since the + * relocate address is still positive, except when the batch is placed + * very low in the GTT. Ensure this doesn't happen. + * + * Note that actual hangs have only been observed on gen7, but for + * paranoia do it everywhere. + */ + if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0) + vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS; + + return vma->obj; +} + static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle) { if (eb->and < 0) { @@ -1341,26 +1370,6 @@ static bool only_mappable_for_reloc(unsigned int flags) return file_priv->bsd_ring; } -static struct drm_i915_gem_object * -eb_get_batch(struct eb_vmas *eb) -{ - struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma), exec_list); - - /* - * SNA is doing fancy tricks with compressing batch buffers, which leads - * to negative relocation deltas. Usually that works out ok since the - * relocate address is still positive, except when the batch is placed - * very low in the GTT. Ensure this doesn't happen. - * - * Note that actual hangs have only been observed on gen7, but for - * paranoia do it everywhere. - */ - if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0) - vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS; - - return vma->obj; -} - #define I915_USER_RINGS (4) static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
Precursor for fix to secure batch execution. We will need to be able to retrieve the batch VMA (as well as the batch itself) from the eb list, so this patch extracts that part of eb_get_batch() into a separate function, and moves both parts to a more logical place in the file, near where the eb list is created. Also, it may not be obvious, but the current execbuffer2 ioctl interface requires that the buffer object containing the batch-to-be-executed be the LAST entry in the exec2_list[] array (I expected it to be the first!). To clarify this, we can replace the rather obscure construct "list_entry(eb->vmas.prev, ...)" in the old version of eb_get_batch() with the equivalent but more explicit "list_last_entry(&eb->vmas,...)" in the new eb_get_batch_vma() and of course add an explanatory comment. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 49 ++++++++++++++++++------------ 1 file changed, 29 insertions(+), 20 deletions(-)