| Message ID | 1432917856-12261-56-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/05/2015 17:44, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The i915_gem_object_flush_active() call used to do lots. Over time it has done > less and less. Now all it does check the various associated requests to see if > they can be retired. Hence this patch renames the function and updates the > comments around it to match the current operation. > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f825942..081cbbf 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2956,12 +2956,10 @@ i915_gem_idle_work_handler(struct work_struct *work) > } > > /** > - * Ensures that an object will eventually get non-busy by flushing any required > - * write domains, emitting any outstanding lazy request and retiring and > - * completed requests. > + * Check an object to see if any of it's associated requests can be retired. > */ > static int > -i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > +i915_gem_object_retire(struct drm_i915_gem_object *obj) > { > int i; > > @@ -3034,8 +3032,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > return -ENOENT; > } > > - /* Need to make sure the object gets inactive eventually. */ > - ret = i915_gem_object_flush_active(obj); > + /* Check if the object is pending clean up. */ > + ret = i915_gem_object_retire(obj); > if (ret) > goto out; > > @@ -4526,12 +4524,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > goto unlock; > } > > - /* Count all active objects as busy, even if they are currently not used > - * by the gpu. Users of this interface expect objects to eventually > - * become non-busy without any further actions, therefore emit any > - * necessary flushes here. > - */ > - ret = i915_gem_object_flush_active(obj); > + /* Check if the object is pending clean up. */ > + ret = i915_gem_object_retire(obj); > if (ret) > goto unref; > > Reviewed-by: Tomas Elf <tomas.elf@intel.com> Thanks, Tomas
On Fri, May 29, 2015 at 05:44:16PM +0100, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The i915_gem_object_flush_active() call used to do lots. Over time it has done > less and less. Now all it does check the various associated requests to see if > they can be retired. Hence this patch renames the function and updates the > comments around it to match the current operation. > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> When rebasing patches and especially like here when also renaming them a bit please leave some indication of what you've changed. Took me a while to figure out where one of my pending comments from the previous round went too. And please don't just "v2: rebase", but please add some indicators against what it conflicted if it's obvious. Thanks, Daniel > --- > drivers/gpu/drm/i915/i915_gem.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f825942..081cbbf 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2956,12 +2956,10 @@ i915_gem_idle_work_handler(struct work_struct *work) > } > > /** > - * Ensures that an object will eventually get non-busy by flushing any required > - * write domains, emitting any outstanding lazy request and retiring and > - * completed requests. > + * Check an object to see if any of it's associated requests can be retired. > */ > static int > -i915_gem_object_flush_active(struct drm_i915_gem_object *obj) > +i915_gem_object_retire(struct drm_i915_gem_object *obj) > { > int i; > > @@ -3034,8 +3032,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) > return -ENOENT; > } > > - /* Need to make sure the object gets inactive eventually. */ > - ret = i915_gem_object_flush_active(obj); > + /* Check if the object is pending clean up. */ > + ret = i915_gem_object_retire(obj); > if (ret) > goto out; > > @@ -4526,12 +4524,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, > goto unlock; > } > > - /* Count all active objects as busy, even if they are currently not used > - * by the gpu. Users of this interface expect objects to eventually > - * become non-busy without any further actions, therefore emit any > - * necessary flushes here. > - */ > - ret = i915_gem_object_flush_active(obj); > + /* Check if the object is pending clean up. */ > + ret = i915_gem_object_retire(obj); > if (ret) > goto unref; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jun 17, 2015 at 04:06:05PM +0200, Daniel Vetter wrote: > On Fri, May 29, 2015 at 05:44:16PM +0100, John.C.Harrison@Intel.com wrote: > > From: John Harrison <John.C.Harrison@Intel.com> > > > > The i915_gem_object_flush_active() call used to do lots. Over time it has done > > less and less. Now all it does check the various associated requests to see if > > they can be retired. Hence this patch renames the function and updates the > > comments around it to match the current operation. > > > > For: VIZ-5115 > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > > When rebasing patches and especially like here when also renaming them a > bit please leave some indication of what you've changed. Took me a while > to figure out where one of my pending comments from the previous round > went too. > > And please don't just "v2: rebase", but please add some indicators against > what it conflicted if it's obvious. This function doesn't do an unconditional retire - the new name is much worse since it is inconsistent with how requests retire. In my make GEM umpteen times faster patches, I repurposed this function for reporting the object's current activeness and called it bool i915_gem_oject_active() - though that is probably better as i915_gem_object_is_active(). -Chris
On 17/06/2015 15:06, Daniel Vetter wrote: > On Fri, May 29, 2015 at 05:44:16PM +0100, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> The i915_gem_object_flush_active() call used to do lots. Over time it has done >> less and less. Now all it does check the various associated requests to see if >> they can be retired. Hence this patch renames the function and updates the >> comments around it to match the current operation. >> >> For: VIZ-5115 >> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > When rebasing patches and especially like here when also renaming them a > bit please leave some indication of what you've changed. Took me a while > to figure out where one of my pending comments from the previous round > went too. The thing is that this isn't really a rebase of a previous patch. It is a completely different patch. The original version was removing this function entirely as it had become a trivial wrapper around a call to i915_gem_retire_requests_ring(). Whereas, some of Chris Wilson's work in the meantime has meant that it is now a non-trivial wrapper. Hence the function cannot be removed. However, it is not doing what its name or comment says. So this new patch is just renaming it and updating the comment to be more accurate. Thus the posted comments about the previous patch do not apply to this patch - the change being commented on is no longer being made. > And please don't just "v2: rebase", but please add some indicators against > what it conflicted if it's obvious. > > Thanks, Daniel > >> --- >> drivers/gpu/drm/i915/i915_gem.c | 18 ++++++------------ >> 1 file changed, 6 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index f825942..081cbbf 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2956,12 +2956,10 @@ i915_gem_idle_work_handler(struct work_struct *work) >> } >> >> /** >> - * Ensures that an object will eventually get non-busy by flushing any required >> - * write domains, emitting any outstanding lazy request and retiring and >> - * completed requests. >> + * Check an object to see if any of it's associated requests can be retired. >> */ >> static int >> -i915_gem_object_flush_active(struct drm_i915_gem_object *obj) >> +i915_gem_object_retire(struct drm_i915_gem_object *obj) >> { >> int i; >> >> @@ -3034,8 +3032,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) >> return -ENOENT; >> } >> >> - /* Need to make sure the object gets inactive eventually. */ >> - ret = i915_gem_object_flush_active(obj); >> + /* Check if the object is pending clean up. */ >> + ret = i915_gem_object_retire(obj); >> if (ret) >> goto out; >> >> @@ -4526,12 +4524,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, >> goto unlock; >> } >> >> - /* Count all active objects as busy, even if they are currently not used >> - * by the gpu. Users of this interface expect objects to eventually >> - * become non-busy without any further actions, therefore emit any >> - * necessary flushes here. >> - */ >> - ret = i915_gem_object_flush_active(obj); >> + /* Check if the object is pending clean up. */ >> + ret = i915_gem_object_retire(obj); >> if (ret) >> goto unref; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 17/06/2015 15:21, Chris Wilson wrote: > On Wed, Jun 17, 2015 at 04:06:05PM +0200, Daniel Vetter wrote: >> On Fri, May 29, 2015 at 05:44:16PM +0100, John.C.Harrison@Intel.com wrote: >>> From: John Harrison <John.C.Harrison@Intel.com> >>> >>> The i915_gem_object_flush_active() call used to do lots. Over time it has done >>> less and less. Now all it does check the various associated requests to see if >>> they can be retired. Hence this patch renames the function and updates the >>> comments around it to match the current operation. >>> >>> For: VIZ-5115 >>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >> When rebasing patches and especially like here when also renaming them a >> bit please leave some indication of what you've changed. Took me a while >> to figure out where one of my pending comments from the previous round >> went too. >> >> And please don't just "v2: rebase", but please add some indicators against >> what it conflicted if it's obvious. > This function doesn't do an unconditional retire - the new name is much > worse since it is inconsistent with how requests retire. In my make GEM > umpteen times faster patches, I repurposed this function for reporting > the object's current activeness and called it bool i915_gem_oject_active() > - though that is probably better as i915_gem_object_is_active(). > -Chris > Retiring is generally not an unconditional operation. i915_gem_retire_requests[_ring]() does not forcefully retire all requests, it only retires stuff that has completed. Same here. It could be called i915_gem_object_check_retire() or some such if you prefer. Either way, it is better than i915_gem_object_flush_active() as that is referring to flushing out the OLR which no longer exists. If you are adding other functionality back in then feel free to rename it again as appropriate. But as this patch series stands, the function is a 'retire completed work associated with this object' operation and really needs to be named accordingly.
On Thu, Jun 18, 2015 at 12:03:12PM +0100, John Harrison wrote: > On 17/06/2015 15:21, Chris Wilson wrote: > >On Wed, Jun 17, 2015 at 04:06:05PM +0200, Daniel Vetter wrote: > >>On Fri, May 29, 2015 at 05:44:16PM +0100, John.C.Harrison@Intel.com wrote: > >>>From: John Harrison <John.C.Harrison@Intel.com> > >>> > >>>The i915_gem_object_flush_active() call used to do lots. Over time it has done > >>>less and less. Now all it does check the various associated requests to see if > >>>they can be retired. Hence this patch renames the function and updates the > >>>comments around it to match the current operation. > >>> > >>>For: VIZ-5115 > >>>Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > >>When rebasing patches and especially like here when also renaming them a > >>bit please leave some indication of what you've changed. Took me a while > >>to figure out where one of my pending comments from the previous round > >>went too. > >> > >>And please don't just "v2: rebase", but please add some indicators against > >>what it conflicted if it's obvious. > >This function doesn't do an unconditional retire - the new name is much > >worse since it is inconsistent with how requests retire. In my make GEM > >umpteen times faster patches, I repurposed this function for reporting > >the object's current activeness and called it bool i915_gem_oject_active() > > - though that is probably better as i915_gem_object_is_active(). > >-Chris > > > > Retiring is generally not an unconditional operation. In the code, I use <object>_retire to perform the retiring operation on that object. I can rename i915_gem_retire_requests if that makes you happier, but I don't think it needs to since retire_requests does not imply to me that all requests are retired, just some indefinite value (though positive indefinite at least!). -Chris
On 18/06/2015 12:10, Chris Wilson wrote: > On Thu, Jun 18, 2015 at 12:03:12PM +0100, John Harrison wrote: >> On 17/06/2015 15:21, Chris Wilson wrote: >>> On Wed, Jun 17, 2015 at 04:06:05PM +0200, Daniel Vetter wrote: >>>> On Fri, May 29, 2015 at 05:44:16PM +0100, John.C.Harrison@Intel.com wrote: >>>>> From: John Harrison <John.C.Harrison@Intel.com> >>>>> >>>>> The i915_gem_object_flush_active() call used to do lots. Over time it has done >>>>> less and less. Now all it does check the various associated requests to see if >>>>> they can be retired. Hence this patch renames the function and updates the >>>>> comments around it to match the current operation. >>>>> >>>>> For: VIZ-5115 >>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >>>> When rebasing patches and especially like here when also renaming them a >>>> bit please leave some indication of what you've changed. Took me a while >>>> to figure out where one of my pending comments from the previous round >>>> went too. >>>> >>>> And please don't just "v2: rebase", but please add some indicators against >>>> what it conflicted if it's obvious. >>> This function doesn't do an unconditional retire - the new name is much >>> worse since it is inconsistent with how requests retire. In my make GEM >>> umpteen times faster patches, I repurposed this function for reporting >>> the object's current activeness and called it bool i915_gem_oject_active() >>> - though that is probably better as i915_gem_object_is_active(). >>> -Chris >>> >> Retiring is generally not an unconditional operation. > In the code, I use <object>_retire to perform the retiring operation on > that object. I can rename i915_gem_retire_requests if that makes you > happier, but I don't think it needs to since retire_requests does not > imply to me that all requests are retired, just some indefinite value > (though positive indefinite at least!). > -Chris > Fair enough. I guess I'm still thinking of the driver as it was when I first wrote the patch series which was before your re-write for read/read optimisations. Like I said, the exact new name isn't as important as at least giving it a new name. The old name is definitely not valid any more. Feel free to suggest something better.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f825942..081cbbf 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2956,12 +2956,10 @@ i915_gem_idle_work_handler(struct work_struct *work) } /** - * Ensures that an object will eventually get non-busy by flushing any required - * write domains, emitting any outstanding lazy request and retiring and - * completed requests. + * Check an object to see if any of it's associated requests can be retired. */ static int -i915_gem_object_flush_active(struct drm_i915_gem_object *obj) +i915_gem_object_retire(struct drm_i915_gem_object *obj) { int i; @@ -3034,8 +3032,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file) return -ENOENT; } - /* Need to make sure the object gets inactive eventually. */ - ret = i915_gem_object_flush_active(obj); + /* Check if the object is pending clean up. */ + ret = i915_gem_object_retire(obj); if (ret) goto out; @@ -4526,12 +4524,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data, goto unlock; } - /* Count all active objects as busy, even if they are currently not used - * by the gpu. Users of this interface expect objects to eventually - * become non-busy without any further actions, therefore emit any - * necessary flushes here. - */ - ret = i915_gem_object_flush_active(obj); + /* Check if the object is pending clean up. */ + ret = i915_gem_object_retire(obj); if (ret) goto unref;