| Message ID | 1441044852-14089-1-git-send-email-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 31, 2015 at 07:14:12PM +0100, Emil Velikov wrote: > Just like we do for the original exec() > > v2: move bo_gem declaration to the top of the function. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: intel-gfx@lists.freedesktop.org > Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> > --- > intel/intel_bufmgr_gem.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c > index 7303903..5287419 100644 > --- a/intel/intel_bufmgr_gem.c > +++ b/intel/intel_bufmgr_gem.c > @@ -2185,10 +2185,14 @@ do_exec2(drm_intel_bo *bo, int used, drm_intel_context *ctx, > unsigned int flags) > { > drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bo->bufmgr; > + drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo; > struct drm_i915_gem_execbuffer2 execbuf; > int ret = 0; > int i; > > + if (bo_gem->has_error) > + return -ENOMEM; > + > switch (flags & 0x7) { > default: > return -EINVAL; > @@ -2259,8 +2263,7 @@ skip_execution: > drm_intel_gem_dump_validation_list(bufmgr_gem); > > for (i = 0; i < bufmgr_gem->exec_count; i++) { > - drm_intel_bo_gem *bo_gem = > - (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i]; > + bo_gem = (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i]; Reusing bo_gem here is a little worrying as it would be very easy for someone to add code to the end of the function thinking that bo_gem still was the batch. If we had static inline drm_intel_bo_gem *to_bo_gem(drm_intel_bo *bo) { return (drm_intel_bo_gem *)bo; } then we can start doing one offs like if (to_bo_gem(bo)->has_error) return -ENOMEM; and of course for (i = 0; i < bufmgr_gem->exec_count; i++) { drm_intel_bo_gem *bo_gem = to_bo_gem(bufmgr_gem->exec_bos[i]); -Chris
On 31 August 2015 at 19:26, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, Aug 31, 2015 at 07:14:12PM +0100, Emil Velikov wrote: >> Just like we do for the original exec() >> >> v2: move bo_gem declaration to the top of the function. >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: intel-gfx@lists.freedesktop.org >> Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> >> --- >> intel/intel_bufmgr_gem.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c >> index 7303903..5287419 100644 >> --- a/intel/intel_bufmgr_gem.c >> +++ b/intel/intel_bufmgr_gem.c >> @@ -2185,10 +2185,14 @@ do_exec2(drm_intel_bo *bo, int used, drm_intel_context *ctx, >> unsigned int flags) >> { >> drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bo->bufmgr; >> + drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo; >> struct drm_i915_gem_execbuffer2 execbuf; >> int ret = 0; >> int i; >> >> + if (bo_gem->has_error) >> + return -ENOMEM; >> + >> switch (flags & 0x7) { >> default: >> return -EINVAL; >> @@ -2259,8 +2263,7 @@ skip_execution: >> drm_intel_gem_dump_validation_list(bufmgr_gem); >> >> for (i = 0; i < bufmgr_gem->exec_count; i++) { >> - drm_intel_bo_gem *bo_gem = >> - (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i]; >> + bo_gem = (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i]; > > Reusing bo_gem here is a little worrying as it would be very easy for > someone to add code to the end of the function thinking that bo_gem > still was the batch. > Doesn't this concert apply to drm_intel_gem_bo_exec() as well ? > If we had > > static inline drm_intel_bo_gem *to_bo_gem(drm_intel_bo *bo) > { > return (drm_intel_bo_gem *)bo; > } > > then we can start doing one offs like > > if (to_bo_gem(bo)->has_error) return -ENOMEM; > > and of course > for (i = 0; i < bufmgr_gem->exec_count; i++) { > drm_intel_bo_gem *bo_gem = to_bo_gem(bufmgr_gem->exec_bos[i]); How about we do this as a follow up patch (4.1/17) that covers both functions ? Thanks again, Emil
On Mon, Aug 31, 2015 at 08:01:21PM +0100, Emil Velikov wrote: > > Reusing bo_gem here is a little worrying as it would be very easy for > > someone to add code to the end of the function thinking that bo_gem > > still was the batch. > > > Doesn't this concert apply to drm_intel_gem_bo_exec() as well ? Probably. Haven't looked at that in years. :p > > If we had > > > > static inline drm_intel_bo_gem *to_bo_gem(drm_intel_bo *bo) > > { > > return (drm_intel_bo_gem *)bo; > > } > > > > then we can start doing one offs like > > > > if (to_bo_gem(bo)->has_error) return -ENOMEM; > > > > and of course > > for (i = 0; i < bufmgr_gem->exec_count; i++) { > > drm_intel_bo_gem *bo_gem = to_bo_gem(bufmgr_gem->exec_bos[i]); > > How about we do this as a follow up patch (4.1/17) that covers both functions ? If you are quick... -Chris
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index 7303903..5287419 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -2185,10 +2185,14 @@ do_exec2(drm_intel_bo *bo, int used, drm_intel_context *ctx, unsigned int flags) { drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bo->bufmgr; + drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo; struct drm_i915_gem_execbuffer2 execbuf; int ret = 0; int i; + if (bo_gem->has_error) + return -ENOMEM; + switch (flags & 0x7) { default: return -EINVAL; @@ -2259,8 +2263,7 @@ skip_execution: drm_intel_gem_dump_validation_list(bufmgr_gem); for (i = 0; i < bufmgr_gem->exec_count; i++) { - drm_intel_bo_gem *bo_gem = - (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i]; + bo_gem = (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i]; bo_gem->idle = false;
Just like we do for the original exec() v2: move bo_gem declaration to the top of the function. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Emil Velikov <emil.l.velikov@gmail.com> --- intel/intel_bufmgr_gem.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)