Re: [PATCH V3 2/6] drm/i915:Initialize the second BSD ring on BDW GT3 machine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2014-04-16 at 10:41 +0800, Zhao Yakui wrote:
> Based on the hardware spec, the BDW GT3 machine has two independent
> BSD ring that can be used to dispatch the video commands.
> So just initialize it.
> 
> Signed-off-by: Zhao Yakui <yakui.zhao@xxxxxxxxx>

A couple of nitpicks below, with or without those:
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>

> ---
>  drivers/gpu/drm/i915/i915_drv.c         |    4 +--
>  drivers/gpu/drm/i915/i915_drv.h         |    2 ++
>  drivers/gpu/drm/i915/i915_gem.c         |    9 +++++-
>  drivers/gpu/drm/i915/i915_gpu_error.c   |    1 +
>  drivers/gpu/drm/i915/i915_reg.h         |    1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c |   54 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h |    4 ++-
>  7 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 17fbbe5..2a7842b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -282,7 +282,7 @@ static const struct intel_device_info intel_broadwell_m_info = {
>  static const struct intel_device_info intel_broadwell_gt3d_info = {
>  	.gen = 8, .num_pipes = 3,
>  	.need_gfx_hws = 1, .has_hotplug = 1,
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  	.has_llc = 1,
>  	.has_ddi = 1,
>  	.has_fbc = 1,
> @@ -292,7 +292,7 @@ static const struct intel_device_info intel_broadwell_gt3d_info = {
>  static const struct intel_device_info intel_broadwell_gt3m_info = {
>  	.gen = 8, .is_mobile = 1, .num_pipes = 3,
>  	.need_gfx_hws = 1, .has_hotplug = 1,
> -	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> +	.ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING | BSD2_RING,
>  	.has_llc = 1,
>  	.has_ddi = 1,
>  	.has_fbc = 1,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 92c3095..74aef6a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1833,7 +1833,9 @@ struct drm_i915_cmd_table {
>  #define BSD_RING		(1<<VCS)
>  #define BLT_RING		(1<<BCS)
>  #define VEBOX_RING		(1<<VECS)
> +#define BSD2_RING		(1<<VCS2)
>  #define HAS_BSD(dev)            (INTEL_INFO(dev)->ring_mask & BSD_RING)
> +#define HAS_BSD2(dev)		(INTEL_INFO(dev)->ring_mask & BSD2_RING)
>  #define HAS_BLT(dev)            (INTEL_INFO(dev)->ring_mask & BLT_RING)
>  #define HAS_VEBOX(dev)            (INTEL_INFO(dev)->ring_mask & VEBOX_RING)
>  #define HAS_LLC(dev)            (INTEL_INFO(dev)->has_llc)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 85c9cf0..b4dcf2a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4374,13 +4374,20 @@ static int i915_gem_init_rings(struct drm_device *dev)
>  			goto cleanup_blt_ring;
>  	}
>  
> +	if (HAS_BSD2(dev)) {
> +		ret = intel_init_bsd2_ring_buffer(dev);
> +		if (ret)
> +			goto cleanup_vebox_ring;
> +	}
>  
>  	ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
>  	if (ret)
> -		goto cleanup_vebox_ring;
> +		goto cleanup_ring;

maybe cleanup_bsd2_ring?

>  
>  	return 0;
>  
> +cleanup_ring:
> +	intel_cleanup_ring_buffer(&dev_priv->ring[VCS2]);
>  cleanup_vebox_ring:
>  	intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
>  cleanup_blt_ring:
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 4865ade..3cab7f9 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -42,6 +42,7 @@ static const char *ring_str(int ring)
>  	case VCS: return "bsd";
>  	case BCS: return "blt";
>  	case VECS: return "vebox";
> +	case VCS2: return "second bsd";

"bsd2" would be more concise

>  	default: return "";
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8f84555..0b88508 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -760,6 +760,7 @@ enum punit_power_well {
>  #define RENDER_RING_BASE	0x02000
>  #define BSD_RING_BASE		0x04000
>  #define GEN6_BSD_RING_BASE	0x12000
> +#define GEN8_BSD2_RING_BASE	0x1c000
>  #define VEBOX_RING_BASE		0x1a000
>  #define BLT_RING_BASE		0x22000
>  #define RING_TAIL(base)		((base)+0x30)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index eb3dd26..8b9b89080 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1924,10 +1924,12 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  		ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_RV;
>  		ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_RB;
>  		ring->semaphore_register[VECS] = MI_SEMAPHORE_SYNC_RVE;
> +		ring->semaphore_register[VCS2] = MI_SEMAPHORE_SYNC_INVALID;

It's unclear why we initialize the semaphore stuff differently for VCS
and VCS2. Semaphores are disabled for GEN8 atm and they will be reworked
in the future, but maybe a comment to revisit this code would good here
and in intel_init_bsd2_ring_buffer().

>  		ring->signal_mbox[RCS] = GEN6_NOSYNC;
>  		ring->signal_mbox[VCS] = GEN6_VRSYNC;
>  		ring->signal_mbox[BCS] = GEN6_BRSYNC;
>  		ring->signal_mbox[VECS] = GEN6_VERSYNC;
> +		ring->signal_mbox[VCS2] = GEN6_NOSYNC;
>  	} else if (IS_GEN5(dev)) {
>  		ring->add_request = pc_render_add_request;
>  		ring->flush = gen4_render_ring_flush;
> @@ -2100,10 +2102,12 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  		ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_INVALID;
>  		ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_VB;
>  		ring->semaphore_register[VECS] = MI_SEMAPHORE_SYNC_VVE;
> +		ring->semaphore_register[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
>  		ring->signal_mbox[RCS] = GEN6_RVSYNC;
>  		ring->signal_mbox[VCS] = GEN6_NOSYNC;
>  		ring->signal_mbox[BCS] = GEN6_BVSYNC;
>  		ring->signal_mbox[VECS] = GEN6_VEVSYNC;
> +		ring->signal_mbox[VCS2] = GEN6_NOSYNC;
>  	} else {
>  		ring->mmio_base = BSD_RING_BASE;
>  		ring->flush = bsd_ring_flush;
> @@ -2126,6 +2130,52 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  	return intel_init_ring_buffer(dev, ring);
>  }
>  
> +/**
> + * Initialize the second BSD ring for Broadwell GT3.
> + * It is noted that this only exists on Broadwell GT3.
> + */
> +int intel_init_bsd2_ring_buffer(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring = &dev_priv->ring[VCS2];
> +
> +	if ((INTEL_INFO(dev)->gen != 8) ) {
> +		DRM_ERROR("No dual-BSD ring on non-BDW machine\n");
> +		return -EINVAL;
> +	}
> +
> +	ring->name = "second bsd ring";

"bsd2 ring" would be more concise

> +	ring->id = VCS2;
> +
> +	ring->write_tail = ring_write_tail;
> +	ring->mmio_base = GEN8_BSD2_RING_BASE;
> +	ring->flush = gen6_bsd_ring_flush;
> +	ring->add_request = gen6_add_request;
> +	ring->get_seqno = gen6_ring_get_seqno;
> +	ring->set_seqno = ring_set_seqno;
> +	ring->irq_enable_mask =
> +			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
> +	ring->irq_get = gen8_ring_get_irq;
> +	ring->irq_put = gen8_ring_put_irq;
> +	ring->dispatch_execbuffer =
> +			gen8_ring_dispatch_execbuffer;
> +	ring->sync_to = gen6_ring_sync;
> +	ring->semaphore_register[RCS] = MI_SEMAPHORE_SYNC_INVALID;
> +	ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_INVALID;
> +	ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_INVALID;
> +	ring->semaphore_register[VECS] = MI_SEMAPHORE_SYNC_INVALID;
> +	ring->semaphore_register[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
> +	ring->signal_mbox[RCS] = GEN6_NOSYNC;
> +	ring->signal_mbox[VCS] = GEN6_NOSYNC;
> +	ring->signal_mbox[BCS] = GEN6_NOSYNC;
> +	ring->signal_mbox[VECS] = GEN6_NOSYNC;
> +	ring->signal_mbox[VCS2] = GEN6_NOSYNC;
> +
> +	ring->init = init_ring_common;
> +
> +	return intel_init_ring_buffer(dev, ring);
> +}
> +
>  int intel_init_blt_ring_buffer(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2157,10 +2207,12 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>  	ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_BV;
>  	ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_INVALID;
>  	ring->semaphore_register[VECS] = MI_SEMAPHORE_SYNC_BVE;
> +	ring->semaphore_register[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
>  	ring->signal_mbox[RCS] = GEN6_RBSYNC;
>  	ring->signal_mbox[VCS] = GEN6_VBSYNC;
>  	ring->signal_mbox[BCS] = GEN6_NOSYNC;
>  	ring->signal_mbox[VECS] = GEN6_VEBSYNC;
> +	ring->signal_mbox[VCS2] = GEN6_NOSYNC;
>  	ring->init = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
> @@ -2198,10 +2250,12 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  	ring->semaphore_register[VCS] = MI_SEMAPHORE_SYNC_VEV;
>  	ring->semaphore_register[BCS] = MI_SEMAPHORE_SYNC_VEB;
>  	ring->semaphore_register[VECS] = MI_SEMAPHORE_SYNC_INVALID;
> +	ring->semaphore_register[VCS2] = MI_SEMAPHORE_SYNC_INVALID;
>  	ring->signal_mbox[RCS] = GEN6_RVESYNC;
>  	ring->signal_mbox[VCS] = GEN6_VVESYNC;
>  	ring->signal_mbox[BCS] = GEN6_BVESYNC;
>  	ring->signal_mbox[VECS] = GEN6_NOSYNC;
> +	ring->signal_mbox[VCS2] = GEN6_NOSYNC;
>  	ring->init = init_ring_common;
>  
>  	return intel_init_ring_buffer(dev, ring);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 413cdc7..8ca4285 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -61,8 +61,9 @@ struct  intel_ring_buffer {
>  		VCS,
>  		BCS,
>  		VECS,
> +		VCS2,
>  	} id;
> -#define I915_NUM_RINGS 4
> +#define I915_NUM_RINGS 5
>  	u32		mmio_base;
>  	void		__iomem *virtual_start;
>  	struct		drm_device *dev;
> @@ -286,6 +287,7 @@ int intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring);
>  
>  int intel_init_render_ring_buffer(struct drm_device *dev);
>  int intel_init_bsd_ring_buffer(struct drm_device *dev);
> +int intel_init_bsd2_ring_buffer(struct drm_device *dev);
>  int intel_init_blt_ring_buffer(struct drm_device *dev);
>  int intel_init_vebox_ring_buffer(struct drm_device *dev);
>  

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux