Re: [Intel-gfx] [RFC 04/44] drm/i915: Fix null pointer dereference in error capture
On Mon, Jun 30, 2014 at 02:40:05PM -0700, Jesse Barnes wrote: On Thu, 26 Jun 2014 18:23:55 +0100 john.c.harri...@intel.com wrote: From: John Harrison john.c.harri...@intel.com The i915_gem_record_rings() code was unconditionally querying and saving state for the batch_obj of a request structure. This is not necessarily set. Thus a null pointer dereference can occur. --- drivers/gpu/drm/i915/i915_gpu_error.c | 13 +++-- 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index 87ec60e..0738f21 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -902,12 +902,13 @@ static void i915_gem_record_rings(struct drm_device *dev, * as the simplest method to avoid being overwritten * by userspace. */ - error-ring[i].batchbuffer = - i915_error_object_create(dev_priv, -request-batch_obj, -request-ctx ? -request-ctx-vm : -dev_priv-gtt.base); + if(request-batch_obj) + error-ring[i].batchbuffer = + i915_error_object_create(dev_priv, + request-batch_obj, +request-ctx ? + request-ctx-vm : + dev_priv-gtt.base); if (HAS_BROKEN_CS_TLB(dev_priv-dev) ring-scratch.obj) Reviewed-by: Jesse Barnes jbar...@virtuosugeek.org Nah, put the NULL check into the macro. i915_error_object_create() was originally written as a no-op on NULL pointers for cleanliness, we may as well do the check centrally and remove the extras we have grown. -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] drm/i915: Remove num_pages parameter to i915_error_object_create()
For cleanliness, i915_error_object_create() was written to handle the
NULL pointer in a central location. The macro that wrapped it and passed
it a num_pages to use, was not safe. As we now never limit the num_pages
to use (we did so at one point to only capture the first page of the
context), we can remove the redundant macro and be NULL safe again.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Jesse Barnes jbar...@virtuousgeek.org
Cc: John Harrison john.c.harri...@intel.com
---
drivers/gpu/drm/i915/i915_gpu_error.c | 25 ++---
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 394e283970a8..f1581a4af7a7 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -538,12 +538,12 @@ static void i915_error_state_free(struct kref *error_ref)
}
static struct drm_i915_error_object *
-i915_error_object_create_sized(struct drm_i915_private *dev_priv,
- struct drm_i915_gem_object *src,
- struct i915_address_space *vm,
- int num_pages)
+i915_error_object_create(struct drm_i915_private *dev_priv,
+struct drm_i915_gem_object *src,
+struct i915_address_space *vm)
{
struct drm_i915_error_object *dst;
+ int num_pages;
bool use_ggtt;
int i = 0;
u32 reloc_offset;
@@ -551,6 +551,8 @@ i915_error_object_create_sized(struct drm_i915_private
*dev_priv,
if (src == NULL || src-pages == NULL)
return NULL;
+ num_pages = src-base.size PAGE_SHIFT;
+
dst = kmalloc(sizeof(*dst) + num_pages * sizeof(u32 *), GFP_ATOMIC);
if (dst == NULL)
return NULL;
@@ -629,13 +631,8 @@ unwind:
kfree(dst);
return NULL;
}
-#define i915_error_object_create(dev_priv, src, vm) \
- i915_error_object_create_sized((dev_priv), (src), (vm), \
- (src)-base.sizePAGE_SHIFT)
-
#define i915_error_ggtt_object_create(dev_priv, src) \
- i915_error_object_create_sized((dev_priv), (src),
(dev_priv)-gtt.base, \
- (src)-base.sizePAGE_SHIFT)
+ i915_error_object_create((dev_priv), (src), (dev_priv)-gtt.base)
static void capture_bo(struct drm_i915_error_buffer *err,
struct i915_vma *vma)
@@ -932,8 +929,7 @@ static void i915_gem_record_rings(struct drm_device *dev,
request-ctx-vm :
dev_priv-gtt.base);
- if (HAS_BROKEN_CS_TLB(dev_priv-dev)
- ring-scratch.obj)
+ if (HAS_BROKEN_CS_TLB(dev_priv-dev))
error-ring[i].wa_batchbuffer =
i915_error_ggtt_object_create(dev_priv,
ring-scratch.obj);
@@ -955,9 +951,8 @@ static void i915_gem_record_rings(struct drm_device *dev,
error-ring[i].ringbuffer =
i915_error_ggtt_object_create(dev_priv,
ring-buffer-obj);
- if (ring-status_page.obj)
- error-ring[i].hws_page =
- i915_error_ggtt_object_create(dev_priv,
ring-status_page.obj);
+ error-ring[i].hws_page =
+ i915_error_ggtt_object_create(dev_priv,
ring-status_page.obj);
i915_gem_record_active_context(ring, error, error-ring[i]);
--
2.0.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Fix VCS2's ring name.
-Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Rodrigo Vivi Sent: Monday, June 30, 2014 5:51 PM To: intel-gfx@lists.freedesktop.org Cc: Vivi, Rodrigo Subject: [Intel-gfx] [PATCH 1/3] drm/i915: Fix VCS2's ring name. It just fix a typo. Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com --- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 2faef26..c3f96a1 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2224,7 +2224,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev) return -EINVAL; } - ring-name = bds2_ring; + ring-name = bsd2_ring; ring-id = VCS2; ring-write_tail = ring_write_tail; Jus nitpicking but, wouldn´t it be better without the underscore, like the other rings?: bsd2 ring ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm: i915: plane B assertion failure, should be off on pipe B but is still active
On Mon, 30 Jun 2014, Paul Bolle pebo...@tiscali.nl wrote: Kernels v3.16-rc2 and v3.16-rc3 trigger a new (for me) warning. (I never booted v3.16-rc1). Machine is a, rather outdated, ThinkPad X41 (ie, single core i686). WARNING: CPU: 0 PID: 221 at drivers/gpu/drm/i915/intel_display.c:1274 assert_planes_disabled+0xf9/0x100 [i915]() plane B assertion failure, should be off on pipe B but is still active Modules linked in: tg3 i915(+) i2c_algo_bit drm_kms_helper ptp drm ata_generic pata_acpi yenta_socket i2c_core pps_core video CPU: 0 PID: 221 Comm: systemd-udevd Not tainted 3.16.0-0.rc3.1.local0.fc20.i686 #1 Hardware name: IBM 2525FAG/2525FAG, BIOS 74ET64WW (2.09 ) 12/14/2006 c0c87907 add7c490 f652b9ac c09fdab7 f652b9ec f652b9dc c045008e f830c6cc f652ba0c 00dd f830c634 04fa f82b4d59 f82b4d59 f6728000 0001 f65a8c00 f652b9f8 c04500ee 0009 f652b9ec f830c6cc f652ba0c Call Trace: [c09fdab7] dump_stack+0x41/0x52 [c045008e] warn_slowpath_common+0x7e/0xa0 [f82b4d59] ? assert_planes_disabled+0xf9/0x100 [i915] [f82b4d59] ? assert_planes_disabled+0xf9/0x100 [i915] [c04500ee] warn_slowpath_fmt+0x3e/0x60 [f82b4d59] assert_planes_disabled+0xf9/0x100 [i915] [f82bd8d6] intel_disable_pipe+0x26/0xb0 [i915] [f82aec40] ? gen4_read8+0xc0/0xc0 [i915] [f82c25d3] i9xx_crtc_disable+0x93/0x3d0 [i915] [f82c91bc] intel_modeset_setup_hw_state+0x7ac/0xbc0 [i915] [f82aec40] ? gen4_read8+0xc0/0xc0 [i915] [f815bc7c] ? drm_modeset_lock_all_crtcs+0x3c/0x50 [drm] [f82c9d04] intel_modeset_init+0x734/0x1220 [i915] [f82a2edb] ? i915_enable_pipestat+0xab/0x120 [i915] [f82a34c4] ? i915_irq_postinstall+0x104/0x110 [i915] [f8146ba9] ? drm_irq_install+0xa9/0x170 [drm] [f82f4016] i915_driver_load+0xa76/0xe70 [i915] [f82f13d0] ? i915_switcheroo_set_state+0x90/0x90 [i915] [c06b5880] ? cleanup_uevent_env+0x10/0x10 [c05d8243] ? sysfs_add_file+0x23/0x30 [c07a4034] ? get_device+0x14/0x30 [c07a8e52] ? klist_class_dev_get+0x12/0x20 [c09f73ce] ? klist_node_init+0x2e/0x50 [c09f7487] ? klist_add_tail+0x27/0x30 [c07a5506] ? device_add+0x1d6/0x5a0 [f814ca8a] ? drm_sysfs_device_add+0xba/0x100 [drm] [f81496ee] drm_dev_register+0x8e/0xe0 [drm] [f814bca9] drm_get_pci_dev+0x79/0x1c0 [drm] [f8274415] i915_pci_probe+0x35/0x60 [i915] [c06f0baf] pci_device_probe+0x6f/0xc0 [c05d8625] ? sysfs_create_link+0x25/0x40 [c07a8093] driver_probe_device+0x93/0x3a0 [c05d8357] ? sysfs_create_dir_ns+0x37/0x80 [c06f0af1] ? pci_match_device+0xc1/0xe0 [c07a8451] __driver_attach+0x71/0x80 [c07a83e0] ? __device_attach+0x40/0x40 [c07a64c7] bus_for_each_dev+0x57/0xa0 [c07a7bbe] driver_attach+0x1e/0x20 [c07a83e0] ? __device_attach+0x40/0x40 [c07a7807] bus_add_driver+0x157/0x230 [f7fc8000] ? 0xf7fc7fff [f7fc8000] ? 0xf7fc7fff [c07a8b39] driver_register+0x59/0xe0 [c0564f56] ? __kmalloc_track_caller+0x46/0x1f0 [c06ef722] __pci_register_driver+0x32/0x40 [f814bed5] drm_pci_init+0xe5/0x110 [drm] [f7fc8000] ? 0xf7fc7fff [f7fc8088] i915_init+0x88/0x8a [i915] [f7fc8000] ? 0xf7fc7fff [c0400492] do_one_initcall+0xc2/0x1f0 [f7fc8000] ? 0xf7fc7fff [c05624dd] ? kfree+0xdd/0x120 [c05524ef] ? __vunmap+0x8f/0xe0 [c05524ef] ? __vunmap+0x8f/0xe0 [c05524ef] ? __vunmap+0x8f/0xe0 [c04bee92] load_module+0x1a92/0x23b0 [c04bbd69] ? copy_module_from_fd.isra.46+0x109/0x1a0 [c04bf94d] SyS_finit_module+0x8d/0xd0 [c0538f43] ? vm_mmap_pgoff+0x93/0xb0 [c0a045df] sysenter_do_call+0x12/0x16 Feel free to prod me for further details. This does not ring any bells to me (but that doesn't prove anything). A bisect result would be awesome. 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 1/2] intel-gpu-tools: pass argc/argv to simple main
On 27 June 2014 15:15, tim.g...@intel.com wrote:
From: Tim Gore tim.g...@intel.com
Quite a few single tests do not use the igt_simple_main
macro because they want access to argc/argv. So change the
igt_simple_main macro to pass these arguments through to the
__real_mainxxx function, and change these tests to use
the macro.
It might also be worth marking igt_simple_init as an internal function
as after this change it is only used in one of the internal framework
tests.
Signed-off-by: Tim Gore tim.g...@intel.com
---
lib/igt_core.h | 8
tests/gem_ctx_basic.c| 6 +-
tests/gem_exec_blt.c | 5 +
tests/gem_gtt_speed.c| 5 +
tests/gem_hang.c | 5 +
tests/gem_render_copy.c | 4 +---
tests/gem_render_linear_blits.c | 5 +
tests/gem_render_tiled_blits.c | 5 +
tests/gem_seqno_wrap.c | 11 ---
tests/gem_stress.c | 5 +
tests/gen3_mixed_blits.c | 5 +
tests/gen3_render_linear_blits.c | 5 +
tests/gen3_render_mixed_blits.c | 5 +
tests/gen3_render_tiledx_blits.c | 5 +
tests/gen3_render_tiledy_blits.c | 5 +
15 files changed, 21 insertions(+), 63 deletions(-)
diff --git a/lib/igt_core.h b/lib/igt_core.h
index e252eba..9853e6b 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -176,13 +176,13 @@ void igt_simple_init(void);
* the test needs to parse additional cmdline arguments of its own.
This documentation comment needs updating to mention that argc and
argv are available.
*/
#define igt_simple_main \
- static void igt_tokencat(__real_main, __LINE__)(void); \
+ static void igt_tokencat(__real_main, __LINE__)(int argc, char
**argv); \
int main(int argc, char **argv) { \
igt_simple_init(); \
- igt_tokencat(__real_main, __LINE__)(); \
- exit(0); \
+ igt_tokencat(__real_main, __LINE__)(argc, argv); \
+ igt_exit(); \
} \
- static void igt_tokencat(__real_main, __LINE__)(void) \
+ static void igt_tokencat(__real_main, __LINE__)(int argc, char
**argv) \
__attribute__((format(printf, 1, 2)))
void igt_skip(const char *f, ...) __attribute__((noreturn));
diff --git a/tests/gem_ctx_basic.c b/tests/gem_ctx_basic.c
index 3e9b688..fe770ea 100644
--- a/tests/gem_ctx_basic.c
+++ b/tests/gem_ctx_basic.c
@@ -145,12 +145,10 @@ static void parse(int argc, char *argv[])
}
}
-int main(int argc, char *argv[])
+igt_simple_main
{
int i;
- igt_simple_init();
-
fd = drm_open_any_render();
devid = intel_get_drm_devid(fd);
@@ -173,6 +171,4 @@ int main(int argc, char *argv[])
free(threads);
close(fd);
-
- return 0;
}
diff --git a/tests/gem_exec_blt.c b/tests/gem_exec_blt.c
index 3bcef18..3d092fe 100644
--- a/tests/gem_exec_blt.c
+++ b/tests/gem_exec_blt.c
@@ -253,12 +253,10 @@ static void run(int object_size)
close(fd);
}
-int main(int argc, char **argv)
+igt_simple_main
{
int i;
- igt_simple_init();
-
igt_skip_on_simulation();
if (argc 1) {
@@ -270,5 +268,4 @@ int main(int argc, char **argv)
} else
run(OBJECT_SIZE);
- return 0;
}
diff --git a/tests/gem_gtt_speed.c b/tests/gem_gtt_speed.c
index 385eeb7..fa20de0 100644
--- a/tests/gem_gtt_speed.c
+++ b/tests/gem_gtt_speed.c
@@ -50,7 +50,7 @@ static double elapsed(const struct timeval *start,
return (1e6*(end-tv_sec - start-tv_sec) + (end-tv_usec -
start-tv_usec))/loop;
}
-int main(int argc, char **argv)
+igt_simple_main
{
struct timeval start, end;
uint8_t *buf;
@@ -59,8 +59,6 @@ int main(int argc, char **argv)
int loop, i, tiling;
int fd;
- igt_simple_init();
-
igt_skip_on_simulation();
if (argc 1)
@@ -329,5 +327,4 @@ int main(int argc, char **argv)
gem_close(fd, handle);
close(fd);
- return 0;
There are a couple of other returns that need to be replaced in this
test and in a few others, since the function igt_simple_main creates
returns void.
}
diff --git a/tests/gem_hang.c b/tests/gem_hang.c
index 6248244..a4f4d10 100644
--- a/tests/gem_hang.c
+++ b/tests/gem_hang.c
@@ -68,12 +68,10 @@ gpu_hang(void)
intel_batchbuffer_flush(batch);
}
-int main(int argc, char **argv)
+igt_simple_main
{
int fd;
- igt_simple_init();
-
igt_assert_f(argc == 2,
usage: %s disabled pipe number\n,
argv[0]);
@@ -93,5 +91,4 @@ int main(int argc, char **argv)
close(fd);
- return 0;
}
diff --git a/tests/gem_render_copy.c b/tests/gem_render_copy.c
index fd26b43..12dd90d 100644
--- a/tests/gem_render_copy.c
+++ b/tests/gem_render_copy.c
@@ -117,7
Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: gmch: factor out intel_set_memory_cxsr
On Tue, 2014-07-01 at 09:18 +0530, Deepak S wrote:
On Friday 13 June 2014 05:24 PM, Imre Deak wrote:
This functionality will be also needed by an upcoming patch, so factor
it out. As a bonus this also makes things a bit more uniform across
platforms. Note that this also changes the register read-modify-write
to a simple write during disabling. This is what we do during enabling
anyway and according to the spec all the relevant bits are reserved-MBZ
or reserved with a 0 default value.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/intel_pm.c | 75
-
2 files changed, 39 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index f36d9eb..211a173 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2590,6 +2590,8 @@ extern void gen6_set_rps(struct drm_device *dev, u8
val);
extern void valleyview_set_rps(struct drm_device *dev, u8 val);
extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
+extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
+ bool enable);
extern void intel_detect_pch(struct drm_device *dev);
extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
extern int intel_enable_rc6(const struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 0b088fe..e55622e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -789,12 +789,33 @@ static const struct cxsr_latency
*intel_get_cxsr_latency(int is_desktop,
return NULL;
}
-static void pineview_disable_cxsr(struct drm_device *dev)
+void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
{
- struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_device *dev = dev_priv-dev;
+ u32 val;
- /* deactivate cxsr */
- I915_WRITE(DSPFW3, I915_READ(DSPFW3) ~PINEVIEW_SELF_REFRESH_EN);
+ if (IS_VALLEYVIEW(dev)) {
+ I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
+ } else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
+ I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
+ } else if (IS_PINEVIEW(dev)) {
+ val = I915_READ(DSPFW3) ~PINEVIEW_SELF_REFRESH_EN;
+ val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
+ I915_WRITE(DSPFW3, val);
+ } else if (IS_I945G(dev) || IS_I945GM(dev)) {
+ val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
+ _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
+ I915_WRITE(FW_BLC_SELF, val);
+ } else if (IS_I915GM(dev)) {
+ val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
+ _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
+ I915_WRITE(INSTPM, val);
+ } else {
+ return;
+ }
+
+ DRM_DEBUG_KMS(memory self-refresh is %s\n,
+ enable ? enabled : disabled);
}
/*
@@ -1033,7 +1054,7 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
dev_priv-fsb_freq,
dev_priv-mem_freq);
if (!latency) {
DRM_DEBUG_KMS(Unknown FSB/MEM found, disable CxSR\n);
- pineview_disable_cxsr(dev);
+ intel_set_memory_cxsr(dev_priv, false);
return;
}
@@ -1085,12 +1106,7 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
DRM_DEBUG_KMS(DSPFW3 register is %x\n, reg);
/* activate cxsr */
- I915_WRITE(DSPFW3,
- I915_READ(DSPFW3) | PINEVIEW_SELF_REFRESH_EN);
- DRM_DEBUG_KMS(Self-refresh is enabled\n);
- } else {
- pineview_disable_cxsr(dev);
- DRM_DEBUG_KMS(Self-refresh is disabled\n);
+ intel_set_memory_cxsr(dev_priv, true);
I think we need to pass false here to disable cxsr right?
Yes, in the else branch, which I left out for some reason.. Thanks a lot
for spotting it, I'll send an updated patch.
--Imre
Apart for this everything else looks good.
Reviewed-by: Deepak Sdeepa...@linux.intel.com
}
}
@@ -1342,10 +1358,9 @@ static void valleyview_update_wm(struct drm_crtc
*crtc)
valleyview_wm_info,
valleyview_cursor_wm_info,
ignore_plane_sr, cursor_sr)) {
- I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
+ intel_set_memory_cxsr(dev_priv, true);
} else {
- I915_WRITE(FW_BLC_SELF_VLV,
- I915_READ(FW_BLC_SELF_VLV) ~FW_CSPWRDWNEN);
+
Re: [Intel-gfx] [PATCH] drm/i915/opregion: ignore firmware requests for backlight change
Hi, I would like to inform that I use UEFI boot. I don't have a spare
machine/disk to test legacy unfortunately.
regards
Anton.
2014-06-27 7:20 GMT+04:00 Aaron Lu aaron...@intel.com:
On 06/25/2014 07:08 PM, Jani Nikula wrote:
On Tue, 24 Jun 2014, Aaron Lu aaron...@intel.com wrote:
Some Thinkpad laptops' firmware will initiate a backlight level change
request through operation region on the events of AC plug/unplug, but
since we are not using firmware's interface to do the backlight setting
on these affected laptops, we do not want the firmware to use some
arbitrary value from its ASL variable to set the backlight level on
AC plug/unplug either.
I'm curious whether this happens with EFI boot, or only with legacy.
Igor, Anton,
Are you using legacy boot or UEFI boot?
Possible to test the other case?
One comment inline, otherwise
Will add that in next revision.
Acked-by: Jani Nikula jani.nik...@intel.com
Thanks for the review!
-Aaron
for merging through the ACPI tree, as the change is more likely to
conflict there.
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=76491
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=77091
Reported-and-tested-by: Igor Gnatenko i.gnatenko.br...@gmail.com
Reported-and-tested-by: Anton Gubarkov anton.gubar...@gmail.com
Signed-off-by: Aaron Lu aaron...@intel.com
---
drivers/acpi/video.c | 3 ++-
drivers/gpu/drm/i915/intel_opregion.c | 7 +++
include/acpi/video.h | 2 ++
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index fb9ffe9adc64..cf99d6d2d491 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -241,13 +241,14 @@ static bool acpi_video_use_native_backlight(void)
return use_native_backlight_dmi;
}
-static bool acpi_video_verify_backlight_support(void)
+bool acpi_video_verify_backlight_support(void)
{
if (acpi_osi_is_win8() acpi_video_use_native_backlight()
backlight_device_registered(BACKLIGHT_RAW))
return false;
return acpi_video_backlight_support();
}
+EXPORT_SYMBOL(acpi_video_verify_backlight_support);
/* backlight device sysfs support */
static int acpi_video_get_brightness(struct backlight_device *bd)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c
b/drivers/gpu/drm/i915/intel_opregion.c
index 2e2c71fcc9ed..02943d93e88e 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -403,6 +403,13 @@ static u32 asle_set_backlight(struct drm_device
*dev, u32 bclp)
DRM_DEBUG_DRIVER(bclp = 0x%08x\n, bclp);
+/*
+ * If the acpi_video interface is not supposed to be used, don't
+ * bother processing backlight level change requests from firmware.
+ */
+if (!acpi_video_verify_backlight_support())
+return 0;
I'd appreciate a DRM_DEBUG_KMS here about what happened. We're bound to
wonder about that staring at some dmesg later on!
+
if (!(bclp ASLE_BCLP_VALID))
return ASLC_BACKLIGHT_FAILED;
diff --git a/include/acpi/video.h b/include/acpi/video.h
index ea4c7bbded4d..92f8c4bffefb 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -22,6 +22,7 @@ extern void acpi_video_unregister(void);
extern void acpi_video_unregister_backlight(void);
extern int acpi_video_get_edid(struct acpi_device *device, int type,
int device_id, void **edid);
+extern bool acpi_video_verify_backlight_support(void);
#else
static inline int acpi_video_register(void) { return 0; }
static inline void acpi_video_unregister(void) { return; }
@@ -31,6 +32,7 @@ static inline int acpi_video_get_edid(struct
acpi_device *device, int type,
{
return -ENODEV;
}
+static bool acpi_video_verify_backlight_support() { return false; }
#endif
#endif
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 1/3] drm/i915: gmch: factor out intel_set_memory_cxsr
This functionality will be also needed by an upcoming patch, so factor
it out. As a bonus this also makes things a bit more uniform across
platforms. Note that this also changes the register read-modify-write
to a simple write during disabling. This is what we do during enabling
anyway and according to the spec all the relevant bits are reserved-MBZ
or reserved with a 0 default value.
v2:
- unchanged
v3:
- fix missing cxsr disabling on pineview (Deepak)
Signed-off-by: Imre Deak imre.d...@intel.com
Reviewed-by: Deepak S deepa...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/intel_pm.c | 76 -
2 files changed, 40 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea596..c1e306e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2648,6 +2648,8 @@ extern void gen6_set_rps(struct drm_device *dev, u8 val);
extern void valleyview_set_rps(struct drm_device *dev, u8 val);
extern int valleyview_rps_max_freq(struct drm_i915_private *dev_priv);
extern int valleyview_rps_min_freq(struct drm_i915_private *dev_priv);
+extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
+ bool enable);
extern void intel_detect_pch(struct drm_device *dev);
extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
extern int intel_enable_rc6(const struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a90fdbd..ddc22c1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -792,12 +792,33 @@ static const struct cxsr_latency
*intel_get_cxsr_latency(int is_desktop,
return NULL;
}
-static void pineview_disable_cxsr(struct drm_device *dev)
+void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
{
- struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_device *dev = dev_priv-dev;
+ u32 val;
- /* deactivate cxsr */
- I915_WRITE(DSPFW3, I915_READ(DSPFW3) ~PINEVIEW_SELF_REFRESH_EN);
+ if (IS_VALLEYVIEW(dev)) {
+ I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
+ } else if (IS_G4X(dev) || IS_CRESTLINE(dev)) {
+ I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
+ } else if (IS_PINEVIEW(dev)) {
+ val = I915_READ(DSPFW3) ~PINEVIEW_SELF_REFRESH_EN;
+ val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
+ I915_WRITE(DSPFW3, val);
+ } else if (IS_I945G(dev) || IS_I945GM(dev)) {
+ val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
+ _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
+ I915_WRITE(FW_BLC_SELF, val);
+ } else if (IS_I915GM(dev)) {
+ val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
+ _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
+ I915_WRITE(INSTPM, val);
+ } else {
+ return;
+ }
+
+ DRM_DEBUG_KMS(memory self-refresh is %s\n,
+ enable ? enabled : disabled);
}
/*
@@ -1036,7 +1057,7 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
dev_priv-fsb_freq,
dev_priv-mem_freq);
if (!latency) {
DRM_DEBUG_KMS(Unknown FSB/MEM found, disable CxSR\n);
- pineview_disable_cxsr(dev);
+ intel_set_memory_cxsr(dev_priv, false);
return;
}
@@ -1087,13 +1108,9 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
I915_WRITE(DSPFW3, reg);
DRM_DEBUG_KMS(DSPFW3 register is %x\n, reg);
- /* activate cxsr */
- I915_WRITE(DSPFW3,
- I915_READ(DSPFW3) | PINEVIEW_SELF_REFRESH_EN);
- DRM_DEBUG_KMS(Self-refresh is enabled\n);
+ intel_set_memory_cxsr(dev_priv, true);
} else {
- pineview_disable_cxsr(dev);
- DRM_DEBUG_KMS(Self-refresh is disabled\n);
+ intel_set_memory_cxsr(dev_priv, false);
}
}
@@ -1345,10 +1362,9 @@ static void valleyview_update_wm(struct drm_crtc *crtc)
valleyview_wm_info,
valleyview_cursor_wm_info,
ignore_plane_sr, cursor_sr)) {
- I915_WRITE(FW_BLC_SELF_VLV, FW_CSPWRDWNEN);
+ intel_set_memory_cxsr(dev_priv, true);
} else {
- I915_WRITE(FW_BLC_SELF_VLV,
- I915_READ(FW_BLC_SELF_VLV) ~FW_CSPWRDWNEN);
+ intel_set_memory_cxsr(dev_priv, false);
plane_sr = cursor_sr = 0;
}
@@ -1397,10 +1413,9 @@ static void g4x_update_wm(struct drm_crtc *crtc)
g4x_wm_info,
[Intel-gfx] [RFC PATCH 1/3] drm/i915: add opregion function to notify BIOS of CDCLK changes
Notifying the BIOS about CDCLK changes lets it program audio controller
EM4/EM5 divider values accordingly.
Signed-off-by: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 6 ++
drivers/gpu/drm/i915/intel_opregion.c | 29 +
2 files changed, 35 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea59649ef2..2feb8215f9fa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2602,6 +2602,7 @@ extern int intel_opregion_notify_encoder(struct
intel_encoder *intel_encoder,
bool enable);
extern int intel_opregion_notify_adapter(struct drm_device *dev,
pci_power_t state);
+extern int intel_opregion_notify_cdclk(struct drm_device *dev, int cdclk);
#else
static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
static inline void intel_opregion_init(struct drm_device *dev) { return; }
@@ -2617,6 +2618,11 @@ intel_opregion_notify_adapter(struct drm_device *dev,
pci_power_t state)
{
return 0;
}
+static inline int
+intel_opregion_notify_cdclk(struct drm_device *dev, int cdclk)
+{
+ return 0;
+}
#endif
/* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_opregion.c
b/drivers/gpu/drm/i915/intel_opregion.c
index 2e2c71fcc9ed..6450d2625624 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -219,6 +219,7 @@ struct opregion_asle {
#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 18)
#define SWSCI_SBCB_POST_VBE_PM SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
#define SWSCI_SBCB_ENABLE_DISABLE_AUDIOSWSCI_FUNCTION_CODE(SWSCI_SBCB,
21)
+#define SWSCI_SBCB_CD_CLOCK_CHANGE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 22)
#define ACPI_OTHER_OUTPUT (08)
#define ACPI_VGA_OUTPUT (18)
@@ -395,6 +396,34 @@ int intel_opregion_notify_adapter(struct drm_device *dev,
pci_power_t state)
return -EINVAL;
}
+int intel_opregion_notify_cdclk(struct drm_device *dev, int cdclk)
+{
+ u32 parm;
+
+ if (!IS_BROADWELL(dev))
+ return 0;
+
+ switch (cdclk) {
+ case 337500:
+ parm = 2;
+ break;
+ case 45:
+ parm = 0;
+ break;
+ case 54:
+ parm = 1;
+ break;
+ case 675000:
+ parm = 3;
+ break;
+ default:
+ WARN_ONCE(1, unknown cdclk %d\n, cdclk);
+ return -EINVAL;
+ }
+
+ return swsci(dev, SWSCI_SBCB_CD_CLOCK_CHANGE, parm, NULL);
+}
+
static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
{
struct drm_i915_private *dev_priv = dev-dev_private;
--
2.0.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC PATCH 2/3] drm/i915: add opregion function to enable/disable audio device
Signed-off-by: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 6 ++
drivers/gpu/drm/i915/intel_opregion.c | 10 ++
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2feb8215f9fa..f8e7a74a21ff 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2603,6 +2603,7 @@ extern int intel_opregion_notify_encoder(struct
intel_encoder *intel_encoder,
extern int intel_opregion_notify_adapter(struct drm_device *dev,
pci_power_t state);
extern int intel_opregion_notify_cdclk(struct drm_device *dev, int cdclk);
+extern int intel_opregion_audio_enable(struct drm_device *dev, bool enable);
#else
static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
static inline void intel_opregion_init(struct drm_device *dev) { return; }
@@ -2623,6 +2624,11 @@ intel_opregion_notify_cdclk(struct drm_device *dev, int
cdclk)
{
return 0;
}
+static inline int
+intel_opregion_audio_enable(struct drm_device *dev, bool enable)
+{
+ return 0;
+}
#endif
/* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_opregion.c
b/drivers/gpu/drm/i915/intel_opregion.c
index 6450d2625624..7482b687ac20 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -424,6 +424,16 @@ int intel_opregion_notify_cdclk(struct drm_device *dev,
int cdclk)
return swsci(dev, SWSCI_SBCB_CD_CLOCK_CHANGE, parm, NULL);
}
+int intel_opregion_audio_enable(struct drm_device *dev, bool enable)
+{
+ u32 parm = enable ? 1 : 0;
+
+ if (!IS_HASWELL(dev) !IS_BROADWELL(dev))
+ return 0;
+
+ return swsci(dev, SWSCI_SBCB_ENABLE_DISABLE_AUDIO, parm, NULL);
+}
+
static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
{
struct drm_i915_private *dev_priv = dev-dev_private;
--
2.0.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC PATCH 0/3] drm/i915: tell bios to update audio controller em4/5 values
Hi Mengdong - Here's the first drafts towards fixing [1], but unfortunately this is still Broadwell specific. Currently patch 2 is not needed, but is included in case we'll need that too (maybe for Haswell?). Please review/test, I don't have access to either hsw or bdw right now. BR, Jani. [1] https://bugzilla.kernel.org/show_bug.cgi?id=74861 Jani Nikula (3): drm/i915: add opregion function to notify BIOS of CDCLK changes drm/i915: add opregion function to enable/disable audio device drm/i915: tell BIOS to update audio controller EM4/EM5 divider values drivers/gpu/drm/i915/i915_drv.h | 12 +++ drivers/gpu/drm/i915/intel_opregion.c | 39 +++ drivers/gpu/drm/i915/intel_pm.c | 11 ++ 3 files changed, 62 insertions(+) -- 2.0.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC PATCH 3/3] drm/i915: tell BIOS to update audio controller EM4/EM5 divider values
If the display power well has been disabled, the display audio controller divider values EM4 MVALUE and EM5 NVALUE will have been lost. Notify the BIOS about CDCLK change through opregion to make it reprogram the values when the audio driver requests the power well. Otherwise the audio playback speed may be wrong. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=74861 Reported-by: Neil Shepperd nshepp...@gmail.com Signed-off-by: Jani Nikula jani.nik...@intel.com --- NOTE: This will *not* yet fix the referenced bug; this is currently for Broadwell only. --- drivers/gpu/drm/i915/intel_pm.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a90fdbd30edf..b1cbb6f4ec06 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6237,6 +6237,17 @@ int i915_request_power_well(void) dev_priv = container_of(hsw_pwr, struct drm_i915_private, power_domains); intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); + + /* +* If the display power well has been disabled, the display audio +* controller divider values EM4 MVALUE and EM5 NVALUE will have been +* lost. Notify the BIOS about CDCLK change through opregion to make it +* reprogram the values. Otherwise the audio playback speed will be +* wrong. +*/ + intel_opregion_notify_cdclk(dev_priv-dev, + intel_ddi_get_cdclk_freq(dev_priv)); + return 0; } EXPORT_SYMBOL_GPL(i915_request_power_well); -- 2.0.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/5] drm/i915: Kick out vga console
Hi Chris,
I had to rediff to get a patch that applies... I am not hanging with this
applied - it
does look like the i915 is starting is initialization later boot the new kernel.
[2.389796] [drm] Radeon Display Connectors
[2.389798] [drm] Connector 0:
[2.389799] [drm] DP-1
[2.389799] [drm] HPD2
[2.389801] [drm] DDC: 0x6530 0x6530 0x6534 0x6534 0x6538 0x6538 0x653c
0x653c
[2.389802] [drm] Encoders:
[2.389803] [drm] DFP1: INTERNAL_UNIPHY2
[2.389804] [drm] Connector 1:
[2.389805] [drm] HDMI-A-1
[2.389805] [drm] HPD3
[2.389806] [drm] DDC: 0x6550 0x6550 0x6554 0x6554 0x6558 0x6558 0x655c
0x655c
[2.389807] [drm] Encoders:
[2.389808] [drm] DFP2: INTERNAL_UNIPHY2
[2.389809] [drm] Connector 2:
[2.389810] [drm] DVI-D-1
[2.389811] [drm] HPD1
[2.389812] [drm] DDC: 0x6560 0x6560 0x6564 0x6564 0x6568 0x6568 0x656c
0x656c
[2.389813] [drm] Encoders:
[2.389814] [drm] DFP3: INTERNAL_UNIPHY1
[2.389815] [drm] Connector 3:
[2.389815] [drm] DVI-I-1
[2.389816] [drm] HPD6
[2.389817] [drm] DDC: 0x6580 0x6580 0x6584 0x6584 0x6588 0x6588 0x658c
0x658c
[2.389818] [drm] Encoders:
[2.389819] [drm] DFP4: INTERNAL_UNIPHY
[2.389820] [drm] CRT1: INTERNAL_KLDSCP_DAC1
[2.435689] raid6: avx2x2 24564 MB/s
[2.435691] Switched to clocksource tsc
[2.489087] usb 1-8: new low-speed USB device number 7 using xhci_hcd
[2.492403] raid6: avx2x4 34887 MB/s
[2.492404] raid6: using algorithm avx2x4 (34887 MB/s)
[2.492405] raid6: using avx2x2 recovery algorithm
[2.492789] xor: automatically using best checksumming function:
[2.502557] Adding 5779452k swap on /dev/sdb2. Priority:-2 extents:1
across:5779452k FS
[2.511532] [drm] fb mappable at 0xE098E000
[2.511536] [drm] vram apper at 0xE000
[2.511538] [drm] size 9216000
[2.511539] [drm] fb depth is 24
[2.511541] [drm]pitch is 7680
[2.511590] fbcon: radeondrmfb (fb0) is primary device
[2.516691] nct6775: Found NCT6776D/F or compatible chip at 0x2e:0x290
[2.525778]avx : 41474.400 MB/sec
[2.532408] Console: switching to colour frame buffer device 240x75
[2.535567] radeon :01:00.0: fb0: radeondrmfb frame buffer device
[2.535567] radeon :01:00.0: registered panic notifier
[2.544968] Btrfs loaded
[2.545276] BTRFS: device fsid 9d4254aa-6715-4fa8-986a-1af0d51768ad devid 1
transid 308068 /dev/sdc1
[2.545739] BTRFS: device fsid 9d4254aa-6715-4fa8-986a-1af0d51768ad devid 2
transid 308068 /dev/sdb1
[2.552946] [drm] Initialized radeon 2.39.0 20080528 for :01:00.0 on
minor 0
[2.553248] [drm] Memory usable by graphics device = 2048M
[2.553273] [drm] Replacing VGA console driver
[2.572539] i915 :00:02.0: irq 46 for MSI/MSI-X
[2.572546] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[2.572565] [drm] Driver supports precise vblank timestamp query.
If you are happy with this you can give this patch my tested by.
Thanks
Ed Tomlinson
On Monday 30 June 2014 07:59:55 Chris Wilson wrote:
On Sat, Jun 28, 2014 at 11:55:19PM -0400, Ed Tomlinson wrote:
On Saturday 28 June 2014 15:28:22 Ed Tomlinson wrote:
Resend without html krud which causes list to bounce the message.
Hi
This commit ( a4de05268e674e8ed31df6348269e22d6c6a1803 ) hangs my boot
with 3.16-git. Reverting it lets the boot proceed.
I have an i7 with a built-in i915 and an pcie r7 260x. The R7 is the
primary console. The i915 is initialized
but does not have a physical display attached.
With the patch applied the boot stops at the messages:
[drm] Memory usable by graphics device = 2048M
[drm] Replacing VGA console driver
The issue looks like that we are ripping out the radeon fb_con whilst it
is active and that upsets everyone. In which case, I think the
compromise is:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5f44581..4915f1d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1439,18 +1439,20 @@ static int i915_kick_out_vgacon(struct
drm_i915_private *dev_priv)
#else
static int i915_kick_out_vgacon(struct drm_i915_private *dev_priv)
{
- int ret;
+ int ret = 0;
DRM_INFO(Replacing VGA console driver\n);
console_lock();
- ret = do_take_over_console(dummy_con, 0, MAX_NR_CONSOLES - 1, 1);
- if (ret == 0) {
- ret = do_unregister_con_driver(vga_con);
-
- /* Ignore already unregistered. */
- if (ret == -ENODEV)
- ret = 0;
+ if (con_is_bound(vga_con)) {
+ ret = do_take_over_console(dummy_con, 0, MAX_NR_CONSOLES -
1, 1);
+ if (ret == 0) {
+ ret = do_unregister_con_driver(vga_con);
+
+ /* Ignore
Re: [Intel-gfx] [RFC PATCH 1/3] drm/i915: add opregion function to notify BIOS of CDCLK changes
On Tue, 01 Jul 2014, Jani Nikula jani.nik...@intel.com wrote:
Notifying the BIOS about CDCLK changes lets it program audio controller
EM4/EM5 divider values accordingly.
Signed-off-by: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 6 ++
drivers/gpu/drm/i915/intel_opregion.c | 29 +
2 files changed, 35 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea59649ef2..2feb8215f9fa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2602,6 +2602,7 @@ extern int intel_opregion_notify_encoder(struct
intel_encoder *intel_encoder,
bool enable);
extern int intel_opregion_notify_adapter(struct drm_device *dev,
pci_power_t state);
+extern int intel_opregion_notify_cdclk(struct drm_device *dev, int cdclk);
#else
static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
static inline void intel_opregion_init(struct drm_device *dev) { return; }
@@ -2617,6 +2618,11 @@ intel_opregion_notify_adapter(struct drm_device *dev,
pci_power_t state)
{
return 0;
}
+static inline int
+intel_opregion_notify_cdclk(struct drm_device *dev, int cdclk)
+{
+ return 0;
+}
#endif
/* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_opregion.c
b/drivers/gpu/drm/i915/intel_opregion.c
index 2e2c71fcc9ed..6450d2625624 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -219,6 +219,7 @@ struct opregion_asle {
#define SWSCI_SBCB_SET_SPREAD_SPECTRUM SWSCI_FUNCTION_CODE(SWSCI_SBCB,
18)
#define SWSCI_SBCB_POST_VBE_PM SWSCI_FUNCTION_CODE(SWSCI_SBCB,
19)
#define SWSCI_SBCB_ENABLE_DISABLE_AUDIO SWSCI_FUNCTION_CODE(SWSCI_SBCB,
21)
+#define SWSCI_SBCB_CD_CLOCK_CHANGE SWSCI_FUNCTION_CODE(SWSCI_SBCB, 22)
#define ACPI_OTHER_OUTPUT (08)
#define ACPI_VGA_OUTPUT (18)
@@ -395,6 +396,34 @@ int intel_opregion_notify_adapter(struct drm_device
*dev, pci_power_t state)
return -EINVAL;
}
+int intel_opregion_notify_cdclk(struct drm_device *dev, int cdclk)
+{
+ u32 parm;
+
+ if (!IS_BROADWELL(dev))
+ return 0;
To clarify, the spec I have states this call is just for Broadwell, so
it seems we can't just call this on Haswell to fix the bug.
BR,
Jani.
+
+ switch (cdclk) {
+ case 337500:
+ parm = 2;
+ break;
+ case 45:
+ parm = 0;
+ break;
+ case 54:
+ parm = 1;
+ break;
+ case 675000:
+ parm = 3;
+ break;
+ default:
+ WARN_ONCE(1, unknown cdclk %d\n, cdclk);
+ return -EINVAL;
+ }
+
+ return swsci(dev, SWSCI_SBCB_CD_CLOCK_CHANGE, parm, NULL);
+}
+
static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
{
struct drm_i915_private *dev_priv = dev-dev_private;
--
2.0.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] drm/i915: flush delayed_resume_work when suspending
On Sat, 28 Jun 2014, Paulo Zanoni przan...@gmail.com wrote: From: Paulo Zanoni paulo.r.zan...@intel.com It is possible that, by the time we run i915_drm_freeze(), delayed_resume_work was already queued but did not run yet. If it still didn't run after intel_runtime_pm_disable_interrupts(), by the time it runs it will try to change the interrupt registers with the interrupts already disabled, which will trigger a WARN. We can reliably reproduce this with the pm_rpm system-suspend test case. In order to avoid the problem, we have to flush the work before disabling the interrupts. We could also cancel the work instead of flushing it, but that would require us to put a runtime PM reference - and any other resource we may need in the future - in case the work was already queued, so I believe flushing the work is more future-proof, although less efficient. But I can also change this part if someone requests. Another thing I tried was to move the intel_suspend_gt_powersave() call to before intel_runtime_pm_disable_interrupts(), but since that function needs to be called after the interrupts are already disabled, due to dev_priv-rps.work, this strategy didn't work. Testcase: igt/pm_rpm/system-suspend Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80517 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Pushed to dinq, thanks for the patch and review. BR, Jani. --- drivers/gpu/drm/i915/i915_drv.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e64547e..672694b 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -524,6 +524,8 @@ static int i915_drm_freeze(struct drm_device *dev) return error; } + flush_delayed_work(dev_priv-rps.delayed_resume_work); + intel_runtime_pm_disable_interrupts(dev); dev_priv-enable_hotplug_processing = false; -- 2.0.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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] [RFC PATCH 4/3 :] drm/i915: tell BIOS to update controller EM4/EM5 divider values
This may do what's required on Haswell to achieve the same as the previous patch does on Broadwell... Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=74861 Signed-off-by: Jani Nikula jani.nik...@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index b1cbb6f4ec06..504c8675e8b4 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6248,6 +6248,8 @@ int i915_request_power_well(void) intel_opregion_notify_cdclk(dev_priv-dev, intel_ddi_get_cdclk_freq(dev_priv)); + intel_opregion_audio_enable(dev_priv-dev, true); + return 0; } EXPORT_SYMBOL_GPL(i915_request_power_well); @@ -6260,6 +6262,8 @@ int i915_release_power_well(void) if (!hsw_pwr) return -ENODEV; + intel_opregion_audio_enable(dev_priv-dev, false); + dev_priv = container_of(hsw_pwr, struct drm_i915_private, power_domains); intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO); -- 2.0.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.
It seems the flexibility on rings is more wanted and needed than I imagined.
Please ignore this patch here...
I liked both execution flag or debugfs, but exec flag would cover this case of
different applications using different command streamers.
With flags Would it be something like:
Execution without flag = ping-pong
Flag BSD1 use only VCS1
Flag BSD2 use only VCS2
Haihao, what do you think?
With debugfs would be something like i195_dual_bsd_ring file with 3 options:
all bsd1 bsd2
Thanks,
Rodrigo.
-Original Message-
From: Zhao, Yakui
Sent: Monday, June 30, 2014 6:37 PM
To: Vivi, Rodrigo
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Introduce dual_bsd_ring
parameter.
On Mon, 2014-06-30 at 10:51 -0600, Rodrigo Vivi wrote:
On Broadwell GT3 we have 2 Video Command Streamers (VCS), but
userspace has no control when using VCS1 or VCS2. So we cannot test,
validate or debug specific changes or workaround that might affect
only one or another ring. So this patch introduces a mechanism to
avoid the ping-pong selection and use one specific ring given at boot time.
If it is mainly used for the test/validation, can we add one override flag
so that the user-space app can explicitly declare which BSD ring is used to
dispatch the corresponding BSD commands? In such case it will force to dispatch
the corresponding commands on the ring passed by user-application.
At the same time this patch is not helpful under the following scenario. For
example: One application hopes to use the BSD Ring 0 while another application
hopes to use the BSD ring 1.
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34
++
drivers/gpu/drm/i915/i915_params.c | 6 ++
3 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h index 8cea596..7b6614f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2069,6 +2069,7 @@ struct i915_params {
int panel_ignore_lid;
unsigned int powersave;
int semaphores;
+ int dual_bsd_ring;
unsigned int lvds_downclock;
int lvds_channel_mode;
int panel_use_ssc;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d815ef5..09f350e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct
drm_device *dev, {
struct drm_i915_private *dev_priv = dev-dev_private;
struct drm_i915_file_private *file_priv = file-driver_priv;
+ int ring_id;
+ int dual = i915.dual_bsd_ring;
/* Check whether the file_priv is using one ring */
if (file_priv-bsd_ring)
return file_priv-bsd_ring-id;
- else {
- /* If no, use the ping-pong mechanism to select one ring */
- int ring_id;
- mutex_lock(dev-struct_mutex);
- if (dev_priv-mm.bsd_ring_dispatch_index == 0) {
- ring_id = VCS;
- dev_priv-mm.bsd_ring_dispatch_index = 1;
- } else {
- ring_id = VCS2;
- dev_priv-mm.bsd_ring_dispatch_index = 0;
- }
- file_priv-bsd_ring = dev_priv-ring[ring_id];
- mutex_unlock(dev-struct_mutex);
- return ring_id;
+ /* If no, use the parameter defined or ping-pong mechanism
+ * to select one ring */
+ mutex_lock(dev-struct_mutex);
+
+ if (dual == 1 || (dual != 2
+ dev_priv-mm.bsd_ring_dispatch_index == 0)) {
+ ring_id = VCS;
+ dev_priv-mm.bsd_ring_dispatch_index = 1;
+ } else {
+ ring_id = VCS2;
+ dev_priv-mm.bsd_ring_dispatch_index = 0;
}
+
+ file_priv-bsd_ring = dev_priv-ring[ring_id];
+ mutex_unlock(dev-struct_mutex);
+
+ WARN(dual, Forcibly trying to use only one bsd ring. Using: %s\n,
+ file_priv-bsd_ring-name);
+ return ring_id;
}
static struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 8145729..d4871c8 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -29,6 +29,7 @@ struct i915_params i915 __read_mostly = {
.panel_ignore_lid = 1,
.powersave = 1,
.semaphores = -1,
+ .dual_bsd_ring = 0,
.lvds_downclock = 0,
.lvds_channel_mode = 0,
.panel_use_ssc = -1,
@@ -70,6 +71,11 @@ MODULE_PARM_DESC(semaphores,
Use semaphores for inter-ring sync
(default: -1 (use per-chip defaults)));
+module_param_named(dual_bsd_ring,
Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx-id to ctx-handle
-Original Message-
From: Jesse Barnes [mailto:jbar...@virtuousgeek.org]
Sent: Monday, June 30, 2014 9:54 PM
To: Mateo Lozano, Oscar
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx-id to ctx-handle
On Thu, 26 Jun 2014 14:24:15 +0100
oscar.ma...@intel.com wrote:
From: Oscar Mateo oscar.ma...@intel.com
This is an Execlists preparatory patch, since they make context ID
become an overloaded term:
- In the software, it was used to distinguish which context userspace was
trying to use.
- In the BSpec, the term is used to describe the 20-bits long field the
hardware uses to it to discriminate the contexts that are submitted to
the ELSP and inform the driver about their current status (via Context
Switch Interrupts and Context Status Buffers).
Initially, I tried to make the different meanings converge, but it
proved
impossible:
- The software ctx-id is per-filp, while the hardware one needs to be
globally unique.
- Also, we multiplex several backing states objects per intel_context,
and all of them need unique HW IDs.
- I tried adding a per-filp ID and then composing the HW context ID as:
ctx-id + file_priv-id + ring-id, but the fact that the hardware only
uses 20-bits means we have to artificially limit the number of filps or
contexts the userspace can create.
The ctx-handle bits are done with this Cocci patch (plus manual
frobbing of the struct declaration):
@@
struct intel_context c;
@@
- (c).id
+ c.handle
@@
struct intel_context *c;
@@
- (c)-id
+ c-handle
Also, while we are at it,
s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c| 2 +-
drivers/gpu/drm/i915/i915_drv.h| 6 +++---
drivers/gpu/drm/i915/i915_gem_context.c| 12 ++--
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/intel_uncore.c| 2 +-
5 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index d4b8391..7484e22 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1867,7 +1867,7 @@ static int per_file_ctx(int id, void *ptr, void
*data)
if (i915_gem_context_is_default(ctx))
seq_puts(m, default context:\n);
else
- seq_printf(m, context %d:\n, ctx-id);
+ seq_printf(m, context %d:\n, ctx-handle);
ppgtt-debug_dump(ppgtt, m);
return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h index 122e942..5d2b6ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -584,10 +584,10 @@ struct i915_ctx_hang_stats { };
/* This must match up with the value previously used for
execbuf2.rsvd1. */ -#define DEFAULT_CONTEXT_ID 0
+#define DEFAULT_CONTEXT_HANDLE 0
struct intel_context {
struct kref ref;
- int id;
+ int handle;
bool rcs_is_initialized;
uint8_t remap_slice;
struct drm_i915_file_private *file_priv; @@ -2458,7 +2458,7 @@
static inline void i915_gem_context_unreference(struct intel_context
*ctx)
static inline bool i915_gem_context_is_default(const struct
intel_context *c) {
- return c-id == DEFAULT_CONTEXT_ID;
+ return c-handle == DEFAULT_CONTEXT_HANDLE;
}
int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index b8b9859..75e903f 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -276,14 +276,14 @@ __create_hw_context(struct drm_device *dev,
/* Default context will never have a file_priv */
if (file_priv != NULL) {
ret = idr_alloc(file_priv-context_idr, ctx,
- DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
+ DEFAULT_CONTEXT_HANDLE, 0,
GFP_KERNEL);
if (ret 0)
goto err_out;
} else
- ret = DEFAULT_CONTEXT_ID;
+ ret = DEFAULT_CONTEXT_HANDLE;
ctx-file_priv = file_priv;
- ctx-id = ret;
+ ctx-handle = ret;
/* NB: Mark all slices as needing a remap so that when the context
first
* loads it will restore whatever remap state already exists. If there
* is no remap info, it will be a NOP. */ @@ -787,7 +787,7 @@ int
i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
if (IS_ERR(ctx))
return PTR_ERR(ctx);
- args-ctx_id = ctx-id;
+ args-ctx_id = ctx-handle;
DRM_DEBUG_DRIVER(HW context %d created\n, args-ctx_id);
return 0;
@@ -801,7 +801,7 @@ int
Re: [Intel-gfx] [PATCH] drm/i915: Try harder to get FBC
Jani, please ignore the 4th patch on this series and merge the 3 I've
reviewed and tested already.
They are essential to allow FBC work on BDW without changing bios
configuration and allow PC7 residency.
Thanks,
Rodrigo.
On Mon, Jun 30, 2014 at 10:41 AM, Rodrigo Vivi rodrigo.v...@intel.com
wrote:
From: Ben Widawsky benjamin.widaw...@intel.com
The GEN FBC unit provides the ability to set a low pass on frames it
attempts to compress. If a frame is less than a certain amount
compressibility (2:1, 4:1) it will not bother. This allows the driver to
reduce the size it requests out of stolen memory.
Unluckily, a few months ago, Ville actually began using this feature for
framebuffers that are 16bpp (not sure why not 8bpp). In those cases, we
are already using this mechanism for a different purpose, and so we can
only achieve one further level of compression (2:1 - 4:1)
FBC GEN1, ie. pre-G45 is ignored.
The cleverness of the patch is Art's. The bugs are mine.
v2: Update message and including missing threshold case 3 (Spotted by
Arthur).
Reviewedby: Rodrigo Vivi rodrigo.v...@intel.com
Cc: Art Runyan arthur.j.run...@intel.com
Signed-off-by: Ben Widawsky b...@bwidawsk.net
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h| 3 +-
drivers/gpu/drm/i915/i915_gem_stolen.c | 54
+-
drivers/gpu/drm/i915/intel_pm.c| 30 +--
3 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 5b7aed2..9953ea8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -600,6 +600,7 @@ struct intel_context {
struct i915_fbc {
unsigned long size;
+ unsigned threshold;
unsigned int fb_id;
enum plane plane;
int y;
@@ -2489,7 +2490,7 @@ static inline void i915_gem_chipset_flush(struct
drm_device *dev)
/* i915_gem_stolen.c */
int i915_gem_init_stolen(struct drm_device *dev);
-int i915_gem_stolen_setup_compression(struct drm_device *dev, int size);
+int i915_gem_stolen_setup_compression(struct drm_device *dev, int size,
int fb_cpp);
void i915_gem_stolen_cleanup_compression(struct drm_device *dev);
void i915_gem_cleanup_stolen(struct drm_device *dev);
struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
b/drivers/gpu/drm/i915/i915_gem_stolen.c
index a86b331..b695d18 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -105,35 +105,61 @@ static unsigned long i915_stolen_to_physical(struct
drm_device *dev)
static int find_compression_threshold(struct drm_device *dev,
struct drm_mm_node *node,
- int size)
+ int size,
+ int fb_cpp)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- const int compression_threshold = 1;
+ int compression_threshold = 1;
int ret;
- /* Try to over-allocate to reduce reallocations and fragmentation
*/
+ /* HACK: This code depends on what we will do in *_enable_fbc. If
that
+* code changes, this code needs to change as well.
+*
+* The enable_fbc code will attempt to use one of our 2 compression
+* thresholds, therefore, in that case, we only have 1 resort.
+*/
+
+ /* Try to over-allocate to reduce reallocations and fragmentation.
*/
ret = drm_mm_insert_node(dev_priv-mm.stolen, node,
size = 1, 4096, DRM_MM_SEARCH_DEFAULT);
- if (ret)
- ret = drm_mm_insert_node(dev_priv-mm.stolen, node,
-size = 1, 4096,
-DRM_MM_SEARCH_DEFAULT);
- if (ret)
+ if (ret == 0)
+ return compression_threshold;
+
+again:
+ /* HW's ability to limit the CFB is 1:4 */
+ if (compression_threshold 4 ||
+ (fb_cpp == 2 compression_threshold == 2))
return 0;
- else
+
+ ret = drm_mm_insert_node(dev_priv-mm.stolen, node,
+size = 1, 4096,
+DRM_MM_SEARCH_DEFAULT);
+ if (ret INTEL_INFO(dev)-gen = 4) {
+ return 0;
+ } else if (ret) {
+ compression_threshold = 1;
+ goto again;
+ } else {
return compression_threshold;
+ }
}
-static int i915_setup_compression(struct drm_device *dev, int size)
+static int i915_setup_compression(struct drm_device *dev, int size, int
fb_cpp)
{
struct drm_i915_private *dev_priv = dev-dev_private;
struct drm_mm_node *uninitialized_var(compressed_llb);
int ret;
ret
Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx-id to ctx-handle
On Tue, 1 Jul 2014 15:46:51 +
Mateo Lozano, Oscar oscar.ma...@intel.com wrote:
-Original Message-
From: Jesse Barnes [mailto:jbar...@virtuousgeek.org]
Sent: Monday, June 30, 2014 9:54 PM
To: Mateo Lozano, Oscar
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/8] drm/i915: Rename ctx-id to ctx-handle
On Thu, 26 Jun 2014 14:24:15 +0100
oscar.ma...@intel.com wrote:
From: Oscar Mateo oscar.ma...@intel.com
This is an Execlists preparatory patch, since they make context ID
become an overloaded term:
- In the software, it was used to distinguish which context userspace was
trying to use.
- In the BSpec, the term is used to describe the 20-bits long field the
hardware uses to it to discriminate the contexts that are submitted to
the ELSP and inform the driver about their current status (via Context
Switch Interrupts and Context Status Buffers).
Initially, I tried to make the different meanings converge, but it
proved
impossible:
- The software ctx-id is per-filp, while the hardware one needs to be
globally unique.
- Also, we multiplex several backing states objects per intel_context,
and all of them need unique HW IDs.
- I tried adding a per-filp ID and then composing the HW context ID as:
ctx-id + file_priv-id + ring-id, but the fact that the hardware only
uses 20-bits means we have to artificially limit the number of filps or
contexts the userspace can create.
The ctx-handle bits are done with this Cocci patch (plus manual
frobbing of the struct declaration):
@@
struct intel_context c;
@@
- (c).id
+ c.handle
@@
struct intel_context *c;
@@
- (c)-id
+ c-handle
Also, while we are at it,
s/DEFAULT_CONTEXT_ID/DEFAULT_CONTEXT_HANDLE.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c| 2 +-
drivers/gpu/drm/i915/i915_drv.h| 6 +++---
drivers/gpu/drm/i915/i915_gem_context.c| 12 ++--
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/intel_uncore.c| 2 +-
5 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index d4b8391..7484e22 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1867,7 +1867,7 @@ static int per_file_ctx(int id, void *ptr, void
*data)
if (i915_gem_context_is_default(ctx))
seq_puts(m, default context:\n);
else
- seq_printf(m, context %d:\n, ctx-id);
+ seq_printf(m, context %d:\n, ctx-handle);
ppgtt-debug_dump(ppgtt, m);
return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h index 122e942..5d2b6ab 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -584,10 +584,10 @@ struct i915_ctx_hang_stats { };
/* This must match up with the value previously used for
execbuf2.rsvd1. */ -#define DEFAULT_CONTEXT_ID 0
+#define DEFAULT_CONTEXT_HANDLE 0
struct intel_context {
struct kref ref;
- int id;
+ int handle;
bool rcs_is_initialized;
uint8_t remap_slice;
struct drm_i915_file_private *file_priv; @@ -2458,7 +2458,7 @@
static inline void i915_gem_context_unreference(struct intel_context
*ctx)
static inline bool i915_gem_context_is_default(const struct
intel_context *c) {
- return c-id == DEFAULT_CONTEXT_ID;
+ return c-handle == DEFAULT_CONTEXT_HANDLE;
}
int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index b8b9859..75e903f 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -276,14 +276,14 @@ __create_hw_context(struct drm_device *dev,
/* Default context will never have a file_priv */
if (file_priv != NULL) {
ret = idr_alloc(file_priv-context_idr, ctx,
- DEFAULT_CONTEXT_ID, 0, GFP_KERNEL);
+ DEFAULT_CONTEXT_HANDLE, 0,
GFP_KERNEL);
if (ret 0)
goto err_out;
} else
- ret = DEFAULT_CONTEXT_ID;
+ ret = DEFAULT_CONTEXT_HANDLE;
ctx-file_priv = file_priv;
- ctx-id = ret;
+ ctx-handle = ret;
/* NB: Mark all slices as needing a remap so that when the context
first
* loads it will restore whatever remap state already exists. If there
* is no remap info, it will be a NOP. */ @@ -787,7 +787,7 @@ int
i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
if (IS_ERR(ctx))
return PTR_ERR(ctx);
- args-ctx_id = ctx-id;
+
[Intel-gfx] [PATCH 2/3] drm/i915: Add ctx param to i915_gem_ring_dispatch trace point
From: Daniele Ceraolo Spurio daniele.ceraolospu...@intel.com The context used to execute a batchbuffer is becoming increasingly important. Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospu...@intel.com --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- drivers/gpu/drm/i915/i915_trace.h | 12 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index d815ef5..0b2b76e 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1372,7 +1372,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, goto err; } - trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags); + trace_i915_gem_ring_dispatch(ring, intel_ring_get_seqno(ring), flags, ctx); i915_gem_execbuffer_move_to_active(eb-vmas, ring); i915_gem_execbuffer_retire_commands(dev, file, ring, batch_obj); diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 9be1421..4e73e3a 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -352,14 +352,16 @@ TRACE_EVENT(i915_gem_ring_sync_to, ); TRACE_EVENT(i915_gem_ring_dispatch, - TP_PROTO(struct intel_engine_cs *ring, u32 seqno, u32 flags), - TP_ARGS(ring, seqno, flags), + TP_PROTO(struct intel_engine_cs *ring, u32 seqno, u32 flags, +struct intel_context *ctx), + TP_ARGS(ring, seqno, flags, ctx), TP_STRUCT__entry( __field(u32, dev) __field(u32, ring) __field(u32, seqno) __field(u32, flags) +__field(struct i915_address_space *, vm) ), TP_fast_assign( @@ -367,11 +369,13 @@ TRACE_EVENT(i915_gem_ring_dispatch, __entry-ring = ring-id; __entry-seqno = seqno; __entry-flags = flags; + __entry-vm = ctx-vm; i915_trace_irq_get(ring, seqno); ), - TP_printk(dev=%u, ring=%u, seqno=%u, flags=%x, - __entry-dev, __entry-ring, __entry-seqno, __entry-flags) + TP_printk(dev=%u, ring=%u, seqno=%u, flags=%x, vm=%p, + __entry-dev, __entry-ring, __entry-seqno, + __entry-flags, __entry-vm) ); TRACE_EVENT(i915_gem_ring_flush, -- 1.8.5.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] pin OABUFFER to GGTT
Submitting again (this time copying the mailing list correctly): The bo_pin ioctl has been discarded in GEN6+ with this patch:    drm/i915: Reject the pin ioctl on gen6+    Especially with ppgtt this kinda stopped making sense. And if we    indeed need this to hack around an issue, we need something that also    works for non-root.    Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch The thing is, the performance team used this call to pin the OABUFFER to GGTT and then mapping it to userspace. This OABUFFER cannot be in PPGTT because: When each context has its own Per Process GTT, this field should be always set to GGTT. (BSpec dixit). Can we re-enable it? or should we find an alternative for this case? -- Oscar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Add ppgtt init/release trace points
From: Daniele Ceraolo Spurio daniele.ceraolospu...@intel.com These tracepoints are useful for observing the creation and destruction of Full PPGTTs. Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospu...@intel.com --- drivers/gpu/drm/i915/i915_gem_context.c | 6 + drivers/gpu/drm/i915/i915_trace.h | 41 + 2 files changed, 47 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 0d2c75b..99f7022 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -88,6 +88,7 @@ #include drm/drmP.h #include drm/i915_drm.h #include i915_drv.h +#include i915_trace.h /* This is a HW constraint. The value below is the largest known requirement * I've seen in a spec to date, and that was a workaround for a non-shipping @@ -136,6 +137,8 @@ static void ppgtt_release(struct kref *kref) struct i915_hw_ppgtt *ppgtt = container_of(kref, struct i915_hw_ppgtt, ref); + trace_ppgtt_release(ppgtt); + do_ppgtt_cleanup(ppgtt); kfree(ppgtt); } @@ -215,6 +218,9 @@ create_vm_for_ctx(struct drm_device *dev, struct intel_context *ctx) } ppgtt-ctx = ctx; + + trace_create_vm_for_ctx(ppgtt); + return ppgtt; } diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index f5aa006..9be1421 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -587,6 +587,47 @@ TRACE_EVENT(intel_gpu_freq_change, TP_printk(new_freq=%u, __entry-freq) ); +TRACE_EVENT(create_vm_for_ctx, + + TP_PROTO(struct i915_hw_ppgtt *ppgtt), + + TP_ARGS(ppgtt), + + TP_STRUCT__entry( + __field(struct i915_address_space *, vm) + __field(u32, dev) + __field(u32, pid) + ), + + TP_fast_assign( + __entry-vm = ppgtt-base; + __entry-dev = ppgtt-base.dev-primary-index; + __entry-pid = (unsigned int)task_pid_nr(current); + ), + + TP_printk(dev=%u, task_pid=%u, vm=%p, + __entry-dev, __entry-pid, __entry-vm) +); + +TRACE_EVENT(ppgtt_release, + + TP_PROTO(struct i915_hw_ppgtt *ppgtt), + + TP_ARGS(ppgtt), + + TP_STRUCT__entry( + __field(struct i915_address_space *, vm) + __field(u32, dev) + ), + + TP_fast_assign( + __entry-vm = ppgtt-base; + __entry-dev = ppgtt-base.dev-primary-index; + ), + + TP_printk(dev=%u, vm=%p, __entry-dev, __entry-vm) +); + #endif /* _I915_TRACE_H_ */ /* This part must be outside protection */ -- 1.8.5.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/i915: Trace point callbacks for validation
From: Daniele Ceraolo Spurio daniele.ceraolospu...@intel.com These callbacks can be assigned to specific functions inside an external validation kernel module. This module can then extract run-time information to make sure everything is working as expected. Specifically, these two callbacks extract information about full PPGTT and batch dispatch. Signed-off-by: Daniele Ceraolo Spurio daniele.ceraolospu...@intel.com --- drivers/gpu/drm/i915/i915_gem_context.c| 3 +++ drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +++ drivers/gpu/drm/i915/i915_trace.h | 22 ++ 3 files changed, 28 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 99f7022..120a319 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -90,6 +90,9 @@ #include i915_drv.h #include i915_trace.h +i915_ppgtt_validation_callback_t ppgtt_validation_callback = NULL; +EXPORT_SYMBOL_GPL(ppgtt_validation_callback); + /* This is a HW constraint. The value below is the largest known requirement * I've seen in a spec to date, and that was a workaround for a non-shipping * part. It should be safe to decrease this, but it's more future proof as is. diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 0b2b76e..7feb977 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -33,6 +33,9 @@ #include intel_drv.h #include linux/dma_remapping.h +i915_gem_ring_dispatch_callback_t i915_gem_ring_dispatch_validation_callback = NULL; +EXPORT_SYMBOL_GPL(i915_gem_ring_dispatch_validation_callback); + #define __EXEC_OBJECT_HAS_PIN (131) #define __EXEC_OBJECT_HAS_FENCE (130) #define __EXEC_OBJECT_NEEDS_BIAS (128) diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index 4e73e3a..98c6f47 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -15,6 +15,18 @@ #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM) #define TRACE_INCLUDE_FILE i915_trace +/* validation callbacks */ + +typedef void (*i915_ppgtt_validation_callback_t)(unsigned int action_code, +struct i915_hw_ppgtt *ppgtt); +extern i915_ppgtt_validation_callback_t ppgtt_validation_callback; + +typedef void (*i915_gem_ring_dispatch_callback_t)(struct intel_engine_cs *ring, + u32 seqno, + u32 flags, + struct intel_context *ctx); +extern i915_gem_ring_dispatch_callback_t i915_gem_ring_dispatch_validation_callback; + /* pipe updates */ TRACE_EVENT(i915_pipe_update_start, @@ -371,6 +383,10 @@ TRACE_EVENT(i915_gem_ring_dispatch, __entry-flags = flags; __entry-vm = ctx-vm; i915_trace_irq_get(ring, seqno); + + if (i915_gem_ring_dispatch_validation_callback) + i915_gem_ring_dispatch_validation_callback(ring, + seqno, flags, ctx); ), TP_printk(dev=%u, ring=%u, seqno=%u, flags=%x, vm=%p, @@ -607,6 +623,9 @@ TRACE_EVENT(create_vm_for_ctx, __entry-vm = ppgtt-base; __entry-dev = ppgtt-base.dev-primary-index; __entry-pid = (unsigned int)task_pid_nr(current); + + if (ppgtt_validation_callback) + ppgtt_validation_callback(0, ppgtt); ), TP_printk(dev=%u, task_pid=%u, vm=%p, @@ -627,6 +646,9 @@ TRACE_EVENT(ppgtt_release, TP_fast_assign( __entry-vm = ppgtt-base; __entry-dev = ppgtt-base.dev-primary-index; + + if (ppgtt_validation_callback) + ppgtt_validation_callback(1, ppgtt); ), TP_printk(dev=%u, vm=%p, __entry-dev, __entry-vm) -- 1.8.5.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] pin OABUFFER to GGTT
On Tue, Jul 01, 2014 at 04:24:56PM +, Mateo Lozano, Oscar wrote: Submitting again (this time copying the mailing list correctly): The bo_pin ioctl has been discarded in GEN6+ with this patch:    drm/i915: Reject the pin ioctl on gen6+    Especially with ppgtt this kinda stopped making sense. And if we    indeed need this to hack around an issue, we need something that also    works for non-root.    Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch The thing is, the performance team used this call to pin the OABUFFER to GGTT and then mapping it to userspace. This OABUFFER cannot be in PPGTT because: When each context has its own Per Process GTT, this field should be always set to GGTT. (BSpec dixit). Can we re-enable it? or should we find an alternative for this case? EXEC_OBJECT_NEEDS_GTT? -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] drm/i915: Fix VCS2's ring name.
It just fix a typo. v2: removing underscore to let this like all other ring names (Oscar) Cc: Oscar Mateo oscar.ma...@intel.com Reviewed-by (v1): Ben Widawsky benjamin.widaw...@intel.com Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com --- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 2faef26..22c2b9a 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2224,7 +2224,7 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev) return -EINVAL; } - ring-name = bds2_ring; + ring-name = bsd2 ring; ring-id = VCS2; ring-write_tail = ring_write_tail; -- 1.9.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] pin OABUFFER to GGTT
-Original Message-
From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
Sent: Tuesday, July 01, 2014 5:30 PM
To: Mateo Lozano, Oscar
Cc: Intel-gfx@lists.freedesktop.org; Madajczak, Tomasz
Subject: Re: [Intel-gfx] pin OABUFFER to GGTT
On Tue, Jul 01, 2014 at 04:24:56PM +, Mateo Lozano, Oscar wrote:
Submitting again (this time copying the mailing list correctly):
The bo_pin ioctl has been discarded in GEN6+ with this patch:
   drm/i915: Reject the pin ioctl on gen6+
Especially with ppgtt this kinda stopped making sense. And if we
   indeed need this to hack around an issue, we need something that also
   works for non-root.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
The thing is, the performance team used this call to pin the OABUFFER to
GGTT and then mapping it to userspace. This OABUFFER cannot be in PPGTT
because: When each context has its own Per Process GTT, this field should
be always set to GGTT. (BSpec dixit).
Can we re-enable it? or should we find an alternative for this case?
EXEC_OBJECT_NEEDS_GTT?
-Chris
The object (AFAICT, please Tomasz correct me if I am wrong) is not really used
inside any batchbuffer.
Also:
if (exec[i].flags EXEC_OBJECT_NEEDS_GTT
USES_FULL_PPGTT(vm-dev)) {
ret = -EINVAL;
goto err;
}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] WAs in init_clock_gating?
Is there any reason why the WAs are applied in *_init_clock_gating? We are finding that some of them are lost during reset, and also the default context ends up with wrong values because the render context is restored saved before we get to gen8_init_clok_gating (at least with Execlists, I´m not sure this happens with MI_SET_CONTEXT because the context won´t be saved until the next switch). I believe this have been brought to the mailing list a couple of times, like: drm/i916: Init chv workarounds at render ring init My bsw is an unhappy camper if we delay the workaround init until init_clock_gating(). Move a bunch of it to the render ring init. FIXME: need to do this for all platforms since some of the registers also get clobbered at reset. Just need to figure out which registers those actually are. This patch is based on a slightly educated guess, but verifying on actual hw would be a good idea. Also should maybe move the init_clock_gating earlier too since we set up a bunch of clock gating stuff there that might be important for a properly working GT. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com And also: http://lists.freedesktop.org/archives/intel-gfx/2013-November/036482.html -- Oscar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] pin OABUFFER to GGTT
On Tue, Jul 01, 2014 at 04:34:48PM +, Mateo Lozano, Oscar wrote:
-Original Message-
From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
Sent: Tuesday, July 01, 2014 5:30 PM
To: Mateo Lozano, Oscar
Cc: Intel-gfx@lists.freedesktop.org; Madajczak, Tomasz
Subject: Re: [Intel-gfx] pin OABUFFER to GGTT
On Tue, Jul 01, 2014 at 04:24:56PM +, Mateo Lozano, Oscar wrote:
Submitting again (this time copying the mailing list correctly):
The bo_pin ioctl has been discarded in GEN6+ with this patch:
   drm/i915: Reject the pin ioctl on gen6+
Especially with ppgtt this kinda stopped making sense. And if we
   indeed need this to hack around an issue, we need something that also
   works for non-root.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
The thing is, the performance team used this call to pin the OABUFFER to
GGTT and then mapping it to userspace. This OABUFFER cannot be in PPGTT
because: When each context has its own Per Process GTT, this field should
be always set to GGTT. (BSpec dixit).
Can we re-enable it? or should we find an alternative for this case?
EXEC_OBJECT_NEEDS_GTT?
-Chris
The object (AFAICT, please Tomasz correct me if I am wrong) is not really
used inside any batchbuffer.
Then what's the issue? If you only use it as via a global gtt mapping it
only exists in the ggtt.
Also:
if (exec[i].flags EXEC_OBJECT_NEEDS_GTT
USES_FULL_PPGTT(vm-dev)) {
ret = -EINVAL;
goto err;
}
Yeah, that's just full-ppgtt not quite being ready yet.
-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] pin OABUFFER to GGTT
-Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Tuesday, July 01, 2014 5:52 PM To: Mateo Lozano, Oscar Cc: Intel-gfx@lists.freedesktop.org; Madajczak, Tomasz; Rutkowski, Adam J; Jesse Barnes (jbar...@virtuousgeek.org) Subject: Re: [Intel-gfx] pin OABUFFER to GGTT On Tue, Jul 01, 2014 at 04:34:48PM +, Mateo Lozano, Oscar wrote: -Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Tuesday, July 01, 2014 5:30 PM To: Mateo Lozano, Oscar Cc: Intel-gfx@lists.freedesktop.org; Madajczak, Tomasz Subject: Re: [Intel-gfx] pin OABUFFER to GGTT On Tue, Jul 01, 2014 at 04:24:56PM +, Mateo Lozano, Oscar wrote: Submitting again (this time copying the mailing list correctly): The bo_pin ioctl has been discarded in GEN6+ with this patch:    drm/i915: Reject the pin ioctl on gen6+ Especially with ppgtt this kinda stopped making sense. And if we    indeed need this to hack around an issue, we need something that also    works for non-root. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch The thing is, the performance team used this call to pin the OABUFFER to GGTT and then mapping it to userspace. This OABUFFER cannot be in PPGTT because: When each context has its own Per Process GTT, this field should be always set to GGTT. (BSpec dixit). Can we re-enable it? or should we find an alternative for this case? EXEC_OBJECT_NEEDS_GTT? -Chris The object (AFAICT, please Tomasz correct me if I am wrong) is not really used inside any batchbuffer. Then what's the issue? If you only use it as via a global gtt mapping it only exists in the ggtt. The issue is they need: A) A buffer object. B) Bound to GGTT. C) That userspace knows the GGTT offset of, so that they can program OABUFFER with it. D) That userspace can map so that they can read the reported counters. They used to create a bo, call bo_pin on it, use args-offset to program OABUFFER (via MI_LOAD_REGISTER_IMM, I imagine), map it and read the counter values. They cannot do this anymore. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] pin OABUFFER to GGTT
-Original Message- From: Mateo Lozano, Oscar Sent: Tuesday, July 01, 2014 6:14 PM To: 'Chris Wilson' Cc: Intel-gfx@lists.freedesktop.org; Madajczak, Tomasz; Rutkowski, Adam J; Jesse Barnes (jbar...@virtuousgeek.org) Subject: RE: [Intel-gfx] pin OABUFFER to GGTT -Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Tuesday, July 01, 2014 5:52 PM To: Mateo Lozano, Oscar Cc: Intel-gfx@lists.freedesktop.org; Madajczak, Tomasz; Rutkowski, Adam J; Jesse Barnes (jbar...@virtuousgeek.org) Subject: Re: [Intel-gfx] pin OABUFFER to GGTT On Tue, Jul 01, 2014 at 04:34:48PM +, Mateo Lozano, Oscar wrote: -Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Tuesday, July 01, 2014 5:30 PM To: Mateo Lozano, Oscar Cc: Intel-gfx@lists.freedesktop.org; Madajczak, Tomasz Subject: Re: [Intel-gfx] pin OABUFFER to GGTT On Tue, Jul 01, 2014 at 04:24:56PM +, Mateo Lozano, Oscar wrote: Submitting again (this time copying the mailing list correctly): The bo_pin ioctl has been discarded in GEN6+ with this patch:    drm/i915: Reject the pin ioctl on gen6+ Especially with ppgtt this kinda stopped making sense. And if we    indeed need this to hack around an issue, we need something that also    works for non-root. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch The thing is, the performance team used this call to pin the OABUFFER to GGTT and then mapping it to userspace. This OABUFFER cannot be in PPGTT because: When each context has its own Per Process GTT, this field should be always set to GGTT. (BSpec dixit). Can we re-enable it? or should we find an alternative for this case? EXEC_OBJECT_NEEDS_GTT? -Chris The object (AFAICT, please Tomasz correct me if I am wrong) is not really used inside any batchbuffer. Then what's the issue? If you only use it as via a global gtt mapping it only exists in the ggtt. The issue is they need: A) A buffer object. B) Bound to GGTT. C) That userspace knows the GGTT offset of, so that they can program OABUFFER with it. D) That userspace can map so that they can read the reported counters. They used to create a bo, call bo_pin on it, use args-offset to program OABUFFER (via MI_LOAD_REGISTER_IMM, I imagine), map it and read the counter values. They cannot do this anymore. The answer might be that all of this needs to be done by the kernel itself, but then we need to provide an interface to userspace... ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/16] Enabling GEN8 full PPGTT + fixes
Here be all the patches to make full PPGTT relatively stable on Broadwell. Most
of the work was actually to the generic PPGTT code, and not BDW specific. There
are basically 3 fixes:
1. Make the error state not horrible, but more work still needed.
2. Fix up another tricky from != from case in do_switch
3. Generally more graceful handling in ppgtt_release
1. Various forms of this subseries have shown up on the list from both
myself, and Chris (and I feel like Mika, also). For whatever reason,
none have been merged. I don't necessarily mind missing information, but
without these patches we can OOPs and GP fault in the error state, which
is not acceptable.
2. The meat of the debugging came here. Essentially this problem has
already been seen, and solved. The issue is, there is another spot in
do_switch that can invoke a switch to the default context. We probably
want to get slightly better handling of this, but for now this solves my
problems.
3. There are 2 categories addressed in this bucket. Reset, and signals. The
patches themselves explain the situation. I take a two step approach with this
where first I make things correct (and slow as heck), and then I do the more
optimal and trickier solution. Both of these are in line to be replaced by
Daniel, but I needed something sooner.
Here is the test case that caught all of the above issues:
while [ 1 ] ; do
(glxgears) pid[0]=$!
(glxgears) pid[1]=$!
(glxgears) pid[2]=$!
sleep 3
kill ${pid[*]}
done
Ben Widawsky (16):
drm/i915: Split up do_switch
drm/i915: Extract l3 remapping out of ctx switch
drm/i915/ppgtt: Load address space after mi_set_context
drm/i915: Fix another another use-after-free in do_switch
drm/i915/ctx: Return earlier on failure
drm/i915/error: Check the potential ctx obj's vm
drm/i915/error: vma error capture prettyify
drm/i915/error: Do a better job of disambiguating VMAs
drm/i915/error: Capture vmas instead of BOs
drm/i915: Add some extra guards in evict_vm
drm/i915: Make an uninterruptible evict
drm/i915: Reorder ctx unref on ppgtt cleanup
drm/i915: More correct (slower) ppgtt cleanup
drm/i915: Defer PPGTT cleanup
drm/i915/bdw: Enable full PPGTT
drm/i915: Get the error state over the wire (HACKish)
drivers/gpu/drm/i915/i915_debugfs.c| 2 +-
drivers/gpu/drm/i915/i915_drv.h| 18 ++-
drivers/gpu/drm/i915/i915_gem.c| 110 +
drivers/gpu/drm/i915/i915_gem_context.c| 238 ++---
drivers/gpu/drm/i915/i915_gem_evict.c | 39 +++--
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/i915_gem_gtt.c| 3 +-
drivers/gpu/drm/i915/i915_gem_gtt.h| 4 +
drivers/gpu/drm/i915/i915_gpu_error.c | 157 ---
drivers/gpu/drm/i915/i915_sysfs.c | 2 +-
10 files changed, 449 insertions(+), 126 deletions(-)
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/16] drm/i915: Fix another another use-after-free in do_switch
See the following for many more details.
commit acc240d41ea1ab9c488a79219fb313b5b46265ae
Author: Daniel Vetter daniel.vet...@ffwll.ch
Date: Thu Dec 5 15:42:34 2013 +0100
drm/i915: Fix use-after-free in do_switch
In this case, the issue is only for full PPGTT:
do_switch
context_unref
ppgtt_release
i915_gpu_idle
switch_to_default
from changes to default context
This could be backported to the pre do_switch cleanup I did in this
series. However, it's much cleaner and more obvious as a patch on top,
so I'd really like to do this as a post cleanup patch.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 601a58f..0e6e743 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -745,13 +745,21 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
from-obj-dirty = 1;
BUG_ON(from-obj-ring != ring);
- /* obj is kept alive until the next request by its active ref */
- i915_gem_object_ggtt_unpin(from-obj);
}
uninitialized = !to-is_initialized from == NULL;
to-is_initialized = true;
do_switch_fini_common(ring, from, to);
+ /* From may have disappeared again after thecontext unref */
+ from = ring-last_context;
+ if (from != NULL) {
+ /* obj is kept alive until the next request by its active ref.
+* XXX: The context needs to be unpinned last, or else we risk
+* hitting evict/idle on the ppgtt free, which will call back
+* into this, and we'll get a double unpin on this context
+*/
+ i915_gem_object_ggtt_unpin(from-obj);
+ }
if (uninitialized) {
ret = i915_gem_render_state_init(ring);
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/16] drm/i915/error: Check the potential ctx obj's vm
The bound list is global (all objects which back the VMAs are stored
here). Recently the BUG() in the offset lookup was demoted to a WARN,
but the fault actually lies in the caller, here.
This bug has existed since the initial introduction of PPGTT (however,
it was fixed in unmerged patches to fix up the error state).
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gpu_error.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 66cf417..550ba38 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -871,6 +871,9 @@ static void i915_gem_record_active_context(struct
intel_engine_cs *ring,
return;
list_for_each_entry(obj, dev_priv-mm.bound_list, global_list) {
+ if (!i915_gem_obj_ggtt_bound(obj))
+ continue;
+
if ((error-ccid PAGE_MASK) == i915_gem_obj_ggtt_offset(obj))
{
ering-ctx = i915_error_ggtt_object_create(dev_priv,
obj);
break;
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/16] drm/i915: Extract l3 remapping out of ctx switch
This is just a cosmetic change to try to put do_switch_rcs on a diet. As
it stands, the function was quite complex, and error prone.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 32
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 6dbe3e7..25cc889 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -623,6 +623,24 @@ static int do_switch_xcs(struct intel_engine_cs *ring,
return 0;
}
+static void remap_l3(struct intel_engine_cs *ring,
+struct intel_context *ctx)
+{
+ int ret, i;
+
+ for (i = 0; i MAX_L3_SLICES; i++) {
+ if (!(ctx-remap_slice (1i)))
+ continue;
+
+ ret = i915_gem_l3_remap(ring, i);
+ /* If it failed, try again next round */
+ if (ret)
+ DRM_DEBUG_DRIVER(L3 remapping failed\n);
+ else
+ ctx-remap_slice = ~(1i);
+ }
+}
+
static int do_switch_rcs(struct intel_engine_cs *ring,
struct intel_context *from,
struct intel_context *to)
@@ -631,7 +649,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
u32 hw_flags = 0;
bool uninitialized = false;
- int ret, i;
+ int ret;
if (from != NULL) {
BUG_ON(from-obj == NULL);
@@ -681,17 +699,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
if (ret)
goto unpin_out;
- for (i = 0; i MAX_L3_SLICES; i++) {
- if (!(to-remap_slice (1i)))
- continue;
-
- ret = i915_gem_l3_remap(ring, i);
- /* If it failed, try again next round */
- if (ret)
- DRM_DEBUG_DRIVER(L3 remapping failed\n);
- else
- to-remap_slice = ~(1i);
- }
+ remap_l3(ring, to);
/* The backing object for the context is done after switching to the
* *next* context. Therefore we cannot retire the previous context until
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/16] drm/i915: Add some extra guards in evict_vm
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_evict.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c
b/drivers/gpu/drm/i915/i915_gem_evict.c
index bbf4b12..38297d3 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -214,6 +214,7 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool
do_idle)
struct i915_vma *vma, *next;
int ret;
+ BUG_ON(!mutex_is_locked(vm-dev-struct_mutex));
trace_i915_gem_evict_vm(vm);
if (do_idle) {
@@ -222,11 +223,15 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool
do_idle)
return ret;
i915_gem_retire_requests(vm-dev);
+
+ WARN_ON(!list_empty(vm-active_list));
}
- list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list)
+ list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) {
+ WARN_ON(!i915_is_ggtt(vm) vma-pin_count);
if (vma-pin_count == 0)
WARN_ON(i915_vma_unbind(vma));
+ }
return 0;
}
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/16] drm/i915/ctx: Return earlier on failure
As what was correctly debugged here:
commit acc240d41ea1ab9c488a79219fb313b5b46265ae
Author: Daniel Vetter daniel.vet...@ffwll.ch
Date: Thu Dec 5 15:42:34 2013 +0100
drm/i915: Fix use-after-free in do_switch
It then becomes apparent that the default context cannot be the context
being switched to for context switch because it is always bound. It
follows that if the ring-last_context (from) has changed after the
bind_to_gtt, it will always be the default context - this is commented
in the code block.
This assertion will help catch issues without our logic sooner than
letting the system move long (which is possible for some time).
I really want this to be a BUG(), but I also want the patch to get
merged. I think the fact that none of the ERRNOs make any sense at all
is just more evidence that this shouldn't be a WARN.
//Cc: Ian Lister (don't have current email address)
Cc: Rafael Barbalho rafael.barba...@intel.com
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 0e6e743..cf7cf81 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -669,6 +669,15 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
*/
from = ring-last_context;
+ /* The only context which 'from' can be, if it was changed, is the
default
+* context. The default context cannot end up in evict everything (as
+* commented above) because it is always pinned.
+*/
+ if (WARN_ON(from == to)) {
+ ret = -EPERM;
+ goto unpin_out;
+ }
+
if (needs_pd_load) {
/* Older GENs still want the load first, PP_DCLV followed by
* PP_DIR_BASE register through Load Register Immediate commands
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/16] drm/i915/error: Capture vmas instead of BOs
To follow up on the last patch, we can now capture the VMAs instead of
the BOs. The hope if we get more accurate error capture while debugging
PPGTT.
Note that this does not impact the previous argument about whether to
capture all VMAs, or just the guilty VMA. This merely allows the code to
do whatever we chose later.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gpu_error.c | 53 +++
2 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cea596..d3a69aa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -305,6 +305,7 @@ struct drm_i915_error_state {
char error_msg[128];
u32 reset_count;
u32 suspend_count;
+ u32 vm_count;
/* Generic register state */
u32 eir;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 123a4fc..e82e590 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -384,15 +384,19 @@ int i915_error_state_to_str(struct
drm_i915_error_state_buf *m,
i915_ring_error_state(m, dev, error-ring[i]);
}
- if (error-active_bo)
- print_error_buffers(m, Active,
- error-active_bo[0],
- error-active_bo_count[0]);
+ for (i = 0; i error-vm_count; i++) {
+ if (error-active_bo[i])
+ print_error_buffers(m, Active,
+ error-active_bo[i],
+ error-active_bo_count[i]);
+ }
- if (error-pinned_bo)
- print_error_buffers(m, Pinned,
- error-pinned_bo[0],
- error-pinned_bo_count[0]);
+ for (i = 0; i error-vm_count; i++) {
+ if (error-pinned_bo[i])
+ print_error_buffers(m, Pinned,
+ error-pinned_bo[i],
+ error-pinned_bo_count[i]);
+ }
for (i = 0; i ARRAY_SIZE(error-ring); i++) {
struct drm_i915_error_object *obj;
@@ -623,22 +627,23 @@ unwind:
i915_error_object_create_sized((dev_priv), (src),
(dev_priv)-gtt.base, \
(src)-base.sizePAGE_SHIFT)
-static void capture_bo(struct drm_i915_error_buffer *err,
- struct drm_i915_gem_object *obj)
+static void capture_vma(struct drm_i915_error_buffer *err, struct i915_vma
*vma)
{
+ struct drm_i915_gem_object *obj = vma-obj;
+
err-size = obj-base.size;
err-name = obj-base.name;
err-rseqno = obj-last_read_seqno;
err-wseqno = obj-last_write_seqno;
- err-gtt_offset = i915_gem_obj_ggtt_offset(obj);
+ err-gtt_offset = vma-node.start;
err-read_domains = obj-base.read_domains;
err-write_domain = obj-base.write_domain;
err-fence_reg = obj-fence_reg;
- err-pinned = 0;
- if (i915_gem_obj_is_pinned(obj))
- err-pinned = 1;
- if (obj-user_pin_count 0)
+ if (obj-user_pin_count 0) {
+ WARN_ON(i915_is_ggtt(vma-vm));
err-pinned = -1;
+ } else
+ err-pinned = !!vma-pin_count;
err-tiling = obj-tiling_mode;
err-dirty = obj-dirty;
err-purgeable = obj-madv != I915_MADV_WILLNEED;
@@ -654,7 +659,7 @@ static u32 capture_active_bo(struct drm_i915_error_buffer
*err,
int i = 0;
list_for_each_entry(vma, head, mm_list) {
- capture_bo(err++, vma-obj);
+ capture_vma(err++, vma);
if (++i == count)
break;
}
@@ -669,10 +674,10 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer
*err,
int i = 0;
list_for_each_entry(vma, head, pin_capture_link) {
- if (!i915_gem_obj_is_pinned(vma-obj))
+ if (!vma-pin_count)
continue;
- capture_bo(err++, vma-obj);
+ capture_vma(err++, vma);
if (++i == count)
break;
}
@@ -1034,16 +1039,16 @@ static void i915_gem_capture_buffers(struct
drm_i915_private *dev_priv,
struct drm_i915_error_state *error)
{
struct i915_address_space *vm;
- int vm_count = 0, i = 0;
+ int i = 0;
list_for_each_entry(vm, dev_priv-vm_list, global_link)
- vm_count++;
+ error-vm_count++;
- error-active_bo = kcalloc(vm_count, sizeof(*error-active_bo),
GFP_ATOMIC);
- error-pinned_bo = kcalloc(vm_count, sizeof(*error-pinned_bo),
GFP_ATOMIC);
-
[Intel-gfx] [PATCH 03/16] drm/i915/ppgtt: Load address space after mi_set_context
The simple explanation is, the docs say to do this for GEN8. Perhaps we
want to do this for GEN7 too, I am not certain.
PDPs are saved and restored with context. Contexts (without execlists)
only exist on the render ring. The docs say that PDPs are not power
context save/restored. I've learned that this actually means something
which SW doesn't care about. So pretend the statement doesn't exist.
For non RCS, nothing changes.
All this patch now does is change the ordering of LRI vs MI_SET_CONTEXT
for the initialization of the context. I do this because the docs say to
do it, and frankly, I cannot reason why it is necessary. I've thought
about it a lot, and tried, without success, to get a reason from design.
The answer I got more or less says, gen7 is different than gen8. I've
given up, and am adding this little bit of code to make it in sync with
the docs.
v2: Completely rewritten commit message that addresses the requests
Ville made for v1
Only load PDPs for initial context load (Ville)
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 29 +++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 25cc889..601a58f 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -649,6 +649,7 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
u32 hw_flags = 0;
bool uninitialized = false;
+ bool needs_pd_load = (INTEL_INFO(ring-dev)-gen 8)
USES_FULL_PPGTT(ring-dev);
int ret;
if (from != NULL) {
@@ -668,7 +669,10 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
*/
from = ring-last_context;
- if (USES_FULL_PPGTT(ring-dev)) {
+ if (needs_pd_load) {
+ /* Older GENs still want the load first, PP_DCLV followed by
+* PP_DIR_BASE register through Load Register Immediate commands
+* in Ring Buffer before submitting a context.*/
ret = ppgtt-switch_mm(ppgtt, ring, false);
if (ret)
goto unpin_out;
@@ -692,13 +696,34 @@ static int do_switch_rcs(struct intel_engine_cs *ring,
vma-bind_vma(vma, to-obj-cache_level, GLOBAL_BIND);
}
- if (!to-is_initialized || i915_gem_context_is_default(to))
+ if (!to-is_initialized || i915_gem_context_is_default(to)) {
hw_flags |= MI_RESTORE_INHIBIT;
+ needs_pd_load = USES_FULL_PPGTT(ring-dev)
IS_GEN8(ring-dev);
+ }
ret = mi_set_context(ring, to, hw_flags);
if (ret)
goto unpin_out;
+ /* GEN8 does *not* require an explicit reload if the PDPs have been
+* setup, and we do not wish to move them.
+*
+* XXX: If we implemented page directory eviction code, this
+* optimization needs to be removed.
+*/
+ if (needs_pd_load) {
+ ret = ppgtt-switch_mm(ppgtt, ring, false);
+ /* The hardware context switch is emitted, but we haven't
+* actually changed the state - so it's probably safe to bail
+* here. Still, let the user know something dangerous has
+* happened.
+*/
+ if (ret) {
+ DRM_ERROR(Failed to change address space on context
switch\n);
+ goto unpin_out;
+ }
+ }
+
remap_l3(ring, to);
/* The backing object for the context is done after switching to the
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/16] drm/i915: Make an uninterruptible evict
There are no users of this yet, but the idea is presented and split out
to find bugs.
Also, while here, return -ERESTARTSYS to the caller, in case they want
to do something with it.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_drv.h| 2 +-
drivers/gpu/drm/i915/i915_gem_context.c| 5 ++---
drivers/gpu/drm/i915/i915_gem_evict.c | 32 ++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
4 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d3a69aa..6c252c3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2478,7 +2478,7 @@ int __must_check i915_gem_evict_something(struct
drm_device *dev,
unsigned long start,
unsigned long end,
unsigned flags);
-int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
+int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle, bool
interruptible);
int i915_gem_evict_everything(struct drm_device *dev);
/* belongs in i915_gem_gtt.h */
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index cf7cf81..b6803b3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -121,11 +121,10 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
if (WARN_ON(list_empty(vma-vma_link) ||
list_is_singular(vma-vma_link)))
break;
-
- i915_gem_evict_vm(ppgtt-base, true);
+ i915_gem_evict_vm(ppgtt-base, true, true);
} else {
i915_gem_retire_requests(dev);
- i915_gem_evict_vm(ppgtt-base, false);
+ i915_gem_evict_vm(ppgtt-base, false, true);
}
ppgtt-base.cleanup(ppgtt-base);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c
b/drivers/gpu/drm/i915/i915_gem_evict.c
index 38297d3..ac8d90f 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -197,8 +197,9 @@ found:
/**
* i915_gem_evict_vm - Evict all idle vmas from a vm
*
- * @vm: Address space to cleanse
- * @do_idle: Boolean directing whether to idle first.
+ * @vm:Address space to cleanse
+ * @do_idle: Boolean directing whether to idle first.
+ * @interruptible: How to wait
*
* This function evicts all idles vmas from a vm. If all unpinned vmas should
be
* evicted the @do_idle needs to be set to true.
@@ -209,18 +210,24 @@ found:
* To clarify: This is for freeing up virtual address space, not for freeing
* memory in e.g. the shrinker.
*/
-int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
+int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle, bool
interruptible)
{
+ struct drm_i915_private *dev_priv = to_i915(vm-dev);
+
struct i915_vma *vma, *next;
+ bool was_intr = dev_priv-mm.interruptible;
int ret;
BUG_ON(!mutex_is_locked(vm-dev-struct_mutex));
trace_i915_gem_evict_vm(vm);
+ if (!interruptible)
+ dev_priv-mm.interruptible = false;
+
if (do_idle) {
ret = i915_gpu_idle(vm-dev);
if (ret)
- return ret;
+ goto out;
i915_gem_retire_requests(vm-dev);
@@ -229,11 +236,20 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool
do_idle)
list_for_each_entry_safe(vma, next, vm-inactive_list, mm_list) {
WARN_ON(!i915_is_ggtt(vm) vma-pin_count);
- if (vma-pin_count == 0)
- WARN_ON(i915_vma_unbind(vma));
+ if (vma-pin_count == 0) {
+ ret = i915_vma_unbind(vma);
+ if (ret == -ERESTARTSYS) {
+ BUG_ON(!interruptible);
+ goto out;
+ } else if (ret)
+ WARN(1, Failed to unbind vma %d\n, ret);
+ }
}
+ ret = 0;
- return 0;
+out:
+ dev_priv-mm.interruptible = was_intr;
+ return ret;
}
/**
@@ -276,7 +292,7 @@ i915_gem_evict_everything(struct drm_device *dev)
/* Having flushed everything, unbind() should never raise an error */
list_for_each_entry(vm, dev_priv-vm_list, global_link)
- WARN_ON(i915_gem_evict_vm(vm, false));
+ WARN_ON(i915_gem_evict_vm(vm, false, true));
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d815ef5..40ec61c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++
[Intel-gfx] [PATCH 12/16] drm/i915: Reorder ctx unref on ppgtt cleanup
The comment [which was mine] is wrong. The context object can never be bound in a PPGTT because it is only capable of living in the Global GTT. So, remove the comment, and reorder the unref. What's nice about the latter is it keeps the context object alive past the PPGTT. This makes the destroy ordering symmetric with the creation ordering. Create: 1. Create context 2. Create PPGTT Destroy: 1. Destroy PPGTT 2. Destroy context As far as I know, this does not fix a bug. The code previously kept the context data structure, only the object was gone. As the code was, nothing tried to use the object after this point. NOTE: If in the future we have cases where the PPGTT can/should outlive the context (which doesn't occur today, but the code permits it), this ordering does not matter. Even if this occurs, as it stands now, we do not expect that to be the normal case, and having this order makes debugging a bit easier if we're tracking object lifetimes for the context vs ppgtt Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_context.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index b6803b3..8d106d9 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -185,14 +185,12 @@ void i915_gem_context_free(struct kref *ctx_ref) /* We refcount even the aliasing PPGTT to keep the code symmetric */ if (USES_PPGTT(ctx-obj-base.dev)) ppgtt = ctx_to_ppgtt(ctx); - - /* XXX: Free up the object before tearing down the address space, in -* case we're bound in the PPGTT */ - drm_gem_object_unreference(ctx-obj-base); } if (ppgtt) kref_put(ppgtt-ref, ppgtt_release); + if (ctx-obj) + drm_gem_object_unreference(ctx-obj-base); list_del(ctx-link); kfree(ctx); } -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 16/16] drm/i915: Get the error state over the wire (HACKish)
I was dealing with a bug recently where the system would hard hang
somewhere between hangcheck and reset. There was time after error
collection to actually get my error state out, but I couldn't get the
reads to work.
This patch is also useful for when reset kills the machine, and you want
to keep reset enabled but still get error state.
Since I found the patch pretty useful, I decided to clean it up and
submit it. It was mostly meant as a one-off hack originally though.
If a maintainer decides it's useful, then here it is.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
drivers/gpu/drm/i915/i915_drv.h | 3 ++-
drivers/gpu/drm/i915/i915_gpu_error.c | 31 +--
drivers/gpu/drm/i915/i915_sysfs.c | 2 +-
4 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 6b7b32b..2daad46 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -929,7 +929,7 @@ static ssize_t i915_error_state_read(struct file *file,
char __user *userbuf,
if (ret)
return ret;
- ret = i915_error_state_to_str(error_str, error_priv);
+ ret = i915_error_state_to_str(error_str, error_priv-dev,
error_priv-error);
if (ret)
goto out;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1045006..b6a4f1e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2544,7 +2544,8 @@ static inline void intel_display_crc_init(struct
drm_device *dev) {}
__printf(2, 3)
void i915_error_printf(struct drm_i915_error_state_buf *e, const char *f, ...);
int i915_error_state_to_str(struct drm_i915_error_state_buf *estr,
- const struct i915_error_state_file_priv *error);
+ struct drm_device *dev,
+ const struct drm_i915_error_state *error);
int i915_error_state_buf_init(struct drm_i915_error_state_buf *eb,
size_t count, loff_t pos);
static inline void i915_error_state_buf_release(
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index e82e590..1540bf6 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -184,8 +184,22 @@ static void i915_error_puts(struct
drm_i915_error_state_buf *e,
__i915_error_advance(e, len);
}
-#define err_printf(e, ...) i915_error_printf(e, __VA_ARGS__)
-#define err_puts(e, s) i915_error_puts(e, s)
+
+static bool wire = false;
+#define err_printf(e, ...) do {\
+ if (wire) { \
+ printk(__VA_ARGS__);\
+ } else {\
+ i915_error_printf(e, __VA_ARGS__); \
+ } \
+} while (0)
+#define err_puts(e, s) do {\
+ if (wire) { \
+ printk(s); \
+ } else {\
+ i915_error_puts(e, s); \
+ } \
+} while (0)
static void print_error_buffers(struct drm_i915_error_state_buf *m,
const char *name,
@@ -240,7 +254,7 @@ 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_ring *ring)
+ const struct drm_i915_error_ring *ring)
{
if (!ring-valid)
return;
@@ -322,11 +336,10 @@ static void print_error_obj(struct
drm_i915_error_state_buf *m,
}
int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
- const struct i915_error_state_file_priv *error_priv)
+ struct drm_device *dev,
+ const struct drm_i915_error_state *error)
{
- struct drm_device *dev = error_priv-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
- struct drm_i915_error_state *error = error_priv-error;
int i, j, offset, elt;
int max_hangcheck_score;
@@ -1197,6 +1210,12 @@ void i915_capture_error_state(struct drm_device *dev,
bool wedged,
spin_lock_irqsave(dev_priv-gpu_error.lock, flags);
if (dev_priv-gpu_error.first_error == NULL) {
dev_priv-gpu_error.first_error = error;
+#ifdef PUSH_TO_WIRE
+ /* Probably racy, but this is emergency debug */
+ wire = true;
+ i915_error_state_to_str(NULL, dev, error);
[Intel-gfx] [PATCH 15/16] drm/i915/bdw: Enable full PPGTT
Broadwell is perfectly capable of full PPGTT. I've been using it for
some time, and seen no especially ill effects.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
Conflicts:
drivers/gpu/drm/i915/i915_drv.h
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c23213d..1045006 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2003,7 +2003,7 @@ struct drm_i915_cmd_table {
#define HAS_HW_CONTEXTS(dev) (INTEL_INFO(dev)-gen = 6)
#define HAS_ALIASING_PPGTT(dev)(INTEL_INFO(dev)-gen = 6)
-#define HAS_PPGTT(dev) (INTEL_INFO(dev)-gen = 7 !IS_GEN8(dev))
+#define HAS_PPGTT(dev) (INTEL_INFO(dev)-gen = 7
!IS_CHERRYVIEW(dev))
#define USES_PPGTT(dev)intel_enable_ppgtt(dev, false)
#define USES_FULL_PPGTT(dev) intel_enable_ppgtt(dev, true)
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/16] drm/i915/error: vma error capture prettyify
Rename some variables, and clean up the code a bit to make things
clearer in our error capture.
There isn't an intentional functional change here.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gpu_error.c | 55 ---
1 file changed, 32 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index 550ba38..ebe2904 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -967,63 +967,72 @@ static void i915_gem_record_rings(struct drm_device *dev,
}
}
-/* FIXME: Since pin count/bound list is global, we duplicate what we capture
per
+/**
+ * i915_gem_capture_vm() - Capture a VMs error state.
+ * @error: The main error structure
+ * @vm:The address space we're capturing.
+ * @vm_ndx:Which vm without the buffer array
+ *
+ * FIXME: Since pin count/bound list is global, we duplicate what we capture
per
* VM.
*/
static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
struct drm_i915_error_state *error,
struct i915_address_space *vm,
- const int ndx)
+ const int vm_ndx)
{
struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
- int i;
+ int active_vma_count = 0;
- i = 0;
list_for_each_entry(vma, vm-active_list, mm_list)
- i++;
- error-active_bo_count[ndx] = i;
+ active_vma_count++;
+
+ error-active_bo_count[vm_ndx] = active_vma_count;
+
list_for_each_entry(obj, dev_priv-mm.bound_list, global_list)
if (i915_gem_obj_is_pinned(obj))
- i++;
- error-pinned_bo_count[ndx] = i - error-active_bo_count[ndx];
+ active_vma_count++;
+
+ /* XXX: this is an incorrect measurement of pinned BOs */
+ error-pinned_bo_count[vm_ndx] = active_vma_count -
error-active_bo_count[vm_ndx];
- if (i) {
- active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
+ if (active_vma_count) {
+ active_bo = kcalloc(active_vma_count, sizeof(*active_bo),
GFP_ATOMIC);
if (active_bo)
- pinned_bo = active_bo + error-active_bo_count[ndx];
+ pinned_bo = active_bo + error-active_bo_count[vm_ndx];
}
if (active_bo)
- error-active_bo_count[ndx] =
+ error-active_bo_count[vm_ndx] =
capture_active_bo(active_bo,
- error-active_bo_count[ndx],
+ error-active_bo_count[vm_ndx],
vm-active_list);
if (pinned_bo)
- error-pinned_bo_count[ndx] =
+ error-pinned_bo_count[vm_ndx] =
capture_pinned_bo(pinned_bo,
- error-pinned_bo_count[ndx],
+ error-pinned_bo_count[vm_ndx],
dev_priv-mm.bound_list);
- error-active_bo[ndx] = active_bo;
- error-pinned_bo[ndx] = pinned_bo;
+ error-active_bo[vm_ndx] = active_bo;
+ error-pinned_bo[vm_ndx] = pinned_bo;
}
static void i915_gem_capture_buffers(struct drm_i915_private *dev_priv,
struct drm_i915_error_state *error)
{
struct i915_address_space *vm;
- int cnt = 0, i = 0;
+ int vm_count = 0, i = 0;
list_for_each_entry(vm, dev_priv-vm_list, global_link)
- cnt++;
+ vm_count++;
- error-active_bo = kcalloc(cnt, sizeof(*error-active_bo), GFP_ATOMIC);
- error-pinned_bo = kcalloc(cnt, sizeof(*error-pinned_bo), GFP_ATOMIC);
- error-active_bo_count = kcalloc(cnt, sizeof(*error-active_bo_count),
+ error-active_bo = kcalloc(vm_count, sizeof(*error-active_bo),
GFP_ATOMIC);
+ error-pinned_bo = kcalloc(vm_count, sizeof(*error-pinned_bo),
GFP_ATOMIC);
+ error-active_bo_count = kcalloc(vm_count,
sizeof(*error-active_bo_count),
GFP_ATOMIC);
- error-pinned_bo_count = kcalloc(cnt, sizeof(*error-pinned_bo_count),
+ error-pinned_bo_count = kcalloc(vm_count,
sizeof(*error-pinned_bo_count),
GFP_ATOMIC);
list_for_each_entry(vm, dev_priv-vm_list, global_link)
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/16] drm/i915/error: Do a better job of disambiguating VMAs
Some of the original PPGTT patches in this area where unmerged, and this
left a lot of confusion in our error capture with regard to which vm/obj
we want to capture. There have been at least a couple of patches from
Chris, and myself to try to fix this up; so here is another shot. Nobody
running without full PPGTT is effected by this, and that is probably why
nobody has bothered to fix it yet.
Instead of using any of the global lists to find the VMAs we want to
capture, we use the union of the active, and the inactive list in the
VM. This allows us to replace our capture_bo with capture_vma, and know
all the VMAs we want to capture are valid.
I could have probably figured out a way to reuse mm_list. As we've had
bugs here before in the shrinker, I think the best way forward is to get
it working, and then optimize it later.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_gtt.c | 1 +
drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++
drivers/gpu/drm/i915/i915_gpu_error.c | 39 ++-
3 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a4153ee..88451dc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2114,6 +2114,7 @@ static struct i915_vma *__i915_gem_vma_create(struct
drm_i915_gem_object *obj,
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(vma-vma_link);
+ INIT_LIST_HEAD(vma-pin_capture_link);
INIT_LIST_HEAD(vma-mm_list);
INIT_LIST_HEAD(vma-exec_list);
vma-vm = vm;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8d6f7c1..1d75801 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -126,6 +126,8 @@ struct i915_vma {
struct list_head vma_link; /* Link in the object's VMA list */
+ struct list_head pin_capture_link; /* Link in the error capture */
+
/** This vma's place in the batchbuffer or on the eviction list */
struct list_head exec_list;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index ebe2904..123a4fc 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -665,14 +665,14 @@ static u32 capture_active_bo(struct drm_i915_error_buffer
*err,
static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
int count, struct list_head *head)
{
- struct drm_i915_gem_object *obj;
+ struct i915_vma *vma;
int i = 0;
- list_for_each_entry(obj, head, global_list) {
- if (!i915_gem_obj_is_pinned(obj))
+ list_for_each_entry(vma, head, pin_capture_link) {
+ if (!i915_gem_obj_is_pinned(vma-obj))
continue;
- capture_bo(err++, obj);
+ capture_bo(err++, vma-obj);
if (++i == count)
break;
}
@@ -982,21 +982,32 @@ static void i915_gem_capture_vm(struct drm_i915_private
*dev_priv,
const int vm_ndx)
{
struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
- struct drm_i915_gem_object *obj;
struct i915_vma *vma;
int active_vma_count = 0;
+ int vma_pin_count = 0;
+ LIST_HEAD(pinned_vma);
- list_for_each_entry(vma, vm-active_list, mm_list)
+ list_for_each_entry(vma, vm-active_list, mm_list) {
active_vma_count++;
+ if (vma-pin_count) {
+ vma_pin_count++;
+ list_move_tail(vma-pin_capture_link, pinned_vma);
+ }
+ }
- error-active_bo_count[vm_ndx] = active_vma_count;
-
- list_for_each_entry(obj, dev_priv-mm.bound_list, global_list)
- if (i915_gem_obj_is_pinned(obj))
- active_vma_count++;
+ list_for_each_entry(vma, vm-inactive_list, mm_list) {
+ /* Certain objects may be on the inactive list, but pinned, when
+* in the global GGTT. */
+ if (WARN_ON(!i915_is_ggtt(vm)
+ vma-pin_count
+ !(vma-exec_entry-flags (131 { /* FIXME:
need the actual flag */
+ vma_pin_count++;
+ list_move_tail(vma-pin_capture_link, pinned_vma);
+ }
+ }
- /* XXX: this is an incorrect measurement of pinned BOs */
- error-pinned_bo_count[vm_ndx] = active_vma_count -
error-active_bo_count[vm_ndx];
+ error-active_bo_count[vm_ndx] = active_vma_count;
+ error-pinned_bo_count[vm_ndx] = vma_pin_count;
if (active_vma_count) {
active_bo = kcalloc(active_vma_count, sizeof(*active_bo),
GFP_ATOMIC);
@@ -1014,7 +1025,7 @@ static void i915_gem_capture_vm(struct
[Intel-gfx] [PATCH 14/16] drm/i915: Defer PPGTT cleanup
The last patch made PPGTT free cases correct. It left a major problem
though where in many cases it was possible to have to idle the GPU in
order to destroy a VM. This is really unfortunate as it is stalling the
active GPU process for the dying GPU process.
The workqueue grew very tricky. I left in my original wait based version
as #if 0, and used Chris' recommendation with the seqno check. I haven't
measure one vs the other, but I am in favor of the code as it is. I am
just leaving the old code for people to observe.
NOTE: I somewhat expect this patch to not get merged because of work in
the pipeline. I won't argue much against that, and I'll simply say, this
solves a real problem, or platforms running with PPGTT. PPGTT is not
enabled by default yet, and I therefore see little harm in merging this.
Because I was expecting this to not get merged, I did not spent much
time investigating any corner cases, in particular, cases where I need
to cleanup the wq. If this patch does head in the merge direction, I
will take a closer look at that.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_drv.h | 10 +++
drivers/gpu/drm/i915/i915_gem.c | 110
drivers/gpu/drm/i915/i915_gem_context.c | 40
drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
drivers/gpu/drm/i915/i915_gem_gtt.h | 2 +
5 files changed, 150 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6c252c3..c23213d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1077,6 +1077,15 @@ struct i915_gem_mm {
struct delayed_work idle_work;
/**
+* PPGTT freeing often happens during interruptible times (fd close,
+* execbuf, busy_ioctl). It therefore becomes difficult to clean up the
+* PPGTT when the refcount reaches 0 if a signal comes in. This
+* workqueue defers the cleanup of the VM to a later time, and allows
+* userspace to continue on.
+*/
+ struct delayed_work ppgtt_work;
+
+ /**
* Are we in a non-interruptible section of code like
* modesetting?
*/
@@ -1461,6 +1470,7 @@ struct drm_i915_private {
struct mutex modeset_restore_lock;
struct list_head vm_list; /* Global list of all address spaces */
+ struct list_head ppgtt_free_list;
struct i915_gtt gtt; /* VM representing the global address space */
struct i915_gem_mm mm;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f6d1238..561bd64 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2707,6 +2707,112 @@ i915_gem_idle_work_handler(struct work_struct *work)
intel_mark_idle(dev_priv-dev);
}
+static void
+i915_gem_ppgtt_work_handler(struct work_struct *work)
+{
+ struct drm_i915_private *dev_priv =
+ container_of(work, typeof(*dev_priv), mm.ppgtt_work.work);
+ struct i915_address_space *vm, *v;
+#if 0
+ unsigned reset_counter;
+ int ret;
+#endif
+
+ if (!mutex_trylock(dev_priv-dev-struct_mutex)) {
+ queue_delayed_work(dev_priv-wq, dev_priv-mm.ppgtt_work,
+ round_jiffies_up_relative(HZ));
+ return;
+ }
+
+ list_for_each_entry_safe(vm, v, dev_priv-ppgtt_free_list,
global_link) {
+ struct i915_hw_ppgtt *ppgtt = container_of(vm, struct
i915_hw_ppgtt, base);
+ struct i915_vma *vma = NULL, *vma_active = NULL, *vma_inactive
= NULL;
+
+ /* The following attempts to find the newest (most recently
+* activated/inactivated) VMA.
+*/
+ if (!list_empty(ppgtt-base.active_list)) {
+ vma_active = list_last_entry(ppgtt-base.active_list,
+typeof(*vma), mm_list);
+ }
+ if (!list_empty(ppgtt-base.inactive_list)) {
+ vma_inactive =
list_last_entry(ppgtt-base.inactive_list,
+ typeof(*vma), mm_list);
+ }
+
+ if (vma_active)
+ vma = vma_active;
+ else if (vma_inactive)
+ vma = vma_inactive;
+
+ /* Sanity check */
+ if (vma_active vma_inactive) {
+ if (WARN_ON(vma_active-retire_seqno =
vma_inactive-retire_seqno))
+ vma = vma_inactive;
+ }
+
+ if (!vma)
+ goto finish;
+
+ /* Another sanity check */
+ if (WARN_ON(!vma-retire_seqno))
+ continue;
+
+ /* NOTE: We could wait here for the seqno, but that makes things
+* significantly more
[Intel-gfx] [PATCH 13/16] drm/i915: More correct (slower) ppgtt cleanup
If a VM still have objects which are bound (exactly: have a node
reserved in the drm_mm), and we are in the middle of a reset, we have no
hope of the standard methods fixing the situation (ring idle won't
work). We must therefore let the reset handler take it's course, and
then we can resume tearing down the VM.
This logic very much duplicates^Wresembles the logic in our wait for
error code. I've decided to leave it as open coded because I expect this
bit of code to require tweaks and changes over time.
Interruptions via signal causes a really similar problem.
This should deprecate the need for the yet unmerged patch from Chris
(and an identical patch from me, which was first!!):
drm/i915: Prevent signals from interrupting close()
I have a followup patch to implement deferred free, before you complain.
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 51 +++--
1 file changed, 48 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 8d106d9..e1b5613 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -101,6 +101,32 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
struct drm_device *dev = ppgtt-base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct i915_address_space *vm = ppgtt-base;
+ bool do_idle = false;
+ int ret;
+
+ /* If we get here while in reset, we need to let the reset handler run
+* first, or else our VM teardown isn't going to go smoothly. There are
+* a could of options at this point, but letting the reset handler do
+* it's thing is the most desirable. The reset handler will take care of
+* retiring the stuck requests.
+*/
+ if (i915_reset_in_progress(dev_priv-gpu_error)) {
+ mutex_unlock(dev-struct_mutex);
+#define EXIT_COND (!i915_reset_in_progress(dev_priv-gpu_error) || \
+ i915_terminally_wedged(dev_priv-gpu_error))
+ ret = wait_event_timeout(dev_priv-gpu_error.reset_queue,
+EXIT_COND,
+10 * HZ);
+ if (!ret) {
+ /* it's unlikely idling will solve anything, but it
+* shouldn't hurt to try. */
+ do_idle = true;
+ /* TODO: go down kicking and screaming harder */
+ }
+#undef EXIT_COND
+
+ mutex_lock(dev-struct_mutex);
+ }
if (ppgtt == dev_priv-mm.aliasing_ppgtt ||
(list_empty(vm-active_list) list_empty(vm-inactive_list))) {
@@ -117,14 +143,33 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
if (!list_empty(vm-active_list)) {
struct i915_vma *vma;
+ do_idle = true;
list_for_each_entry(vma, vm-active_list, mm_list)
if (WARN_ON(list_empty(vma-vma_link) ||
list_is_singular(vma-vma_link)))
break;
- i915_gem_evict_vm(ppgtt-base, true, true);
- } else {
+ } else
i915_gem_retire_requests(dev);
- i915_gem_evict_vm(ppgtt-base, false, true);
+
+ /* We have a problem here where VM teardown cannot be interrupted, or
+* else the ppgtt cleanup will fail. As an example, a precisely timed
+* SIGKILL could leads to an OOPS, or worse. There are two options:
+* 1. Make the eviction uninterruptible
+* 2. Defer the eviction if it was interrupted.
+*
+* Option #1 is not the friendliest, but it's the easiest to implement,
+* and least error prone.
+* TODO: Implement option 2
+*/
+ ret = i915_gem_evict_vm(ppgtt-base, do_idle, !do_idle);
+ if (ret == -ERESTARTSYS)
+ ret = i915_gem_evict_vm(ppgtt-base, do_idle, false);
+ WARN_ON(ret);
+ WARN_ON(!list_empty(vm-active_list));
+
+ /* This is going to blow up badly if the mm is unclean */
+ if (WARN_ON(!list_empty(ppgtt-base.mm.head_node.node_list))) {
+ /* TODO: go down kicking and screaming harder++ */
}
ppgtt-base.cleanup(ppgtt-base);
--
2.0.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] how to build intel-gpu-tools without cairo
The latest intel-gpu-tools has dependency on cairo. We don't use cairo. Is there any way to build intel-gpu-tools without cairo? Thank you so much for your help Ying ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] pin OABUFFER to GGTT
On Tue, Jul 01, 2014 at 05:16:30PM +, Mateo Lozano, Oscar wrote: The issue is they need: A) A buffer object. B) Bound to GGTT. C) That userspace knows the GGTT offset of, so that they can program OABUFFER with it. D) That userspace can map so that they can read the reported counters. They used to create a bo, call bo_pin on it, use args-offset to program OABUFFER (via MI_LOAD_REGISTER_IMM, I imagine), map it and read the counter values. They cannot do this anymore. The answer might be that all of this needs to be done by the kernel itself, but then we need to provide an interface to userspace... Yes. If you need to pin a buffer for a register, then it needs to be handled by the kernel. Especially one that provides information about other users. -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 00/19] ddi: respin of runtime PM for DPMS
2014-06-25 16:01 GMT-03:00 Imre Deak imre.d...@intel.com: This is a respin of the unmerged part of Daniel's runtime PM for DPMS patchset [1]. The original one also included a refactoring of the DDI PCH/CRT encoder modesetting path, I left the corresponding patches out from this series. This is because there hasn't been yet an agreement on those parts, but people would like to see the RPM DPMS support already applied. Some patches needed to be updated/rebased because of the above omission, but these weren't anywhere significant so I just marked the fact with my s-o-b line. I also added two minor change to keep the modeset sequence at its current order and collected all the reviewed-by lines. Tested on HSW DP/VGA, with basic DPMS on/off and igt/pm_rpm. For patches 2, 4, 5, 6, 7, 19: Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com However, I tested these patches on a HSW Machine with eDP+HDMI, and there are WARNs on dmesg for the dpms-non-lpsp subtest. I found at least two problems: 1 - Function hsw_ddi_pll_get_hw_state() reads registers while the device is suspended. 2 - When _intel_set_mode() calls intel_crtc_disable(), it calls assert_plane() which reads register 0x71180, which triggers an Unclaimed register error. I didn't investigate this deeply, so I don't have a suggestion for a solution. I can reproduce these errors 100% of the time. [1] http://lists.freedesktop.org/archives/intel-gfx/2014-April/044179.html Daniel Vetter (17): drm/i915: Check hw state in assert_can_disable_lcpll drm/i915: Remove spll_refcount for hsw drm/i915: Clean up WRPLL/SPLL #defines drm/i915: Move the SPLL enabling into hsw_crt_pre_enable drm/i915: Move SPLL disabling into hsw_crt_post_disable drm/i915: Add a debugfs file for the shared dpll state drm/i915: Move ddi_pll_sel into the pipe config drm/i915: State readout and cross-checking for ddi_pll_sel drm/i915: Precompute static ddi_pll_sel values in encoders drm/i915: Basic shared dpll support for WRPLLs drm/i915: Document that the pll-mode_set hook is optional drm/i915: State readout support for WRPLLs drm/i915: -disable hook for WRPLLs drm/i915: -enable hook for WRPLLs drm/i915: Switch to common shared dpll framework for WRPLLs drm/i915: Only touch WRPLL hw state in enable/disable hooks drm/i915: ddi: enable runtime pm during dpms Imre Deak (2): drm/i915: ddi: move pch setup after encoder-pre_enable drm/i915: ddi: move pch cleanup before encoder-post_disable drivers/gpu/drm/i915/i915_debugfs.c | 27 +++ drivers/gpu/drm/i915/i915_drv.h | 16 +- drivers/gpu/drm/i915/i915_reg.h | 10 +- drivers/gpu/drm/i915/intel_crt.c | 32 +++- drivers/gpu/drm/i915/intel_ddi.c | 340 +++ drivers/gpu/drm/i915/intel_display.c | 157 +--- drivers/gpu/drm/i915/intel_dp.c | 23 ++- drivers/gpu/drm/i915/intel_drv.h | 14 +- 8 files changed, 258 insertions(+), 361 deletions(-) -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Introduce dual_bsd_ring parameter.
On Wed, 2014-07-02 at 07:52 +0800, Zhao, Yakui wrote:
On Tue, 2014-07-01 at 09:26 -0600, Vivi, Rodrigo wrote:
It seems the flexibility on rings is more wanted and needed than I imagined.
Please ignore this patch here...
I liked both execution flag or debugfs, but exec flag would cover this
case of different applications using different command streamers.
With flags Would it be something like:
Execution without flag = ping-pong
Flag BSD1 use only VCS1
Flag BSD2 use only VCS2
IMO the execution flag looks reasonable. It can cover the flexibility of
different applications. In such case it can determine which ring is used
to dispatch command at runtime.
I prefer the execution flag too.
Thanks
Haihao
Haihao, what do you think?
With debugfs would be something like i195_dual_bsd_ring file with 3
options: all bsd1 bsd2
Thanks,
Rodrigo.
-Original Message-
From: Zhao, Yakui
Sent: Monday, June 30, 2014 6:37 PM
To: Vivi, Rodrigo
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915: Introduce dual_bsd_ring
parameter.
On Mon, 2014-06-30 at 10:51 -0600, Rodrigo Vivi wrote:
On Broadwell GT3 we have 2 Video Command Streamers (VCS), but
userspace has no control when using VCS1 or VCS2. So we cannot test,
validate or debug specific changes or workaround that might affect
only one or another ring. So this patch introduces a mechanism to
avoid the ping-pong selection and use one specific ring given at boot
time.
If it is mainly used for the test/validation, can we add one override
flag so that the user-space app can explicitly declare which BSD ring is
used to dispatch the corresponding BSD commands? In such case it will force
to dispatch the corresponding commands on the ring passed by
user-application.
At the same time this patch is not helpful under the following scenario.
For example: One application hopes to use the BSD Ring 0 while another
application hopes to use the BSD ring 1.
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h| 1 +
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34
++
drivers/gpu/drm/i915/i915_params.c | 6 ++
3 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h index 8cea596..7b6614f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2069,6 +2069,7 @@ struct i915_params {
int panel_ignore_lid;
unsigned int powersave;
int semaphores;
+ int dual_bsd_ring;
unsigned int lvds_downclock;
int lvds_channel_mode;
int panel_use_ssc;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d815ef5..09f350e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1035,26 +1035,32 @@ static int gen8_dispatch_bsd_ring(struct
drm_device *dev, {
struct drm_i915_private *dev_priv = dev-dev_private;
struct drm_i915_file_private *file_priv = file-driver_priv;
+ int ring_id;
+ int dual = i915.dual_bsd_ring;
/* Check whether the file_priv is using one ring */
if (file_priv-bsd_ring)
return file_priv-bsd_ring-id;
- else {
- /* If no, use the ping-pong mechanism to select one ring */
- int ring_id;
- mutex_lock(dev-struct_mutex);
- if (dev_priv-mm.bsd_ring_dispatch_index == 0) {
- ring_id = VCS;
- dev_priv-mm.bsd_ring_dispatch_index = 1;
- } else {
- ring_id = VCS2;
- dev_priv-mm.bsd_ring_dispatch_index = 0;
- }
- file_priv-bsd_ring = dev_priv-ring[ring_id];
- mutex_unlock(dev-struct_mutex);
- return ring_id;
+ /* If no, use the parameter defined or ping-pong mechanism
+ * to select one ring */
+ mutex_lock(dev-struct_mutex);
+
+ if (dual == 1 || (dual != 2
+ dev_priv-mm.bsd_ring_dispatch_index == 0)) {
+ ring_id = VCS;
+ dev_priv-mm.bsd_ring_dispatch_index = 1;
+ } else {
+ ring_id = VCS2;
+ dev_priv-mm.bsd_ring_dispatch_index = 0;
}
+
+ file_priv-bsd_ring = dev_priv-ring[ring_id];
+ mutex_unlock(dev-struct_mutex);
+
+ WARN(dual, Forcibly trying to use only one bsd ring. Using: %s\n,
+ file_priv-bsd_ring-name);
+ return ring_id;
}
static struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 8145729..d4871c8 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -29,6 +29,7 @@
