| Message ID | 1454095171-22475-3-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -Chris
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
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.
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@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
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@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.
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? */ goto out_unlock; + } ret = i915_gem_init_hw(dev); if (ret == -EIO) { diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 3a03646..60d7cdd 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1993,17 +1993,11 @@ static int gen8_init_rcs_context(struct drm_i915_gem_request *req) */ void intel_logical_ring_cleanup(struct intel_engine_cs *ring) { - struct drm_i915_private *dev_priv; - if (!intel_ring_initialized(ring)) return; - dev_priv = ring->dev->dev_private; - - if (ring->buffer) { - intel_logical_ring_stop(ring); - WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); - } + /* should not be set in LRC mode */ + WARN_ON(ring->buffer); if (ring->cleanup) ring->cleanup(ring);
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(-)