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

[Dave Gordon]

On 11/02/16 08:47, Daniel Vetter wrote:
> 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

That was already done in the updated (v5) patchset posted 2016-02-05.
This (v4) sequence is therefore already obsolete.

.Dave.