[Intel-gfx] [PATCH v4 2/6] drm/i915: tidy up initialisation failure paths (GEM & LRC)

[Daniel Vetter]

On Mon, Feb 01, 2016 at 09:45:25AM +0000, Dave Gordon wrote:
> On 30/01/16 11:28, Chris Wilson wrote:
> >On Sat, Jan 30, 2016 at 10:56:27AM +0000, Chris Wilson wrote:
> >>On Fri, Jan 29, 2016 at 07:19:27PM +0000, Dave Gordon wrote:
> >>>1. add call to i915_gem_context_fini() to deallocate the default
> >>>    context(s) if the call to init_rings() fails, so that we don't
> >>>    leak the context in that situation.
> >>>
> >>>2. remove useless code in intel_logical_ring_cleanup(), presumably
> >>>    copypasted from legacy ringbuffer version at creation.
> >>>

If your commit message has a list of independent changes ...

> >>>Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> >>>---
> >>>  drivers/gpu/drm/i915/i915_gem.c  |  5 ++++-
> >>>  drivers/gpu/drm/i915/intel_lrc.c | 10 ++--------
> >>>  2 files changed, 6 insertions(+), 9 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>>index a928823..5a4d468 100644
> >>>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>>@@ -4986,8 +4986,11 @@ int i915_gem_init(struct drm_device *dev)
> >>>  		goto out_unlock;
> >>>
> >>>  	ret = dev_priv->gt.init_rings(dev);
> >>>-	if (ret)
> >>>+	if (ret) {
> >>>+		i915_gem_context_fini(dev);
> >>>+		/* XXX: anything else to be undone here? */
> >>
> >>Yes. Make this a separate patch and begin the onion unwind.
> >
> >Hmm. Actually, we have to make sure that we can still modeset if this
> >function fails - that is anything but utter catastrophe should just
> >result in loss of functionality (no stolen, no GEM execution etc) but we
> >can still drive the displays so the user can see how bad the damage is.
> >-Chris
> 
> Yes, Mika said that's why (he thought) there wasn't a complete reversal of
> everything the driver has done up to this point.
> 
> The addition of the context_fini() seems reasonable, that's going to make it
> leak a bit less, while still leaving basic framebuffer working.
> 
> Could be a separate patch if you like, but hardly seems worth splitting from
> the other chunk, which after all only replaces unreachable code with a
> WARNing.

... it should be split. As a rule of thumb at least. I don't really care
all that much here though, so jut for the future.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch