[git pull] drm: previous pull req + 1.

[git pull] drm: previous pull req + 1.

[Dave Airlie]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 15440 bytes --]


Hi Linus,

Please pull the 'drm-linus' branch from
ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git drm-linus

This is the same tree from the previous pull request + the fix from Pierre 
that actually makes the PAE/GEM combination on i965 work. \o/

Dave.

 drivers/char/agp/agp.h                   |   12 +-
 drivers/char/agp/ali-agp.c               |   28 +-
 drivers/char/agp/amd-k7-agp.c            |    2 +-
 drivers/char/agp/amd64-agp.c             |    2 +-
 drivers/char/agp/ati-agp.c               |   23 +-
 drivers/char/agp/backend.c               |    8 +-
 drivers/char/agp/efficeon-agp.c          |    5 +-
 drivers/char/agp/generic.c               |   69 ++---
 drivers/char/agp/hp-agp.c                |    9 +-
 drivers/char/agp/i460-agp.c              |   47 ++--
 drivers/char/agp/intel-agp.c             |   49 ++--
 drivers/char/agp/nvidia-agp.c            |    2 +-
 drivers/char/agp/parisc-agp.c            |   20 +-
 drivers/char/agp/sgi-agp.c               |    9 +-
 drivers/char/agp/sworks-agp.c            |    2 +-
 drivers/char/agp/uninorth-agp.c          |   30 ++-
 drivers/gpu/drm/drm_agpsupport.c         |   14 +-
 drivers/gpu/drm/drm_auth.c               |    4 +-
 drivers/gpu/drm/drm_bufs.c               |  140 ++++------
 drivers/gpu/drm/drm_context.c            |    4 +-
 drivers/gpu/drm/drm_debugfs.c            |    9 +-
 drivers/gpu/drm/drm_dma.c                |   31 +--
 drivers/gpu/drm/drm_drawable.c           |   25 +-
 drivers/gpu/drm/drm_drv.c                |   18 +-
 drivers/gpu/drm/drm_edid.c               |  100 ++++---
 drivers/gpu/drm/drm_fops.c               |    8 +-
 drivers/gpu/drm/drm_gem.c                |    8 +-
 drivers/gpu/drm/drm_hashtab.c            |    6 +-
 drivers/gpu/drm/drm_ioctl.c              |   14 +-
 drivers/gpu/drm/drm_irq.c                |   44 +--
 drivers/gpu/drm/drm_memory.c             |   33 +--
 drivers/gpu/drm/drm_mm.c                 |   48 +---
 drivers/gpu/drm/drm_pci.c                |   53 +----
 drivers/gpu/drm/drm_proc.c               |    8 +-
 drivers/gpu/drm/drm_scatter.c            |   33 +--
 drivers/gpu/drm/drm_sman.c               |   29 +-
 drivers/gpu/drm/drm_stub.c               |   19 +-
 drivers/gpu/drm/drm_vm.c                 |   12 +-
 drivers/gpu/drm/i810/i810_dma.c          |    6 +-
 drivers/gpu/drm/i830/i830_dma.c          |    6 +-
 drivers/gpu/drm/i915/i915_dma.c          |   45 +--
 drivers/gpu/drm/i915/i915_drv.h          |    2 +
 drivers/gpu/drm/i915/i915_gem.c          |   86 ++++--
 drivers/gpu/drm/i915/i915_gem_debugfs.c  |    4 +-
 drivers/gpu/drm/i915/i915_gem_tiling.c   |   67 ++++-
 drivers/gpu/drm/i915/i915_mem.c          |   24 +-
 drivers/gpu/drm/i915/intel_bios.c        |    6 +-
 drivers/gpu/drm/i915/intel_display.c     |   20 +-
 drivers/gpu/drm/i915/intel_fb.c          |    6 +-
 drivers/gpu/drm/i915/intel_tv.c          |   11 +-
 drivers/gpu/drm/mga/mga_dma.c            |   14 +-
 drivers/gpu/drm/r128/r128_cce.c          |   12 +-
 drivers/gpu/drm/r128/r128_state.c        |   84 +++---
 drivers/gpu/drm/radeon/r100.c            |   85 +++---
 drivers/gpu/drm/radeon/r300.c            |  478 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/radeon/r300.h            |   36 +++
 drivers/gpu/drm/radeon/radeon.h          |    9 +-
 drivers/gpu/drm/radeon/radeon_asic.h     |   15 +-
 drivers/gpu/drm/radeon/radeon_atombios.c |    2 -
 drivers/gpu/drm/radeon/radeon_combios.c  |    9 +
 drivers/gpu/drm/radeon/radeon_cp.c       |    9 +-
 drivers/gpu/drm/radeon/radeon_device.c   |    4 +
 drivers/gpu/drm/radeon/radeon_display.c  |    2 +-
 drivers/gpu/drm/radeon/radeon_drv.c      |    2 +-
 drivers/gpu/drm/radeon/radeon_i2c.c      |    6 +-
 drivers/gpu/drm/radeon/radeon_kms.c      |    4 +-
 drivers/gpu/drm/radeon/radeon_mem.c      |   24 +-
 drivers/gpu/drm/radeon/radeon_reg.h      |    1 +
 drivers/gpu/drm/radeon/radeon_state.c    |   16 +-
 drivers/gpu/drm/radeon/radeon_ttm.c      |    8 +-
 drivers/gpu/drm/radeon/rv515.c           |   58 ++++
 drivers/gpu/drm/savage/savage_bci.c      |   21 +-
 drivers/gpu/drm/savage/savage_state.c    |   17 +-
 drivers/gpu/drm/sis/sis_drv.c            |    6 +-
 drivers/gpu/drm/ttm/ttm_agp_backend.c    |    3 +-
 drivers/gpu/drm/ttm/ttm_bo.c             |   11 +-
 drivers/gpu/drm/ttm/ttm_tt.c             |   11 +-
 drivers/gpu/drm/via/via_map.c            |    8 +-
 include/drm/drmP.h                       |   52 ----
 include/drm/drm_edid.h                   |   92 +++---
 include/drm/drm_memory_debug.h           |  309 -------------------
 include/drm/drm_mm.h                     |   21 +-
 include/linux/agp_backend.h              |    2 +-
 83 files changed, 1391 insertions(+), 1300 deletions(-)
 create mode 100644 drivers/gpu/drm/radeon/r300.h
 delete mode 100644 include/drm/drm_memory_debug.h

commit 0b7af262aba912f52bc6ef76f1bc0960b01b8502
Author: Pierre Willenbrock <pierre@pirsoft.de>
Date:   Fri Jun 19 18:31:47 2009 +0200

    agp/intel: Make intel_i965_mask_memory use dma_addr_t for physical addresses
    
    Otherwise, the high bits to be stuffed in the unused lower bits of the
    page address are lost.
    
    Signed-off-by: Pierre Willenbrock <pierre@pirsoft.de>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit a95fe463e73b8c7b2d97606ac86ce261f1270726
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Jun 19 10:52:57 2009 +1000

    agp: add user mapping support to ATI AGP bridge.
    
    This should fix TTM/KMS on some of the original ATI IGP chipsets.
    (rs100/rs200)
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 95934f939c46ea2b37f3c91a4f8c82e003727761
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Jun 19 10:29:20 2009 +1000

    drm/i915: enable GEM on PAE.
    
    In theory now that the AGP subsystem is using struct page, we should
    have on problems enabling GEM on PAE systems.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit fc436d9d3720b382566e03bef2d7391a58714999
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Jun 19 10:22:21 2009 +1000

    drm/radeon: fix unused variables warning
    
    just remove i variable left over from previous code.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 07613ba2f464f59949266f4337b75b91eb610795
Author: Dave Airlie <airlied@redhat.com>
Date:   Fri Jun 12 14:11:41 2009 +1000

    agp: switch AGP to use page array instead of unsigned long array
    
    This switches AGP to use an array of pages for tracking the
    pages allocated to the GART. This should enable GEM on PAE to work
    a lot better as we can pass highmem pages to the PAT code and it will
    do the right thing with them.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 2908826d045a89805714e0a3055a99dc40565d41
Author: Ondrej Zary <linux@rainbow-software.org>
Date:   Wed Jun 10 12:41:11 2009 -0700

    agpgart: detected ALi M???? chipset with M1621
    
    Add M1621 chipset name to ali-agp, preventing "Detected ALi M???? chipset"
    message.
    
    Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 068a117ca38f27c9641db7642f24fe9270d9424e
Author: Jerome Glisse <glisse@freedesktop.org>
Date:   Wed Jun 17 13:28:30 2009 +0200

    drm/radeon: command stream checker for r3xx-r5xx hardware
    
    For security purpose we want to make sure the userspace process doesn't
    access memory beyond buffer it owns. To achieve this we need to check
    states the userspace program. For color buffer and zbuffer we check that
    the clipping register will discard access beyond buffers set as color
    or zbuffer. For vertex buffer we check that no vertex fetch will happen
    beyond buffer end. For texture we check various texture states (number
    of mipmap level, texture size, texture depth, ...) to compute the amount
    of memory the texture fetcher might access.
    
    The command stream checking impact the performances so far quick benchmark
    shows an average of 3% decrease in fps of various applications. It can
    be optimized a bit more by caching result of checking and thus avoid a
    full recheck if no states changed since last check.
    
    Note that this patch is still incomplete on checking side as it doesn't
    check 2d rendering states.
    
    Signed-off-by: Jerome Glisse <jglisse@redhat.com>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 8b5c744485b75d940ccb1c83c9a358b20eb91346
Author: Michel Dänzer <daenzer@vmware.com>
Date:   Wed Jun 17 18:28:38 2009 +0200

    drm/radeon: Fully initialize LVDS info also when we can't get it from the ROM.
    
    This makes the panel on my PowerBook light up.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 4e484e7dc5856ff5086b6329d82e36d4adaf1f02
Author: Michel Dänzer <daenzer@vmware.com>
Date:   Tue Jun 16 17:29:06 2009 +0200

    radeon: Fix CP byte order on big endian architectures with KMS.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 62369028c7e2039b821799b3db52f0d622f0e8b5
