[55/55] drm/i915: Rename the somewhat reduced i915_gem_object_flush_active()

Submitted by John.C.Harrison@Intel.com on May 29, 2015, 4:44 p.m.

Details

Message ID 1432917856-12261-56-git-send-email-John.C.Harrison@Intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

John.C.Harrison@Intel.com May 29, 2015, 4:44 p.m.
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(-)

Patch hide | download patch | download mbox

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;
 

Comments

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.