[Intel-gfx] [PATCH] drm/i915: Per-process stats work better when evaluated per-process
The idea of printing objects used by each process is to judge how each
process is using them. This means that we need to evaluate whether the
object is bound for that particular process, rather than just whether it
is bound into the global GTT.
Signed-off-by: Chris Wilson
Cc: Ben Widawsky
---
drivers/gpu/drm/i915/i915_debugfs.c | 34 ++---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_gem_context.c | 1 +
3 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 95a97ef45a3e..ab7b6f3b15df 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -299,28 +299,46 @@ static int i915_gem_stolen_list_info(struct seq_file *m,
void *data)
} while (0)
struct file_stats {
+ struct drm_i915_file_private *file_priv;
int count;
- size_t total, active, inactive, unbound;
+ size_t total, global, active, inactive, unbound;
};
static int per_file_stats(int id, void *ptr, void *data)
{
struct drm_i915_gem_object *obj = ptr;
struct file_stats *stats = data;
+ struct i915_vma *vma;
stats->count++;
stats->total += obj->base.size;
- if (i915_gem_obj_ggtt_bound(obj)) {
- if (!list_empty(&obj->ring_list))
+ list_for_each_entry(vma, &obj->vma_list, vma_link) {
+ struct i915_hw_ppgtt *ppgtt;
+
+ if (!drm_mm_node_allocated(&vma->node))
+ continue;
+
+ ppgtt = container_of(vma->vm, struct i915_hw_ppgtt, base);
+ if (ppgtt->ctx == NULL) {
+ stats->global += obj->base.size;
+ continue;
+ }
+
+ if (ppgtt->ctx->file_priv != stats->file_priv)
+ continue;
+
+ if (obj->ring) /* XXX per-vma statistic */
stats->active += obj->base.size;
else
stats->inactive += obj->base.size;
- } else {
- if (!list_empty(&obj->global_list))
- stats->unbound += obj->base.size;
+
+ return 0;
}
+ if (!list_empty(&obj->global_list))
+ stats->unbound += obj->base.size;
+
return 0;
}
@@ -411,6 +429,7 @@ static int i915_gem_object_info(struct seq_file *m, void*
data)
struct task_struct *task;
memset(&stats, 0, sizeof(stats));
+ stats.file_priv = file->driver_priv;
idr_for_each(&file->object_idr, per_file_stats, &stats);
/*
* Although we have a valid reference on file->pid, that does
@@ -420,12 +439,13 @@ static int i915_gem_object_info(struct seq_file *m, void*
data)
*/
rcu_read_lock();
task = pid_task(file->pid, PIDTYPE_PID);
- seq_printf(m, "%s: %u objects, %zu bytes (%zu active, %zu
inactive, %zu unbound)\n",
+ seq_printf(m, "%s: %u objects, %zu bytes (%zu active, %zu
inactive, %zu global, %zu unbound)\n",
task ? task->comm : "",
stats.count,
stats.total,
stats.active,
stats.inactive,
+ stats.global,
stats.unbound);
rcu_read_unlock();
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 49f5ce77ef21..c4aa3f8c0ba4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -684,6 +684,8 @@ struct i915_hw_ppgtt {
dma_addr_t *gen8_pt_dma_addr[4];
};
+ struct i915_hw_context *ctx;
+
int (*enable)(struct i915_hw_ppgtt *ppgtt);
int (*switch_mm)(struct i915_hw_ppgtt *ppgtt,
struct intel_ring_buffer *ring,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 6bbf0654b9b5..a1692227b55b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -174,6 +174,7 @@ create_vm_for_ctx(struct drm_device *dev, struct
i915_hw_context *ctx)
return ERR_PTR(ret);
}
+ ppgtt->ctx = ctx;
return ppgtt;
}
--
1.9.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: add a display info file to debugfs
On Mon, Jan 20, 2014 at 9:55 PM, Jesse Barnes wrote:
> Can be expanded up on to include all sorts of things (HDMI infoframe
> data, more DP status, etc). Should be useful for bug reports to get a
> baseline on the display config and info.
>
> Signed-off-by: Jesse Barnes
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 155
>
> drivers/gpu/drm/i915/i915_drv.h | 4 +
> 2 files changed, 159 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 75a489e..20ae7ed 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1992,6 +1992,160 @@ static int i915_power_domain_info(struct seq_file *m,
> void *unused)
> return 0;
> }
>
> +static void intel_encoder_info(struct seq_file *m,
> + struct intel_crtc *intel_crtc,
> + struct intel_encoder *intel_encoder)
> +{
> + struct drm_info_node *node = (struct drm_info_node *) m->private;
> + struct drm_device *dev = node->minor->dev;
> + struct drm_crtc *crtc = &intel_crtc->base;
> + struct intel_connector *intel_connector;
> + struct drm_encoder *encoder;
> +
> + encoder = &intel_encoder->base;
> + seq_printf(m, "\tencoder %d: type: %s, connectors:\n",
> + encoder->base.id, drm_get_encoder_name(encoder));
> + for_each_connector_on_encoder(dev, encoder, intel_connector) {
> + struct drm_connector *connector = &intel_connector->base;
> + seq_printf(m, "\t\tconnector %d: type: %s, status: %s",
> + connector->base.id,
> + drm_get_connector_name(connector),
> + drm_get_connector_status_name(connector->status));
> + if (connector->status == connector_status_connected) {
> + struct drm_display_mode *mode = &crtc->mode;
> + seq_printf(m, ", mode:\n");
> + seq_printf(m, "\t\t%d:\"%s\" %d %d %d %d %d %d %d %d
> %d %d 0x%x 0x%x\n",
> + mode->base.id, mode->name,
> + mode->vrefresh, mode->clock,
> + mode->hdisplay, mode->hsync_start,
> + mode->hsync_end, mode->htotal,
> + mode->vdisplay, mode->vsync_start,
> + mode->vsync_end, mode->vtotal,
> + mode->type, mode->flags);
> + } else {
> + seq_printf(m, "\n");
for cases like this shouldn't we use seq_put instead of seq_printf?
> + }
> + }
> +}
> +
> +static void intel_crtc_info(struct seq_file *m, struct intel_crtc
> *intel_crtc)
> +{
> + struct drm_info_node *node = (struct drm_info_node *) m->private;
> + struct drm_device *dev = node->minor->dev;
> + struct drm_crtc *crtc = &intel_crtc->base;
> + struct intel_encoder *intel_encoder;
> +
> + seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d\n",
> + crtc->fb->base.id, crtc->x, crtc->y,
> + crtc->fb->width, crtc->fb->height);
> + for_each_encoder_on_crtc(dev, crtc, intel_encoder)
> + intel_encoder_info(m, intel_crtc, intel_encoder);
> +}
> +
> +static void intel_panel_info(struct seq_file *m, struct intel_panel *panel)
> +{
> + struct drm_display_mode *mode = panel->fixed_mode;
> +
> + seq_printf(m, "\tfixed mode:\n");
> + seq_printf(m, "\t\t%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x
> 0x%x\n",
> + mode->base.id, mode->name,
> + mode->vrefresh, mode->clock,
> + mode->hdisplay, mode->hsync_start,
> + mode->hsync_end, mode->htotal,
> + mode->vdisplay, mode->vsync_start,
> + mode->vsync_end, mode->vtotal,
> + mode->type, mode->flags);
> +}
I would prefer a more bloated info, with variable_name =
vriable_value... I know this is bloated, but I'm also think on our
lazyness when reading bug reports ;)
> +
> +static void intel_dp_info(struct seq_file *m,
> + struct intel_connector *intel_connector)
> +{
> + struct intel_encoder *intel_encoder = intel_connector->encoder;
> + struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +
> + seq_printf(m, "\tDPCD rev: %x\n", intel_dp->dpcd[DP_DPCD_REV]);
> + seq_printf(m, "\taudio support: %s\n", intel_dp->has_audio ? "yes" :
> + "no");
> + if (intel_encoder->type == INTEL_OUTPUT_EDP)
> + intel_panel_info(m, &intel_connector->panel);
> +}
> +
> +static void intel_hdmi_info(struct seq_file *m,
> + struct intel_connector *intel_connector)
> +{
> + struct intel_en
Re: [Intel-gfx] kernfs oops with i915+i2c_core in 3.14 merge window
On Thu, Jan 30, 2014 at 2:20 PM, Josh Boyer wrote: > On Thu, Jan 30, 2014 at 2:05 PM, Tejun Heo wrote: >> On Thu, Jan 30, 2014 at 02:03:18PM -0500, Josh Boyer wrote: >>> Hi All, >>> >>> I'm seeing the oops below on my MacBookPro 10,2 machine using i915 >>> graphics. It's after the DRM merge for 3.14 ( v3.13-10094-g9b0cd30) , >>> but we seem to have one report[1] of this happening well before that, >>> in v3.13-3260-g03d11a0 as well. Does anyone have a clue what is going >>> on here? >>> >>> https://bugzilla.redhat.com/show_bug.cgi?id=1055105 >> >> Should be fixed by the following patch which is already queued. >> >> http://lkml.kernel.org/g/20140129170403.gj30...@htj.dyndns.org > > Oh, excellent! I'll throw that into a build and test it here. Thanks > for the quick reply, Tejun. FWIW, my test build with that patch does seem to solve the problem. Thanks again. josh ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Add register whitelist for DRM master
Re-adding intel-gfx, this seems generally useful. On Thu, Jan 30, 2014 at 6:22 PM, Volkin, Bradley D wrote: > Ok. There's still a couple ways I could see doing it. > - one big patch with all the new #defines, up front > - split the #defines from their current patches, all up front > - split the #defines from their current patches, just before the patch that > uses them > > I guess I would tend towards the 3rd option, but maybe that's overkill. Which > do you suggest? Whatever you like better, and tbh I'd wait for review and only split things if you need to. In essence this was just a suggestion for the future for smoother merging. For the patches themselves the riskiest thing from my maintainer pov is breaking old userspace. So I'd like to get the command streamer merged as soon as possible, and enabled in enforcing mode. We can then plug in all the missing pieces while getting test coverage in the wild (lots of people tend to use -nightly). I've sigend up Jani to review the current series in detail, including the tests. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] kernfs oops with i915+i2c_core in 3.14 merge window
On Thu, Jan 30, 2014 at 2:05 PM, Tejun Heo wrote: > On Thu, Jan 30, 2014 at 02:03:18PM -0500, Josh Boyer wrote: >> Hi All, >> >> I'm seeing the oops below on my MacBookPro 10,2 machine using i915 >> graphics. It's after the DRM merge for 3.14 ( v3.13-10094-g9b0cd30) , >> but we seem to have one report[1] of this happening well before that, >> in v3.13-3260-g03d11a0 as well. Does anyone have a clue what is going >> on here? >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1055105 > > Should be fixed by the following patch which is already queued. > > http://lkml.kernel.org/g/20140129170403.gj30...@htj.dyndns.org Oh, excellent! I'll throw that into a build and test it here. Thanks for the quick reply, Tejun. josh ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFT] drm/i915: Ensure a context is loaded before RC6
On Thu, Jan 30, 2014 at 11:00:35AM -0800, Ben Widawsky wrote:
> RC6 works a lot like HW contexts in that when the GPU enters RC6 it
> saves away the state to a context, and loads it upon wake.
>
> It's to be somewhat expected that BIOS will not set up valid GPU state.
> As a result, if loading bad state can cause the GPU to get angry, it
> would make sense then that we need to load state first. There are two
> ways in which we can do this:
>
> 1. Create 3d state in the driver, load it up, then enable RC6.
> 1b. Reuse a known good state, and just bind objects where needed. Then
> enable RC6
> 2. Hold off enabling RC6 until userspace has had a chance to complete
> batches.
>
> This patch is a bad hack. It suffers two flaws. The first is, if the
> driver is loaded, but a batch is not submitted/completed, we'll never
> enter rc6. The other is, it expects userspace to submit a batch with 3d
> state first. Both of these things are not actual flaws for most users.
There is another flaw actually. If the objects for the indirect state
gets unbound before we enable RC6, or before we return from RC6.
I think for the case I want to test however, both of these are unlikely
- but it's another reason to not use a patch like this upstream.
>
> Technically, this tactic is required for all platforms, though I am not
> certain we've seen real failures.
>
> Cc: David E. Box
> Cc: Kristen Carlson Accardi
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_gem.c | 6 ++
> drivers/gpu/drm/i915/intel_display.c | 4
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 08331e1..83847fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2482,6 +2482,7 @@ void i915_gem_reset(struct drm_device *dev)
> void
> i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
> {
> + static bool rc6_enabled = false;
> uint32_t seqno;
>
> if (list_empty(&ring->request_list))
> @@ -2505,6 +2506,11 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer
> *ring)
> if (!i915_seqno_passed(seqno, obj->last_read_seqno))
> break;
>
> + if (unlikely(!rc6_enabled) && ring->id == RCS) {
> + intel_enable_gt_powersave(ring->dev);
> + rc6_enabled = true;
> + }
> +
> i915_gem_object_move_to_inactive(obj);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4d4a0d9..990819a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10982,10 +10982,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
> intel_init_clock_gating(dev);
>
> intel_reset_dpio(dev);
> -
> - mutex_lock(&dev->struct_mutex);
> - intel_enable_gt_powersave(dev);
> - mutex_unlock(&dev->struct_mutex);
> }
>
> void intel_modeset_suspend_hw(struct drm_device *dev)
> --
> 1.8.5.3
>
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] kernfs oops with i915+i2c_core in 3.14 merge window
Hi All, I'm seeing the oops below on my MacBookPro 10,2 machine using i915 graphics. It's after the DRM merge for 3.14 ( v3.13-10094-g9b0cd30) , but we seem to have one report[1] of this happening well before that, in v3.13-3260-g03d11a0 as well. Does anyone have a clue what is going on here? https://bugzilla.redhat.com/show_bug.cgi?id=1055105 josh [6.058198] INFO: trying to register non-static key. [6.058203] the code is fine but needs lockdep annotation. [6.058206] turning off the locking correctness validator. [6.058210] CPU: 2 PID: 225 Comm: systemd-udevd Not tainted 3.14.0-0.rc0.git17.1.fc21.x86_64 #1 [6.058214] Hardware name: Apple Inc. MacBookPro10,2/Mac-AFD8A9D944EA4843, BIOS MBP102.88Z.0106.B03.1211161133 11/16/2012 [6.058219] 8b5190d0 88025cc67460 817cdb1f [6.058225] 0002 88025cc67470 817c8aa9 88025cc67550 [6.058230] 810fa886 0002 88025cc66000 88025cc67500 [6.058236] Call Trace: [6.058242] [] dump_stack+0x4d/0x66 [6.058247] [] register_lock_class.part.26+0x38/0x3c [6.058253] [] __lock_acquire+0x1776/0x1c40 [6.058258] [] ? mark_held_locks+0xb9/0x140 [6.058262] [] ? __raw_spin_lock_init+0x21/0x60 [6.058267] [] ? lockdep_init_map+0xac/0x4a0 [6.058271] [] lock_acquire+0xa2/0x1d0 [6.058275] [] ? kernfs_addrm_finish+0x38/0x60 [6.058279] [] kernfs_deactivate+0x13e/0x1a0 [6.058283] [] ? kernfs_addrm_finish+0x38/0x60 [6.058287] [] ? mark_held_locks+0xb9/0x140 [6.058291] [] ? mark_held_locks+0xb9/0x140 [6.058295] [] kernfs_addrm_finish+0x38/0x60 [6.058299] [] kernfs_remove_by_name_ns+0x60/0xc0 [6.058304] [] remove_files.isra.1+0x41/0x80 [6.058308] [] sysfs_remove_group+0x47/0xa0 [6.058312] [] sysfs_remove_groups+0x33/0x50 [6.058318] [] device_remove_attrs+0x5e/0x80 [6.058322] [] device_del+0x12e/0x1d0 [6.058325] [] device_unregister+0x1e/0x60 [6.058331] [] i2c_del_adapter+0x267/0x3b0 [i2c_core] [6.058354] [] intel_sdvo_init+0x20e/0x8c0 [i915] [6.058359] [] ? trace_hardirqs_on_caller+0x105/0x1d0 [6.058363] [] ? trace_hardirqs_on+0xd/0x10 [6.058381] [] ? gen6_read32+0x52/0x1c0 [i915] [6.058398] [] intel_modeset_init+0xb62/0xff0 [i915] [6.058414] [] ? intel_power_domains_init_hw+0xa8/0x110 [i915] [6.058429] [] i915_driver_load+0xccc/0xec0 [i915] [6.058440] [] ? drm_get_minor+0x1ad/0x200 [drm] [6.058447] [] drm_dev_register+0x7d/0x180 [drm] [6.058455] [] drm_get_pci_dev+0xa0/0x220 [drm] [6.058468] [] i915_pci_probe+0x3b/0x60 [i915] [6.058473] [] local_pci_probe+0x45/0xa0 [6.058477] [] ? pci_match_device+0xc5/0xd0 [6.058481] [] pci_device_probe+0xf9/0x150 [6.058486] [] driver_probe_device+0x125/0x3a0 [6.058490] [] __driver_attach+0x93/0xa0 [6.058494] [] ? __device_attach+0x40/0x40 [6.058498] [] bus_for_each_dev+0x73/0xc0 [6.058502] [] driver_attach+0x1e/0x20 [6.058505] [] bus_add_driver+0x188/0x260 [6.058509] [] ? 0xa0153fff [6.058513] [] driver_register+0x64/0xf0 [6.058516] [] ? 0xa0153fff [6.058520] [] __pci_register_driver+0x60/0x70 [6.058527] [] drm_pci_init+0x11a/0x130 [drm] [6.058531] [] ? 0xa0153fff [6.058543] [] i915_init+0x6a/0x6c [i915] [6.058548] [] do_one_initcall+0xfa/0x1b0 [6.058552] [] ? set_memory_nx+0x43/0x50 [6.058557] [] load_module+0x1eb3/0x26e0 [6.058560] [] ? store_uevent+0x70/0x70 [6.058565] [] ? kernel_read+0x50/0x80 [6.058569] [] SyS_finit_module+0xa6/0xd0 [6.058574] [] system_call_fastpath+0x16/0x1b ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] [RFT] drm/i915: Ensure a context is loaded before RC6
RC6 works a lot like HW contexts in that when the GPU enters RC6 it
saves away the state to a context, and loads it upon wake.
It's to be somewhat expected that BIOS will not set up valid GPU state.
As a result, if loading bad state can cause the GPU to get angry, it
would make sense then that we need to load state first. There are two
ways in which we can do this:
1. Create 3d state in the driver, load it up, then enable RC6.
1b. Reuse a known good state, and just bind objects where needed. Then
enable RC6
2. Hold off enabling RC6 until userspace has had a chance to complete
batches.
This patch is a bad hack. It suffers two flaws. The first is, if the
driver is loaded, but a batch is not submitted/completed, we'll never
enter rc6. The other is, it expects userspace to submit a batch with 3d
state first. Both of these things are not actual flaws for most users.
Technically, this tactic is required for all platforms, though I am not
certain we've seen real failures.
Cc: David E. Box
Cc: Kristen Carlson Accardi
Signed-off-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_gem.c | 6 ++
drivers/gpu/drm/i915/intel_display.c | 4
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 08331e1..83847fc 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2482,6 +2482,7 @@ void i915_gem_reset(struct drm_device *dev)
void
i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
{
+ static bool rc6_enabled = false;
uint32_t seqno;
if (list_empty(&ring->request_list))
@@ -2505,6 +2506,11 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer
*ring)
if (!i915_seqno_passed(seqno, obj->last_read_seqno))
break;
+ if (unlikely(!rc6_enabled) && ring->id == RCS) {
+ intel_enable_gt_powersave(ring->dev);
+ rc6_enabled = true;
+ }
+
i915_gem_object_move_to_inactive(obj);
}
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4d4a0d9..990819a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10982,10 +10982,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
intel_init_clock_gating(dev);
intel_reset_dpio(dev);
-
- mutex_lock(&dev->struct_mutex);
- intel_enable_gt_powersave(dev);
- mutex_unlock(&dev->struct_mutex);
}
void intel_modeset_suspend_hw(struct drm_device *dev)
--
1.8.5.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Thu, Jan 30, 2014 at 03:07:15AM -0800, Daniel Vetter wrote:
> On Thu, Jan 30, 2014 at 10:12:06AM +0100, Daniel Vetter wrote:
> > On Thu, Jan 30, 2014 at 10:05:28AM +0100, Daniel Vetter wrote:
> > > On Thu, Jan 30, 2014 at 09:53:28AM +0100, Daniel Vetter wrote:
> > > > On Wed, Jan 29, 2014 at 10:28:36PM +, Chris Wilson wrote:
> > > > > On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com
> > > > > wrote:
> > > > > > +/*
> > > > > > + * Returns a pointer to a descriptor for the command specified by
> > > > > > cmd_header.
> > > > > > + *
> > > > > > + * The caller must supply space for a default descriptor via the
> > > > > > default_desc
> > > > > > + * parameter. If no descriptor for the specified command exists in
> > > > > > the ring's
> > > > > > + * command parser tables, this function fills in default_desc
> > > > > > based on the
> > > > > > + * ring's default length encoding and returns default_desc.
> > > > > > + */
> > > > > > +static const struct drm_i915_cmd_descriptor*
> > > > > > +find_cmd(struct intel_ring_buffer *ring,
> > > > > > +u32 cmd_header,
> > > > > > +struct drm_i915_cmd_descriptor *default_desc)
> > > > > > +{
> > > > > > + u32 mask;
> > > > > > + int i;
> > > > > > +
> > > > > > + for (i = 0; i < ring->cmd_table_count; i++) {
> > > > > > + const struct drm_i915_cmd_descriptor *desc;
> > > > > > +
> > > > > > + desc = find_cmd_in_table(&ring->cmd_tables[i],
> > > > > > cmd_header);
> > > > > > + if (desc)
> > > > > > + return desc;
> > > > > > + }
> > > > > > +
> > > > > > + mask = ring->get_cmd_length_mask(cmd_header);
> > > > > > + if (!mask)
> > > > > > + return NULL;
> > > > > > +
> > > > > > + BUG_ON(!default_desc);
> > > > > > + default_desc->flags = CMD_DESC_SKIP;
> > > > > > + default_desc->length.mask = mask;
> > > > >
> > > > > If we turn off all hw validation (through use of the secure bit)
> > > > > should
> > > > > we not default to a whitelist of commands? Otherwise it just seems to
> > > > > be
> > > > > a case of running a fuzzer until we kill the machine.
> > > >
> > > > Preventing hangs and dos is imo not the attack model, gpus are too
> > > > fickle
> > > > for that. The attach model here is to prevent priveledge escalation and
> > > > information leaks. I.e. we want just containement of all read/write
> > > > access
> > > > to the gtt space.
> > > >
> > > > I think for that purpose an explicit whitelist of commands which target
> > > > things outside of the (pp)gtt is sufficient. radeon's checker design is
> > > > completely different, but pretty much the only command they have is
> > > > to load register values. Intel gpus otoh have a big set of
> > > > special-purpose
> > > > commands to load (most) of the rendering pipeline state. So we have
> > > > hw built-in register whitelists for all that stuff since you just can't
> > > > load arbitrary registers and state with those commands.
> > > >
> > > > Also note that for raw register access Bradley's scanner _is_ whitelist
> > > > based. And for general reads/writes gpu designers confirmed that those
> > > > are
> > > > all MI_ commands (with very few specific exceptions like PIPE_CONTROL),
> > > > so
> > > > as long as we check for the exceptions and otherwise only whitelist MI_
> > > > commands we know about we should be covered.
> > > >
> > > > So I think this is sound.
> > >
> > > Hm, but while scrolling through the checker I haven't spotted a "reject
> > > everything unknown" for MI_CLIENT commands. Bradley, have I missed that?
> > >
> > > I think submitting an invented MI_CLIENT command would also be a good
> > > testcase.
> >
> > One more: I think it would be good to have an overview comment at the top
> > of i915_cmd_parser.c which details the security attack model and the
> > overall blacklist/whitelist design of the checker. We don't (yet) have
> > autogenerated documentation for i915, but that's something I'm working on.
> > And the kerneldoc system can also pull in multi-paragraph overview
> > comments besides the usual api documentation, so it's good to have things
> > ready.
>
> Chatted with Chris a bit more on irc about this, and for more paranoia I
> guess we should also reject any unknown client and media subclient
> commands.
Hmm, not sure I follow. Can you elaborate?
Are you suggesting we add all the MI and Media commands to the tables and reject
any command from those client/subclients that is not found in the table? Or that
we look at the client and subclient fields of the command and reject if they are
not from a set of expected values? Or other?
- Brad
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Thu, Jan 30, 2014 at 01:12:06AM -0800, Daniel Vetter wrote:
> On Thu, Jan 30, 2014 at 10:05:28AM +0100, Daniel Vetter wrote:
> > On Thu, Jan 30, 2014 at 09:53:28AM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 29, 2014 at 10:28:36PM +, Chris Wilson wrote:
> > > > On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com
> > > > wrote:
> > > > > +/*
> > > > > + * Returns a pointer to a descriptor for the command specified by
> > > > > cmd_header.
> > > > > + *
> > > > > + * The caller must supply space for a default descriptor via the
> > > > > default_desc
> > > > > + * parameter. If no descriptor for the specified command exists in
> > > > > the ring's
> > > > > + * command parser tables, this function fills in default_desc based
> > > > > on the
> > > > > + * ring's default length encoding and returns default_desc.
> > > > > + */
> > > > > +static const struct drm_i915_cmd_descriptor*
> > > > > +find_cmd(struct intel_ring_buffer *ring,
> > > > > + u32 cmd_header,
> > > > > + struct drm_i915_cmd_descriptor *default_desc)
> > > > > +{
> > > > > + u32 mask;
> > > > > + int i;
> > > > > +
> > > > > + for (i = 0; i < ring->cmd_table_count; i++) {
> > > > > + const struct drm_i915_cmd_descriptor *desc;
> > > > > +
> > > > > + desc = find_cmd_in_table(&ring->cmd_tables[i],
> > > > > cmd_header);
> > > > > + if (desc)
> > > > > + return desc;
> > > > > + }
> > > > > +
> > > > > + mask = ring->get_cmd_length_mask(cmd_header);
> > > > > + if (!mask)
> > > > > + return NULL;
> > > > > +
> > > > > + BUG_ON(!default_desc);
> > > > > + default_desc->flags = CMD_DESC_SKIP;
> > > > > + default_desc->length.mask = mask;
> > > >
> > > > If we turn off all hw validation (through use of the secure bit) should
> > > > we not default to a whitelist of commands? Otherwise it just seems to be
> > > > a case of running a fuzzer until we kill the machine.
> > >
> > > Preventing hangs and dos is imo not the attack model, gpus are too fickle
> > > for that. The attach model here is to prevent priveledge escalation and
> > > information leaks. I.e. we want just containement of all read/write access
> > > to the gtt space.
> > >
> > > I think for that purpose an explicit whitelist of commands which target
> > > things outside of the (pp)gtt is sufficient. radeon's checker design is
> > > completely different, but pretty much the only command they have is
> > > to load register values. Intel gpus otoh have a big set of special-purpose
> > > commands to load (most) of the rendering pipeline state. So we have
> > > hw built-in register whitelists for all that stuff since you just can't
> > > load arbitrary registers and state with those commands.
> > >
> > > Also note that for raw register access Bradley's scanner _is_ whitelist
> > > based. And for general reads/writes gpu designers confirmed that those are
> > > all MI_ commands (with very few specific exceptions like PIPE_CONTROL), so
> > > as long as we check for the exceptions and otherwise only whitelist MI_
> > > commands we know about we should be covered.
> > >
> > > So I think this is sound.
> >
> > Hm, but while scrolling through the checker I haven't spotted a "reject
> > everything unknown" for MI_CLIENT commands. Bradley, have I missed that?
> >
> > I think submitting an invented MI_CLIENT command would also be a good
> > testcase.
>
> One more: I think it would be good to have an overview comment at the top
> of i915_cmd_parser.c which details the security attack model and the
> overall blacklist/whitelist design of the checker. We don't (yet) have
> autogenerated documentation for i915, but that's something I'm working on.
> And the kerneldoc system can also pull in multi-paragraph overview
> comments besides the usual api documentation, so it's good to have things
> ready.
Ok, I'll add that and maybe kerneldoc for the non-static functions to the
next revision.
- Brad
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v6] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.
From: Deepak S
When we enter RC6 and GFX Clocks are off, the voltage remains higher
than Vmin. When we try to set the freq to RPn, it might fail since the
Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
and set the freq to RPn then move GFx down.
v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
v3: Fix the timeout during wait for gfx clock (Jesse)
v4: addressed comments on set freq and punit wait (Ville)
v5: use wait_for while waiting for GFX clk to be up. (Daniel)
update cur_delay before requesting min_delay. (Ville)
v6: use wait_for while waiting for punit. (Ville)
Signed-off-by: Deepak S
Reviewed-by: Ville Syrjä
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_irq.c | 2 +-
drivers/gpu/drm/i915/i915_reg.h | 4 +++
drivers/gpu/drm/i915/intel_pm.c | 55 -
4 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ac5cd7e..6f9108d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1961,6 +1961,8 @@ extern void intel_console_resume(struct work_struct
*work);
void i915_queue_hangcheck(struct drm_device *dev);
void i915_handle_error(struct drm_device *dev, bool wedged);
+void gen6_set_pm_mask(struct drm_i915_private *dev_priv, u32 pm_iir,
+ int new_delay);
extern void intel_irq_init(struct drm_device *dev);
extern void intel_hpd_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b226ae6..acbee73 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -986,7 +986,7 @@ static void notify_ring(struct drm_device *dev,
i915_queue_hangcheck(dev);
}
-static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
+void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
u32 pm_iir, int new_delay)
{
if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cbbaf26..00d2f2d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4949,6 +4949,10 @@
GEN6_PM_RP_DOWN_THRESHOLD | \
GEN6_PM_RP_DOWN_TIMEOUT)
+#define VLV_GTLC_SURVIVABILITY_REG 0x130098
+#define VLV_GFX_CLK_STATUS_BIT (1<<3)
+#define VLV_GFX_CLK_FORCE_ON_BIT (1<<2)
+
#define GEN6_GT_GFX_RC6_LOCKED 0x138104
#define VLV_COUNTER_CONTROL0x138104
#define VLV_COUNT_RANGE_HIGH (1<<15)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4876ba5..3b5a1b9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3038,6 +3038,58 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
trace_intel_gpu_freq_change(val * 50);
}
+/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
+ *
+ * * If Gfx is Idle, then
+ * 1. Mask Turbo interrupts
+ * 2. Bring up Gfx clock
+ * 3. Change the freq to Rpn and wait till P-Unit updates freq
+ * 4. Clear the Force GFX CLK ON bit so that Gfx can down
+ * 5. Unmask Turbo interrupts
+*/
+static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
+{
+ /*
+* When we are idle. Drop to min voltage state.
+*/
+
+ if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
+ return;
+
+ /* Mask turbo interrupt so that they will not come in between */
+ I915_WRITE(GEN6_PMINTRMSK, 0x);
+
+ /* Bring up the Gfx clock */
+ I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
+ I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
+ VLV_GFX_CLK_FORCE_ON_BIT);
+
+ if (wait_for(((VLV_GFX_CLK_STATUS_BIT &
+ I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
+ DRM_ERROR("GFX_CLK_ON request timed out\n");
+ return;
+ }
+
+ dev_priv->rps.cur_delay = dev_priv->rps.min_delay;
+
+ vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
+ dev_priv->rps.min_delay);
+
+ if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
+ & GENFREQSTATUS) == 0, 5))
+ DRM_ERROR("timed out waiting for Punit\n");
+
+ /* Release the Gfx clock */
+ I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
+ I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
+ ~VLV_GFX_CLK_FORCE_ON_BIT);
+
+ /* Unmask Up interrupts */
+ dev_priv->rps.rp_up_masked = true;
+ gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD,
+ dev_priv->r
Re: [Intel-gfx] [PATCH] intel: Merge i915_drm.h with cmd parser define
On Thu, Jan 30, 2014 at 01:20:57AM -0800, Daniel Vetter wrote: > On Wed, Jan 29, 2014 at 02:26:12PM -0800, Volkin, Bradley D wrote: > > On Wed, Jan 29, 2014 at 02:13:21PM -0800, Chris Wilson wrote: > > > On Wed, Jan 29, 2014 at 01:57:28PM -0800, bradley.d.vol...@intel.com > > > wrote: > > > > From: Brad Volkin > > > > > > > > Signed-off-by: Brad Volkin > > > > --- > > > > include/drm/i915_drm.h | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h > > > > index 2f4eb8c..ba863c4 100644 > > > > --- a/include/drm/i915_drm.h > > > > +++ b/include/drm/i915_drm.h > > > > @@ -27,7 +27,7 @@ > > > > #ifndef _I915_DRM_H_ > > > > #define _I915_DRM_H_ > > > > > > > > -#include > > > > +#include > > > > > > Something about this patch smells very fishy > > > > Yeah, I wasn't completely sure about this one. I followed what I thought was > > the procedure for updating the header (i.e. make headers_install in kernel, > > copy to libdrm) and this is what I got. > > I guess either works, so maybe just add a note to the commit message about > the little change. Imo it's better to have a 1:1 copy of the header > generated by the kernel. Sorry, I'm a bit confused. Did I follow the right procedure for updating the header? > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/13] drm/i915: Add a CMD_PARSER_VERSION getparam
On Thu, Jan 30, 2014 at 01:19:15AM -0800, Daniel Vetter wrote:
> On Wed, Jan 29, 2014 at 01:55:13PM -0800, bradley.d.vol...@intel.com wrote:
> > From: Brad Volkin
> >
> > So userspace can query the kernel for command parser support.
> >
> > OTC-Tracker: AXIA-4631
> > Change-Id: I58af650db9f6753c2dcac9c54ab432fd31db302f
> > Signed-off-by: Brad Volkin
> > ---
> > drivers/gpu/drm/i915/i915_dma.c | 4
> > include/uapi/drm/i915_drm.h | 1 +
> > 2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 258b1be..34ba199 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1013,6 +1013,10 @@ static int i915_getparam(struct drm_device *dev,
> > void *data,
> > case I915_PARAM_HAS_EXEC_HANDLE_LUT:
> > value = 1;
> > break;
> > + case I915_PARAM_CMD_PARSER_VERSION:
> > + /* TODO: version info (e.g. what is allowed?) */
> > + value = 1;
>
> I think an increasing integer without any mean special grouping (like
> major/minor) is good enough. What we need though is a small revision log
> with one-line blurbs that explain what has been added, e.g.
>
> 1: Initial version.
> 2: Allow streamout related registers
> 3: Add gen8 support
>
> ...
>
> I think it would be good to have this as a comment right next to the
> parser code itself, so what about adding a i915_cmd_parser_get_version
> function to i915_cmd_parser.c who's only job is to return and integer and
> contain this comment?
> -Daniel
Agree on the revision log. I forgot that I left writing it as a todo.
I'll add i915_cmd_parser_get_version and fill that in.
- Brad
>
> > + break;
> > default:
> > DRM_DEBUG("Unknown parameter %d\n", param->param);
> > return -EINVAL;
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 126bfaa..8a3e4ef00 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -337,6 +337,7 @@ typedef struct drm_i915_irq_wait {
> > #define I915_PARAM_HAS_EXEC_NO_RELOC25
> > #define I915_PARAM_HAS_EXEC_HANDLE_LUT 26
> > #define I915_PARAM_HAS_WT 27
> > +#define I915_PARAM_CMD_PARSER_VERSION 28
> >
> > typedef struct drm_i915_getparam {
> > int param;
> > --
> > 1.8.5.2
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix use-after-free in the shadow-attache exit code
On Thu, Jan 30, 2014 at 05:58:38PM +0100, Daniel Vetter wrote: > This regression has been introduced in > > commit b3f2333de8e81b089262b26d52272911523e605f > Author: Daniel Vetter > Date: Wed Dec 11 11:34:31 2013 +0100 > > drm: restrict the device list for shadow attached drivers > btw, I noticed this because it got flagged in the nightly coverity runs. Of the 18 new issues added yesterday 14 were from drivers/gpu/ If drm developers want to sign up at http://scan.coverity.com to help out looking over those (and the backlog: stats below) I can get those accounts approved quickly. I've been going through trying to clear out as much of the 'noise' as possible, but it's a huge job. There's a bunch of cases where the checker can't figure out if it's a real bug or not because it doesn't know things like "the hardware will only ever return these values", but the majority look like actual coding flaws. Dave Currently outstanding issues: Radeon: 64 Nouveau: 36 i915: 32 misc drm: 24 gma500: 11 qxl: 7 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Fix use-after-free in the shadow-attache exit code
Hi
On Thu, Jan 30, 2014 at 5:58 PM, Daniel Vetter wrote:
> This regression has been introduced in
>
> commit b3f2333de8e81b089262b26d52272911523e605f
> Author: Daniel Vetter
> Date: Wed Dec 11 11:34:31 2013 +0100
>
> drm: restrict the device list for shadow attached drivers
>
> Reported-by: Dave Jones
> Cc: Dave Jones
> Cc: Dave Airlie
> Cc: David Herrmann
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 5736aaa7e86c..f7af69bcf3f4 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -468,8 +468,8 @@ void drm_pci_exit(struct drm_driver *driver, struct
> pci_driver *pdriver)
> } else {
> list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list,
> legacy_dev_list) {
> - drm_put_dev(dev);
> list_del(&dev->legacy_dev_list);
> + drm_put_dev(dev);
This code-path is the only user of legacy_dev_list (besides ->probe)
and both are locked against each other. So removing the device before
destroying it is fine. So no objections from me:
Reviewed-by: David Herrmann
Thanks
David
> }
> }
> DRM_INFO("Module unloaded\n");
> --
> 1.8.5.2
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Get rid of acthd based guilty batch search
As we seek the guilty batch using request and hangcheck
score, this code is not needed anymore.
v2: Rebase. Passing dev_priv instead of getting it from last_ring
Signed-off-by: Mika Kuoppala
Reviewed-by: Ben Widawsky (v1)
---
drivers/gpu/drm/i915/i915_gem.c | 97 +++
1 file changed, 6 insertions(+), 91 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 37c2ea4..d230c3b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2241,74 +2241,9 @@ i915_gem_request_remove_from_client(struct
drm_i915_gem_request *request)
spin_unlock(&file_priv->mm.lock);
}
-static bool i915_head_inside_object(u32 acthd, struct drm_i915_gem_object *obj,
- struct i915_address_space *vm)
-{
- if (acthd >= i915_gem_obj_offset(obj, vm) &&
- acthd < i915_gem_obj_offset(obj, vm) + obj->base.size)
- return true;
-
- return false;
-}
-
-static bool i915_head_inside_request(const u32 acthd_unmasked,
-const u32 request_start,
-const u32 request_end)
-{
- const u32 acthd = acthd_unmasked & HEAD_ADDR;
-
- if (request_start < request_end) {
- if (acthd >= request_start && acthd < request_end)
- return true;
- } else if (request_start > request_end) {
- if (acthd >= request_start || acthd < request_end)
- return true;
- }
-
- return false;
-}
-
-static struct i915_address_space *
-request_to_vm(struct drm_i915_gem_request *request)
-{
- struct drm_i915_private *dev_priv = request->ring->dev->dev_private;
- struct i915_address_space *vm;
-
- if (request->ctx)
- vm = request->ctx->vm;
- else
- vm = &dev_priv->gtt.base;
-
- return vm;
-}
-
-static bool i915_request_guilty(struct drm_i915_gem_request *request,
- const u32 acthd, bool *inside)
-{
- /* There is a possibility that unmasked head address
-* pointing inside the ring, matches the batch_obj address range.
-* However this is extremely unlikely.
-*/
- if (request->batch_obj) {
- if (i915_head_inside_object(acthd, request->batch_obj,
- request_to_vm(request))) {
- *inside = true;
- return true;
- }
- }
-
- if (i915_head_inside_request(acthd, request->head, request->tail)) {
- *inside = false;
- return true;
- }
-
- return false;
-}
-
-static bool i915_context_is_banned(struct drm_device *dev,
+static bool i915_context_is_banned(struct drm_i915_private *dev_priv,
const struct i915_hw_context *ctx)
{
- struct drm_i915_private *dev_priv = to_i915(dev);
unsigned long elapsed;
elapsed = get_seconds() - ctx->hang_stats.guilty_ts;
@@ -2330,39 +2265,19 @@ static bool i915_context_is_banned(struct drm_device
*dev,
return false;
}
-static void i915_set_reset_status(struct intel_ring_buffer *ring,
- struct drm_i915_gem_request *request,
+static void i915_set_reset_status(struct drm_i915_private *dev_priv,
+ struct i915_hw_context *ctx,
const bool guilty)
{
- const u32 acthd = intel_ring_get_active_head(ring);
- bool inside;
- unsigned long offset = 0;
- struct i915_hw_context *ctx = request->ctx;
struct i915_ctx_hang_stats *hs;
if (WARN_ON(!ctx))
return;
- if (request->batch_obj)
- offset = i915_gem_obj_offset(request->batch_obj,
-request_to_vm(request));
-
- if (guilty &&
- i915_request_guilty(request, acthd, &inside)) {
- DRM_DEBUG("%s hung %s bo (0x%lx ctx %d) at 0x%x\n",
- ring->name,
- inside ? "inside" : "flushing",
- offset,
- ctx->id,
- acthd);
- }
-
- WARN_ON(!ctx->last_ring);
-
hs = &ctx->hang_stats;
if (guilty) {
- hs->banned = i915_context_is_banned(ring->dev, ctx);
+ hs->banned = i915_context_is_banned(dev_priv, ctx);
hs->batch_active++;
hs->guilty_ts = get_seconds();
} else {
@@ -2410,10 +2325,10 @@ static void i915_gem_reset_ring_status(struct
drm_i915_private *dev_priv,
ring_hung = ring->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG;
- i915_set_reset_status(ring, request, ring_hung);
+ i915_set_reset_status(dev_priv, request->ctx, ring_hung);
list_for_each_entry_con
[Intel-gfx] [PATCH 1/2] drm/i915: Use hangcheck score to find guilty context
With full ppgtt using acthd is not enough to find guilty
batch buffer. We get multiple false positives as acthd is
per vm.
Instead of scanning which vm was running on a ring,
to find corressponding context, use a different, simpler,
strategy of finding batches that caused gpu hang:
If hangcheck has declared ring to be hung, find first non complete
request on that ring and claim it was guilty.
v2: Rebase
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73652
Suggested-by: Chris Wilson
Signed-off-by: Mika Kuoppala
Reviewed-by: Ben Widawsky (v1)
---
drivers/gpu/drm/i915/i915_gem.c | 42 +--
drivers/gpu/drm/i915/i915_irq.c |3 +--
drivers/gpu/drm/i915/intel_ringbuffer.h |2 ++
3 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 08331e1..37c2ea4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2332,9 +2332,10 @@ static bool i915_context_is_banned(struct drm_device
*dev,
static void i915_set_reset_status(struct intel_ring_buffer *ring,
struct drm_i915_gem_request *request,
- u32 acthd)
+ const bool guilty)
{
- bool inside, guilty;
+ const u32 acthd = intel_ring_get_active_head(ring);
+ bool inside;
unsigned long offset = 0;
struct i915_hw_context *ctx = request->ctx;
struct i915_ctx_hang_stats *hs;
@@ -2342,14 +2343,11 @@ static void i915_set_reset_status(struct
intel_ring_buffer *ring,
if (WARN_ON(!ctx))
return;
- /* Innocent until proven guilty */
- guilty = false;
-
if (request->batch_obj)
offset = i915_gem_obj_offset(request->batch_obj,
request_to_vm(request));
- if (ring->hangcheck.action != HANGCHECK_WAIT &&
+ if (guilty &&
i915_request_guilty(request, acthd, &inside)) {
DRM_DEBUG("%s hung %s bo (0x%lx ctx %d) at 0x%x\n",
ring->name,
@@ -2357,8 +2355,6 @@ static void i915_set_reset_status(struct
intel_ring_buffer *ring,
offset,
ctx->id,
acthd);
-
- guilty = true;
}
WARN_ON(!ctx->last_ring);
@@ -2385,19 +2381,39 @@ static void i915_gem_free_request(struct
drm_i915_gem_request *request)
kfree(request);
}
-static void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv,
- struct intel_ring_buffer *ring)
+static struct drm_i915_gem_request *
+i915_gem_find_first_non_complete(struct intel_ring_buffer *ring)
{
- u32 completed_seqno = ring->get_seqno(ring, false);
- u32 acthd = intel_ring_get_active_head(ring);
struct drm_i915_gem_request *request;
+ const u32 completed_seqno = ring->get_seqno(ring, false);
list_for_each_entry(request, &ring->request_list, list) {
if (i915_seqno_passed(completed_seqno, request->seqno))
continue;
- i915_set_reset_status(ring, request, acthd);
+ return request;
}
+
+ return NULL;
+}
+
+static void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv,
+ struct intel_ring_buffer *ring)
+{
+ struct drm_i915_gem_request *request;
+ bool ring_hung;
+
+ request = i915_gem_find_first_non_complete(ring);
+
+ if (request == NULL)
+ return;
+
+ ring_hung = ring->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG;
+
+ i915_set_reset_status(ring, request, ring_hung);
+
+ list_for_each_entry_continue(request, &ring->request_list, list)
+ i915_set_reset_status(ring, request, false);
}
static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b226ae6..ec9eec4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2532,7 +2532,6 @@ static void i915_hangcheck_elapsed(unsigned long data)
#define BUSY 1
#define KICK 5
#define HUNG 20
-#define FIRE 30
if (!i915.enable_hangcheck)
return;
@@ -2616,7 +2615,7 @@ static void i915_hangcheck_elapsed(unsigned long data)
}
for_each_ring(ring, dev_priv, i) {
- if (ring->hangcheck.score > FIRE) {
+ if (ring->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG) {
DRM_INFO("%s on %s\n",
stuck[i] ? "stuck" : "no progress",
ring->name);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 71a73f4..38c757e 100644
--- a/drivers/gpu/drm/
Re: [Intel-gfx] [PATCH v5] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.
On Thu, Jan 30, 2014 at 09:54:41PM +0530, deepa...@intel.com wrote:
> From: Deepak S
>
> When we enter RC6 and GFX Clocks are off, the voltage remains higher
> than Vmin. When we try to set the freq to RPn, it might fail since the
> Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
> and set the freq to RPn then move GFx down.
>
> v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
>
> v3: Fix the timeout during wait for gfx clock (Jesse)
>
> v4: addressed comments on set freq and punit wait (Ville)
>
> v5: use wait_for while waiting for GFX clk to be up. (Daniel)
> update cur_delay before requesting min_delay. (Ville)
>
> Signed-off-by: Deepak S
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/i915_reg.h | 4 +++
> drivers/gpu/drm/i915/intel_pm.c | 57
> -
> 4 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ac5cd7e..6f9108d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1961,6 +1961,8 @@ extern void intel_console_resume(struct work_struct
> *work);
> void i915_queue_hangcheck(struct drm_device *dev);
> void i915_handle_error(struct drm_device *dev, bool wedged);
>
> +void gen6_set_pm_mask(struct drm_i915_private *dev_priv, u32 pm_iir,
> + int new_delay);
> extern void intel_irq_init(struct drm_device *dev);
> extern void intel_hpd_init(struct drm_device *dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b226ae6..acbee73 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -986,7 +986,7 @@ static void notify_ring(struct drm_device *dev,
> i915_queue_hangcheck(dev);
> }
>
> -static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
> +void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
>u32 pm_iir, int new_delay)
> {
> if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cbbaf26..00d2f2d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4949,6 +4949,10 @@
>GEN6_PM_RP_DOWN_THRESHOLD | \
>GEN6_PM_RP_DOWN_TIMEOUT)
>
> +#define VLV_GTLC_SURVIVABILITY_REG 0x130098
> +#define VLV_GFX_CLK_STATUS_BIT (1<<3)
> +#define VLV_GFX_CLK_FORCE_ON_BIT (1<<2)
> +
> #define GEN6_GT_GFX_RC6_LOCKED 0x138104
> #define VLV_COUNTER_CONTROL 0x138104
> #define VLV_COUNT_RANGE_HIGH (1<<15)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4876ba5..36795ee 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3038,6 +3038,60 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> trace_intel_gpu_freq_change(val * 50);
> }
>
> +/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
> + *
> + * * If Gfx is Idle, then
> + * 1. Mask Turbo interrupts
> + * 2. Bring up Gfx clock
> + * 3. Change the freq to Rpn and wait till P-Unit updates freq
> + * 4. Clear the Force GFX CLK ON bit so that Gfx can down
> + * 5. Unmask Turbo interrupts
> +*/
> +static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> +{
> + /*
> + * When we are idle. Drop to min voltage state.
> + */
> +
> + if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
> + return;
> +
> + /* Mask turbo interrupt so that they will not come in between */
> + I915_WRITE(GEN6_PMINTRMSK, 0x);
> +
> + /* Bring up the Gfx clock */
> + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> + I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
> + VLV_GFX_CLK_FORCE_ON_BIT);
> +
> + if (wait_for(((VLV_GFX_CLK_STATUS_BIT &
> + I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
> + DRM_ERROR("GFX_CLK_ON request timed out\n");
> + return;
> + }
> +
> + dev_priv->rps.cur_delay = dev_priv->rps.min_delay;
> +
> + vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> + dev_priv->rps.min_delay);
> +
> + if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
^^
Still atomic here.
> + & GENFREQSTATUS) == 0, 5))
> + DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
Maybe this should now be an error as well?
> +
> + /* Release the Gfx clock */
> + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
> + I915_READ(VL
Re: [Intel-gfx] [PATCH] drm/i915: Add Baytrail PSR Support.
On Thu, Jan 30, 2014 at 11:02 AM, Chris Wilson wrote: > On Wed, Jan 29, 2014 at 01:50:06PM -0200, Rodrigo Vivi wrote: >> @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc >> *crtc, >> u32 base = 0, pos = 0; >> bool visible; >> >> + if (IS_VALLEYVIEW(dev)) >> + intel_edp_psr_inactivate(dev); >> + > > Inactivate means that we turn off PSR for some period of time until idle > again, right? Yes, right. > In the case of a moving cursor that means indefinitely. That's true... So I think we really need a work queue delaying the enable. Or do you have any better idea? > > Also it sounds overkill to have to disable PSR just for the cursor > sprite. Is there not some preferrable alternatives or hw that dtrt? Yeap, I totally agree. The other option implemented by other drivers is to use PSR SW control mode where driver controls all entry and exit flow, since Baytrail cannot do it right on HW mode. But even the force exit on this case was using the PSR_RESET that I'm using on inactivate function. and they exported that over ioctl. So I preferred to let all this on kernel side using hardware as much as possible and workarounding the cases where hardware buggy on screen update detection, although it looks like an overkill for a single cursor update. :/ > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm: Fix use-after-free in the shadow-attache exit code
This regression has been introduced in
commit b3f2333de8e81b089262b26d52272911523e605f
Author: Daniel Vetter
Date: Wed Dec 11 11:34:31 2013 +0100
drm: restrict the device list for shadow attached drivers
Reported-by: Dave Jones
Cc: Dave Jones
Cc: Dave Airlie
Cc: David Herrmann
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 5736aaa7e86c..f7af69bcf3f4 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -468,8 +468,8 @@ void drm_pci_exit(struct drm_driver *driver, struct
pci_driver *pdriver)
} else {
list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list,
legacy_dev_list) {
- drm_put_dev(dev);
list_del(&dev->legacy_dev_list);
+ drm_put_dev(dev);
}
}
DRM_INFO("Module unloaded\n");
--
1.8.5.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: vlv: fix DP PHY lockup due to invalid PP sequencer setup
On Thu, 2014-01-30 at 18:22 +0200, Jani Nikula wrote: > On Thu, 30 Jan 2014, Daniel Vetter wrote: > > On Thu, Jan 30, 2014 at 04:50:42PM +0200, Imre Deak wrote: > >> Atm we setup the HW panel power sequencer logic both for eDP and DP > >> ports. On eDP we then go on and start the power on sequence and commence > >> with link training when it's ready. On DP we don't do the power on > >> sequencing but do the link training immediately. At this point the DP > >> PHY block gets stuck, since - supposedly - it is waiting for the power > >> on sequence to finish. The actual register write that seems to hold off > >> the PHY is PIPEX_PP_ON_DELAYS[Panel Control Port Select]. Writing here > >> a non-0 value eventually sets PIPEX_PP_STATUS[Require Asset Status] to > >> 1 and blocks the PHY until the panel power on is ready. > >> > >> Fix this by not doing any PP sequencing setup for DP ports. > >> > >> Thanks to Ville Syrjälä, Jesse Barnes and Todd Previte for the help in > >> tracking this down. > >> > >> Signed-off-by: Imre Deak > > > > Ah, the infamous ABCD hack we're using all over the place in intel_lvds.c. > > On edp we didn't have a need for it thus far since the "require asset > > status" checks have all been fused of, with the PP being on the PCH and > > the edp port on the north display block. If this is really all we need to > > appease the hardware then I'm heavily in favour of it as opposed to > > resurrect the ABCD hack for intel_dp.c. > > > > One thing though: Should we add a check for the "Required Asset Status" > > bit somewhere? I don't really have a good idea for a spot to put this > > into, hence the question. > > Don't know about the asset status stuff, but I know it was me who > screwed this up in > > commit bf13e81b904a37d94d83dd6c3b53a147719a3ead > Author: Jani Nikula > Date: Fri Sep 6 07:40:05 2013 +0300 > > drm/i915: add support for per-pipe power sequencing on vlv > > We need to make sure the PP registers are set up correctly on the pipe > being enabled, which might be different from the last time. But only for > eDP. > > I'm a bit surprised this hasn't been bisected to. One thing making that more difficult is buggy VBTs. If that reports the DP port being eDP, like in my case, you won't see the problem. --Imre signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: vlv: fix DP PHY lockup due to invalid PP sequencer setup
On Thu, 30 Jan 2014, Daniel Vetter wrote: > On Thu, Jan 30, 2014 at 04:50:42PM +0200, Imre Deak wrote: >> Atm we setup the HW panel power sequencer logic both for eDP and DP >> ports. On eDP we then go on and start the power on sequence and commence >> with link training when it's ready. On DP we don't do the power on >> sequencing but do the link training immediately. At this point the DP >> PHY block gets stuck, since - supposedly - it is waiting for the power >> on sequence to finish. The actual register write that seems to hold off >> the PHY is PIPEX_PP_ON_DELAYS[Panel Control Port Select]. Writing here >> a non-0 value eventually sets PIPEX_PP_STATUS[Require Asset Status] to >> 1 and blocks the PHY until the panel power on is ready. >> >> Fix this by not doing any PP sequencing setup for DP ports. >> >> Thanks to Ville Syrjälä, Jesse Barnes and Todd Previte for the help in >> tracking this down. >> >> Signed-off-by: Imre Deak > > Ah, the infamous ABCD hack we're using all over the place in intel_lvds.c. > On edp we didn't have a need for it thus far since the "require asset > status" checks have all been fused of, with the PP being on the PCH and > the edp port on the north display block. If this is really all we need to > appease the hardware then I'm heavily in favour of it as opposed to > resurrect the ABCD hack for intel_dp.c. > > One thing though: Should we add a check for the "Required Asset Status" > bit somewhere? I don't really have a good idea for a spot to put this > into, hence the question. Don't know about the asset status stuff, but I know it was me who screwed this up in commit bf13e81b904a37d94d83dd6c3b53a147719a3ead Author: Jani Nikula Date: Fri Sep 6 07:40:05 2013 +0300 drm/i915: add support for per-pipe power sequencing on vlv We need to make sure the PP registers are set up correctly on the pipe being enabled, which might be different from the last time. But only for eDP. I'm a bit surprised this hasn't been bisected to. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v5] drm/i915/vlv: WA to fix Voltage not getting dropped to Vmin when Gfx is power gated.
From: Deepak S
When we enter RC6 and GFX Clocks are off, the voltage remains higher
than Vmin. When we try to set the freq to RPn, it might fail since the
Gfx clocks are down. So to fix this in Gfx idle, Bring the GFX clock up
and set the freq to RPn then move GFx down.
v2: remove vlv_update_rps_cur_delay function. Update commit message (Daniel)
v3: Fix the timeout during wait for gfx clock (Jesse)
v4: addressed comments on set freq and punit wait (Ville)
v5: use wait_for while waiting for GFX clk to be up. (Daniel)
update cur_delay before requesting min_delay. (Ville)
Signed-off-by: Deepak S
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_irq.c | 2 +-
drivers/gpu/drm/i915/i915_reg.h | 4 +++
drivers/gpu/drm/i915/intel_pm.c | 57 -
4 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ac5cd7e..6f9108d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1961,6 +1961,8 @@ extern void intel_console_resume(struct work_struct
*work);
void i915_queue_hangcheck(struct drm_device *dev);
void i915_handle_error(struct drm_device *dev, bool wedged);
+void gen6_set_pm_mask(struct drm_i915_private *dev_priv, u32 pm_iir,
+ int new_delay);
extern void intel_irq_init(struct drm_device *dev);
extern void intel_hpd_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b226ae6..acbee73 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -986,7 +986,7 @@ static void notify_ring(struct drm_device *dev,
i915_queue_hangcheck(dev);
}
-static void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
+void gen6_set_pm_mask(struct drm_i915_private *dev_priv,
u32 pm_iir, int new_delay)
{
if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cbbaf26..00d2f2d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4949,6 +4949,10 @@
GEN6_PM_RP_DOWN_THRESHOLD | \
GEN6_PM_RP_DOWN_TIMEOUT)
+#define VLV_GTLC_SURVIVABILITY_REG 0x130098
+#define VLV_GFX_CLK_STATUS_BIT (1<<3)
+#define VLV_GFX_CLK_FORCE_ON_BIT (1<<2)
+
#define GEN6_GT_GFX_RC6_LOCKED 0x138104
#define VLV_COUNTER_CONTROL0x138104
#define VLV_COUNT_RANGE_HIGH (1<<15)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4876ba5..36795ee 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3038,6 +3038,60 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
trace_intel_gpu_freq_change(val * 50);
}
+/* vlv_set_rps_idle: Set the frequency to Rpn if Gfx clocks are down
+ *
+ * * If Gfx is Idle, then
+ * 1. Mask Turbo interrupts
+ * 2. Bring up Gfx clock
+ * 3. Change the freq to Rpn and wait till P-Unit updates freq
+ * 4. Clear the Force GFX CLK ON bit so that Gfx can down
+ * 5. Unmask Turbo interrupts
+*/
+static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
+{
+ /*
+* When we are idle. Drop to min voltage state.
+*/
+
+ if (dev_priv->rps.cur_delay <= dev_priv->rps.min_delay)
+ return;
+
+ /* Mask turbo interrupt so that they will not come in between */
+ I915_WRITE(GEN6_PMINTRMSK, 0x);
+
+ /* Bring up the Gfx clock */
+ I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
+ I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
+ VLV_GFX_CLK_FORCE_ON_BIT);
+
+ if (wait_for(((VLV_GFX_CLK_STATUS_BIT &
+ I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
+ DRM_ERROR("GFX_CLK_ON request timed out\n");
+ return;
+ }
+
+ dev_priv->rps.cur_delay = dev_priv->rps.min_delay;
+
+ vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
+ dev_priv->rps.min_delay);
+
+ if (wait_for_atomic(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
+ & GENFREQSTATUS) == 0, 5))
+ DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
+
+ /* Release the Gfx clock */
+ I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
+ I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
+ ~VLV_GFX_CLK_FORCE_ON_BIT);
+
+ /* Unmask Up interrupts */
+ dev_priv->rps.rp_up_masked = true;
+ gen6_set_pm_mask(dev_priv, GEN6_PM_RP_DOWN_THRESHOLD,
+ dev_priv->rps.min_delay);
+}
+
+
+
void gen6_rps_idle(struct drm_i915_pri
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)
On Thu, Jan 30, 2014 at 05:32:10PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 30, 2014 at 03:11:08PM +, Chris Wilson wrote:
> > On Thu, Jan 30, 2014 at 05:08:09PM +0200, Ville Syrjälä wrote:
> > > On Thu, Jan 30, 2014 at 02:38:16PM +, Chris Wilson wrote:
> > > > We want to use the GTT for reading back objects upon an error so that we
> > > > have exactly the information that the GPU saw. However, it is verboten
> > > > to access snoopable pages through the GTT and causes my PineView GPU to
> > > > throw a page fault instead.
> > > >
> > > > This has not been a problem in the past as we only dumped ringbuffers
> > > > and batchbuffers, both of which must be uncached. However, the
> > > > introduction of HWS page dumping leads to a read of a snooped object
> > > > through the GTT. This was introduced by
> > > >
> > > > commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> > > > Author: Chris Wilson
> > > > Date: Thu Jan 23 22:40:36 2014 +
> > > >
> > > > drm/i915: Include HW status page in error capture
> > > >
> > > > Signed-off-by: Chris Wilson
> > > > ---
> > > > drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > index 6d5ab945132c..3b18c2dff3b8 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > > @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct
> > > > drm_i915_private *dev_priv,
> > > > goto unwind;
> > > >
> > > > local_irq_save(flags);
> > > > - if (reloc_offset < dev_priv->gtt.mappable_end &&
> > > > + if (src->cache_level == I915_CACHE_NONE &&
> > > > + reloc_offset < dev_priv->gtt.mappable_end &&
> > > > src->has_global_gtt_mapping &&
> > > > i915_is_ggtt(vm)) {
> > > > void __iomem *s;
> > >
> > > So you don't want to make it (cache_level == NONE || HAS_LLC)?
> >
> > That llc is coherent between the physical pages and GTT is deep rooted
> > in our code by now, and Ben is working hard to make sure we never take
> > that path on llc anyway. Hence I let it go.
>
> Fine by me. For the series:
>
> Reviewed-by: Ville Syrjälä
Both merge, with uncached rectified.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: vlv: fix DP PHY lockup due to invalid PP sequencer setup
On Thu, 2014-01-30 at 16:52 +0100, Daniel Vetter wrote:
> On Thu, Jan 30, 2014 at 04:50:42PM +0200, Imre Deak wrote:
> > Atm we setup the HW panel power sequencer logic both for eDP and DP
> > ports. On eDP we then go on and start the power on sequence and commence
> > with link training when it's ready. On DP we don't do the power on
> > sequencing but do the link training immediately. At this point the DP
> > PHY block gets stuck, since - supposedly - it is waiting for the power
> > on sequence to finish. The actual register write that seems to hold off
> > the PHY is PIPEX_PP_ON_DELAYS[Panel Control Port Select]. Writing here
> > a non-0 value eventually sets PIPEX_PP_STATUS[Require Asset Status] to
> > 1 and blocks the PHY until the panel power on is ready.
> >
> > Fix this by not doing any PP sequencing setup for DP ports.
> >
> > Thanks to Ville Syrjälä, Jesse Barnes and Todd Previte for the help in
> > tracking this down.
> >
> > Signed-off-by: Imre Deak
>
> Ah, the infamous ABCD hack we're using all over the place in intel_lvds.c.
> On edp we didn't have a need for it thus far since the "require asset
> status" checks have all been fused of, with the PP being on the PCH and
> the edp port on the north display block. If this is really all we need to
> appease the hardware then I'm heavily in favour of it as opposed to
> resurrect the ABCD hack for intel_dp.c.
Yea, it seems it's not needed on BYT, since even with the PP_CONTROL
being all 0 (and not doing any further PP sequencing setup) things work
fine.
> One thing though: Should we add a check for the "Required Asset Status"
> bit somewhere? I don't really have a good idea for a spot to put this
> into, hence the question.
We could add it for eDP, but I guess based on the above for DP we don't
need to check it.
--Imre
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 10 ++
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index ffac7e8..b744073 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1933,10 +1933,12 @@ static void vlv_pre_enable_dp(struct intel_encoder
> > *encoder)
> >
> > mutex_unlock(&dev_priv->dpio_lock);
> >
> > - /* init power sequencer on this pipe and port */
> > - intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
> > - &power_seq);
> > + if (is_edp(intel_dp)) {
> > + /* init power sequencer on this pipe and port */
> > + intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
> > + &power_seq);
> > + }
> >
> > intel_enable_dp(encoder);
> >
> > --
> > 1.8.4
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
signature.asc
Description: This is a digitally signed message part
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: vlv: fix DP PHY lockup due to invalid PP sequencer setup
On Thu, Jan 30, 2014 at 04:52:24PM +0100, Daniel Vetter wrote: > On Thu, Jan 30, 2014 at 04:50:42PM +0200, Imre Deak wrote: > > Atm we setup the HW panel power sequencer logic both for eDP and DP > > ports. On eDP we then go on and start the power on sequence and commence > > with link training when it's ready. On DP we don't do the power on > > sequencing but do the link training immediately. At this point the DP > > PHY block gets stuck, since - supposedly - it is waiting for the power > > on sequence to finish. The actual register write that seems to hold off > > the PHY is PIPEX_PP_ON_DELAYS[Panel Control Port Select]. Writing here > > a non-0 value eventually sets PIPEX_PP_STATUS[Require Asset Status] to > > 1 and blocks the PHY until the panel power on is ready. > > > > Fix this by not doing any PP sequencing setup for DP ports. > > > > Thanks to Ville Syrjälä, Jesse Barnes and Todd Previte for the help in > > tracking this down. > > > > Signed-off-by: Imre Deak > > Ah, the infamous ABCD hack we're using all over the place in intel_lvds.c. > On edp we didn't have a need for it thus far since the "require asset > status" checks have all been fused of, with the PP being on the PCH and > the edp port on the north display block. If this is really all we need to > appease the hardware then I'm heavily in favour of it as opposed to > resurrect the ABCD hack for intel_dp.c. > > One thing though: Should we add a check for the "Required Asset Status" > bit somewhere? I don't really have a good idea for a spot to put this > into, hence the question. Also: Picked up for -fixes, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: vlv: fix DP PHY lockup due to invalid PP sequencer setup
On Thu, Jan 30, 2014 at 04:50:42PM +0200, Imre Deak wrote:
> Atm we setup the HW panel power sequencer logic both for eDP and DP
> ports. On eDP we then go on and start the power on sequence and commence
> with link training when it's ready. On DP we don't do the power on
> sequencing but do the link training immediately. At this point the DP
> PHY block gets stuck, since - supposedly - it is waiting for the power
> on sequence to finish. The actual register write that seems to hold off
> the PHY is PIPEX_PP_ON_DELAYS[Panel Control Port Select]. Writing here
> a non-0 value eventually sets PIPEX_PP_STATUS[Require Asset Status] to
> 1 and blocks the PHY until the panel power on is ready.
>
> Fix this by not doing any PP sequencing setup for DP ports.
>
> Thanks to Ville Syrjälä, Jesse Barnes and Todd Previte for the help in
> tracking this down.
>
> Signed-off-by: Imre Deak
Ah, the infamous ABCD hack we're using all over the place in intel_lvds.c.
On edp we didn't have a need for it thus far since the "require asset
status" checks have all been fused of, with the PP being on the PCH and
the edp port on the north display block. If this is really all we need to
appease the hardware then I'm heavily in favour of it as opposed to
resurrect the ABCD hack for intel_dp.c.
One thing though: Should we add a check for the "Required Asset Status"
bit somewhere? I don't really have a good idea for a spot to put this
into, hence the question.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_dp.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ffac7e8..b744073 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1933,10 +1933,12 @@ static void vlv_pre_enable_dp(struct intel_encoder
> *encoder)
>
> mutex_unlock(&dev_priv->dpio_lock);
>
> - /* init power sequencer on this pipe and port */
> - intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
> - &power_seq);
> + if (is_edp(intel_dp)) {
> + /* init power sequencer on this pipe and port */
> + intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
> + &power_seq);
> + }
>
> intel_enable_dp(encoder);
>
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Excessive WARN()s in Intel 915 driver
On Thu, 30 Jan 2014, Daniel Vetter wrote: > Ok, I seem to have been truly blind all the time. This seems to have been > fallout from > > commit b6c5164d7bf624f3e1b750787ddb983150c5117c > Author: Daniel Vetter > Date: Fri Apr 12 18:48:43 2013 +0200 > > drm/i915: Fixup Oops in the pipe config computation > > Meanwhile we've moved the overall infrastructure ahead again quite a bit, > so I think it's time to give the full atomic modeset paths another shot. > But I'll be travelling to fosdem the next few days, so this will take a > bit of time. > > As long as there's not real bad side-effects I guess we simply need to > live with the WARN for a tad longer. Okay. Let me know when you've got something else ready to test. Alan Stern ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Use i915_hw_context to set reset stats
On Thu, Jan 30, 2014 at 04:01:15PM +0200, Mika Kuoppala wrote: > With full ppgtt support drm_i915_file_private gained knowledge > about the default context. Also reset stats are now inside > i915_hw_context so we can use proper abstraction. > > v2: Move BUG_ON and WARN_ON to more proper locations (Ben) > > Suggested-by: Ben Widawsky > Signed-off-by: Mika Kuoppala > Reviewed-by: Ben Widawsky [snip] > + BUG_ON(!ctx->last_ring); I've changed this to a WARN_ON - killing the entire driver over this seems a bit excessive. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)
On Thu, Jan 30, 2014 at 03:11:08PM +, Chris Wilson wrote:
> On Thu, Jan 30, 2014 at 05:08:09PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 30, 2014 at 02:38:16PM +, Chris Wilson wrote:
> > > We want to use the GTT for reading back objects upon an error so that we
> > > have exactly the information that the GPU saw. However, it is verboten
> > > to access snoopable pages through the GTT and causes my PineView GPU to
> > > throw a page fault instead.
> > >
> > > This has not been a problem in the past as we only dumped ringbuffers
> > > and batchbuffers, both of which must be uncached. However, the
> > > introduction of HWS page dumping leads to a read of a snooped object
> > > through the GTT. This was introduced by
> > >
> > > commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> > > Author: Chris Wilson
> > > Date: Thu Jan 23 22:40:36 2014 +
> > >
> > > drm/i915: Include HW status page in error capture
> > >
> > > Signed-off-by: Chris Wilson
> > > ---
> > > drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > index 6d5ab945132c..3b18c2dff3b8 100644
> > > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > > @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct
> > > drm_i915_private *dev_priv,
> > > goto unwind;
> > >
> > > local_irq_save(flags);
> > > - if (reloc_offset < dev_priv->gtt.mappable_end &&
> > > + if (src->cache_level == I915_CACHE_NONE &&
> > > + reloc_offset < dev_priv->gtt.mappable_end &&
> > > src->has_global_gtt_mapping &&
> > > i915_is_ggtt(vm)) {
> > > void __iomem *s;
> >
> > So you don't want to make it (cache_level == NONE || HAS_LLC)?
>
> That llc is coherent between the physical pages and GTT is deep rooted
> in our code by now, and Ben is working hard to make sure we never take
> that path on llc anyway. Hence I let it go.
Fine by me. For the series:
Reviewed-by: Ville Syrjälä
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Excessive WARN()s in Intel 915 driver
On Thu, Jan 30, 2014 at 09:53:54AM -0500, Alan Stern wrote:
> On Tue, 28 Jan 2014, Daniel Vetter wrote:
>
> > I think I need a bit more debug output first. Can you please apply the
> > below patch to drm-intel-nightly and then grab a drm.debug=0xe dmesg from
> > boot?
>
> The dmesg output is below. Since you didn't say whether this should go
> on top of the previous patch or in place of it, I put this in place of
> the first one.
>
> BTW, I had to fix a bug in the patch:
>
> > @@ -9171,6 +9181,11 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc,
> > unsigned *modeset_pipes,
> > *disable_pipes |= 1 << intel_crtc->pipe;
> > else
> > *prepare_pipes |= 1 << intel_crtc->pipe;
> > +
> > + DRM_DEBUG_KMS("[CRTC:%d:%s]: prepare_pipes %u\n",
> > + intel_crtc->base.base.id,
> > + pipe_name(intel_crtc->pipe),
> > + *prepare_pipes);
> > }
>
> pipe_name() returns char, not char *. I changed the output format
> specifier from %s to %c.
Ok, I seem to have been truly blind all the time. This seems to have been
fallout from
commit b6c5164d7bf624f3e1b750787ddb983150c5117c
Author: Daniel Vetter
Date: Fri Apr 12 18:48:43 2013 +0200
drm/i915: Fixup Oops in the pipe config computation
Meanwhile we've moved the overall infrastructure ahead again quite a bit,
so I think it's time to give the full atomic modeset paths another shot.
But I'll be travelling to fosdem the next few days, so this will take a
bit of time.
As long as there's not real bad side-effects I guess we simply need to
live with the WARN for a tad longer.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)
On Thu, Jan 30, 2014 at 05:08:09PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 30, 2014 at 02:38:16PM +, Chris Wilson wrote:
> > We want to use the GTT for reading back objects upon an error so that we
> > have exactly the information that the GPU saw. However, it is verboten
> > to access snoopable pages through the GTT and causes my PineView GPU to
> > throw a page fault instead.
> >
> > This has not been a problem in the past as we only dumped ringbuffers
> > and batchbuffers, both of which must be uncached. However, the
> > introduction of HWS page dumping leads to a read of a snooped object
> > through the GTT. This was introduced by
> >
> > commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> > Author: Chris Wilson
> > Date: Thu Jan 23 22:40:36 2014 +
> >
> > drm/i915: Include HW status page in error capture
> >
> > Signed-off-by: Chris Wilson
> > ---
> > drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> > b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 6d5ab945132c..3b18c2dff3b8 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private
> > *dev_priv,
> > goto unwind;
> >
> > local_irq_save(flags);
> > - if (reloc_offset < dev_priv->gtt.mappable_end &&
> > + if (src->cache_level == I915_CACHE_NONE &&
> > + reloc_offset < dev_priv->gtt.mappable_end &&
> > src->has_global_gtt_mapping &&
> > i915_is_ggtt(vm)) {
> > void __iomem *s;
>
> So you don't want to make it (cache_level == NONE || HAS_LLC)?
That llc is coherent between the physical pages and GTT is deep rooted
in our code by now, and Ben is working hard to make sure we never take
that path on llc anyway. Hence I let it go.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)
On Thu, Jan 30, 2014 at 02:38:16PM +, Chris Wilson wrote:
> We want to use the GTT for reading back objects upon an error so that we
> have exactly the information that the GPU saw. However, it is verboten
> to access snoopable pages through the GTT and causes my PineView GPU to
> throw a page fault instead.
>
> This has not been a problem in the past as we only dumped ringbuffers
> and batchbuffers, both of which must be uncached. However, the
> introduction of HWS page dumping leads to a read of a snooped object
> through the GTT. This was introduced by
>
> commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
> Author: Chris Wilson
> Date: Thu Jan 23 22:40:36 2014 +
>
> drm/i915: Include HW status page in error capture
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 6d5ab945132c..3b18c2dff3b8 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private
> *dev_priv,
> goto unwind;
>
> local_irq_save(flags);
> - if (reloc_offset < dev_priv->gtt.mappable_end &&
> + if (src->cache_level == I915_CACHE_NONE &&
> + reloc_offset < dev_priv->gtt.mappable_end &&
> src->has_global_gtt_mapping &&
> i915_is_ggtt(vm)) {
> void __iomem *s;
So you don't want to make it (cache_level == NONE || HAS_LLC)?
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup
On Thu, Jan 30, 2014 at 4:05 PM, Jani Nikula wrote: > On Thu, 30 Jan 2014, Daniel Vetter wrote: >> On Thu, Jan 30, 2014 at 11:37:00AM +0200, Jani Nikula wrote: >>> Fix and cleanup the rather confused native aux read return values and >>> return value checks. Maybe we'll get them right from now on. Just maybe. >>> >>> Did not have a chance to test this, I'm afraid, but it's pretty >>> straightforward. >> >> How does this fit in with Thierry's dp aux helper improvements? I prefer >> if we start to converge on that sooner than later ... > > For the i2c-over-aux conversion > > commit 8a5e6aeb30ecaf8f11a99c0d008c8935cd6fba9f > Author: Paulo Zanoni > Date: Wed Oct 30 19:50:26 2013 -0200 > > drm/i915: turn the eDP VDD on for any i2c transactions > > will be a PITA, as we lose control of that level in the stack. > > So we'd need to move the vdd enable/disable higher or lower. Per that > commit, higher was problematic. If we move it lower, we may need to add > refcounting to the vdd, but then we'll lose the warn for "eDP VDD > already requested on\n". > > Ideas welcome. Nasty. Although this was just to make the /dev i2c access work from userspace, so I don't expect more places to pop up. Hence I think a panel_vdd_force_on which returns the current state + panel_vdd_restore to undo that in the low-level dp aux to be sufficient. But not sure. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup
On Thu, 30 Jan 2014, Daniel Vetter wrote: > On Thu, Jan 30, 2014 at 11:37:00AM +0200, Jani Nikula wrote: >> Fix and cleanup the rather confused native aux read return values and >> return value checks. Maybe we'll get them right from now on. Just maybe. >> >> Did not have a chance to test this, I'm afraid, but it's pretty >> straightforward. > > How does this fit in with Thierry's dp aux helper improvements? I prefer > if we start to converge on that sooner than later ... For the i2c-over-aux conversion commit 8a5e6aeb30ecaf8f11a99c0d008c8935cd6fba9f Author: Paulo Zanoni Date: Wed Oct 30 19:50:26 2013 -0200 drm/i915: turn the eDP VDD on for any i2c transactions will be a PITA, as we lose control of that level in the stack. So we'd need to move the vdd enable/disable higher or lower. Per that commit, higher was problematic. If we move it lower, we may need to add refcounting to the vdd, but then we'll lose the warn for "eDP VDD already requested on\n". Ideas welcome. Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/13] drm/i915/bdw: collect semaphore error state
On Thu, Jan 30, 2014 at 04:53:32PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 29, 2014 at 11:55:31AM -0800, Ben Widawsky wrote:
> > + obj = error->semaphore_obj;
> > + if (obj) {
>
> Chris will come along and change this to
>
> if ((obj = error->semaphore_obj))
I was merely keeping in style with the rest of the code. Which was
probably written by me, so I can't win!
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/13] drm/i915/bdw: collect semaphore error state
On Wed, Jan 29, 2014 at 11:55:31AM -0800, Ben Widawsky wrote:
> Since the semaphore information is in an object, just dump it, and let
> the user parse it later.
>
> NOTE: The page being used for the semaphores are incoherent with the
> CPU. No matter what I do, I cannot figure out a way to read anything but
> 0s. Note that the semaphore waits are indeed working.
>
> v2: Don't print signal, and wait (they should be the same). Instead,
> print sync_seqno (Chris)
>
> v3: Free the semaphore error object (Chris)
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gpu_error.c | 47
> ++---
> drivers/gpu/drm/i915/intel_ringbuffer.h | 12 -
> 3 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f521059..b08e6eb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -313,6 +313,7 @@ struct drm_i915_error_state {
> u32 acthd[I915_NUM_RINGS];
> u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
> u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
> + struct drm_i915_error_object *semaphore_obj;
> u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
> /* our own tracking of ring head and tail */
> u32 cpu_ring_head[I915_NUM_RINGS];
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index efaad96..d6afc01 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -297,6 +297,7 @@ int i915_error_state_to_str(struct
> drm_i915_error_state_buf *m,
> struct drm_device *dev = error_priv->dev;
> drm_i915_private_t *dev_priv = dev->dev_private;
> struct drm_i915_error_state *error = error_priv->error;
> + struct drm_i915_error_object *obj;
> int i, j, page, offset, elt;
>
> if (!error) {
> @@ -345,8 +346,6 @@ int i915_error_state_to_str(struct
> drm_i915_error_state_buf *m,
> error->pinned_bo_count[0]);
>
> for (i = 0; i < ARRAY_SIZE(error->ring); i++) {
> - struct drm_i915_error_object *obj;
> -
> if ((obj = error->ring[i].batchbuffer)) {
> err_printf(m, "%s --- gtt_offset = 0x%08x\n",
> dev_priv->ring[i].name,
> @@ -421,6 +420,19 @@ int i915_error_state_to_str(struct
> drm_i915_error_state_buf *m,
> }
> }
>
> + obj = error->semaphore_obj;
> + if (obj) {
Chris will come along and change this to
if ((obj = error->semaphore_obj))
> + err_printf(m, "Semaphore page = 0x%08x\n", obj->gtt_offset);
> + for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
> + err_printf(m, "[%04x] %08x %08x %08x %08x\n",
> +elt * 4,
> +obj->pages[0][elt],
> +obj->pages[0][elt+1],
> +obj->pages[0][elt+2],
> +obj->pages[0][elt+3]);
> + }
That'll be the third copy of this page dumping code. Time to refactor?
> + }
> +
> if (error->overlay)
> intel_overlay_print_error_state(m, error->overlay);
>
> @@ -491,6 +503,7 @@ static void i915_error_state_free(struct kref *error_ref)
> kfree(error->ring[i].requests);
> }
>
> + i915_error_object_free(error->semaphore_obj);
> kfree(error->active_bo);
> kfree(error->overlay);
> kfree(error->display);
> @@ -772,6 +785,31 @@ static void gen6_record_semaphore_state(struct
> drm_i915_private *dev_priv,
> }
> }
>
> +static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
> + struct drm_i915_error_state *error,
> + struct intel_ring_buffer *ring)
> +{
> + struct intel_ring_buffer *useless;
> + int i;
> +
> + if (!i915_semaphore_is_enabled(dev_priv->dev))
> + return;
> +
> + if (!error->semaphore_obj)
> + error->semaphore_obj =
> + i915_error_object_create(dev_priv,
> + dev_priv->semaphore_obj,
> + &dev_priv->gtt.base);
> +
> + for_each_ring(useless, dev_priv, i) {
> + u16 signal_offset = GEN8_SIGNAL_OFFSET(ring, i) / 4;
GEN8_SIGNAL_OFFSET() returns the full ggtt offset.
> + u32 *tmp = error->semaphore_obj->pages[0];
> +
> + error->semaphore_mboxes[ring->id][i] = tmp[signal_offset];
> + error->semaphore_seqno[ring->id][i] =
> ring->semaphore.sync_seqno[i];
> + }
> +}
> +
> static void i915_record_ring_state(struct drm_device *dev,
> struct drm_i915_e
[Intel-gfx] [PATCH] drm/i915: vlv: fix DP PHY lockup due to invalid PP sequencer setup
Atm we setup the HW panel power sequencer logic both for eDP and DP
ports. On eDP we then go on and start the power on sequence and commence
with link training when it's ready. On DP we don't do the power on
sequencing but do the link training immediately. At this point the DP
PHY block gets stuck, since - supposedly - it is waiting for the power
on sequence to finish. The actual register write that seems to hold off
the PHY is PIPEX_PP_ON_DELAYS[Panel Control Port Select]. Writing here
a non-0 value eventually sets PIPEX_PP_STATUS[Require Asset Status] to
1 and blocks the PHY until the panel power on is ready.
Fix this by not doing any PP sequencing setup for DP ports.
Thanks to Ville Syrjälä, Jesse Barnes and Todd Previte for the help in
tracking this down.
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/intel_dp.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ffac7e8..b744073 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1933,10 +1933,12 @@ static void vlv_pre_enable_dp(struct intel_encoder
*encoder)
mutex_unlock(&dev_priv->dpio_lock);
- /* init power sequencer on this pipe and port */
- intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
- intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
- &power_seq);
+ if (is_edp(intel_dp)) {
+ /* init power sequencer on this pipe and port */
+ intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
+ intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
+ &power_seq);
+ }
intel_enable_dp(encoder);
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)
On Thu, Jan 30, 2014 at 02:38:16PM +, Chris Wilson wrote: > We want to use the GTT for reading back objects upon an error so that we > have exactly the information that the GPU saw. However, it is verboten > to access snoopable pages through the GTT and causes my PineView GPU to > throw a page fault instead. > > This has not been a problem in the past as we only dumped ringbuffers > and batchbuffers, both of which must be uncached. However, the s/uncached/not snooped/ to avoid confusion on SNB and friends. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Only print information for filing bug reports once
Repeating the same information multiple times is just annoying.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gpu_error.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 8e6d8f744e7e..6d5ab945132c 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -980,6 +980,7 @@ static void i915_gem_capture_buffers(struct
drm_i915_private *dev_priv,
*/
void i915_capture_error_state(struct drm_device *dev)
{
+ static bool warned;
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_error_state *error;
unsigned long flags;
@@ -1000,10 +1001,13 @@ void i915_capture_error_state(struct drm_device *dev)
DRM_INFO("GPU crash dump saved to /sys/class/drm/card%d/error\n",
dev->primary->index);
- DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx
stack, including userspace.\n");
- DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org
against DRI -> DRM/Intel\n");
- DRM_INFO("drm/i915 developers can then reassign to the right component
if it's not a kernel issue.\n");
- DRM_INFO("The gpu crash dump is required to analyze gpu hangs, so
please always attach it.\n");
+ if (!warned) {
+ DRM_INFO("GPU hangs can indicate a bug anywhere in the entire
gfx stack, including userspace.\n");
+ DRM_INFO("Please file a _new_ bug report on
bugs.freedesktop.org against DRI -> DRM/Intel\n");
+ DRM_INFO("drm/i915 developers can then reassign to the right
component if it's not a kernel issue.\n");
+ DRM_INFO("The gpu crash dump is required to analyze gpu hangs,
so please always attach it.\n");
+ warned = true;
+ }
kref_init(&error->ref);
error->eir = I915_READ(EIR);
--
1.9.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Don't access snooped pages through the GTT (even for error capture)
We want to use the GTT for reading back objects upon an error so that we
have exactly the information that the GPU saw. However, it is verboten
to access snoopable pages through the GTT and causes my PineView GPU to
throw a page fault instead.
This has not been a problem in the past as we only dumped ringbuffers
and batchbuffers, both of which must be uncached. However, the
introduction of HWS page dumping leads to a read of a snooped object
through the GTT. This was introduced by
commit f3ce3821393e31a3f1a8ca6c24eb2d735a428445
Author: Chris Wilson
Date: Thu Jan 23 22:40:36 2014 +
drm/i915: Include HW status page in error capture
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gpu_error.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 6d5ab945132c..3b18c2dff3b8 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -527,7 +527,8 @@ i915_error_object_create_sized(struct drm_i915_private
*dev_priv,
goto unwind;
local_irq_save(flags);
- if (reloc_offset < dev_priv->gtt.mappable_end &&
+ if (src->cache_level == I915_CACHE_NONE &&
+ reloc_offset < dev_priv->gtt.mappable_end &&
src->has_global_gtt_mapping &&
i915_is_ggtt(vm)) {
void __iomem *s;
--
1.9.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 2/4] drm/i915: Tune down debug output when context is banned
If we have stopped rings then we know that test is running
so no need for spam. In addition, only spam when default
context gets banned.
v2: - make sure default context ban gets shown (Chris)
- use helper for checking for default context, everywhere (Chris)
v3: - dont be quiet when debug is set (Ben, Daniel)
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=73652
Signed-off-by: Mika Kuoppala
Reviewed-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_drv.h |5 +
drivers/gpu/drm/i915/i915_gem.c |8 +++-
drivers/gpu/drm/i915/i915_gem_context.c |9 ++---
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3673ba1..b34fd31 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2271,6 +2271,11 @@ static inline void i915_gem_context_unreference(struct
i915_hw_context *ctx)
kref_put(&ctx->ref, i915_gem_context_free);
}
+static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
+{
+ return c->id == DEFAULT_CONTEXT_ID;
+}
+
int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4b7e6a2..0d1760f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2317,7 +2317,13 @@ static bool i915_context_is_banned(const struct
i915_hw_context *ctx)
return true;
if (elapsed <= DRM_I915_CTX_BAN_PERIOD) {
- DRM_ERROR("context hanging too fast, declaring banned!\n");
+ if (dev_priv->gpu_error.stop_rings == 0 &&
+ i915_gem_context_is_default(ctx)) {
+ DRM_ERROR("gpu hanging too fast, banning!\n");
+ } else {
+ DRM_DEBUG("context hanging too fast, banning!\n");
+ }
+
return true;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 2b0598e..985c1ed 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -228,11 +228,6 @@ err_out:
return ERR_PTR(ret);
}
-static inline bool is_default_context(struct i915_hw_context *ctx)
-{
- return (ctx->id == DEFAULT_CONTEXT_ID);
-}
-
/**
* 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
@@ -478,7 +473,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
struct i915_hw_context *ctx = p;
/* Ignore the default context because close will handle it */
- if (is_default_context(ctx))
+ if (i915_gem_context_is_default(ctx))
return 0;
i915_gem_context_unreference(ctx);
@@ -656,7 +651,7 @@ static int do_switch(struct intel_ring_buffer *ring,
vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
}
- if (!to->is_initialized || is_default_context(to))
+ if (!to->is_initialized || i915_gem_context_is_default(to))
hw_flags |= MI_RESTORE_INHIBIT;
ret = mi_set_context(ring, to, hw_flags);
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 2/4] drm/i915: Tune down debug output when context is banned
If we have stopped rings then we know that test is running
so no need for spam. In addition, only spam when default
context gets banned.
v2: - make sure default context ban gets shown (Chris)
- use helper for checking for default context, everywhere (Chris)
v3: - dont be quiet when debug is set (Ben, Daniel)
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=73652
Signed-off-by: Mika Kuoppala
Reviewed-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_drv.h |5 +
drivers/gpu/drm/i915/i915_gem.c |8 +++-
drivers/gpu/drm/i915/i915_gem_context.c |9 ++---
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3673ba1..b34fd31 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2271,6 +2271,11 @@ static inline void i915_gem_context_unreference(struct
i915_hw_context *ctx)
kref_put(&ctx->ref, i915_gem_context_free);
}
+static inline bool i915_gem_context_is_default(const struct i915_hw_context *c)
+{
+ return c->id == DEFAULT_CONTEXT_ID;
+}
+
int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
struct drm_file *file);
int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4b7e6a2..0d1760f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2317,7 +2317,13 @@ static bool i915_context_is_banned(const struct
i915_hw_context *ctx)
return true;
if (elapsed <= DRM_I915_CTX_BAN_PERIOD) {
- DRM_ERROR("context hanging too fast, declaring banned!\n");
+ if (dev_priv->gpu_error.stop_rings == 0 &&
+ i915_gem_context_is_default(ctx)) {
+ DRM_ERROR("gpu hanging too fast, banning!\n");
+ } else {
+ DRM_DEBUG("context hanging too fast, banning!\n");
+ }
+
return true;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 2b0598e..985c1ed 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -228,11 +228,6 @@ err_out:
return ERR_PTR(ret);
}
-static inline bool is_default_context(struct i915_hw_context *ctx)
-{
- return (ctx->id == DEFAULT_CONTEXT_ID);
-}
-
/**
* 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
@@ -478,7 +473,7 @@ static int context_idr_cleanup(int id, void *p, void *data)
struct i915_hw_context *ctx = p;
/* Ignore the default context because close will handle it */
- if (is_default_context(ctx))
+ if (i915_gem_context_is_default(ctx))
return 0;
i915_gem_context_unreference(ctx);
@@ -656,7 +651,7 @@ static int do_switch(struct intel_ring_buffer *ring,
vma->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND);
}
- if (!to->is_initialized || is_default_context(to))
+ if (!to->is_initialized || i915_gem_context_is_default(to))
hw_flags |= MI_RESTORE_INHIBIT;
ret = mi_set_context(ring, to, hw_flags);
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 1/4] drm/i915: Use i915_hw_context to set reset stats
With full ppgtt support drm_i915_file_private gained knowledge
about the default context. Also reset stats are now inside
i915_hw_context so we can use proper abstraction.
v2: Move BUG_ON and WARN_ON to more proper locations (Ben)
Suggested-by: Ben Widawsky
Signed-off-by: Mika Kuoppala
Reviewed-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_gem.c | 44 ---
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 39770f7..4b7e6a2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2305,11 +2305,15 @@ static bool i915_request_guilty(struct
drm_i915_gem_request *request,
return false;
}
-static bool i915_context_is_banned(const struct i915_ctx_hang_stats *hs)
+static bool i915_context_is_banned(const struct i915_hw_context *ctx)
{
- const unsigned long elapsed = get_seconds() - hs->guilty_ts;
+ struct drm_i915_private *dev_priv;
+ unsigned long elapsed;
- if (hs->banned)
+ dev_priv = ctx->last_ring->dev->dev_private;
+ elapsed = get_seconds() - ctx->hang_stats.guilty_ts;
+
+ if (ctx->hang_stats.banned)
return true;
if (elapsed <= DRM_I915_CTX_BAN_PERIOD) {
@@ -2324,9 +2328,13 @@ static void i915_set_reset_status(struct
intel_ring_buffer *ring,
struct drm_i915_gem_request *request,
u32 acthd)
{
- struct i915_ctx_hang_stats *hs = NULL;
bool inside, guilty;
unsigned long offset = 0;
+ struct i915_hw_context *ctx = request->ctx;
+ struct i915_ctx_hang_stats *hs;
+
+ if (WARN_ON(!ctx))
+ return;
/* Innocent until proven guilty */
guilty = false;
@@ -2341,28 +2349,22 @@ static void i915_set_reset_status(struct
intel_ring_buffer *ring,
ring->name,
inside ? "inside" : "flushing",
offset,
- request->ctx ? request->ctx->id : 0,
+ ctx->id,
acthd);
guilty = true;
}
- /* If contexts are disabled or this is the default context, use
-* file_priv->reset_state
-*/
- if (request->ctx && request->ctx->id != DEFAULT_CONTEXT_ID)
- hs = &request->ctx->hang_stats;
- else if (request->file_priv)
- hs = &request->file_priv->private_default_ctx->hang_stats;
-
- if (hs) {
- if (guilty) {
- hs->banned = i915_context_is_banned(hs);
- hs->batch_active++;
- hs->guilty_ts = get_seconds();
- } else {
- hs->batch_pending++;
- }
+ BUG_ON(!ctx->last_ring);
+
+ hs = &ctx->hang_stats;
+
+ if (guilty) {
+ hs->banned = i915_context_is_banned(ctx);
+ hs->batch_active++;
+ hs->guilty_ts = get_seconds();
+ } else {
+ hs->batch_pending++;
}
}
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup
On Thu, 30 Jan 2014, Daniel Vetter wrote: > How does this fit in with Thierry's dp aux helper improvements? I prefer > if we start to converge on that sooner than later ... Patch 1/4 actually fixes a potential bug in intel_dp_sink_crc(), which now detects failure only if the read returns 0, but plunges on with any negative error values. So we may want to queue that for fixes. Patch 2/4 could be dropped. I'd like to do 3/4 as a prep patch anyway, as it potentially changes behaviour, and makes conversion to the helpers more straightforward. Patch 4/4 could be dropped. To see how this would pan out, I'm almost done with the native aux conversion (did not look at the i2c-over-aux part yet). If we use those interfaces directly all over the place, the only annoyance is the potential for the same confusion that I've tried to avoid in this series. The only correct check for success is comparing the transfer function return value to the number of bytes that was to be transferred: if (drm_dp_dpcd_read(&intel_dp->dp_aux, offset, buffer, len) != len) but len may in fact be a fairly long #define like DP_DEVICE_SERVICE_IRQ_VECTOR. I do notice Thierry checks for ret < 0 in his code, although, IIUC, it's possible the transfer ends with fewer bytes transferred than requested. I'm beginning to feel like the functions should return and guarantee an error code < 0 if not all bytes were transferred, just for the ease of use. I mean, it's a nice feature to be able to make the distinction, but I can't fathom a practical situation where that would be necessary. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/13] drm/i915/bdw: implement semaphore signal
On Thu, Jan 30, 2014 at 02:18:32PM +0100, Daniel Vetter wrote: > On Thu, Jan 30, 2014 at 1:46 PM, Chris Wilson > wrote: > > Oh. So they changed how post-sync writes operated - this should be a > > separate fix for stable I believe (so that batches are not run before we > > have finished invalidating the TLBs required). > > We have an igt to exercise tlb invalidation stuff, which runs on all > rings. But it only runs a batch, so only uses the CS tlb. Do we need > to extend this? So the spec says: Pipe Control Flush Enable (IVB+) If ENABLED, the PIPE_CONTROL command will wait until all previous writes of immediate data from post sync circles are complete before executing the next command. Post Sync Operation This field specifies an optional action to be taken upon completion of the synchronization operation. TLB Invalidate If ENABLED, all TLBs belonging to Render Engine will be invalidated once the flush operation is complete. Command Streamer Stall Enable If ENABLED, the sync operation will not occur until all previous flush operations pending a completion of those previous flushes will complete, including the flush produced from this command. This enables the command to act similar to the legacy MI_FLUSH command. Going by that, the order is flush, stall, TLB invalidate / post-sync op, [pipe control flush] Based on my reading of the above (which unless someone has a more definitive source) says that without the CONTROL_FLUSH_ENABLE, the CS can continue operations as soon as the flush is complete - in parallel to the TLB invalidate. Adding CONTROL_FLUSH_ENABLE would then stall the CS until the post-sync operation completes. That still leaves the possibility that the TLB invalidate is being performed in parallel and is itself provides no CS sync. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/13] drm/i915/bdw: implement semaphore signal
On Thu, Jan 30, 2014 at 02:18:32PM +0100, Daniel Vetter wrote: > On Thu, Jan 30, 2014 at 1:46 PM, Chris Wilson > wrote: > > Oh. So they changed how post-sync writes operated - this should be a > > separate fix for stable I believe (so that batches are not run before we > > have finished invalidating the TLBs required). > > We have an igt to exercise tlb invalidation stuff, which runs on all > rings. But it only runs a batch, so only uses the CS tlb. Do we need > to extend this? You could try and catch out the sampler. Or it may be that the hardware internally serialises the operation of invalidating the TLBs and lookup. Or it may be just such a slim window that it will only be hit during a demo and never a test case ;) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/13] drm/i915/bdw: poll semaphores
On Wed, Jan 29, 2014 at 11:55:29AM -0800, Ben Widawsky wrote: > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 3cfcc78..3a3ba81 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -812,6 +812,7 @@ gen8_ring_sync(struct intel_ring_buffer *waiter, > > intel_ring_emit(waiter, MI_SEMAPHORE_WAIT | > MI_SEMAPHORE_GLOBAL_GTT | > + MI_SEMAPHORE_POLL | > MI_SEMAPHORE_SAD_GTE_SDD); I was thinking that we shouldn't need this. However the docs suck a bit and they don't actually specify whether the hardware will wait for the signal before even checking the semaphore once. But that sounds so wrong that it can't possibly be true. > intel_ring_emit(waiter, seqno); > intel_ring_emit(waiter, > -- > 1.8.5.3 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/13] drm/i915/bdw: implement semaphore signal
On Thu, Jan 30, 2014 at 1:46 PM, Chris Wilson wrote: > Oh. So they changed how post-sync writes operated - this should be a > separate fix for stable I believe (so that batches are not run before we > have finished invalidating the TLBs required). We have an igt to exercise tlb invalidation stuff, which runs on all rings. But it only runs a batch, so only uses the CS tlb. Do we need to extend this? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add Baytrail PSR Support.
On Wed, Jan 29, 2014 at 01:50:06PM -0200, Rodrigo Vivi wrote: > @@ -7501,6 +7501,9 @@ static void intel_crtc_update_cursor(struct drm_crtc > *crtc, > u32 base = 0, pos = 0; > bool visible; > > + if (IS_VALLEYVIEW(dev)) > + intel_edp_psr_inactivate(dev); > + Inactivate means that we turn off PSR for some period of time until idle again, right? In the case of a moving cursor that means indefinitely. Also it sounds overkill to have to disable PSR just for the cursor sprite. Is there not some preferrable alternatives or hw that dtrt? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Get rid of acthd based guilty batch search
On Wed, Jan 29, 2014 at 01:44:34PM -0800, Ben Widawsky wrote: > Last word: As I've discussed with Chris too, I am overall a bit wary of > removing any use upon hardware for doing a lot of these error triage, > detection and collection functions. I really like that no matter how > bonghits our driver gets, we can read certain registers to try to figure > things out. I say this now since I think after this series I will no > longer have a leg to stand on in the, we shouldn't use requests for > error collection, discussion. Thanks for reading my rant. I know exactly what you mean. But without a complete snapshot of the hardware and memory (including old contents of overwritten buffers) we will always be missing something when we come to post-mortem debugging. I trust tracking activity by seqno though, and in the error states where that itself has been erroneous the fault has stood out (and had been the root cause of the hang anyway). Hence my feeling that it is exactly what I want for port-mortem debugging. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/13] drm/i915/bdw: implement semaphore wait
On Wed, Jan 29, 2014 at 11:55:27AM -0800, Ben Widawsky wrote:
> Semaphore waits use a new instruction, MI_SEMAPHORE_WAIT. The seqno to
> wait on is all well defined by the table in the previous patch. There is
> nothing else different from previous GEN's semaphore synchronization
> code.
>
> v2: Update macros to not require the other ring's ring->id (Chris)
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_reg.h | 3 ++
> drivers/gpu/drm/i915/intel_ringbuffer.c | 66
> +++--
> drivers/gpu/drm/i915/intel_ringbuffer.h | 30 +++
> 3 files changed, 62 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8b745dc..6e8edaf 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -243,6 +243,9 @@
> #define MI_RESTORE_INHIBIT (1<<0)
> #define MI_SEMAPHORE_SIGNAL MI_INSTR(0x1b, 0) /* GEN8+ */
> #define MI_SEMAPHORE_TARGET(engine)((engine)<<15)
> +#define MI_SEMAPHORE_WAITMI_INSTR(0x1c, 2) /* GEN8+ */
> +#define MI_SEMAPHORE_POLL (1<<15)
> +#define MI_SEMAPHORE_SAD_GTE_SDD (1<<12)
> #define MI_STORE_DWORD_IMM MI_INSTR(0x20, 1)
> #define MI_MEM_VIRTUAL (1 << 22) /* 965+ only */
> #define MI_STORE_DWORD_INDEX MI_INSTR(0x21, 1)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index b750835..3cfcc78 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -797,6 +797,31 @@ static inline bool i915_gem_has_seqno_wrapped(struct
> drm_device *dev,
> * @signaller - ring which has, or will signal
> * @seqno - seqno which the waiter will block on
> */
> +
> +static int
> +gen8_ring_sync(struct intel_ring_buffer *waiter,
> +struct intel_ring_buffer *signaller,
> +u32 seqno)
> +{
> + struct drm_i915_private *dev_priv = waiter->dev->dev_private;
> + int ret;
> +
> + ret = intel_ring_begin(waiter, 4);
> + if (ret)
> + return ret;
> +
> + intel_ring_emit(waiter, MI_SEMAPHORE_WAIT |
> + MI_SEMAPHORE_GLOBAL_GTT |
> + MI_SEMAPHORE_SAD_GTE_SDD);
> + intel_ring_emit(waiter, seqno);
> + intel_ring_emit(waiter,
> + lower_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
> + intel_ring_emit(waiter,
> + upper_32_bits(GEN8_WAIT_OFFSET(waiter, signaller->id)));
> + intel_ring_advance(waiter);
> + return 0;
> +}
> +
> static int
> gen6_ring_sync(struct intel_ring_buffer *waiter,
> struct intel_ring_buffer *signaller,
> @@ -1939,39 +1964,6 @@ static int gen6_ring_flush(struct intel_ring_buffer
> *ring,
> return 0;
> }
>
> -/* seqno size is actually only a uint32, but since we plan to use
> MI_FLUSH_DW to
> - * do the writes, and that must have qw aligned offsets, simply pretend it's
> 8b.
> - */
> -#define SEQNO_SIZE sizeof(uint64_t)
> -#define GEN8_SIGNAL_OFFSET(to) \
> - (i915_gem_obj_ggtt_offset(dev_priv->semaphore_obj) + \
> - (ring->id * I915_NUM_RINGS * SEQNO_SIZE) + \
> - (SEQNO_SIZE * (to)))
> -
> -#define GEN8_WAIT_OFFSET(from) \
> - (i915_gem_obj_ggtt_offset(dev_priv->semaphore_obj) + \
> - ((from) * I915_NUM_RINGS * SEQNO_SIZE) + \
> - (SEQNO_SIZE * ring->id))
> -
> -#define GEN8_RING_SEMAPHORE_INIT do { \
> - if (!dev_priv->semaphore_obj) { \
> - break; \
> - } \
> - ring->semaphore.signal_ggtt[RCS] = GEN8_SIGNAL_OFFSET(RCS); \
> - ring->semaphore.signal_ggtt[VCS] = GEN8_SIGNAL_OFFSET(VCS); \
> - ring->semaphore.signal_ggtt[BCS] = GEN8_SIGNAL_OFFSET(BCS); \
> - ring->semaphore.signal_ggtt[VECS] = GEN8_SIGNAL_OFFSET(VECS); \
> - ring->semaphore.mbox[RCS] = GEN8_WAIT_OFFSET(RCS); \
> - ring->semaphore.mbox[VCS] = GEN8_WAIT_OFFSET(VCS); \
> - ring->semaphore.mbox[BCS] = GEN8_WAIT_OFFSET(BCS); \
> - ring->semaphore.mbox[VECS] = GEN8_WAIT_OFFSET(VECS); \
> - ring->semaphore.signal_ggtt[ring->id] = MI_SEMAPHORE_SYNC_INVALID; \
> - ring->semaphore.mbox[ring->id] = GEN6_NOSYNC; \
> - } while(0)
> -#undef seqno_size
> -
> -
> -
Maybe stick this stuff into the header to begin with to avoid churn.
> int intel_init_render_ring_buffer(struct drm_device *dev)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> @@ -2007,7 +1999,7 @@ int intel_init_render_ring_buffer(struct drm_device
> *dev)
> ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
> ring->get_seqno = gen6_ring_get_seqno;
> ring->set_seqno = ring_set_seqno;
> - ring->semaphore.sync_to = gen6_ring_sync;
> + ring->semaphore.sync_to = gen8_ring_sync;
> if (i915_semaphore_is_enabled(dev)) {
> BUG_ON(!dev_priv->semaphore_obj);
> ring->sema
Re: [Intel-gfx] [PATCH 06/13] drm/i915/bdw: implement semaphore signal
On Thu, Jan 30, 2014 at 02:38:17PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 29, 2014 at 11:55:26AM -0800, Ben Widawsky wrote:
> > Semaphore signalling works similarly to previous GENs with the exception
> > that the per ring mailboxes no longer exist. Instead you must define
> > your own space, somewhere in the GTT.
> >
> > The comments in the code define the layout I've opted for, which should
> > be fairly future proof. Ie. I tried to define offsets in abstract terms
> > (NUM_RINGS, seqno size, etc).
> >
> > NOTE: If one wanted to move this to the HWSP they could. I've decided
> > one 4k object would be easier to deal with, and provide potential wins
> > with cache locality, but that's all speculative.
> >
> > v2: Update the macro to not need the other ring's ring->id (Chris)
> > Update the comment to use the correct formula (Chris)
> >
> > Signed-off-by: Ben Widawsky
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_reg.h | 5 +-
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 199
> > +---
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 38 +-
> > 4 files changed, 197 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3673ba1..f521059 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1380,6 +1380,7 @@ typedef struct drm_i915_private {
> >
> > struct pci_dev *bridge_dev;
> > struct intel_ring_buffer ring[I915_NUM_RINGS];
> > + struct drm_i915_gem_object *semaphore_obj;
> > uint32_t last_seqno, next_seqno;
> >
> > drm_dma_handle_t *status_page_dmah;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index cbbaf26..8b745dc 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -216,7 +216,7 @@
> > #define MI_DISPLAY_FLIP_IVB_SPRITE_B (3 << 19)
> > #define MI_DISPLAY_FLIP_IVB_PLANE_C (4 << 19)
> > #define MI_DISPLAY_FLIP_IVB_SPRITE_C (5 << 19)
> > -#define MI_SEMAPHORE_MBOX MI_INSTR(0x16, 1) /* gen6+ */
> > +#define MI_SEMAPHORE_MBOX MI_INSTR(0x16, 1) /* gen6, gen7 */
> > #define MI_SEMAPHORE_GLOBAL_GTT(1<<22)
> > #define MI_SEMAPHORE_UPDATE (1<<21)
> > #define MI_SEMAPHORE_COMPARE (1<<20)
> > @@ -241,6 +241,8 @@
> > #define MI_RESTORE_EXT_STATE_EN (1<<2)
> > #define MI_FORCE_RESTORE (1<<1)
> > #define MI_RESTORE_INHIBIT (1<<0)
> > +#define MI_SEMAPHORE_SIGNALMI_INSTR(0x1b, 0) /* GEN8+ */
> > +#define MI_SEMAPHORE_TARGET(engine) ((engine)<<15)
> > #define MI_STORE_DWORD_IMM MI_INSTR(0x20, 1)
> > #define MI_MEM_VIRTUAL (1 << 22) /* 965+ only */
> > #define MI_STORE_DWORD_INDEX MI_INSTR(0x21, 1)
> > @@ -329,6 +331,7 @@
> > #define PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE(1<<10) /*
> > GM45+ only */
> > #define PIPE_CONTROL_INDIRECT_STATE_DISABLE (1<<9)
> > #define PIPE_CONTROL_NOTIFY (1<<8)
> > +#define PIPE_CONTROL_FLUSH_ENABLE(1<<7) /* gen7+
> > */
Oh. So they changed how post-sync writes operated - this should be a
separate fix for stable I believe (so that batches are not run before we
have finished invalidating the TLBs required).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: enable HiZ Raw Stall Optimization on HSW
On Thu, Jan 30, 2014 at 01:10:07PM +0100, Daniel Vetter wrote: > On Wed, Jan 29, 2014 at 06:23:40PM -0800, Matt Turner wrote: > > On Wed, Jan 29, 2014 at 9:56 AM, Daniel Vetter wrote: > > > On Tue, Jan 28, 2014 at 6:29 AM, Chia-I Wu wrote: > > >> From: Chia-I Wu > > >> > > >> The optimization is available on Ivy Bridge and later, and is disabled by > > >> default. Enabling it helps certain workloads such as GLBenchmark TRex > > >> test. > > >> > > >> No piglit regression. > > >> > > >> v2 > > >> - no need to save the register before suspend as init_clock_gating can > > >>correctly program it after resume > > >> - split IVB change to another commit > > >> > > >> Signed-off-by: Chia-I Wu > > > > > > What about byt? > > > -Daniel > > > > In the previous thread, Ville pointed out that the documentation > > doesn't say anything about BYT for this bit, so it's unknown whether > > it's supported, and Chia-I said > > > > > Though I will leave BDW/VLV out as I do not have the hardware. > > > > I do have BYT, so I'll check it out, but this patch can go in without > > it that information, since Chia-I took Ville's advice and split the > > patches into per-generation hunks. > > Make sense. Patches merged and I'll happily pull in the byt update if that > one checks out ok. and bdw also needs this (bspec says hsw+) -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/13] drm/i915/bdw: implement semaphore signal
On Wed, Jan 29, 2014 at 11:55:26AM -0800, Ben Widawsky wrote:
> Semaphore signalling works similarly to previous GENs with the exception
> that the per ring mailboxes no longer exist. Instead you must define
> your own space, somewhere in the GTT.
>
> The comments in the code define the layout I've opted for, which should
> be fairly future proof. Ie. I tried to define offsets in abstract terms
> (NUM_RINGS, seqno size, etc).
>
> NOTE: If one wanted to move this to the HWSP they could. I've decided
> one 4k object would be easier to deal with, and provide potential wins
> with cache locality, but that's all speculative.
>
> v2: Update the macro to not need the other ring's ring->id (Chris)
> Update the comment to use the correct formula (Chris)
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_reg.h | 5 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 199
> +---
> drivers/gpu/drm/i915/intel_ringbuffer.h | 38 +-
> 4 files changed, 197 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3673ba1..f521059 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1380,6 +1380,7 @@ typedef struct drm_i915_private {
>
> struct pci_dev *bridge_dev;
> struct intel_ring_buffer ring[I915_NUM_RINGS];
> + struct drm_i915_gem_object *semaphore_obj;
> uint32_t last_seqno, next_seqno;
>
> drm_dma_handle_t *status_page_dmah;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cbbaf26..8b745dc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -216,7 +216,7 @@
> #define MI_DISPLAY_FLIP_IVB_SPRITE_B (3 << 19)
> #define MI_DISPLAY_FLIP_IVB_PLANE_C (4 << 19)
> #define MI_DISPLAY_FLIP_IVB_SPRITE_C (5 << 19)
> -#define MI_SEMAPHORE_MBOXMI_INSTR(0x16, 1) /* gen6+ */
> +#define MI_SEMAPHORE_MBOXMI_INSTR(0x16, 1) /* gen6, gen7 */
> #define MI_SEMAPHORE_GLOBAL_GTT(1<<22)
> #define MI_SEMAPHORE_UPDATE(1<<21)
> #define MI_SEMAPHORE_COMPARE (1<<20)
> @@ -241,6 +241,8 @@
> #define MI_RESTORE_EXT_STATE_EN(1<<2)
> #define MI_FORCE_RESTORE (1<<1)
> #define MI_RESTORE_INHIBIT (1<<0)
> +#define MI_SEMAPHORE_SIGNAL MI_INSTR(0x1b, 0) /* GEN8+ */
> +#define MI_SEMAPHORE_TARGET(engine)((engine)<<15)
> #define MI_STORE_DWORD_IMM MI_INSTR(0x20, 1)
> #define MI_MEM_VIRTUAL (1 << 22) /* 965+ only */
> #define MI_STORE_DWORD_INDEX MI_INSTR(0x21, 1)
> @@ -329,6 +331,7 @@
> #define PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE (1<<10) /*
> GM45+ only */
> #define PIPE_CONTROL_INDIRECT_STATE_DISABLE(1<<9)
> #define PIPE_CONTROL_NOTIFY(1<<8)
> +#define PIPE_CONTROL_FLUSH_ENABLE (1<<7) /* gen7+ */
> #define PIPE_CONTROL_VF_CACHE_INVALIDATE (1<<4)
> #define PIPE_CONTROL_CONST_CACHE_INVALIDATE(1<<3)
> #define PIPE_CONTROL_STATE_CACHE_INVALIDATE(1<<2)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 37ae2b1..b750835 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -619,6 +619,13 @@ static int init_render_ring(struct intel_ring_buffer
> *ring)
> static void render_ring_cleanup(struct intel_ring_buffer *ring)
> {
> struct drm_device *dev = ring->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (dev_priv->semaphore_obj) {
> + i915_gem_object_ggtt_unpin(dev_priv->semaphore_obj);
> + drm_gem_object_unreference(&dev_priv->semaphore_obj->base);
> + dev_priv->semaphore_obj = NULL;
> + }
>
> if (ring->scratch.obj == NULL)
> return;
> @@ -632,6 +639,86 @@ static void render_ring_cleanup(struct intel_ring_buffer
> *ring)
> ring->scratch.obj = NULL;
> }
>
> +static int gen8_rcs_signal(struct intel_ring_buffer *signaller,
> +unsigned int num_dwords)
> +{
> +#define MBOX_UPDATE_DWORDS 8
> + struct drm_device *dev = signaller->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_ring_buffer *waiter;
> + int i, ret, num_rings;
> +
> + num_rings = hweight_long(INTEL_INFO(dev)->ring_mask);
> + num_dwords = (num_rings-1) * MBOX_UPDATE_DWORDS;
Again num_dwords +=
> +#undef MBOX_UPDATE_DWORDS
> +
> + /* XXX: + 4 for the caller */
> + ret = intel_ring_begin(signaller, num_dwords + 4);
and the +4 goes away.
> + if (ret)
> + return ret;
> +
> + for_each_ring(waiter, dev_priv, i) {
> + u64 gtt_offset = signaller->semaphore.signal_ggtt[i];
> + if (gtt_offset == MI_S
Re: [Intel-gfx] [PATCH 1/2] drm/i915: enable HiZ Raw Stall Optimization on HSW
On Wed, Jan 29, 2014 at 06:23:40PM -0800, Matt Turner wrote: > On Wed, Jan 29, 2014 at 9:56 AM, Daniel Vetter wrote: > > On Tue, Jan 28, 2014 at 6:29 AM, Chia-I Wu wrote: > >> From: Chia-I Wu > >> > >> The optimization is available on Ivy Bridge and later, and is disabled by > >> default. Enabling it helps certain workloads such as GLBenchmark TRex > >> test. > >> > >> No piglit regression. > >> > >> v2 > >> - no need to save the register before suspend as init_clock_gating can > >>correctly program it after resume > >> - split IVB change to another commit > >> > >> Signed-off-by: Chia-I Wu > > > > What about byt? > > -Daniel > > In the previous thread, Ville pointed out that the documentation > doesn't say anything about BYT for this bit, so it's unknown whether > it's supported, and Chia-I said > > > Though I will leave BDW/VLV out as I do not have the hardware. > > I do have BYT, so I'll check it out, but this patch can go in without > it that information, since Chia-I took Ville's advice and split the > patches into per-generation hunks. Make sense. Patches merged and I'll happily pull in the byt update if that one checks out ok. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915/dp: move intel_dp_aux_native_read_retry() near other reads
Just code shuffling to have intel_dp_aux_native_read_retry() next to its
sibling intel_dp_aux_native_read(). No functional changes.
v2: rebase.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 48 +++
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8c61fce..4d04507 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -662,6 +662,30 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
recv, recv_bytes) == recv_bytes;
}
+/*
+ * Native read with retry for link status and receiver capability reads for
+ * cases where the sink may still be asleep.
+ */
+static bool
+intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
+ uint8_t *recv, int recv_bytes)
+{
+ int i;
+
+ /*
+* Sinks are *supposed* to come up within 1ms from an off state,
+* but we're also supposed to retry 3 times per the spec.
+*/
+ for (i = 0; i < 3; i++) {
+ if (intel_dp_aux_native_read(intel_dp, address, recv,
+recv_bytes))
+ return true;
+ msleep(1);
+ }
+
+ return false;
+}
+
static int
intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
uint8_t write_byte, uint8_t *read_byte)
@@ -1980,30 +2004,6 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder
*encoder)
}
/*
- * Native read with retry for link status and receiver capability reads for
- * cases where the sink may still be asleep.
- */
-static bool
-intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
- uint8_t *recv, int recv_bytes)
-{
- int i;
-
- /*
-* Sinks are *supposed* to come up within 1ms from an off state,
-* but we're also supposed to retry 3 times per the spec.
-*/
- for (i = 0; i < 3; i++) {
- if (intel_dp_aux_native_read(intel_dp, address, recv,
-recv_bytes))
- return true;
- msleep(1);
- }
-
- return false;
-}
-
-/*
* Fetch AUX CH registers 0x202 - 0x207 which contain
* link status information
*/
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915/dp: make intel_dp_aux_native_read() return bool
In most cases we only care about success or fail, so make the function
easier to use, and consistent with intel_dp_aux_native_read_retry().
This fixes the intel_dp_aux_native_read() usage in intel_dp_sink_crc()
which has already assumed bool, and would only have caught very specific
failure modes.
v2: simplify intel_dp_aux_native_read_retry() while at it. (Chris)
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0ef2690..8c61fce 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -613,8 +613,8 @@ intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
/* read bytes from a native aux channel */
static int
-intel_dp_aux_native_read(struct intel_dp *intel_dp,
-uint16_t address, uint8_t *recv, int recv_bytes)
+_intel_dp_aux_native_read(struct intel_dp *intel_dp,
+ uint16_t address, uint8_t *recv, int recv_bytes)
{
uint8_t msg[4];
int msg_bytes;
@@ -654,6 +654,14 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
}
}
+static inline bool
+intel_dp_aux_native_read(struct intel_dp *intel_dp,
+uint16_t address, uint8_t *recv, int recv_bytes)
+{
+ return _intel_dp_aux_native_read(intel_dp, address,
+recv, recv_bytes) == recv_bytes;
+}
+
static int
intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
uint8_t write_byte, uint8_t *read_byte)
@@ -1979,16 +1987,15 @@ static bool
intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
uint8_t *recv, int recv_bytes)
{
- int ret, i;
+ int i;
/*
* Sinks are *supposed* to come up within 1ms from an off state,
* but we're also supposed to retry 3 times per the spec.
*/
for (i = 0; i < 3; i++) {
- ret = intel_dp_aux_native_read(intel_dp, address, recv,
- recv_bytes);
- if (ret == recv_bytes)
+ if (intel_dp_aux_native_read(intel_dp, address, recv,
+recv_bytes))
return true;
msleep(1);
}
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/6] tests/gem_exec_parse: Test for batches w/o MI_BATCH_BUFFER_END
On Wed, Jan 29, 2014 at 10:10:47PM +, Chris Wilson wrote:
> On Wed, Jan 29, 2014 at 01:58:29PM -0800, bradley.d.vol...@intel.com wrote:
> > From: Brad Volkin
> >
> > Signed-off-by: Brad Volkin
> > ---
> > tests/gem_exec_parse.c | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c
> > index 9e90408..004c3bf 100644
> > --- a/tests/gem_exec_parse.c
> > +++ b/tests/gem_exec_parse.c
> > @@ -257,6 +257,15 @@ igt_main
> > -EINVAL));
> > }
> >
> > + igt_subtest("batch-without-end") {
> > + uint32_t noop[1024] = { 0 };
> > + igt_assert(
> > + exec_batch(fd, handle,
> > + noop, sizeof(noop),
> > + I915_EXEC_RENDER,
> > + -EINVAL));
>
> Cheekier would be
> uint32_t empty[] = { MI_NOOP, MI_NOOP, MI_BATCH_BUFFER_END, 0 };
> for_each_ring() {
> igt_assert(exec_batch(fd, handle, empty, sizeof(empty), ring, 0));
> igt_assert(exec_batch(fd, handle, empty, 8, ring, -EINVAL));
> }
On this subject, it should be
{ INVALID, INVALID, NOOP, NOOP, END, 0}
assert(exec(0, 4) == -EINVAL);
assert(exec(0, 8) == -EINVAL);
assert(exec(0, 12) == -EINVAL);
assert(exec(4, 8) == -EINVAL);
assert(exec(4, 12) == 0);
assert(exec(8, 12) == 0);
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Capture PPGTT info on error capture
On Thu, Jan 30, 2014 at 12:26:49PM +0100, Daniel Vetter wrote: > On Thu, Jan 30, 2014 at 12:19:40AM -0800, Ben Widawsky wrote: > > v2: Rebased upon cleaned up error state > > v3: Make sure hangcheck info remains last (Chris) > > > > Cc: Chris Wilson > > Signed-off-by: Ben Widawsky > > Pulled in entire series with Chris' irc ack on the remaining two patches. > Note that there have been a few conflicts aroung ring_error_state->valid > (dunno how that happen, I guess this series wasn't strictly based on > -nightly), please double-check things. Meh, I've forgotten to check -fixes - the ring->valid patch I've been looking for was obviously there. Coffee doesn't seem to work today, I'll do a backmerge and sort this out. Sorry for the fuss. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Capture PPGTT info on error capture
On Thu, Jan 30, 2014 at 12:19:40AM -0800, Ben Widawsky wrote:
> v2: Rebased upon cleaned up error state
> v3: Make sure hangcheck info remains last (Chris)
>
> Cc: Chris Wilson
> Signed-off-by: Ben Widawsky
Pulled in entire series with Chris' irc ack on the remaining two patches.
Note that there have been a few conflicts aroung ring_error_state->valid
(dunno how that happen, I guess this series wasn't strictly based on
-nightly), please double-check things.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 9 +
> drivers/gpu/drm/i915/i915_gpu_error.c | 37
> +++
> 2 files changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e41f30a..3035bf3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -361,6 +361,14 @@ struct drm_i915_error_state {
> u32 seqno;
> u32 tail;
> } *requests;
> +
> + struct {
> + u32 gfx_mode;
> + union {
> + u64 pdp[4];
> + u32 pp_dir_base;
> + };
> + } vm_info;
> } ring[I915_NUM_RINGS];
>
> struct drm_i915_error_buffer {
> @@ -378,6 +386,7 @@ struct drm_i915_error_state {
> s32 ring:4;
> u32 cache_level:3;
> } **active_bo, **pinned_bo;
> +
> u32 *active_bo_count, *pinned_bo_count;
> u32 vm_count;
> };
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 4c3ca11..9d04e6a 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -270,6 +270,19 @@ static void i915_ring_error_state(struct
> drm_i915_error_state_buf *m,
> ring->semaphore_seqno[2]);
> }
> }
> + if (USES_PPGTT(dev)) {
> + err_printf(m, " GFX_MODE: 0x%08x\n", ring->vm_info.gfx_mode);
> +
> + if (INTEL_INFO(dev)->gen >= 8) {
> + int i;
> + for (i = 0; i < 4; i++)
> + err_printf(m, " PDP%d: 0x%016llx\n",
> +i, ring->vm_info.pdp[i]);
> + } else {
> + err_printf(m, " PP_DIR_BASE: 0x%08x\n",
> +ring->vm_info.pp_dir_base);
> + }
> + }
> err_printf(m, " seqno: 0x%08x\n", ring->seqno);
> err_printf(m, " waiting: %s\n", yesno(ring->waiting));
> err_printf(m, " ring->head: 0x%08x\n", ring->cpu_ring_head);
> @@ -847,6 +860,30 @@ static void i915_record_ring_state(struct drm_device
> *dev,
>
> ering->hangcheck_score = ring->hangcheck.score;
> ering->hangcheck_action = ring->hangcheck.action;
> +
> + if (USES_PPGTT(dev)) {
> + int i;
> +
> + ering->vm_info.gfx_mode = I915_READ(RING_MODE_GEN7(ring));
> +
> + switch (INTEL_INFO(dev)->gen) {
> + case 8:
> + for (i = 0; i < 4; i++) {
> + ering->vm_info.pdp[i] =
> + I915_READ(GEN8_RING_PDP_UDW(ring, i));
> + ering->vm_info.pdp[i] <<= 32;
> + ering->vm_info.pdp[i] |=
> + I915_READ(GEN8_RING_PDP_LDW(ring, i));
> + }
> + break;
> + case 7:
> + ering->vm_info.pp_dir_base = RING_PP_DIR_BASE(ring);
> + break;
> + case 6:
> + ering->vm_info.pp_dir_base =
> RING_PP_DIR_BASE_READ(ring);
> + break;
> + }
> + }
> }
>
>
> --
> 1.8.5.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/13] drm/i915: Make semaphore updates more precise
On Wed, Jan 29, 2014 at 11:55:24AM -0800, Ben Widawsky wrote:
> With the ring mask we now have an easy way to know the number of rings
> in the system, and therefore can accurately predict the number of dwords
> to emit for semaphore signalling. This was not possible (easily)
> previously.
>
> There should be no functional impact, simply fewer instructions emitted.
>
> While we're here, simply do the round up to 2 instead of the fancier
> rounding we did before, which rounding up per mbox, ie 4.
>
> Signed-off-by: Ben Widawsky
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 43
> +
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 70f7190..97789ff 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -635,24 +635,20 @@ static void render_ring_cleanup(struct
> intel_ring_buffer *ring)
> static int gen6_signal(struct intel_ring_buffer *signaller,
> unsigned int num_dwords)
> {
> +#define MBOX_UPDATE_DWORDS 4
> struct drm_device *dev = signaller->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_ring_buffer *useless;
> - int i, ret;
> + int i, ret, num_rings;
>
> - /* NB: In order to be able to do semaphore MBOX updates for varying
> - * number of rings, it's easiest if we round up each individual update
> - * to a multiple of 2 (since ring updates must always be a multiple of
> - * 2) even though the actual update only requires 3 dwords.
> - */
> -#define MBOX_UPDATE_DWORDS 4
> - if (i915_semaphore_is_enabled(dev))
> - num_dwords += ((I915_NUM_RINGS-1) * MBOX_UPDATE_DWORDS);
> + num_rings = hweight_long(INTEL_INFO(dev)->ring_mask);
> + num_dwords = round_up((num_rings-1) * MBOX_UPDATE_DWORDS, 2);
num_dwords +=
Also round_up() is useless since it's already a multiple of 4. Or did
you mean to change it to emit only 3 dwords per mbox?
> +#undef MBOX_UPDATE_DWORDS
>
> - ret = intel_ring_begin(signaller, num_dwords);
> + /* XXX: + 4 for the caller */
> + ret = intel_ring_begin(signaller, num_dwords + 4);
The += earlier gets rid of the +4 here.
> if (ret)
> return ret;
> -#undef MBOX_UPDATE_DWORDS
>
> for_each_ring(useless, dev_priv, i) {
> u32 mbox_reg = signaller->semaphore.signal_mbox[i];
> @@ -661,14 +657,11 @@ static int gen6_signal(struct intel_ring_buffer
> *signaller,
> intel_ring_emit(signaller, mbox_reg);
> intel_ring_emit(signaller,
> signaller->outstanding_lazy_seqno);
> intel_ring_emit(signaller, MI_NOOP);
> - } else {
> - intel_ring_emit(signaller, MI_NOOP);
> - intel_ring_emit(signaller, MI_NOOP);
> - intel_ring_emit(signaller, MI_NOOP);
> - intel_ring_emit(signaller, MI_NOOP);
> }
> }
>
> + WARN_ON(i != num_rings);
So we're not expecting dev_priv->ring[] to be sparsely populated ever?
> +
> return 0;
> }
>
> @@ -686,7 +679,11 @@ gen6_add_request(struct intel_ring_buffer *ring)
> {
> int ret;
>
> - ret = ring->semaphore.signal(ring, 4);
> + if (ring->semaphore.signal)
> + ret = ring->semaphore.signal(ring, 4);
> + else
> + ret = intel_ring_begin(ring, 4);
> +
> if (ret)
> return ret;
>
> @@ -1881,7 +1878,8 @@ int intel_init_render_ring_buffer(struct drm_device
> *dev)
> ring->get_seqno = gen6_ring_get_seqno;
> ring->set_seqno = ring_set_seqno;
> ring->semaphore.sync_to = gen6_ring_sync;
> - ring->semaphore.signal = gen6_signal;
> + if (i915_semaphore_is_enabled(dev))
> + ring->semaphore.signal = gen6_signal;
I guess we could also set .sync_to conditionally, but doesn't really
matter since we won't call it anyway w/o semaphores enabled.
> ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_INVALID;
> ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_RV;
> ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_RB;
> @@ -2058,7 +2056,8 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
> gen6_ring_dispatch_execbuffer;
> }
> ring->semaphore.sync_to = gen6_ring_sync;
> - ring->semaphore.signal = gen6_signal;
> + if (i915_semaphore_is_enabled(dev))
> + ring->semaphore.signal = gen6_signal;
> ring->semaphore.mbox[RCS] = MI_SEMAPHORE_SYNC_VR;
> ring->semaphore.mbox[VCS] = MI_SEMAPHORE_SYNC_INVALID;
> ring->semaphore.mbox[BCS] = MI_SEMAPHORE_SYNC_VB;
> @@ -2116,7 +2115,8 @@ int intel_init_blt_ring_buffer(
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Thu, Jan 30, 2014 at 10:12:06AM +0100, Daniel Vetter wrote:
> On Thu, Jan 30, 2014 at 10:05:28AM +0100, Daniel Vetter wrote:
> > On Thu, Jan 30, 2014 at 09:53:28AM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 29, 2014 at 10:28:36PM +, Chris Wilson wrote:
> > > > On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com
> > > > wrote:
> > > > > +/*
> > > > > + * Returns a pointer to a descriptor for the command specified by
> > > > > cmd_header.
> > > > > + *
> > > > > + * The caller must supply space for a default descriptor via the
> > > > > default_desc
> > > > > + * parameter. If no descriptor for the specified command exists in
> > > > > the ring's
> > > > > + * command parser tables, this function fills in default_desc based
> > > > > on the
> > > > > + * ring's default length encoding and returns default_desc.
> > > > > + */
> > > > > +static const struct drm_i915_cmd_descriptor*
> > > > > +find_cmd(struct intel_ring_buffer *ring,
> > > > > + u32 cmd_header,
> > > > > + struct drm_i915_cmd_descriptor *default_desc)
> > > > > +{
> > > > > + u32 mask;
> > > > > + int i;
> > > > > +
> > > > > + for (i = 0; i < ring->cmd_table_count; i++) {
> > > > > + const struct drm_i915_cmd_descriptor *desc;
> > > > > +
> > > > > + desc = find_cmd_in_table(&ring->cmd_tables[i],
> > > > > cmd_header);
> > > > > + if (desc)
> > > > > + return desc;
> > > > > + }
> > > > > +
> > > > > + mask = ring->get_cmd_length_mask(cmd_header);
> > > > > + if (!mask)
> > > > > + return NULL;
> > > > > +
> > > > > + BUG_ON(!default_desc);
> > > > > + default_desc->flags = CMD_DESC_SKIP;
> > > > > + default_desc->length.mask = mask;
> > > >
> > > > If we turn off all hw validation (through use of the secure bit) should
> > > > we not default to a whitelist of commands? Otherwise it just seems to be
> > > > a case of running a fuzzer until we kill the machine.
> > >
> > > Preventing hangs and dos is imo not the attack model, gpus are too fickle
> > > for that. The attach model here is to prevent priveledge escalation and
> > > information leaks. I.e. we want just containement of all read/write access
> > > to the gtt space.
> > >
> > > I think for that purpose an explicit whitelist of commands which target
> > > things outside of the (pp)gtt is sufficient. radeon's checker design is
> > > completely different, but pretty much the only command they have is
> > > to load register values. Intel gpus otoh have a big set of special-purpose
> > > commands to load (most) of the rendering pipeline state. So we have
> > > hw built-in register whitelists for all that stuff since you just can't
> > > load arbitrary registers and state with those commands.
> > >
> > > Also note that for raw register access Bradley's scanner _is_ whitelist
> > > based. And for general reads/writes gpu designers confirmed that those are
> > > all MI_ commands (with very few specific exceptions like PIPE_CONTROL), so
> > > as long as we check for the exceptions and otherwise only whitelist MI_
> > > commands we know about we should be covered.
> > >
> > > So I think this is sound.
> >
> > Hm, but while scrolling through the checker I haven't spotted a "reject
> > everything unknown" for MI_CLIENT commands. Bradley, have I missed that?
> >
> > I think submitting an invented MI_CLIENT command would also be a good
> > testcase.
>
> One more: I think it would be good to have an overview comment at the top
> of i915_cmd_parser.c which details the security attack model and the
> overall blacklist/whitelist design of the checker. We don't (yet) have
> autogenerated documentation for i915, but that's something I'm working on.
> And the kerneldoc system can also pull in multi-paragraph overview
> comments besides the usual api documentation, so it's good to have things
> ready.
Chatted with Chris a bit more on irc about this, and for more paranoia I
guess we should also reject any unknown client and media subclient
commands.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl
On Wed, Jan 29, 2014 at 10:58:48PM +0100, Daniel Vetter wrote: > On Wed, Jan 29, 2014 at 10:53 PM, Chris Wilson > wrote: > > On Wed, Jan 29, 2014 at 09:25:51PM +0100, Daniel Vetter wrote: > >> So originally I've thought we need this due to the massive overhead of the > >> mmu notifier. But now with the nice shared mmu notifiers I've thought that > >> overhead is gone I prefer to also ditch this option. > >> > >> Same goes about the MMU_NOTIFIER conditional code, imo we simply should > >> select this - most distros will have it anyway and users will be really > >> suprised if they lose userspace driver features for seemingly irrelevant > >> reasons. > > > > Seriously? You think the overhead is magically gone? > > Well the once-per-process overhead is still there, and imo it's ok to > eat that. But the complaints I've heard concerned the per-object > overhead, so I wonder how much of that is still relevant. I am still annoyed by the thought of having to enable an extra feature in my kernels, and the extra code that is then run on every mm operation. (Mixing mmu_notifiers + mm debuging was an especially unpleasant experience that I don't wish to ever do again.) Numbers talk though, if we can't demonstrate a significant difference between the two, it can die. Keeping a debug mode to turn off mmu_notifiers would still be good so that we can keep track of any impact over time. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Thu, Jan 30, 2014 at 10:07:42AM +0100, Daniel Vetter wrote: > On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com wrote: > > From: Brad Volkin > > > > The command parser scans batch buffers submitted via execbuffer ioctls > > before > > the driver submits them to hardware. At a high level, it looks for several > > things: > > > > 1) Commands which are explicitly defined as privileged or which should only > > be > >used by the kernel driver. The parser generally rejects such commands, > > with > >the provision that it may allow some from the drm master process. > > 2) Commands which access registers. To support correct/enhanced userspace > >functionality, particularly certain OpenGL extensions, the parser > > provides a > >whitelist of registers which userspace may safely access (for both > > normal and > >drm master processes). > > 3) Commands which access privileged memory (i.e. GGTT, HWS page, etc). The > >parser always rejects such commands. > > > > Each ring maintains tables of commands and registers which the parser uses > > in > > scanning batch buffers submitted to that ring. > > > > The set of commands that the parser must check for is significantly smaller > > than the number of commands supported, especially on the render ring. As > > such, > > the parser tables (built up in subsequent patches) contain only those > > commands > > required by the parser. This generally works because command opcode ranges > > have > > standard command length encodings. So for commands that the parser does not > > need > > to check, it can easily skip them. This is implementated via a per-ring > > length > > decoding vfunc. > > > > Unfortunately, there are a number of commands that do not follow the > > standard > > length encoding for their opcode range, primarily amongst the MI_* > > commands. To > > handle this, the parser provides a way to define explicit "skip" entries in > > the > > per-ring command tables. > > > > Other command table entries will map fairly directly to high level > > categories > > mentioned above: rejected, master-only, register whitelist. A number of > > checks, > > including the privileged memory checks, are implemented via a general > > bitmasking > > mechanism. > > > > OTC-Tracker: AXIA-4631 > > Change-Id: I50b98c71c6655893291c78a2d1b8954577b37a30 > > Signed-off-by: Brad Volkin > > > > +#include "i915_drv.h" > > + > > +#define CLIENT_MASK 0xE000 > > +#define SUBCLIENT_MASK 0x1800 > > +#define MI_CLIENT0x > > +#define RC_CLIENT0x6000 > > +#define BC_CLIENT0x4000 > > +#define MEDIA_SUBCLIENT 0x1000 > > I think these would fit neatly right next to all the other MI_* #defines > in i915_reg.h. The other idea that just crossed my mind is to extract all > the command #defines into a new i915_cmd.h (included by i915_drv.h by > default) since i915_reg.h is already giant. i915_cmd.h please. (i915_cs.h? i915_command_stream.h?) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915/dp: make intel_dp_aux_native_read() return bool
On Thu, Jan 30, 2014 at 11:37:01AM +0200, Jani Nikula wrote:
> +static inline bool
> +intel_dp_aux_native_read(struct intel_dp *intel_dp,
> + uint16_t address, uint8_t *recv, int recv_bytes)
> +{
> + return _intel_dp_aux_native_read(intel_dp, address,
> + recv, recv_bytes) == recv_bytes;
> +}
> +
> static int
> intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
> uint8_t write_byte, uint8_t *read_byte)
> @@ -1986,8 +1994,8 @@ intel_dp_aux_native_read_retry(struct intel_dp
> *intel_dp, uint16_t address,
>* but we're also supposed to retry 3 times per the spec.
>*/
> for (i = 0; i < 3; i++) {
> - ret = intel_dp_aux_native_read(intel_dp, address, recv,
> -recv_bytes);
> + ret = _intel_dp_aux_native_read(intel_dp, address, recv,
> + recv_bytes);
> if (ret == recv_bytes)
> return true;
ret is not being used other than to evaulate the boolean exactly like
the new intel_dp_aux_native_read().
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup
On Thu, Jan 30, 2014 at 11:37:00AM +0200, Jani Nikula wrote: > Fix and cleanup the rather confused native aux read return values and > return value checks. Maybe we'll get them right from now on. Just maybe. > > Did not have a chance to test this, I'm afraid, but it's pretty > straightforward. How does this fit in with Thierry's dp aux helper improvements? I prefer if we start to converge on that sooner than later ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915/dp: clean up cargo culted intel_dp_aux_native_read_retry() usage
intel_dp_aux_native_read_retry() is only needed when the sink might be
asleep. Use the regular read without retries otherwise.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 32
1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9f305f7..3b2c4f0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2877,9 +2877,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
/* Check if the panel supports PSR */
memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
if (is_edp(intel_dp)) {
- intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
- intel_dp->psr_dpcd,
- sizeof(intel_dp->psr_dpcd));
+ intel_dp_aux_native_read(intel_dp, DP_PSR_SUPPORT,
+intel_dp->psr_dpcd,
+sizeof(intel_dp->psr_dpcd));
if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
dev_priv->psr.sink_support = true;
DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
@@ -2901,9 +2901,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
return true; /* no per-port downstream info */
- if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
- intel_dp->downstream_ports,
- DP_MAX_DOWNSTREAM_PORTS) == 0)
+ if (!intel_dp_aux_native_read(intel_dp, DP_DOWNSTREAM_PORT_0,
+ intel_dp->downstream_ports,
+ DP_MAX_DOWNSTREAM_PORTS))
return false; /* downstream port status fetch failed */
return true;
@@ -2919,11 +2919,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
edp_panel_vdd_on(intel_dp);
- if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
+ if (intel_dp_aux_native_read(intel_dp, DP_SINK_OUI, buf, 3))
DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
buf[0], buf[1], buf[2]);
- if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
+ if (intel_dp_aux_native_read(intel_dp, DP_BRANCH_OUI, buf, 3))
DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
buf[0], buf[1], buf[2]);
@@ -2959,18 +2959,11 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8
*crc)
return 0;
}
-static bool
+static inline bool
intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
{
- int ret;
-
- ret = intel_dp_aux_native_read_retry(intel_dp,
-DP_DEVICE_SERVICE_IRQ_VECTOR,
-sink_irq_vector, 1);
- if (!ret)
- return false;
-
- return true;
+ return intel_dp_aux_native_read(intel_dp, DP_DEVICE_SERVICE_IRQ_VECTOR,
+ sink_irq_vector, 1);
}
static void
@@ -3053,8 +3046,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
uint8_t reg;
- if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
- ®, 1))
+ if (!intel_dp_aux_native_read(intel_dp, DP_SINK_COUNT, ®, 1))
return connector_status_unknown;
return DP_GET_SINK_COUNT(reg) ? connector_status_connected
: connector_status_disconnected;
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] drm/i915/dp: fix intel_dp_aux_native_read_retry() return value check
intel_dp_aux_native_read_retry() returns a bool, so check for a bool for consistency. No functional changes. Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/intel_dp.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 3b2c4f0..27ba24d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2863,8 +2863,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3]; - if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, - sizeof(intel_dp->dpcd)) == 0) + if (!intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, + sizeof(intel_dp->dpcd))) return false; /* aux transfer failed */ hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd), -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915/dp: make intel_dp_aux_native_read() return bool
In most cases we only care about success or fail, so make the function
easier to use, and consistent with intel_dp_aux_native_read_retry().
This fixes the intel_dp_aux_native_read() usage in intel_dp_sink_crc()
which has already assumed bool, and would only have caught very specific
failure modes.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0ef2690..7e3d56e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -613,8 +613,8 @@ intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
/* read bytes from a native aux channel */
static int
-intel_dp_aux_native_read(struct intel_dp *intel_dp,
-uint16_t address, uint8_t *recv, int recv_bytes)
+_intel_dp_aux_native_read(struct intel_dp *intel_dp,
+ uint16_t address, uint8_t *recv, int recv_bytes)
{
uint8_t msg[4];
int msg_bytes;
@@ -654,6 +654,14 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
}
}
+static inline bool
+intel_dp_aux_native_read(struct intel_dp *intel_dp,
+uint16_t address, uint8_t *recv, int recv_bytes)
+{
+ return _intel_dp_aux_native_read(intel_dp, address,
+recv, recv_bytes) == recv_bytes;
+}
+
static int
intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
uint8_t write_byte, uint8_t *read_byte)
@@ -1986,8 +1994,8 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp,
uint16_t address,
* but we're also supposed to retry 3 times per the spec.
*/
for (i = 0; i < 3; i++) {
- ret = intel_dp_aux_native_read(intel_dp, address, recv,
- recv_bytes);
+ ret = _intel_dp_aux_native_read(intel_dp, address, recv,
+ recv_bytes);
if (ret == recv_bytes)
return true;
msleep(1);
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915/dp: move intel_dp_aux_native_read_retry() near other reads
Just code shuffling to have intel_dp_aux_native_read_retry() next to its
sibling intel_dp_aux_native_read(). No functional changes.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/i915/intel_dp.c | 50 +++
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7e3d56e..9f305f7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -662,6 +662,31 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
recv, recv_bytes) == recv_bytes;
}
+/*
+ * Native read with retry for link status and receiver capability reads for
+ * cases where the sink may still be asleep.
+ */
+static bool
+intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
+ uint8_t *recv, int recv_bytes)
+{
+ int ret, i;
+
+ /*
+* Sinks are *supposed* to come up within 1ms from an off state,
+* but we're also supposed to retry 3 times per the spec.
+*/
+ for (i = 0; i < 3; i++) {
+ ret = _intel_dp_aux_native_read(intel_dp, address, recv,
+ recv_bytes);
+ if (ret == recv_bytes)
+ return true;
+ msleep(1);
+ }
+
+ return false;
+}
+
static int
intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
uint8_t write_byte, uint8_t *read_byte)
@@ -1980,31 +2005,6 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder
*encoder)
}
/*
- * Native read with retry for link status and receiver capability reads for
- * cases where the sink may still be asleep.
- */
-static bool
-intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
- uint8_t *recv, int recv_bytes)
-{
- int ret, i;
-
- /*
-* Sinks are *supposed* to come up within 1ms from an off state,
-* but we're also supposed to retry 3 times per the spec.
-*/
- for (i = 0; i < 3; i++) {
- ret = _intel_dp_aux_native_read(intel_dp, address, recv,
- recv_bytes);
- if (ret == recv_bytes)
- return true;
- msleep(1);
- }
-
- return false;
-}
-
-/*
* Fetch AUX CH registers 0x202 - 0x207 which contain
* link status information
*/
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup
Fix and cleanup the rather confused native aux read return values and return value checks. Maybe we'll get them right from now on. Just maybe. Did not have a chance to test this, I'm afraid, but it's pretty straightforward. BR, Jani. Jani Nikula (4): drm/i915/dp: make intel_dp_aux_native_read() return bool drm/i915/dp: move intel_dp_aux_native_read_retry() near other reads drm/i915/dp: clean up cargo culted intel_dp_aux_native_read_retry() usage drm/i915/dp: fix intel_dp_aux_native_read_retry() return value check drivers/gpu/drm/i915/intel_dp.c | 98 +++ 1 file changed, 49 insertions(+), 49 deletions(-) -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests/gem_userptr_blits: Expanded userptr test cases
On Thu, Jan 30, 2014 at 10:20 AM, Tvrtko Ursulin wrote: > On 01/29/2014 08:11 PM, Daniel Vetter wrote: >> >> On Wed, Jan 29, 2014 at 01:30:54PM +, Tvrtko Ursulin wrote: >>> >>> From: Tvrtko Ursulin >>> >>> A set of userptr test cases to support the new feature. > > > [snip] > > >> Minor thing on patch style: I'd split this up into 3 parts: >> - Extraction of the helpers - always useful to shine a bit light onto new >>helper stuff so other people also notice them. >> - The new testcase. >> - Removal of the old vmap testcase. > > > I was afraid someone will say that, but was hoping for a lower bar since, to > quote what yourself say later in this mail, "is a bit evil, but this is a > testuite ;-)". :) > > Okay, I'll split it up. > > >> The other patch style thing are the helpers - the forking_eviction stuff >> doesn't really sound like a bit of helper code. igt_exchange_uint32_t >> certainly is, but the other stuff I'd just put into a common source file >> which is included by both tests. Yeah #include "common.c" is a bit evil, >> but this is a testsuite ;-) Or just copy&paste the code into the userptr >> test, which is the approach I'd have done. > > > It would be too much c&p for my liking so I chose this route. You have too high standards ;-) I'm quite a bit in favour of massive c&p in testcases since change tests always has the risk to break them, and hence lose a bit of coverage. But since this is for stress-tests the risk for subtle breakage is fairly small I think. >> Now for test coverage it sounds like this testcases here has been more >> than good enough to shake down the userptr implementation, so I think >> we're covered. >> >> But there is also basic interface coverage for sanity-checking and >> defending against evil userspace. For this case here I think we need: >> >> - Tests with un-aligned ptr/size. >> - Tests with invalid flags. > > > Above are already there as test_usage_restrictions and test_input_checking. Oh, missed that. In that case we're good already. >> - Basic nastiness of handing in an invalid pointer. > > > You mean trying to map something which doesn't exist in user address space? > Any idea how to obtain such a pointer? Or just use zero? NULL should be good enough. 2nd version could use a gtt mmap'ed area obtained from i915.ko - those aren't struct page backed either and in addition also provoke lock inversion issues. I think with those two cases we should be well covered. >> Then there's all the interactions with other gem interfaces: >> - pwrite/pread/set_tiling: Should probably all fail with -EINVAL or >>something like that. > > > Ok. > > >> - dma-buf export/gem flink: should succeed. >> - But: dma-buf mapping for a foreign device should fail. This will be a >>pain to test on Android since we don't have anything else really. I can >>take that and do a test like the pile of prime_nv tests we have. > > > Flink is there in forking_evictions. > > Dmabuf is something I know nothing at the moment so I'll have to look into > it. dma-buf has two use-case: - Improved buffer sharing (better security) than flink, used by wayland and dri3. - Sharing backing storage across devices (similar to what ION does on Android - actually ION is nowadays based on dma-buf internally). We might want allow the 1. usecase with userptr I think, but we also need to reject the 2. one. But since this is a bit tricky it might be easier to outright reject all dma-buf exporting of userptrs for now. See the prime_self_import testcase, the actual exporting (i.e. where we'd need to insert a return -EINVAL) in the kernel happens in i915_gem_prime_export >> - gtt mmaps: Theoretically works, but dunno whether it makes sense. > > > According to Chris not on all architectures, I have to find relevant > documentation for that. I think we can just reject gtt mmaps for now then. Tbh I don't think it makes too much sense as a use-case really anyway. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests/gem_userptr_blits: Expanded userptr test cases
Hi, On 01/29/2014 08:11 PM, Daniel Vetter wrote: On Wed, Jan 29, 2014 at 01:30:54PM +, Tvrtko Ursulin wrote: From: Tvrtko Ursulin A set of userptr test cases to support the new feature. [snip] Minor thing on patch style: I'd split this up into 3 parts: - Extraction of the helpers - always useful to shine a bit light onto new helper stuff so other people also notice them. - The new testcase. - Removal of the old vmap testcase. I was afraid someone will say that, but was hoping for a lower bar since, to quote what yourself say later in this mail, "is a bit evil, but this is a testuite ;-)". :) Okay, I'll split it up. The other patch style thing are the helpers - the forking_eviction stuff doesn't really sound like a bit of helper code. igt_exchange_uint32_t certainly is, but the other stuff I'd just put into a common source file which is included by both tests. Yeah #include "common.c" is a bit evil, but this is a testsuite ;-) Or just copy&paste the code into the userptr test, which is the approach I'd have done. It would be too much c&p for my liking so I chose this route. Now for test coverage it sounds like this testcases here has been more than good enough to shake down the userptr implementation, so I think we're covered. But there is also basic interface coverage for sanity-checking and defending against evil userspace. For this case here I think we need: - Tests with un-aligned ptr/size. - Tests with invalid flags. Above are already there as test_usage_restrictions and test_input_checking. - Basic nastiness of handing in an invalid pointer. You mean trying to map something which doesn't exist in user address space? Any idea how to obtain such a pointer? Or just use zero? Then there's all the interactions with other gem interfaces: - pwrite/pread/set_tiling: Should probably all fail with -EINVAL or something like that. Ok. - dma-buf export/gem flink: should succeed. - But: dma-buf mapping for a foreign device should fail. This will be a pain to test on Android since we don't have anything else really. I can take that and do a test like the pile of prime_nv tests we have. Flink is there in forking_evictions. Dmabuf is something I know nothing at the moment so I'll have to look into it. - gtt mmaps: Theoretically works, but dunno whether it makes sense. According to Chris not on all architectures, I have to find relevant documentation for that. - Anything esle I've forgotten? Don't know, but thanks for your comments! Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] intel: Merge i915_drm.h with cmd parser define
On Wed, Jan 29, 2014 at 02:26:12PM -0800, Volkin, Bradley D wrote: > On Wed, Jan 29, 2014 at 02:13:21PM -0800, Chris Wilson wrote: > > On Wed, Jan 29, 2014 at 01:57:28PM -0800, bradley.d.vol...@intel.com wrote: > > > From: Brad Volkin > > > > > > Signed-off-by: Brad Volkin > > > --- > > > include/drm/i915_drm.h | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h > > > index 2f4eb8c..ba863c4 100644 > > > --- a/include/drm/i915_drm.h > > > +++ b/include/drm/i915_drm.h > > > @@ -27,7 +27,7 @@ > > > #ifndef _I915_DRM_H_ > > > #define _I915_DRM_H_ > > > > > > -#include > > > +#include > > > > Something about this patch smells very fishy > > Yeah, I wasn't completely sure about this one. I followed what I thought was > the procedure for updating the header (i.e. make headers_install in kernel, > copy to libdrm) and this is what I got. I guess either works, so maybe just add a note to the commit message about the little change. Imo it's better to have a 1:1 copy of the header generated by the kernel. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/13] drm/i915: Add a CMD_PARSER_VERSION getparam
On Wed, Jan 29, 2014 at 01:55:13PM -0800, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> So userspace can query the kernel for command parser support.
>
> OTC-Tracker: AXIA-4631
> Change-Id: I58af650db9f6753c2dcac9c54ab432fd31db302f
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_dma.c | 4
> include/uapi/drm/i915_drm.h | 1 +
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 258b1be..34ba199 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1013,6 +1013,10 @@ static int i915_getparam(struct drm_device *dev, void
> *data,
> case I915_PARAM_HAS_EXEC_HANDLE_LUT:
> value = 1;
> break;
> + case I915_PARAM_CMD_PARSER_VERSION:
> + /* TODO: version info (e.g. what is allowed?) */
> + value = 1;
I think an increasing integer without any mean special grouping (like
major/minor) is good enough. What we need though is a small revision log
with one-line blurbs that explain what has been added, e.g.
1: Initial version.
2: Allow streamout related registers
3: Add gen8 support
...
I think it would be good to have this as a comment right next to the
parser code itself, so what about adding a i915_cmd_parser_get_version
function to i915_cmd_parser.c who's only job is to return and integer and
contain this comment?
-Daniel
> + break;
> default:
> DRM_DEBUG("Unknown parameter %d\n", param->param);
> return -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 126bfaa..8a3e4ef00 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -337,6 +337,7 @@ typedef struct drm_i915_irq_wait {
> #define I915_PARAM_HAS_EXEC_NO_RELOC 25
> #define I915_PARAM_HAS_EXEC_HANDLE_LUT 26
> #define I915_PARAM_HAS_WT 27
> +#define I915_PARAM_CMD_PARSER_VERSION 28
>
> typedef struct drm_i915_getparam {
> int param;
> --
> 1.8.5.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Thu, Jan 30, 2014 at 10:05:28AM +0100, Daniel Vetter wrote:
> On Thu, Jan 30, 2014 at 09:53:28AM +0100, Daniel Vetter wrote:
> > On Wed, Jan 29, 2014 at 10:28:36PM +, Chris Wilson wrote:
> > > On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com
> > > wrote:
> > > > +/*
> > > > + * Returns a pointer to a descriptor for the command specified by
> > > > cmd_header.
> > > > + *
> > > > + * The caller must supply space for a default descriptor via the
> > > > default_desc
> > > > + * parameter. If no descriptor for the specified command exists in the
> > > > ring's
> > > > + * command parser tables, this function fills in default_desc based on
> > > > the
> > > > + * ring's default length encoding and returns default_desc.
> > > > + */
> > > > +static const struct drm_i915_cmd_descriptor*
> > > > +find_cmd(struct intel_ring_buffer *ring,
> > > > +u32 cmd_header,
> > > > +struct drm_i915_cmd_descriptor *default_desc)
> > > > +{
> > > > + u32 mask;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ring->cmd_table_count; i++) {
> > > > + const struct drm_i915_cmd_descriptor *desc;
> > > > +
> > > > + desc = find_cmd_in_table(&ring->cmd_tables[i],
> > > > cmd_header);
> > > > + if (desc)
> > > > + return desc;
> > > > + }
> > > > +
> > > > + mask = ring->get_cmd_length_mask(cmd_header);
> > > > + if (!mask)
> > > > + return NULL;
> > > > +
> > > > + BUG_ON(!default_desc);
> > > > + default_desc->flags = CMD_DESC_SKIP;
> > > > + default_desc->length.mask = mask;
> > >
> > > If we turn off all hw validation (through use of the secure bit) should
> > > we not default to a whitelist of commands? Otherwise it just seems to be
> > > a case of running a fuzzer until we kill the machine.
> >
> > Preventing hangs and dos is imo not the attack model, gpus are too fickle
> > for that. The attach model here is to prevent priveledge escalation and
> > information leaks. I.e. we want just containement of all read/write access
> > to the gtt space.
> >
> > I think for that purpose an explicit whitelist of commands which target
> > things outside of the (pp)gtt is sufficient. radeon's checker design is
> > completely different, but pretty much the only command they have is
> > to load register values. Intel gpus otoh have a big set of special-purpose
> > commands to load (most) of the rendering pipeline state. So we have
> > hw built-in register whitelists for all that stuff since you just can't
> > load arbitrary registers and state with those commands.
> >
> > Also note that for raw register access Bradley's scanner _is_ whitelist
> > based. And for general reads/writes gpu designers confirmed that those are
> > all MI_ commands (with very few specific exceptions like PIPE_CONTROL), so
> > as long as we check for the exceptions and otherwise only whitelist MI_
> > commands we know about we should be covered.
> >
> > So I think this is sound.
>
> Hm, but while scrolling through the checker I haven't spotted a "reject
> everything unknown" for MI_CLIENT commands. Bradley, have I missed that?
>
> I think submitting an invented MI_CLIENT command would also be a good
> testcase.
One more: I think it would be good to have an overview comment at the top
of i915_cmd_parser.c which details the security attack model and the
overall blacklist/whitelist design of the checker. We don't (yet) have
autogenerated documentation for i915, but that's something I'm working on.
And the kerneldoc system can also pull in multi-paragraph overview
comments besides the usual api documentation, so it's good to have things
ready.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com wrote: > From: Brad Volkin > > The command parser scans batch buffers submitted via execbuffer ioctls before > the driver submits them to hardware. At a high level, it looks for several > things: > > 1) Commands which are explicitly defined as privileged or which should only be >used by the kernel driver. The parser generally rejects such commands, with >the provision that it may allow some from the drm master process. > 2) Commands which access registers. To support correct/enhanced userspace >functionality, particularly certain OpenGL extensions, the parser provides > a >whitelist of registers which userspace may safely access (for both normal > and >drm master processes). > 3) Commands which access privileged memory (i.e. GGTT, HWS page, etc). The >parser always rejects such commands. > > Each ring maintains tables of commands and registers which the parser uses in > scanning batch buffers submitted to that ring. > > The set of commands that the parser must check for is significantly smaller > than the number of commands supported, especially on the render ring. As such, > the parser tables (built up in subsequent patches) contain only those commands > required by the parser. This generally works because command opcode ranges > have > standard command length encodings. So for commands that the parser does not > need > to check, it can easily skip them. This is implementated via a per-ring length > decoding vfunc. > > Unfortunately, there are a number of commands that do not follow the standard > length encoding for their opcode range, primarily amongst the MI_* commands. > To > handle this, the parser provides a way to define explicit "skip" entries in > the > per-ring command tables. > > Other command table entries will map fairly directly to high level categories > mentioned above: rejected, master-only, register whitelist. A number of > checks, > including the privileged memory checks, are implemented via a general > bitmasking > mechanism. > > OTC-Tracker: AXIA-4631 > Change-Id: I50b98c71c6655893291c78a2d1b8954577b37a30 > Signed-off-by: Brad Volkin > +#include "i915_drv.h" > + > +#define CLIENT_MASK 0xE000 > +#define SUBCLIENT_MASK 0x1800 > +#define MI_CLIENT0x > +#define RC_CLIENT0x6000 > +#define BC_CLIENT0x4000 > +#define MEDIA_SUBCLIENT 0x1000 I think these would fit neatly right next to all the other MI_* #defines in i915_reg.h. The other idea that just crossed my mind is to extract all the command #defines into a new i915_cmd.h (included by i915_drv.h by default) since i915_reg.h is already giant. But that should be done as a follow-up patch to avoid patch shuffling hell. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Thu, Jan 30, 2014 at 09:53:28AM +0100, Daniel Vetter wrote:
> On Wed, Jan 29, 2014 at 10:28:36PM +, Chris Wilson wrote:
> > On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com wrote:
> > > +/*
> > > + * Returns a pointer to a descriptor for the command specified by
> > > cmd_header.
> > > + *
> > > + * The caller must supply space for a default descriptor via the
> > > default_desc
> > > + * parameter. If no descriptor for the specified command exists in the
> > > ring's
> > > + * command parser tables, this function fills in default_desc based on
> > > the
> > > + * ring's default length encoding and returns default_desc.
> > > + */
> > > +static const struct drm_i915_cmd_descriptor*
> > > +find_cmd(struct intel_ring_buffer *ring,
> > > + u32 cmd_header,
> > > + struct drm_i915_cmd_descriptor *default_desc)
> > > +{
> > > + u32 mask;
> > > + int i;
> > > +
> > > + for (i = 0; i < ring->cmd_table_count; i++) {
> > > + const struct drm_i915_cmd_descriptor *desc;
> > > +
> > > + desc = find_cmd_in_table(&ring->cmd_tables[i], cmd_header);
> > > + if (desc)
> > > + return desc;
> > > + }
> > > +
> > > + mask = ring->get_cmd_length_mask(cmd_header);
> > > + if (!mask)
> > > + return NULL;
> > > +
> > > + BUG_ON(!default_desc);
> > > + default_desc->flags = CMD_DESC_SKIP;
> > > + default_desc->length.mask = mask;
> >
> > If we turn off all hw validation (through use of the secure bit) should
> > we not default to a whitelist of commands? Otherwise it just seems to be
> > a case of running a fuzzer until we kill the machine.
>
> Preventing hangs and dos is imo not the attack model, gpus are too fickle
> for that. The attach model here is to prevent priveledge escalation and
> information leaks. I.e. we want just containement of all read/write access
> to the gtt space.
>
> I think for that purpose an explicit whitelist of commands which target
> things outside of the (pp)gtt is sufficient. radeon's checker design is
> completely different, but pretty much the only command they have is
> to load register values. Intel gpus otoh have a big set of special-purpose
> commands to load (most) of the rendering pipeline state. So we have
> hw built-in register whitelists for all that stuff since you just can't
> load arbitrary registers and state with those commands.
>
> Also note that for raw register access Bradley's scanner _is_ whitelist
> based. And for general reads/writes gpu designers confirmed that those are
> all MI_ commands (with very few specific exceptions like PIPE_CONTROL), so
> as long as we check for the exceptions and otherwise only whitelist MI_
> commands we know about we should be covered.
>
> So I think this is sound.
Hm, but while scrolling through the checker I haven't spotted a "reject
everything unknown" for MI_CLIENT commands. Bradley, have I missed that?
I think submitting an invented MI_CLIENT command would also be a good
testcase.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/13] drm/i915: Add register whitelist for DRM master
On Wed, Jan 29, 2014 at 03:18:21PM -0800, Volkin, Bradley D wrote:
> On Wed, Jan 29, 2014 at 02:37:25PM -0800, Chris Wilson wrote:
> > On Wed, Jan 29, 2014 at 01:55:08PM -0800, bradley.d.vol...@intel.com wrote:
> > > From: Brad Volkin
> > >
> > > These are used to implement scanline waits in the X server.
> > >
> > > Signed-off-by: Brad Volkin
> > > ---
> > > drivers/gpu/drm/i915/i915_cmd_parser.c | 30
> > > ++
> > > 1 file changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > index 18d5b05..296e322 100644
> > > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > > @@ -234,6 +234,20 @@ static const u32 gen7_blt_regs[] = {
> > > BCS_SWCTRL,
> > > };
> > >
> > > +/* Whitelists for the DRM master. Magic numbers are taken from sna, to
> > > match. */
> >
> > It would be wiser to use the kernel defines, makes it look like we are
> > actually in charge. ;-)
>
> Will fix, though based on the sna commit history, it looks like you're in
> charge
> either way :)
Yeah, for the register tables I think we really should use symbolic values
consistently, adding new ones if i915_reg.h has them lacking. At least as
long as the lists are this short.
Aside: The bkm for getting big feature work which adds lots of register
#defines like this is to split out patches with just the #defines. That
way those can be reviewed independently from any discussions about the
code itself and so merged early. Helps with rebasing pains ;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Wed, Jan 29, 2014 at 10:28:36PM +, Chris Wilson wrote:
> On Wed, Jan 29, 2014 at 01:55:03PM -0800, bradley.d.vol...@intel.com wrote:
> > +/*
> > + * Returns a pointer to a descriptor for the command specified by
> > cmd_header.
> > + *
> > + * The caller must supply space for a default descriptor via the
> > default_desc
> > + * parameter. If no descriptor for the specified command exists in the
> > ring's
> > + * command parser tables, this function fills in default_desc based on the
> > + * ring's default length encoding and returns default_desc.
> > + */
> > +static const struct drm_i915_cmd_descriptor*
> > +find_cmd(struct intel_ring_buffer *ring,
> > +u32 cmd_header,
> > +struct drm_i915_cmd_descriptor *default_desc)
> > +{
> > + u32 mask;
> > + int i;
> > +
> > + for (i = 0; i < ring->cmd_table_count; i++) {
> > + const struct drm_i915_cmd_descriptor *desc;
> > +
> > + desc = find_cmd_in_table(&ring->cmd_tables[i], cmd_header);
> > + if (desc)
> > + return desc;
> > + }
> > +
> > + mask = ring->get_cmd_length_mask(cmd_header);
> > + if (!mask)
> > + return NULL;
> > +
> > + BUG_ON(!default_desc);
> > + default_desc->flags = CMD_DESC_SKIP;
> > + default_desc->length.mask = mask;
>
> If we turn off all hw validation (through use of the secure bit) should
> we not default to a whitelist of commands? Otherwise it just seems to be
> a case of running a fuzzer until we kill the machine.
Preventing hangs and dos is imo not the attack model, gpus are too fickle
for that. The attach model here is to prevent priveledge escalation and
information leaks. I.e. we want just containement of all read/write access
to the gtt space.
I think for that purpose an explicit whitelist of commands which target
things outside of the (pp)gtt is sufficient. radeon's checker design is
completely different, but pretty much the only command they have is
to load register values. Intel gpus otoh have a big set of special-purpose
commands to load (most) of the rendering pipeline state. So we have
hw built-in register whitelists for all that stuff since you just can't
load arbitrary registers and state with those commands.
Also note that for raw register access Bradley's scanner _is_ whitelist
based. And for general reads/writes gpu designers confirmed that those are
all MI_ commands (with very few specific exceptions like PIPE_CONTROL), so
as long as we check for the exceptions and otherwise only whitelist MI_
commands we know about we should be covered.
So I think this is sound.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/13] drm/i915: Refactor shmem pread setup
On Wed, Jan 29, 2014 at 01:55:02PM -0800, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> The command parser is going to need the same synchronization and
> setup logic, so factor it out for reuse.
>
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 +++
> drivers/gpu/drm/i915/i915_gem.c | 48
> +
> 2 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3673ba1..bfb30df 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2045,6 +2045,9 @@ void i915_gem_release_all_mmaps(struct drm_i915_private
> *dev_priv);
> void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
> void i915_gem_lastclose(struct drm_device *dev);
>
> +int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
> + int *needs_clflush);
> +
> int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
> static inline struct page *i915_gem_object_get_page(struct
> drm_i915_gem_object *obj, int n)
> {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 39770f7..fdc1f40 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -332,6 +332,39 @@ __copy_from_user_swizzled(char *gpu_vaddr, int
> gpu_offset,
> return 0;
> }
>
> +/*
> + * Pins the specified object's pages and synchronizes the object with
> + * GPU accesses. Sets needs_clflush to non-zero if the caller should
> + * flush the object from the CPU cache.
> + */
> +int i915_gem_obj_prepare_shmem_read(struct drm_i915_gem_object *obj,
> + int *needs_clflush)
> +{
> + int ret;
I think for safety reasons (userspace can blow up the kernel otherwise
with the execbuf cmd parser) we need to replicate the "is this really
shmem backed" check from i915_gem_pread_ioctl to this place here.
-Daniel
> +
> + *needs_clflush = 0;
> +
> + if (!(obj->base.read_domains & I915_GEM_DOMAIN_CPU)) {
> + /* If we're not in the cpu read domain, set ourself into the gtt
> + * read domain and manually flush cachelines (if required). This
> + * optimizes for the case when the gpu will dirty the data
> + * anyway again before the next pread happens. */
> + *needs_clflush = !cpu_cache_is_coherent(obj->base.dev,
> + obj->cache_level);
> + ret = i915_gem_object_wait_rendering(obj, true);
> + if (ret)
> + return ret;
> + }
> +
> + ret = i915_gem_object_get_pages(obj);
> + if (ret)
> + return ret;
> +
> + i915_gem_object_pin_pages(obj);
> +
> + return ret;
> +}
> +
> /* Per-page copy function for the shmem pread fastpath.
> * Flushes invalid cachelines before reading the target if
> * needs_clflush is set. */
> @@ -429,23 +462,10 @@ i915_gem_shmem_pread(struct drm_device *dev,
>
> obj_do_bit17_swizzling = i915_gem_object_needs_bit17_swizzle(obj);
>
> - if (!(obj->base.read_domains & I915_GEM_DOMAIN_CPU)) {
> - /* If we're not in the cpu read domain, set ourself into the gtt
> - * read domain and manually flush cachelines (if required). This
> - * optimizes for the case when the gpu will dirty the data
> - * anyway again before the next pread happens. */
> - needs_clflush = !cpu_cache_is_coherent(dev, obj->cache_level);
> - ret = i915_gem_object_wait_rendering(obj, true);
> - if (ret)
> - return ret;
> - }
> -
> - ret = i915_gem_object_get_pages(obj);
> + ret = i915_gem_obj_prepare_shmem_read(obj, &needs_clflush);
> if (ret)
> return ret;
>
> - i915_gem_object_pin_pages(obj);
> -
> offset = args->offset;
>
> for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents,
> --
> 1.8.5.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/6] drm/i915: Add some more registers to error state
Chris:
Do we also want to capture?
GAC_ECO_BITS /* gen6,7 */
GAM_ECOCHK /* gen6,7 */
GAB_CTL /* gen6 */
GFX_MODE /* gen6 */
Requested-by: Chris Wilson
Signed-off-by: Ben Widawsky
Reviewed-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_drv.h | 4
drivers/gpu/drm/i915/i915_gpu_error.c | 11 ++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d20fc80..e41f30a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -307,6 +307,10 @@ struct drm_i915_error_state {
u32 error; /* gen6+ */
u32 err_int; /* gen7 */
u32 done_reg;
+ u32 gac_eco;
+ u32 gam_ecochk;
+ u32 gab_ctl;
+ u32 gfx_mode;
u32 extra_instdone[I915_NUM_INSTDONE_REG];
u32 pipestat[I915_MAX_PIPES];
u64 fence[I915_MAX_NUM_FENCES];
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 07433bc..4c3ca11 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1035,8 +1035,11 @@ static void i915_capture_reg_state(struct
drm_i915_private *dev_priv,
if (IS_GEN7(dev))
error->err_int = I915_READ(GEN7_ERR_INT);
- if (IS_GEN6(dev))
+ if (IS_GEN6(dev)) {
error->forcewake = I915_READ(FORCEWAKE);
+ error->gab_ctl = I915_READ(GAB_CTL);
+ error->gfx_mode = I915_READ(GFX_MODE);
+ }
if (IS_GEN2(dev))
error->ier = I915_READ16(IER);
@@ -1052,6 +1055,12 @@ static void i915_capture_reg_state(struct
drm_i915_private *dev_priv,
}
/* 3: Feature specific registers */
+ if (IS_GEN6(dev) || IS_GEN7(dev)) {
+ error->gam_ecochk = I915_READ(GAM_ECOCHK);
+ error->gac_eco = I915_READ(GAC_ECO_BITS);
+ }
+
+ /* 4: Everything else */
if (HAS_HW_CONTEXTS(dev))
error->ccid = I915_READ(CCID);
--
1.8.5.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/6] drm/i915: Move per ring error state to ring_error
v2: Moved num_requests up (Chris)
Rebased on new hws page capture which required a rename since it made
two members named, 'hws' in the per ring error state. (Ben)
Signed-off-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_drv.h | 65
drivers/gpu/drm/i915/i915_gpu_error.c | 143 +-
2 files changed, 104 insertions(+), 104 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cd97c86..d20fc80 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -313,49 +313,50 @@ struct drm_i915_error_state {
struct intel_overlay_error_state *overlay;
struct intel_display_error_state *display;
- /* Per ring register state
-* TODO: Move these to per ring */
- u32 tail[I915_NUM_RINGS];
- u32 head[I915_NUM_RINGS];
- u32 ctl[I915_NUM_RINGS];
- u32 hws[I915_NUM_RINGS];
- u32 ipeir[I915_NUM_RINGS];
- u32 ipehr[I915_NUM_RINGS];
- u32 instdone[I915_NUM_RINGS];
- u32 acthd[I915_NUM_RINGS];
- u32 bbstate[I915_NUM_RINGS];
- u32 instpm[I915_NUM_RINGS];
- u32 instps[I915_NUM_RINGS];
- u32 seqno[I915_NUM_RINGS];
- u64 bbaddr[I915_NUM_RINGS];
- u32 fault_reg[I915_NUM_RINGS];
- u32 faddr[I915_NUM_RINGS];
- u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
- u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
-
- /* Software tracked state */
- bool waiting[I915_NUM_RINGS];
- int hangcheck_score[I915_NUM_RINGS];
- enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS];
-
- /* our own tracking of ring head and tail */
- u32 cpu_ring_head[I915_NUM_RINGS];
- u32 cpu_ring_tail[I915_NUM_RINGS];
- u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
-
struct drm_i915_error_ring {
bool valid;
+ /* Software tracked state */
+ bool waiting;
+ int hangcheck_score;
+ enum intel_ring_hangcheck_action hangcheck_action;
+ int num_requests;
+
+ /* our own tracking of ring head and tail */
+ u32 cpu_ring_head;
+ u32 cpu_ring_tail;
+
+ u32 semaphore_seqno[I915_NUM_RINGS - 1];
+
+ /* Register state */
+ u32 tail;
+ u32 head;
+ u32 ctl;
+ u32 hws;
+ u32 ipeir;
+ u32 ipehr;
+ u32 instdone;
+ u32 acthd;
+ u32 bbstate;
+ u32 instpm;
+ u32 instps;
+ u32 seqno;
+ u64 bbaddr;
+ u32 fault_reg;
+ u32 faddr;
+ u32 rc_psmi; /* sleep state */
+ u32 semaphore_mboxes[I915_NUM_RINGS - 1];
+
struct drm_i915_error_object {
int page_count;
u32 gtt_offset;
u32 *pages[0];
- } *ringbuffer, *batchbuffer, *ctx, *hws;
+ } *ringbuffer, *batchbuffer, *ctx, *hws_page;
+
struct drm_i915_error_request {
long jiffies;
u32 seqno;
u32 tail;
} *requests;
- int num_requests;
} ring[I915_NUM_RINGS];
struct drm_i915_error_buffer {
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index c9d4a18..07433bc 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -235,51 +235,48 @@ static const char *hangcheck_action_to_str(enum
intel_ring_hangcheck_action a)
static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
struct drm_device *dev,
- struct drm_i915_error_state *error,
- unsigned ring)
+ struct drm_i915_error_ring *ring)
{
- BUG_ON(ring >= I915_NUM_RINGS); /* shut up confused gcc */
- if (!error->ring[ring].valid)
+ if (!ring->valid)
return;
- err_printf(m, "%s command stream:\n", ring_str(ring));
- err_printf(m, " HEAD: 0x%08x\n", error->head[ring]);
- err_printf(m, " TAIL: 0x%08x\n", error->tail[ring]);
- err_printf(m, " CTL: 0x%08x\n", error->ctl[ring]);
- err_printf(m, " HWS: 0x%08x\n", error->hws[ring]);
- err_printf(m, " ACTHD: 0x%08x\n", error->acthd[ring]);
- err_printf(m, " IPEIR: 0x%08x\n", error->ipeir[ring]);
- err_printf(m, " IPEHR: 0x%08x\n", error->ipehr[ring]);
- err_printf(m, " INSTDONE: 0x%08x\n", error->instdone[ring]);
+ err_printf(m, " HEAD: 0x%08x\n", ring->head);
+ err_printf(m, " TAIL: 0x%08x\n", ring->tail);
+ err_printf(m, " CTL: 0x%08x\n", ring->ctl);
+
[Intel-gfx] [PATCH 3/6] drm/i915: Reorder struct members
This helps make an upcoming patch a bit more reviewable
Signed-off-by: Ben Widawsky
Reviewed-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_drv.h | 43 -
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3782b36..cd97c86 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -295,14 +295,26 @@ struct intel_display_error_state;
struct drm_i915_error_state {
struct kref ref;
+ struct timeval time;
+
+ /* Generic register state */
u32 eir;
u32 pgtbl_er;
u32 ier;
u32 ccid;
u32 derrmr;
u32 forcewake;
- bool waiting[I915_NUM_RINGS];
+ u32 error; /* gen6+ */
+ u32 err_int; /* gen7 */
+ u32 done_reg;
+ u32 extra_instdone[I915_NUM_INSTDONE_REG];
u32 pipestat[I915_MAX_PIPES];
+ u64 fence[I915_MAX_NUM_FENCES];
+ struct intel_overlay_error_state *overlay;
+ struct intel_display_error_state *display;
+
+ /* Per ring register state
+* TODO: Move these to per ring */
u32 tail[I915_NUM_RINGS];
u32 head[I915_NUM_RINGS];
u32 ctl[I915_NUM_RINGS];
@@ -311,25 +323,25 @@ struct drm_i915_error_state {
u32 ipehr[I915_NUM_RINGS];
u32 instdone[I915_NUM_RINGS];
u32 acthd[I915_NUM_RINGS];
- u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
- u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
- u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
- /* our own tracking of ring head and tail */
- u32 cpu_ring_head[I915_NUM_RINGS];
- u32 cpu_ring_tail[I915_NUM_RINGS];
- u32 error; /* gen6+ */
- u32 err_int; /* gen7 */
u32 bbstate[I915_NUM_RINGS];
u32 instpm[I915_NUM_RINGS];
u32 instps[I915_NUM_RINGS];
- u32 extra_instdone[I915_NUM_INSTDONE_REG];
u32 seqno[I915_NUM_RINGS];
u64 bbaddr[I915_NUM_RINGS];
u32 fault_reg[I915_NUM_RINGS];
- u32 done_reg;
u32 faddr[I915_NUM_RINGS];
- u64 fence[I915_MAX_NUM_FENCES];
- struct timeval time;
+ u32 rc_psmi[I915_NUM_RINGS]; /* sleep state */
+ u32 semaphore_mboxes[I915_NUM_RINGS][I915_NUM_RINGS - 1];
+
+ /* Software tracked state */
+ bool waiting[I915_NUM_RINGS];
+ int hangcheck_score[I915_NUM_RINGS];
+ enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS];
+
+ /* our own tracking of ring head and tail */
+ u32 cpu_ring_head[I915_NUM_RINGS];
+ u32 cpu_ring_tail[I915_NUM_RINGS];
+ u32 semaphore_seqno[I915_NUM_RINGS][I915_NUM_RINGS - 1];
struct drm_i915_error_ring {
bool valid;
@@ -363,11 +375,6 @@ struct drm_i915_error_state {
} **active_bo, **pinned_bo;
u32 *active_bo_count, *pinned_bo_count;
u32 vm_count;
-
- struct intel_overlay_error_state *overlay;
- struct intel_display_error_state *display;
- int hangcheck_score[I915_NUM_RINGS];
- enum intel_ring_hangcheck_action hangcheck_action[I915_NUM_RINGS];
};
struct intel_connector;
--
1.8.5.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/6] drm/i915: Capture PPGTT info on error capture
v2: Rebased upon cleaned up error state
v3: Make sure hangcheck info remains last (Chris)
Cc: Chris Wilson
Signed-off-by: Ben Widawsky
---
drivers/gpu/drm/i915/i915_drv.h | 9 +
drivers/gpu/drm/i915/i915_gpu_error.c | 37 +++
2 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e41f30a..3035bf3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -361,6 +361,14 @@ struct drm_i915_error_state {
u32 seqno;
u32 tail;
} *requests;
+
+ struct {
+ u32 gfx_mode;
+ union {
+ u64 pdp[4];
+ u32 pp_dir_base;
+ };
+ } vm_info;
} ring[I915_NUM_RINGS];
struct drm_i915_error_buffer {
@@ -378,6 +386,7 @@ struct drm_i915_error_state {
s32 ring:4;
u32 cache_level:3;
} **active_bo, **pinned_bo;
+
u32 *active_bo_count, *pinned_bo_count;
u32 vm_count;
};
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 4c3ca11..9d04e6a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -270,6 +270,19 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
ring->semaphore_seqno[2]);
}
}
+ if (USES_PPGTT(dev)) {
+ err_printf(m, " GFX_MODE: 0x%08x\n", ring->vm_info.gfx_mode);
+
+ if (INTEL_INFO(dev)->gen >= 8) {
+ int i;
+ for (i = 0; i < 4; i++)
+ err_printf(m, " PDP%d: 0x%016llx\n",
+ i, ring->vm_info.pdp[i]);
+ } else {
+ err_printf(m, " PP_DIR_BASE: 0x%08x\n",
+ ring->vm_info.pp_dir_base);
+ }
+ }
err_printf(m, " seqno: 0x%08x\n", ring->seqno);
err_printf(m, " waiting: %s\n", yesno(ring->waiting));
err_printf(m, " ring->head: 0x%08x\n", ring->cpu_ring_head);
@@ -847,6 +860,30 @@ static void i915_record_ring_state(struct drm_device *dev,
ering->hangcheck_score = ring->hangcheck.score;
ering->hangcheck_action = ring->hangcheck.action;
+
+ if (USES_PPGTT(dev)) {
+ int i;
+
+ ering->vm_info.gfx_mode = I915_READ(RING_MODE_GEN7(ring));
+
+ switch (INTEL_INFO(dev)->gen) {
+ case 8:
+ for (i = 0; i < 4; i++) {
+ ering->vm_info.pdp[i] =
+ I915_READ(GEN8_RING_PDP_UDW(ring, i));
+ ering->vm_info.pdp[i] <<= 32;
+ ering->vm_info.pdp[i] |=
+ I915_READ(GEN8_RING_PDP_LDW(ring, i));
+ }
+ break;
+ case 7:
+ ering->vm_info.pp_dir_base = RING_PP_DIR_BASE(ring);
+ break;
+ case 6:
+ ering->vm_info.pp_dir_base =
RING_PP_DIR_BASE_READ(ring);
+ break;
+ }
+ }
}
--
1.8.5.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/6] drm/i915: Extract register state error capture
The code has become quite hairy. By relocating all the generic registers
it will become more obvious where future ones should go. There is still
admittedly a bit of confusion left for things like per ring registers.
A subsequent patch will clean this function up.
Signed-off-by: Ben Widawsky
Reviewed-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gpu_error.c | 77 +++
1 file changed, 43 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 21cf0cf..67c82e5 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1012,43 +1012,13 @@ static void i915_gem_capture_buffers(struct
drm_i915_private *dev_priv,
}
}
-/**
- * i915_capture_error_state - capture an error record for later analysis
- * @dev: drm device
- *
- * Should be called when an error is detected (either a hang or an error
- * interrupt) to capture error state from the time of the error. Fills
- * out a structure which becomes available in debugfs for user level tools
- * to pick up.
- */
-void i915_capture_error_state(struct drm_device *dev)
+/* Capture all registers which don't fit into another category. */
+static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
+ struct drm_i915_error_state *error)
{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_i915_error_state *error;
- unsigned long flags;
+ struct drm_device *dev = dev_priv->dev;
int pipe;
- spin_lock_irqsave(&dev_priv->gpu_error.lock, flags);
- error = dev_priv->gpu_error.first_error;
- spin_unlock_irqrestore(&dev_priv->gpu_error.lock, flags);
- if (error)
- return;
-
- /* Account for pipe specific data like PIPE*STAT */
- error = kzalloc(sizeof(*error), GFP_ATOMIC);
- if (!error) {
- DRM_DEBUG_DRIVER("out of memory, not capturing error state\n");
- return;
- }
-
- DRM_INFO("GPU crash dump saved to /sys/class/drm/card%d/error\n",
-dev->primary->index);
- DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx
stack, including userspace.\n");
- DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org
against DRI -> DRM/Intel\n");
- DRM_INFO("drm/i915 developers can then reassign to the right component
if it's not a kernel issue.\n");
- DRM_INFO("The gpu crash dump is required to analyze gpu hangs, so
please always attach it.\n");
-
- kref_init(&error->ref);
error->eir = I915_READ(EIR);
error->pgtbl_er = I915_READ(PGTBL_ER);
if (HAS_HW_CONTEXTS(dev))
@@ -1086,7 +1056,46 @@ void i915_capture_error_state(struct drm_device *dev)
error->err_int = I915_READ(GEN7_ERR_INT);
i915_get_extra_instdone(dev, error->extra_instdone);
+}
+
+/**
+ * i915_capture_error_state - capture an error record for later analysis
+ * @dev: drm device
+ *
+ * Should be called when an error is detected (either a hang or an error
+ * interrupt) to capture error state from the time of the error. Fills
+ * out a structure which becomes available in debugfs for user level tools
+ * to pick up.
+ */
+void i915_capture_error_state(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_error_state *error;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dev_priv->gpu_error.lock, flags);
+ error = dev_priv->gpu_error.first_error;
+ spin_unlock_irqrestore(&dev_priv->gpu_error.lock, flags);
+ if (error)
+ return;
+
+ /* Account for pipe specific data like PIPE*STAT */
+ error = kzalloc(sizeof(*error), GFP_ATOMIC);
+ if (!error) {
+ DRM_DEBUG_DRIVER("out of memory, not capturing error state\n");
+ return;
+ }
+
+ DRM_INFO("GPU crash dump saved to /sys/class/drm/card%d/error\n",
+dev->primary->index);
+ DRM_INFO("GPU hangs can indicate a bug anywhere in the entire gfx
stack, including userspace.\n");
+ DRM_INFO("Please file a _new_ bug report on bugs.freedesktop.org
against DRI -> DRM/Intel\n");
+ DRM_INFO("drm/i915 developers can then reassign to the right component
if it's not a kernel issue.\n");
+ DRM_INFO("The gpu crash dump is required to analyze gpu hangs, so
please always attach it.\n");
+
+ kref_init(&error->ref);
+ i915_capture_reg_state(dev_priv, error);
i915_gem_capture_buffers(dev_priv, error);
i915_gem_record_fences(dev, error);
i915_gem_record_rings(dev, error);
--
1.8.5.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/6] drm/i915: Logically reorder error register capture
Create logical sections in an attempt to clean up, and continue to keep
future additions clean.
v2: Reworded the comments. Added section headers (Chris)
Signed-off-by: Ben Widawsky
Reviewed-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_gpu_error.c | 59 +--
1 file changed, 36 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 67c82e5..c9d4a18 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1019,41 +1019,54 @@ static void i915_capture_reg_state(struct
drm_i915_private *dev_priv,
struct drm_device *dev = dev_priv->dev;
int pipe;
- error->eir = I915_READ(EIR);
- error->pgtbl_er = I915_READ(PGTBL_ER);
- if (HAS_HW_CONTEXTS(dev))
- error->ccid = I915_READ(CCID);
+ /* General organization
+* 1. Registers specific to a single generation
+* 2. Registers which belong to multiple generations
+* 3. Feature specific registers.
+* 4. Everything else
+* Please try to follow the order.
+*/
- if (HAS_PCH_SPLIT(dev))
- error->ier = I915_READ(DEIER) | I915_READ(GTIER);
- else if (IS_VALLEYVIEW(dev))
+ /* 1: Registers specific to a single generation */
+ if (IS_VALLEYVIEW(dev)) {
error->ier = I915_READ(GTIER) | I915_READ(VLV_IER);
- else if (IS_GEN2(dev))
- error->ier = I915_READ16(IER);
- else
- error->ier = I915_READ(IER);
+ error->forcewake = I915_READ(FORCEWAKE_VLV);
+ }
- if (INTEL_INFO(dev)->gen >= 6)
- error->derrmr = I915_READ(DERRMR);
+ if (IS_GEN7(dev))
+ error->err_int = I915_READ(GEN7_ERR_INT);
- if (IS_VALLEYVIEW(dev))
- error->forcewake = I915_READ(FORCEWAKE_VLV);
- else if (INTEL_INFO(dev)->gen >= 7)
- error->forcewake = I915_READ(FORCEWAKE_MT);
- else if (INTEL_INFO(dev)->gen == 6)
+ if (IS_GEN6(dev))
error->forcewake = I915_READ(FORCEWAKE);
- if (!HAS_PCH_SPLIT(dev))
- for_each_pipe(pipe)
- error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
+ if (IS_GEN2(dev))
+ error->ier = I915_READ16(IER);
+
+ /* 2: Registers which belong to multiple generations */
+ if (INTEL_INFO(dev)->gen >= 7)
+ error->forcewake = I915_READ(FORCEWAKE_MT);
if (INTEL_INFO(dev)->gen >= 6) {
+ error->derrmr = I915_READ(DERRMR);
error->error = I915_READ(ERROR_GEN6);
error->done_reg = I915_READ(DONE_REG);
}
- if (INTEL_INFO(dev)->gen == 7)
- error->err_int = I915_READ(GEN7_ERR_INT);
+ /* 3: Feature specific registers */
+ if (HAS_HW_CONTEXTS(dev))
+ error->ccid = I915_READ(CCID);
+
+ if (HAS_PCH_SPLIT(dev))
+ error->ier = I915_READ(DEIER) | I915_READ(GTIER);
+ else {
+ error->ier = I915_READ(IER);
+ for_each_pipe(pipe)
+ error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
+ }
+
+ /* 4: Everything else */
+ error->eir = I915_READ(EIR);
+ error->pgtbl_er = I915_READ(PGTBL_ER);
i915_get_extra_instdone(dev, error->extra_instdone);
}
--
1.8.5.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