Author: Michel Dänzer <daenzer@vmware.com>
Date:   Mon Jun 15 16:56:15 2009 +0200

    agp/uninorth: Handle user memory types.
    
    This adds support for TTM to the uninorth AGP bridge.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 46f4b3eab73e621bc239bfa62ebdc44dcc0a877a
Author: Michel Dänzer <daenzer@vmware.com>
Date:   Mon Jun 15 16:56:13 2009 +0200

    drm/ttm: Add some powerpc cache flush code.
    
    Optimise the powerpc flushing path for TTM.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 919f32f1df228723f66bf5c5aed23e0ab076b1a1
Author: Michel Dänzer <daenzer@vmware.com>
Date:   Mon Jun 15 16:56:09 2009 +0200

    radeon: Enable modesetting on non-x86.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 55c93278ec03c349af7b01933655b31c7f740df4
Author: Michel Dänzer <daenzer@vmware.com>
Date:   Mon Jun 15 16:56:11 2009 +0200

    drm/radeon: Respect AGP cant_use_aperture flag.
    
    Some AGP devices can't map the aperture, radeon needs to tell TTM this.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 0454beab0f6bc6d350860abd549b86959d2f6f40
Author: Michel Dänzer <daenzer@vmware.com>
Date:   Mon Jun 15 16:56:07 2009 +0200

    drm: EDID endianness fixes.
    
    Mostly replacing bitfields with explicit masks and shifts.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 00fa28ae29f70c9f26023f9922c4d2e1ca1297e3
Author: Dave Airlie <airlied@itt42.(none)>
Date:   Thu Jun 18 18:08:33 2009 +1000

    drm/radeon: this VRAM vs aperture test is wrong, just remove it.
    
    Its quite valid to have VRAM < aperture size.
    
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 87ef92092fd092936535ba057ee19b97bb6a709a
Author: Thomas Hellstrom <thellstrom@vmware.com>
Date:   Wed Jun 17 12:29:57 2009 +0200

    drm/ttm: fix an error path to exit function correctly
    
    Just a goto instead of a direct exit.
    
    Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 89579f778266d5a4d08d0c64c46b1565218de9f9
Author: Thomas Hellstrom <thellstrom@vmware.com>
Date:   Wed Jun 17 12:29:56 2009 +0200

    drm: Apply "Memory fragmentation from lost alignment blocks"
    
    also for the atomic path by using a common code-path.
    
    Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 78ecf091aa592a9e160ebbbfa5873c2bb2e2d0f8
Author: Thomas Hellstrom <thellstrom@vmware.com>
Date:   Wed Jun 17 12:29:55 2009 +0200

    ttm: Return -ERESTART when a signal interrupts bo eviction.
    
    A bug caused the ttm code to just terminate the wait when a signal
    was received while waiting for the GPU to release a buffer object that
    was to be evicted.
    
    Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

commit 9a298b2acd771d8a5c0004d8f8e4156c65b11f6b
Author: Eric Anholt <eric@anholt.net>
Date:   Tue Mar 24 12:23:04 2009 -0700

    drm: Remove memory debugging infrastructure.
    
    It hasn't been used in ages, and having the user tell your how much
    memory is being freed at free time is a recipe for disaster even if it
    was ever used.
    
    Signed-off-by: Eric Anholt <eric@anholt.net>

commit 52dc7d32b88156248167864f77a9026abe27b432
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sat Jun 6 09:46:01 2009 +0100

    drm/i915: Clear fence register on tiling stride change.
    
    The fence register value also depends upon the stride of the object, so we
    need to clear the fence if that is changed as well.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    [anholt: Added 8xx and 965 paths, and renamed the confusing
    i915_gem_object_tiling_ok function to i915_gem_object_fence_offset_ok]
    Signed-off-by: Eric Anholt <eric@anholt.net>

commit 8c4b8c3f34de4e2da20df042bba173fe557f8b45
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jun 17 22:08:52 2009 +0100

    drm/i915: Install fence register for tiled scanout on i915
    
    With the work by Jesse Barnes to eliminate allocation of fences during
    execbuffer, it becomes possible to write to the scan-out buffer with it
    never acquiring a fence (simply by only ever writing to the object using
    tiled GPU commands and never writing to it via the GTT). So for pre-i965
    chipsets which require fenced access for tiled scan-out buffers, we need
    to obtain a fence register.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Signed-off-by: Eric Anholt <eric@anholt.net>

commit d78b47b9a527bf46cb6081555847facd6efd5f81
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jun 17 21:52:49 2009 +0100

    drm/i915: detach/attach get/put pages symmetry
    
    After performing an operation over the page list for a buffer retrieved by
    i915_gem_object_get_pages() the pages need to be returned with
    i915_gem_object_put_pages(). This was not being observed for the phys
    objects which were thus leaking references to their backing pages.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    CC: Dave Airlie <airlied@gmail.com>
    Signed-off-by: Eric Anholt <eric@anholt.net>

commit 76cff81ad1cfa3bd8b52b5e4510702ce2ed28335
Author: Ben Gamari <bgamari.foss@gmail.com>
Date:   Wed Jun 10 18:26:20 2009 -0400

    drm/i915: A few debugfs formatting fixes
    
    Signed-Off-By: Ben Gamari <bgamari.foss@gmail.com>
    Signed-off-by: Eric Anholt <eric@anholt.net>

commit 049b77cb2ad8bd36308a4a424ca4f2eb4d65d2af
Author: Ben Gamari <bgamari.foss@gmail.com>
Date:   Thu Jun 11 00:44:26 2009 -0400

    drm/i915: Warn when inteldrmfb fails to restore its framebuffer config
    
    While sifting through the inteldrmfb code trying to solve #22040 I found that
    the fb restore path doesn't check the return value of
    drm_crtc_helper_set_config(), which seems to have all sorts of potential
    failure modes. We should warn someone if one of these is triggered.
    
    Signed-Off-By: Ben Gamari <bgamari.foss@gmail.com>
    Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
    [anholt: hand-applied, failures are mine]
    Signed-off-by: Eric Anholt <eric@anholt.net>

Re: [git pull] drm: previous pull req + 1.

[Maxim Levitsky]

On Sat, 2009-06-20 at 06:23 +0100, Dave Airlie wrote:
> Hi Linus,
> 
> Please pull the 'drm-linus' branch from
> ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git drm-linus
> 
> This is the same tree from the previous pull request + the fix from Pierre 
> that actually makes the PAE/GEM combination on i965 work. \o/

Something from this tree breaks my i965.
Using -git just before this was pulled,

 a552f0af753eb4b5bbbe9eff205fe874b04c4583 works, but using latest git

makes google earth stall, it doesn't update its main window. It appears
that openining and closing its menu, allows it to progress frame after
frame. No crashes hangs however.

Used latest -git of all graphic components (mesa, libdrm, xserver, intel
driver)

KMS is used, as well as UXA and DRI2 (this is enabled by default now)


Compiz used as well, but this happens without it as well.


Best regards,
	Maxim Levitsky

Re: [git pull] drm: previous pull req + 1.

[Linus Torvalds]

On Sun, 21 Jun 2009, Maxim Levitsky wrote:
> 
> Something from this tree breaks my i965.
> Using -git just before this was pulled,
> 
>  a552f0af753eb4b5bbbe9eff205fe874b04c4583 works, but using latest git
> 
> makes google earth stall, it doesn't update its main window. It appears
> that openining and closing its menu, allows it to progress frame after
> frame. No crashes hangs however.

Can you bisect? There's not a tons of commit there, so it shouldn't be 
more than a couple of recompiles/reboots, and you'd be able to pinpoint 
the exact commit that breaks. That will help people figure it out, or at 
worst just pinpoint what we need to revert.

		Linus

Re: [git pull] drm: previous pull req + 1.

[Dave Airlie]

> > 
> > Please pull the 'drm-linus' branch from
> > ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git drm-linus
> > 
> > This is the same tree from the previous pull request + the fix from Pierre 
> > that actually makes the PAE/GEM combination on i965 work. \o/
> 
> Something from this tree breaks my i965.
> Using -git just before this was pulled,
> 
>  a552f0af753eb4b5bbbe9eff205fe874b04c4583 works, but using latest git
> 
> makes google earth stall, it doesn't update its main window. It appears
> that openining and closing its menu, allows it to progress frame after
> frame. No crashes hangs however.
> 
> Used latest -git of all graphic components (mesa, libdrm, xserver, intel
> driver)
> 
> KMS is used, as well as UXA and DRI2 (this is enabled by default now)

Wierd I'm not seeing anything in there that would just break something 
like googleearth, and still leave compiz going, I think a git bisect on 
the drivers/gpu/drm/i915 subdir should work.

Were you running a PAE kernel before now?

Dave.

> 
> 
> Compiz used as well, but this happens without it as well.
> 
> 
> Best regards,
> 	Maxim Levitsky
> 
> 

Re: [git pull] drm: previous pull req + 1.

[Andy Lutomirski]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

Dave Airlie wrote:
> Hi Linus,
> 
> Please pull the 'drm-linus' branch from
> ssh://master.kernel.org/pub/scm/linux/kernel/git/airlied/drm-2.6.git drm-linus
> 
> This is the same tree from the previous pull request + the fix from Pierre 
> that actually makes the PAE/GEM combination on i965 work. \o/
> 

