Re: [PATCH 04/50] drm/i915: Extract trivial parts of ring init (early init)

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 09, 2014 at 01:08:34PM +0100, oscar.mateo@intel.com wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> It's beneficial to be able to get a name, base, and id before we've
> actually initialized the rings. This ability was effectively destroyed
> in the ringbuffer fire which Daniel started.
> 
> With the simple early init function, that ability is restored.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> v2: The Full PPGTT series have moved things around a little bit.
> Also, don't forget the VEBOX.
> 
> v3: Checking ring->dev is not a good way to test if a ring is
> initialized...
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

Needs to be updated for VEBOX2. Also I don't really see the point, where
exactly do we need this? Ripping apart the ring init like this doesn't
look too great imo.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  2 ++
>  drivers/gpu/drm/i915/i915_gpu_error.c   |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 60 ++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 +
>  4 files changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ce941cf..6ef53bd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4502,6 +4502,8 @@ int i915_gem_init(struct drm_device *dev)
>  
>  	i915_gem_init_global_gtt(dev);
>  
> +	intel_init_rings_early(dev);
> +
>  	ret = i915_gem_context_init(dev);
>  	if (ret) {
>  		mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 2d81985..8f37238 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -886,7 +886,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  	for (i = 0; i < I915_NUM_RINGS; i++) {
>  		struct intel_ring_buffer *ring = &dev_priv->ring[i];
>  
> -		if (ring->dev == NULL)
> +		if (!intel_ring_initialized(ring))
>  			continue;
>  
>  		error->ring[i].valid = true;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index a112971..fc737c8 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1417,7 +1417,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  {
>  	int ret;
>  
> -	ring->dev = dev;
>  	INIT_LIST_HEAD(&ring->active_list);
>  	INIT_LIST_HEAD(&ring->request_list);
>  	ring->size = 32 * PAGE_SIZE;
> @@ -1908,10 +1907,6 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
>  
> -	ring->name = "render ring";
> -	ring->id = RCS;
> -	ring->mmio_base = RENDER_RING_BASE;
> -
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		ring->add_request = gen6_add_request;
>  		ring->flush = gen7_render_ring_flush;
> @@ -2019,10 +2014,6 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
>  	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
>  	int ret;
>  
> -	ring->name = "render ring";
> -	ring->id = RCS;
> -	ring->mmio_base = RENDER_RING_BASE;
> -
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		/* non-kms not supported on gen6+ */
>  		return -ENODEV;
> @@ -2056,7 +2047,6 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
>  	ring->init = init_render_ring;
>  	ring->cleanup = render_ring_cleanup;
>  
> -	ring->dev = dev;
>  	INIT_LIST_HEAD(&ring->active_list);
>  	INIT_LIST_HEAD(&ring->request_list);
>  
> @@ -2086,12 +2076,8 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *ring = &dev_priv->ring[VCS];
>  
> -	ring->name = "bsd ring";
> -	ring->id = VCS;
> -
>  	ring->write_tail = ring_write_tail;
>  	if (INTEL_INFO(dev)->gen >= 6) {
> -		ring->mmio_base = GEN6_BSD_RING_BASE;
>  		/* gen6 bsd needs a special wa for tail updates */
>  		if (IS_GEN6(dev))
>  			ring->write_tail = gen6_bsd_ring_write_tail;
> @@ -2132,7 +2118,6 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>  		ring->semaphore.mbox.signal[VECS] = GEN6_VEVSYNC;
>  		ring->semaphore.mbox.signal[VCS2] = GEN6_NOSYNC;
>  	} else {
> -		ring->mmio_base = BSD_RING_BASE;
>  		ring->flush = bsd_ring_flush;
>  		ring->add_request = i9xx_add_request;
>  		ring->get_seqno = ring_get_seqno;
> @@ -2167,11 +2152,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>  		return -EINVAL;
>  	}
>  
> -	ring->name = "bds2_ring";
> -	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;
> @@ -2210,10 +2191,6 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
>  
> -	ring->name = "blitter ring";
> -	ring->id = BCS;
> -
> -	ring->mmio_base = BLT_RING_BASE;
>  	ring->write_tail = ring_write_tail;
>  	ring->flush = gen6_ring_flush;
>  	ring->add_request = gen6_add_request;
> @@ -2259,10 +2236,6 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_ring_buffer *ring = &dev_priv->ring[VECS];
>  
> -	ring->name = "video enhancement ring";
> -	ring->id = VECS;
> -
> -	ring->mmio_base = VEBOX_RING_BASE;
>  	ring->write_tail = ring_write_tail;
>  	ring->flush = gen6_ring_flush;
>  	ring->add_request = gen6_add_request;
> @@ -2351,3 +2324,36 @@ intel_stop_ring_buffer(struct intel_ring_buffer *ring)
>  
>  	stop_ring(ring);
>  }
> +
> +void intel_init_rings_early(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	dev_priv->ring[RCS].name = "render ring";
> +	dev_priv->ring[RCS].id = RCS;
> +	dev_priv->ring[RCS].mmio_base = RENDER_RING_BASE;
> +	dev_priv->ring[RCS].dev = dev;
> +
> +	dev_priv->ring[BCS].name = "blitter ring";
> +	dev_priv->ring[BCS].id = BCS;
> +	dev_priv->ring[BCS].mmio_base = BLT_RING_BASE;
> +	dev_priv->ring[BCS].dev = dev;
> +
> +	dev_priv->ring[VCS].name = "bsd ring";
> +	dev_priv->ring[VCS].id = VCS;
> +	if (INTEL_INFO(dev)->gen >= 6)
> +		dev_priv->ring[VCS].mmio_base = GEN6_BSD_RING_BASE;
> +	else
> +		dev_priv->ring[VCS].mmio_base = BSD_RING_BASE;
> +	dev_priv->ring[VCS].dev = dev;
> +
> +	dev_priv->ring[VCS2].name = "bds2_ring";
> +	dev_priv->ring[VCS2].id = VCS2;
> +	dev_priv->ring[VCS2].mmio_base = GEN8_BSD2_RING_BASE;
> +	dev_priv->ring[VCS2].dev = dev;
> +
> +	dev_priv->ring[VECS].name = "video enhancement ring";
> +	dev_priv->ring[VECS].id = VECS;
> +	dev_priv->ring[VECS].mmio_base = VEBOX_RING_BASE;
> +	dev_priv->ring[VECS].dev = dev;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 72c3c15..b1bf767 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -297,6 +297,7 @@ void intel_ring_init_seqno(struct intel_ring_buffer *ring, u32 seqno);
>  int intel_ring_flush_all_caches(struct intel_ring_buffer *ring);
>  int intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring);
>  
> +void intel_init_rings_early(struct drm_device *dev);
>  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);
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch