Re: [PATCH 50/55] drm/i915: Move request list retirement to i915_gem_request.c

Joonas Lahtinen
2016-07-26T12:48:46Z

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> As the list retirement is now clean of implementation details, we can
> move it closer to the request management.
>

This is again a combined motion + simplification, which should be split
motion first then changes...

Not splitting makes even simple patches slow to review.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 44 ---------------------------------
>  drivers/gpu/drm/i915/i915_gem_request.c | 35 ++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index cc84f00cf883..c572c80a6604 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2541,50 +2541,6 @@ void i915_gem_reset(struct drm_device *dev)
>   i915_gem_restore_fences(dev);
>  }
>
> -/**
> - * This function clears the request list as sequence numbers are passed.
> - * @engine: engine to retire requests on
> - */
> -void
> -i915_gem_retire_requests_ring(struct intel_engine_cs *engine)
> -{
> - while (!list_empty(&engine->request_list)) {
> - struct drm_i915_gem_request *request;
> -
> - request = list_first_entry(&engine->request_list,
> -    struct drm_i915_gem_request,
> -    link);
> -
> - if (!i915_gem_request_completed(request))
> - break;
> -
> - i915_gem_request_retire_upto(request);
> - }
> -}
> -
> -void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
> -{
> - struct intel_engine_cs *engine;
> -
> - lockdep_assert_held(&dev_priv->drm.struct_mutex);
> -
> - if (dev_priv->gt.active_engines == 0)
> - return;
> -
> - GEM_BUG_ON(!dev_priv->gt.awake);
> -
> - for_each_engine(engine, dev_priv) {
> - i915_gem_retire_requests_ring(engine);
> - if (list_empty(&engine->request_list))
> - dev_priv->gt.active_engines &= ~intel_engine_flag(engine);
> - }
> -
> - if (dev_priv->gt.active_engines == 0)
> - queue_delayed_work(dev_priv->wq,
> -    &dev_priv->gt.idle_work,
> -    msecs_to_jiffies(100));
> -}
> -
>  static void
>  i915_gem_retire_work_handler(struct work_struct *work)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index 1935591a98c1..f41572aa53d4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -716,3 +716,38 @@ complete:
>
>   return ret;
>  }
> +
> +void i915_gem_retire_requests_ring(struct intel_engine_cs *engine)

_engine?

> +{
> + struct drm_i915_gem_request *request, *next;
> +
> + list_for_each_entry_safe(request, next, &engine->request_list, link) {
> + if (!i915_gem_request_completed(request))
> + break;
> +
> + i915_gem_request_retire(request);
> + }
> +}
> +
> +void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
> +{
> + struct intel_engine_cs *engine;
> +
> + lockdep_assert_held(&dev_priv->drm.struct_mutex);
> +
> + if (dev_priv->gt.active_engines == 0)
> + return;
> +
> + GEM_BUG_ON(!dev_priv->gt.awake);
> +
> + for_each_engine(engine, dev_priv) {
> + i915_gem_retire_requests_ring(engine);
> + if (list_empty(&engine->request_list))
> + dev_priv->gt.active_engines &= ~intel_engine_flag(engine);
> + }
> +
> + if (dev_priv->gt.active_engines == 0)
> + queue_delayed_work(dev_priv->wq,
> +    &dev_priv->gt.idle_work,
> +    msecs_to_jiffies(100));
> +}
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation