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@xxxxxxxxx>
> >>>---
> >>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