I tried this tree (specifically, a merge of Linus' fb20871 this tree) on 
Fedora 11 with modesetting enabled on an integrated Radeon 2100, and 
plymouthd crashes immediately with a corrupt page table.  Photo 
attached.  After the crash, bootup stops, although ctrl-alt-del works.

This is on a Gigabyte GA-MA74GM-S2 (RS690 chipset) with a somewhat old 
BIOS which has never had reliable graphics on X with any kernel and 
userspace I've tried, modesetting or otherwise, so this isn't a 
regression per se.  But on F11's kernel, I can at least boot to a 
console with modesetting enabled and nothing dies until I try to start 
X, and, with modesetting off on older kernels I can boot just fine 
(again, until I try to use X, at which point things break).

I'd be happy to test things to help debug.

Thanks,
Andy

[-- Attachment #2: radeon_modeset_crash.jpg --]
[-- Type: image/jpeg, Size: 278259 bytes --]

Re: [git pull] drm: previous pull req + 1.

[Dave Airlie]

> 
> I tried this tree (specifically, a merge of Linus' fb20871 this tree) on
> Fedora 11 with modesetting enabled on an integrated Radeon 2100, and plymouthd
> crashes immediately with a corrupt page table.  Photo attached.  After the
> crash, bootup stops, although ctrl-alt-del works.

You need a different userspace, -ati from koji in F11 should do it.

However we have some outstanding rs690 issues that I'm trying to track 
down as we speak,

Dave.

> 
> This is on a Gigabyte GA-MA74GM-S2 (RS690 chipset) with a somewhat old BIOS
> which has never had reliable graphics on X with any kernel and userspace I've
> tried, modesetting or otherwise, so this isn't a regression per se.  But on
> F11's kernel, I can at least boot to a console with modesetting enabled and
> nothing dies until I try to start X, and, with modesetting off on older
> kernels I can boot just fine (again, until I try to use X, at which point
> things break).
> 
> I'd be happy to test things to help debug.
> 
> Thanks,
> Andy
> 

Re: [git pull] drm: previous pull req + 1.

[Andrew Lutomirski]

On Sun, Jun 21, 2009 at 1:16 AM, Dave Airlie<airlied@linux.ie> wrote:
>>
>> I tried this tree (specifically, a merge of Linus' fb20871 this tree) on
>> Fedora 11 with modesetting enabled on an integrated Radeon 2100, and plymouthd
>> crashes immediately with a corrupt page table.  Photo attached.  After the
>> crash, bootup stops, although ctrl-alt-del works.
>
> You need a different userspace, -ati from koji in F11 should do it.

I'm already running that (xorg-x11-drv-ati-6.12.2-17.fc11.x86_64,
which is the latest in koji).

But the oops is in *plymouth*, which as far as I can tell uses the
framebuffer interface, so it shouldn't matter what xorg driver I have
installed.  Shouldn't it be impossible for a buggy framebuffer client
to cause an oops?

>
> However we have some outstanding rs690 issues that I'm trying to track
> down as we speak,

Great!  I'll keep on testing.

--Andy

>
> Dave.
>
>>
>> This is on a Gigabyte GA-MA74GM-S2 (RS690 chipset) with a somewhat old BIOS
>> which has never had reliable graphics on X with any kernel and userspace I've
>> tried, modesetting or otherwise, so this isn't a regression per se.  But on
>> F11's kernel, I can at least boot to a console with modesetting enabled and
>> nothing dies until I try to start X, and, with modesetting off on older
>> kernels I can boot just fine (again, until I try to use X, at which point
>> things break).
>>
>> I'd be happy to test things to help debug.
>>
>> Thanks,
>> Andy
>>
>

Re: [git pull] drm: previous pull req + 1.

[Maxim Levitsky]

On Sat, 2009-06-20 at 17:42 -0700, Linus Torvalds wrote:
> 
> On Sun, 21 Jun 2009, Maxim Levitsky wrote:
> > 
> > Something from this tree breaks my i965.
> > Using -git just before this was pulled,
> > 
> >  a552f0af753eb4b5bbbe9eff205fe874b04c4583 works, but using latest git
> > 
> > makes google earth stall, it doesn't update its main window. It appears
> > that openining and closing its menu, allows it to progress frame after
> > frame. No crashes hangs however.
> 
> Can you bisect? There's not a tons of commit there, so it shouldn't be 
> more than a couple of recompiles/reboots, and you'd be able to pinpoint 
> the exact commit that breaks. That will help people figure it out, or at 
> worst just pinpoint what we need to revert.
> 
> 		Linus


Here the result:


> 52dc7d32b88156248167864f77a9026abe27b432 is first bad commit
> commit 52dc7d32b88156248167864f77a9026abe27b432
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Sat Jun 6 09:46:01 2009 +0100
> 
>     drm/i915: Clear fence register on tiling stride change.
>     
>     The fence register value also depends upon the stride of the object, so we
>     need to clear the fence if that is changed as well.
>     
>     Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>     [anholt: Added 8xx and 965 paths, and renamed the confusing
>     i915_gem_object_tiling_ok function to i915_gem_object_fence_offset_ok]
>     Signed-off-by: Eric Anholt <eric@anholt.net>
> 



However I can't reproduce the situation I have earlier, maybe I have changed some settings, don't know.
Now, the bad behavior (and I reproduced it many times, is that GE shows incorrect textures 
(like they are divided in tiny interlaced rows, one row ok, other contain image from other part of world), only few textures are such
it seems logical that this is related to tiling.

Also, if I maximize it, it hangs. This seems to be a separate bug introduced by these series.

commit 43813f399c72aa22e01a680559c1cb5274bf2140 both textures and maximize broken
commit 52dc7d32b88156248167864f77a9026abe27b432, shows this incorect textures, but doesn't hang the system on maximize
commit 8c4b8c3f34de4e2da20df042bba173fe557f8b45 works just fine



Unfortunaly I have no time now to do another bisect now, I try to do such as soon as possible.

Thanks,
	Maxim Levitsky

Re: [git pull] drm: previous pull req + 1.

[Linus Torvalds]

On Sun, 21 Jun 2009, Dave Airlie wrote:
> > 
> > I tried this tree (specifically, a merge of Linus' fb20871 this tree) on
> > Fedora 11 with modesetting enabled on an integrated Radeon 2100, and plymouthd
> > crashes immediately with a corrupt page table.  Photo attached.  After the
> > crash, bootup stops, although ctrl-alt-del works.
> 
> You need a different userspace, -ati from koji in F11 should do it.

Dave - no amount of userspace differences make a corrupted page table 
acceptable. 

This needs to be fixed. No excuses. Kernel crashes are never an issue of 
"you used the wrong user space".

		Linus

Re: [git pull] drm: previous pull req + 1.

[Linus Torvalds]

On Sun, 21 Jun 2009, Linus Torvalds wrote:
> 
> Dave - no amount of userspace differences make a corrupted page table 
> acceptable. 
> 
> This needs to be fixed. No excuses. Kernel crashes are never an issue of 
> "you used the wrong user space".

So "corrupted page table" means that one of the reserved bits was set, and 
we get a page fault with the PF_RSVD bit on in the error code.

Looking at the debug output, it says

	PGD        12148a067
	PUD        12148b067
	PMD        121496067
	PTE ffffc90011780237

where the top-level entries look fine, but the PTE is total crap. It looks 
like it has filled in the page frame number with a virtual address rather 
than with an actual page

The PTE _should_ look like this:

 - bit 63: NX
 - bits 62-52: zero (available to sw, but I don't think we use them)
 - bits 51-47: zero (reserved)
 - bits 46-12: page frame
 - bits 11-0: protection and PAT bits etc (bits 8-7 are also reserved)

and that PTE clearly does not match.

Strictly speaking, that "47-bit" physical address is purely theoretical. I 
think existing CPU's are limited to 40 bits or so, so there are even more 
reserved bits. 

Anyway, here's a totally UNTESTED patch that hopefully gives a warning on 
where exactly we set the invalid bits. Andy, mind trying it out? You 
should get the warnign much earlier, and it should have a much more useful 
back-trace.

(But this is _untested_, so maybe I screwed up and it doesn't compile or 
work. The BAD_PTE_BITS mask could also be improved upon, but that mask 
should be "good enough" - it doesn't include _all_ the bits it could, 
but it certainly has enough bits set to trigger that obviously bad case).

			Linus

---
 arch/x86/include/asm/pgtable_64.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_64.h b/arch/x86/include/asm/pgtable_64.h
index c57a301..b95828e 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -49,8 +49,11 @@ static inline void native_pte_clear(struct mm_struct *mm, unsigned long addr,
 	*ptep = native_make_pte(0);
 }
 
+#define BAD_PTE_BITS (_PAGE_NX - (1ul << __PHYSICAL_MASK_SHIFT))
+
 static inline void native_set_pte(pte_t *ptep, pte_t pte)
 {
+	WARN_ON_ONCE(pte.pte & BAD_PTE_BITS);
 	*ptep = pte;
 }
 

Re: [git pull] drm: previous pull req + 1.

[Andrew Lutomirski]

[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]

On Sun, Jun 21, 2009 at 1:13 PM, Linus
Torvalds<torvalds@linux-foundation.org> wrote:
>
>
> On Sun, 21 Jun 2009, Linus Torvalds wrote:
>>
>> Dave - no amount of userspace differences make a corrupted page table
>> acceptable.
>>
>> This needs to be fixed. No excuses. Kernel crashes are never an issue of
>> "you used the wrong user space".
>
> So "corrupted page table" means that one of the reserved bits was set, and
> we get a page fault with the PF_RSVD bit on in the error code.
>
> Looking at the debug output, it says
>
>        PGD        12148a067
>        PUD        12148b067
>        PMD        121496067
>        PTE ffffc90011780237
>
> where the top-level entries look fine, but the PTE is total crap. It looks
> like it has filled in the page frame number with a virtual address rather
> than with an actual page
>
> The PTE _should_ look like this:
>
>  - bit 63: NX
>  - bits 62-52: zero (available to sw, but I don't think we use them)
>  - bits 51-47: zero (reserved)
>  - bits 46-12: page frame
>  - bits 11-0: protection and PAT bits etc (bits 8-7 are also reserved)
>
> and that PTE clearly does not match.
>
> Strictly speaking, that "47-bit" physical address is purely theoretical. I
> think existing CPU's are limited to 40 bits or so, so there are even more
> reserved bits.
>
> Anyway, here's a totally UNTESTED patch that hopefully gives a warning on
> where exactly we set the invalid bits. Andy, mind trying it out? You
> should get the warnign much earlier, and it should have a much more useful
> back-trace.

Your patch worked.  Photo attached.

--Andy

[-- Attachment #2: radeon_modeset_crash_2.jpg --]
[-- Type: image/jpeg, Size: 287242 bytes --]

Re: [git pull] drm: previous pull req + 1.

[Linus Torvalds]

On Sun, 21 Jun 2009, Andrew Lutomirski wrote:
> >
> > Anyway, here's a totally UNTESTED patch that hopefully gives a warning on
> > where exactly we set the invalid bits. Andy, mind trying it out? You
> > should get the warnign much earlier, and it should have a much more useful
> > back-trace.
> 
> Your patch worked.  Photo attached.

Ok.

So it's fb_mmap() that uses an invalid page frame number when it does the 
"io_remap_pfn_range()" thing. 

And the way it gets that page frame number is basically

 - Offset (in bytes) from start of mapping:

	off = vma->vm_pgoff << PAGE_SHIFT;
	..

 - frame buffer start address:

        /* frame buffer memory */
        start = info->fix.smem_start;
        len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.smem_len);
	..
	off += start;

 - do the remap:

	io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT, ..

and there has been no changes to this logic in drivers/video/fbmem.c 
lately.

What *has* changed is that we have a newradeon driver, and it looks like 
that new radeon driver is crap, and does this:

	info->fix.smem_start = (unsigned long)fbptr;

which is totally screwed up. It assigns a _virtual_ address to that 
"smem_start" thing, even though it should be a physical one. 

I don't know the radeon driver, so I don't know where to find the physical 
address.  It's also possible that there is no good single physical 
address, and the radeon driver should implement a "fb_mmap" function.

Does this patch make the warning and the oops at least go away? Obviously 
it won't result in a working frame buffer, but that's a separate issue

		Linus

---
 drivers/gpu/drm/radeon/radeon_fb.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index fa86d39..4aa151e 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -380,6 +380,14 @@ int radeonfb_blank(int blank, struct fb_info *info)
 	return 0;
 }
 
+/*
+ * Not yet implemented. The fix.smem_start is crap.
+ */
+static int radeonfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+	return -EINVAL;
+}
+
 static struct fb_ops radeonfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = radeonfb_check_var,
@@ -390,6 +398,7 @@ static struct fb_ops radeonfb_ops = {
 	.fb_imageblit = cfb_imageblit,
 	.fb_pan_display = radeonfb_pan_display,
 	.fb_blank = radeonfb_blank,
+	.fb_mmap = radeonfb_mmap,
 };
 
 /**

Re: [git pull] drm: previous pull req + 1.

[Andrew Lutomirski]

On Sun, Jun 21, 2009 at 3:47 PM, Linus
Torvalds<torvalds@linux-foundation.org> wrote:
>
>
> What *has* changed is that we have a newradeon driver, and it looks like
> that new radeon driver is crap, and does this:
>
>        info->fix.smem_start = (unsigned long)fbptr;
>
> which is totally screwed up. It assigns a _virtual_ address to that
> "smem_start" thing, even though it should be a physical one.
>
> I don't know the radeon driver, so I don't know where to find the physical
> address.  It's also possible that there is no good single physical
> address, and the radeon driver should implement a "fb_mmap" function.
>
> Does this patch make the warning and the oops at least go away? Obviously
> it won't result in a working frame buffer, but that's a separate issue
>

I haven't tried your patch, but I hacked up the one below instead,
which also fixes the oops.  It still doesn't boot, though -- plymouth
hangs (or otherwise dies), preventing my initramfs from finishing.
The same kernel image boots fine with radeon.modeset=0.

I wouldn't apply this without someone more familiar with the code's
ack -- I have no idea if the lifetime of the framebuffer object is
right, and there may be any number of other bugs lurking in here.  Not
to mention a missing "static."

Signed-off-by: Andy Lutomirski <luto@mit.edu>

diff --git a/drivers/gpu/drm/radeon/radeon_fb.c
b/drivers/gpu/drm/radeon/radeon_fb.c
index fa86d39..cbb330d 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -380,6 +380,12 @@ int radeonfb_blank(int blank, struct fb_info *info)
 	return 0;
 }

+int radeonfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
+{
+	struct radeon_fb_device *rfbdev = info->par;
+	return radeon_object_fbdev_mmap(rfbdev->rdev->fbdev_robj, vma);
+}
+
 static struct fb_ops radeonfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = radeonfb_check_var,
@@ -390,6 +396,7 @@ static struct fb_ops radeonfb_ops = {
 	.fb_imageblit = cfb_imageblit,
 	.fb_pan_display = radeonfb_pan_display,
 	.fb_blank = radeonfb_blank,
+	.fb_mmap = radeonfb_mmap,
 };

 /**
@@ -499,7 +506,7 @@ int radeonfb_create(struct radeon_device *rdev,

 	ret = radeon_gem_object_create(rdev, aligned_size, 0,
 				       RADEON_GEM_DOMAIN_VRAM,
-				       false, ttm_bo_type_kernel,
+				       false, ttm_bo_type_device,
 				       false, &gobj);
 	if (ret) {
 		printk(KERN_ERR "failed to allocate framebuffer\n");
@@ -547,8 +554,6 @@ int radeonfb_create(struct radeon_device *rdev,
 	info->fbops = &radeonfb_ops;
 	info->fix.line_length = fb->pitch;
 	info->screen_base = fbptr;
-	info->fix.smem_start = (unsigned long)fbptr;
-	info->fix.smem_len = size;
 	info->screen_base = fbptr;
 	info->screen_size = size;
 	info->pseudo_palette = fb->pseudo_palette;
diff --git a/drivers/gpu/drm/radeon/radeon_object.c
b/drivers/gpu/drm/radeon/radeon_object.c
index 983e8df..433e52e 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -267,7 +267,6 @@ int radeon_object_pin(struct radeon_object *robj,
uint32_t domain,
 		}
 		if (!r) {
 			robj->rdev->fbdev_info->screen_base = fbptr;
-			robj->rdev->fbdev_info->fix.smem_start = (unsigned long)fbptr;
 		}
 		mutex_unlock(&robj->rdev->fbdev_info->lock);
 	}
@@ -316,7 +315,6 @@ void radeon_object_unpin(struct radeon_object *robj)
 		}
 		if (!r) {
 			robj->rdev->fbdev_info->screen_base = fbptr;
-			robj->rdev->fbdev_info->fix.smem_start = (unsigned long)fbptr;
 		}
 		mutex_unlock(&robj->rdev->fbdev_info->lock);
 	}

Re: [git pull] drm: previous pull req + 1.

[Chris Wilson]

On Sun, 2009-06-21 at 17:47 +0300, Maxim Levitsky wrote:
> > 52dc7d32b88156248167864f77a9026abe27b432 is first bad commit
> > commit 52dc7d32b88156248167864f77a9026abe27b432
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Sat Jun 6 09:46:01 2009 +0100

The error here seems to be my presumption that only the i915 was using
fences for GPU access. (In hindsight, it seems obvious that we do not
know why the fence was allocated for the object and so if it has
outstanding rendering, we must assume that it is using a fence for a
rendering op.)

To confirm, please can you try:

diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index fd2b8bd..0735518 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2347,7 +2347,7 @@ i915_gem_object_put_fence_reg(struct
drm_gem_object *obj)
         * therefore we must wait for any outstanding access to complete
         * before clearing the fence.
         */
-       if (!IS_I965G(dev)) {
+       if (1) {
                int ret;
 
                i915_gem_object_flush_gpu_write_domain(obj);

(I'm travelling tomorrow, so if this does turn out to be the fault, I'd
appreciate it if someone wrote it up as a complete patch.)

> However I can't reproduce the situation I have earlier, maybe I have changed some settings, don't know.
> Now, the bad behavior (and I reproduced it many times, is that GE shows incorrect textures 
> (like they are divided in tiny interlaced rows, one row ok, other contain image from other part of world), only few textures are such
> it seems logical that this is related to tiling.

What you describe is exactly the artefact you would see when you access
a tiled texture using linear addressing. Pretty conclusive evidence that
we do need to flush outstanding GPU access.

> Also, if I maximize it, it hangs. This seems to be a separate bug introduced by these series.

It does seem like a separate bug. Hopefully, we can get a better handle
on it after we fix this obnoxious tiling issue.
-ickle

Re: [git pull] drm: previous pull req + 1.

[Dave Airlie]

> 
> 
> On Sun, 21 Jun 2009, Andrew Lutomirski wrote:
> > >
> > > Anyway, here's a totally UNTESTED patch that hopefully gives a warning on
> > > where exactly we set the invalid bits. Andy, mind trying it out? You
> > > should get the warnign much earlier, and it should have a much more useful
> > > back-trace.
> > 
> > Your patch worked.  Photo attached.
> 
> Ok.
> 
> So it's fb_mmap() that uses an invalid page frame number when it does the 
> "io_remap_pfn_range()" thing. 
> 
> And the way it gets that page frame number is basically
> 
>  - Offset (in bytes) from start of mapping:
> 
> 	off = vma->vm_pgoff << PAGE_SHIFT;
> 	..
> 
>  - frame buffer start address:
> 
>         /* frame buffer memory */
>         start = info->fix.smem_start;
>         len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.smem_len);
> 	..
> 	off += start;
> 
>  - do the remap:
> 
> 	io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT, ..
> 
> and there has been no changes to this logic in drivers/video/fbmem.c 
> lately.
> 
> What *has* changed is that we have a newradeon driver, and it looks like 
> that new radeon driver is crap, and does this:
> 
> 	info->fix.smem_start = (unsigned long)fbptr;
> 
> which is totally screwed up. It assigns a _virtual_ address to that 
> "smem_start" thing, even though it should be a physical one. 
> 
> I don't know the radeon driver, so I don't know where to find the physical 
> address.  It's also possible that there is no good single physical 
> address, and the radeon driver should implement a "fb_mmap" function.
> 
> Does this patch make the warning and the oops at least go away? Obviously 
> it won't result in a working frame buffer, but that's a separate issue
> 
> 		Linus

I noticed the same bogus line myself last night, I'll get Jerome to look 
at it since its his code, he was trying to be smart about how the 
radeon fbdev emulation should work, but fbdev isn't smart enough to do 
what he wants, so I've asked him to go back to the dumb pin the fbcon in 
VRAM until we can fix fbdev to do some sort of prepare/commit type hooks 
around blocks of reads/writes.

With the safe method we end up with an 8MB pinned fbcon on 32MB in some 
scenarios, which is still totally unacceptable from a user pov.

Btw this driver is under staging for a good reason, nobody claimed it was 
perfect, we just said we felt it was a good baseline to build the final 
driver on top off. Maybe I can add 
CONFIG_DONT_CC_LINUS_ON_STAGING_DRIVERS.

Dave.

 > 
> ---
>  drivers/gpu/drm/radeon/radeon_fb.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
> index fa86d39..4aa151e 100644
> --- a/drivers/gpu/drm/radeon/radeon_fb.c
> +++ b/drivers/gpu/drm/radeon/radeon_fb.c
> @@ -380,6 +380,14 @@ int radeonfb_blank(int blank, struct fb_info *info)
>  	return 0;
>  }
>  
> +/*
> + * Not yet implemented. The fix.smem_start is crap.
> + */
> +static int radeonfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> +{
> +	return -EINVAL;
> +}
> +
>  static struct fb_ops radeonfb_ops = {
>  	.owner = THIS_MODULE,
>  	.fb_check_var = radeonfb_check_var,
> @@ -390,6 +398,7 @@ static struct fb_ops radeonfb_ops = {
>  	.fb_imageblit = cfb_imageblit,
>  	.fb_pan_display = radeonfb_pan_display,
>  	.fb_blank = radeonfb_blank,
> +	.fb_mmap = radeonfb_mmap,
>  };
>  
>  /**
> 
> ------------------------------------------------------------------------------
> Are you an open source citizen? Join us for the Open Source Bridge conference!
> Portland, OR, June 17-19. Two days of sessions, one day of unconference: $250.
> Need another reason to go? 24-hour hacker lounge. Register today!
> http://ad.doubleclick.net/clk;215844324;13503038;v?http://opensourcebridge.org
> --
> _______________________________________________
> Dri-devel mailing list
> Dri-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/dri-devel
> 
> 

Re: [git pull] drm: previous pull req + 1.

[Dave Airlie]

> 
> 
> On Sun, 21 Jun 2009, Dave Airlie wrote:
> > > 
> > > I tried this tree (specifically, a merge of Linus' fb20871 this tree) on
> > > Fedora 11 with modesetting enabled on an integrated Radeon 2100, and plymouthd
> > > crashes immediately with a corrupt page table.  Photo attached.  After the
> > > crash, bootup stops, although ctrl-alt-del works.
> > 
> > You need a different userspace, -ati from koji in F11 should do it.
> 
> Dave - no amount of userspace differences make a corrupted page table 
> acceptable. 

I was original responding to a line I saw in his dmesg (which was about an 
ioctl not being available, I only got to parsing the crash later.

 
> This needs to be fixed. No excuses. Kernel crashes are never an issue of 
> "you used the wrong user space".

Agreed, and we block old userspaces running on KMS kernels (at least 
trying to use DRI) and I thought that was what he was seeing.

Dave.

> 
> 		Linus
> 
> ------------------------------------------------------------------------------
> Are you an open source citizen? Join us for the Open Source Bridge conference!
> Portland, OR, June 17-19. Two days of sessions, one day of unconference: $250.
> Need another reason to go? 24-hour hacker lounge. Register today!
> http://ad.doubleclick.net/clk;215844324;13503038;v?http://opensourcebridge.org
> --
> _______________________________________________
> Dri-devel mailing list
> Dri-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/dri-devel
> 
> 

Re: [git pull] drm: previous pull req + 1.

[Andrew Lutomirski]

On Sun, Jun 21, 2009 at 5:14 PM, Andrew Lutomirski<luto@mit.edu> wrote:
> On Sun, Jun 21, 2009 at 3:47 PM, Linus
> Torvalds<torvalds@linux-foundation.org> wrote:
>>
>>
>> What *has* changed is that we have a newradeon driver, and it looks like
>> that new radeon driver is crap, and does this:
>>
>>        info->fix.smem_start = (unsigned long)fbptr;
>>
>> which is totally screwed up. It assigns a _virtual_ address to that
>> "smem_start" thing, even though it should be a physical one.
>>
>> I don't know the radeon driver, so I don't know where to find the physical
>> address.  It's also possible that there is no good single physical
>> address, and the radeon driver should implement a "fb_mmap" function.
>>
>> Does this patch make the warning and the oops at least go away? Obviously
>> it won't result in a working frame buffer, but that's a separate issue
>>
>
> I haven't tried your patch, but I hacked up the one below instead,
> which also fixes the oops.  It still doesn't boot, though -- plymouth
> hangs (or otherwise dies), preventing my initramfs from finishing.
> The same kernel image boots fine with radeon.modeset=0.

My patch is no good -- plymouthd just dies more silently with it.
Returning -EINVAL from radeonfb_mmap prevents plymouthd's splash
screen from working (of course) but lets the system boot.  (Not only
does the system boot, but this is the first modesetting kernel I've
ever seen work on this hardware.)  Linus's patch works and even lets
me start X.

David -- there are still plenty of bugs.  The kernel gets my monitor's
resolution wrong (X reads it off DDC correctly but I have to use
xrandr to force the resolution).  And glxgears triggers the kernel's
command verification and dies.  But this is still a huge improvement
over modesetting on the F11 kernel, which is unusable for me (image is
garbled beyond recognition once I start X).

--Andy

Re: [git pull] drm: previous pull req + 1.

[Thomas Hellström]

Dave Airlie wrote:
>> On Sun, 21 Jun 2009, Andrew Lutomirski wrote:
>>     
>>>> Anyway, here's a totally UNTESTED patch that hopefully gives a warning on
>>>> where exactly we set the invalid bits. Andy, mind trying it out? You
>>>> should get the warnign much earlier, and it should have a much more useful
>>>> back-trace.
>>>>         
>>> Your patch worked.  Photo attached.
>>>       
>> Ok.
>>
>> So it's fb_mmap() that uses an invalid page frame number when it does the 
>> "io_remap_pfn_range()" thing. 
>>
>> And the way it gets that page frame number is basically
>>
>>  - Offset (in bytes) from start of mapping:
>>
>> 	off = vma->vm_pgoff << PAGE_SHIFT;
>> 	..
>>
>>  - frame buffer start address:
>>
>>         /* frame buffer memory */
>>         start = info->fix.smem_start;
>>         len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.smem_len);
>> 	..
>> 	off += start;
>>
>>  - do the remap:
>>
>> 	io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT, ..
>>
>> and there has been no changes to this logic in drivers/video/fbmem.c 
>> lately.
>>
>> What *has* changed is that we have a newradeon driver, and it looks like 
>> that new radeon driver is crap, and does this:
>>
>> 	info->fix.smem_start = (unsigned long)fbptr;
>>
>> which is totally screwed up. It assigns a _virtual_ address to that 
>> "smem_start" thing, even though it should be a physical one. 
>>
>> I don't know the radeon driver, so I don't know where to find the physical 
>> address.  It's also possible that there is no good single physical 
>> address, and the radeon driver should implement a "fb_mmap" function.
>>
>> Does this patch make the warning and the oops at least go away? Obviously 
>> it won't result in a working frame buffer, but that's a separate issue
>>
>> 		Linus
>>     
>
> I noticed the same bogus line myself last night, I'll get Jerome to look 
> at it since its his code, he was trying to be smart about how the 
> radeon fbdev emulation should work, but fbdev isn't smart enough to do 
> what he wants, so I've asked him to go back to the dumb pin the fbcon in 
> VRAM until we can fix fbdev to do some sort of prepare/commit type hooks 
> around blocks of reads/writes.
>
> With the safe method we end up with an 8MB pinned fbcon on 32MB in some 
> scenarios, which is still totally unacceptable from a user pov.
>
>   
There is a ttm_fbdev_mmap() function in TTM that may help in this 
situation. As with the standard ttm mmap it's using fault() which means 
it's possible to move out the backing buffer object if you first reserve 
it and then call unmap_mapping_range() on the relevant fbdev address 
space to kill existing user-space mappings.

We've experimented a little with this on the Poulsbo / Moorestown KMS 
driver (we threw out the fbcon buffer when the X server was switched in) 
but the problem turned out to be that fbdev always expects an always 
present _kernel_ mapping and it seemed a bit too hard to block accesses 
to that mapping while it was torn down and set up again to point to the 
new location.
In particular, the fbdev acceleration hooks seems to be called from 
atomic context.

It would be very helpful if we could introduce an fbdev mutex that 
protects fbdev accesses to the kernel map and to the fbdev acceleration 
functions.

/Thomas

Re: [git pull] drm: previous pull req + 1.

[Dave Airlie]

> >   
> There is a ttm_fbdev_mmap() function in TTM that may help in this situation.
> As with the standard ttm mmap it's using fault() which means it's possible to
> move out the backing buffer object if you first reserve it and then call
> unmap_mapping_range() on the relevant fbdev address space to kill existing
> user-space mappings.

Yup I've looked at this from the fbdev pov, however I hit the same problem
with the fbcon writes happening pretty much whenever they wanted to.

It might be possible to move the fbcon around by updating screen_base, but 
you'd need to have some sort of lock around the read/write functions, I 
think locking on each individual read/write might be a lot of overhead.

Ideallly something analogous to the X server Prepare/Finish access hooks.

Dave.

Re: [git pull] drm: previous pull req + 1.

[Maxim Levitsky]

On Sun, 2009-06-21 at 22:24 +0100, Chris Wilson wrote:
> On Sun, 2009-06-21 at 17:47 +0300, Maxim Levitsky wrote:
> > > 52dc7d32b88156248167864f77a9026abe27b432 is first bad commit
> > > commit 52dc7d32b88156248167864f77a9026abe27b432
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date:   Sat Jun 6 09:46:01 2009 +0100
> 
> The error here seems to be my presumption that only the i915 was using
> fences for GPU access. (In hindsight, it seems obvious that we do not
> know why the fence was allocated for the object and so if it has
> outstanding rendering, we must assume that it is using a fence for a
> rendering op.)
> 
> To confirm, please can you try:
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index fd2b8bd..0735518 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2347,7 +2347,7 @@ i915_gem_object_put_fence_reg(struct
> drm_gem_object *obj)
>          * therefore we must wait for any outstanding access to complete
>          * before clearing the fence.
>          */
> -       if (!IS_I965G(dev)) {
> +       if (1) {
>                 int ret;
>  
>                 i915_gem_object_flush_gpu_write_domain(obj);

Nope, same thing.

I use commit 87ef92092fd092936535ba057ee19b97bb6a709a + this patch
Note that GE doesn't hang the system when maximizing it.

It is for sure tiled textures accessed incorrectly, same behavior
observed in many games (they still run though)

Best regards,
	Maxim Levitsky

Re: [git pull] drm: previous pull req + 1.

[Linus Torvalds]

On Mon, 22 Jun 2009, Thomas Hellström wrote:
> 
> It would be very helpful if we could introduce an fbdev mutex that protects
> fbdev accesses to the kernel map and to the fbdev acceleration functions.

Not going to happen.

Why? 'printk'.

If you can't handle printk, then you're basically useless. And printk 
absolutely -has- to work in bad situations (the most important messages 
could happen in any context).

			Linus

Re: [git pull] drm: previous pull req + 1.

[Andrew Lutomirski]

2009/6/22 Linus Torvalds <torvalds@linux-foundation.org>:
>
>
> On Mon, 22 Jun 2009, Thomas Hellström wrote:
>>
>> It would be very helpful if we could introduce an fbdev mutex that protects
>> fbdev accesses to the kernel map and to the fbdev acceleration functions.
>
> Not going to happen.
>
> Why? 'printk'.
>
> If you can't handle printk, then you're basically useless. And printk
> absolutely -has- to work in bad situations (the most important messages
> could happen in any context).

What if we only guaranteed that the framebuffer is mapped when it's
showing on the screen?

printk doesn't need to write to the framebuffer immediately when X
isn't running (since the framebuffer isn't shown) and presumably the
framebuffer needs to be pinned somewhere when it's being displayed
anyway.  This would involve fbcon knowing how to buffer text to be
shown later so that printk still works in interrupt context.

--Andy

Re: [git pull] drm: previous pull req + 1.

[Arnaldo Carvalho de Melo]

Em Sun, Jun 21, 2009 at 08:05:36PM -0400, Andrew Lutomirski escreveu:
> 
> David -- there are still plenty of bugs.  The kernel gets my monitor's
> resolution wrong (X reads it off DDC correctly but I have to use
> xrandr to force the resolution).

No crashes here, but I was able to use an external Samsung SyncMaster
226BW at 1680x1050 and now this resolution doesn't even appear as one of
the possible resolutions on xrandr :-(

F11, uptodate:

xorg-x11-drv-intel-2.7.0-7.fc11.x86_64

Thinkpad T60W

00:02.0 VGA compatible controller: Intel Corporation Mobile 945GM/GMS,
943/940GML Express Integrated Graphics Controller (rev 03)
00:02.1 Display controller: Intel Corporation Mobile 945GM/GMS/GME,
943/940GML Express Integrated Graphics Controller (rev 03)

dmesg lines related: That DAC-6 line should be setting it to 1680x1050
as has been till I switched to this new kernel:

ACPI: Video Device [VID] (multi-head: yes  rom: no  post: no)
[drm] Initialized drm 1.1.0 20060810
i915 0000:00:02.0: power state changed by ACPI to D0
i915 0000:00:02.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
i915 0000:00:02.0: setting latency timer to 64
<SNIP>
allocated 1680x1050 fb: 0x007df000, bo ffff880037c3a780
Console: switching to colour frame buffer device 160x64
[drm] DAC-6: set mode 1280x1024 b <====================================
<SNIP>
[drm] LVDS-8: set mode 1680x1050 21
fb0: inteldrmfb frame buffer device
registered panic notifier
[drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0

- Arnaldo

Re: [git pull] drm: previous pull req + 1.

[Linus Torvalds]

On Mon, 22 Jun 2009, Andrew Lutomirski wrote:
> 
> What if we only guaranteed that the framebuffer is mapped when it's
> showing on the screen?

I think that works ok. We only care about printk being immediate in that 
case, and if it gets buffered I don't think we care.

> printk doesn't need to write to the framebuffer immediately when X
> isn't running (since the framebuffer isn't shown) and presumably the
> framebuffer needs to be pinned somewhere when it's being displayed
> anyway.  This would involve fbcon knowing how to buffer text to be
> shown later so that printk still works in interrupt context.

But doesn't fbcon do that _anyway_ for VC switching?

(I've tried to stay out of fbcon, and have traditionally personally always 
preferred just regular VGA text mode, so I really have no clue about the 
internals).

			Linus

Re: [git pull] drm: previous pull req + 1.

[Benjamin Herrenschmidt]

On Sun, 2009-06-21 at 12:47 -0700, Linus Torvalds wrote:
> 
> What *has* changed is that we have a newradeon driver, and it looks
> like 
> that new radeon driver is crap, and does this:
> 
>         info->fix.smem_start = (unsigned long)fbptr;
> 
> which is totally screwed up. It assigns a _virtual_ address to that 
> "smem_start" thing, even though it should be a physical one. 

BTW. While we are at it that there's an additional problem (old fbdev
design issue) with that which is smem_start is an unsigned long, which
is too small to hold a physical address on a variety of 32-bit
platforms.

Dave, what about we split those structures into a "sane" in-kernel one
and a "user visible" one ? At that point, we can create new saner
variants of the user ones using separate ioctls ...

> I don't know the radeon driver, so I don't know where to find the
> physical  address.  It's also possible that there is no good single
> physical  address, and the radeon driver should implement a "fb_mmap"
> function.
> 
> Does this patch make the warning and the oops at least go away?
> Obviously  it won't result in a working frame buffer, but that's a
> separate issue

There should be a physical address but it's nasty to let userspace map
it directly without using the DRM for access control, not sure what
David plans here, but it may need surface control set properly etc...

Dave ? Should the fbdev mmap go down the DRM object map path or
similar ?


Cheers,
Ben.

Re: [git pull] drm: previous pull req + 1.

[Benjamin Herrenschmidt]

On Mon, 2009-06-22 at 10:18 +0200, Thomas Hellström wrote:

> It would be very helpful if we could introduce an fbdev mutex that 
> protects fbdev accesses to the kernel map and to the fbdev acceleration 
> functions.

As far as I can remember, all fbdev operations are done under the
console semaphore.

(I know I did fix a bunch of that crap ages ago).

Cheers,
Ben.

Re: [git pull] drm: previous pull req + 1.

[Benjamin Herrenschmidt]

On Mon, 2009-06-22 at 11:22 -0700, Linus Torvalds wrote:
> Not going to happen.
> 
> Why? 'printk'.
> 
> If you can't handle printk, then you're basically useless. And printk 
> absolutely -has- to work in bad situations (the most important
> messages could happen in any context).

Well... yes and no. If X is frontmost, printk is not going to be
printed, ie, I'm talking about today, when the console is !KD_TEXT.

There -is- a mechanism to deal with these things today, and the console
semaphore does take care of accesses to the fb.

(That doesn't exclude having the ability to force-switch back to kernel
fb for printing things like oops btw, which KMS could do, but for basic
access control, it makes sense).

Cheers,
Ben.

Re: [git pull] drm: previous pull req + 1.

[Linus Torvalds]

On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
> 
> As far as I can remember, all fbdev operations are done under the
> console semaphore.

Yeah, and some of them are horribly broken (ie copying data from user 
space while doing it - causing horrible things like VC switching latencies 
and invisible printk's if an oops happens during the op).

Or maybe that got fixed. 

			Linus

Re: [git pull] drm: previous pull req + 1.

[Benjamin Herrenschmidt]

On Mon, 2009-06-22 at 17:24 -0700, Linus Torvalds wrote:
> 
> On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
> > 
> > As far as I can remember, all fbdev operations are done under the
> > console semaphore.
> 
> Yeah, and some of them are horribly broken (ie copying data from user 
> space while doing it - causing horrible things like VC switching latencies 
> and invisible printk's if an oops happens during the op).
> 
> Or maybe that got fixed. 

Well, it does rely on userspace behaving.. ie, no accel ops are done by
the kernel in KD_GRAPHICS and userspace is -supposed- to switch to
KD_GRAPHICS before touching the fb.

In fact, nowdays, we do have the infrastructure to be smart and enforce
that. IE. Instead of using a boring remap_page_ranges() in fb_mmap() we
could use a fault handler. When in KD_TEXT, we fail them, when in
KD_GRAPHICS, we service them, and we unmap_mapping_range() when
switching. Something like that...

Dunno how that interacts with the new DRM thingy though.

Cheers,
Ben.

Re: [git pull] drm: previous pull req + 1.

[Jesse Barnes]

On Tue, 23 Jun 2009 11:04:39 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Mon, 2009-06-22 at 17:24 -0700, Linus Torvalds wrote:
> > 
> > On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
> > > 
> > > As far as I can remember, all fbdev operations are done under the
> > > console semaphore.
> > 
> > Yeah, and some of them are horribly broken (ie copying data from
> > user space while doing it - causing horrible things like VC
> > switching latencies and invisible printk's if an oops happens
> > during the op).
> > 
> > Or maybe that got fixed. 
> 
> Well, it does rely on userspace behaving.. ie, no accel ops are done
> by the kernel in KD_GRAPHICS and userspace is -supposed- to switch to
> KD_GRAPHICS before touching the fb.
> 
> In fact, nowdays, we do have the infrastructure to be smart and
> enforce that. IE. Instead of using a boring remap_page_ranges() in
> fb_mmap() we could use a fault handler. When in KD_TEXT, we fail
> them, when in KD_GRAPHICS, we service them, and we
> unmap_mapping_range() when switching. Something like that...
> 
> Dunno how that interacts with the new DRM thingy though.

I think it could work, but ideally we'd keep the kernel fbcon object
pinned, and keep printing into it even while some other gfx app is
running.  That way we don't have to dump the whole queue into it when a
panic occurs, we can just switch buffers (something like this would
also be handy for dual head debugging; one head running your desktop
and the other a debug console printing all the messages).  That's
slightly more invasive surgery though...  I should have a chance to do
something like that as part of the kdb/kms work I'll be doing with
Jason.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [git pull] drm: previous pull req + 1.

[Benjamin Herrenschmidt]

On Mon, 2009-06-22 at 18:18 -0700, Jesse Barnes wrote:
> I think it could work, but ideally we'd keep the kernel fbcon object
> pinned, and keep printing into it even while some other gfx app is
> running.  That way we don't have to dump the whole queue into it when
> a
> panic occurs, we can just switch buffers (something like this would
> also be handy for dual head debugging; one head running your desktop
> and the other a debug console printing all the messages).  That's
> slightly more invasive surgery though...  I should have a chance to do
> something like that as part of the kdb/kms work I'll be doing with
> Jason.
> 
Do we really need that ?

We can easily repaint (ie, regenerate the fb content from the pseudo
vgacon image kept by the console layer).

So if we want the kernel to "take" over, it's reasonably easy to
make it also repaint the content of the fb.

How, of course, kicking out usespace with unmap_mapping_ranges() isn't
going to work well from an oops or something at interrupt time, we
do need to have a reasonably safe path for these things, which is why
I believe that sort of emergency printing should be done without
any acceleration, just basic manual painting in the front buffer...

Should we even bother changing the mode ? Not sure...

Cheers,
Ben.

Re: [git pull] drm: previous pull req + 1.

[Jesse Barnes]

On Tue, 23 Jun 2009 11:58:44 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Mon, 2009-06-22 at 18:18 -0700, Jesse Barnes wrote:
> > I think it could work, but ideally we'd keep the kernel fbcon object
> > pinned, and keep printing into it even while some other gfx app is
> > running.  That way we don't have to dump the whole queue into it
> > when a
> > panic occurs, we can just switch buffers (something like this would
> > also be handy for dual head debugging; one head running your desktop
> > and the other a debug console printing all the messages).  That's
> > slightly more invasive surgery though...  I should have a chance to
> > do something like that as part of the kdb/kms work I'll be doing
> > with Jason.
> > 
> Do we really need that ?
> 
> We can easily repaint (ie, regenerate the fb content from the pseudo
> vgacon image kept by the console layer).
> 
> So if we want the kernel to "take" over, it's reasonably easy to
> make it also repaint the content of the fb.

For just repainting panic messages, probably not.  For the dual head
case I talked about though it would sure be nice...

> How, of course, kicking out usespace with unmap_mapping_ranges() isn't
> going to work well from an oops or something at interrupt time, we
> do need to have a reasonably safe path for these things, which is why
> I believe that sort of emergency printing should be done without
> any acceleration, just basic manual painting in the front buffer...
> 
> Should we even bother changing the mode ? Not sure...

Yeah I don't think we should try to change the mode, unless we really
have to for whatever reason.  fbcon should generally be able to paint
to whatever we have up as long as we set it up properly.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [git pull] drm: previous pull req + 1.

[Benjamin Herrenschmidt]

On Mon, 2009-06-22 at 19:07 -0700, Jesse Barnes wrote:

> Yeah I don't think we should try to change the mode, unless we really
> have to for whatever reason.  fbcon should generally be able to paint
> to whatever we have up as long as we set it up properly.

Well... it may not. IE. The text consoles can be using a different mode
than X, and so fbcon will be all setup for that ... (including how many
lines/columns etc...).

So it's not totally trivial... but on the other hands, it would be
useful to avoid as much as possible complicated things like mode
switches when hitting an oops.

Cheers,
Ben.

Re: [git pull] drm: previous pull req + 1.

[Michel Dänzer]

On Mon, 2009-06-22 at 18:18 -0700, Jesse Barnes wrote:
> On Tue, 23 Jun 2009 11:04:39 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > On Mon, 2009-06-22 at 17:24 -0700, Linus Torvalds wrote:
> > > 
> > > On Tue, 23 Jun 2009, Benjamin Herrenschmidt wrote:
> > > > 
> > > > As far as I can remember, all fbdev operations are done under the
> > > > console semaphore.
> > > 
> > > Yeah, and some of them are horribly broken (ie copying data from
> > > user space while doing it - causing horrible things like VC
> > > switching latencies and invisible printk's if an oops happens
> > > during the op).
> > > 
> > > Or maybe that got fixed. 
> > 
> > Well, it does rely on userspace behaving.. ie, no accel ops are done
> > by the kernel in KD_GRAPHICS and userspace is -supposed- to switch to
> > KD_GRAPHICS before touching the fb.
> > 
> > In fact, nowdays, we do have the infrastructure to be smart and
> > enforce that. IE. Instead of using a boring remap_page_ranges() in
> > fb_mmap() we could use a fault handler. When in KD_TEXT, we fail
> > them, when in KD_GRAPHICS, we service them, and we
> > unmap_mapping_range() when switching. Something like that...
> > 
> > Dunno how that interacts with the new DRM thingy though.
> 
> I think it could work, but ideally we'd keep the kernel fbcon object
> pinned, and keep printing into it even while some other gfx app is
> running.

It doesn't need to be pinned for that, does it? I think in the long run
it's a bad idea to have it pinned all the time, think of machines with
only 8 MB of VRAM...


> (something like this would also be handy for dual head debugging; one
> head running your desktop and the other a debug console printing all
> the messages).

On a side note, I did precisely that about ten years ago on my Amiga. :)
Granted, that was using two separate framebuffer devices (X glint driver
on top of pm2fb, debug messages on amifb), but I think even that case
isn't possible ATM. I agree it would be nice, though realistically
there's hardly a way around a second machine for graphics driver
development anyway.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [git pull] drm: previous pull req + 1.

[Jesse Barnes]

On Tue, 23 Jun 2009 09:48:00 +0200
Michel Dänzer <michel@daenzer.net> wrote:
> > > In fact, nowdays, we do have the infrastructure to be smart and
> > > enforce that. IE. Instead of using a boring remap_page_ranges() in
> > > fb_mmap() we could use a fault handler. When in KD_TEXT, we fail
> > > them, when in KD_GRAPHICS, we service them, and we
> > > unmap_mapping_range() when switching. Something like that...
> > > 
> > > Dunno how that interacts with the new DRM thingy though.
> > 
> > I think it could work, but ideally we'd keep the kernel fbcon object
> > pinned, and keep printing into it even while some other gfx app is
> > running.
> 
> It doesn't need to be pinned for that, does it? I think in the long
> run it's a bad idea to have it pinned all the time, think of machines
> with only 8 MB of VRAM...

Yeah, in the general case we shouldn't have to pin it...

> > (something like this would also be handy for dual head debugging;
> > one head running your desktop and the other a debug console
> > printing all the messages).
> 
> On a side note, I did precisely that about ten years ago on my
> Amiga. :) Granted, that was using two separate framebuffer devices (X
> glint driver on top of pm2fb, debug messages on amifb), but I think
> even that case isn't possible ATM. I agree it would be nice, though
> realistically there's hardly a way around a second machine for
> graphics driver development anyway.

Oh I know, most operating systems have reasonable debugging facilities,
and have had for a long time.  Linux is the one lagging here.

I agree it probably won't be that helpful for low level gfx debugging,
but I'm trying to think beyond that too. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [git pull] drm: previous pull req + 1.

[Jesse Barnes]

On Tue, 23 Jun 2009 12:26:15 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Mon, 2009-06-22 at 19:07 -0700, Jesse Barnes wrote:
> 
> > Yeah I don't think we should try to change the mode, unless we
> > really have to for whatever reason.  fbcon should generally be able
> > to paint to whatever we have up as long as we set it up properly.
> 
> Well... it may not. IE. The text consoles can be using a different
> mode than X, and so fbcon will be all setup for that ... (including
> how many lines/columns etc...).

Yeah, I was suggesting we change fbcon's view of the world at panic
time, rather than trying to change the mode.  It's a bit of surgery
though...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [git pull] drm: previous pull req + 1.

[Jesse Barnes]

On Tue, 23 Jun 2009 08:39:44 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 23 Jun 2009 09:48:00 +0200
> Michel Dänzer <michel@daenzer.net> wrote:
> > On a side note, I did precisely that about ten years ago on my
> > Amiga. :) Granted, that was using two separate framebuffer devices
> > (X glint driver on top of pm2fb, debug messages on amifb), but I
> > think even that case isn't possible ATM. I agree it would be nice,
> > though realistically there's hardly a way around a second machine
> > for graphics driver development anyway.
> 
> Oh I know, most operating systems have reasonable debugging
> facilities, and have had for a long time.  Linux is the one lagging
> here.
> 
Heh, I saw "amiga" and stopped reading.  So it seems Linux on x86 is
the main laggard here...  I know PPC has had interesting debug features
for awhile now, and most arches have had kgdb for a long time.  Anyway,
there's plenty of work to do in this area to give Linux a decent system
level debugger...

--
Jesse Barnes, Intel Open Source Technology Center

Re: [git pull] drm: previous pull req + 1.

[Chris Wilson]

On Mon, 2009-06-22 at 21:09 +0300, Maxim Levitsky wrote:
> Nope, same thing.
> 
> I use commit 87ef92092fd092936535ba057ee19b97bb6a709a + this patch
> Note that GE doesn't hang the system when maximizing it.
> 
> It is for sure tiled textures accessed incorrectly, same behavior
> observed in many games (they still run though)

Sorry for the delay, I was travelling last week and was sure I had
nailed the problem. Aside from the missing flush on i965 when a batch
buffer might be using fenced commands, the only other issue I found was
that we did not zap the mapping range on clear - which could also cause
tiling errors if textures were being written via a GTT mmap.

So please can you try this patch:

>From 20b7c9322914213bb589035afbbc03faf1ae3bf0 Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris@chris-wilson.co.uk>
Date: Mon, 29 Jun 2009 08:45:31 +0100
Subject: [PATCH] drm/i915: Remove mappings on clearing fence register

As the fence register is not locked whilst the user has mmaped the buffer
through the GTT, in order for the buffer to reacquire a fence register we
need to cause a fresh page-fault on the next user access. In order to
cause the page fault, we zap the current mapping on clearing the register.
We also ensure that all potential outstanding access via the fence
register is flushed before release as well.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   41 ++++++++++++++++++--------------------
 1 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 685a876..6dc74c8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1946,12 +1946,6 @@ i915_gem_object_unbind(struct drm_gem_object *obj)
 		obj_priv->agp_mem = NULL;
 	}
 
-
-	/* blow away mappings if mapped through GTT */
-	if (obj_priv->mmap_offset && dev->dev_mapping)
-		unmap_mapping_range(dev->dev_mapping,
-				    obj_priv->mmap_offset, obj->size, 1);
-
 	i915_gem_object_put_pages(obj);
 	BUG_ON(obj_priv->pages);
 
@@ -2350,8 +2344,7 @@ try_again:
 			if (old_obj_priv->pin_count)
 				continue;
 
-			/* i915 uses fences for GPU access to tiled buffers */
-			if (IS_I965G(dev) || !old_obj_priv->active)
+			if (!old_obj_priv->active)
 				break;
 
 			/* find the seqno of the first available fence */
@@ -2440,6 +2433,8 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
 		 obj_priv->gtt_offset, obj->size);
 #endif
 
+	BUG_ON(obj_priv->active);
+
 	if (IS_I965G(dev))
 		I915_WRITE64(FENCE_REG_965_0 + (obj_priv->fence_reg * 8), 0);
 	else {
@@ -2456,6 +2451,11 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
 
 	dev_priv->fence_regs[obj_priv->fence_reg].obj = NULL;
 	obj_priv->fence_reg = I915_FENCE_REG_NONE;
+
+	/* blow away mappings if mapped through GTT */
+	if (obj_priv->mmap_offset && dev->dev_mapping)
+		unmap_mapping_range(dev->dev_mapping,
+				    obj_priv->mmap_offset, obj->size, 1);
 }
 
 /**
@@ -2469,27 +2469,24 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
 int
 i915_gem_object_put_fence_reg(struct drm_gem_object *obj)
 {
-	struct drm_device *dev = obj->dev;
 	struct drm_i915_gem_object *obj_priv = obj->driver_private;
+	int ret;
 
 	if (obj_priv->fence_reg == I915_FENCE_REG_NONE)
 		return 0;
 
-	/* On the i915, GPU access to tiled buffers is via a fence,
-	 * therefore we must wait for any outstanding access to complete
-	 * before clearing the fence.
+	/* If there is outstanding activity on the buffer whilst it holds
+	 * a fence register we must assume that it requires that fence for
+	 * correct operation. Therefore we must wait for any outstanding
+	 * access to complete before clearing the fence.
 	 */
-	if (!IS_I965G(dev)) {
-		int ret;
-
-		i915_gem_object_flush_gpu_write_domain(obj);
-		i915_gem_object_flush_gtt_write_domain(obj);
-		ret = i915_gem_object_wait_rendering(obj);
-		if (ret != 0)
-			return ret;
-	}
+	i915_gem_object_flush_gpu_write_domain(obj);
+	i915_gem_object_flush_gtt_write_domain(obj);
+	ret = i915_gem_object_wait_rendering(obj);
+	if (ret != 0)
+		return ret;
 
-	i915_gem_clear_fence_reg (obj);
+	i915_gem_clear_fence_reg(obj);
 
 	return 0;
 }
-- 
1.6.3.1

Re: [git pull] drm: previous pull req + 1.

[Chris Wilson]

Revised patch, unmap_mapping_range() on unbind and clear register.

>From 8f13b6389ee0c8a39a2073279928a3a228bd27dc Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris@chris-wilson.co.uk>
Date: Mon, 29 Jun 2009 08:45:31 +0100
Subject: [PATCH] drm/i915: Remove mappings on clearing fence register

As the fence register is not locked whilst the user has mmaped the buffer
through the GTT, in order for the buffer to reacquire a fence register we
need to cause a fresh page-fault on the next user access. In order to
cause the page fault, we zap the current mapping on clearing the register.
We also ensure that all potential outstanding access via the fence
register is flushed before release as well.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   35 +++++++++++++++++++----------------
 1 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 685a876..7fb636b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1946,8 +1946,7 @@ i915_gem_object_unbind(struct drm_gem_object *obj)
 		obj_priv->agp_mem = NULL;
 	}
 
-
-	/* blow away mappings if mapped through GTT */
+	/* Force the next mmap access to trigger a fault and rebind */
 	if (obj_priv->mmap_offset && dev->dev_mapping)
 		unmap_mapping_range(dev->dev_mapping,
 				    obj_priv->mmap_offset, obj->size, 1);
@@ -2350,8 +2349,7 @@ try_again:
 			if (old_obj_priv->pin_count)
 				continue;
 
-			/* i915 uses fences for GPU access to tiled buffers */
-			if (IS_I965G(dev) || !old_obj_priv->active)
+			if (!old_obj_priv->active)
 				break;
 
 			/* find the seqno of the first available fence */
@@ -2440,6 +2438,8 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
 		 obj_priv->gtt_offset, obj->size);
 #endif
 
+	BUG_ON(obj_priv->active);
+
 	if (IS_I965G(dev))
 		I915_WRITE64(FENCE_REG_965_0 + (obj_priv->fence_reg * 8), 0);
 	else {
@@ -2471,25 +2471,28 @@ i915_gem_object_put_fence_reg(struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
 	struct drm_i915_gem_object *obj_priv = obj->driver_private;
+	int ret;
 
 	if (obj_priv->fence_reg == I915_FENCE_REG_NONE)
 		return 0;
 
-	/* On the i915, GPU access to tiled buffers is via a fence,
-	 * therefore we must wait for any outstanding access to complete
-	 * before clearing the fence.
+	/* If there is outstanding activity on the buffer whilst it holds
+	 * a fence register we must assume that it requires that fence for
+	 * correct operation. Therefore we must wait for any outstanding
+	 * access to complete before clearing the fence.
 	 */
-	if (!IS_I965G(dev)) {
-		int ret;
+	i915_gem_object_flush_gpu_write_domain(obj);
+	i915_gem_object_flush_gtt_write_domain(obj);
+	ret = i915_gem_object_wait_rendering(obj);
+	if (ret != 0)
+		return ret;
 
-		i915_gem_object_flush_gpu_write_domain(obj);
-		i915_gem_object_flush_gtt_write_domain(obj);
-		ret = i915_gem_object_wait_rendering(obj);
-		if (ret != 0)
-			return ret;
-	}
+	i915_gem_clear_fence_reg(obj);
 
-	i915_gem_clear_fence_reg (obj);
+	/* Reacquire fence register on next mmap access (via page fault) */
+	if (obj_priv->mmap_offset)
+		unmap_mapping_range(dev->dev_mapping,
+				    obj_priv->mmap_offset, obj->size, 1);
 
 	return 0;
 }
-- 
1.6.3.3

Re: [git pull] drm: previous pull req + 1.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 4299 bytes --]

On Mon, 2009-06-29 at 08:57 +0100, Chris Wilson wrote:
> On Mon, 2009-06-22 at 21:09 +0300, Maxim Levitsky wrote:
> > Nope, same thing.
> > 
> > I use commit 87ef92092fd092936535ba057ee19b97bb6a709a + this patch
> > Note that GE doesn't hang the system when maximizing it.
> > 
> > It is for sure tiled textures accessed incorrectly, same behavior
> > observed in many games (they still run though)
> 
> Sorry for the delay, I was travelling last week and was sure I had
> nailed the problem. Aside from the missing flush on i965 when a batch
> buffer might be using fenced commands, the only other issue I found was
> that we did not zap the mapping range on clear - which could also cause
> tiling errors if textures were being written via a GTT mmap.
> 
> So please can you try this patch:
> 
> >From 20b7c9322914213bb589035afbbc03faf1ae3bf0 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Mon, 29 Jun 2009 08:45:31 +0100
> Subject: [PATCH] drm/i915: Remove mappings on clearing fence register
> 
> As the fence register is not locked whilst the user has mmaped the buffer
> through the GTT, in order for the buffer to reacquire a fence register we
> need to cause a fresh page-fault on the next user access. In order to
> cause the page fault, we zap the current mapping on clearing the register.
> We also ensure that all potential outstanding access via the fence
> register is flushed before release as well.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   41 ++++++++++++++++++--------------------
>  1 files changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 685a876..6dc74c8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1946,12 +1946,6 @@ i915_gem_object_unbind(struct drm_gem_object *obj)
>  		obj_priv->agp_mem = NULL;
>  	}
>  
> -
> -	/* blow away mappings if mapped through GTT */
> -	if (obj_priv->mmap_offset && dev->dev_mapping)
> -		unmap_mapping_range(dev->dev_mapping,
> -				    obj_priv->mmap_offset, obj->size, 1);
> -

