[PATCH v7 1/2] drm/i915/gvt: Use vgpu_lock to protect per vgpu access

[Zhenyu Wang]

On 2018.04.21 13:10:54 +0800, colin.xu at intel.com wrote:
> From: Pei Zhang <pei.zhang at intel.com>
> 
> The patch set splits out 2 small locks from the original big gvt lock:
>   - vgpu_lock protects per-vGPU data and logic, especially the vGPU
>     trap emulation path.
>   - sched_lock protects gvt scheudler structure, context schedule logic
>     and vGPU's schedule data.
> 
> Use vgpu_lock to replace the gvt big lock. By doing this, the
> mmio read/write trap path, vgpu virtual event emulation and other
> vgpu related process, would be protected under per vgpu_lock.
> 
> v7:
>   - Rebase to latest gvt-staging.
>   - Remove gtt_lock since already proteced by gvt_lock and vgpu_lock.
>   - Fix a typo in intel_gvt_deactivate_vgpu, unlock the wrong lock.
> v6:
>   - Rebase to latest gvt-staging.
> v5:
>   - Rebase to latest gvt-staging.
>   - intel_vgpu_page_track_handler should use vgpu_lock.
> v4:
>   - Rebase to latest gvt-staging.
>   - Protect vgpu->active access with vgpu_lock.
>   - Do not wait gpu idle in vgpu_lock.
> v3: update to latest code base
> v2: add gvt->lock in function gvt_check_vblank_emulation
> 
> Performance comparison on Kabylake platform.
>   - Configuration:
>     Host: Ubuntu 16.04.
>     Guest 1 & 2: Ubuntu 16.04.
> 
> glmark2 score comparison:
>   - Configuration:
>     Host: glxgears.
>     Guests: glmark2.
> +--------------------------------+-----------------+
> | Setup                          | glmark2 score   |
> +--------------------------------+-----------------+
> | unified lock, iommu=on         | 58~62 (avg. 60) |
> +--------------------------------+-----------------+
> | unified lock, iommu=igfx_off   | 57~61 (avg. 59) |
> +--------------------------------+-----------------+
> | per-logic lock, iommu=on       | 60~68 (avg. 64) |
> +--------------------------------+-----------------+
> | per-logic lock, iommu=igfx_off | 61~67 (avg. 64) |
> +--------------------------------+-----------------+
> 
> lock_stat comparison:
>   - Configuration:
>     Stop lock stat immediately after boot up.
>     Boot 2 VM Guests.
>     Run glmark2 in guests.
>     Start perf lock_stat for 20 seconds and stop again.
>   - Legend: c - contentions; w - waittime-avg
> +------------+-----------------+-----------+---------------+------------+
> |            | gvt_lock        |sched_lock | vgpu_lock     | gtt_lock   |
> + lock type; +-----------------+-----------+---------------+------------+
> | iommu set  | c     | w       | c  | w    | c    | w      | c   | w    |
> +------------+-------+---------+----+------+------+--------+-----+------+
> | unified;   | 20697 | 839     |N/A | N/A  | N/A  | N/A    | N/A | N/A  |
> | on         |       |         |    |      |      |        |     |      |
> +------------+-------+---------+----+------+------+--------+-----+------+
> | unified;   | 21838 | 658.15  |N/A | N/A  | N/A  | N/A    | N/A | N/A  |
> | igfx_off   |       |         |    |      |      |        |     |      |
> +------------+-------+---------+----+------+------+--------+-----+------+
> | per-logic; | 1553  | 1599.96 |9458|429.97| 5846 | 274.33 | 0   | 0.00 |
> | on         |       |         |    |      |      |        |     |      |
> +------------+-------+---------+----+------+------+--------+-----+------+
> | per-logic; | 1911  | 1678.32 |8335|445.16| 5451 | 244.80 | 0   | 0.00 |
> | igfx_off   |       |         |    |      |      |        |     |      |
> +------------+-------+---------+----+------+------+--------+-----+------+
> 
> Signed-off-by: Pei Zhang <pei.zhang at intel.com>
> Signed-off-by: Colin Xu <colin.xu at intel.com>
> ---

Except some style issues, looks good to me.

Reviewed-by: Zhenyu Wang <zhenyuw at linux.intel.com>


> diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
> index 6ec888822a0f..9eb85785eee5 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.h
> +++ b/drivers/gpu/drm/i915/gvt/gvt.h
> @@ -171,6 +171,7 @@ struct intel_vgpu_submission {
>  
>  struct intel_vgpu {
>  	struct intel_gvt *gvt;
> +	struct mutex vgpu_lock;
>  	int id;
>  	unsigned long handle; /* vGPU handle used by hypervisor MPT modules */
>  	bool active;
> @@ -178,6 +179,7 @@ struct intel_vgpu {
>  	bool failsafe;
>  	unsigned int resetting_eng;
>  	void *sched_data;
> +
>  	struct vgpu_sched_ctl sched_ctl;

no extra line

>  
>  	struct intel_vgpu_fence fence;


> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c b/drivers/gpu/drm/i915/gvt/handlers.c
> index b77adcca0d4a..16f6f92b0ca9 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -319,6 +319,7 @@ static int gdrst_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>  		}
>  	}
>  
> +	// vgpu_lock already hold by emulate mmio r/w

fix comment style

>  	intel_gvt_reset_vgpu_locked(vgpu, false, engine_mask);
>  
>  	/* sw will wait for the device to ack the reset request */
> @@ -423,7 +424,10 @@ static int pipeconf_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
>  		vgpu_vreg(vgpu, offset) |= I965_PIPECONF_ACTIVE;
>  	else
>  		vgpu_vreg(vgpu, offset) &= ~I965_PIPECONF_ACTIVE;
> +	// vgpu_lock already hold by emulate mmio r/w

ditto
> +	mutex_unlock(&vgpu->vgpu_lock);
>  	intel_gvt_check_vblank_emulation(vgpu->gvt);
> +	mutex_lock(&vgpu->vgpu_lock);
>  	return 0;
>  }

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gvt-dev/attachments/20180514/63e8704a/attachment.sig>