LinuxLists.cc - [git pull] drm: previous pull req + 1.

[permalink] [raw]

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

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

2009-06-21 01:33:34

[permalink] [raw]

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

> >
> > 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
>
>

2009-06-21 03:49:26

[permalink] [raw]

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

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

Attachments:

radeon_modeset_crash.jpg (271.74 kB)

2009-06-21 05:16:55

[permalink] [raw]

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

>
> 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
>

2009-06-21 12:06:12

[permalink] [raw]

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

On Sun, Jun 21, 2009 at 1:16 AM, Dave Airlie<[email protected]> 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
>>
>

2009-06-21 14:47:50

by Maxim Levitsky

[permalink] [raw]

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

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 <[email protected]>
> 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 <[email protected]>
> [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 <[email protected]>
>

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

2009-06-21 16:47:28

[permalink] [raw]

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

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

2009-06-21 17:13:44

[permalink] [raw]

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

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;
}

2009-06-21 18:50:39

[permalink] [raw]

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

On Sun, Jun 21, 2009 at 1:13 PM, Linus
Torvalds<[email protected]> 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

Attachments:

radeon_modeset_crash_2.jpg (280.51 kB)

2009-06-21 19:48:00

[permalink] [raw]

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

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,
};

/**

2009-06-21 21:15:03

[permalink] [raw]

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

On Sun, Jun 21, 2009 at 3:47 PM, Linus
Torvalds<[email protected]> 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 <[email protected]>

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);
}

2009-06-21 21:39:41

by Chris Wilson

[permalink] [raw]

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

On Sun, 2009-06-21 at 17:47 +0300, Maxim Levitsky wrote:
> > 52dc7d32b88156248167864f77a9026abe27b432 is first bad commit
> > commit 52dc7d32b88156248167864f77a9026abe27b432
> > Author: Chris Wilson <[email protected]>
> > 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

2009-06-21 22:40:26

[permalink] [raw]

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

>
>
> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/dri-devel
>
>

2009-06-21 22:41:53

[permalink] [raw]

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

>
>
> 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/dri-devel
>
>

2009-06-22 00:05:44

[permalink] [raw]

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

On Sun, Jun 21, 2009 at 5:14 PM, Andrew Lutomirski<[email protected]> wrote:
> On Sun, Jun 21, 2009 at 3:47 PM, Linus
> Torvalds<[email protected]> 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

2009-06-22 08:30:35

[permalink] [raw]

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

> >
> 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.

2009-06-22 08:35:11

by Thomas Hellstrom

[permalink] [raw]

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

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

2009-06-22 18:09:43

by Maxim Levitsky

[permalink] [raw]

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

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 <[email protected]>
> > > 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

2009-06-22 18:25:56

[permalink] [raw]

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

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

2009-06-22 18:59:20

[permalink] [raw]

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

2009/6/22 Linus Torvalds <[email protected]>:
>
>
> 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

2009-06-22 19:44:27

[permalink] [raw]

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

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

2009-06-22 20:26:48

by Arnaldo Carvalho de Melo

[permalink] [raw]

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

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

2009-06-22 23:58:34

[permalink] [raw]

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

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.

2009-06-23 00:08:00

[permalink] [raw]

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

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.

2009-06-23 00:25:30

[permalink] [raw]

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

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

2009-06-23 00:26:01

[permalink] [raw]

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

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.

2009-06-23 01:05:23

[permalink] [raw]

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

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.

2009-06-23 01:19:18

[permalink] [raw]

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

On Tue, 23 Jun 2009 11:04:39 +1000
Benjamin Herrenschmidt <[email protected]> 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

2009-06-23 01:59:28

[permalink] [raw]

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

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.

2009-06-23 02:07:28

[permalink] [raw]

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

On Tue, 23 Jun 2009 11:58:44 +1000
Benjamin Herrenschmidt <[email protected]> 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

2009-06-23 02:26:53

[permalink] [raw]

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

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.

2009-06-23 07:57:43

by Michel Dänzer

[permalink] [raw]

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

On Mon, 2009-06-22 at 18:18 -0700, Jesse Barnes wrote:
> On Tue, 23 Jun 2009 11:04:39 +1000
> Benjamin Herrenschmidt <[email protected]> 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

2009-06-23 15:39:58

[permalink] [raw]

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

On Tue, 23 Jun 2009 09:48:00 +0200
Michel Dänzer <[email protected]> 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

2009-06-23 15:41:11

[permalink] [raw]

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

On Tue, 23 Jun 2009 12:26:15 +1000
Benjamin Herrenschmidt <[email protected]> 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

2009-06-23 16:28:24