Re: [Intel-gfx] [PATCH] tests: Add gem_exec_params
On Wed, 2014-04-23 at 12:32 -0600, Daniel Vetter wrote: > This fills all the gaps we've had in our execbuf testing. Overflow > testing of the various arrays is already done by gem_reloc_overflow. > > Also add kms_flip_tiling to .gitignore. > > This will cause a bunch of failures since current kernels don't catch > all fallout. > Very good patch. Except some small concerns, it is OK to me. > Signed-off-by: Daniel Vetter > --- > tests/.gitignore| 2 + > tests/Makefile.sources | 1 + > tests/gem_exec_params.c | 212 > > 3 files changed, 215 insertions(+) > create mode 100644 tests/gem_exec_params.c > > diff --git a/tests/.gitignore b/tests/.gitignore > index 146bab06b565..4c50bae93aa3 100644 > --- a/tests/.gitignore > +++ b/tests/.gitignore > @@ -35,6 +35,7 @@ gem_exec_blt > gem_exec_faulting_reloc > gem_exec_lut_handle > gem_exec_nop > +gem_exec_params > gem_exec_parse > gem_fd_exhaustion > gem_fenced_exec_thrash > @@ -113,6 +114,7 @@ kms_addfb > kms_cursor_crc > kms_fbc_crc > kms_flip > +kms_flip_tiling > kms_pipe_crc_basic > kms_plane > kms_render > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index c957ace2ace0..9b2d7cff1113 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -29,6 +29,7 @@ TESTS_progs_M = \ > gem_exec_bad_domains \ > gem_exec_faulting_reloc \ > gem_exec_nop \ > + gem_exec_params \ > gem_exec_parse \ > gem_fenced_exec_thrash \ > gem_fence_thrash \ > diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c > new file mode 100644 > index ..b1d996c530f5 > --- /dev/null > +++ b/tests/gem_exec_params.c > @@ -0,0 +1,212 @@ > +/* > + * Copyright (c) 2014 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: > + *Daniel Vetter > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "drm.h" > + > +#include "ioctl_wrappers.h" > +#include "drmtest.h" > +#include "intel_io.h" > +#include "intel_chipset.h" > +#include "igt_aux.h" > + > +#define LOCAL_I915_EXEC_VEBOX (4<<0) > + > +struct drm_i915_gem_execbuffer2 execbuf; > +struct drm_i915_gem_exec_object2 gem_exec[1]; > +uint32_t batch[2] = {MI_BATCH_BUFFER_END}; > +uint32_t handle, devid; > +int fd; > + > +igt_main > +{ > + igt_fixture { > + fd = drm_open_any(); > + > + devid = intel_get_drm_devid(fd); > + > + handle = gem_create(fd, 4096); > + gem_write(fd, handle, 0, batch, sizeof(batch)); > + > + gem_exec[0].handle = handle; > + gem_exec[0].relocation_count = 0; > + gem_exec[0].relocs_ptr = 0; > + gem_exec[0].alignment = 0; > + gem_exec[0].offset = 0; > + gem_exec[0].flags = 0; > + gem_exec[0].rsvd1 = 0; > + gem_exec[0].rsvd2 = 0; > + > + execbuf.buffers_ptr = (uintptr_t)gem_exec; > + execbuf.buffer_count = 1; > + execbuf.batch_start_offset = 0; > + execbuf.batch_len = 8; Can we use the sizeof(batch) instead of 8? > + execbuf.cliprects_ptr = 0; > + execbuf.num_cliprects = 0; > + execbuf.DR1 = 0; > + execbuf.DR4 = 0; > + execbuf.flags = 0; > + i915_execbuffer2_set_context_id(execbuf, 0); > + execbuf.rsvd2 = 0; > + } > + > + igt_subtest("control") { > + igt_assert(drmIoctl(fd, > + DRM_IOCTL_I915_GEM_EXECBUFFER2, > + &execbuf) == 0); > + execbuf.flags = I915_EXEC_RENDER; > + igt_assert(drmIoctl(fd, > + DRM_
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields
On Thu, Apr 24, 2014 at 07:19:56AM +0100, Chris Wilson wrote: > On Wed, Apr 23, 2014 at 08:32:20PM +0200, Daniel Vetter wrote: > > We need to make sure that userspace keeps on following the contract, > > otherwise we won't be able to use the reserved fields at all. > > > > Testcase: igt/gem_exec_params/*-dirt > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index c2e5d39a1df8..0f0aebdd8dbd 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -1115,6 +1115,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void > > *data, > > ret = -EFAULT; > > goto pre_mutex_err; > > } > > + } else { > > + if (args->DR1 || args->DR4 || args->cliprects_ptr) > > + return -EINVAL; > > We're inside a mutex here. No, no we're not. Fine, your series passes. Reviewed-by: Chris Wilson The only thing we perhaps should consider is a sprinkling of DRM_DEBUG() to distinguish the EINVALs. -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 3/3] drm/i915: Catch dirt in unused execbuffer fields
On Wed, Apr 23, 2014 at 08:32:20PM +0200, Daniel Vetter wrote: > We need to make sure that userspace keeps on following the contract, > otherwise we won't be able to use the reserved fields at all. > > Testcase: igt/gem_exec_params/*-dirt > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index c2e5d39a1df8..0f0aebdd8dbd 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1115,6 +1115,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void > *data, > ret = -EFAULT; > goto pre_mutex_err; > } > + } else { > + if (args->DR1 || args->DR4 || args->cliprects_ptr) > + return -EINVAL; We're inside a mutex here. -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] [RFC] xf86-video-intel: enable hw-generated binding tables
On Thu, Apr 24, 2014 at 09:08:14AM +0300, Abdiel Janulgue wrote: > Anyway I haven't tried the work-around where we explictly only disable the BT > and RS on the other user-space clients (xorg driver in this case) when Mesa > is > using RS instead of forcing the reset of the clients to use RS format. I'll > try that first and let you know if it works. If it does, it might be more > efficient to do that in the kernel? It has to be done in the kernel in order for interoperability with third party clients. -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] 3.15-rc2: i915 regression: only top 20% of screen works in X
On Thu, Apr 24, 2014 at 7:50 AM, Chris Wilson wrote: > That says that i915.ko failed to initialise the GPU (or rather the GPU > wasn't responding) and bailed during module load. The key line here is > > [drm:init_ring_common] *ERROR* render ring initialization failed ctl > 0001f001 head 2034 tail start 0012f000 > > Jiri has been seeing a similar issue creep in during resume, but it is > not reliable enough to bisect. Is your boot failure reliable enough to > bisect? Also drm-intel-nightly should mitigate this failure and allow > i915.ko to continue to load and run X, which would be worth testing to > make sure that works as intended. Oh right, g4x going beserk :( Apparently something changed in 3.15 somewhere which made this much more likely, but like Chris said in Jiri's case it's too unreliable to reproduce for a bisect. We've had this come&go pretty much ever since kms support was merged and never tracked it down. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554 Like Chris said please test latest drm-intel-nighlty from http://cgit.freedesktop.org/drm-intel to make sure that the recently merged mitigation measures work properly. But those won't get your gpu back, only the display and it's only for 3.16. We're still hunting a proper fix for 3.15. And if you can indeed reliably reproduce this a bisect could be really useful. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] xf86-video-intel: enable hw-generated binding tables
On Wednesday, April 23, 2014 08:50:21 PM Ville Syrjälä wrote: > On Wed, Apr 23, 2014 at 06:52:09PM +0200, Daniel Vetter wrote: > > On Wed, Apr 23, 2014 at 1:21 PM, Abdiel Janulgue > > > > wrote: > > > I've already tried disabling RS at the end of every batch so that next > > > batch in different context can continue to use older non-RS format. > > > That does not work either and still causes hangs. > > > > > > What I've seen so far, it seems GPU does not like switching the format > > > of > > > commands from RS-format to non-RS format. It's either one way or the > > > other. If switched on, it affects subsequent contexes henceforth > > > expecting RS-format commands until the GPU gets reset. That's probably > > > the note in bspec: > > > > > > "the binding table generator feature has a simple all or nothing model". > > > > Oh hooray, that's just awesome :( So we indeed need to stop the gpu > > and reset it if there's a non-RS render batch after any RS render > > batch. > > I'm not sure I buy it. There's an example in the spec showing how to > disable the RS around eg. GPGPU workloads and re-enable it for 3D > workloads even within one batch. I guess it's possible that the GPGPU > pipeline mode is somehow special here, but since the RS state is > supposed to be saved to the context I'm thinking you should be able to > disable it whenever you want. > > What's really confusing with that example is the fact that it first > re-enables the binding tables and then the RS, but then there's a note > after that saying you have to those steps in the opposite order. > > Anyhoo, I'm wondering if the problems are simply due to disabling RS but > leaving the binding tables enabled? Yes, the work-around I tried previously is that at the end of each batch both hw-binding tables and RS are disabled in that particular order. > > So one idea might be that when we switch from executing an RS batch to a > non-RS batch, we should disable the RS and binding tables manually. We > would then have to demand that any user batch which needs the binding > tables enabled must enable them, even if if we just executed a batch that > already used the binding tables. And when we need to disable the RS and > binding tables we could either have the kernel do that autmagically when > it notices a RS->non-RS transition, or we could also demand that all user > batches must disable the binding tables at the end. But maybe demanding > that from every batch would incur some extra overhead? In any case it > should be fairly easy to try that and see if it cures the hangs. Or are > you already doing things that way? This is actually what current RS implementation in Mesa does. It explicitly enables RS then hw-binding tables for each batch start and then the tear-down work-around at the batch end I mentioned above; which didn't help at all. Anyway I haven't tried the work-around where we explictly only disable the BT and RS on the other user-space clients (xorg driver in this case) when Mesa is using RS instead of forcing the reset of the clients to use RS format. I'll try that first and let you know if it works. If it does, it might be more efficient to do that in the kernel? --Abdiel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] mm: Throttle shrinkers harder
On Wed, Apr 23, 2014 at 02:14:36PM -0700, Dave Hansen wrote: > On 04/22/2014 12:30 PM, Daniel Vetter wrote: > >> > > During testing of i915.ko with working texture sets larger than RAM, we > >> > > encounter OOM with plenty of memory still trapped within writeback, > >> > > e.g: > >> > > > >> > > [ 42.386039] active_anon:10134 inactive_anon:1900781 isolated_anon:32 > >> > > active_file:33 inactive_file:39 isolated_file:0 > >> > > unevictable:0 dirty:0 writeback:337627 unstable:0 > >> > > free:11985 slab_reclaimable:9458 slab_unreclaimable:23614 > >> > > mapped:41 shmem:1560769 pagetables:1276 bounce:0 > >> > > > >> > > If we throttle for writeback following shrink_slab, this gives us time > >> > > to wait upon the writeback generated by the i915.ko shinker: > >> > > > >> > > [ 4756.750808] active_anon:24386 inactive_anon:900793 isolated_anon:0 > >> > > active_file:23 inactive_file:20 isolated_file:0 > >> > > unevictable:0 dirty:0 writeback:0 unstable:0 > >> > > free:5550 slab_reclaimable:5184 slab_unreclaimable:4888 > >> > > mapped:3 shmem:472393 pagetables:1249 bounce:0 > > Could you get some dumps of the entire set of OOM information? These > are only tiny snippets. For reference the last oom report after flushing all the writeback: [ 4756.749554] crond invoked oom-killer: gfp_mask=0x201da, order=0, oom_score_adj=0 [ 4756.749603] crond cpuset=/ mems_allowed=0 [ 4756.749628] CPU: 0 PID: 3574 Comm: crond Tainted: GW 3.14.0_prts_de579f_20140410 #2 [ 4756.749676] Hardware name: Gigabyte Technology Co., Ltd. H55M-UD2H/H55M-UD2H, BIOS F4 12/02/2009 [ 4756.749723] 000201da 81717273 8800d235dc40 [ 4756.749762] 81714541 0400 8800cb6f3b10 880117ff8000 [ 4756.749800] 81072266 0206 812d6ebe 880112f25c40 [ 4756.749838] Call Trace: [ 4756.749856] [] ? dump_stack+0x41/0x51 [ 4756.749881] [] ? dump_header.isra.8+0x69/0x191 [ 4756.749911] [] ? ktime_get_ts+0x49/0xab [ 4756.749938] [] ? ___ratelimit+0xae/0xc8 [ 4756.749965] [] ? oom_kill_process+0x76/0x32c [ 4756.749992] [] ? find_lock_task_mm+0x22/0x6e [ 4756.750018] [] ? out_of_memory+0x41c/0x44f [ 4756.750045] [] ? __alloc_pages_nodemask+0x680/0x78d [ 4756.750076] [] ? alloc_pages_current+0xbf/0xdc [ 4756.750103] [] ? filemap_fault+0x266/0x38b [ 4756.750130] [] ? __do_fault+0xac/0x3bf [ 4756.750155] [] ? handle_mm_fault+0x1e7/0x7e2 [ 4756.750181] [] ? tlb_flush_mmu+0x4b/0x64 [ 4756.750219] [] ? timerqueue_add+0x79/0x98 [ 4756.750254] [] ? enqueue_hrtimer+0x15/0x37 [ 4756.750287] [] ? __do_page_fault+0x42e/0x47b [ 4756.750319] [] ? hrtimer_try_to_cancel+0x67/0x70 [ 4756.750353] [] ? hrtimer_cancel+0xc/0x16 [ 4756.750385] [] ? do_nanosleep+0xb3/0xf1 [ 4756.750415] [] ? hrtimer_nanosleep+0x89/0x10b [ 4756.750447] [] ? page_fault+0x22/0x30 [ 4756.750476] Mem-Info: [ 4756.750490] Node 0 DMA per-cpu: [ 4756.750510] CPU0: hi:0, btch: 1 usd: 0 [ 4756.750533] CPU1: hi:0, btch: 1 usd: 0 [ 4756.750555] CPU2: hi:0, btch: 1 usd: 0 [ 4756.750576] CPU3: hi:0, btch: 1 usd: 0 [ 4756.750598] Node 0 DMA32 per-cpu: [ 4756.750615] CPU0: hi: 186, btch: 31 usd: 0 [ 4756.750637] CPU1: hi: 186, btch: 31 usd: 0 [ 4756.750660] CPU2: hi: 186, btch: 31 usd: 0 [ 4756.750681] CPU3: hi: 186, btch: 31 usd: 0 [ 4756.750702] Node 0 Normal per-cpu: [ 4756.750720] CPU0: hi: 90, btch: 15 usd: 0 [ 4756.750742] CPU1: hi: 90, btch: 15 usd: 0 [ 4756.750763] CPU2: hi: 90, btch: 15 usd: 0 [ 4756.750785] CPU3: hi: 90, btch: 15 usd: 0 [ 4756.750808] active_anon:24386 inactive_anon:900793 isolated_anon:0 active_file:23 inactive_file:20 isolated_file:0 unevictable:0 dirty:0 writeback:0 unstable:0 free:5550 slab_reclaimable:5184 slab_unreclaimable:4888 mapped:3 shmem:472393 pagetables:1249 bounce:0 free_cma:0 [ 4756.750938] Node 0 DMA free:14664kB min:32kB low:40kB high:48kB active_anon:0kB inactive_anon:1024kB active_file:0kB inactive_file:4kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:15992kB managed:15908kB mlocked:0kB dirty:0kB writeback:0kB mapped:0kB shmem:412kB slab_reclaimable:80kB slab_unreclaimable:24kB kernel_stack:0kB pagetables:48kB unstable:0kB bounce:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:76 all_unreclaimable? yes [ 4756.751103] lowmem_reserve[]: 0 3337 3660 3660 [ 4756.751133] Node 0 DMA32 free:7208kB min:7044kB low:8804kB high:10564kB active_anon:36172kB inactive_anon:3351408kB active_file:92kB inactive_file:72kB unevictable:0kB isolated(anon):0kB isolated(file):0kB present:3518336kB managed:3440548kB mlocked:0kB dirty:0kB writeback:0kB mapped:12kB shmem:1661420kB slab_reclaimable:17624kB slab_unreclaimable:14400kB kernel_stack:696kB pagetables:4324kB unstable:0kB bounce:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:327 all_unreclaimable? yes [ 4756.751341] lowmem_reserve[]: 0 0 322 32
Re: [Intel-gfx] 3.15-rc2: i915 regression: only top 20% of screen works in X
On Thu, Apr 24, 2014 at 12:09:52AM +0200, Pavel Machek wrote: > Hi! > > > > After update to 3.15-rc2, only top 20% of screen works on X. > > > > > > 00:02.0 VGA compatible controller: Intel Corporation 4 Series Chipset > > > Integrated Graphics Controller (rev 03) > > > > > > 00:02.1 Display controller: Intel Corporation 4 Series Chipset > > > Integrated Graphics Controller (rev 03) > > >Subsystem: Intel Corporation Device d614 > > >Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- > > >ParErr- Stepping- SERR- FastB2B- DisINTx- > > >Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast > > >>TAbort- SERR- > >Latency: 0 > > >Region 0: Memory at d040 (64-bit, non-prefetchable) > > >[size=1M] > > >Capabilities: > > > > > > This worked before. I believe it worked in 3.14. It definitely works > > > in 3.11-rc2. > > > > Screenshot or more detailed description of what "only top 20% of > > screen works in X" means? > > Anything in dmesg? > > Actually yes, dmesg suggests it is quite > sick. drivers/gpu/drm/drm_mm.c:767 warning triggered > repeatedly. Also.. initial framebuffer does not work ; I don't seem to > see anything before X start up. (This is Debian 6.0.9) That says that i915.ko failed to initialise the GPU (or rather the GPU wasn't responding) and bailed during module load. The key line here is [drm:init_ring_common] *ERROR* render ring initialization failed ctl 0001f001 head 2034 tail start 0012f000 Jiri has been seeing a similar issue creep in during resume, but it is not reliable enough to bisect. Is your boot failure reliable enough to bisect? Also drm-intel-nightly should mitigate this failure and allow i915.ko to continue to load and run X, which would be worth testing to make sure that works as intended. -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 I-g-t V4 0/2] Tests: Add test cases based on multi drm_fd to test sync
This follows Daniel's advice to add the two test cases based on multi drm_fd to test the ring sync and CPU<->GPU sync. The Broadwell GT3 machine has two independent BSD rings that can be used to process the video commands. This is implemented in kernel driver and transparent to the user-space. But we still need to check the ring sync and CPU<->GPU sync for the second BSD ring. Two tests are created based on the multi drm_fds to test the sync. Multi drm_fd can assure that the second BSD ring has the opportunity to dispatch the GPU command. V1->V2: Follow Daniel's comment to add one subtext instead of one individual test case, which is used to test the CPU<->GPU sync under multi BSD rings/ V2->V3: Follow Imre's comment to remove the unnecessary initialization and use igt_assert_f instead of igt_assert V3->V4: Add gem_multi_bsd_sync_loop.c into the tests/.gitignore Zhao Yakui (2): tests: Add one ring sync case based on multi drm_fd to test ring semaphore sync under multi BSD rings tests/gem_dummy_reloc_loop: Add one subtest based on multi drm_fd to test CPU<->GPU sync under multi BSD rings tests/.gitignore|1 + tests/Makefile.sources |1 + tests/gem_dummy_reloc_loop.c| 107 +++- tests/gem_multi_bsd_sync_loop.c | 175 +++ 4 files changed, 283 insertions(+), 1 deletion(-) create mode 100644 tests/gem_multi_bsd_sync_loop.c -- 1.7.10.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH I-g-t V4 2/2] tests/gem_dummy_reloc_loop: Add one subtest based on multi drm_fd to test CPU<->GPU sync under multi BSD rings
The Broadwell GT3 machine has two independent BSD rings in kernel driver while it is transparent to the user-space driver. In such case it needs to check the CPU<->GPU sync for the second BSD ring. V1->V2: Follow Daniel's comment to add one subtext instead of one individual test case, which is used to test the CPU<->GPU sync under multi BSD rings. V2->V3: Follow Imre's comment to remove the unnecessary initialization and use igt_assert_f instead of igt_assert Reviewed-by: Imre Deak Signed-off-by: Zhao Yakui --- tests/gem_dummy_reloc_loop.c | 107 +- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/tests/gem_dummy_reloc_loop.c b/tests/gem_dummy_reloc_loop.c index a61b59b..4e4dd49 100644 --- a/tests/gem_dummy_reloc_loop.c +++ b/tests/gem_dummy_reloc_loop.c @@ -48,6 +48,13 @@ static drm_intel_bufmgr *bufmgr; struct intel_batchbuffer *batch; static drm_intel_bo *target_buffer; +#define NUM_FD 50 + +static int mfd[NUM_FD]; +static drm_intel_bufmgr *mbufmgr[NUM_FD]; +static struct intel_batchbuffer *mbatch[NUM_FD]; +static drm_intel_bo *mbuffer[NUM_FD]; + /* * Testcase: Basic check of ring<->cpu sync using a dummy reloc * @@ -124,6 +131,50 @@ dummy_reloc_loop_random_ring(int num_rings) } } +static void +dummy_reloc_loop_random_ring_multi_fd(int num_rings) +{ + int i; + struct intel_batchbuffer *saved_batch; + + saved_batch = batch; + + srandom(0xdeadbeef); + + for (i = 0; i < 0x10; i++) { + int mindex; + int ring = random() % num_rings + 1; + + mindex = random() % NUM_FD; + batch = mbatch[mindex]; + + if (ring == I915_EXEC_RENDER) { + BEGIN_BATCH(4); + OUT_BATCH(MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE); + OUT_BATCH(0x); /* compare dword */ + OUT_RELOC(mbuffer[mindex], I915_GEM_DOMAIN_RENDER, + I915_GEM_DOMAIN_RENDER, 0); + OUT_BATCH(MI_NOOP); + ADVANCE_BATCH(); + } else { + BEGIN_BATCH(4); + OUT_BATCH(MI_FLUSH_DW | 1); + OUT_BATCH(0); /* reserved */ + OUT_RELOC(mbuffer[mindex], I915_GEM_DOMAIN_RENDER, + I915_GEM_DOMAIN_RENDER, 0); + OUT_BATCH(MI_NOOP | (1<<22) | (0xf)); + ADVANCE_BATCH(); + } + intel_batchbuffer_flush_on_ring(batch, ring); + + drm_intel_bo_map(target_buffer, 0); + // map to force waiting on rendering + drm_intel_bo_unmap(target_buffer); + } + + batch = saved_batch; +} + int fd; int devid; int num_rings; @@ -133,6 +184,7 @@ igt_main igt_skip_on_simulation(); igt_fixture { + int i; fd = drm_open_any(); devid = intel_get_drm_devid(fd); num_rings = gem_get_num_rings(fd); @@ -148,6 +200,40 @@ igt_main target_buffer = drm_intel_bo_alloc(bufmgr, "target bo", 4096, 4096); igt_assert(target_buffer); + + /* Create multi drm_fd and map one gem object to multi gem_contexts */ + { + unsigned int target_flink; + char buffer_name[32]; + if (dri_bo_flink(target_buffer, &target_flink)) { + printf("fail to get flink for target buffer\n"); + igt_assert_f(0, "fail to create global " +"gem_handle for target buffer\n"); + } + for (i = 0; i < NUM_FD; i++) { + sprintf(buffer_name, "Target buffer %d\n", i); + mfd[i] = drm_open_any(); + mbufmgr[i] = drm_intel_bufmgr_gem_init(mfd[i], 4096); + igt_assert_f(mbufmgr[i], +"fail to initialize buf manager " +"for drm_fd %d\n", +mfd[i]); + drm_intel_bufmgr_gem_enable_reuse(mbufmgr[i]); + mbatch[i] = intel_batchbuffer_alloc(mbufmgr[i], devid); + igt_assert_f(mbatch[i], +"fail to create batchbuffer " +"for drm_fd %d\n", +mfd[i]); + mbuffer[i] = intel_bo_gem_create_from_name( + mbufmgr[i], +
[Intel-gfx] [PATCH I-g-t V4 1/2] tests: Add one ring sync case based on multi drm_fd to test ring semaphore sync under multi BSD rings
The Broadwell GT3 machine has two independent BSD rings in kernel driver while it is transparent to the user-space driver. In such case it needs to check the ring sync between the two BSD rings. At the same time it also needs to check the sync among the second BSD ring and the other rings. V2->V3: Follow Imre's comment to remove the unnecessary initialization and use igt_assert_f instead of igt_assert. V3->V4: Add gem_multi_bsd_sync_loop.c into the tests/.gitignore Reviewed-by: Imre Deak Signed-off-by: Zhao Yakui --- tests/.gitignore|1 + tests/Makefile.sources |1 + tests/gem_multi_bsd_sync_loop.c | 175 +++ 3 files changed, 177 insertions(+) create mode 100644 tests/gem_multi_bsd_sync_loop.c diff --git a/tests/.gitignore b/tests/.gitignore index 146bab0..42690dd 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -54,6 +54,7 @@ gem_media_fill gem_mmap gem_mmap_gtt gem_mmap_offset_exhaustion +gem_multi_bsd_sync_loop gem_non_secure_batch gem_partial_pwrite_pread gem_persistent_relocs diff --git a/tests/Makefile.sources b/tests/Makefile.sources index c957ace..7cd9ca8 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -105,6 +105,7 @@ TESTS_progs = \ gem_render_tiled_blits \ gem_ring_sync_copy \ gem_ring_sync_loop \ + gem_multi_bsd_sync_loop \ gem_seqno_wrap \ gem_set_tiling_vs_gtt \ gem_set_tiling_vs_pwrite \ diff --git a/tests/gem_multi_bsd_sync_loop.c b/tests/gem_multi_bsd_sync_loop.c new file mode 100644 index 000..b01764a --- /dev/null +++ b/tests/gem_multi_bsd_sync_loop.c @@ -0,0 +1,175 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Daniel Vetter (based on gem_ring_sync_loop_*.c) + *Zhao Yakui + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include "drm.h" +#include "ioctl_wrappers.h" +#include "drmtest.h" +#include "intel_bufmgr.h" +#include "intel_batchbuffer.h" +#include "intel_io.h" +#include "i830_reg.h" +#include "intel_chipset.h" + +static drm_intel_bufmgr *bufmgr; +struct intel_batchbuffer *batch; +static drm_intel_bo *target_buffer; + +#define NUM_FD 50 + +static int mfd[NUM_FD]; +static drm_intel_bufmgr *mbufmgr[NUM_FD]; +static struct intel_batchbuffer *mbatch[NUM_FD]; +static drm_intel_bo *mbuffer[NUM_FD]; + + +/* + * Testcase: Basic check of ring<->ring sync using a dummy reloc + * + * Extremely efficient at catching missed irqs with semaphores=0 ... + */ + +#define MI_COND_BATCH_BUFFER_END (0x36<<23 | 1) +#define MI_DO_COMPARE (1<<21) + +static void +store_dword_loop(int fd) +{ + int i; + int num_rings = gem_get_num_rings(fd); + + srandom(0xdeadbeef); + + for (i = 0; i < SLOW_QUICK(0x10, 10); i++) { + int ring, mindex; + ring = random() % num_rings + 1; + mindex = random() % NUM_FD; + batch = mbatch[mindex]; + if (ring == I915_EXEC_RENDER) { + BEGIN_BATCH(4); + OUT_BATCH(MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE); + OUT_BATCH(0x); /* compare dword */ + OUT_RELOC(mbuffer[mindex], I915_GEM_DOMAIN_RENDER, + I915_GEM_DOMAIN_RENDER, 0); + OUT_BATCH(MI_NOOP); + ADVANCE_BATCH(); + } else { + BEGIN_BATCH(4); + OUT_BATCH(MI_FLUSH_DW | 1); + OUT_BATCH(0); /* reserved */ + OUT_RELOC(mbuffer[mindex], I915_GEM_DOMAIN_RENDER, + I915_GEM_DOMAIN_RENDER, 0); + OUT_BATCH(MI_NOOP | (1<<22) | (0xf)); +
Re: [Intel-gfx] [PATCH I-g-t V4 0/2] Tests: Add test cases based on multi drm_fd to test sync
On Wed, 2014-04-23 at 20:02 -0600, Zhao, Yakui wrote: It seems that the patch 01 is filter out. So I will try to resend it again. Thanks. Yakui > This follows Daniel's advice to add the two test cases based on multi drm_fd > to > test the ring sync and CPU<->GPU sync. > The Broadwell GT3 machine has two independent BSD rings that can be used > to process the video commands. This is implemented in kernel driver and > transparent > to the user-space. But we still need to check the ring sync and CPU<->GPU > sync for > the second BSD ring. Two tests are created based on the multi drm_fds to > test the sync. Multi drm_fd can assure that the second BSD ring has the > opportunity > to dispatch the GPU command. > > V1->V2: Follow Daniel's comment to add one subtext instead of one individual > test case, which is used to test the CPU<->GPU sync under multi BSD rings/ > > V2->V3: Follow Imre's comment to remove the unnecessary initialization and > use igt_assert_f instead of igt_assert > > > Zhao Yakui (2): > tests: Add one ring sync case based on multi drm_fd to test ring > semaphore sync under multi BSD rings > tests/gem_dummy_reloc_loop: Add one subtest based on multi drm_fd to > test CPU<->GPU sync under multi BSD rings > > tests/.gitignore|1 + > tests/Makefile.sources |1 + > tests/gem_dummy_reloc_loop.c| 107 +++- > tests/gem_multi_bsd_sync_loop.c | 175 > +++ > 4 files changed, 283 insertions(+), 1 deletion(-) > create mode 100644 tests/gem_multi_bsd_sync_loop.c > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH I-g-t 2/2] tests/gem_dummy_reloc_loop: Add one subtest based on multi drm_fd to test CPU<->GPU sync under multi BSD rings
The Broadwell GT3 machine has two independent BSD rings in kernel driver while it is transparent to the user-space driver. In such case it needs to check the CPU<->GPU sync for the second BSD ring. V1->V2: Follow Daniel's comment to add one subtext instead of one individual test case, which is used to test the CPU<->GPU sync under multi BSD rings. V2->V3: Follow Imre's comment to remove the unnecessary initialization and use igt_assert_f instead of igt_assert Reviewed-by: Imre Deak Signed-off-by: Zhao Yakui --- tests/gem_dummy_reloc_loop.c | 107 +- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/tests/gem_dummy_reloc_loop.c b/tests/gem_dummy_reloc_loop.c index a61b59b..4e4dd49 100644 --- a/tests/gem_dummy_reloc_loop.c +++ b/tests/gem_dummy_reloc_loop.c @@ -48,6 +48,13 @@ static drm_intel_bufmgr *bufmgr; struct intel_batchbuffer *batch; static drm_intel_bo *target_buffer; +#define NUM_FD 50 + +static int mfd[NUM_FD]; +static drm_intel_bufmgr *mbufmgr[NUM_FD]; +static struct intel_batchbuffer *mbatch[NUM_FD]; +static drm_intel_bo *mbuffer[NUM_FD]; + /* * Testcase: Basic check of ring<->cpu sync using a dummy reloc * @@ -124,6 +131,50 @@ dummy_reloc_loop_random_ring(int num_rings) } } +static void +dummy_reloc_loop_random_ring_multi_fd(int num_rings) +{ + int i; + struct intel_batchbuffer *saved_batch; + + saved_batch = batch; + + srandom(0xdeadbeef); + + for (i = 0; i < 0x10; i++) { + int mindex; + int ring = random() % num_rings + 1; + + mindex = random() % NUM_FD; + batch = mbatch[mindex]; + + if (ring == I915_EXEC_RENDER) { + BEGIN_BATCH(4); + OUT_BATCH(MI_COND_BATCH_BUFFER_END | MI_DO_COMPARE); + OUT_BATCH(0x); /* compare dword */ + OUT_RELOC(mbuffer[mindex], I915_GEM_DOMAIN_RENDER, + I915_GEM_DOMAIN_RENDER, 0); + OUT_BATCH(MI_NOOP); + ADVANCE_BATCH(); + } else { + BEGIN_BATCH(4); + OUT_BATCH(MI_FLUSH_DW | 1); + OUT_BATCH(0); /* reserved */ + OUT_RELOC(mbuffer[mindex], I915_GEM_DOMAIN_RENDER, + I915_GEM_DOMAIN_RENDER, 0); + OUT_BATCH(MI_NOOP | (1<<22) | (0xf)); + ADVANCE_BATCH(); + } + intel_batchbuffer_flush_on_ring(batch, ring); + + drm_intel_bo_map(target_buffer, 0); + // map to force waiting on rendering + drm_intel_bo_unmap(target_buffer); + } + + batch = saved_batch; +} + int fd; int devid; int num_rings; @@ -133,6 +184,7 @@ igt_main igt_skip_on_simulation(); igt_fixture { + int i; fd = drm_open_any(); devid = intel_get_drm_devid(fd); num_rings = gem_get_num_rings(fd); @@ -148,6 +200,40 @@ igt_main target_buffer = drm_intel_bo_alloc(bufmgr, "target bo", 4096, 4096); igt_assert(target_buffer); + + /* Create multi drm_fd and map one gem object to multi gem_contexts */ + { + unsigned int target_flink; + char buffer_name[32]; + if (dri_bo_flink(target_buffer, &target_flink)) { + printf("fail to get flink for target buffer\n"); + igt_assert_f(0, "fail to create global " +"gem_handle for target buffer\n"); + } + for (i = 0; i < NUM_FD; i++) { + sprintf(buffer_name, "Target buffer %d\n", i); + mfd[i] = drm_open_any(); + mbufmgr[i] = drm_intel_bufmgr_gem_init(mfd[i], 4096); + igt_assert_f(mbufmgr[i], +"fail to initialize buf manager " +"for drm_fd %d\n", +mfd[i]); + drm_intel_bufmgr_gem_enable_reuse(mbufmgr[i]); + mbatch[i] = intel_batchbuffer_alloc(mbufmgr[i], devid); + igt_assert_f(mbatch[i], +"fail to create batchbuffer " +"for drm_fd %d\n", +mfd[i]); + mbuffer[i] = intel_bo_gem_create_from_name( + mbufmgr[i], +
[Intel-gfx] [PATCH I-g-t V4 0/2] Tests: Add test cases based on multi drm_fd to test sync
This follows Daniel's advice to add the two test cases based on multi drm_fd to test the ring sync and CPU<->GPU sync. The Broadwell GT3 machine has two independent BSD rings that can be used to process the video commands. This is implemented in kernel driver and transparent to the user-space. But we still need to check the ring sync and CPU<->GPU sync for the second BSD ring. Two tests are created based on the multi drm_fds to test the sync. Multi drm_fd can assure that the second BSD ring has the opportunity to dispatch the GPU command. V1->V2: Follow Daniel's comment to add one subtext instead of one individual test case, which is used to test the CPU<->GPU sync under multi BSD rings/ V2->V3: Follow Imre's comment to remove the unnecessary initialization and use igt_assert_f instead of igt_assert Zhao Yakui (2): tests: Add one ring sync case based on multi drm_fd to test ring semaphore sync under multi BSD rings tests/gem_dummy_reloc_loop: Add one subtest based on multi drm_fd to test CPU<->GPU sync under multi BSD rings tests/.gitignore|1 + tests/Makefile.sources |1 + tests/gem_dummy_reloc_loop.c| 107 +++- tests/gem_multi_bsd_sync_loop.c | 175 +++ 4 files changed, 283 insertions(+), 1 deletion(-) create mode 100644 tests/gem_multi_bsd_sync_loop.c -- 1.7.10.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] 3.15-rc2: i915 regression: only top 20% of screen works in X
Hi! > > After update to 3.15-rc2, only top 20% of screen works on X. > > > > 00:02.0 VGA compatible controller: Intel Corporation 4 Series Chipset > > Integrated Graphics Controller (rev 03) > > > > 00:02.1 Display controller: Intel Corporation 4 Series Chipset > > Integrated Graphics Controller (rev 03) > >Subsystem: Intel Corporation Device d614 > >Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- > >ParErr- Stepping- SERR- FastB2B- DisINTx- > >Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast > >>TAbort- SERR- >Latency: 0 > >Region 0: Memory at d040 (64-bit, non-prefetchable) > >[size=1M] > >Capabilities: > > > > This worked before. I believe it worked in 3.14. It definitely works > > in 3.11-rc2. > > Screenshot or more detailed description of what "only top 20% of > screen works in X" means? > Anything in dmesg? Actually yes, dmesg suggests it is quite sick. drivers/gpu/drm/drm_mm.c:767 warning triggered repeatedly. Also.. initial framebuffer does not work ; I don't seem to see anything before X start up. (This is Debian 6.0.9) Best regards, Pavel Initializing cgroup subsys cpu Linux version 3.15.0-rc1+ (pavel@amd) (gcc version 4.4.5 (Debian 4.4.5-8) ) #335 SMP Sat Apr 19 17:58:01 CEST 2014 e820: BIOS-provided physical RAM map: BIOS-e820: [mem 0x-0x0009ebff] usable BIOS-e820: [mem 0x0009ec00-0x0009] reserved BIOS-e820: [mem 0x000e-0x000f] reserved BIOS-e820: [mem 0x0010-0xbd87dfff] usable BIOS-e820: [mem 0xbd87e000-0xbd900fff] ACPI NVS BIOS-e820: [mem 0xbd901000-0xbda42fff] reserved BIOS-e820: [mem 0xbda43000-0xbda56fff] ACPI NVS BIOS-e820: [mem 0xbda57000-0xbdb54fff] reserved BIOS-e820: [mem 0xbdb55000-0xbdb5dfff] ACPI data BIOS-e820: [mem 0xbdb5e000-0xbdb67fff] ACPI NVS BIOS-e820: [mem 0xbdb68000-0xbdb88fff] reserved BIOS-e820: [mem 0xbdb89000-0xbdb8efff] ACPI NVS BIOS-e820: [mem 0xbdb8f000-0xbdcf] usable BIOS-e820: [mem 0xfec0-0xfec00fff] reserved BIOS-e820: [mem 0xfed0-0xfed00fff] reserved BIOS-e820: [mem 0xfed1c000-0xfed8] reserved BIOS-e820: [mem 0xfff0-0x] reserved BIOS-e820: [mem 0x0001-0x00013fff] usable Notice: NX (Execute Disable) protection cannot be enabled: non-PAE kernel! SMBIOS 2.4 present. DMI: /DG41MJ, BIOS MJG4110H.86A.0006.2009.1223.1155 12/23/2009 e820: update [mem 0x-0x0fff] usable ==> reserved e820: remove [mem 0x000a-0x000f] usable e820: last_pfn = 0xbdd00 max_arch_pfn = 0x10 MTRR default type: uncachable MTRR fixed ranges enabled: 0-9 write-back A-E7FFF uncachable E8000-F write-protect MTRR variable ranges enabled: 0 base 0 mask F write-back 1 base 1 mask FC000 write-back 2 base 0BDD0 mask 0 write-through 3 base 0BDE0 mask FFFE0 uncachable 4 base 0BE00 mask FFE00 uncachable 5 base 0C000 mask FC000 uncachable 6 disabled x86 PAT enabled: cpu 0, old 0x7040600070406, new 0x7010600070106 initial memory mapped: [mem 0x-0x053f] Base memory trampoline at [c009a000] 9a000 size 16384 init_memory_mapping: [mem 0x-0x000f] [mem 0x-0x000f] page 4k init_memory_mapping: [mem 0x3700-0x373f] [mem 0x3700-0x373f] page 4k BRK [0x05109000, 0x05109fff] PGTABLE init_memory_mapping: [mem 0x3000-0x36ff] [mem 0x3000-0x36ff] page 4k BRK [0x0510a000, 0x0510afff] PGTABLE BRK [0x0510b000, 0x0510bfff] PGTABLE BRK [0x0510c000, 0x0510cfff] PGTABLE BRK [0x0510d000, 0x0510dfff] PGTABLE BRK [0x0510e000, 0x0510efff] PGTABLE init_memory_mapping: [mem 0x0010-0x2fff] [mem 0x0010-0x2fff] page 4k init_memory_mapping: [mem 0x3740-0x377fdfff] [mem 0x3740-0x377fdfff] page 4k ACPI: RSDP 0x000F03C0 24 (v02 INTEL ) ACPI: XSDT 0xBDB5CE18 44 (v01 INTEL DG41MJ 0006 MSFT 00010013) ACPI: FACP 0xBDB5BD98 F4 (v04 INTEL DG41RQ 0006 MSFT 00010013) ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT: 0xBDB5FF40/0xBDB64F40, using 64-bit address (20140214/tbfadt-271) ACPI: DSDT 0xBDB55018 005983 (v01 INTEL DG41MJ 0006 INTL 20051117) ACPI: FACS 0xBDB64F40 40 ACPI: APIC 0xBDB5BF18 6C (v02 INTEL DG41MJ 0006 MSFT 00010013) ACPI: MCFG 0xBDB66E18 3C (v01 INTEL DG41MJ 0006 MSFT 0097) ACPI: HPET 0xBDB66D98 38 (v01 INTEL DG41MJ 0006 AMI. 0003) ACPI: Local APIC address 0xfee0 2149MB HIGHMEM available. 887MB LOWMEM available. mapped low ram: 0 - 377fe000 low ram: 0 - 377fe000 Zone
Re: [Intel-gfx] 3.15-rc2: i915 regression: only top 20% of screen works in X
On Wed 2014-04-23 23:09:45, Daniel Vetter wrote: > On Wed, Apr 23, 2014 at 10:22 PM, Pavel Machek wrote: > > After update to 3.15-rc2, only top 20% of screen works on X. > > > > 00:02.0 VGA compatible controller: Intel Corporation 4 Series Chipset > > Integrated Graphics Controller (rev 03) > > > > 00:02.1 Display controller: Intel Corporation 4 Series Chipset > > Integrated Graphics Controller (rev 03) > >Subsystem: Intel Corporation Device d614 > >Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- > >ParErr- Stepping- SERR- FastB2B- DisINTx- > >Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast > >>TAbort- SERR- >Latency: 0 > >Region 0: Memory at d040 (64-bit, non-prefetchable) > >[size=1M] > >Capabilities: > > > > This worked before. I believe it worked in 3.14. It definitely works > > in 3.11-rc2. > > Screenshot or more detailed description of what "only top 20% of > screen works in X" means? Well, top cca 20% is ok, then I got repeated pattern of some part of screen. > Anything in dmesg? That will take a look. > bisect result presuming that it reproduces reliably? If there's no other chance, I guess I could do bisect. But first Do you have similar hardware? Does it work for you? Are there any experimental changes that went in recently and I should try reverting first? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/24] drm/i915: Add dev_priv->wm.mutex
2014-03-07 13:32 GMT-03:00 : > From: Ville Syrjälä > > Add a mutex to protect most of the watermark state. Will be useful when > we start to update watermarks asynchronously from plane updates, or > when we get finer grained locking for planes. > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/i915_drv.h | 11 ++- > drivers/gpu/drm/i915/intel_drv.h | 5 - > drivers/gpu/drm/i915/intel_pm.c | 21 +++-- > 3 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 29da39f..0b19723 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1595,8 +1595,17 @@ typedef struct drm_i915_private { > /* cursor */ > uint16_t cur_latency[5]; > > - /* current hardware state */ > + /* > +* current hardware state > +* protected by dev_priv->wm.mutex > +*/ > struct ilk_wm_values hw; > + > + /* > +* protects some dev_priv->wm and intel_crtc->wm > +* state as well as the actual hardware registers > +*/ > + struct mutex mutex; > } wm; > > struct i915_package_c8 pc8; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index f022a78..8e32d69 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -384,7 +384,10 @@ struct intel_crtc { > > /* per-pipe watermark state */ > struct { > - /* watermarks currently being used */ > + /* > +* watermarks currently being used > +* protected by dev_priv->wm.mutex > +*/ > struct intel_pipe_wm active; > } wm; > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 3f5c1dc..d8adcb3 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2692,8 +2692,13 @@ static void ilk_write_wm_values(struct > drm_i915_private *dev_priv, > static bool ilk_disable_lp_wm(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + bool changed; > > - return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > + mutex_lock(&dev_priv->wm.mutex); > + changed = _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > + mutex_unlock(&dev_priv->wm.mutex); > + > + return changed; > } > > static void ilk_program_watermarks(struct drm_device *dev) > @@ -2733,6 +2738,7 @@ static void ilk_update_wm(struct drm_crtc *crtc) > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > struct ilk_pipe_wm_parameters params = {}; > struct intel_pipe_wm pipe_wm = {}; > > @@ -2740,12 +2746,17 @@ static void ilk_update_wm(struct drm_crtc *crtc) > > intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm); > > + mutex_lock(&dev_priv->wm.mutex); > + > if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm))) > - return; > + goto unlock; > > intel_crtc->wm.active = pipe_wm; > > ilk_program_watermarks(dev); > + > + unlock: > + mutex_unlock(&dev_priv->wm.mutex); > } > > static void ilk_update_sprite_wm(struct drm_plane *plane, > @@ -2817,10 +2828,14 @@ void ilk_wm_get_hw_state(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc; > > + mutex_lock(&dev_priv->wm.mutex); > + > _ilk_wm_get_hw_state(dev, &dev_priv->wm.hw); > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > _ilk_pipe_wm_hw_to_sw(crtc); > + > + mutex_unlock(&dev_priv->wm.mutex); > } > > /** > @@ -5760,6 +5775,8 @@ void intel_init_pm(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + mutex_init(&dev_priv->wm.mutex); > + I think you should probably init this at intel_pm_setup(), which is called way earlier. Just to be safe. With that fixed: Reviewed-by: Paulo Zanoni . > if (HAS_FBC(dev)) { > if (INTEL_INFO(dev)->gen >= 7) { > dev_priv->display.fbc_enabled = ironlake_fbc_enabled; > -- > 1.8.3.2 > > ___ > 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 13/24] drm/i915: Refactor ilk_update_wm
2014-03-07 13:32 GMT-03:00 : > From: Ville Syrjälä > > Split ilk_update_wm() into two parts; one doing the progragramming > and the other the calculations. There are 1.060 google results for the word "progragramming", which you used above :) Sounds like one of those words that have some special funny meaning. Reviewed-by: Paulo Zanoni > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_pm.c | 38 ++ > 1 file changed, 22 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index e142095..3f5c1dc 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2696,27 +2696,14 @@ static bool ilk_disable_lp_wm(struct drm_device *dev) > return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > } > > -static void ilk_update_wm(struct drm_crtc *crtc) > +static void ilk_program_watermarks(struct drm_device *dev) > { > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm; > struct ilk_wm_maximums max; > - struct ilk_pipe_wm_parameters params = {}; > + struct intel_wm_config config = {}; > struct ilk_wm_values results = {}; > enum intel_ddb_partitioning partitioning; > - struct intel_pipe_wm pipe_wm = {}; > - struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm; > - struct intel_wm_config config = {}; > - > - ilk_compute_wm_parameters(crtc, ¶ms); > - > - intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm); > - > - if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm))) > - return; > - > - intel_crtc->wm.active = pipe_wm; > > ilk_compute_wm_config(dev, &config); > > @@ -2742,6 +2729,25 @@ static void ilk_update_wm(struct drm_crtc *crtc) > ilk_write_wm_values(dev_priv, &results); > } > > +static void ilk_update_wm(struct drm_crtc *crtc) > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_device *dev = crtc->dev; > + struct ilk_pipe_wm_parameters params = {}; > + struct intel_pipe_wm pipe_wm = {}; > + > + ilk_compute_wm_parameters(crtc, ¶ms); > + > + intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm); > + > + if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm))) > + return; > + > + intel_crtc->wm.active = pipe_wm; > + > + ilk_program_watermarks(dev); > +} > + > static void ilk_update_sprite_wm(struct drm_plane *plane, > struct drm_crtc *crtc, > uint32_t sprite_width, int pixel_size, > -- > 1.8.3.2 > > ___ > 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 12/24] drm/i915: Refactor ilk_validate_pipe_wm()
2014-03-07 13:32 GMT-03:00 : > From: Ville Syrjälä > > Pull the LP0 validate out from intel_compute_pipe_wm() into a separate > function. We will have further need for such a function to validate > both the final watermarks and the intermediate watermarks we will be > using while the plane(s) transition between different configurations. > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_pm.c | 30 +++--- > 1 file changed, 19 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index e519578a1..e142095 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2172,6 +2172,24 @@ static void ilk_compute_wm_config(struct drm_device > *dev, > } > } > > +static bool ilk_validate_pipe_wm(struct drm_device *dev, > +struct intel_pipe_wm *pipe_wm) > +{ > + /* LP0 watermark maximums depend on this pipe alone */ > + const struct intel_wm_config config = { > + .num_pipes_active = 1, > + .sprites_enabled = pipe_wm->sprites_enabled, > + .sprites_scaled = pipe_wm->sprites_scaled, > + }; > + struct ilk_wm_maximums max; > + > + /* LP0 watermarks always use 1/2 DDB partitioning */ > + ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max); > + > + /* At least LP0 must be valid */ I would remove this comment, and keep the original one on the caller. IMHO it makes more sense there. With or without that change: Reviewed-by: Paulo Zanoni > + return ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]); > +} > + > /* Compute new watermarks for the pipe */ > static bool intel_compute_pipe_wm(struct drm_crtc *crtc, > const struct ilk_pipe_wm_parameters *params, > @@ -2180,12 +2198,6 @@ static bool intel_compute_pipe_wm(struct drm_crtc > *crtc, > struct drm_device *dev = crtc->dev; > const struct drm_i915_private *dev_priv = dev->dev_private; > int level, max_level = ilk_wm_max_level(dev); > - /* LP0 watermark maximums depend on this pipe alone */ > - struct intel_wm_config config = { > - .num_pipes_active = 1, > - .sprites_enabled = params->spr.enabled, > - .sprites_scaled = params->spr.scaled, > - }; > struct ilk_wm_maximums max; > > pipe_wm->pipe_enabled = params->active; > @@ -2205,11 +2217,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc > *crtc, > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc); > > - /* LP0 watermarks always use 1/2 DDB partitioning */ > - ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max); > - > - /* At least LP0 must be valid */ > - if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0])) > + if (!ilk_validate_pipe_wm(dev, pipe_wm)) > return false; > > ilk_compute_wm_reg_maximums(dev, 1, &max); > -- > 1.8.3.2 > > ___ > 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 11/24] drm/i915: Check hw vs. sw watermark state after programming
2014-03-07 13:32 GMT-03:00 : > From: Ville Syrjälä > > Make sure we programmed the watermarks correctly, by reading out the > hardware state again after programming and comparing it with the > state we supposedly programmed into hardware. Dump the watermark > registers after a mismatch, very much like we for the pipe config. > The only difference is that we don't dump the entire watermark > software tracking state since that's spread around a bit. > > Signed-off-by: Ville Syrjälä This one could also have been split into more than one patch: first you extract the functions, then later you add the new callers. My only comment is: do we really want to check HW/SW state right after we write the register values? Shouldn't this be done at some other time, like the end of the modeset sequence? Still, the patch seems correct, so: Reviewed-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/intel_pm.c | 117 > > 1 file changed, 83 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index ba4b23e..e519578a1 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2524,15 +2524,84 @@ static bool _ilk_disable_lp_wm(struct > drm_i915_private *dev_priv, > return changed; > } > > +static void _ilk_pipe_wm_get_hw_state(struct drm_device *dev, > + enum pipe pipe, > + struct ilk_wm_values *hw) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + static const unsigned int wm0_pipe_reg[] = { > + [PIPE_A] = WM0_PIPEA_ILK, > + [PIPE_B] = WM0_PIPEB_ILK, > + [PIPE_C] = WM0_PIPEC_IVB, > + }; > + > + hw->wm_pipe[pipe] = I915_READ(wm0_pipe_reg[pipe]); > + > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > + hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe)); > +} > + > +static void _ilk_wm_get_hw_state(struct drm_device *dev, > +struct ilk_wm_values *hw) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + enum pipe pipe; > + > + for_each_pipe(pipe) > + _ilk_pipe_wm_get_hw_state(dev, pipe, hw); > + > + hw->wm_lp[0] = I915_READ(WM1_LP_ILK); > + hw->wm_lp[1] = I915_READ(WM2_LP_ILK); > + hw->wm_lp[2] = I915_READ(WM3_LP_ILK); > + > + hw->wm_lp_spr[0] = I915_READ(WM1S_LP_ILK); > + if (INTEL_INFO(dev)->gen >= 7) { > + hw->wm_lp_spr[1] = I915_READ(WM2S_LP_IVB); > + hw->wm_lp_spr[2] = I915_READ(WM3S_LP_IVB); > + } > + > + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > + hw->partitioning = (I915_READ(WM_MISC) & > WM_MISC_DATA_PARTITION_5_6) ? > + INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2; > + else if (IS_IVYBRIDGE(dev)) > + hw->partitioning = (I915_READ(DISP_ARB_CTL2) & > DISP_DATA_PARTITION_5_6) ? > + INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2; > + > + hw->enable_fbc_wm = > + !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS); > +} > + > +static void ilk_dump_wm_values(const struct ilk_wm_values *hw, > + const char *context) > +{ > + DRM_DEBUG_KMS("%s watermark values\n", context); > + DRM_DEBUG_KMS("WM_PIPE_A = 0x%08x\n", hw->wm_pipe[PIPE_A]); > + DRM_DEBUG_KMS("WM_PIPE_B = 0x%08x\n", hw->wm_pipe[PIPE_B]); > + DRM_DEBUG_KMS("WM_PIPE_C = 0x%08x\n", hw->wm_pipe[PIPE_C]); > + DRM_DEBUG_KMS("WM_LP_1 = 0x%08x\n", hw->wm_lp[0]); > + DRM_DEBUG_KMS("WM_LP_2 = 0x%08x\n", hw->wm_lp[1]); > + DRM_DEBUG_KMS("WM_LP_3 = 0x%08x\n", hw->wm_lp[2]); > + DRM_DEBUG_KMS("WM_LP_SPR_1 = 0x%08x\n", hw->wm_lp_spr[0]); > + DRM_DEBUG_KMS("WM_LP_SPR_2 = 0x%08x\n", hw->wm_lp_spr[1]); > + DRM_DEBUG_KMS("WM_LP_SPR_3 = 0x%08x\n", hw->wm_lp_spr[2]); > + DRM_DEBUG_KMS("WM_LINETIME_A = 0x%08x\n", hw->wm_linetime[PIPE_A]); > + DRM_DEBUG_KMS("WM_LINETIME_B = 0x%08x\n", hw->wm_linetime[PIPE_B]); > + DRM_DEBUG_KMS("WM_LINETIME_C = 0x%08x\n", hw->wm_linetime[PIPE_C]); > + DRM_DEBUG_KMS("enable FBC watermark = %d\n", hw->enable_fbc_wm); > + DRM_DEBUG_KMS("DDB partitioning = %s\n", > + hw->partitioning == INTEL_DDB_PART_1_2 ? "1/2" : "5/6"); > +} > + > /* > * The spec says we shouldn't write when we don't need, because every write > * causes WMs to be re-evaluated, expending some power. > */ > static void ilk_write_wm_values(struct drm_i915_private *dev_priv, > - struct ilk_wm_values *results) > + const struct ilk_wm_values *results) > { > struct drm_device *dev = dev_priv->dev; > struct ilk_wm_values *previous = &dev_priv->wm.hw; > + struct ilk_wm_values hw = {}; > unsigned int dirty; >
Re: [Intel-gfx] [PATCH] mm: Throttle shrinkers harder
On 04/22/2014 12:30 PM, Daniel Vetter wrote: >> > > During testing of i915.ko with working texture sets larger than RAM, we >> > > encounter OOM with plenty of memory still trapped within writeback, e.g: >> > > >> > > [ 42.386039] active_anon:10134 inactive_anon:1900781 isolated_anon:32 >> > > active_file:33 inactive_file:39 isolated_file:0 >> > > unevictable:0 dirty:0 writeback:337627 unstable:0 >> > > free:11985 slab_reclaimable:9458 slab_unreclaimable:23614 >> > > mapped:41 shmem:1560769 pagetables:1276 bounce:0 >> > > >> > > If we throttle for writeback following shrink_slab, this gives us time >> > > to wait upon the writeback generated by the i915.ko shinker: >> > > >> > > [ 4756.750808] active_anon:24386 inactive_anon:900793 isolated_anon:0 >> > > active_file:23 inactive_file:20 isolated_file:0 >> > > unevictable:0 dirty:0 writeback:0 unstable:0 >> > > free:5550 slab_reclaimable:5184 slab_unreclaimable:4888 >> > > mapped:3 shmem:472393 pagetables:1249 bounce:0 Could you get some dumps of the entire set of OOM information? These are only tiny snippets. Also, the vmstat output from the bug: > https://bugs.freedesktop.org/show_bug.cgi?id=72742 shows there being an *AWFUL* lot of swap I/O going on here. From the looks of it, we stuck ~2GB in swap and evicted another 1.5GB of page cache (although I guess that could be double-counting tmpfs getting swapped out too). Hmmm, was this one of the cases where you actually ran _out_ of swap? > 2 0 19472 33952296 36103240 19472 0 19472 1474 151 3 27 71 > 0 > 4 0 484964 66468296 31758640 465492 0 465516 2597 1395 0 32 > 66 2 > 0 2 751940 23692980 30228840 266976 688 266976 3681 636 0 27 > 66 6 > procs ---memory-- ---swap-- -io -system-- cpu > r b swpd free buff cache si sobibo in cs us sy id wa > 2 1 1244580 295336988 26069840 492896 0 492908 1237 311 1 9 > 50 41 > 0 2 2047996 28760988 20371440 803160 0 803160 1221 1291 1 15 > 69 14 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] 3.15-rc2: i915 regression: only top 20% of screen works in X
On Wed, Apr 23, 2014 at 10:22 PM, Pavel Machek wrote: > After update to 3.15-rc2, only top 20% of screen works on X. > > 00:02.0 VGA compatible controller: Intel Corporation 4 Series Chipset > Integrated Graphics Controller (rev 03) > > 00:02.1 Display controller: Intel Corporation 4 Series Chipset > Integrated Graphics Controller (rev 03) >Subsystem: Intel Corporation Device d614 >Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- >ParErr- Stepping- SERR- FastB2B- DisINTx- >Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >>TAbort- SERR- Latency: 0 >Region 0: Memory at d040 (64-bit, non-prefetchable) >[size=1M] >Capabilities: > > This worked before. I believe it worked in 3.14. It definitely works > in 3.11-rc2. Screenshot or more detailed description of what "only top 20% of screen works in X" means? Anything in dmesg? bisect result presuming that it reproduces reliably? Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] 3.15-rc2: i915 regression: only top 20% of screen works in X
Hi! After update to 3.15-rc2, only top 20% of screen works on X. 00:02.0 VGA compatible controller: Intel Corporation 4 Series Chipset Integrated Graphics Controller (rev 03) 00:02.1 Display controller: Intel Corporation 4 Series Chipset Integrated Graphics Controller (rev 03) Subsystem: Intel Corporation Device d614 Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- SERR- This worked before. I believe it worked in 3.14. It definitely works in 3.11-rc2. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/24] drm/i915: Merge LP1+ watermarks in safer way
On Wed, Apr 23, 2014 at 04:13:25PM -0300, Paulo Zanoni wrote: > 2014-03-07 13:32 GMT-03:00 : > > From: Ville Syrjälä > > > > On ILK when we disable a particular watermark level, we must > > Just to be clear: do you mean ILK or ILK+ here? IIRC ILK was where I saw it really easily. IVB didn't seem quite as sensitive to it. > > > maintain the actual watermark values for that level for some time > > (until the next vblank possibly). Otherwise we risk underruns. > > > > In order to achieve that result we must merge the LP1+ watermarks a > > bit differently since we must also merge levels that are to be > > disabled. We must also make sure we don't overflow the fields in the > > watermark registers in case the calculated watermarks come out too > > big to fit. > > > > As early as possbile we mark all computed watermark levels as > > disabled if they would exceed the register maximums. We make sure > > to leave the actual watermarks for such levels zeroed out. The during > > merging, we take the maxium values for every level, regardless if > > they're disabled or not. That may seem a bit pointless since at the > > moment all the watermark levels we merge should have their values > > zeroed if the level is already disabled. However soon we will be > > dealing with intermediate watermarks that, in addition to the new > > watermark values, also contain the previous watermark values, and so > > levels that are disabled may no longer be zeroed out. > > I am having a hard time here. Watermarks code is extremely complex > these days, and my brain does not have enough memory to think about > all the implications and side effects of all the changes you did here. > I have stared a this patch for a long time, and I don't think I fully > understand it. I think you should probably try to break this change > into some smaller steps, with nice commit messages. Some stupid > questions to help me clarify: > > As far as I understood, the biggest change of this patch is that when > some WM level is disabled, we won't write 0x0 to the WM register, but > we will write actual values, but with bit 31 set to zero, right? That's the idea. > On > the first sentence of the commit message, you say we must maintain the > old values for that level for some time, but on the current code we > won't keep the old values: we will generate new values, and if a level > that was previously enabled needs to be disabled, there's no guarantee > that the new value we will generate will be exactly the same as the > old-value-but-with-bit-31-disabled. Why does that work? It's not really hooked up yet. There's going to be another patch down the line that calculates the "intermediate" watermarks for an individual pipe as max(old,new). Those intermediate watermarks get applied to the affected pipe, and then as we merge the LP1+ watermarks from all pipes we must use similar logic. Ie. if one pipe has/had LP2=100 but another pipe has/had LP2 disabled we must write 100 to the register (w/o the enable bit set of course). So this patch just modifies the LP1+ merge logic to make that happen. Another patch will deal with the old vs. new situation. > Since it > doesn't make sense to me, I probably understood something very wrong > here... > > A little more below... > > > > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/intel_pm.c | 81 > > - > > 1 file changed, 64 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index f061ef1..ba4b23e 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -1921,6 +1921,16 @@ static void ilk_compute_wm_maximums(const struct > > drm_device *dev, > > max->fbc = ilk_fbc_wm_reg_max(dev); > > } > > > > +static void ilk_compute_wm_reg_maximums(struct drm_device *dev, > > + int level, > > + struct ilk_wm_maximums *max) > > +{ > > + max->pri = ilk_plane_wm_reg_max(dev, level, false); > > + max->spr = ilk_plane_wm_reg_max(dev, level, true); > > + max->cur = ilk_cursor_wm_reg_max(dev, level); > > + max->fbc = ilk_fbc_wm_reg_max(dev); > > +} > > + > > static bool ilk_validate_wm_level(int level, > > const struct ilk_wm_maximums *max, > > struct intel_wm_level *result) > > @@ -2178,9 +2188,6 @@ static bool intel_compute_pipe_wm(struct drm_crtc > > *crtc, > > }; > > struct ilk_wm_maximums max; > > > > - /* LP0 watermarks always use 1/2 DDB partitioning */ > > - ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max); > > - > > pipe_wm->pipe_enabled = params->active; > > pipe_wm->sprites_enabled = params->spr.enabled; > > pipe_wm->sprites_scaled = params->spr.scaled; > > @@ -2193,15 +2200,37 @@ static bool intel_compu
[Intel-gfx] [PATCH 3/3] drm/i915: Catch dirt in unused execbuffer fields
We need to make sure that userspace keeps on following the contract, otherwise we won't be able to use the reserved fields at all. Testcase: igt/gem_exec_params/*-dirt Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index c2e5d39a1df8..0f0aebdd8dbd 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1115,6 +1115,9 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, ret = -EFAULT; goto pre_mutex_err; } + } else { + if (args->DR1 || args->DR4 || args->cliprects_ptr) + return -EINVAL; } intel_runtime_pm_get(dev_priv); @@ -1392,6 +1395,9 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data, return -EINVAL; } + if (args->rsvd2 != 0) + return -EINVAL; + exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY); if (exec2_list == NULL) -- 1.8.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: Catch abuse of I915_EXEC_CONSTANTS_*
A bit tricky since 0 is also a valid constant ... Testcase: igt/gem_exec_params/rel-constants-* Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 40ae5ff0a031..c2e5d39a1df8 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1058,8 +1058,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, case I915_EXEC_CONSTANTS_REL_GENERAL: case I915_EXEC_CONSTANTS_ABSOLUTE: case I915_EXEC_CONSTANTS_REL_SURFACE: - if (ring == &dev_priv->ring[RCS] && - mode != dev_priv->relative_constants_mode) { + if (mode != 0 && ring != &dev_priv->ring[RCS]) + return -EINVAL; + + if (mode != dev_priv->relative_constants_mode) { if (INTEL_INFO(dev)->gen < 4) return -EINVAL; -- 1.8.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Catch abuse of I915_EXEC_GEN7_SOL_RESET
Currently we catch it, but silently succeed. Our userspace is better than this. Testcase: igt/gem_exec_params/sol-reset-* Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 0ec8621eb4f8..40ae5ff0a031 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -982,7 +982,7 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev, int ret, i; if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS]) - return 0; + return -EINVAL; ret = intel_ring_begin(ring, 4 * 3); if (ret) -- 1.8.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests: Add gem_exec_params
This fills all the gaps we've had in our execbuf testing. Overflow testing of the various arrays is already done by gem_reloc_overflow. Also add kms_flip_tiling to .gitignore. This will cause a bunch of failures since current kernels don't catch all fallout. Signed-off-by: Daniel Vetter --- tests/.gitignore| 2 + tests/Makefile.sources | 1 + tests/gem_exec_params.c | 212 3 files changed, 215 insertions(+) create mode 100644 tests/gem_exec_params.c diff --git a/tests/.gitignore b/tests/.gitignore index 146bab06b565..4c50bae93aa3 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -35,6 +35,7 @@ gem_exec_blt gem_exec_faulting_reloc gem_exec_lut_handle gem_exec_nop +gem_exec_params gem_exec_parse gem_fd_exhaustion gem_fenced_exec_thrash @@ -113,6 +114,7 @@ kms_addfb kms_cursor_crc kms_fbc_crc kms_flip +kms_flip_tiling kms_pipe_crc_basic kms_plane kms_render diff --git a/tests/Makefile.sources b/tests/Makefile.sources index c957ace2ace0..9b2d7cff1113 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -29,6 +29,7 @@ TESTS_progs_M = \ gem_exec_bad_domains \ gem_exec_faulting_reloc \ gem_exec_nop \ + gem_exec_params \ gem_exec_parse \ gem_fenced_exec_thrash \ gem_fence_thrash \ diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c new file mode 100644 index ..b1d996c530f5 --- /dev/null +++ b/tests/gem_exec_params.c @@ -0,0 +1,212 @@ +/* + * Copyright (c) 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Daniel Vetter + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "drm.h" + +#include "ioctl_wrappers.h" +#include "drmtest.h" +#include "intel_io.h" +#include "intel_chipset.h" +#include "igt_aux.h" + +#define LOCAL_I915_EXEC_VEBOX (4<<0) + +struct drm_i915_gem_execbuffer2 execbuf; +struct drm_i915_gem_exec_object2 gem_exec[1]; +uint32_t batch[2] = {MI_BATCH_BUFFER_END}; +uint32_t handle, devid; +int fd; + +igt_main +{ + igt_fixture { + fd = drm_open_any(); + + devid = intel_get_drm_devid(fd); + + handle = gem_create(fd, 4096); + gem_write(fd, handle, 0, batch, sizeof(batch)); + + gem_exec[0].handle = handle; + gem_exec[0].relocation_count = 0; + gem_exec[0].relocs_ptr = 0; + gem_exec[0].alignment = 0; + gem_exec[0].offset = 0; + gem_exec[0].flags = 0; + gem_exec[0].rsvd1 = 0; + gem_exec[0].rsvd2 = 0; + + execbuf.buffers_ptr = (uintptr_t)gem_exec; + execbuf.buffer_count = 1; + execbuf.batch_start_offset = 0; + execbuf.batch_len = 8; + execbuf.cliprects_ptr = 0; + execbuf.num_cliprects = 0; + execbuf.DR1 = 0; + execbuf.DR4 = 0; + execbuf.flags = 0; + i915_execbuffer2_set_context_id(execbuf, 0); + execbuf.rsvd2 = 0; + } + + igt_subtest("control") { + igt_assert(drmIoctl(fd, + DRM_IOCTL_I915_GEM_EXECBUFFER2, + &execbuf) == 0); + execbuf.flags = I915_EXEC_RENDER; + igt_assert(drmIoctl(fd, + DRM_IOCTL_I915_GEM_EXECBUFFER2, + &execbuf) == 0); + } + +#define RUN_FAIL(expected_errno) do { \ + igt_assert(drmIoctl(fd, \ + DRM_IOCTL_I915_GEM_EXECBUFFER2, \ + &execbuf) == -1); \ + igt_assert_cmpint(errno, ==, expected_errno); \ + } while(0) +
Re: [Intel-gfx] [PATCH 05/24] drm/i915: Merge LP1+ watermarks in safer way
2014-03-07 13:32 GMT-03:00 : > From: Ville Syrjälä > > On ILK when we disable a particular watermark level, we must Just to be clear: do you mean ILK or ILK+ here? > maintain the actual watermark values for that level for some time > (until the next vblank possibly). Otherwise we risk underruns. > > In order to achieve that result we must merge the LP1+ watermarks a > bit differently since we must also merge levels that are to be > disabled. We must also make sure we don't overflow the fields in the > watermark registers in case the calculated watermarks come out too > big to fit. > > As early as possbile we mark all computed watermark levels as > disabled if they would exceed the register maximums. We make sure > to leave the actual watermarks for such levels zeroed out. The during > merging, we take the maxium values for every level, regardless if > they're disabled or not. That may seem a bit pointless since at the > moment all the watermark levels we merge should have their values > zeroed if the level is already disabled. However soon we will be > dealing with intermediate watermarks that, in addition to the new > watermark values, also contain the previous watermark values, and so > levels that are disabled may no longer be zeroed out. I am having a hard time here. Watermarks code is extremely complex these days, and my brain does not have enough memory to think about all the implications and side effects of all the changes you did here. I have stared a this patch for a long time, and I don't think I fully understand it. I think you should probably try to break this change into some smaller steps, with nice commit messages. Some stupid questions to help me clarify: As far as I understood, the biggest change of this patch is that when some WM level is disabled, we won't write 0x0 to the WM register, but we will write actual values, but with bit 31 set to zero, right? On the first sentence of the commit message, you say we must maintain the old values for that level for some time, but on the current code we won't keep the old values: we will generate new values, and if a level that was previously enabled needs to be disabled, there's no guarantee that the new value we will generate will be exactly the same as the old-value-but-with-bit-31-disabled. Why does that work? Since it doesn't make sense to me, I probably understood something very wrong here... A little more below... > > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_pm.c | 81 > - > 1 file changed, 64 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f061ef1..ba4b23e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1921,6 +1921,16 @@ static void ilk_compute_wm_maximums(const struct > drm_device *dev, > max->fbc = ilk_fbc_wm_reg_max(dev); > } > > +static void ilk_compute_wm_reg_maximums(struct drm_device *dev, > + int level, > + struct ilk_wm_maximums *max) > +{ > + max->pri = ilk_plane_wm_reg_max(dev, level, false); > + max->spr = ilk_plane_wm_reg_max(dev, level, true); > + max->cur = ilk_cursor_wm_reg_max(dev, level); > + max->fbc = ilk_fbc_wm_reg_max(dev); > +} > + > static bool ilk_validate_wm_level(int level, > const struct ilk_wm_maximums *max, > struct intel_wm_level *result) > @@ -2178,9 +2188,6 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc, > }; > struct ilk_wm_maximums max; > > - /* LP0 watermarks always use 1/2 DDB partitioning */ > - ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max); > - > pipe_wm->pipe_enabled = params->active; > pipe_wm->sprites_enabled = params->spr.enabled; > pipe_wm->sprites_scaled = params->spr.scaled; > @@ -2193,15 +2200,37 @@ static bool intel_compute_pipe_wm(struct drm_crtc > *crtc, > if (params->spr.scaled) > max_level = 0; > > - for (level = 0; level <= max_level; level++) > - ilk_compute_wm_level(dev_priv, level, params, > -&pipe_wm->wm[level]); > + ilk_compute_wm_level(dev_priv, 0, params, &pipe_wm->wm[0]); > > if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc); > > + /* LP0 watermarks always use 1/2 DDB partitioning */ > + ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max); > + > /* At least LP0 must be valid */ > - return ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]); > + if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0])) > + return false; > + For example, this chunk introduces an early return to the function. I really think this should be on
Re: [Intel-gfx] [PATCH 2/3] drm/plane-helper: Add drm_primary_helper_check_update()
On Wed, Apr 23, 2014 at 10:04:00AM -0700, Matt Roper wrote: > Pull the parameter checking from drm_primary_helper_update() out into > its own function; drivers that provide their own setplane() > implementations rather than using the helper may still want to share > this parameter checking logic. > > A few of the checks here were also updated based on suggestions by > Ville Syrjälä. > > Cc: Ville Syrjälä > Cc: dri-de...@lists.freedesktop.org > Signed-off-by: Matt Roper > --- > drivers/gpu/drm/drm_plane_helper.c | 148 > + > include/drm/drm_plane_helper.h | 9 +++ > 2 files changed, 128 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane_helper.c > b/drivers/gpu/drm/drm_plane_helper.c > index 65c4a00..9bbbdf2 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -66,6 +66,102 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, > } > > /** > + * drm_primary_helper_check_update() - Check primary plane update for > validity > + * @plane: plane object to update > + * @crtc: owning CRTC of owning plane > + * @fb: framebuffer to flip onto plane > + * @crtc_x: x offset of primary plane on crtc > + * @crtc_y: y offset of primary plane on crtc > + * @crtc_w: width of primary plane rectangle on crtc > + * @crtc_h: height of primary plane rectangle on crtc > + * @src_x: x offset of @fb for panning > + * @src_y: y offset of @fb for panning > + * @src_w: width of source rectangle in @fb > + * @src_h: height of source rectangle in @fb > + * @can_scale: is primary plane scaling legal? > + * @can_position: is it legal to position the primary plane such that it > + *doesn't cover the entire crtc? > + * > + * Checks that a desired primary plane update is valid. Drivers that provide > + * their own primary plane handling may still wish to call this function to > + * avoid duplication of error checking code. > + * > + * RETURNS: > + * Zero if update appears valid, error code on failure > + */ > +int drm_primary_helper_check_update(struct drm_plane *plane, > + struct drm_crtc *crtc, > + struct drm_framebuffer *fb, > + int crtc_x, int crtc_y, > + unsigned int crtc_w, unsigned int crtc_h, > + uint32_t src_x, uint32_t src_y, > + uint32_t src_w, uint32_t src_h, > + bool can_scale, > + bool can_position) > +{ > + struct drm_rect dest = { > + .x1 = crtc_x, > + .y1 = crtc_y, > + .x2 = crtc_x + crtc_w, > + .y2 = crtc_y + crtc_h, > + }; > + struct drm_rect src = { > + .x1 = src_x >> 16, > + .y1 = src_y >> 16, > + .x2 = (src_x + src_w) >> 16, > + .y2 = (src_y + src_h) >> 16, I think you want '(x>>16) + (y>>16)' instead. Otherwise you may end up increasing the source width/height. > + }; > + const struct drm_rect clip = { > + .x2 = crtc->mode.hdisplay, > + .y2 = crtc->mode.vdisplay, > + }; > + int hscale, vscale; > + int visible; > + > + if (!crtc->enabled) { > + DRM_DEBUG_KMS("Cannot update primary plane of a disabled > CRTC.\n"); > + return -EINVAL; > + } We allow this for sprites, so I'd allow it for everything. I'd be fine with leaving this restriction in drm_primary_helper_update() simply because I have no interst in auditing every other driver. Although I think we still overwrite the primary plane configure during setcrtc. We should really change that so that the user can pre-configure all the planes and then watch them pop into view during a modeset as previously configured. > + > + /* setplane API takes shifted source rectangle values; unshift them */ > + src_x >>= 16; > + src_y >>= 16; > + src_w >>= 16; > + src_h >>= 16; > + > + /* check scaling */ > + if (!can_scale && (crtc_w != src_w || crtc_h != src_h)) { > + DRM_DEBUG_KMS("Can't scale primary plane\n"); > + return -EINVAL; > + } > + > + /* > + * Drivers that can scale should perform their own min/max scale > + * factor checks. > + */ > + hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX); > + vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX); > + visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale); w/o sub-pixel coordinates the scaling factors will be truncated to integers, which makes the clipping rather bogus if the plane can actually scale. I think I'd just make this code assume that scaling isn't supported, and if the driver supports scaling it can just implent the appropriate code itself. We can try to refactor some of the scaling aware code from intel_sprite later if warranted.
Re: [Intel-gfx] [PATCH 1/3] drm: Check CRTC compatibility in setplane
On Wed, Apr 23, 2014 at 11:26:04AM -0700, Matt Roper wrote: > On Wed, Apr 23, 2014 at 08:03:50PM +0200, Daniel Vetter wrote: > > On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote: > > > The DRM core setplane code should check that the plane is usable on the > > > specified CRTC before calling into the driver. > > > > > > Prior to this patch, a plane's possible_crtcs field was purely > > > informational for userspace and was never actually verified at the > > > kernel level (aside from the primary plane helper). > > > > > > Cc: dri-de...@lists.freedesktop.org > > > Signed-off-by: Matt Roper > > > > Do you have a nasty igt somewhere which tries to use a plane on the wrong > > crtc? Especially useful since our i915 code and hw relies on this. > > -Daniel > > Not yet; I'll add/modify a test to include that. I'm still working on > some other i-g-t test updates for the primary plane stuff based on your > previous feedback. > > Speaking of i-g-t testing, what is the expected behavior if someone > issues a pageflip ioctl while the primary plane is disabled? I'd expect > it to return an error, but the -EBUSY currently returned by the DRM core > seems a bit confusing/misleading in my opinion. The comments indicate > that the existing assumption is that crtc->primary->fb == NULL indicates > a hotplug event that userspace hasn't noticed yet, although now we > clearly have other ways to hit that error, so I'm not sure EBUSY is > really the right response. That comment is outdated since nowadays the kernel doesn't randomly kill a crtc if its outputs gets unplugged. I think a simple -EINVAL should be good. We'd need to update kms_flip with that new value though. A quick grep through the intel ddx shows that we don't really depend on this either way. -EBUSY for a disabled primary plane (whether the crtc is on or not doesn't matter imo) really makes no sense. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm: Check CRTC compatibility in setplane
On Wed, Apr 23, 2014 at 08:03:50PM +0200, Daniel Vetter wrote: > On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote: > > The DRM core setplane code should check that the plane is usable on the > > specified CRTC before calling into the driver. > > > > Prior to this patch, a plane's possible_crtcs field was purely > > informational for userspace and was never actually verified at the > > kernel level (aside from the primary plane helper). > > > > Cc: dri-de...@lists.freedesktop.org > > Signed-off-by: Matt Roper > > Do you have a nasty igt somewhere which tries to use a plane on the wrong > crtc? Especially useful since our i915 code and hw relies on this. > -Daniel Not yet; I'll add/modify a test to include that. I'm still working on some other i-g-t test updates for the primary plane stuff based on your previous feedback. Speaking of i-g-t testing, what is the expected behavior if someone issues a pageflip ioctl while the primary plane is disabled? I'd expect it to return an error, but the -EBUSY currently returned by the DRM core seems a bit confusing/misleading in my opinion. The comments indicate that the existing assumption is that crtc->primary->fb == NULL indicates a hotplug event that userspace hasn't noticed yet, although now we clearly have other ways to hit that error, so I'm not sure EBUSY is really the right response. Matt > > > --- > > drivers/gpu/drm/drm_crtc.c | 7 +++ > > drivers/gpu/drm/drm_plane_helper.c | 6 -- > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 461d19b..b6d6c04 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -2140,6 +2140,13 @@ int drm_mode_setplane(struct drm_device *dev, void > > *data, > > } > > crtc = obj_to_crtc(obj); > > > > + /* Check whether this plane is usable on this CRTC */ > > + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) { > > + DRM_DEBUG_KMS("Invalid crtc for plane\n"); > > + ret = -EINVAL; > > + goto out; > > + } > > + > > fb = drm_framebuffer_lookup(dev, plane_req->fb_id); > > if (!fb) { > > DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", > > diff --git a/drivers/gpu/drm/drm_plane_helper.c > > b/drivers/gpu/drm/drm_plane_helper.c > > index b72736d..65c4a00 100644 > > --- a/drivers/gpu/drm/drm_plane_helper.c > > +++ b/drivers/gpu/drm/drm_plane_helper.c > > @@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, > > struct drm_crtc *crtc, > > return -EINVAL; > > } > > > > - /* Primary planes are locked to their owning CRTC */ > > - if (plane->possible_crtcs != drm_crtc_mask(crtc)) { > > - DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); > > - return -EINVAL; > > - } > > - > > /* Disallow scaling */ > > src_w >>= 16; > > src_h >>= 16; > > -- > > 1.8.5.1 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm: Check CRTC compatibility in setplane
On Wed, Apr 23, 2014 at 10:03:59AM -0700, Matt Roper wrote: > The DRM core setplane code should check that the plane is usable on the > specified CRTC before calling into the driver. > > Prior to this patch, a plane's possible_crtcs field was purely > informational for userspace and was never actually verified at the > kernel level (aside from the primary plane helper). > > Cc: dri-de...@lists.freedesktop.org > Signed-off-by: Matt Roper Do you have a nasty igt somewhere which tries to use a plane on the wrong crtc? Especially useful since our i915 code and hw relies on this. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 7 +++ > drivers/gpu/drm/drm_plane_helper.c | 6 -- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 461d19b..b6d6c04 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -2140,6 +2140,13 @@ int drm_mode_setplane(struct drm_device *dev, void > *data, > } > crtc = obj_to_crtc(obj); > > + /* Check whether this plane is usable on this CRTC */ > + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) { > + DRM_DEBUG_KMS("Invalid crtc for plane\n"); > + ret = -EINVAL; > + goto out; > + } > + > fb = drm_framebuffer_lookup(dev, plane_req->fb_id); > if (!fb) { > DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", > diff --git a/drivers/gpu/drm/drm_plane_helper.c > b/drivers/gpu/drm/drm_plane_helper.c > index b72736d..65c4a00 100644 > --- a/drivers/gpu/drm/drm_plane_helper.c > +++ b/drivers/gpu/drm/drm_plane_helper.c > @@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, > struct drm_crtc *crtc, > return -EINVAL; > } > > - /* Primary planes are locked to their owning CRTC */ > - if (plane->possible_crtcs != drm_crtc_mask(crtc)) { > - DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); > - return -EINVAL; > - } > - > /* Disallow scaling */ > src_w >>= 16; > src_h >>= 16; > -- > 1.8.5.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 0/3] render state initialization (bdw rc6)
On Tue, Apr 22, 2014 at 09:51:56PM +0100, Chris Wilson wrote: > On Tue, Apr 22, 2014 at 08:19:41PM +0300, Mika Kuoppala wrote: > > Hi, > > > > Here are patches to initialize first render context to a hopefully, > > valid state. If pipeline/context is not initialized and we enter rc6 on bdw, > > the render ring can hung on the first batch submitted. That is atleast > > the hypothesis this work is based on. > > > > The states are stripped from rendercopy_genX's from i-g-t/lib and > > the state generators are part of i-g-t also. The states are > > propably overshoot from what can be consider the minimal valid > > (null) state on the pipeline. I just initialized everything rendercopy > > does and haven't really put effort on optimizing the state until > > I get some test results that this indeed solves anything. > > > > The state generators can be found here but they are not needed for testing. > > http://cgit.freedesktop.org/~miku/intel-gpu-tools/log/?h=null_state_gen > > > > Gen7 and Gen8 seems to atleast survive the boot but Gen6 is totally > > untested. > > > > Here is the branch for testing: > > http://cgit.freedesktop.org/~miku/drm-intel/log/?h=render_state > > > > I am interested to know if these patches make matters better/worse for those > > who have issues with first batch hanging on bdw, but as always, any feedback > > is greatly appreciated. > > > > Mika Kuoppala (3): > > drm/i915: export vmap_batch from command parser > It's only a single page, it does not need to be vmapped. > > > drm/i915: add render state initialization > > drm/i915: add null render states for gen6, gen7 and gen8 > > I still don't buy that this is anything more than papering over a > problem. The state you load into the context is invalid as soon as it is > executed, which may lead to problems, we don't know since the problem is > not being discussed, and it will certainly be more explicit if the right > bits are poked into the context directly to keep the hw from falling over. > -Chris > Paper is better than no paper. Anyway there are a couple of units where we know NULL is better than not NULL (VFE is one). I have been unable to get an exact reason why this is needed so that we know exactly what to fix. It has been a very frustrating experience. We could try to get info on what Windows does, but we may not be able to get a, "why." -- Ben Widawsky, 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: Sanitize the enable_ppgtt module option once
On Wed, Apr 23, 2014 at 11:37:46AM +0300, Jani Nikula wrote: > On Tue, 22 Apr 2014, Daniel Vetter wrote: > > Otherwise we'll end up spamming dmesg on every context creation on snb > > with vt-d enabled. This regression was introduced in > > > > commit 246cbfb5fb9a1ca0997fbb135464c1ff5bb9c549 > > Author: Ben Widawsky > > Date: Fri Dec 6 14:11:14 2013 -0800 > > > > drm/i915: Reorganize intel_enable_ppgtt > > > > References: https://lkml.org/lkml/2014/4/17/599 > > Cc: Alessandro Suardi > > Cc: Ben Widawsky > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 26 +++--- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index 0d514ff9b94c..47491c4a1181 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -34,25 +34,35 @@ static void gen8_setup_private_ppat(struct > > drm_i915_private *dev_priv); > > > > bool intel_enable_ppgtt(struct drm_device *dev, bool full) > > { > > - if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) > > + if (i915.enable_ppgtt == 0) > > return false; > > > > if (i915.enable_ppgtt == 1 && full) > > return false; > > > > + return true; > > +} > > + > > +static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > > +{ > > + if (enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) > > + return 0; > > + > > + if (i915.enable_ppgtt == 1) > > + return 1; > > + > > + if (i915.enable_ppgtt == 2 && HAS_PPGTT(dev)) > > + return 2; > > You should probably either pass enable_ppgtt as parameter and use that > exclusively, or not pass it and refer to i915.enable_ppgtt directly, but > not mix them. New patch on its way with Chris' comments also applied. > > > + > > #ifdef CONFIG_INTEL_IOMMU > > /* Disable ppgtt on SNB if VT-d is on. */ > > if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped) { > > DRM_INFO("Disabling PPGTT because VT-d is on\n"); > > - return false; > > + return 0; > > } > > #endif > > > > - /* Full ppgtt disabled by default for now due to issues. */ > > - if (full) > > - return HAS_PPGTT(dev) && (i915.enable_ppgtt == 2); > > - else > > - return HAS_ALIASING_PPGTT(dev); > > + return HAS_ALIASING_PPGTT(dev) ? 1 : 0; > > > This conflicts in -fixes due to > commit 8d214b7d9c45f4af23ce41b2bc74f79c44f760de > Author: Ben Widawsky > Date: Mon Mar 24 18:06:00 2014 -0700 > > drm/i915: Allow full PPGTT with param override > > Should I incorporate that in the conflict resolution for simplicity, > letting 3.15 users also play with full ppgtt with the module param? Yeah I guess it's easiest to just cherry-pick that one to -fixes, too. -Daniel > > > BR, > Jani. > > > > } > > > > > > @@ -1157,6 +1167,8 @@ int i915_gem_init_ppgtt(struct drm_device *dev, > > struct i915_hw_ppgtt *ppgtt) > > struct drm_i915_private *dev_priv = dev->dev_private; > > int ret = 0; > > > > + i915.enable_ppgtt = sanitize_enable_ppgtt(dev, i915.enable_ppgtt); > > + > > ppgtt->base.dev = dev; > > ppgtt->base.scratch = dev_priv->gtt.base.scratch; > > > > -- > > 1.9.2 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Sanitize the enable_ppgtt module option once
Otherwise we'll end up spamming dmesg on every context creation on snb with vt-d enabled. This regression was introduced in commit 246cbfb5fb9a1ca0997fbb135464c1ff5bb9c549 Author: Ben Widawsky Date: Fri Dec 6 14:11:14 2013 -0800 drm/i915: Reorganize intel_enable_ppgtt As the i915.enable_ppgtt is read-only it cannot be changed after the module is loaded and so we can perform an early sanitization of the values. v2: - Add comment and pimp commit message (Chris) - Use the param consistently (Jani) References: https://lkml.org/lkml/2014/4/17/599 Cc: Alessandro Suardi Cc: Ben Widawsky Reviewed-by: Chris Wilson Reviewed-by: Ben Widawsky Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.c | 32 +--- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 0d514ff9b94c..70e8892f81a6 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -34,25 +34,35 @@ static void gen8_setup_private_ppat(struct drm_i915_private *dev_priv); bool intel_enable_ppgtt(struct drm_device *dev, bool full) { - if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) + if (i915.enable_ppgtt == 0) return false; if (i915.enable_ppgtt == 1 && full) return false; + return true; +} + +static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) +{ + if (enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) + return 0; + + if (enable_ppgtt == 1) + return 1; + + if (enable_ppgtt == 2 && HAS_PPGTT(dev)) + return 2; + #ifdef CONFIG_INTEL_IOMMU /* Disable ppgtt on SNB if VT-d is on. */ if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped) { DRM_INFO("Disabling PPGTT because VT-d is on\n"); - return false; + return 0; } #endif - /* Full ppgtt disabled by default for now due to issues. */ - if (full) - return HAS_PPGTT(dev) && (i915.enable_ppgtt == 2); - else - return HAS_ALIASING_PPGTT(dev); + return HAS_ALIASING_PPGTT(dev) ? 1 : 0; } @@ -1157,6 +1167,14 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) struct drm_i915_private *dev_priv = dev->dev_private; int ret = 0; + /* +* i915.enable_ppgtt is read-only, so do an early pass to validate the +* user's requested state against the hardware/driver capabilities. We +* do this now so that we can print out any log messages once rather +* than every time we check intel_enable_ppgtt(). +*/ + i915.enable_ppgtt = sanitize_enable_ppgtt(dev, i915.enable_ppgtt); + ppgtt->base.dev = dev; ppgtt->base.scratch = dev_priv->gtt.base.scratch; -- 1.9.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] tests/gem_userptr_blits: Expanded userptr test cases
On Wed, Apr 23, 2014 at 08:32:27AM -0700, Volkin, Bradley D wrote: > On Wed, Apr 23, 2014 at 06:33:40AM -0700, Tvrtko Ursulin wrote: > > > > On 04/18/2014 06:10 PM, Volkin, Bradley D wrote: > > > On Wed, Mar 19, 2014 at 04:13:04AM -0700, Tvrtko Ursulin wrote: > > >> From: Tvrtko Ursulin > > >> > > >> A set of userptr test cases to support the new feature. > > >> > > >> For the eviction and swapping stress testing I have extracted > > >> some common behaviour from gem_evict_everything and made both > > >> test cases use it to avoid duplicating the code. > > >> > > >> Both unsynchronized and synchronized userptr objects are > > >> tested but the latter set of tests will be skipped if kernel > > >> is compiled without MMU_NOTIFIERS. > > >> > > >> Also, with 32-bit userspace swapping tests are skipped if > > >> the system has a lot more RAM than process address space. > > >> Forking swapping tests are not skipped since they can still > > >> trigger swapping by cumulative effect. > > >> > > >> v2: > > >> * Fixed dmabuf test. > > >> * Added test for rejecting read-only. > > >> * Fixed ioctl detection for latest kernel patch. > > >> > > >> v3: > > >> * Updated copy() for Gen8+. > > >> * Fixed ioctl detection on kernels without MMU_NOTIFIERs. > > >> > > >> Signed-off-by: Tvrtko Ursulin > > > > > > A number of the comments I made on patch 3 apply here as well. > > > The sizeof(linear) thing is more prevalent in this test, though > > > it looks like linear is at least used. Other than those comments > > > this looks good to me. > > > > Believe it or not that sizeof(linear) "idiom" I inherited from other > > blitter tests. Personally I don't care one way or another. But since it > > makes sense to get rid of it for the benchmark part, perhaps I should > > change it here as well to be consistent. How strongly do you feel > > strongly about this? > > I think changing it would be slightly more readable, but if it's > consistent with other blit tests then I don't feel too strongly > about it. In fact, consistency with the other tests might be the > better approach. I'm fine with whichever approach you prefer. Some of the igt tests are so Gross Hacks that justifying ugliness in new tests with consistency is ill-advised ;-) If you find some spare cycles to clean up existing tests that would be awesome, but I don't mind if they keep being ugly. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] xf86-video-intel: enable hw-generated binding tables
On Wed, Apr 23, 2014 at 06:52:09PM +0200, Daniel Vetter wrote: > On Wed, Apr 23, 2014 at 1:21 PM, Abdiel Janulgue > wrote: > > I've already tried disabling RS at the end of every batch so that next batch > > in different context can continue to use older non-RS format. That does not > > work either and still causes hangs. > > > > What I've seen so far, it seems GPU does not like switching the format of > > commands from RS-format to non-RS format. It's either one way or the other. > > If > > switched on, it affects subsequent contexes henceforth expecting RS-format > > commands until the GPU gets reset. That's probably the note in bspec: > > > > "the binding table generator feature has a simple all or nothing model". > > Oh hooray, that's just awesome :( So we indeed need to stop the gpu > and reset it if there's a non-RS render batch after any RS render > batch. I'm not sure I buy it. There's an example in the spec showing how to disable the RS around eg. GPGPU workloads and re-enable it for 3D workloads even within one batch. I guess it's possible that the GPGPU pipeline mode is somehow special here, but since the RS state is supposed to be saved to the context I'm thinking you should be able to disable it whenever you want. What's really confusing with that example is the fact that it first re-enables the binding tables and then the RS, but then there's a note after that saying you have to those steps in the opposite order. Anyhoo, I'm wondering if the problems are simply due to disabling RS but leaving the binding tables enabled? So one idea might be that when we switch from executing an RS batch to a non-RS batch, we should disable the RS and binding tables manually. We would then have to demand that any user batch which needs the binding tables enabled must enable them, even if if we just executed a batch that already used the binding tables. And when we need to disable the RS and binding tables we could either have the kernel do that autmagically when it notices a RS->non-RS transition, or we could also demand that all user batches must disable the binding tables at the end. But maybe demanding that from every batch would incur some extra overhead? In any case it should be fairly easy to try that and see if it cures the hangs. Or are you already doing things that way? -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanitize the enable_ppgtt module option once
On Wed, Apr 23, 2014 at 09:18:03AM +0200, Daniel Vetter wrote: > On Tue, Apr 22, 2014 at 10:22:15PM +0100, Chris Wilson wrote: > > On Tue, Apr 22, 2014 at 10:17:50PM +0200, Daniel Vetter wrote: > > > Otherwise we'll end up spamming dmesg on every context creation on snb > > > with vt-d enabled. This regression was introduced in > > > > > > commit 246cbfb5fb9a1ca0997fbb135464c1ff5bb9c549 > > > Author: Ben Widawsky > > > Date: Fri Dec 6 14:11:14 2013 -0800 > > > > > > drm/i915: Reorganize intel_enable_ppgtt > > > > I started to consider what would happen if i915.enable_ppgtt changed on > > the fly, but then saw that it is 0400 and this pre-initialisation makes > > a lot of sense. So maybe we could mention that here: > > > > As the i915.enable_ppgtt is read-only it cannot be changed after the > > module is loaded and so we can perform an early sanitization of the > > values. > > > > > References: https://lkml.org/lkml/2014/4/17/599 > > > Cc: Alessandro Suardi > > > Cc: Ben Widawsky > > > Signed-off-by: Daniel Vetter > > > > If you care to add in some of the comments, > > Reviewed-by: Chris Wilson > > Fully agreed on both since the first thing I've checked is that the > enable_ppgtt option is indeed 0400 ;-) > > Jani, can you please apply Chris' commit message text and comment when > merging? > -Daniel Here as well: Reviewed-by: Ben Widawsky > > > > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 26 +++--- > > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > index 0d514ff9b94c..47491c4a1181 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > @@ -34,25 +34,35 @@ static void gen8_setup_private_ppat(struct > > > drm_i915_private *dev_priv); > > > > > > bool intel_enable_ppgtt(struct drm_device *dev, bool full) > > > { > > > - if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) > > > + if (i915.enable_ppgtt == 0) > > > return false; > > > > > > if (i915.enable_ppgtt == 1 && full) > > > return false; > > > > > > + return true; > > > +} > > > + > > > > /* i915.enable_ppgtt is read-only, so do an early pass to validate > > * the user's requested state against the hardware/driver capabilities. > > * We do this now so that we can print out any log messages once rather > > * than every time we check intel_enable_ppgtt(). > > */ > > > +static int sanitize_enable_ppgtt(struct drm_device *dev, int > > > enable_ppgtt) > > > +{ > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ben Widawsky, 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/3] tests/gem_userptr_blits: Expanded userptr test cases
Reviewed-by: Brad Volkin On Wed, Apr 23, 2014 at 05:38:33PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > A set of userptr test cases to support the new feature. > > For the eviction and swapping stress testing I have extracted > some common behaviour from gem_evict_everything and made both > test cases use it to avoid duplicating the code. > > Both unsynchronized and synchronized userptr objects are > tested but the latter set of tests will be skipped if kernel > is compiled without MMU_NOTIFIERS. > > Also, with 32-bit userspace swapping tests are skipped if > the system has a lot more RAM than process address space. > Forking swapping tests are not skipped since they can still > trigger swapping by cumulative effect. > > v2: >* Fixed dmabuf test. >* Added test for rejecting read-only. >* Fixed ioctl detection for latest kernel patch. > > v3: >* Use ALIGN macro. >* Catchup with big lib/ reorganization. >* Fixed up some warnings. > > Signed-off-by: Tvrtko Ursulin > --- > tests/.gitignore |1 + > tests/Makefile.sources|1 + > tests/gem_userptr_blits.c | 1247 > + > 3 files changed, 1249 insertions(+) > create mode 100644 tests/gem_userptr_blits.c > > diff --git a/tests/.gitignore b/tests/.gitignore > index 146bab0..ff193bd 100644 > --- a/tests/.gitignore > +++ b/tests/.gitignore > @@ -95,6 +95,7 @@ gem_tiling_max_stride > gem_unfence_active_buffers > gem_unref_active_buffers > gem_vmap_blits > +gem_userptr_blits > gem_wait_render_timeout > gem_write_read_ring_switch > gen3_mixed_blits > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index c957ace..09d6aa3 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -121,6 +121,7 @@ TESTS_progs = \ > gem_unfence_active_buffers \ > gem_unref_active_buffers \ > gem_vmap_blits \ > + gem_userptr_blits \ > gem_wait_render_timeout \ > gen3_mixed_blits \ > gen3_render_linear_blits \ > diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c > new file mode 100644 > index 000..03af58e > --- /dev/null > +++ b/tests/gem_userptr_blits.c > @@ -0,0 +1,1247 @@ > +/* > + * Copyright © 2009-2014 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: > + *Eric Anholt > + *Chris Wilson > + *Tvrtko Ursulin > + * > + */ > + > +/** @file gem_userptr_blits.c > + * > + * This is a test of doing many blits using a mixture of normal system pages > + * and uncached linear buffers, with a working set larger than the > + * aperture size. > + * > + * The goal is to simply ensure the basics work. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "drm.h" > +#include "i915_drm.h" > +#include "drmtest.h" > +#include "intel_bufmgr.h" > +#include "intel_batchbuffer.h" > +#include "intel_chipset.h" > +#include "ioctl_wrappers.h" > + > +#include "eviction_common.c" > + > +#ifndef PAGE_SIZE > +#define PAGE_SIZE 4096 > +#endif > + > +#define LOCAL_I915_GEM_USERPTR 0x34 > +#define LOCAL_IOCTL_I915_GEM_USERPTR DRM_IOWR (DRM_COMMAND_BASE + > LOCAL_I915_GEM_USERPTR, struct local_i915_gem_userptr) > +struct local_i915_gem_userptr { > + uint64_t user_ptr; > + uint64_t user_size; > + uint32_t flags; > +#define LOCAL_I915_USERPTR_READ_ONLY (1<<0) > +#define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31) > + uint32_t handle; > +}; > + > +static uint32_t userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; > + > +#define WIDTH 512 > +#define HEIGHT 512 > + > +static uint32_t linear[WIDTH*HEIGHT]; > + > +static void gem_userptr_test_unsynchronized(void) > +{ > + userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; > +} > + > +static void gem_u
Re: [Intel-gfx] [PATCH 3/3] tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact
On Wed, Apr 23, 2014 at 05:38:35PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > This adds a small benchmark for the new userptr functionality. > > Apart from basic surface creation and destruction, also tested is the > impact of having userptr surfaces in the process address space. Reason > for that is the impact of MMU notifiers on common address space > operations like munmap() which is per process. > > v2: > * Moved to benchmarks. > * Added pointer read/write tests. > * Changed output to say iterations per second instead of > operations per second. > * Multiply result by batch size for multi-create* tests > for a more comparable number with create-destroy test. > > v3: > * Use ALIGN macro. > * Catchup with big lib/ reorganization. > * Removed unused code and one global variable. > * Fixed up some warnings. > > Signed-off-by: Tvrtko Ursulin > Cc: Chris Wilson > --- > benchmarks/.gitignore | 1 + > benchmarks/Makefile.sources| 3 +- > benchmarks/gem_userptr_benchmark.c | 497 > + > 3 files changed, 500 insertions(+), 1 deletion(-) > create mode 100644 benchmarks/gem_userptr_benchmark.c > > diff --git a/benchmarks/.gitignore b/benchmarks/.gitignore > index ddea6f7..09e5bd8 100644 > --- a/benchmarks/.gitignore > +++ b/benchmarks/.gitignore > @@ -1,3 +1,4 @@ > +gem_userptr_benchmark > intel_upload_blit_large > intel_upload_blit_large_gtt > intel_upload_blit_large_map > diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources > index f9da579..60bdae2 100644 > --- a/benchmarks/Makefile.sources > +++ b/benchmarks/Makefile.sources > @@ -2,4 +2,5 @@ bin_PROGRAMS = \ > intel_upload_blit_large \ > intel_upload_blit_large_gtt \ > intel_upload_blit_large_map \ > - intel_upload_blit_small > + intel_upload_blit_small \ > + gem_userptr_benchmark > diff --git a/benchmarks/gem_userptr_benchmark.c > b/benchmarks/gem_userptr_benchmark.c > new file mode 100644 > index 000..a51201c > --- /dev/null > +++ b/benchmarks/gem_userptr_benchmark.c > @@ -0,0 +1,497 @@ > +/* > + * Copyright © 2014 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > + * IN THE SOFTWARE. > + * > + * Authors: > + *Tvrtko Ursulin > + * > + */ > + > +/** @file gem_userptr_benchmark.c > + * > + * Benchmark the userptr code and impact of having userptr surfaces > + * in process address space on some normal operations. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "drm.h" > +#include "i915_drm.h" > +#include "drmtest.h" > +#include "intel_bufmgr.h" > +#include "intel_batchbuffer.h" > +#include "intel_chipset.h" > +#include "ioctl_wrappers.h" > +#include "igt_aux.h" > + > +#ifndef PAGE_SIZE > + #define PAGE_SIZE 4096 > +#endif > + > +#define LOCAL_I915_GEM_USERPTR 0x34 > +#define LOCAL_IOCTL_I915_GEM_USERPTR DRM_IOWR (DRM_COMMAND_BASE + > LOCAL_I915_GEM_USERPTR, struct local_i915_gem_userptr) > +struct local_i915_gem_userptr { > + uint64_t user_ptr; > + uint64_t user_size; > + uint32_t flags; > +#define LOCAL_I915_USERPTR_READ_ONLY (1<<0) > +#define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31) > + uint32_t handle; > +}; > + > +static uint32_t userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; > + > +#define BO_SIZE (65536) > + > +static void gem_userptr_test_unsynchronized(void) > +{ > + userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; > +} > + > +static void gem_userptr_test_synchronized(void) > +{ > + userptr_flags = 0; > +} > + > +static int gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t > *handle) > +{ > + struct local_i915_gem_userptr userptr; > + int ret; > + > + userptr.user_ptr
Re: [Intel-gfx] [PATCH 2/3] tests/gem_vmap_blits: Remove obsolete test case
Reviewed-by: Brad Volkin On Wed, Apr 23, 2014 at 05:38:34PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > No need for the old test case once the new one was added. > > v2: >* Just rebase for lib/ reorganization. > > Signed-off-by: Tvrtko Ursulin > --- > tests/.gitignore | 1 - > tests/Makefile.sources | 1 - > tests/gem_vmap_blits.c | 345 > - > 3 files changed, 347 deletions(-) > delete mode 100644 tests/gem_vmap_blits.c > > diff --git a/tests/.gitignore b/tests/.gitignore > index ff193bd..d192bb9 100644 > --- a/tests/.gitignore > +++ b/tests/.gitignore > @@ -94,7 +94,6 @@ gem_tiled_swapping > gem_tiling_max_stride > gem_unfence_active_buffers > gem_unref_active_buffers > -gem_vmap_blits > gem_userptr_blits > gem_wait_render_timeout > gem_write_read_ring_switch > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > index 09d6aa3..57a0da2 100644 > --- a/tests/Makefile.sources > +++ b/tests/Makefile.sources > @@ -120,7 +120,6 @@ TESTS_progs = \ > gem_tiling_max_stride \ > gem_unfence_active_buffers \ > gem_unref_active_buffers \ > - gem_vmap_blits \ > gem_userptr_blits \ > gem_wait_render_timeout \ > gen3_mixed_blits \ > diff --git a/tests/gem_vmap_blits.c b/tests/gem_vmap_blits.c > deleted file mode 100644 > index 430338b..000 > --- a/tests/gem_vmap_blits.c > +++ /dev/null > @@ -1,345 +0,0 @@ > -/* > - * Copyright © 2009,2011 Intel Corporation > - * > - * Permission is hereby granted, free of charge, to any person obtaining a > - * copy of this software and associated documentation files (the "Software"), > - * to deal in the Software without restriction, including without limitation > - * the rights to use, copy, modify, merge, publish, distribute, sublicense, > - * and/or sell copies of the Software, and to permit persons to whom the > - * Software is furnished to do so, subject to the following conditions: > - * > - * The above copyright notice and this permission notice (including the next > - * paragraph) shall be included in all copies or substantial portions of the > - * Software. > - * > - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS > - * IN THE SOFTWARE. > - * > - * Authors: > - *Eric Anholt > - *Chris Wilson > - * > - */ > - > -/** @file gem_vmap_blits.c > - * > - * This is a test of doing many blits using a mixture of normal system pages > - * and uncached linear buffers, with a working set larger than the > - * aperture size. > - * > - * The goal is to simply ensure the basics work. > - */ > - > -#include > -#include > -#include > -#include > -#include > -#include > -#include > -#include > - > -#include "drm.h" > -#include "ioctl_wrappers.h" > -#include "drmtest.h" > -#include "intel_bufmgr.h" > -#include "intel_batchbuffer.h" > -#include "intel_io.h" > - > -#if !defined(I915_PARAM_HAS_VMAP) > -#pragma message("No vmap support in drm, skipping") > -int main(int argc, char **argv) > -{ > - fprintf(stderr, "No vmap support in drm.\n"); > - return 77; > -} > -#else > - > -#define WIDTH 512 > -#define HEIGHT 512 > - > -static uint32_t linear[WIDTH*HEIGHT]; > - > -static uint32_t gem_vmap(int fd, void *ptr, int size, int read_only) > -{ > - struct drm_i915_gem_vmap vmap; > - > - vmap.user_ptr = (uintptr_t)ptr; > - vmap.user_size = size; > - vmap.flags = 0; > - if (read_only) > - vmap.flags |= I915_VMAP_READ_ONLY; > - > - if (drmIoctl(fd, DRM_IOCTL_I915_GEM_VMAP, &vmap)) > - return 0; > - > - return vmap.handle; > -} > - > - > -static void gem_vmap_sync(int fd, uint32_t handle) > -{ > - gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); > -} > - > -static void > -copy(int fd, uint32_t dst, uint32_t src) > -{ > - uint32_t batch[10]; > - struct drm_i915_gem_relocation_entry reloc[2]; > - struct drm_i915_gem_exec_object2 obj[3]; > - struct drm_i915_gem_execbuffer2 exec; > - uint32_t handle; > - int ret; > - > - batch[0] = XY_SRC_COPY_BLT_CMD | > - XY_SRC_COPY_BLT_WRITE_ALPHA | > - XY_SRC_COPY_BLT_WRITE_RGB | 6; > - batch[1] = (3 << 24) | /* 32 bits */ > - (0xcc << 16) | /* copy ROP */ > - WIDTH*4; > - batch[2] = 0; /* dst x1,y1 */ > - batch[3] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */ > - batch[4] = 0; /* dst reloc */ > - batch[5] = 0; /* src x1,y1 */ > - batch[6] = WIDTH*4; > - batch[7] = 0; /* src reloc */ > - batch[8] =
Re: [Intel-gfx] [PATCH] benchmarks: Build them on Android.
Reviewed-by: Brad Volkin On Wed, Apr 23, 2014 at 09:03:23AM -0700, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > They build fine so give them some exposure. > > Signed-off-by: Tvrtko Ursulin > --- > Android.mk | 2 +- > benchmarks/Android.mk | 36 > benchmarks/Makefile.am | 6 +- > benchmarks/Makefile.sources | 5 + > 4 files changed, 43 insertions(+), 6 deletions(-) > create mode 100644 benchmarks/Android.mk > create mode 100644 benchmarks/Makefile.sources > > diff --git a/Android.mk b/Android.mk > index 8aeb2d4..681d114 100644 > --- a/Android.mk > +++ b/Android.mk > @@ -1,2 +1,2 @@ > -include $(call all-named-subdir-makefiles, lib tests tools) > +include $(call all-named-subdir-makefiles, lib tests tools benchmarks) > > diff --git a/benchmarks/Android.mk b/benchmarks/Android.mk > new file mode 100644 > index 000..5bb8ef5 > --- /dev/null > +++ b/benchmarks/Android.mk > @@ -0,0 +1,36 @@ > +LOCAL_PATH := $(call my-dir) > + > +include $(LOCAL_PATH)/Makefile.sources > + > +## > + > +define add_benchmark > +include $(CLEAR_VARS) > + > +LOCAL_SRC_FILES := $1.c > + > +LOCAL_CFLAGS += -DHAVE_STRUCT_SYSINFO_TOTALRAM > +LOCAL_CFLAGS += -DANDROID -UNDEBUG -include "check-ndebug.h" > +LOCAL_CFLAGS += -std=c99 > +# FIXME: drop once Bionic correctly annotates "noreturn" on pthread_exit > +LOCAL_CFLAGS += -Wno-error=return-type > +# Excessive complaining for established cases. Rely on the Linux version > warnings. > +LOCAL_CFLAGS += -Wno-sign-compare > + > +LOCAL_MODULE := $1 > +LOCAL_MODULE_TAGS := optional > + > +LOCAL_STATIC_LIBRARIES := libintel_gpu_tools > + > +LOCAL_SHARED_LIBRARIES := libpciaccess \ > + libdrm\ > + libdrm_intel > + > +include $(BUILD_EXECUTABLE) > +endef > + > +## > + > +benchmark_list := $(bin_PROGRAMS) > + > +$(foreach item,$(benchmark_list),$(eval $(call add_benchmark,$(item > diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am > index e2ad784..86f755a 100644 > --- a/benchmarks/Makefile.am > +++ b/benchmarks/Makefile.am > @@ -1,9 +1,5 @@ > > -bin_PROGRAMS = \ > - intel_upload_blit_large \ > - intel_upload_blit_large_gtt \ > - intel_upload_blit_large_map \ > - intel_upload_blit_small > +include Makefile.sources > > AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib > AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) > diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources > new file mode 100644 > index 000..f9da579 > --- /dev/null > +++ b/benchmarks/Makefile.sources > @@ -0,0 +1,5 @@ > +bin_PROGRAMS = \ > + intel_upload_blit_large \ > + intel_upload_blit_large_gtt \ > + intel_upload_blit_large_map \ > + intel_upload_blit_small > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/i915: Intel-specific primary plane handling (v5)
Intel hardware allows the primary plane to be disabled independently of the CRTC. Provide custom primary plane handling to allow this. v5: - Use new drm_primary_helper_check_update() helper function to check setplane parameter validity. - Swap primary plane's pipe for pre-gen4 FBC (caught by Ville Syrjälä) - Cleanup primary plane properly on crtc init failure v4: - Don't add a primary_plane field to intel_crtc; that was left over from a much earlier iteration of this patch series, but is no longer needed/used now that the DRM core primary plane support has been merged. v3: - Provide gen-specific primary plane format lists (suggested by Daniel Vetter). - If the primary plane is already enabled, go ahead and just call the primary plane helper to do the update (suggested by Daniel Vetter). - Don't try to disable the primary plane on destruction; the DRM layer should have already taken care of this for us. v2: - Unpin fb properly on primary plane disable - Provide an Intel-specific set of primary plane formats - Additional sanity checks on setplane (in line with the checks currently being done by the DRM core primary plane helper) Signed-off-by: Matt Roper --- drivers/gpu/drm/i915/intel_display.c | 161 ++- 1 file changed, 159 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b57210c..ca83970 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -39,8 +39,35 @@ #include "i915_trace.h" #include #include +#include +#include #include +/* Primary plane formats supported by all gen */ +#define COMMON_PRIMARY_FORMATS \ + DRM_FORMAT_C8, \ + DRM_FORMAT_RGB565, \ + DRM_FORMAT_XRGB, \ + DRM_FORMAT_ARGB + +/* Primary plane formats for gen <= 3 */ +static const uint32_t intel_primary_formats_gen3[] = { + COMMON_PRIMARY_FORMATS, + DRM_FORMAT_XRGB1555, + DRM_FORMAT_ARGB1555, +}; + +/* Primary plane formats for gen >= 4 */ +static const uint32_t intel_primary_formats_gen4[] = { + COMMON_PRIMARY_FORMATS, \ + DRM_FORMAT_XBGR, + DRM_FORMAT_ABGR, + DRM_FORMAT_XRGB2101010, + DRM_FORMAT_ARGB2101010, + DRM_FORMAT_XBGR2101010, + DRM_FORMAT_ABGR2101010, +}; + static void intel_increase_pllclock(struct drm_crtc *crtc); static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on); @@ -10543,17 +10570,147 @@ static void intel_shared_dpll_init(struct drm_device *dev) BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); } +static int +intel_primary_plane_disable(struct drm_plane *plane) +{ + struct drm_device *dev = plane->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_plane *intel_plane = to_intel_plane(plane); + struct intel_crtc *intel_crtc; + + if (WARN_ON(!plane->crtc)) + return -EINVAL; + + intel_crtc = to_intel_crtc(plane->crtc); + if (!intel_crtc->primary_enabled) + return 0; + + intel_disable_primary_hw_plane(dev_priv, intel_plane->plane, + intel_plane->pipe); + + /* +* N.B. The DRM setplane code will update the plane->fb pointer after +* we finish here. +*/ + intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj); + + return 0; +} + +static int +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc, +struct drm_framebuffer *fb, int crtc_x, int crtc_y, +unsigned int crtc_w, unsigned int crtc_h, +uint32_t src_x, uint32_t src_y, +uint32_t src_w, uint32_t src_h) +{ + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_rect dest = { + .x1 = crtc_x, + .y1 = crtc_y, + .x2 = crtc_x + crtc_w, + .y2 = crtc_y + crtc_h, + }; + struct drm_rect src = { + .x1 = src_x >> 16, + .y1 = src_y >> 16, + .x2 = (src_x + src_w) >> 16, + .y2 = (src_y + src_h) >> 16, + }; + const struct drm_rect clip = { + .x2 = intel_crtc->config.pipe_src_w, + .y2 = intel_crtc->config.pipe_src_h, + }; + int visible; + int ret; + + ret = drm_primary_helper_check_update(plane, crtc, fb, + crtc_x, crtc_y, crtc_w, crtc_h, + src_x, src_y, src_w, src_h, + false, false); + if (ret) + return ret; + + visible = drm_rect_clip_scaled(&src, &dest, &clip, 1, 1); + if (!visible)
[Intel-gfx] [PATCH 2/3] drm/plane-helper: Add drm_primary_helper_check_update()
Pull the parameter checking from drm_primary_helper_update() out into its own function; drivers that provide their own setplane() implementations rather than using the helper may still want to share this parameter checking logic. A few of the checks here were also updated based on suggestions by Ville Syrjälä. Cc: Ville Syrjälä Cc: dri-de...@lists.freedesktop.org Signed-off-by: Matt Roper --- drivers/gpu/drm/drm_plane_helper.c | 148 + include/drm/drm_plane_helper.h | 9 +++ 2 files changed, 128 insertions(+), 29 deletions(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 65c4a00..9bbbdf2 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -66,6 +66,102 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, } /** + * drm_primary_helper_check_update() - Check primary plane update for validity + * @plane: plane object to update + * @crtc: owning CRTC of owning plane + * @fb: framebuffer to flip onto plane + * @crtc_x: x offset of primary plane on crtc + * @crtc_y: y offset of primary plane on crtc + * @crtc_w: width of primary plane rectangle on crtc + * @crtc_h: height of primary plane rectangle on crtc + * @src_x: x offset of @fb for panning + * @src_y: y offset of @fb for panning + * @src_w: width of source rectangle in @fb + * @src_h: height of source rectangle in @fb + * @can_scale: is primary plane scaling legal? + * @can_position: is it legal to position the primary plane such that it + *doesn't cover the entire crtc? + * + * Checks that a desired primary plane update is valid. Drivers that provide + * their own primary plane handling may still wish to call this function to + * avoid duplication of error checking code. + * + * RETURNS: + * Zero if update appears valid, error code on failure + */ +int drm_primary_helper_check_update(struct drm_plane *plane, + struct drm_crtc *crtc, + struct drm_framebuffer *fb, + int crtc_x, int crtc_y, + unsigned int crtc_w, unsigned int crtc_h, + uint32_t src_x, uint32_t src_y, + uint32_t src_w, uint32_t src_h, + bool can_scale, + bool can_position) +{ + struct drm_rect dest = { + .x1 = crtc_x, + .y1 = crtc_y, + .x2 = crtc_x + crtc_w, + .y2 = crtc_y + crtc_h, + }; + struct drm_rect src = { + .x1 = src_x >> 16, + .y1 = src_y >> 16, + .x2 = (src_x + src_w) >> 16, + .y2 = (src_y + src_h) >> 16, + }; + const struct drm_rect clip = { + .x2 = crtc->mode.hdisplay, + .y2 = crtc->mode.vdisplay, + }; + int hscale, vscale; + int visible; + + if (!crtc->enabled) { + DRM_DEBUG_KMS("Cannot update primary plane of a disabled CRTC.\n"); + return -EINVAL; + } + + /* setplane API takes shifted source rectangle values; unshift them */ + src_x >>= 16; + src_y >>= 16; + src_w >>= 16; + src_h >>= 16; + + /* check scaling */ + if (!can_scale && (crtc_w != src_w || crtc_h != src_h)) { + DRM_DEBUG_KMS("Can't scale primary plane\n"); + return -EINVAL; + } + + /* +* Drivers that can scale should perform their own min/max scale +* factor checks. +*/ + hscale = drm_rect_calc_hscale(&src, &dest, 0, INT_MAX); + vscale = drm_rect_calc_vscale(&src, &dest, 0, INT_MAX); + visible = drm_rect_clip_scaled(&src, &dest, &clip, hscale, vscale); + if (!visible) + /* +* Primary plane isn't visible; some drivers can handle this +* so we just return success here. Drivers that can't +* (including those that use the primary plane helper's +* update function) will return an error from their +* update_plane handler. +*/ + return 0; + + if (!can_position && !drm_rect_equals(&dest, &clip)) { + DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n"); + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL(drm_primary_helper_check_update); + +/** * drm_primary_helper_update() - Helper for primary plane update * @plane: plane object to update * @crtc: owning CRTC of owning plane @@ -113,6 +209,12 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, .x = src_x >> 16, .y = src_y >> 16, }; + struct drm_rect src = { + .x1 = src_x >> 16, + .y1 = src_y >> 16, + .x
[Intel-gfx] [PATCH 1/3] drm: Check CRTC compatibility in setplane
The DRM core setplane code should check that the plane is usable on the specified CRTC before calling into the driver. Prior to this patch, a plane's possible_crtcs field was purely informational for userspace and was never actually verified at the kernel level (aside from the primary plane helper). Cc: dri-de...@lists.freedesktop.org Signed-off-by: Matt Roper --- drivers/gpu/drm/drm_crtc.c | 7 +++ drivers/gpu/drm/drm_plane_helper.c | 6 -- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 461d19b..b6d6c04 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2140,6 +2140,13 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } crtc = obj_to_crtc(obj); + /* Check whether this plane is usable on this CRTC */ + if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) { + DRM_DEBUG_KMS("Invalid crtc for plane\n"); + ret = -EINVAL; + goto out; + } + fb = drm_framebuffer_lookup(dev, plane_req->fb_id); if (!fb) { DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index b72736d..65c4a00 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -137,12 +137,6 @@ int drm_primary_helper_update(struct drm_plane *plane, struct drm_crtc *crtc, return -EINVAL; } - /* Primary planes are locked to their owning CRTC */ - if (plane->possible_crtcs != drm_crtc_mask(crtc)) { - DRM_DEBUG_KMS("Cannot change primary plane CRTC\n"); - return -EINVAL; - } - /* Disallow scaling */ src_w >>= 16; src_h >>= 16; -- 1.8.5.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests: Extract ALIGN macro into a common header
Reviewed-by: Brad Volkin On Wed, Apr 23, 2014 at 08:07:55AM -0700, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > Makes for a little bit less code duplication, especially since > it will be used from more callers in the future. > > Signed-off-by: Tvrtko Ursulin > --- > lib/drmtest.h | 9 + > lib/media_fill_gen7.c | 2 +- > lib/media_fill_gen8.c | 2 +- > lib/rendercopy_gen6.c | 1 - > lib/rendercopy_gen7.c | 1 - > lib/rendercopy_gen8.c | 1 - > 6 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/lib/drmtest.h b/lib/drmtest.h > index f3afbaa..84f80dc 100644 > --- a/lib/drmtest.h > +++ b/lib/drmtest.h > @@ -60,6 +60,15 @@ static inline void *igt_mmap64(void *addr, size_t length, > int prot, int flags, > */ > #define ARRAY_SIZE(arr) (sizeof(arr)/sizeof(arr[0])) > > +/** > + * ALIGN: > + * @v: value to be aligned > + * @a: alignment unit in bytes > + * > + * Macro to align a value @v to a specified unit @a. > + */ > +#define ALIGN(v, a) (((v) + (a)-1) & ~((a)-1)) > + > int drm_get_card(void); > int drm_open_any(void); > int drm_open_any_render(void); > diff --git a/lib/media_fill_gen7.c b/lib/media_fill_gen7.c > index cdf4b60..82c3469 100644 > --- a/lib/media_fill_gen7.c > +++ b/lib/media_fill_gen7.c > @@ -4,10 +4,10 @@ > #include "media_fill.h" > #include "gen7_media.h" > #include "intel_reg.h" > +#include "drmtest.h" > > #include > > -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) > > static const uint32_t media_kernel[][4] = { > { 0x0041, 0x20200231, 0x0020, 0x }, > diff --git a/lib/media_fill_gen8.c b/lib/media_fill_gen8.c > index 996d4d0..54309d5 100644 > --- a/lib/media_fill_gen8.c > +++ b/lib/media_fill_gen8.c > @@ -4,10 +4,10 @@ > #include "media_fill.h" > #include "gen8_media.h" > #include "intel_reg.h" > +#include "drmtest.h" > > #include > > -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) > > static const uint32_t media_kernel[][4] = { > { 0x0041, 0x20202288, 0x0020, 0x }, > diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c > index d806cef..7b3104c 100644 > --- a/lib/rendercopy_gen6.c > +++ b/lib/rendercopy_gen6.c > @@ -20,7 +20,6 @@ > #include "gen6_render.h" > #include "intel_reg.h" > > -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) > #define VERTEX_SIZE (3*4) > > static const uint32_t ps_kernel_nomask_affine[][4] = { > diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c > index cdbc70c..5131d8f 100644 > --- a/lib/rendercopy_gen7.c > +++ b/lib/rendercopy_gen7.c > @@ -21,7 +21,6 @@ > #include "gen7_render.h" > #include "intel_reg.h" > > -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) > > static const uint32_t ps_kernel[][4] = { > { 0x0080005a, 0x2e2077bd, 0x00c0, 0x008d0040 }, > diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c > index e846376..6f5a698 100644 > --- a/lib/rendercopy_gen8.c > +++ b/lib/rendercopy_gen8.c > @@ -25,7 +25,6 @@ > > #include > > -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) > #define VERTEX_SIZE (3*4) > > #if DEBUG_RENDERCPY > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC] xf86-video-intel: enable hw-generated binding tables
On Wed, Apr 23, 2014 at 1:21 PM, Abdiel Janulgue wrote: > I've already tried disabling RS at the end of every batch so that next batch > in different context can continue to use older non-RS format. That does not > work either and still causes hangs. > > What I've seen so far, it seems GPU does not like switching the format of > commands from RS-format to non-RS format. It's either one way or the other. If > switched on, it affects subsequent contexes henceforth expecting RS-format > commands until the GPU gets reset. That's probably the note in bspec: > > "the binding table generator feature has a simple all or nothing model". Oh hooray, that's just awesome :( So we indeed need to stop the gpu and reset it if there's a non-RS render batch after any RS render batch. Which also means that we need to enable this for _all_ userspace to avoid completely disastrous performance. So uxa, sna, libva, maybe opencl ... I guess before we engage in this endeavor we need to track this down with the hardware people. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] tests/gem_userptr_blits: Expanded userptr test cases
From: Tvrtko Ursulin A set of userptr test cases to support the new feature. For the eviction and swapping stress testing I have extracted some common behaviour from gem_evict_everything and made both test cases use it to avoid duplicating the code. Both unsynchronized and synchronized userptr objects are tested but the latter set of tests will be skipped if kernel is compiled without MMU_NOTIFIERS. Also, with 32-bit userspace swapping tests are skipped if the system has a lot more RAM than process address space. Forking swapping tests are not skipped since they can still trigger swapping by cumulative effect. v2: * Fixed dmabuf test. * Added test for rejecting read-only. * Fixed ioctl detection for latest kernel patch. v3: * Use ALIGN macro. * Catchup with big lib/ reorganization. * Fixed up some warnings. Signed-off-by: Tvrtko Ursulin --- tests/.gitignore |1 + tests/Makefile.sources|1 + tests/gem_userptr_blits.c | 1247 + 3 files changed, 1249 insertions(+) create mode 100644 tests/gem_userptr_blits.c diff --git a/tests/.gitignore b/tests/.gitignore index 146bab0..ff193bd 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -95,6 +95,7 @@ gem_tiling_max_stride gem_unfence_active_buffers gem_unref_active_buffers gem_vmap_blits +gem_userptr_blits gem_wait_render_timeout gem_write_read_ring_switch gen3_mixed_blits diff --git a/tests/Makefile.sources b/tests/Makefile.sources index c957ace..09d6aa3 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -121,6 +121,7 @@ TESTS_progs = \ gem_unfence_active_buffers \ gem_unref_active_buffers \ gem_vmap_blits \ + gem_userptr_blits \ gem_wait_render_timeout \ gen3_mixed_blits \ gen3_render_linear_blits \ diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c new file mode 100644 index 000..03af58e --- /dev/null +++ b/tests/gem_userptr_blits.c @@ -0,0 +1,1247 @@ +/* + * Copyright © 2009-2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Eric Anholt + *Chris Wilson + *Tvrtko Ursulin + * + */ + +/** @file gem_userptr_blits.c + * + * This is a test of doing many blits using a mixture of normal system pages + * and uncached linear buffers, with a working set larger than the + * aperture size. + * + * The goal is to simply ensure the basics work. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "drm.h" +#include "i915_drm.h" +#include "drmtest.h" +#include "intel_bufmgr.h" +#include "intel_batchbuffer.h" +#include "intel_chipset.h" +#include "ioctl_wrappers.h" + +#include "eviction_common.c" + +#ifndef PAGE_SIZE +#define PAGE_SIZE 4096 +#endif + +#define LOCAL_I915_GEM_USERPTR 0x34 +#define LOCAL_IOCTL_I915_GEM_USERPTR DRM_IOWR (DRM_COMMAND_BASE + LOCAL_I915_GEM_USERPTR, struct local_i915_gem_userptr) +struct local_i915_gem_userptr { + uint64_t user_ptr; + uint64_t user_size; + uint32_t flags; +#define LOCAL_I915_USERPTR_READ_ONLY (1<<0) +#define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31) + uint32_t handle; +}; + +static uint32_t userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; + +#define WIDTH 512 +#define HEIGHT 512 + +static uint32_t linear[WIDTH*HEIGHT]; + +static void gem_userptr_test_unsynchronized(void) +{ + userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; +} + +static void gem_userptr_test_synchronized(void) +{ + userptr_flags = 0; +} + +static int gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t *handle) +{ + struct local_i915_gem_userptr userptr; + int ret; + + userptr.user_ptr = (uintptr_t)ptr; + userptr.user_size = size; + userptr.flags = userptr_flags; + if (read_only) +
[Intel-gfx] [PATCH 2/3] tests/gem_vmap_blits: Remove obsolete test case
From: Tvrtko Ursulin No need for the old test case once the new one was added. v2: * Just rebase for lib/ reorganization. Signed-off-by: Tvrtko Ursulin --- tests/.gitignore | 1 - tests/Makefile.sources | 1 - tests/gem_vmap_blits.c | 345 - 3 files changed, 347 deletions(-) delete mode 100644 tests/gem_vmap_blits.c diff --git a/tests/.gitignore b/tests/.gitignore index ff193bd..d192bb9 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -94,7 +94,6 @@ gem_tiled_swapping gem_tiling_max_stride gem_unfence_active_buffers gem_unref_active_buffers -gem_vmap_blits gem_userptr_blits gem_wait_render_timeout gem_write_read_ring_switch diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 09d6aa3..57a0da2 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -120,7 +120,6 @@ TESTS_progs = \ gem_tiling_max_stride \ gem_unfence_active_buffers \ gem_unref_active_buffers \ - gem_vmap_blits \ gem_userptr_blits \ gem_wait_render_timeout \ gen3_mixed_blits \ diff --git a/tests/gem_vmap_blits.c b/tests/gem_vmap_blits.c deleted file mode 100644 index 430338b..000 --- a/tests/gem_vmap_blits.c +++ /dev/null @@ -1,345 +0,0 @@ -/* - * Copyright © 2009,2011 Intel Corporation - * - * Permission is hereby granted, free of charge, to any person obtaining a - * copy of this software and associated documentation files (the "Software"), - * to deal in the Software without restriction, including without limitation - * the rights to use, copy, modify, merge, publish, distribute, sublicense, - * and/or sell copies of the Software, and to permit persons to whom the - * Software is furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice (including the next - * paragraph) shall be included in all copies or substantial portions of the - * Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL - * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS - * IN THE SOFTWARE. - * - * Authors: - *Eric Anholt - *Chris Wilson - * - */ - -/** @file gem_vmap_blits.c - * - * This is a test of doing many blits using a mixture of normal system pages - * and uncached linear buffers, with a working set larger than the - * aperture size. - * - * The goal is to simply ensure the basics work. - */ - -#include -#include -#include -#include -#include -#include -#include -#include - -#include "drm.h" -#include "ioctl_wrappers.h" -#include "drmtest.h" -#include "intel_bufmgr.h" -#include "intel_batchbuffer.h" -#include "intel_io.h" - -#if !defined(I915_PARAM_HAS_VMAP) -#pragma message("No vmap support in drm, skipping") -int main(int argc, char **argv) -{ - fprintf(stderr, "No vmap support in drm.\n"); - return 77; -} -#else - -#define WIDTH 512 -#define HEIGHT 512 - -static uint32_t linear[WIDTH*HEIGHT]; - -static uint32_t gem_vmap(int fd, void *ptr, int size, int read_only) -{ - struct drm_i915_gem_vmap vmap; - - vmap.user_ptr = (uintptr_t)ptr; - vmap.user_size = size; - vmap.flags = 0; - if (read_only) - vmap.flags |= I915_VMAP_READ_ONLY; - - if (drmIoctl(fd, DRM_IOCTL_I915_GEM_VMAP, &vmap)) - return 0; - - return vmap.handle; -} - - -static void gem_vmap_sync(int fd, uint32_t handle) -{ - gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); -} - -static void -copy(int fd, uint32_t dst, uint32_t src) -{ - uint32_t batch[10]; - struct drm_i915_gem_relocation_entry reloc[2]; - struct drm_i915_gem_exec_object2 obj[3]; - struct drm_i915_gem_execbuffer2 exec; - uint32_t handle; - int ret; - - batch[0] = XY_SRC_COPY_BLT_CMD | - XY_SRC_COPY_BLT_WRITE_ALPHA | - XY_SRC_COPY_BLT_WRITE_RGB | 6; - batch[1] = (3 << 24) | /* 32 bits */ - (0xcc << 16) | /* copy ROP */ - WIDTH*4; - batch[2] = 0; /* dst x1,y1 */ - batch[3] = (HEIGHT << 16) | WIDTH; /* dst x2,y2 */ - batch[4] = 0; /* dst reloc */ - batch[5] = 0; /* src x1,y1 */ - batch[6] = WIDTH*4; - batch[7] = 0; /* src reloc */ - batch[8] = MI_BATCH_BUFFER_END; - batch[9] = MI_NOOP; - - handle = gem_create(fd, 4096); - gem_write(fd, handle, 0, batch, sizeof(batch)); - - reloc[0].target_handle = dst; - reloc[0].delta = 0; - reloc[0].offset = 4 * sizeof(batch[0]); - reloc[0].presumed_offset = 0; - reloc[0].read_domains = I9
[Intel-gfx] [PATCH 3/3] tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact
From: Tvrtko Ursulin This adds a small benchmark for the new userptr functionality. Apart from basic surface creation and destruction, also tested is the impact of having userptr surfaces in the process address space. Reason for that is the impact of MMU notifiers on common address space operations like munmap() which is per process. v2: * Moved to benchmarks. * Added pointer read/write tests. * Changed output to say iterations per second instead of operations per second. * Multiply result by batch size for multi-create* tests for a more comparable number with create-destroy test. v3: * Use ALIGN macro. * Catchup with big lib/ reorganization. * Removed unused code and one global variable. * Fixed up some warnings. Signed-off-by: Tvrtko Ursulin Cc: Chris Wilson --- benchmarks/.gitignore | 1 + benchmarks/Makefile.sources| 3 +- benchmarks/gem_userptr_benchmark.c | 497 + 3 files changed, 500 insertions(+), 1 deletion(-) create mode 100644 benchmarks/gem_userptr_benchmark.c diff --git a/benchmarks/.gitignore b/benchmarks/.gitignore index ddea6f7..09e5bd8 100644 --- a/benchmarks/.gitignore +++ b/benchmarks/.gitignore @@ -1,3 +1,4 @@ +gem_userptr_benchmark intel_upload_blit_large intel_upload_blit_large_gtt intel_upload_blit_large_map diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources index f9da579..60bdae2 100644 --- a/benchmarks/Makefile.sources +++ b/benchmarks/Makefile.sources @@ -2,4 +2,5 @@ bin_PROGRAMS = \ intel_upload_blit_large \ intel_upload_blit_large_gtt \ intel_upload_blit_large_map \ - intel_upload_blit_small + intel_upload_blit_small \ + gem_userptr_benchmark diff --git a/benchmarks/gem_userptr_benchmark.c b/benchmarks/gem_userptr_benchmark.c new file mode 100644 index 000..a51201c --- /dev/null +++ b/benchmarks/gem_userptr_benchmark.c @@ -0,0 +1,497 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Tvrtko Ursulin + * + */ + +/** @file gem_userptr_benchmark.c + * + * Benchmark the userptr code and impact of having userptr surfaces + * in process address space on some normal operations. + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "drm.h" +#include "i915_drm.h" +#include "drmtest.h" +#include "intel_bufmgr.h" +#include "intel_batchbuffer.h" +#include "intel_chipset.h" +#include "ioctl_wrappers.h" +#include "igt_aux.h" + +#ifndef PAGE_SIZE + #define PAGE_SIZE 4096 +#endif + +#define LOCAL_I915_GEM_USERPTR 0x34 +#define LOCAL_IOCTL_I915_GEM_USERPTR DRM_IOWR (DRM_COMMAND_BASE + LOCAL_I915_GEM_USERPTR, struct local_i915_gem_userptr) +struct local_i915_gem_userptr { + uint64_t user_ptr; + uint64_t user_size; + uint32_t flags; +#define LOCAL_I915_USERPTR_READ_ONLY (1<<0) +#define LOCAL_I915_USERPTR_UNSYNCHRONIZED (1<<31) + uint32_t handle; +}; + +static uint32_t userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; + +#define BO_SIZE (65536) + +static void gem_userptr_test_unsynchronized(void) +{ + userptr_flags = LOCAL_I915_USERPTR_UNSYNCHRONIZED; +} + +static void gem_userptr_test_synchronized(void) +{ + userptr_flags = 0; +} + +static int gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t *handle) +{ + struct local_i915_gem_userptr userptr; + int ret; + + userptr.user_ptr = (uintptr_t)ptr; + userptr.user_size = size; + userptr.flags = userptr_flags; + if (read_only) + userptr.flags |= LOCAL_I915_USERPTR_READ_ONLY; + + ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_USERPTR, &userptr); + if (ret) + ret = errno; + igt_skip_on_f(ret == ENODEV && +
[Intel-gfx] [PATCH] benchmarks: Build them on Android.
From: Tvrtko Ursulin They build fine so give them some exposure. Signed-off-by: Tvrtko Ursulin --- Android.mk | 2 +- benchmarks/Android.mk | 36 benchmarks/Makefile.am | 6 +- benchmarks/Makefile.sources | 5 + 4 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 benchmarks/Android.mk create mode 100644 benchmarks/Makefile.sources diff --git a/Android.mk b/Android.mk index 8aeb2d4..681d114 100644 --- a/Android.mk +++ b/Android.mk @@ -1,2 +1,2 @@ -include $(call all-named-subdir-makefiles, lib tests tools) +include $(call all-named-subdir-makefiles, lib tests tools benchmarks) diff --git a/benchmarks/Android.mk b/benchmarks/Android.mk new file mode 100644 index 000..5bb8ef5 --- /dev/null +++ b/benchmarks/Android.mk @@ -0,0 +1,36 @@ +LOCAL_PATH := $(call my-dir) + +include $(LOCAL_PATH)/Makefile.sources + +## + +define add_benchmark +include $(CLEAR_VARS) + +LOCAL_SRC_FILES := $1.c + +LOCAL_CFLAGS += -DHAVE_STRUCT_SYSINFO_TOTALRAM +LOCAL_CFLAGS += -DANDROID -UNDEBUG -include "check-ndebug.h" +LOCAL_CFLAGS += -std=c99 +# FIXME: drop once Bionic correctly annotates "noreturn" on pthread_exit +LOCAL_CFLAGS += -Wno-error=return-type +# Excessive complaining for established cases. Rely on the Linux version warnings. +LOCAL_CFLAGS += -Wno-sign-compare + +LOCAL_MODULE := $1 +LOCAL_MODULE_TAGS := optional + +LOCAL_STATIC_LIBRARIES := libintel_gpu_tools + +LOCAL_SHARED_LIBRARIES := libpciaccess \ + libdrm\ + libdrm_intel + +include $(BUILD_EXECUTABLE) +endef + +## + +benchmark_list := $(bin_PROGRAMS) + +$(foreach item,$(benchmark_list),$(eval $(call add_benchmark,$(item diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am index e2ad784..86f755a 100644 --- a/benchmarks/Makefile.am +++ b/benchmarks/Makefile.am @@ -1,9 +1,5 @@ -bin_PROGRAMS = \ - intel_upload_blit_large \ - intel_upload_blit_large_gtt \ - intel_upload_blit_large_map \ - intel_upload_blit_small +include Makefile.sources AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources new file mode 100644 index 000..f9da579 --- /dev/null +++ b/benchmarks/Makefile.sources @@ -0,0 +1,5 @@ +bin_PROGRAMS = \ + intel_upload_blit_large \ + intel_upload_blit_large_gtt \ + intel_upload_blit_large_map \ + intel_upload_blit_small -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] tests/gem_userptr_blits: Expanded userptr test cases
On Wed, Apr 23, 2014 at 06:33:40AM -0700, Tvrtko Ursulin wrote: > > On 04/18/2014 06:10 PM, Volkin, Bradley D wrote: > > On Wed, Mar 19, 2014 at 04:13:04AM -0700, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin > >> > >> A set of userptr test cases to support the new feature. > >> > >> For the eviction and swapping stress testing I have extracted > >> some common behaviour from gem_evict_everything and made both > >> test cases use it to avoid duplicating the code. > >> > >> Both unsynchronized and synchronized userptr objects are > >> tested but the latter set of tests will be skipped if kernel > >> is compiled without MMU_NOTIFIERS. > >> > >> Also, with 32-bit userspace swapping tests are skipped if > >> the system has a lot more RAM than process address space. > >> Forking swapping tests are not skipped since they can still > >> trigger swapping by cumulative effect. > >> > >> v2: > >> * Fixed dmabuf test. > >> * Added test for rejecting read-only. > >> * Fixed ioctl detection for latest kernel patch. > >> > >> v3: > >> * Updated copy() for Gen8+. > >> * Fixed ioctl detection on kernels without MMU_NOTIFIERs. > >> > >> Signed-off-by: Tvrtko Ursulin > > > > A number of the comments I made on patch 3 apply here as well. > > The sizeof(linear) thing is more prevalent in this test, though > > it looks like linear is at least used. Other than those comments > > this looks good to me. > > Believe it or not that sizeof(linear) "idiom" I inherited from other > blitter tests. Personally I don't care one way or another. But since it > makes sense to get rid of it for the benchmark part, perhaps I should > change it here as well to be consistent. How strongly do you feel > strongly about this? I think changing it would be slightly more readable, but if it's consistent with other blit tests then I don't feel too strongly about it. In fact, consistency with the other tests might be the better approach. I'm fine with whichever approach you prefer. Thanks, Brad > > Will see what you reply on the static initializer comment it 3/3, not > sure what you meant there. > > Thanks, > > Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact
[snip] On Wed, Apr 23, 2014 at 06:28:54AM -0700, Tvrtko Ursulin wrote: > On 04/18/2014 12:18 AM, Volkin, Bradley D wrote: > > On Wed, Mar 19, 2014 at 04:13:06AM -0700, Tvrtko Ursulin wrote: > >> +static void **handle_ptr_map; > >> +static unsigned int num_handle_ptr_map; > > > > I'd prefer that we explicitly initialize at least num_handle_ptr_map. > > To zero, why? Partly because I just like explicitly initialized variables and partly because I forgot that static variables are *guaranteed* to be initialized to zero vs just-happen-to-be-zero :) You can leave it as is or change it, I'm fine either way. Thanks, Brad > > >> + > >> +static void add_handle_ptr(uint32_t handle, void *ptr) > >> +{ > >> + if (handle >= num_handle_ptr_map) { > >> + handle_ptr_map = realloc(handle_ptr_map, > >> + (handle + 1000) * sizeof(void*)); > >> + num_handle_ptr_map = handle + 1000; > >> + } > >> + > >> + handle_ptr_map[handle] = ptr; > >> +} > >> + > >> +static void *get_handle_ptr(uint32_t handle) > >> +{ > >> + return handle_ptr_map[handle]; > >> +} > >> + > >> +static void free_handle_ptr(uint32_t handle) > >> +{ > >> + igt_assert(handle < num_handle_ptr_map); > >> + igt_assert(handle_ptr_map[handle]); > >> + > >> + free(handle_ptr_map[handle]); > >> + handle_ptr_map[handle] = NULL; > >> +} > >> + > >> +static uint32_t create_userptr_bo(int fd, int size) > >> +{ > >> + void *ptr; > >> + uint32_t handle; > >> + int ret; > >> + > >> + ret = posix_memalign(&ptr, PAGE_SIZE, size); > >> + igt_assert(ret == 0); > >> + > >> + ret = gem_userptr(fd, (uint32_t *)ptr, size, 0, &handle); > >> + igt_assert(ret == 0); > >> + add_handle_ptr(handle, ptr); > >> + > >> + return handle; > >> +} > >> + > >> +static void free_userptr_bo(int fd, uint32_t handle) > >> +{ > >> + gem_close(fd, handle); > >> + free_handle_ptr(handle); > >> +} > >> + > >> +static int has_userptr(int fd) > >> +{ > >> + uint32_t handle = 0; > >> + void *ptr; > >> + uint32_t oldflags; > >> + int ret; > >> + > >> + assert(posix_memalign(&ptr, PAGE_SIZE, PAGE_SIZE) == 0); > >> + oldflags = userptr_flags; > >> + gem_userptr_test_unsynchronized(); > >> + ret = gem_userptr(fd, ptr, PAGE_SIZE, 0, &handle); > >> + userptr_flags = oldflags; > >> + if (ret != 0) { > >> + free(ptr); > >> + return 0; > >> + } > >> + > >> + gem_close(fd, handle); > >> + free(ptr); > >> + > >> + return handle != 0; > >> +} > >> + > >> +static const unsigned int nr_bos[] = {0, 1, 10, 100, 1000}; > >> +static const unsigned int test_duration_sec = 3; > >> + > >> +static volatile unsigned int run_test; > >> + > >> +static void alarm_handler(int sig) > >> +{ > >> + assert(run_test == 1); > >> + run_test = 0; > >> +} > >> + > >> +static void start_test(unsigned int duration) > >> +{ > >> + run_test = 1; > >> + if (duration == 0) > >> + duration = test_duration_sec; > >> + signal(SIGALRM, alarm_handler); > >> + alarm(duration); > >> +} > >> + > >> +static void exchange_ptr(void *array, unsigned i, unsigned j) > >> +{ > >> + void **arr, *tmp; > >> + arr = (void **)array; > >> + > >> + tmp = arr[i]; > >> + arr[i] = arr[j]; > >> + arr[j] = tmp; > >> +} > >> + > >> +static void test_malloc_free(int random) > >> +{ > >> + unsigned long iter = 0; > >> + unsigned int i, tot = 1000; > >> + void *ptr[tot]; > >> + > >> + start_test(test_duration_sec); > >> + > >> + while (run_test) { > >> + for (i = 0; i < tot; i++) { > >> + ptr[i] = malloc(1000); > >> + assert(ptr[i]); > >> + } > >> + if (random) > >> + igt_permute_array(ptr, tot, exchange_ptr); > >> + for (i = 0; i < tot; i++) > >> + free(ptr[i]); > >> + iter++; > >> + } > >> + > >> + printf("%8lu iter/s\n", iter / test_duration_sec); > >> +} > >> + > >> +static void test_malloc_realloc_free(int random) > >> +{ > >> + unsigned long iter = 0; > >> + unsigned int i, tot = 1000; > >> + void *ptr[tot]; > >> + > >> + start_test(test_duration_sec); > >> + > >> + while (run_test) { > >> + for (i = 0; i < tot; i++) { > >> + ptr[i] = malloc(1000); > >> + assert(ptr[i]); > >> + } > >> + if (random) > >> + igt_permute_array(ptr, tot, exchange_ptr); > >> + for (i = 0; i < tot; i++) { > >> + ptr[i] = realloc(ptr[i], 2000); > >> + assert(ptr[i]); > >> + } > >> + if (random) > >> + igt_permute_array(ptr, tot, exchange_ptr); > >> + for (i = 0; i < tot; i++) > >> + free(ptr[i]); > >> + iter++; > >> + } > >> + > >> + printf("%8lu iter/s\n", iter / test_duration_sec); > >> +} > >> + > >> +static void test_mmap_unmap(int random) > >> +{ > >> + unsigned long iter = 0; > >> + unsigned int i, tot = 1000; > >> + void *ptr[tot]; > >> + > >> + s
[Intel-gfx] [PATCH] tests: Extract ALIGN macro into a common header
From: Tvrtko Ursulin Makes for a little bit less code duplication, especially since it will be used from more callers in the future. Signed-off-by: Tvrtko Ursulin --- lib/drmtest.h | 9 + lib/media_fill_gen7.c | 2 +- lib/media_fill_gen8.c | 2 +- lib/rendercopy_gen6.c | 1 - lib/rendercopy_gen7.c | 1 - lib/rendercopy_gen8.c | 1 - 6 files changed, 11 insertions(+), 5 deletions(-) diff --git a/lib/drmtest.h b/lib/drmtest.h index f3afbaa..84f80dc 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -60,6 +60,15 @@ static inline void *igt_mmap64(void *addr, size_t length, int prot, int flags, */ #define ARRAY_SIZE(arr) (sizeof(arr)/sizeof(arr[0])) +/** + * ALIGN: + * @v: value to be aligned + * @a: alignment unit in bytes + * + * Macro to align a value @v to a specified unit @a. + */ +#define ALIGN(v, a) (((v) + (a)-1) & ~((a)-1)) + int drm_get_card(void); int drm_open_any(void); int drm_open_any_render(void); diff --git a/lib/media_fill_gen7.c b/lib/media_fill_gen7.c index cdf4b60..82c3469 100644 --- a/lib/media_fill_gen7.c +++ b/lib/media_fill_gen7.c @@ -4,10 +4,10 @@ #include "media_fill.h" #include "gen7_media.h" #include "intel_reg.h" +#include "drmtest.h" #include -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) static const uint32_t media_kernel[][4] = { { 0x0041, 0x20200231, 0x0020, 0x }, diff --git a/lib/media_fill_gen8.c b/lib/media_fill_gen8.c index 996d4d0..54309d5 100644 --- a/lib/media_fill_gen8.c +++ b/lib/media_fill_gen8.c @@ -4,10 +4,10 @@ #include "media_fill.h" #include "gen8_media.h" #include "intel_reg.h" +#include "drmtest.h" #include -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) static const uint32_t media_kernel[][4] = { { 0x0041, 0x20202288, 0x0020, 0x }, diff --git a/lib/rendercopy_gen6.c b/lib/rendercopy_gen6.c index d806cef..7b3104c 100644 --- a/lib/rendercopy_gen6.c +++ b/lib/rendercopy_gen6.c @@ -20,7 +20,6 @@ #include "gen6_render.h" #include "intel_reg.h" -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) #define VERTEX_SIZE (3*4) static const uint32_t ps_kernel_nomask_affine[][4] = { diff --git a/lib/rendercopy_gen7.c b/lib/rendercopy_gen7.c index cdbc70c..5131d8f 100644 --- a/lib/rendercopy_gen7.c +++ b/lib/rendercopy_gen7.c @@ -21,7 +21,6 @@ #include "gen7_render.h" #include "intel_reg.h" -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) static const uint32_t ps_kernel[][4] = { { 0x0080005a, 0x2e2077bd, 0x00c0, 0x008d0040 }, diff --git a/lib/rendercopy_gen8.c b/lib/rendercopy_gen8.c index e846376..6f5a698 100644 --- a/lib/rendercopy_gen8.c +++ b/lib/rendercopy_gen8.c @@ -25,7 +25,6 @@ #include -#define ALIGN(x, y) (((x) + (y)-1) & ~((y)-1)) #define VERTEX_SIZE (3*4) #if DEBUG_RENDERCPY -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests: Skip building kms_flip_tiling on Android
From: Tvrtko Ursulin Dependencies are not available at the moment so it does not build. Signed-off-by: Tvrtko Ursulin --- tests/Android.mk | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Android.mk b/tests/Android.mk index 9233b2b..6e77e45 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -40,6 +40,7 @@ skip_tests_list := \ kms_addfb \ kms_cursor_crc \ kms_flip \ +kms_flip_tiling \ kms_pipe_crc_basic \ kms_fbc_crc \ kms_render \ -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm: Introduce drm_fb_helper_prepare()
On Wed, Apr 23, 2014 at 09:35:48AM +0200, Daniel Vetter wrote: > On Wed, Apr 23, 2014 at 09:14:41AM +0200, Thierry Reding wrote: > > On Tue, Apr 22, 2014 at 05:54:06PM +0200, Daniel Vetter wrote: > > > On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote: > > [...] > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > > > b/drivers/gpu/drm/drm_fb_helper.c > > [...] > > > > @@ -502,6 +503,33 @@ static void drm_fb_helper_crtc_free(struct > > > > drm_fb_helper *helper) > > > > } > > > > > > > > /** > > > > + * drm_fb_helper_prepare - setup a drm_fb_helper structure > > > > + * @dev: DRM device > > > > + * @helper: driver-allocated fbdev helper structure to set up > > > > + * @funcs: pointer to structure of functions associate with this helper > > > > + * > > > > + * Sets up the bare minimum to make the framebuffer helper usable. > > > > This is > > > > + * useful to implement race-free initialization of the polling > > > > helpers. In > > > > + * that case a typical sequence would be: > > > > + * > > > > + * - call drm_fb_helper_prepare() > > > > + * - set dev->mode_config.funcs > > > > > > This step is done in drm_fb_helper_prepare already. > > > > drm_fb_helper_prepare() sets fb_helper->funcs. dev->mode_config.funcs > > needs to be set because it gets called by drm_kms_helper_hotplug_event() > > which in turn is called by output_poll_execute(), which can be called at > > any point after drm_kms_helper_poll_init(). It could be scheduled > > immediately by drm_kms_helper_poll_enable(). > > > > I wonder if perhaps we should be wrapping that into a function as well. > > Currently this is only documented in code by the drivers that call this. > > But since it's only a single step it doesn't seem worth it. Perhaps if > > we rolled the min/max_width/height into that function as well it would > > be more worth it? That could be difficult to do since a couple of > > drivers need to set those depending on the chipset generation. > > Oh I've misread this step for the fb_helper->funcs assignment. Makes sense > and I don't think we need to augment your kerneldoc more to explain this. > > > > > + * - perform driver-specific setup > > Hm, maybe clarify this as "initialize modeset objects like crtcs, encoders > and connectors"? Since that's the important part we need to get done here. > > > > > + * - call drm_kms_helper_poll_init() > > > > > > Maybe mention that after this you can start to handle hpd events using the > > > probe helpers? > > > > Isn't that somewhat implied already? The poll helpers call directly the > > dev->mode_config.funcs->output_poll_changed() function, which has > > already been set up earlier. > > I've more meant that after this it's save for drivers to enable hpd > interrupts and start to process them. I.e. > > - enable interrupts and start processing hpd events > > as an additional step to make it really cleear how it all fits together. > Otherwise driver authors are left wondering wtf this isn't just one > function call to do it all for them ;-) > > > > > + * - call drm_fb_helper_init() > > > > + * - call drm_fb_helper_single_add_all_connectors() > > > > + * - call drm_fb_helper_initial_config() > > > > > > Does this list look sane in the generated DocBook? Afaik kerneldoc > > > unfortunately lacks any form of markup, at least afaik :( > > > > I must admit I didn't check. I'll make sure I do that before sending out > > the next version. > > If it looks ugly I'm ok as-is, just wondered. Kerneldoc is a bit > simplistic ime. In the second version I just sent out I ended up moving the description of this sequence into the fbdev helper section rather than keeping it in the description of the drm_fb_helper_prepare() function, since the new function is really just a part of the whole sequence, so it seemed to fit more nicely. And I've dropped the list formatting and turned it into prose instead. Thierry pgp194LSyswFd.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] tests/gem_userptr_blits: Expanded userptr test cases
On 04/18/2014 06:10 PM, Volkin, Bradley D wrote: On Wed, Mar 19, 2014 at 04:13:04AM -0700, Tvrtko Ursulin wrote: From: Tvrtko Ursulin A set of userptr test cases to support the new feature. For the eviction and swapping stress testing I have extracted some common behaviour from gem_evict_everything and made both test cases use it to avoid duplicating the code. Both unsynchronized and synchronized userptr objects are tested but the latter set of tests will be skipped if kernel is compiled without MMU_NOTIFIERS. Also, with 32-bit userspace swapping tests are skipped if the system has a lot more RAM than process address space. Forking swapping tests are not skipped since they can still trigger swapping by cumulative effect. v2: * Fixed dmabuf test. * Added test for rejecting read-only. * Fixed ioctl detection for latest kernel patch. v3: * Updated copy() for Gen8+. * Fixed ioctl detection on kernels without MMU_NOTIFIERs. Signed-off-by: Tvrtko Ursulin A number of the comments I made on patch 3 apply here as well. The sizeof(linear) thing is more prevalent in this test, though it looks like linear is at least used. Other than those comments this looks good to me. Believe it or not that sizeof(linear) "idiom" I inherited from other blitter tests. Personally I don't care one way or another. But since it makes sense to get rid of it for the benchmark part, perhaps I should change it here as well to be consistent. How strongly do you feel strongly about this? Will see what you reply on the static initializer comment it 3/3, not sure what you meant there. Thanks, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact
Hi Brad, On 04/18/2014 12:18 AM, Volkin, Bradley D wrote: On Wed, Mar 19, 2014 at 04:13:06AM -0700, Tvrtko Ursulin wrote: From: Tvrtko Ursulin This adds a small benchmark for the new userptr functionality. Apart from basic surface creation and destruction, also tested is the impact of having userptr surfaces in the process address space. Reason for that is the impact of MMU notifiers on common address space operations like munmap() which is per process. v2: * Moved to benchmarks. * Added pointer read/write tests. * Changed output to say iterations per second instead of operations per second. * Multiply result by batch size for multi-create* tests for a more comparable number with create-destroy test. v3: * Fixed ioctl detection on kernels without MMU_NOTIFIERs. Signed-off-by: Tvrtko Ursulin Cc: Chris Wilson --- Android.mk | 3 +- benchmarks/.gitignore | 1 + benchmarks/Android.mk | 36 +++ benchmarks/Makefile.am | 7 +- benchmarks/Makefile.sources| 6 + benchmarks/gem_userptr_benchmark.c | 513 + 6 files changed, 558 insertions(+), 8 deletions(-) create mode 100644 benchmarks/Android.mk create mode 100644 benchmarks/Makefile.sources create mode 100644 benchmarks/gem_userptr_benchmark.c diff --git a/Android.mk b/Android.mk index 8aeb2d4..0c969b8 100644 --- a/Android.mk +++ b/Android.mk @@ -1,2 +1 @@ -include $(call all-named-subdir-makefiles, lib tests tools) - +include $(call all-named-subdir-makefiles, lib tests tools benchmarks) diff --git a/benchmarks/.gitignore b/benchmarks/.gitignore index ddea6f7..09e5bd8 100644 --- a/benchmarks/.gitignore +++ b/benchmarks/.gitignore @@ -1,3 +1,4 @@ +gem_userptr_benchmark intel_upload_blit_large intel_upload_blit_large_gtt intel_upload_blit_large_map diff --git a/benchmarks/Android.mk b/benchmarks/Android.mk new file mode 100644 index 000..5bb8ef5 --- /dev/null +++ b/benchmarks/Android.mk @@ -0,0 +1,36 @@ +LOCAL_PATH := $(call my-dir) + +include $(LOCAL_PATH)/Makefile.sources + +## + +define add_benchmark +include $(CLEAR_VARS) + +LOCAL_SRC_FILES := $1.c + +LOCAL_CFLAGS += -DHAVE_STRUCT_SYSINFO_TOTALRAM +LOCAL_CFLAGS += -DANDROID -UNDEBUG -include "check-ndebug.h" +LOCAL_CFLAGS += -std=c99 +# FIXME: drop once Bionic correctly annotates "noreturn" on pthread_exit +LOCAL_CFLAGS += -Wno-error=return-type +# Excessive complaining for established cases. Rely on the Linux version warnings. +LOCAL_CFLAGS += -Wno-sign-compare + +LOCAL_MODULE := $1 +LOCAL_MODULE_TAGS := optional + +LOCAL_STATIC_LIBRARIES := libintel_gpu_tools + +LOCAL_SHARED_LIBRARIES := libpciaccess \ + libdrm\ + libdrm_intel + +include $(BUILD_EXECUTABLE) +endef + +## + +benchmark_list := $(bin_PROGRAMS) + +$(foreach item,$(benchmark_list),$(eval $(call add_benchmark,$(item diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am index e2ad784..d173bf4 100644 --- a/benchmarks/Makefile.am +++ b/benchmarks/Makefile.am @@ -1,9 +1,4 @@ - -bin_PROGRAMS = \ - intel_upload_blit_large \ - intel_upload_blit_large_gtt \ - intel_upload_blit_large_map \ - intel_upload_blit_small +include Makefile.sources AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources new file mode 100644 index 000..fd6c107 --- /dev/null +++ b/benchmarks/Makefile.sources @@ -0,0 +1,6 @@ +bin_PROGRAMS = \ +intel_upload_blit_large \ +intel_upload_blit_large_gtt \ +intel_upload_blit_large_map \ +intel_upload_blit_small \ +gem_userptr_benchmark You might split the makefile cleanup aspect of this into a separate patch, but I'm fine either way. Good idea, will try to squeeze that in. diff --git a/benchmarks/gem_userptr_benchmark.c b/benchmarks/gem_userptr_benchmark.c new file mode 100644 index 000..218f6f1 --- /dev/null +++ b/benchmarks/gem_userptr_benchmark.c @@ -0,0 +1,513 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial porti
Re: [Intel-gfx] [RFC] xf86-video-intel: enable hw-generated binding tables
On Tuesday, April 22, 2014 07:20:58 PM Daniel Vetter wrote: > On Tue, Apr 22, 2014 at 04:23:12PM +0100, Chris Wilson wrote: > > On Tue, Apr 22, 2014 at 06:16:34PM +0300, Abdiel Janulgue wrote: > > > This patch series enables resource streamer for xf86-video-intel UXA. > > > > > > Fixes i965 Mesa driver that makes use of resource streamer optimization > > > to survive a full piglit run without bricking the machine. Previously, > > > I get random hungs when doing a full piglit round when enabling RS. > > > After months of head-scratching, I noticed I didn't quite pay attention > > > > > > to this small detail in bspec: > > > "The binding table generator feature has a simple all or nothing > > > > > > model. If HW generated binding tables are enabled, the driver must > > > enable > > > the pool and use 3D_HW_BINDING_TABLE_POINTER_* commands." > > > > > > I realized that our xf86-video-intel driver is piping out 3D commands > > > as well that used the older manual-generated binding table format > > > that caused a conflict when Mesa is using the hw-generated binding table > > > format. > > > > This has to be per-context right? Otherwise how do you intend to > > coordinate multiple clients submitting to the kernel using incompatible > > command sets? Even in the restricted X/DRI sense, how do you intend to > > negotiate which to use? > > Hm, I've thought that the MI_BB_START should synchronize with the > asynchronous RS parsing/processing? Is there no way to disable RS again > for the next batch in a different context? I've already tried disabling RS at the end of every batch so that next batch in different context can continue to use older non-RS format. That does not work either and still causes hangs. What I've seen so far, it seems GPU does not like switching the format of commands from RS-format to non-RS format. It's either one way or the other. If switched on, it affects subsequent contexes henceforth expecting RS-format commands until the GPU gets reset. That's probably the note in bspec: "the binding table generator feature has a simple all or nothing model". > > If there's no way to solve this with contexts or some manual reset trick > using LRIs then we're pretty decently screwed up. Worst case we need to > stop the gpu and reset it to keep backwards compat :( ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v2 7/8] kms_cursor_crc: Add random cursor placement test
On Thu, Apr 10, 2014 at 03:08:11PM +0300, Antti Koskipaa wrote: > Signed-off-by: Antti Koskipaa > --- > tests/kms_cursor_crc.c | 14 ++ > 1 file changed, 14 insertions(+) > > diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c > index b2498a1..e00abf5 100644 > --- a/tests/kms_cursor_crc.c > +++ b/tests/kms_cursor_crc.c > @@ -214,6 +214,18 @@ static void test_crc_sliding(test_data_t *test_data) > } > } > > +static void test_crc_random(test_data_t *test_data) > +{ > + int i; > + > + /* Random cursor placement */ > + for (i = 0; i < 50; i++) { > + int x = rand() % (test_data->screenw + test_data->curw * 2) - > test_data->curw; > + int y = rand() % (test_data->screenh + test_data->curh * 2) - > test_data->curh; > + do_single_test(test_data, x, y); > + } > +} As this is not deterministic it would be nice if the test would print out some of the test parameters on failure (cursor coordinates and size at least). Otherwise there's no good way to analyze failures. > + > static bool prepare_crtc(test_data_t *test_data, igt_output_t *output, >int cursor_w, int cursor_h) > { > @@ -359,6 +371,8 @@ static void run_test_generic(data_t *data, int > cursor_max_size) > run_test(data, test_crc_offscreen, cursor_size, > cursor_size); > igt_subtest_f("cursor-%s-sliding", c_size) > run_test(data, test_crc_sliding, cursor_size, > cursor_size); > + igt_subtest_f("cursor-%s-random", c_size) > + run_test(data, test_crc_random, cursor_size, > cursor_size); > } > > } > -- > 1.8.3.2 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't check gmch state on inherited configs
On Sun, 13 Apr 2014, Daniel Vetter wrote: > ... our current modeset code isn't good enough yet to handle this. The > scenario is: > > 1. BIOS sets up a cloned config with lvds+external screen on the same > pipe, e.g. pipe B. > > 2. We read out that state for pipe B and assign the gmch_pfit state to > it. > > 3. The initial modeset switches the lvds to pipe A but due to lack of > atomic modeset we don't recompute the config of pipe B. > > -> both pipes now claim (in the sw pipe config structure) to use the > gmch_pfit, which just won't work. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74081 > Cc: Alan Stern > Cc: sta...@vger.kernel.org > Signed-off-by: Daniel Vetter Pushed to -fixes, thanks for the patch and review. BR, Jani. > --- > drivers/gpu/drm/i915/intel_display.c | 23 ++- > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 1390ab5e00dc..7b7987dc65ba 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9655,11 +9655,22 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_I(pipe_src_w); > PIPE_CONF_CHECK_I(pipe_src_h); > > - PIPE_CONF_CHECK_I(gmch_pfit.control); > - /* pfit ratios are autocomputed by the hw on gen4+ */ > - if (INTEL_INFO(dev)->gen < 4) > - PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); > - PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); > + /* > + * FIXME: BIOS likes to set up a cloned config with lvds+external > + * screen. Since we don't yet re-compute the pipe config when moving > + * just the lvds port away to another pipe the sw tracking won't match. > + * > + * Proper atomic modesets with recomputed global state will fix this. > + * Until then just don't check gmch state for inherited modes. > + */ > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { > + PIPE_CONF_CHECK_I(gmch_pfit.control); > + /* pfit ratios are autocomputed by the hw on gen4+ */ > + if (INTEL_INFO(dev)->gen < 4) > + PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); > + PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); > + } > + > PIPE_CONF_CHECK_I(pch_pfit.enabled); > if (current_config->pch_pfit.enabled) { > PIPE_CONF_CHECK_I(pch_pfit.pos); > @@ -11618,6 +11629,8 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > base.head) { > memset(&crtc->config, 0, sizeof(crtc->config)); > > + crtc->config.quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > + > crtc->active = dev_priv->display.get_pipe_config(crtc, >&crtc->config); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index c551472b892e..b885df150910 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -236,7 +236,8 @@ struct intel_crtc_config { >* tracked with quirk flags so that fastboot and state checker can act >* accordingly. >*/ > -#define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync > mode.flags */ > +#define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS(1<<0) /* unreliable sync > mode.flags */ > +#define PIPE_CONFIG_QUIRK_INHERITED_MODE (1<<1) /* mode inherited from > firmware */ > unsigned long quirks; > > /* User requested mode, only valid as a starting point to > -- > 1.8.4.rc3 > > ___ > 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: Discard BIOS framebuffers too small to accommodate chosen mode
On Wed, 23 Apr 2014, Ville Syrjälä wrote: > On Wed, Apr 23, 2014 at 08:54:31AM +0100, Chris Wilson wrote: >> If the inherited BIOS framebuffer is smaller than the mode selected for >> fbdev, then if we continue to use it then we cause display corruption as >> we do not setup the panel fitter to upscale. >> >> Regression from commit d978ef14456a38034f6c0e94a794129501f89200 >> Author: Jesse Barnes >> Date: Fri Mar 7 08:57:51 2014 -0800 >> >> drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS >> fbcon v12 >> >> v2: Add a debug message to track the discard of the BIOS fb. >> v3: Ville pointed out the difference between ref/unref >> >> Reported-by: Knut Petersen >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77767 >> Signed-off-by: Chris Wilson >> Cc: Jesse Barnes >> Reviewed-by: Daniel Vetter > > Reviewed-by: Ville Syrjälä Pushed to -fixes, thanks for the patch and review. BR, Jani. > >> --- >> drivers/gpu/drm/i915/intel_fbdev.c | 10 ++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c >> b/drivers/gpu/drm/i915/intel_fbdev.c >> index b16116db6c37..fbe7941f88c8 100644 >> --- a/drivers/gpu/drm/i915/intel_fbdev.c >> +++ b/drivers/gpu/drm/i915/intel_fbdev.c >> @@ -133,6 +133,16 @@ static int intelfb_create(struct drm_fb_helper *helper, >> >> mutex_lock(&dev->struct_mutex); >> >> +if (intel_fb && >> +(sizes->fb_width > intel_fb->base.width || >> + sizes->fb_height > intel_fb->base.height)) { >> +DRM_DEBUG_KMS("BIOS fb too small (%dx%d), we require (%dx%d)," >> + " releasing it\n", >> + intel_fb->base.width, intel_fb->base.height, >> + sizes->fb_width, sizes->fb_height); >> +drm_framebuffer_unreference(&intel_fb->base); >> +intel_fb = ifbdev->fb = NULL; >> +} >> if (!intel_fb || WARN_ON(!intel_fb->obj)) { >> DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n"); >> ret = intelfb_alloc(helper, sizes); >> -- >> 1.9.2 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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/4] drm/i915: get power domain in case the BIOS enabled eDP VDD
On Wed, 23 Apr 2014, Paulo Zanoni wrote: > From: Paulo Zanoni > > If I unplug the eDP monitor, the BIOS of my machine will enable the > VDD bit, then when the driver loads it will think VDD is enabled. It > will detect that the eDP is not enabled and return false from > intel_edp_init_connector. This will trigger a call to > edp_panel_vdd_off_sync(), which trigger a WARN saying that the > refcount of the power domain is less than zero. > > The problem happens because the driver gets a refcount whenever it > enables the VDD bit, and puts the refcount whenever it disables the > VDD bit. But on this case, the BIOS enabled VDD, so all we do is to > call put() without calling get() first, so the code added is there to > make sure we always have the get() in case the BIOS enabled the bit. > > v2: - Rebase > > Tested-by: Chris Wilson (v1) > Signed-off-by: Paulo Zanoni Patch 1/4 pushed to -fixes. BR, Jani. > --- > drivers/gpu/drm/i915/intel_dp.c | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 80e5598..44df493 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3785,7 +3785,8 @@ static bool intel_edp_init_connector(struct intel_dp > *intel_dp, > { > struct drm_connector *connector = &intel_connector->base; > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > + struct drm_device *dev = intel_encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_display_mode *fixed_mode = NULL; > struct drm_display_mode *downclock_mode = NULL; > @@ -3798,6 +3799,14 @@ static bool intel_edp_init_connector(struct intel_dp > *intel_dp, > if (!is_edp(intel_dp)) > return true; > > + /* The VDD bit needs a power domain reference, so if the bit is already > + * enabled when we boot, grab this reference. */ > + if (edp_have_panel_vdd(intel_dp)) { > + enum intel_display_power_domain power_domain; > + power_domain = intel_display_port_power_domain(intel_encoder); > + intel_display_power_get(dev_priv, power_domain); > + } > + > /* Cache DPCD and EDID for edp. */ > intel_edp_panel_vdd_on(intel_dp); > has_dpcd = intel_dp_get_dpcd(intel_dp); > -- > 1.9.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 i-g-t v2 2/8] kms_cursor_crc: Move cursor enable and disable calls where they belong
On Thu, Apr 10, 2014 at 03:08:06PM +0300, Antti Koskipaa wrote: > We can't have the hw cursor enabled during software render tests. > > Signed-off-by: Antti Koskipaa > --- > tests/kms_cursor_crc.c | 54 > -- > 1 file changed, 26 insertions(+), 28 deletions(-) > > diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c > index 52281d0..8802da6 100644 > --- a/tests/kms_cursor_crc.c > +++ b/tests/kms_cursor_crc.c > @@ -67,6 +67,30 @@ static void draw_cursor(cairo_t *cr, int x, int y, int w) > igt_paint_color_alpha(cr, x + w, y + w, w, w, 0.5, 0.5, 0.5, 1.0); > } > > +static void cursor_enable(test_data_t *test_data) > +{ > + data_t *data = test_data->data; > + igt_display_t *display = &data->display; > + igt_output_t *output = test_data->output; > + igt_plane_t *cursor; > + > + cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR); > + igt_plane_set_fb(cursor, &data->fb); > + igt_display_commit(display); > +} > + > +static void cursor_disable(test_data_t *test_data) > +{ > + data_t *data = test_data->data; > + igt_display_t *display = &data->display; > + igt_output_t *output = test_data->output; > + igt_plane_t *cursor; > + > + cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR); > + igt_plane_set_fb(cursor, NULL); > + igt_display_commit(display); > +} > + > static igt_pipe_crc_t *create_crc(data_t *data, enum pipe pipe) > { > igt_pipe_crc_t *crc; > @@ -85,10 +109,12 @@ static void do_single_test(test_data_t *test_data, int > x, int y) > > printf("."); fflush(stdout); > > + cursor_enable(test_data); > cursor = igt_output_get_plane(test_data->output, IGT_PLANE_CURSOR); > igt_plane_set_position(cursor, x, y); > igt_display_commit(display); > igt_wait_for_vblank(data->drm_fd, test_data->pipe); > + cursor_disable(test_data); > > igt_pipe_crc_collect_crc(pipe_crc, &crc); Cursor is getting disabled before we collect the crc:? > if (test_data->crc_must_match) > @@ -106,30 +132,6 @@ static void do_test(test_data_t *test_data, > do_single_test(test_data, left, bottom); > } > > -static void cursor_enable(test_data_t *test_data) > -{ > - data_t *data = test_data->data; > - igt_display_t *display = &data->display; > - igt_output_t *output = test_data->output; > - igt_plane_t *cursor; > - > - cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR); > - igt_plane_set_fb(cursor, &data->fb); > - igt_display_commit(display); > -} > - > -static void cursor_disable(test_data_t *test_data) > -{ > - data_t *data = test_data->data; > - igt_display_t *display = &data->display; > - igt_output_t *output = test_data->output; > - igt_plane_t *cursor; > - > - cursor = igt_output_get_plane(output, IGT_PLANE_CURSOR); > - igt_plane_set_fb(cursor, NULL); > - igt_display_commit(display); > -} > - > static void test_crc(test_data_t *test_data, >bool onscreen, int cursor_w, int cursor_h) > { > @@ -138,8 +140,6 @@ static void test_crc(test_data_t *test_data, > int top = test_data->top; > int bottom = test_data->bottom; > > - cursor_enable(test_data); > - > if (onscreen) { > /* cursor onscreen, crc should match, except when white visible > cursor is used */ > test_data->crc_must_match = false; > @@ -183,8 +183,6 @@ static void test_crc(test_data_t *test_data, > /* go nuts */ > do_test(test_data, INT_MIN, INT_MAX, INT_MIN, INT_MAX); > } > - > - cursor_disable(test_data); > } > > static bool prepare_crtc(test_data_t *test_data, igt_output_t *output, > -- > 1.8.3.2 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: remove useless runtime PM get call
On Tue, Apr 22, 2014 at 07:55:45PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > We already call intel_dp_power_get, which will get a power domain, and > every power domain should get a runtime PM reference, which will wake > up the machine. intel_crt_detect() could use the same treatment > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/intel_dp.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index a25f708..a0b1cda 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3171,11 +3171,9 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct intel_encoder *intel_encoder = &intel_dig_port->base; > struct drm_device *dev = connector->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > enum drm_connector_status status; > struct edid *edid = NULL; > > - intel_runtime_pm_get(dev_priv); > intel_dp_power_get(intel_encoder); > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > @@ -3209,7 +3207,6 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > > out: > intel_dp_power_put(intel_encoder); > - intel_runtime_pm_put(dev_priv); > > return status; > } > -- > 1.9.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/4] drm/i915: add intel_dp_power_get/put
On Tue, Apr 22, 2014 at 07:55:43PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > This was suggested by Chris on his review to the first version of > "drm/i915: get power domain in case the BIOS enabled eDP VDD". Well, > at least that's what I understood from his comment :) > > The downside of this patch is that in a few places we will call > intel_display_port_power_domain() twice instead of once. > > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/intel_dp.c | 61 > - > 1 file changed, 29 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 44df493..b385b03 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1063,13 +1063,30 @@ static u32 ironlake_get_pp_control(struct intel_dp > *intel_dp) > return control; > } > > +static void intel_dp_power_get(struct intel_encoder *encoder) > +{ > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + enum intel_display_power_domain power_domain; > + > + power_domain = intel_display_port_power_domain(encoder); > + intel_display_power_get(dev_priv, power_domain); > +} > + > +static void intel_dp_power_put(struct intel_encoder *encoder) > +{ > + struct drm_i915_private *dev_priv = encoder->base.dev->dev_private; > + enum intel_display_power_domain power_domain; > + > + power_domain = intel_display_port_power_domain(encoder); > + intel_display_power_put(dev_priv, power_domain); > +} There's nothing DP specific about these functions, so they could be moved somewhere else and used for other output types as well. > + > static bool _edp_panel_vdd_on(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > struct intel_encoder *intel_encoder = &intel_dig_port->base; > struct drm_i915_private *dev_priv = dev->dev_private; > - enum intel_display_power_domain power_domain; > u32 pp; > u32 pp_stat_reg, pp_ctrl_reg; > bool need_to_disable = !intel_dp->want_panel_vdd; > @@ -1082,8 +1099,7 @@ static bool _edp_panel_vdd_on(struct intel_dp *intel_dp) > if (edp_have_panel_vdd(intel_dp)) > return need_to_disable; > > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > + intel_dp_power_get(intel_encoder); > > DRM_DEBUG_KMS("Turning eDP VDD on\n"); > > @@ -1133,7 +1149,6 @@ static void edp_panel_vdd_off_sync(struct intel_dp > *intel_dp) > struct intel_digital_port *intel_dig_port = > dp_to_dig_port(intel_dp); > struct intel_encoder *intel_encoder = &intel_dig_port->base; > - enum intel_display_power_domain power_domain; > > DRM_DEBUG_KMS("Turning eDP VDD off\n"); > > @@ -1153,8 +1168,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp > *intel_dp) > if ((pp & POWER_TARGET_ON) == 0) > intel_dp->last_power_cycle = jiffies; > > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_put(dev_priv, power_domain); > + intel_dp_power_put(intel_encoder); > } > } > > @@ -1242,7 +1256,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > struct intel_encoder *intel_encoder = &intel_dig_port->base; > struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct drm_i915_private *dev_priv = dev->dev_private; > - enum intel_display_power_domain power_domain; > u32 pp; > u32 pp_ctrl_reg; > > @@ -1272,8 +1285,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > wait_panel_off(intel_dp); > > /* We got a reference when we enabled the VDD. */ > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_put(dev_priv, power_domain); > + intel_dp_power_put(intel_encoder); > } > > void intel_edp_backlight_on(struct intel_dp *intel_dp) > @@ -3161,13 +3173,10 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > struct drm_device *dev = connector->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > enum drm_connector_status status; > - enum intel_display_power_domain power_domain; > struct edid *edid = NULL; > > intel_runtime_pm_get(dev_priv); > - > - power_domain = intel_display_port_power_domain(intel_encoder); > - intel_display_power_get(dev_priv, power_domain); > + intel_dp_power_get(intel_encoder); > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, drm_get_connector_name(connector)); > @@ -3199,8 +3208,7 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > st
Re: [Intel-gfx] [PATCH I-g-t V2 1/2] tests: Add one ring sync case based on multi drm_fd to test ring semaphore sync under multi BSD rings
On Wed, 2014-04-23 at 09:13 +0800, Zhao Yakui wrote: > On Tue, 2014-04-22 at 13:44 -0600, Daniel Vetter wrote: > > On Tue, Apr 22, 2014 at 02:52:04PM +0300, Imre Deak wrote: > > > On Tue, 2014-04-15 at 10:38 +0800, Zhao Yakui wrote: > > > > The Broadwell GT3 machine has two independent BSD rings in kernel > > > > driver while > > > > it is transparent to the user-space driver. In such case it needs to > > > > check > > > > the ring sync between the two BSD rings. At the same time it also needs > > > > to > > > > check the sync among the second BSD ring and the other rings. > > > > > > > > Signed-off-by: Zhao Yakui > > > > --- > > > > tests/Makefile.sources |1 + > > > > tests/gem_multi_bsd_sync_loop.c | 172 > > > > +++ > > > > 2 files changed, 173 insertions(+) > > > > create mode 100644 tests/gem_multi_bsd_sync_loop.c > > > > > > > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources > > > > index c957ace..7cd9ca8 100644 > > > > --- a/tests/Makefile.sources > > > > +++ b/tests/Makefile.sources > > > > @@ -105,6 +105,7 @@ TESTS_progs = \ > > > > gem_render_tiled_blits \ > > > > gem_ring_sync_copy \ > > > > gem_ring_sync_loop \ > > > > + gem_multi_bsd_sync_loop \ > > > > gem_seqno_wrap \ > > > > gem_set_tiling_vs_gtt \ > > > > gem_set_tiling_vs_pwrite \ > > > > diff --git a/tests/gem_multi_bsd_sync_loop.c > > > > b/tests/gem_multi_bsd_sync_loop.c > > > > new file mode 100644 > > > > index 000..7f5b832 > > > > --- /dev/null > > > > +++ b/tests/gem_multi_bsd_sync_loop.c > > > > @@ -0,0 +1,172 @@ > > > > +/* > > > > + * Copyright © 2014 Intel Corporation > > > > + * > > > > + * Permission is hereby granted, free of charge, to any person > > > > obtaining a > > > > + * copy of this software and associated documentation files (the > > > > "Software"), > > > > + * to deal in the Software without restriction, including without > > > > limitation > > > > + * the rights to use, copy, modify, merge, publish, distribute, > > > > sublicense, > > > > + * and/or sell copies of the Software, and to permit persons to whom > > > > the > > > > + * Software is furnished to do so, subject to the following conditions: > > > > + * > > > > + * The above copyright notice and this permission notice (including > > > > the next > > > > + * paragraph) shall be included in all copies or substantial portions > > > > of the > > > > + * Software. > > > > + * > > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > > > EXPRESS OR > > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > > > MERCHANTABILITY, > > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > > > > SHALL > > > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES > > > > OR OTHER > > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > > > ARISING > > > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > > > > DEALINGS > > > > + * IN THE SOFTWARE. > > > > + * > > > > + * Authors: > > > > + *Daniel Vetter (based on > > > > gem_ring_sync_loop_*.c) > > > > + *Zhao Yakui > > > > + * > > > > + */ > > > > + > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include "drm.h" > > > > +#include "ioctl_wrappers.h" > > > > +#include "drmtest.h" > > > > +#include "intel_bufmgr.h" > > > > +#include "intel_batchbuffer.h" > > > > +#include "intel_io.h" > > > > +#include "i830_reg.h" > > > > +#include "intel_chipset.h" > > > > + > > > > +static drm_intel_bufmgr *bufmgr; > > > > +struct intel_batchbuffer *batch; > > > > +static drm_intel_bo *target_buffer; > > > > + > > > > +#define NUM_FD 50 > > > > + > > > > +static int mfd[NUM_FD]; > > > > +static drm_intel_bufmgr *mbufmgr[NUM_FD]; > > > > +static struct intel_batchbuffer *mbatch[NUM_FD]; > > > > +static drm_intel_bo *mbuffer[NUM_FD]; > > > > + > > > > + > > > > +/* > > > > + * Testcase: Basic check of ring<->ring sync using a dummy reloc > > > > + * > > > > + * Extremely efficient at catching missed irqs with semaphores=0 ... > > > > + */ > > > > + > > > > +#define MI_COND_BATCH_BUFFER_END (0x36<<23 | 1) > > > > +#define MI_DO_COMPARE (1<<21) > > > > + > > > > +static void > > > > +store_dword_loop(int fd) > > > > +{ > > > > + int i; > > > > + int num_rings = gem_get_num_rings(fd); > > > > + > > > > + srandom(0xdeadbeef); > > > > + > > > > + for (i = 0; i < SLOW_QUICK(0x10, 10); i++) { > > > > + int ring, mindex; > > > > + ring = random() % num_rings + 1; > > > > + mindex = random() % NUM_FD; > > > > + batch = mbatch[mindex]; > > > > + if (ring == I915_EXEC_RENDER) { > > > > + BEGIN_BATCH(4); > > > > +
Re: [Intel-gfx] [PATCH] drm/i915: Sanitize the enable_ppgtt module option once
On Tue, 22 Apr 2014, Daniel Vetter wrote: > Otherwise we'll end up spamming dmesg on every context creation on snb > with vt-d enabled. This regression was introduced in > > commit 246cbfb5fb9a1ca0997fbb135464c1ff5bb9c549 > Author: Ben Widawsky > Date: Fri Dec 6 14:11:14 2013 -0800 > > drm/i915: Reorganize intel_enable_ppgtt > > References: https://lkml.org/lkml/2014/4/17/599 > Cc: Alessandro Suardi > Cc: Ben Widawsky > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 26 +++--- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 0d514ff9b94c..47491c4a1181 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -34,25 +34,35 @@ static void gen8_setup_private_ppat(struct > drm_i915_private *dev_priv); > > bool intel_enable_ppgtt(struct drm_device *dev, bool full) > { > - if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) > + if (i915.enable_ppgtt == 0) > return false; > > if (i915.enable_ppgtt == 1 && full) > return false; > > + return true; > +} > + > +static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > +{ > + if (enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) > + return 0; > + > + if (i915.enable_ppgtt == 1) > + return 1; > + > + if (i915.enable_ppgtt == 2 && HAS_PPGTT(dev)) > + return 2; You should probably either pass enable_ppgtt as parameter and use that exclusively, or not pass it and refer to i915.enable_ppgtt directly, but not mix them. > + > #ifdef CONFIG_INTEL_IOMMU > /* Disable ppgtt on SNB if VT-d is on. */ > if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped) { > DRM_INFO("Disabling PPGTT because VT-d is on\n"); > - return false; > + return 0; > } > #endif > > - /* Full ppgtt disabled by default for now due to issues. */ > - if (full) > - return HAS_PPGTT(dev) && (i915.enable_ppgtt == 2); > - else > - return HAS_ALIASING_PPGTT(dev); > + return HAS_ALIASING_PPGTT(dev) ? 1 : 0; This conflicts in -fixes due to commit 8d214b7d9c45f4af23ce41b2bc74f79c44f760de Author: Ben Widawsky Date: Mon Mar 24 18:06:00 2014 -0700 drm/i915: Allow full PPGTT with param override Should I incorporate that in the conflict resolution for simplicity, letting 3.15 users also play with full ppgtt with the module param? BR, Jani. > } > > > @@ -1157,6 +1167,8 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct > i915_hw_ppgtt *ppgtt) > struct drm_i915_private *dev_priv = dev->dev_private; > int ret = 0; > > + i915.enable_ppgtt = sanitize_enable_ppgtt(dev, i915.enable_ppgtt); > + > ppgtt->base.dev = dev; > ppgtt->base.scratch = dev_priv->gtt.base.scratch; > > -- > 1.9.2 > > ___ > 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: Discard BIOS framebuffers too small to accommodate chosen mode
On Wed, Apr 23, 2014 at 08:54:31AM +0100, Chris Wilson wrote: > If the inherited BIOS framebuffer is smaller than the mode selected for > fbdev, then if we continue to use it then we cause display corruption as > we do not setup the panel fitter to upscale. > > Regression from commit d978ef14456a38034f6c0e94a794129501f89200 > Author: Jesse Barnes > Date: Fri Mar 7 08:57:51 2014 -0800 > > drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS > fbcon v12 > > v2: Add a debug message to track the discard of the BIOS fb. > v3: Ville pointed out the difference between ref/unref > > Reported-by: Knut Petersen > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77767 > Signed-off-by: Chris Wilson > Cc: Jesse Barnes > Reviewed-by: Daniel Vetter Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_fbdev.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > b/drivers/gpu/drm/i915/intel_fbdev.c > index b16116db6c37..fbe7941f88c8 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -133,6 +133,16 @@ static int intelfb_create(struct drm_fb_helper *helper, > > mutex_lock(&dev->struct_mutex); > > + if (intel_fb && > + (sizes->fb_width > intel_fb->base.width || > + sizes->fb_height > intel_fb->base.height)) { > + DRM_DEBUG_KMS("BIOS fb too small (%dx%d), we require (%dx%d)," > + " releasing it\n", > + intel_fb->base.width, intel_fb->base.height, > + sizes->fb_width, sizes->fb_height); > + drm_framebuffer_unreference(&intel_fb->base); > + intel_fb = ifbdev->fb = NULL; > + } > if (!intel_fb || WARN_ON(!intel_fb->obj)) { > DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n"); > ret = intelfb_alloc(helper, sizes); > -- > 1.9.2 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't check gmch state on inherited configs
On Sun, Apr 13, 2014 at 12:00:33PM +0200, Daniel Vetter wrote: > ... our current modeset code isn't good enough yet to handle this. The > scenario is: > > 1. BIOS sets up a cloned config with lvds+external screen on the same > pipe, e.g. pipe B. > > 2. We read out that state for pipe B and assign the gmch_pfit state to > it. > > 3. The initial modeset switches the lvds to pipe A but due to lack of > atomic modeset we don't recompute the config of pipe B. > > -> both pipes now claim (in the sw pipe config structure) to use the > gmch_pfit, which just won't work. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74081 > Cc: Alan Stern > Cc: sta...@vger.kernel.org > Signed-off-by: Daniel Vetter Right. Until we get atomic modeset this seems like the least hackish thing to do. Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_display.c | 23 ++- > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 1390ab5e00dc..7b7987dc65ba 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9655,11 +9655,22 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_I(pipe_src_w); > PIPE_CONF_CHECK_I(pipe_src_h); > > - PIPE_CONF_CHECK_I(gmch_pfit.control); > - /* pfit ratios are autocomputed by the hw on gen4+ */ > - if (INTEL_INFO(dev)->gen < 4) > - PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); > - PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); > + /* > + * FIXME: BIOS likes to set up a cloned config with lvds+external > + * screen. Since we don't yet re-compute the pipe config when moving > + * just the lvds port away to another pipe the sw tracking won't match. > + * > + * Proper atomic modesets with recomputed global state will fix this. > + * Until then just don't check gmch state for inherited modes. > + */ > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { > + PIPE_CONF_CHECK_I(gmch_pfit.control); > + /* pfit ratios are autocomputed by the hw on gen4+ */ > + if (INTEL_INFO(dev)->gen < 4) > + PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); > + PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); > + } > + > PIPE_CONF_CHECK_I(pch_pfit.enabled); > if (current_config->pch_pfit.enabled) { > PIPE_CONF_CHECK_I(pch_pfit.pos); > @@ -11618,6 +11629,8 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > base.head) { > memset(&crtc->config, 0, sizeof(crtc->config)); > > + crtc->config.quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > + > crtc->active = dev_priv->display.get_pipe_config(crtc, >&crtc->config); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index c551472b892e..b885df150910 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -236,7 +236,8 @@ struct intel_crtc_config { >* tracked with quirk flags so that fastboot and state checker can act >* accordingly. >*/ > -#define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync > mode.flags */ > +#define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS(1<<0) /* unreliable sync > mode.flags */ > +#define PIPE_CONFIG_QUIRK_INHERITED_MODE (1<<1) /* mode inherited from > firmware */ > unsigned long quirks; > > /* User requested mode, only valid as a starting point to > -- > 1.8.4.rc3 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 21/25] drm/i915: vlv: factor out vlv_force_gfx_clock and check for pending force-off
On Fri, Apr 18, 2014 at 04:35:02PM +0300, Imre Deak wrote: > This will be needed by the VLV runtime PM helpers too, so factor it out. > > Also add a safety check for the case where the previous force-off is > still pending, since I'm not sure if Punit can handle a new setting > while the previous one hasn't settled yet. > > v2: > - unchanged > v3: > - add a note to the commit message about the safety check (Ville) > > Signed-off-by: Imre Deak Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/i915_drv.c | 37 + > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 16 ++-- > 3 files changed, 40 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 1f88917..795caea 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -905,6 +905,43 @@ static void hsw_runtime_resume(struct drm_i915_private > *dev_priv) > hsw_disable_pc8(dev_priv); > } > > +int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on) > +{ > + u32 val; > + int err; > + > + val = I915_READ(VLV_GTLC_SURVIVABILITY_REG); > + WARN_ON(!!(val & VLV_GFX_CLK_FORCE_ON_BIT) == force_on); > + > +#define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT) > + /* Wait for a previous force-off to settle */ > + if (force_on) { > + err = wait_for(!COND, 5); > + if (err) { > + DRM_ERROR("timeout waiting for GFX clock force-off > (%08x)\n", > + I915_READ(VLV_GTLC_SURVIVABILITY_REG)); > + return err; > + } > + } > + > + val = I915_READ(VLV_GTLC_SURVIVABILITY_REG); > + val &= ~VLV_GFX_CLK_FORCE_ON_BIT; > + if (force_on) > + val |= VLV_GFX_CLK_FORCE_ON_BIT; > + I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, val); > + > + if (!force_on) > + return 0; > + > + err = wait_for(COND, 5); > + if (err) > + DRM_ERROR("timeout waiting for GFX clock force-on (%08x)\n", > + I915_READ(VLV_GTLC_SURVIVABILITY_REG)); > + > + return err; > +#undef COND > +} > + > static int intel_runtime_suspend(struct device *device) > { > struct pci_dev *pdev = to_pci_dev(device); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 5254f4b..3cac434 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1968,6 +1968,7 @@ extern unsigned long i915_chipset_val(struct > drm_i915_private *dev_priv); > extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv); > extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv); > extern void i915_update_gfx_val(struct drm_i915_private *dev_priv); > +int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on); > > extern void intel_console_resume(struct work_struct *work); > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index c45e5c1..d64ac32 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3129,16 +3129,7 @@ static void vlv_set_rps_idle(struct drm_i915_private > *dev_priv) > /* Mask turbo interrupt so that they will not come in between */ > I915_WRITE(GEN6_PMINTRMSK, 0x); > > - /* Bring up the Gfx clock */ > - I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, > - I915_READ(VLV_GTLC_SURVIVABILITY_REG) | > - VLV_GFX_CLK_FORCE_ON_BIT); > - > - if (wait_for(((VLV_GFX_CLK_STATUS_BIT & > - I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) { > - DRM_ERROR("GFX_CLK_ON request timed out\n"); > - return; > - } > + vlv_force_gfx_clock(dev_priv, true); > > dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit; > > @@ -3149,10 +3140,7 @@ static void vlv_set_rps_idle(struct drm_i915_private > *dev_priv) > & GENFREQSTATUS) == 0, 5)) > DRM_ERROR("timed out waiting for Punit\n"); > > - /* Release the Gfx clock */ > - I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, > - I915_READ(VLV_GTLC_SURVIVABILITY_REG) & > - ~VLV_GFX_CLK_FORCE_ON_BIT); > + vlv_force_gfx_clock(dev_priv, false); > > I915_WRITE(GEN6_PMINTRMSK, > gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq)); > -- > 1.8.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 19/25] drm/i915: reinit GT power save during resume
On Tue, Apr 22, 2014 at 08:21:07PM +0300, Imre Deak wrote: > During runtime suspend there can be a last pending rps.work, so make > sure it's canceled. Note that in the runtime suspend callback we can't > get any RPS interrupts since it's called only after the GPU goes idle > and we set the minimum RPS frequency. The next possibility for an RPS > interrupt is only after getting an RPM ref (for example because of a new > GPU command) and calling the RPM resume callback. > > v2: > - patch introduced in v2 of the patchset > v3: > - Change the order of canceling the rps.work and disabling interrupts to > avoid the race between interrupt disabling and the the rps.work. Race > spotted by Ville. > > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_drv.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index b87109c..edd4ab8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -919,6 +919,12 @@ static int intel_runtime_suspend(struct device *device) > > DRM_DEBUG_KMS("Suspending device\n"); > > + /* > + * rps.work can't be rearmed here, since we get here only after making > + * sure the GPU is idle and the RPS freq is set to the minimum. See > + * intel_mark_idle(). > + */ > + cancel_work_sync(&dev_priv->rps.work); Yeah makes sense. Well, unless the hardware is bonkers and generates an UP interrupt while idle. Reviewed-by: Ville Syrjälä > intel_runtime_pm_disable_interrupts(dev); > > if (IS_GEN6(dev)) > @@ -970,6 +976,7 @@ static int intel_runtime_resume(struct device *device) > gen6_update_ring_freq(dev); > > intel_runtime_pm_restore_interrupts(dev); > + intel_reset_gt_powersave(dev); > > DRM_DEBUG_KMS("Device resumed\n"); > return 0; > -- > 1.8.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 17/25] drm/i915: factor out gen6_update_ring_freq
On Fri, Apr 18, 2014 at 04:16:23PM +0300, Imre Deak wrote: > This is needed by the next patch moving the call out from platform > specific RPM callbacks to platform independent code. > > No functional change. > > v2: > - patch introduce in v2 of the patchset > v3: > - simplify platform check condition (Ville) > > Signed-off-by: Imre Deak Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/i915_drv.c | 2 -- > drivers/gpu/drm/i915/intel_display.c | 2 -- > drivers/gpu/drm/i915/intel_pm.c | 18 +++--- > 3 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index f3f9a33..afc31e3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -899,9 +899,7 @@ static void snb_runtime_resume(struct drm_i915_private > *dev_priv) > > intel_init_pch_refclk(dev); > i915_gem_init_swizzling(dev); > - mutex_lock(&dev_priv->rps.hw_lock); > gen6_update_ring_freq(dev); > - mutex_unlock(&dev_priv->rps.hw_lock); > } > > static void hsw_runtime_resume(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a902e13..bb7671d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7065,9 +7065,7 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) > > intel_prepare_ddi(dev); > i915_gem_init_swizzling(dev); > - mutex_lock(&dev_priv->rps.hw_lock); > gen6_update_ring_freq(dev); > - mutex_unlock(&dev_priv->rps.hw_lock); > } > > static void snb_modeset_global_resources(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 4e30b15..46f7b1a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3525,7 +3525,7 @@ static void gen6_enable_rps(struct drm_device *dev) > gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > } > > -void gen6_update_ring_freq(struct drm_device *dev) > +static void __gen6_update_ring_freq(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > int min_freq = 15; > @@ -3595,6 +3595,18 @@ void gen6_update_ring_freq(struct drm_device *dev) > } > } > > +void gen6_update_ring_freq(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + if (INTEL_INFO(dev)->gen < 6 || IS_VALLEYVIEW(dev)) > + return; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + __gen6_update_ring_freq(dev); > + mutex_unlock(&dev_priv->rps.hw_lock); > +} > + > int valleyview_rps_max_freq(struct drm_i915_private *dev_priv) > { > u32 val, rp0; > @@ -4566,10 +4578,10 @@ static void intel_gen6_powersave_work(struct > work_struct *work) > valleyview_enable_rps(dev); > } else if (IS_BROADWELL(dev)) { > gen8_enable_rps(dev); > - gen6_update_ring_freq(dev); > + __gen6_update_ring_freq(dev); > } else { > gen6_enable_rps(dev); > - gen6_update_ring_freq(dev); > + __gen6_update_ring_freq(dev); > } > dev_priv->rps.enabled = true; > mutex_unlock(&dev_priv->rps.hw_lock); > -- > 1.8.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 14/25] drm/i915: sanitize enable_rc6 option
On Fri, Apr 18, 2014 at 04:01:02PM +0300, Imre Deak wrote: > Atm, an invalid enable_rc6 module option will be silently ignored, so > emit an info message about it. Doing an early sanitization we can also > reuse intel_enable_rc6() in a follow-up patch to see if RC6 is actually > enabled. Currently the caller would have to filter a non-zero return > value based on the platform we are running on. For example on VLV with > i915.enable_rc6 set to 2, RC6 won't be enabled but atm > intel_enable_rc6() would still return 2 in this case. > > v2: > - simplify the platform check condition (Ville) > > Signed-off-by: Imre Deak Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_pm.c | 30 +++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index a56f6b1..075405a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3262,15 +3262,32 @@ static void intel_print_rc6_info(struct drm_device > *dev, u32 mode) >(mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off"); > } > > -int intel_enable_rc6(const struct drm_device *dev) > +static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6) > { > /* No RC6 before Ironlake */ > if (INTEL_INFO(dev)->gen < 5) > return 0; > > + /* RC6 is only on Ironlake mobile not on desktop */ > + if (INTEL_INFO(dev)->gen == 5 && !IS_IRONLAKE_M(dev)) > + return 0; > + > /* Respect the kernel parameter if it is set */ > - if (i915.enable_rc6 >= 0) > - return i915.enable_rc6; > + if (enable_rc6 >= 0) { > + int mask; > + > + if (INTEL_INFO(dev)->gen == 6 || IS_IVYBRIDGE(dev)) > + mask = INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE | > +INTEL_RC6pp_ENABLE; > + else > + mask = INTEL_RC6_ENABLE; > + > + if ((enable_rc6 & mask) != enable_rc6) > + DRM_INFO("Adjusting RC6 mask to %d (requested %d, valid > %d)\n", > + enable_rc6, enable_rc6 & mask, mask); > + > + return enable_rc6 & mask; > + } > > /* Disable RC6 on Ironlake */ > if (INTEL_INFO(dev)->gen == 5) > @@ -3282,6 +3299,11 @@ int intel_enable_rc6(const struct drm_device *dev) > return INTEL_RC6_ENABLE; > } > > +int intel_enable_rc6(const struct drm_device *dev) > +{ > + return i915.enable_rc6; > +} > + > static void gen6_enable_rps_interrupts(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -4496,6 +4518,8 @@ static void intel_init_emon(struct drm_device *dev) > > void intel_init_gt_powersave(struct drm_device *dev) > { > + i915.enable_rc6 = sanitize_rc6_option(dev, i915.enable_rc6); > + > if (IS_VALLEYVIEW(dev)) > valleyview_setup_pctx(dev); > } > -- > 1.8.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Discard BIOS framebuffers too small to accommodate chosen mode
If the inherited BIOS framebuffer is smaller than the mode selected for fbdev, then if we continue to use it then we cause display corruption as we do not setup the panel fitter to upscale. Regression from commit d978ef14456a38034f6c0e94a794129501f89200 Author: Jesse Barnes Date: Fri Mar 7 08:57:51 2014 -0800 drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v12 v2: Add a debug message to track the discard of the BIOS fb. v3: Ville pointed out the difference between ref/unref Reported-by: Knut Petersen Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77767 Signed-off-by: Chris Wilson Cc: Jesse Barnes Reviewed-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_fbdev.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index b16116db6c37..fbe7941f88c8 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -133,6 +133,16 @@ static int intelfb_create(struct drm_fb_helper *helper, mutex_lock(&dev->struct_mutex); + if (intel_fb && + (sizes->fb_width > intel_fb->base.width || +sizes->fb_height > intel_fb->base.height)) { + DRM_DEBUG_KMS("BIOS fb too small (%dx%d), we require (%dx%d)," + " releasing it\n", + intel_fb->base.width, intel_fb->base.height, + sizes->fb_width, sizes->fb_height); + drm_framebuffer_unreference(&intel_fb->base); + intel_fb = ifbdev->fb = NULL; + } if (!intel_fb || WARN_ON(!intel_fb->obj)) { DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n"); ret = intelfb_alloc(helper, sizes); -- 1.9.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 11/25] drm/i915: add missing error capturing of the PIPESTAT reg
On Fri, Apr 18, 2014 at 03:55:04PM +0300, Imre Deak wrote: > While checking the error capture path I noticed that we lacked the > power domain-on check for PIPESTAT so fix this by moving that to where > the rest of pipe registers are captured. > > The move also revealed that we actually don't include this register in > the error report, so fix that too. > > v2: > - patch introduced in v2 of the patchset > v3: > - add back !HAS_PCH_SPLIT check (Ville) > > Signed-off-by: Imre Deak Reviewed-by: Ville Syrjälä > > [ Ignore my previous comment about the gen<=5 || vlv check, I realized > that it's the same as !HAS_PCH_SPLIT. ] > > --- > drivers/gpu/drm/i915/i915_drv.h | 1 - > drivers/gpu/drm/i915/i915_gpu_error.c | 3 --- > drivers/gpu/drm/i915/intel_display.c | 5 + > 3 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7d6acb4..5254f4b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -325,7 +325,6 @@ struct drm_i915_error_state { > u32 gab_ctl; > u32 gfx_mode; > u32 extra_instdone[I915_NUM_INSTDONE_REG]; > - u32 pipestat[I915_MAX_PIPES]; > u64 fence[I915_MAX_NUM_FENCES]; > struct intel_overlay_error_state *overlay; > struct intel_display_error_state *display; > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c > b/drivers/gpu/drm/i915/i915_gpu_error.c > index ba79b59..7b5cc08 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -1028,7 +1028,6 @@ static void i915_capture_reg_state(struct > drm_i915_private *dev_priv, > struct drm_i915_error_state *error) > { > struct drm_device *dev = dev_priv->dev; > - int pipe; > > /* General organization >* 1. Registers specific to a single generation > @@ -1080,8 +1079,6 @@ static void i915_capture_reg_state(struct > drm_i915_private *dev_priv, > error->ier = I915_READ16(IER); > else > error->ier = I915_READ(IER); > - for_each_pipe(pipe) > - error->pipestat[pipe] = I915_READ(PIPESTAT(pipe)); > } > > /* 4: Everything else */ > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index cd68a24..a2f3790 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11901,6 +11901,7 @@ struct intel_display_error_state { > struct intel_pipe_error_state { > bool power_domain_on; > u32 source; > + u32 stat; > } pipe[I915_MAX_PIPES]; > > struct intel_plane_error_state { > @@ -11982,6 +11983,9 @@ intel_display_capture_error_state(struct drm_device > *dev) > } > > error->pipe[i].source = I915_READ(PIPESRC(i)); > + > + if (!HAS_PCH_SPLIT(dev)) > + error->pipe[i].stat = I915_READ(PIPESTAT(i)); > } > > error->num_transcoders = INTEL_INFO(dev)->num_pipes; > @@ -12032,6 +12036,7 @@ intel_display_print_error_state(struct > drm_i915_error_state_buf *m, > err_printf(m, " Power: %s\n", > error->pipe[i].power_domain_on ? "on" : "off"); > err_printf(m, " SRC: %08x\n", error->pipe[i].source); > + err_printf(m, " STAT: %08x\n", error->pipe[i].stat); > > err_printf(m, "Plane [%d]:\n", i); > err_printf(m, " CNTR: %08x\n", error->plane[i].control); > -- > 1.8.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] drm/i915: get a runtime PM ref for the deferred GPU reset work
On Wed, 2014-04-23 at 09:07 +0200, Daniel Vetter wrote: > On Wed, Apr 23, 2014 at 01:13:40AM +0300, Imre Deak wrote: > > Atm we can end up in the GPU reset deferred work in D3 state if the last > > runtime PM reference is dropped between detecting a hang/scheduling the > > work and executing the work. At least one such case I could trigger is > > the simulated reset via the i915_wedged debugfs entry. Fix this by > > getting an RPM reference around accessing the HW in the reset work. > > > > v2: > > - Instead of getting/putting the RPM reference in the reset work itself, > > get it already before scheduling the work. By this we also prevent > > going to D3 before the work gets to run, in addition to making sure > > that we run the work itself in D0. (Ville, Daniel) > > v3: > > - fix inverted logic fail when putting the RPM ref on behalf of a > > cancelled GPU reset work (Ville) > > v4: > > - Taking the RPM ref in the interrupt handler isn't really needed b/c > > it's already guaranteed that we hold an RPM ref until the end of the > > reset work in all cases we care about. So take the ref in the reset > > work (for cases like i915_wedged_set). (Daniel) > > > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/i915/i915_irq.c | 10 ++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index a651d0d..0e47111 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2175,6 +2175,14 @@ static void i915_error_work_func(struct work_struct > > *work) > >reset_event); > > > > /* > > +* In most cases it's guaranteed that we get here with an RPM > > +* reference held, for example because there is a pending GPU > > +* request that won't finish until the reset is done. This > > +* isn't the case at least when we get here by doing a > > +* simulated reset via debugs, so get an RPM reference. > > ^debugfs > > Also maybe "..., so get a RPM reference just to quiet the warnings." Do you mean it's not a problem to perform the reset in D3 state other than getting warnings? --Imre > -Daniel > > > +*/ > > + intel_runtime_pm_get(dev_priv); > > + /* > > * All state reset _must_ be completed before we update the > > * reset counter, for otherwise waiters might miss the reset > > * pending state and not properly drop locks, resulting in > > @@ -2184,6 +2192,8 @@ static void i915_error_work_func(struct work_struct > > *work) > > > > intel_display_handle_reset(dev); > > > > + intel_runtime_pm_put(dev_priv); > > + > > if (ret == 0) { > > /* > > * After all the gem state is reset, increment the reset > > -- > > 1.8.4 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Discard BIOS framebuffers too small to accommodate chosen mode
On Tue, Apr 22, 2014 at 10:05:58PM +0100, Chris Wilson wrote: > If the inherited BIOS framebuffer is smaller than the mode selected for > fbdev, then if we continue to use it then we cause display corruption as > we do not setup the panel fitter to upscale. > > Regression from commit d978ef14456a38034f6c0e94a794129501f89200 > Author: Jesse Barnes > Date: Fri Mar 7 08:57:51 2014 -0800 > > drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS > fbcon v12 > > Reported-by: Knut Petersen > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77767 > Signed-off-by: Chris Wilson > Cc: Jesse Barnes > --- > drivers/gpu/drm/i915/intel_fbdev.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > b/drivers/gpu/drm/i915/intel_fbdev.c > index b16116db6c37..28220ca838df 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -133,6 +133,12 @@ static int intelfb_create(struct drm_fb_helper *helper, > > mutex_lock(&dev->struct_mutex); > > + if (intel_fb && > + (sizes->fb_width > intel_fb->base.width || > + sizes->fb_height > intel_fb->base.height)) { > + drm_framebuffer_reference(&ifbdev->fb->base); unreference I know intel_fb == ifbdev->fb, but still I think it would look a bit less confusing if you passed &intel_fb->base to drm_framebuffer_unreference() instead of &ifbdev->fb->base. Simply because you use intel_fb->base in the size checks just before. > + intel_fb = ifbdev->fb = NULL; > + } > if (!intel_fb || WARN_ON(!intel_fb->obj)) { > DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n"); > ret = intelfb_alloc(helper, sizes); > -- > 1.9.2 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Discard BIOS framebuffers too small to accommodate chosen mode
If the inherited BIOS framebuffer is smaller than the mode selected for fbdev, then if we continue to use it then we cause display corruption as we do not setup the panel fitter to upscale. Regression from commit d978ef14456a38034f6c0e94a794129501f89200 Author: Jesse Barnes Date: Fri Mar 7 08:57:51 2014 -0800 drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v12 v2: Add a debug message to track the discard of the BIOS fb. Reported-by: Knut Petersen Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77767 Signed-off-by: Chris Wilson Cc: Jesse Barnes Reviewed-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_fbdev.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index b16116db6c37..dbd3f7ce6354 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -133,6 +133,16 @@ static int intelfb_create(struct drm_fb_helper *helper, mutex_lock(&dev->struct_mutex); + if (intel_fb && + (sizes->fb_width > intel_fb->base.width || +sizes->fb_height > intel_fb->base.height)) { + DRM_DEBUG_KMS("BIOS fb too small (%dx%d), we require (%dx%d)," + " releasing it\n", + intel_fb->base.width, intel_fb->base.height, + sizes->fb_width, sizes->fb_height); + drm_framebuffer_reference(&ifbdev->fb->base); + intel_fb = ifbdev->fb = NULL; + } if (!intel_fb || WARN_ON(!intel_fb->obj)) { DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n"); ret = intelfb_alloc(helper, sizes); -- 1.9.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm: Introduce drm_fb_helper_prepare()
On Wed, Apr 23, 2014 at 09:14:41AM +0200, Thierry Reding wrote: > On Tue, Apr 22, 2014 at 05:54:06PM +0200, Daniel Vetter wrote: > > On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote: > [...] > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > > b/drivers/gpu/drm/drm_fb_helper.c > [...] > > > @@ -502,6 +503,33 @@ static void drm_fb_helper_crtc_free(struct > > > drm_fb_helper *helper) > > > } > > > > > > /** > > > + * drm_fb_helper_prepare - setup a drm_fb_helper structure > > > + * @dev: DRM device > > > + * @helper: driver-allocated fbdev helper structure to set up > > > + * @funcs: pointer to structure of functions associate with this helper > > > + * > > > + * Sets up the bare minimum to make the framebuffer helper usable. This > > > is > > > + * useful to implement race-free initialization of the polling helpers. > > > In > > > + * that case a typical sequence would be: > > > + * > > > + * - call drm_fb_helper_prepare() > > > + * - set dev->mode_config.funcs > > > > This step is done in drm_fb_helper_prepare already. > > drm_fb_helper_prepare() sets fb_helper->funcs. dev->mode_config.funcs > needs to be set because it gets called by drm_kms_helper_hotplug_event() > which in turn is called by output_poll_execute(), which can be called at > any point after drm_kms_helper_poll_init(). It could be scheduled > immediately by drm_kms_helper_poll_enable(). > > I wonder if perhaps we should be wrapping that into a function as well. > Currently this is only documented in code by the drivers that call this. > But since it's only a single step it doesn't seem worth it. Perhaps if > we rolled the min/max_width/height into that function as well it would > be more worth it? That could be difficult to do since a couple of > drivers need to set those depending on the chipset generation. Oh I've misread this step for the fb_helper->funcs assignment. Makes sense and I don't think we need to augment your kerneldoc more to explain this. > > > + * - perform driver-specific setup Hm, maybe clarify this as "initialize modeset objects like crtcs, encoders and connectors"? Since that's the important part we need to get done here. > > > + * - call drm_kms_helper_poll_init() > > > > Maybe mention that after this you can start to handle hpd events using the > > probe helpers? > > Isn't that somewhat implied already? The poll helpers call directly the > dev->mode_config.funcs->output_poll_changed() function, which has > already been set up earlier. I've more meant that after this it's save for drivers to enable hpd interrupts and start to process them. I.e. - enable interrupts and start processing hpd events as an additional step to make it really cleear how it all fits together. Otherwise driver authors are left wondering wtf this isn't just one function call to do it all for them ;-) > > > + * - call drm_fb_helper_init() > > > + * - call drm_fb_helper_single_add_all_connectors() > > > + * - call drm_fb_helper_initial_config() > > > > Does this list look sane in the generated DocBook? Afaik kerneldoc > > unfortunately lacks any form of markup, at least afaik :( > > I must admit I didn't check. I'll make sure I do that before sending out > the next version. If it looks ugly I'm ok as-is, just wondered. Kerneldoc is a bit simplistic ime. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Sanitize the enable_ppgtt module option once
On Tue, Apr 22, 2014 at 10:22:15PM +0100, Chris Wilson wrote: > On Tue, Apr 22, 2014 at 10:17:50PM +0200, Daniel Vetter wrote: > > Otherwise we'll end up spamming dmesg on every context creation on snb > > with vt-d enabled. This regression was introduced in > > > > commit 246cbfb5fb9a1ca0997fbb135464c1ff5bb9c549 > > Author: Ben Widawsky > > Date: Fri Dec 6 14:11:14 2013 -0800 > > > > drm/i915: Reorganize intel_enable_ppgtt > > I started to consider what would happen if i915.enable_ppgtt changed on > the fly, but then saw that it is 0400 and this pre-initialisation makes > a lot of sense. So maybe we could mention that here: > > As the i915.enable_ppgtt is read-only it cannot be changed after the > module is loaded and so we can perform an early sanitization of the > values. > > > References: https://lkml.org/lkml/2014/4/17/599 > > Cc: Alessandro Suardi > > Cc: Ben Widawsky > > Signed-off-by: Daniel Vetter > > If you care to add in some of the comments, > Reviewed-by: Chris Wilson Fully agreed on both since the first thing I've checked is that the enable_ppgtt option is indeed 0400 ;-) Jani, can you please apply Chris' commit message text and comment when merging? -Daniel > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 26 +++--- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index 0d514ff9b94c..47491c4a1181 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -34,25 +34,35 @@ static void gen8_setup_private_ppat(struct > > drm_i915_private *dev_priv); > > > > bool intel_enable_ppgtt(struct drm_device *dev, bool full) > > { > > - if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) > > + if (i915.enable_ppgtt == 0) > > return false; > > > > if (i915.enable_ppgtt == 1 && full) > > return false; > > > > + return true; > > +} > > + > > /* i915.enable_ppgtt is read-only, so do an early pass to validate > * the user's requested state against the hardware/driver capabilities. > * We do this now so that we can print out any log messages once rather > * than every time we check intel_enable_ppgtt(). > */ > > +static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) > > +{ > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm: Introduce drm_fb_helper_prepare()
On Tue, Apr 22, 2014 at 05:54:06PM +0200, Daniel Vetter wrote: > On Tue, Apr 22, 2014 at 04:42:20PM +0200, Thierry Reding wrote: [...] > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > b/drivers/gpu/drm/drm_fb_helper.c [...] > > @@ -502,6 +503,33 @@ static void drm_fb_helper_crtc_free(struct > > drm_fb_helper *helper) > > } > > > > /** > > + * drm_fb_helper_prepare - setup a drm_fb_helper structure > > + * @dev: DRM device > > + * @helper: driver-allocated fbdev helper structure to set up > > + * @funcs: pointer to structure of functions associate with this helper > > + * > > + * Sets up the bare minimum to make the framebuffer helper usable. This is > > + * useful to implement race-free initialization of the polling helpers. In > > + * that case a typical sequence would be: > > + * > > + * - call drm_fb_helper_prepare() > > + * - set dev->mode_config.funcs > > This step is done in drm_fb_helper_prepare already. drm_fb_helper_prepare() sets fb_helper->funcs. dev->mode_config.funcs needs to be set because it gets called by drm_kms_helper_hotplug_event() which in turn is called by output_poll_execute(), which can be called at any point after drm_kms_helper_poll_init(). It could be scheduled immediately by drm_kms_helper_poll_enable(). I wonder if perhaps we should be wrapping that into a function as well. Currently this is only documented in code by the drivers that call this. But since it's only a single step it doesn't seem worth it. Perhaps if we rolled the min/max_width/height into that function as well it would be more worth it? That could be difficult to do since a couple of drivers need to set those depending on the chipset generation. > > + * - perform driver-specific setup > > + * - call drm_kms_helper_poll_init() > > Maybe mention that after this you can start to handle hpd events using the > probe helpers? Isn't that somewhat implied already? The poll helpers call directly the dev->mode_config.funcs->output_poll_changed() function, which has already been set up earlier. > > + * - call drm_fb_helper_init() > > + * - call drm_fb_helper_single_add_all_connectors() > > + * - call drm_fb_helper_initial_config() > > Does this list look sane in the generated DocBook? Afaik kerneldoc > unfortunately lacks any form of markup, at least afaik :( I must admit I didn't check. I'll make sure I do that before sending out the next version. Thierry pgppQ9Eb9BdhW.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't check gmch state on inherited configs
On Sun, Apr 13, 2014 at 12:00:33PM +0200, Daniel Vetter wrote: > ... our current modeset code isn't good enough yet to handle this. The > scenario is: > > 1. BIOS sets up a cloned config with lvds+external screen on the same > pipe, e.g. pipe B. > > 2. We read out that state for pipe B and assign the gmch_pfit state to > it. > > 3. The initial modeset switches the lvds to pipe A but due to lack of > atomic modeset we don't recompute the config of pipe B. > > -> both pipes now claim (in the sw pipe config structure) to use the > gmch_pfit, which just won't work. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74081 Tested-by: max > Cc: Alan Stern > Cc: sta...@vger.kernel.org > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_display.c | 23 ++- > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 1390ab5e00dc..7b7987dc65ba 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9655,11 +9655,22 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_I(pipe_src_w); > PIPE_CONF_CHECK_I(pipe_src_h); > > - PIPE_CONF_CHECK_I(gmch_pfit.control); > - /* pfit ratios are autocomputed by the hw on gen4+ */ > - if (INTEL_INFO(dev)->gen < 4) > - PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); > - PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); > + /* > + * FIXME: BIOS likes to set up a cloned config with lvds+external > + * screen. Since we don't yet re-compute the pipe config when moving > + * just the lvds port away to another pipe the sw tracking won't match. > + * > + * Proper atomic modesets with recomputed global state will fix this. > + * Until then just don't check gmch state for inherited modes. > + */ > + if (!PIPE_CONF_QUIRK(PIPE_CONFIG_QUIRK_INHERITED_MODE)) { > + PIPE_CONF_CHECK_I(gmch_pfit.control); > + /* pfit ratios are autocomputed by the hw on gen4+ */ > + if (INTEL_INFO(dev)->gen < 4) > + PIPE_CONF_CHECK_I(gmch_pfit.pgm_ratios); > + PIPE_CONF_CHECK_I(gmch_pfit.lvds_border_bits); > + } > + > PIPE_CONF_CHECK_I(pch_pfit.enabled); > if (current_config->pch_pfit.enabled) { > PIPE_CONF_CHECK_I(pch_pfit.pos); > @@ -11618,6 +11629,8 @@ static void intel_modeset_readout_hw_state(struct > drm_device *dev) > base.head) { > memset(&crtc->config, 0, sizeof(crtc->config)); > > + crtc->config.quirks |= PIPE_CONFIG_QUIRK_INHERITED_MODE; > + > crtc->active = dev_priv->display.get_pipe_config(crtc, >&crtc->config); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index c551472b892e..b885df150910 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -236,7 +236,8 @@ struct intel_crtc_config { >* tracked with quirk flags so that fastboot and state checker can act >* accordingly. >*/ > -#define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS (1<<0) /* unreliable sync > mode.flags */ > +#define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS(1<<0) /* unreliable sync > mode.flags */ > +#define PIPE_CONFIG_QUIRK_INHERITED_MODE (1<<1) /* mode inherited from > firmware */ > unsigned long quirks; > > /* User requested mode, only valid as a starting point to > -- > 1.8.4.rc3 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Discard BIOS framebuffers too small to accommodate chosen mode
On Tue, Apr 22, 2014 at 10:05:58PM +0100, Chris Wilson wrote: > If the inherited BIOS framebuffer is smaller than the mode selected for > fbdev, then if we continue to use it then we cause display corruption as > we do not setup the panel fitter to upscale. > > Regression from commit d978ef14456a38034f6c0e94a794129501f89200 > Author: Jesse Barnes > Date: Fri Mar 7 08:57:51 2014 -0800 > > drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS > fbcon v12 > > Reported-by: Knut Petersen > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77767 > Signed-off-by: Chris Wilson > Cc: Jesse Barnes > --- > drivers/gpu/drm/i915/intel_fbdev.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > b/drivers/gpu/drm/i915/intel_fbdev.c > index b16116db6c37..28220ca838df 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -133,6 +133,12 @@ static int intelfb_create(struct drm_fb_helper *helper, > > mutex_lock(&dev->struct_mutex); > > + if (intel_fb && > + (sizes->fb_width > intel_fb->base.width || > + sizes->fb_height > intel_fb->base.height)) { > + drm_framebuffer_reference(&ifbdev->fb->base); > + intel_fb = ifbdev->fb = NULL; Hm, maybe add a DRM_DEBUG_KMS("BIOS fb to small, releasing it\n"); here. With or without this is Reviewed-by: Daniel Vetter > + } > if (!intel_fb || WARN_ON(!intel_fb->obj)) { > DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n"); > ret = intelfb_alloc(helper, sizes); > -- > 1.9.2 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] drm/i915: get a runtime PM ref for the deferred GPU reset work
On Wed, Apr 23, 2014 at 01:13:40AM +0300, Imre Deak wrote: > Atm we can end up in the GPU reset deferred work in D3 state if the last > runtime PM reference is dropped between detecting a hang/scheduling the > work and executing the work. At least one such case I could trigger is > the simulated reset via the i915_wedged debugfs entry. Fix this by > getting an RPM reference around accessing the HW in the reset work. > > v2: > - Instead of getting/putting the RPM reference in the reset work itself, > get it already before scheduling the work. By this we also prevent > going to D3 before the work gets to run, in addition to making sure > that we run the work itself in D0. (Ville, Daniel) > v3: > - fix inverted logic fail when putting the RPM ref on behalf of a > cancelled GPU reset work (Ville) > v4: > - Taking the RPM ref in the interrupt handler isn't really needed b/c > it's already guaranteed that we hold an RPM ref until the end of the > reset work in all cases we care about. So take the ref in the reset > work (for cases like i915_wedged_set). (Daniel) > > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_irq.c | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index a651d0d..0e47111 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2175,6 +2175,14 @@ static void i915_error_work_func(struct work_struct > *work) > reset_event); > > /* > + * In most cases it's guaranteed that we get here with an RPM > + * reference held, for example because there is a pending GPU > + * request that won't finish until the reset is done. This > + * isn't the case at least when we get here by doing a > + * simulated reset via debugs, so get an RPM reference. ^debugfs Also maybe "..., so get a RPM reference just to quiet the warnings." -Daniel > + */ > + intel_runtime_pm_get(dev_priv); > + /* >* All state reset _must_ be completed before we update the >* reset counter, for otherwise waiters might miss the reset >* pending state and not properly drop locks, resulting in > @@ -2184,6 +2192,8 @@ static void i915_error_work_func(struct work_struct > *work) > > intel_display_handle_reset(dev); > > + intel_runtime_pm_put(dev_priv); > + > if (ret == 0) { > /* >* After all the gem state is reset, increment the reset > -- > 1.8.4 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Revert "drm/i915: fix infinite loop at gen6_update_ring_freq"
On Tue, Apr 22, 2014 at 06:25:12PM -0300, Paulo Zanoni wrote: > 2014-04-11 6:02 GMT-03:00 Daniel Vetter : > > On Thu, Apr 10, 2014 at 10:52:26AM -0700, Ben Widawsky wrote: > >> On Thu, Apr 10, 2014 at 10:50:43AM -0700, Ben Widawsky wrote: > >> > On Thu, Apr 10, 2014 at 09:04:47AM +0200, Daniel Vetter wrote: > >> > > This reverts commit 4b28a1f3ef55a3b0b68dbab1fe6dbaf18e186710. > >> > > > >> > > This patch duct-tapes over some issue in the current bdw rps patches > >> > > which must wait with enabling rc6/rps until the very first batch has > >> > > been submitted by userspace. > >> > > > >> > > But those patches aren't merged yet, and for upstream we need to have > >> > > an in-kernel emission of the very first batch. I shouldn't have > >> > > merged this patch so let's revert it again. > >> > > >> > I said this on the mailing last before you merged the patch. > >> > >> 20140402050338.gb13...@bwidawsk.net > > > > 20140402145813.GV7225@phenom.ffwll.local will explain things. > > There's now a regression report pointing to the revert: > https://bugs.freedesktop.org/show_bug.cgi?id=77565 . > > What is the proposed solution now? Just WARN and still avoid the > infinite loop? Or keep the infinite loop and leave the bug open? > Disable BDW runtime PM? I've thought that we can only hit this with the as-yet unmerged rc6 patches on bdw, so I'm really confused why this blows up now? In any case I've thought Imre has stumbled over a similar issue on byt and he has a fix to prevent runtime pm until the delayed rps init has run. I've assigned the bug to him. Still confused why this suddenly blew up ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: get power domain in case the BIOS enabled eDP VDD
On Tue, Apr 22, 2014 at 07:55:42PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > If I unplug the eDP monitor, the BIOS of my machine will enable the > VDD bit, then when the driver loads it will think VDD is enabled. It > will detect that the eDP is not enabled and return false from > intel_edp_init_connector. This will trigger a call to > edp_panel_vdd_off_sync(), which trigger a WARN saying that the > refcount of the power domain is less than zero. > > The problem happens because the driver gets a refcount whenever it > enables the VDD bit, and puts the refcount whenever it disables the > VDD bit. But on this case, the BIOS enabled VDD, so all we do is to > call put() without calling get() first, so the code added is there to > make sure we always have the get() in case the BIOS enabled the bit. > > v2: - Rebase > > Tested-by: Chris Wilson (v1) > Signed-off-by: Paulo Zanoni This regression was introduced in commit e9cb81a22841908b1c075156b409a538d09c8466 Author: Paulo Zanoni Date: Thu Nov 21 13:47:23 2013 -0200 drm/i915: get a runtime PM reference when the panel VDD is on Please don't forget the regression notice! With that this is Reviewed-by: Daniel Vetter Cc: sta...@vger.kernel.org (v3.13+) > --- > drivers/gpu/drm/i915/intel_dp.c | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 80e5598..44df493 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3785,7 +3785,8 @@ static bool intel_edp_init_connector(struct intel_dp > *intel_dp, > { > struct drm_connector *connector = &intel_connector->base; > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > - struct drm_device *dev = intel_dig_port->base.base.dev; > + struct intel_encoder *intel_encoder = &intel_dig_port->base; > + struct drm_device *dev = intel_encoder->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_display_mode *fixed_mode = NULL; > struct drm_display_mode *downclock_mode = NULL; > @@ -3798,6 +3799,14 @@ static bool intel_edp_init_connector(struct intel_dp > *intel_dp, > if (!is_edp(intel_dp)) > return true; > > + /* The VDD bit needs a power domain reference, so if the bit is already > + * enabled when we boot, grab this reference. */ > + if (edp_have_panel_vdd(intel_dp)) { > + enum intel_display_power_domain power_domain; > + power_domain = intel_display_port_power_domain(intel_encoder); > + intel_display_power_get(dev_priv, power_domain); > + } > + > /* Cache DPCD and EDID for edp. */ > intel_edp_panel_vdd_on(intel_dp); > has_dpcd = intel_dp_get_dpcd(intel_dp); > -- > 1.9.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx