Re: [PATCH 02/18] drm/i915: preliminary context support

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Mar 18, 2012 at 01:39:42PM -0700, Ben Widawsky wrote:
> Very basic code for context setup/destruction in the driver.
> 
> There are 4 entry points into the contexts, load, unload, open, close.
> The names are self-explanatory except that load can be called during
> reset, and also during pm thaw/resume. As we expect our context to be
> preserved across these events, we do not reinitialize in this case.
> 
> Also an important note, as I intend to use contexts for ILK RC6, the
> context initialization must always come before RC6 initialization.
> 
> As Adam Jackson pointed out, I picked an arbitrary cutoff of 1MB where I
> decide the HW context is too big. The reason for this is even though
> context sizes are increasing with every generation, they are still
> measured in pages. If we somehow read back way more than that, it
> probably means BIOS has done something strange, or we're running on a
> platform that wasn't designed for this.
> 
> The 1MB was just a nice round number. I'm open to changing it to
> something sensible if someone has a better idea.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

<bikeshed> I see not that much precedence for _load and _unload for
setup/teardown ...

Also this patch is imo way too early in the series - you just add empty
functions so I have no idea what they're doing. And hence can't check
whether you add them at the right place. Whereas if this comes later I
already know what they're doing and can check without applying whether
they're all called at the right place.
</bikeshed>

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/Makefile           |    1 +
>  drivers/gpu/drm/i915/i915_dma.c         |    4 ++
>  drivers/gpu/drm/i915/i915_drv.c         |    1 +
>  drivers/gpu/drm/i915/i915_drv.h         |    9 +++
>  drivers/gpu/drm/i915/i915_gem.c         |    1 +
>  drivers/gpu/drm/i915/i915_gem_context.c |  114 +++++++++++++++++++++++++++++++
>  6 files changed, 130 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/i915_gem_context.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index ce7fc77..a625d30 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -7,6 +7,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
>  	  i915_debugfs.o \
>            i915_suspend.o \
>  	  i915_gem.o \
> +	  i915_gem_context.o \
>  	  i915_gem_debug.o \
>  	  i915_gem_evict.o \
>  	  i915_gem_execbuffer.o \
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 9341eb8..4c7c1dc 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2155,6 +2155,7 @@ int i915_driver_unload(struct drm_device *dev)
>  	ret = i915_gpu_idle(dev, true);
>  	if (ret)
>  		DRM_ERROR("failed to idle hardware: %d\n", ret);
> +	i915_gem_context_unload(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	/* Cancel the retire work handler, which should be idle now. */
> @@ -2244,6 +2245,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
>  	spin_lock_init(&file_priv->mm.lock);
>  	INIT_LIST_HEAD(&file_priv->mm.request_list);
>  
> +	i915_gem_context_open(dev, file);
> +
>  	return 0;
>  }
>  
> @@ -2276,6 +2279,7 @@ void i915_driver_lastclose(struct drm_device * dev)
>  
>  void i915_driver_preclose(struct drm_device * dev, struct drm_file *file_priv)
>  {
> +	i915_gem_context_close(dev, file_priv);
>  	i915_gem_release(dev, file_priv);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0694e17..b2c56db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -742,6 +742,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
>  		if (HAS_BLT(dev))
>  		    dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
>  
> +		i915_gem_context_load(dev);
>  		i915_gem_init_ppgtt(dev);
>  
>  		mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0f19f5..33c232a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -779,6 +779,9 @@ 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;
>  } drm_i915_private_t;
>  
>  enum hdmi_force_audio {
> @@ -1280,6 +1283,12 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_device *dev,
>  int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  				    enum i915_cache_level cache_level);
>  
> +/* i915_gem_context.c */
> +void i915_gem_context_load(struct drm_device *dev);
> +void i915_gem_context_unload(struct drm_device *dev);
> +void i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> +void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
> +
>  /* i915_gem_gtt.c */
>  int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1f441f5..6343a82 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3811,6 +3811,7 @@ i915_gem_init_hw(struct drm_device *dev)
>  
>  	dev_priv->next_seqno = 1;
>  
> +	i915_gem_context_load(dev);
>  	i915_gem_init_ppgtt(dev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> new file mode 100644
> index 0000000..caa0e06
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -0,0 +1,114 @@
> +/*
> + * Copyright © 2012 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Ben Widawsky <ben@bwidawsk.net>
> + *
> + */
> +
> +#include "drmP.h"
> +#include "i915_drm.h"
> +#include "i915_drv.h"
> +
> +static int get_context_size(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
> +	u32 reg;
> +
> +	/* Context size (as of gen7) is determined in number of cache lines */
> +	switch (INTEL_INFO(dev)->gen) {
> +	case 5: /* ILK & SNB have the same context reg layout */
> +	case 6:
> +		reg = I915_READ(CXT_SIZE);
> +		ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
> +		break;
> +	case 7:
> +		reg = I915_READ(GEN7_CTX_SIZE);
> +		ret = GEN7_CTX_TOTAL_SIZE(reg) * 64;
> +		break;
> +	default:
> +		ret = -1;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * The default context needs to exist per ring that uses contexts. It stores the
> + * context state of the GPU for applications that don't utilize HW contexts, as
> + * well as an idle case.
> + */
> +static int create_default_context(struct drm_i915_private *dev_priv)
> +{
> +	return 0;
> +}
> +
> +void i915_gem_context_load(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	uint32_t ctx_size;
> +
> +	/* If called from reset, or thaw... we've been here already */
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +
> +	ctx_size = get_context_size(dev);
> +	dev_priv->hw_context_size = get_context_size(dev);
> +	dev_priv->hw_context_size = round_up(dev_priv->hw_context_size, 4096);
> +
> +	if (ctx_size <= 0 || ctx_size > (1<<20)) {
> +		dev_priv->hw_contexts_disabled = true;
> +		return;
> +	}
> +
> +	if (create_default_context(dev_priv)) {
> +		dev_priv->hw_contexts_disabled = true;
> +		return;
> +	}
> +
> +	DRM_DEBUG_DRIVER("HW context support initialized\n");
> +}
> +
> +void i915_gem_context_unload(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +}
> +
> +void i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +}
> +
> +void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (dev_priv->hw_contexts_disabled)
> +		return;
> +}
> -- 
> 1.7.9.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48