Err, so now untiled things wouldn't fault to rebind?  Seems wrong.


> @@ -2456,6 +2451,11 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
>  
>  	dev_priv->fence_regs[obj_priv->fence_reg].obj = NULL;
>  	obj_priv->fence_reg = I915_FENCE_REG_NONE;
> +
> +	/* blow away mappings if mapped through GTT */
> +	if (obj_priv->mmap_offset && dev->dev_mapping)
> +		unmap_mapping_range(dev->dev_mapping,
> +				    obj_priv->mmap_offset, obj->size, 1);
>  }

This part seems good.

>  
>  /**
> @@ -2469,27 +2469,24 @@ i915_gem_clear_fence_reg(struct drm_gem_object *obj)
>  int
>  i915_gem_object_put_fence_reg(struct drm_gem_object *obj)
>  {
> -	struct drm_device *dev = obj->dev;
>  	struct drm_i915_gem_object *obj_priv = obj->driver_private;
> +	int ret;
>  
>  	if (obj_priv->fence_reg == I915_FENCE_REG_NONE)
>  		return 0;
>  
> -	/* On the i915, GPU access to tiled buffers is via a fence,
> -	 * therefore we must wait for any outstanding access to complete
> -	 * before clearing the fence.
> +	/* If there is outstanding activity on the buffer whilst it holds
> +	 * a fence register we must assume that it requires that fence for
> +	 * correct operation. Therefore we must wait for any outstanding
> +	 * access to complete before clearing the fence.
>  	 */
> -	if (!IS_I965G(dev)) {
> -		int ret;
> -
> -		i915_gem_object_flush_gpu_write_domain(obj);
> -		i915_gem_object_flush_gtt_write_domain(obj);
> -		ret = i915_gem_object_wait_rendering(obj);
> -		if (ret != 0)
> -			return ret;
> -	}
> +	i915_gem_object_flush_gpu_write_domain(obj);
> +	i915_gem_object_flush_gtt_write_domain(obj);
> +	ret = i915_gem_object_wait_rendering(obj);
> +	if (ret != 0)
> +		return ret;
>  
> -	i915_gem_clear_fence_reg (obj);
> +	i915_gem_clear_fence_reg(obj);
>  
>  	return 0;
>  }

This part doesn't make sense to me.  There should be nothing in the 965
rendering path that's using fences.  Did you identify something that
was?

-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]