On Wed, 2014-04-16 at 10:23 -0600, Deak, Imre wrote:
> 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.z...@intel.com>
>
> A couple of nitpicks below, with or without those:
> Reviewed-by: Imre Deak <imre.d...@intel.com>
Hi, Imre
Thanks for your review and the comments.
I will update the patch based on your comment.
>
> > ---
> > 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
>
OK. I will update it.
> > 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().
The current code for the semaphore initialization is only applied for
pre-gen8 platform. The VCS2 only exists on the Broadwell GT3 and there
is no VCS2 on the other platform. So I initialize the semaphore stuff
differently for VCS and VCS2.
OK. I will add more comment to clarify it.
>
> > 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);
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
