All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 08/18] drm/i915: Combine seqno + tracking into a global timeline struct
Date: Wed, 14 Sep 2016 18:42:28 +0300	[thread overview]
Message-ID: <1473867748.3924.47.camel@linux.intel.com> (raw)
In-Reply-To: <20160914065250.15482-9-chris@chris-wilson.co.uk>

On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote:
>  i915_next_seqno_get(void *data, u64 *val)
>  {
>  	struct drm_i915_private *dev_priv = data;
> -	int ret;
> -
> -	ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> -	if (ret)
> -		return ret;
> -
> -	*val = dev_priv->next_seqno;
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
>  
> +	*val = dev_priv->gt.global_timeline.next_seqno;

This should be marked as unlocked access somehow.

> +static int wait_for_timeline(struct i915_gem_timeline *tl, unsigned int flags)
>  {
> -	struct intel_engine_cs *engine;
> -	int ret;
> +	int ret, i;
>  
> -	for_each_engine(engine, dev_priv) {
> -		if (engine->last_context == NULL)
> -			continue;
> +	for (i = 0; i < ARRAY_SIZE(tl->engine); i++) {

Lets untangle them headers and not manually roll these loops?

> @@ -4475,13 +4489,24 @@ i915_gem_load_init(struct drm_device *dev)
>  				  SLAB_RECLAIM_ACCOUNT |
>  				  SLAB_DESTROY_BY_RCU,
>  				  NULL);
> +	if (!dev_priv->requests) {
> +		err = -ENOMEM;
> +		goto err_vmas;
> +	}
> +
> +	mutex_lock(&dev_priv->drm.struct_mutex);

Hngg, locking in initializer, maybe we should have our own variant for
assert_held_struct_mutex (or as you suggested have no struct_mutex!)

> +	INIT_LIST_HEAD(&dev_priv->gt.timelines);
> +	err = i915_gem_timeline_init(dev_priv,
> +				     &dev_priv->gt.global_timeline,
> +				     "[execution]");

"[global]" would not be clearer to give better mapping between
code/debug?

> 
> -static int i915_gem_get_seqno(struct drm_i915_private *dev_priv, u32 *seqno)
> +static int i915_gem_get_global_seqno(struct drm_i915_private *dev_priv,
> +				     u32 *seqno)
>  {
> +	struct i915_gem_timeline *tl = &dev_priv->gt.global_timeline;

*gtl? to indicate the global one.

> +struct intel_timeline {
> +	u64 fence_context;
> +	u32 last_submitted_seqno;
> +
> +	/**
> +	 * List of breadcrumbs associated with GPU requests currently
> +	 * outstanding.
> +	 */
> +	struct list_head requests;
> +
> +	/* An RCU guarded pointer to the last request. No reference is
> +	 * held to the request, users must carefully acquire a reference to
> +	 * the request using i915_gem_active_get_request_rcu(), or hold the
> +	 * struct_mutex.
> +	 */
> +	struct i915_gem_active last_request;

RCU guarded pointer sounds off when we're speaking of an object.
 
> 
> @@ -217,8 +217,7 @@ void intel_engine_init_hangcheck(struct intel_engine_cs *engine)
>  
>  static void intel_engine_init_requests(struct intel_engine_cs *engine)
>  {
> -	init_request_active(&engine->last_request, NULL);
> -	INIT_LIST_HEAD(&engine->request_list);
> +	engine->timeline = &engine->i915->gt.global_timeline.engine[engine->id];

function name is not very current, please update.

> @@ -141,7 +142,6 @@ struct intel_engine_cs {
>  		VCS2,	/* Keep instances of the same type engine together. */
>  		VECS
>  	} id;
> -#define I915_NUM_ENGINES 5
>  #define _VCS(n) (VCS + (n))
>  	unsigned int exec_id;
>  	enum intel_engine_hw_id {
> @@ -152,10 +152,10 @@ struct intel_engine_cs {
>  		VCS2_HW
>  	} hw_id;
>  	enum intel_engine_hw_id guc_id; /* XXX same as hw_id? */
> -	u64 fence_context;
>  	u32		mmio_base;
>  	unsigned int irq_shift;
>  	struct intel_ring *buffer;
> +	struct intel_timeline *timeline;

I don't see why not as a non-pointer member?

Again a rather big patch, with above addressed;

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

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2016-09-14 15:42 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-14  6:52 Tracking multiple timelines (full-ppgtt) Chris Wilson
2016-09-14  6:52 ` [PATCH 01/18] drm/i915: Support asynchronous waits on struct fence from i915_gem_request Chris Wilson
2016-09-14  7:37   ` Joonas Lahtinen
2016-09-19 11:26     ` Chris Wilson
2016-09-14  6:52 ` [PATCH 02/18] drm/i915: Allow i915_sw_fence_await_sw_fence() to allocate Chris Wilson
2016-09-14  7:51   ` Joonas Lahtinen
2016-09-14  8:46     ` Chris Wilson
2016-09-14  6:52 ` [PATCH 03/18] drm/i915: Rearrange i915_wait_request() accounting with callers Chris Wilson
2016-09-14  8:47   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 04/18] drm/i915: Remove unused i915_gem_active_wait() in favour of _unlocked() Chris Wilson
2016-09-14  8:48   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 05/18] drm/i915: Move GEM activity tracking into a common struct reservation_object Chris Wilson
2016-09-14  9:44   ` Joonas Lahtinen
2016-09-14 17:35     ` Chris Wilson
2016-09-15  9:38       ` Dave Gordon
2016-09-15  9:55         ` Jani Nikula
2016-09-16 11:40   ` Chris Wilson
2016-09-14  6:52 ` [PATCH 06/18] drm: Add reference counting to drm_atomic_state Chris Wilson
2016-09-14  6:52 ` [PATCH 07/18] drm/i915: Restore nonblocking awaits for modesetting Chris Wilson
2016-09-19 16:01   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 08/18] drm/i915: Combine seqno + tracking into a global timeline struct Chris Wilson
2016-09-14 15:42   ` Joonas Lahtinen [this message]
2016-09-14  6:52 ` [PATCH 09/18] drm/i915: Wait first for submission, before waiting for request completion Chris Wilson
2016-09-19  8:59   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 10/18] drm/i915: Introduce a global_seqno for each request Chris Wilson
2016-09-19 10:36   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 11/18] drm/i915: Record space required for request emission Chris Wilson
2016-09-14 13:30   ` Tvrtko Ursulin
2016-09-14 17:33     ` Chris Wilson
2016-09-15  8:59       ` Tvrtko Ursulin
2016-09-19 10:47   ` Joonas Lahtinen
2016-09-19 11:32     ` Chris Wilson
2016-09-19 16:09       ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 12/18] drm/i915: Defer " Chris Wilson
2016-09-19 12:06   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 13/18] drm/i915: Move the global sync optimisation to the timeline Chris Wilson
2016-09-19 13:16   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 14/18] drm/i915: Create a unique name for the context Chris Wilson
2016-09-19 13:23   ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 15/18] drm/i915: Reserve space in the global seqno during request allocation Chris Wilson
2016-09-19 13:47   ` Joonas Lahtinen
2016-09-19 15:35     ` Jani Nikula
2016-09-19 16:07       ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 16/18] drm/i915: Enable multiple timelines Chris Wilson
2016-09-19 15:52   ` Joonas Lahtinen
2016-10-20 12:49     ` Chris Wilson
2016-10-20 15:25       ` Joonas Lahtinen
2016-09-14  6:52 ` [PATCH 17/18] drm/i915: Enable userspace to opt-out of implicit fencing Chris Wilson
2016-09-14  6:52 ` [PATCH 18/18] drm/i915: Support explicit fencing for execbuf Chris Wilson
2016-09-14  9:16 ` ✗ Fi.CI.BAT: failure for series starting with [01/18] drm/i915: Support asynchronous waits on struct fence from i915_gem_request Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1473867748.3924.47.camel@linux.intel.com \
    --to=joonas.lahtinen@linux.intel.com \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.