[Intel-gfx] [RFC] [PATCH] drm/i915: Require HW contexts (when possible)
Ben Widawsky
ben at bwidawsk.net
Thu Oct 17 20:01:05 CEST 2013
On Thu, Oct 17, 2013 at 08:52:17PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 17, 2013 at 09:59:43AM -0700, Ben Widawsky wrote:
> > From: Ben Widawsky <benjamin.widawsky at intel.com>
> >
> > Only compile tested
> >
> > CC: Kenneth Graunke <kenneth at whitecape.org>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 5 +---
> > drivers/gpu/drm/i915/i915_drv.h | 3 +--
> > drivers/gpu/drm/i915/i915_gem.c | 7 +++++-
> > drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++++++------------------
> > drivers/gpu/drm/i915/i915_sysfs.c | 6 ++---
> > 5 files changed, 31 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1060a96..8c90559 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -750,14 +750,11 @@ int i915_reset(struct drm_device *dev)
> > */
> > if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> > !dev_priv->ums.mm_suspended) {
> > - bool hw_contexts_disabled = dev_priv->hw_contexts_disabled;
> > dev_priv->ums.mm_suspended = 0;
> >
> > ret = i915_gem_init_hw(dev);
> > - if (!hw_contexts_disabled && dev_priv->hw_contexts_disabled)
> > - DRM_ERROR("HW contexts didn't survive reset\n");
> > - mutex_unlock(&dev->struct_mutex);
> > if (ret) {
> > + mutex_unlock(&dev->struct_mutex);
>
> Botched locking.
My mistake was intentional at least, and not the usual copy/paste.
Somehow this looked right to me earlier.
Thanks.
>
> > DRM_ERROR("Failed hw init on reset %d\n", ret);
> > return ret;
> > }
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index e4ff8e9..21b2c37 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1411,7 +1411,6 @@ typedef struct drm_i915_private {
> > struct drm_property *broadcast_rgb_property;
> > struct drm_property *force_audio_property;
> >
> > - bool hw_contexts_disabled;
> > uint32_t hw_context_size;
> > struct list_head context_list;
> >
> > @@ -2140,7 +2139,7 @@ i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
> > }
> >
> > /* i915_gem_context.c */
> > -void i915_gem_context_init(struct drm_device *dev);
> > +int __must_check i915_gem_context_init(struct drm_device *dev);
> > void i915_gem_context_fini(struct drm_device *dev);
> > void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> > int i915_switch_context(struct intel_ring_buffer *ring,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 34df59b..0df40e6 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4463,7 +4463,12 @@ i915_gem_init_hw(struct drm_device *dev)
> > * XXX: There was some w/a described somewhere suggesting loading
> > * contexts before PPGTT.
> > */
> > - i915_gem_context_init(dev);
> > + ret = i915_gem_context_init(dev);
> > + if (ret) {
>
> i915_gem_cleanup_ringbuffer() ?
>
> Damn our error unwinding is hard to follow, which means it's probably
> broken somewhere. From i915_load_modeset_init() we call
> i915_gem_init() which does some stuff directly and then calls
> i915_gem_init_hw(). And if we get some error later in i915_load_modeset_init(),
> we manually unwind all the stuff that i915_gem_init() and i915_gem_init_hw()
> supposedly did. This could sure use some i915_gem_fini() and i915_gem_fini_hw()
> to keep the do+undo code close together.
>
It's definitely a mess. TBF, it's been changing a lot more recently than
it had in the past.
But yeah this is missing. I wonder if I missed this last time I wrote
the patch too...? Thanks.
> > + DRM_ERROR("Context initialization failed %d\n", ret);
> > + return ret;
> > + }
> > +
> > if (dev_priv->mm.aliasing_ppgtt) {
> > ret = dev_priv->mm.aliasing_ppgtt->enable(dev);
> > if (ret) {
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index cc619c1..4625670 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -244,36 +244,34 @@ err_destroy:
> > return ret;
> > }
> >
> > -void i915_gem_context_init(struct drm_device *dev)
> > +int i915_gem_context_init(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + int ret;
> >
> > - if (!HAS_HW_CONTEXTS(dev)) {
> > - dev_priv->hw_contexts_disabled = true;
> > - DRM_DEBUG_DRIVER("Disabling HW Contexts; old hardware\n");
> > - return;
> > - }
> > + if (!HAS_HW_CONTEXTS(dev))
> > + return 0;
> >
> > /* If called from reset, or thaw... we've been here already */
> > - if (dev_priv->hw_contexts_disabled ||
> > - dev_priv->ring[RCS].default_context)
> > - return;
> > + if (dev_priv->ring[RCS].default_context)
> > + return 0;
> >
> > dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
> >
> > if (dev_priv->hw_context_size > (1<<20)) {
> > - dev_priv->hw_contexts_disabled = true;
> > DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size\n");
> > - return;
> > + return -E2BIG;
> > }
> >
> > - if (create_default_context(dev_priv)) {
> > - dev_priv->hw_contexts_disabled = true;
> > - DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed\n");
> > - return;
> > + ret = create_default_context(dev_priv);
> > + if (ret) {
> > + DRM_DEBUG_DRIVER("Disabling HW Contexts; create failed %d\n",
> > + ret);
> > + return ret;
> > }
> >
> > DRM_DEBUG_DRIVER("HW context support initialized\n");
> > + return 0;
> > }
> >
> > void i915_gem_context_fini(struct drm_device *dev)
> > @@ -281,7 +279,7 @@ void i915_gem_context_fini(struct drm_device *dev)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct i915_hw_context *dctx = dev_priv->ring[RCS].default_context;
> >
> > - if (dev_priv->hw_contexts_disabled)
> > + if (!HAS_HW_CONTEXTS(dev))
> > return;
> >
> > /* The only known way to stop the gpu from accessing the hw context is
> > @@ -324,16 +322,16 @@ i915_gem_context_get_hang_stats(struct drm_device *dev,
> > struct drm_file *file,
> > u32 id)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_file_private *file_priv = file->driver_priv;
> > struct i915_hw_context *ctx;
> >
> > if (id == DEFAULT_CONTEXT_ID)
> > return &file_priv->hang_stats;
> >
> > - ctx = NULL;
> > - if (!dev_priv->hw_contexts_disabled)
> > - ctx = i915_gem_context_get(file->driver_priv, id);
> > + if (!HAS_HW_CONTEXTS(dev))
> > + return ERR_PTR(-ENOENT);
> > +
> > + ctx = i915_gem_context_get(file->driver_priv, id);
> > if (ctx == NULL)
> > return ERR_PTR(-ENOENT);
> >
> > @@ -506,7 +504,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> > struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > struct i915_hw_context *to;
> >
> > - if (dev_priv->hw_contexts_disabled)
> > + if (!HAS_HW_CONTEXTS(ring->dev))
> > return 0;
> >
> > WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
> > @@ -531,7 +529,6 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> > int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > struct drm_file *file)
> > {
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_context_create *args = data;
> > struct drm_i915_file_private *file_priv = file->driver_priv;
> > struct i915_hw_context *ctx;
> > @@ -540,7 +537,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> > if (!(dev->driver->driver_features & DRIVER_GEM))
> > return -ENODEV;
> >
> > - if (dev_priv->hw_contexts_disabled)
> > + if (!HAS_HW_CONTEXTS(dev))
> > return -ENODEV;
> >
> > ret = i915_mutex_lock_interruptible(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index bb31ff3..47bc51b 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -183,13 +183,13 @@ i915_l3_write(struct file *filp, struct kobject *kobj,
> > int slice = (int)(uintptr_t)attr->private;
> > int ret;
> >
> > + if (!HAS_HW_CONTEXTS(drm_dev))
> > + return -ENXIO;
> > +
> > ret = l3_access_valid(drm_dev, offset);
> > if (ret)
> > return ret;
> >
> > - if (dev_priv->hw_contexts_disabled)
> > - return -ENXIO;
> > -
> > ret = i915_mutex_lock_interruptible(drm_dev);
> > if (ret)
> > return ret;
> > --
> > 1.8.4.1
> >
Nobody is commenting on whether or not this is okay, which is what I was
really interesting.
--
Ben Widawsky, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list