Re: [Intel-gfx] [PATCH 02/18] drm/i915: preliminary context support
On Wed, Mar 28, 2012 at 03:59:17PM -0700, Ben Widawsky wrote: > On Thu, 29 Mar 2012 00:43:00 +0200 > Daniel Vetter wrote: > > > 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 > > > > 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. > > > > I understand that you get no greater pleasure than bikeshedding my > patches until I want to cry... but seriously with the precedent, it's > in our driver already. So what do you want instead, setup()/teardown() > - init/fini? > > Anyway, here is the precedent: > i915_driver_UNLOAD()->i915_gem_context_unload() > i915_driver_LOAD()->i915_gem_LOAD() // which used to call my function > i915_driver_LOAD()->...->i915)gem_context_load() Well, I've fixed up gem_load, that's now also called init ;-) And driver_load and _unload are remnants from the stoneage, when these two functions could essentially only be called a moduel load/unload time. Now we have hotplug and drm drivers don't use stealth attach any more ... Anyway, I've seen a few things while reading your patches yesterday that looked puzzling and strange, but I didn't really know what to do with them. So I just added some easy bikeshed comments. After a nights worth of sleep I think I'm seeing clearer, so expect some real feedback soon. Cheers, Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/18] drm/i915: preliminary context support
On Thu, 29 Mar 2012 00:43:00 +0200 Daniel Vetter wrote: > 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 > > 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. > I understand that you get no greater pleasure than bikeshedding my patches until I want to cry... but seriously with the precedent, it's in our driver already. So what do you want instead, setup()/teardown() - init/fini? Anyway, here is the precedent: i915_driver_UNLOAD()->i915_gem_context_unload() i915_driver_LOAD()->i915_gem_LOAD() // which used to call my function i915_driver_LOAD()->...->i915)gem_context_load() As for your other comment, I do agree. The point of this patch was to show where the hooks go to allow people to review them. But without code, or comments explaining what the code will do - it's pretty useless. I think this can just be combined with the next patch. Also, some people may want to review it this way (it is easier for me to think of this way, for example). > > Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/18] drm/i915: preliminary context support
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
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.
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_clos
[Intel-gfx] [PATCH 02/18] drm/i915: preliminary context support
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
---
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/
