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

[Dave Gordon <david.s.gordon@intel.com>]

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.
>>>
>>> Signed-off-by: Dave Gordon <david.s.gordon@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.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx