| Message ID | 1453504211-7982-4-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 22, 2016 at 11:10:08PM +0000, Dave Gordon 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'. > > 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 9030e2b..29de64e 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -551,6 +551,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; > } > > @@ -2071,12 +2073,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) > { > @@ -2139,11 +2135,14 @@ struct intel_ringbuffer * > } > > void > -intel_ringbuffer_free(struct intel_ringbuffer *ring) > +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf) ringbuf? > - intel_destroy_ringbuffer_obj(ring); > - list_del(&ring->link); > - kfree(ring); > + if (ringbuf->obj) { > + drm_gem_object_unreference(&ringbuf->obj->base); Already tests for NULL > + ringbuf->obj = NULL; Redundant as we are aobut to free it. > + } > + list_del(&ringbuf->link); > + kfree(ringbuf); > } > > static int intel_init_ring_buffer(struct drm_device *dev, > @@ -2171,6 +2170,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) > @@ -2182,14 +2188,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; > @@ -2203,19 +2201,18 @@ static int intel_init_ring_buffer(struct drm_device *dev, > > void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) > { > - struct drm_i915_private *dev_priv; > + struct intel_ringbuffer *ringbuf; 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) Cleaner code, and more idiomatic, if we let unpin early return. > + intel_unpin_ringbuffer_obj(ringbuf); > + intel_ringbuffer_free(ringbuf); > ring->buffer = NULL; > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 25/01/16 10:52, Chris Wilson wrote: > On Fri, Jan 22, 2016 at 11:10:08PM +0000, Dave Gordon 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'. >> >> 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 9030e2b..29de64e 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -551,6 +551,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; >> } >> >> @@ -2071,12 +2073,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) >> { >> @@ -2139,11 +2135,14 @@ struct intel_ringbuffer * >> } >> >> void >> -intel_ringbuffer_free(struct intel_ringbuffer *ring) >> +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf) > > ringbuf? Yes, it's a ring*buffer*, so 'ringbuf' is a good name for it. It's NOT an *engine*, which is what the name 'ring' has historically been used for. I count 276 instances of 'ringbuf' in the driver; AFAICT, they all refer to a 'struct intel_ringbuffer'. Of the ~2000 instances of 'ring', they nearly (but not quite) all refer to a 'struct intel_engine_cs' :( Someday we may want to call ringbuffers 'ring', but not until the last vestige of the earlier usage has been purged from the codebase and from people's heads. For now, the name 'ringbuf' will reduce confusion. >> - intel_destroy_ringbuffer_obj(ring); >> - list_del(&ring->link); >> - kfree(ring); >> + if (ringbuf->obj) { >> + drm_gem_object_unreference(&ringbuf->obj->base); > > Already tests for NULL But there's no guarantee that ringbuf->obj == NULL implies that &ringbuf->obj->base == NULL. It may happen to be so at present, but that's an implementation detail, so I'm not assuming that it will always be so. I suspect also that some static analysis tools would complain about this construct if it didn't have the test-before-use. >> + ringbuf->obj = NULL; > > Redundant as we are aobut to free it. True, but it's harmless. >> + } >> + list_del(&ringbuf->link); >> + kfree(ringbuf); >> } >> >> static int intel_init_ring_buffer(struct drm_device *dev, >> @@ -2171,6 +2170,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) >> @@ -2182,14 +2188,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; >> @@ -2203,19 +2201,18 @@ static int intel_init_ring_buffer(struct drm_device *dev, >> >> void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) >> { >> - struct drm_i915_private *dev_priv; >> + struct intel_ringbuffer *ringbuf; > > ringbuf? See above. Also the name 'ring' here already refers to an engine. >> 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) > > Cleaner code, and more idiomatic, if we let unpin early return. Maybe, but that's not the way it was previously written, so I didn't change it around. >> + intel_unpin_ringbuffer_obj(ringbuf); >> + intel_ringbuffer_free(ringbuf); >> ring->buffer = NULL; >> } >> >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Jan 25, 2016 at 12:08:11PM +0000, Dave Gordon wrote: > On 25/01/16 10:52, Chris Wilson wrote: > >On Fri, Jan 22, 2016 at 11:10:08PM +0000, Dave Gordon wrote: > >>+ 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) > > > >Cleaner code, and more idiomatic, if we let unpin early return. > > Maybe, but that's not the way it was previously written, so I didn't change > it around. We unfortunately let a lot of these through ... Early returns are preferred, if it's possible. Same for skips in loops using if (!cond) continue;, it makes for much less right-leaning code. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 9030e2b..29de64e 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -551,6 +551,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; } @@ -2071,12 +2073,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) { @@ -2139,11 +2135,14 @@ struct intel_ringbuffer * } 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, @@ -2171,6 +2170,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) @@ -2182,14 +2188,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; @@ -2203,19 +2201,18 @@ static int intel_init_ring_buffer(struct drm_device *dev, 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(-)