| Message ID | 1450291011-31486-4-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/12/2015 18:36, Gordon, David S wrote: > 1. Fix intel_cleanup_ring_buffer() to handle the error cleanup > case where the ringbuffer has been allocated but map-and-pin > failed. Unpin it iff it's previously been mapped-and-pinned. > > 2. Fix the error path in intel_init_ring_buffer(), which already > called intel_destroy_ringbuffer_obj(), but failed to free the > actual ringbuffer structure. Calling intel_ringbuffer_free() > instead does both in one go. > > 3. With the above change, intel_destroy_ringbuffer_obj() is only > called in one place (intel_ringbuffer_free()), so flatten it > into that function. > > 4. move low-level register accesses from intel_cleanup_ring_buffer() > (which calls intel_stop_ring_buffer(ring) which calls stop_ring()) > down into stop_ring() itself), which is already doing low-level > register accesses. Then, intel_cleanup_ring_buffer() no longer > needs 'dev_priv'. > Reviewed-by: Nick Hoath <nicholas.hoath@intel.com> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++++++------------------ > 1 file changed, 22 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index eefce9a..2853754 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -549,6 +549,8 @@ static bool stop_ring(struct intel_engine_cs *ring) > I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING)); > } > > + WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); > + > return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0; > } > > @@ -2057,12 +2059,6 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > return 0; > } > > -static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > -{ > - drm_gem_object_unreference(&ringbuf->obj->base); > - ringbuf->obj = NULL; > -} > - > static int intel_alloc_ringbuffer_obj(struct drm_device *dev, > struct intel_ringbuffer *ringbuf) > { > @@ -2125,11 +2121,14 @@ intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int size) > } > > void > -intel_ringbuffer_free(struct intel_ringbuffer *ring) > +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf) > { > - intel_destroy_ringbuffer_obj(ring); > - list_del(&ring->link); > - kfree(ring); > + if (ringbuf->obj) { > + drm_gem_object_unreference(&ringbuf->obj->base); > + ringbuf->obj = NULL; > + } > + list_del(&ringbuf->link); > + kfree(ringbuf); > } > > static int intel_init_ring_buffer(struct drm_device *dev, > @@ -2157,6 +2156,13 @@ static int intel_init_ring_buffer(struct drm_device *dev, > } > ring->buffer = ringbuf; > > + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n", > + ring->name, ret); > + goto error; > + } > + > if (I915_NEED_GFX_HWS(dev)) { > ret = init_status_page(ring); > if (ret) > @@ -2168,14 +2174,6 @@ static int intel_init_ring_buffer(struct drm_device *dev, > goto error; > } > > - ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); > - if (ret) { > - DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n", > - ring->name, ret); > - intel_destroy_ringbuffer_obj(ringbuf); > - goto error; > - } > - > ret = i915_cmd_parser_init_ring(ring); > if (ret) > goto error; > @@ -2189,19 +2187,18 @@ error: > > void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) > { > - struct drm_i915_private *dev_priv; > + struct intel_ringbuffer *ringbuf; > > if (!intel_ring_initialized(ring)) > return; > > - dev_priv = to_i915(ring->dev); > - > - if (ring->buffer) { > + ringbuf = ring->buffer; > + if (ringbuf) { > intel_stop_ring_buffer(ring); > - WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); > > - intel_unpin_ringbuffer_obj(ring->buffer); > - intel_ringbuffer_free(ring->buffer); > + if (ringbuf->virtual_start) > + intel_unpin_ringbuffer_obj(ringbuf); > + intel_ringbuffer_free(ringbuf); > ring->buffer = NULL; > } > >
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index eefce9a..2853754 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -549,6 +549,8 @@ static bool stop_ring(struct intel_engine_cs *ring) I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING)); } + WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); + return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0; } @@ -2057,12 +2059,6 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, return 0; } -static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) -{ - drm_gem_object_unreference(&ringbuf->obj->base); - ringbuf->obj = NULL; -} - static int intel_alloc_ringbuffer_obj(struct drm_device *dev, struct intel_ringbuffer *ringbuf) { @@ -2125,11 +2121,14 @@ intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int size) } void -intel_ringbuffer_free(struct intel_ringbuffer *ring) +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf) { - intel_destroy_ringbuffer_obj(ring); - list_del(&ring->link); - kfree(ring); + if (ringbuf->obj) { + drm_gem_object_unreference(&ringbuf->obj->base); + ringbuf->obj = NULL; + } + list_del(&ringbuf->link); + kfree(ringbuf); } static int intel_init_ring_buffer(struct drm_device *dev, @@ -2157,6 +2156,13 @@ static int intel_init_ring_buffer(struct drm_device *dev, } ring->buffer = ringbuf; + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); + if (ret) { + DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n", + ring->name, ret); + goto error; + } + if (I915_NEED_GFX_HWS(dev)) { ret = init_status_page(ring); if (ret) @@ -2168,14 +2174,6 @@ static int intel_init_ring_buffer(struct drm_device *dev, goto error; } - ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); - if (ret) { - DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n", - ring->name, ret); - intel_destroy_ringbuffer_obj(ringbuf); - goto error; - } - ret = i915_cmd_parser_init_ring(ring); if (ret) goto error; @@ -2189,19 +2187,18 @@ error: void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) { - struct drm_i915_private *dev_priv; + struct intel_ringbuffer *ringbuf; if (!intel_ring_initialized(ring)) return; - dev_priv = to_i915(ring->dev); - - if (ring->buffer) { + ringbuf = ring->buffer; + if (ringbuf) { intel_stop_ring_buffer(ring); - WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); - intel_unpin_ringbuffer_obj(ring->buffer); - intel_ringbuffer_free(ring->buffer); + if (ringbuf->virtual_start) + intel_unpin_ringbuffer_obj(ringbuf); + intel_ringbuffer_free(ringbuf); ring->buffer = NULL; }
1. Fix intel_cleanup_ring_buffer() to handle the error cleanup case where the ringbuffer has been allocated but map-and-pin failed. Unpin it iff it's previously been mapped-and-pinned. 2. Fix the error path in intel_init_ring_buffer(), which already called intel_destroy_ringbuffer_obj(), but failed to free the actual ringbuffer structure. Calling intel_ringbuffer_free() instead does both in one go. 3. With the above change, intel_destroy_ringbuffer_obj() is only called in one place (intel_ringbuffer_free()), so flatten it into that function. 4. move low-level register accesses from intel_cleanup_ring_buffer() (which calls intel_stop_ring_buffer(ring) which calls stop_ring()) down into stop_ring() itself), which is already doing low-level register accesses. Then, intel_cleanup_ring_buffer() no longer needs 'dev_priv'. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++++++------------------ 1 file changed, 22 insertions(+), 25 deletions(-)