Major 2.6.38 regression ignored?

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

Quoted 7 lines

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

See the bugzilla entry for the bisection history.

~r.

It is on the list of known regressions from 2.6.37, but we're not tracking them any more now that 2.6.39 is out.

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

On Fri, May 20, 2011 at 10:06 AM, Luke-Jr <[email protected]> wrote:

Quoted 6 lines

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

See the bugzilla entry for the bisection history.

Quoted 10 lines · Ray Lee

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

Quoted 5 lines · Luke-Jr

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

Quoted 1 line

See the bugzilla entry for the bisection history.

Which has nothing to do with Luke's bug. Considering the thousand things that can go wrong during X starting, without a hint as to which it is nigh on impossible to debug except by trial and error. If you set up netconsole, does the kernel emit an OOPS with it's last dying breath?

Quoted 11 lines · Chris Wilson

Quoted 10 lines · Ray Lee

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

Quoted 5 lines · Luke-Jr

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

Quoted 6 lines

Quoted 1 line

See the bugzilla entry for the bisection history.

Which has nothing to do with Luke's bug. Considering the thousand things that can go wrong during X starting, without a hint as to which it is nigh on impossible to debug except by trial and error. If you set up netconsole, does the kernel emit an OOPS with it's last dying breath?

Why assume it's a different bug? I would almost wonder if it might affect all Sandy Bridge GPUs. In any case, I no longer have the original motherboard (it was recalled, as I said in the first post), nor even the revision of it (it had other issues that weren't being fixed). I *assume* I will have the same problem with my new motherboard (Intel DQ67SW), but I haven't verified that yet. I'll be sure to try a netconsole when I have to reboot next and get a chance to try the most recent 2.6.38 and .39 kernels, but at the moment it seems reasonable to address the problem bisected in the bug, even if it turns out to be different.

The bisection is into an old DRI1 bug on 945GM. That DRI has inadequate locking between release and IRQ and so is prone to such races as befell Kirill should not surprise anyone. As neither UMS nor DRI supported SNB, I can quite confidently state they are separate bugs.

Quoted 19 lines · Luke-Jr

Quoted 11 lines · Chris Wilson

Quoted 10 lines · Ray Lee

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

Quoted 5 lines · Luke-Jr

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

Quoted 6 lines

Quoted 1 line

See the bugzilla entry for the bisection history.

Which has nothing to do with Luke's bug. Considering the thousand things that can go wrong during X starting, without a hint as to which it is nigh on impossible to debug except by trial and error. If you set up netconsole, does the kernel emit an OOPS with it's last dying breath?

Quoted 9 lines

Why assume it's a different bug? I would almost wonder if it might affect all Sandy Bridge GPUs. In any case, I no longer have the original motherboard (it was recalled, as I said in the first post), nor even the revision of it (it had other issues that weren't being fixed). I *assume* I will have the same problem with my new motherboard (Intel DQ67SW), but I haven't verified that yet. I'll be sure to try a netconsole when I have to reboot next and get a chance to try the most recent 2.6.38 and .39 kernels, but at the moment it seems reasonable to address the problem bisected in the bug, even if it turns out to be different.

The bisection is into an old DRI1 bug on 945GM. That DRI has inadequate locking between release and IRQ and so is prone to such races as befell Kirill should not surprise anyone. As neither UMS nor DRI supported SNB, I can quite confidently state they are separate bugs.
-Chris

Hello Chris, everyone,

Quoted 30 lines · Chris Wilson

Quoted 19 lines · Luke-Jr

Quoted 11 lines · Chris Wilson

Quoted 10 lines · Ray Lee

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

Quoted 5 lines · Luke-Jr

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

Quoted 6 lines

Quoted 1 line

See the bugzilla entry for the bisection history.

Which has nothing to do with Luke's bug. Considering the thousand things that can go wrong during X starting, without a hint as to which it is nigh on impossible to debug except by trial and error. If you set up netconsole, does the kernel emit an OOPS with it's last dying breath?

Quoted 9 lines

Why assume it's a different bug? I would almost wonder if it might affect all Sandy Bridge GPUs. In any case, I no longer have the original motherboard (it was recalled, as I said in the first post), nor even the revision of it (it had other issues that weren't being fixed). I *assume* I will have the same problem with my new motherboard (Intel DQ67SW), but I haven't verified that yet. I'll be sure to try a netconsole when I have to reboot next and get a chance to try the most recent 2.6.38 and .39 kernels, but at the moment it seems reasonable to address the problem bisected in the bug, even if it turns out to be different.

Quoted 5 lines

The bisection is into an old DRI1 bug on 945GM. That DRI has inadequate locking between release and IRQ and so is prone to such races as befell Kirill should not surprise anyone. As neither UMS nor DRI supported SNB, I can quite confidently state they are separate bugs.
-Chris

I see DRI1 is maybe buggy and old, but still, pre-kms X used to work ok on kernels < 2.6.38, and starting from 2.6.38 the system is just unusable because X either crashes the kernel (2.6.38), or does not start at all (2.6.39):

https://bugzilla.kernel.org/show_bug.cgi?id=36052

It's a regression. It's blocking me to upgrade to newer kernels. I've done my homework -- digged it and came with detailed OOPS on netconsole and bisected to single commit. Could this please be fixed?

Quoted 44 lines · Kirill Smelkov

Hello Chris, everyone,

Quoted 30 lines · Chris Wilson

Quoted 19 lines · Luke-Jr

Quoted 11 lines · Chris Wilson

Quoted 10 lines · Ray Lee

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

Quoted 5 lines · Luke-Jr

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

Quoted 6 lines

Quoted 1 line

See the bugzilla entry for the bisection history.

Which has nothing to do with Luke's bug. Considering the thousand things that can go wrong during X starting, without a hint as to which it is nigh on impossible to debug except by trial and error. If you set up netconsole, does the kernel emit an OOPS with it's last dying breath?

Quoted 9 lines

Why assume it's a different bug? I would almost wonder if it might affect all Sandy Bridge GPUs. In any case, I no longer have the original motherboard (it was recalled, as I said in the first post), nor even the revision of it (it had other issues that weren't being fixed). I *assume* I will have the same problem with my new motherboard (Intel DQ67SW), but I haven't verified that yet. I'll be sure to try a netconsole when I have to reboot next and get a chance to try the most recent 2.6.38 and .39 kernels, but at the moment it seems reasonable to address the problem bisected in the bug, even if it turns out to be different.

Quoted 5 lines

The bisection is into an old DRI1 bug on 945GM. That DRI has inadequate locking between release and IRQ and so is prone to such races as befell Kirill should not surprise anyone. As neither UMS nor DRI supported SNB, I can quite confidently state they are separate bugs.
-Chris

I see DRI1 is maybe buggy and old, but still, pre-kms X used to work ok on kernels < 2.6.38, and starting from 2.6.38 the system is just unusable because X either crashes the kernel (2.6.38), or does not start at all (2.6.39):

Quoted 6 lines

https://bugzilla.kernel.org/show_bug.cgi?id=36052

It's a regression. It's blocking me to upgrade to newer kernels. I've done my homework -- digged it and came with detailed OOPS on netconsole and bisected to single commit. Could this please be fixed?

Silence...

Still, reverting the bisected patch helps even for 3.0:

https://bugzilla.kernel.org/show_bug.cgi?id=36052#c4

Quoted 54 lines · Kirill Smelkov

Quoted 44 lines · Kirill Smelkov

Hello Chris, everyone,

Quoted 30 lines · Chris Wilson

Quoted 19 lines · Luke-Jr

Quoted 11 lines · Chris Wilson

Quoted 10 lines · Ray Lee

[ Adding Chris Wilson (author of the problematic patch) and Rafael Wysocki to the message ]

Quoted 5 lines · Luke-Jr

I submitted https://bugzilla.kernel.org/show_bug.cgi?id=33662 a month ago against 2.6.38. Now 2.6.39 was just released without the regression being addressed. This bug makes the system unusable... Some guys on IRC suggested I
email, so here it is.

Quoted 6 lines

Quoted 1 line

See the bugzilla entry for the bisection history.

Which has nothing to do with Luke's bug. Considering the thousand things that can go wrong during X starting, without a hint as to which it is nigh on impossible to debug except by trial and error. If you set up netconsole, does the kernel emit an OOPS with it's last dying breath?

Quoted 9 lines

Why assume it's a different bug? I would almost wonder if it might affect all Sandy Bridge GPUs. In any case, I no longer have the original motherboard (it was recalled, as I said in the first post), nor even the revision of it (it had other issues that weren't being fixed). I *assume* I will have the same problem with my new motherboard (Intel DQ67SW), but I haven't verified that yet. I'll be sure to try a netconsole when I have to reboot next and get a chance to try the most recent 2.6.38 and .39 kernels, but at the moment it seems reasonable to address the problem bisected in the bug, even if it turns out to be different.

Quoted 5 lines

The bisection is into an old DRI1 bug on 945GM. That DRI has inadequate locking between release and IRQ and so is prone to such races as befell Kirill should not surprise anyone. As neither UMS nor DRI supported SNB, I can quite confidently state they are separate bugs.
-Chris

I see DRI1 is maybe buggy and old, but still, pre-kms X used to work ok on kernels < 2.6.38, and starting from 2.6.38 the system is just unusable because X either crashes the kernel (2.6.38), or does not start at all (2.6.39):

Quoted 6 lines

https://bugzilla.kernel.org/show_bug.cgi?id=36052

It's a regression. It's blocking me to upgrade to newer kernels. I've done my homework -- digged it and came with detailed OOPS on netconsole and bisected to single commit. Could this please be fixed?

Silence...

Quoted 3 lines

Still, reverting the bisected patch helps even for 3.0:

https://bugzilla.kernel.org/show_bug.cgi?id=36052#c4

Keith, Chris, what's up with this regression from 2.6.38? It seems commit e8616b6 ("drm/i915: Initialise ring vfuncs for old DRI paths") caused problems on other machines.

Or would it be more fair to say that UMS is not supported anymore, is broken and just remove support for it?

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first bad io access the system freezes completely:

Quoted 3 lines · Kirill Smelkov

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001
From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in
intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_dma.c +2 -4

index 1271282..8a3942c 100644
@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct drm_device *dev)
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct drm_device *dev)
 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 	}
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah->vaddr;
 
-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem *)dev_priv->status_page_dmah->vaddr,
+		  0, PAGE_SIZE);
 
 	i915_write_hws_pga(dev);
 

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index e961568..47b9b27 100644

What kind of a workaround are you talking about?

Sorry, to me it all looked like "UMS is being ignored forever".

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Quoted 1 line · Kirill Smelkov

What kind of a workaround are you talking about?

Just reverting the commit -- that makes your machine work, even if it's wrong for other machines.

Quoted 1 line

Sorry, to me it all looked like "UMS is being ignored forever".

You're right, of course -- UMS is a huge wart on the kernel driver at this point, keeping it working while also adding new functionality continues to cause challenges. We tend to expect that most people will run reasonably contemporaneous kernel and user space code, and so three years after the switch, it continues to surprise us when someone actually tries UMS.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Let me know; I've pushed this patch to my drm-intel-fixes tree on kernel.org in the meantime; if it does solve the problem, I'd like to add your Tested-by: line.

Quoted 6 lines · Keith Packard

You're right, of course -- UMS is a huge wart on the kernel driver at this point, keeping it working while also adding new functionality continues to cause challenges. We tend to expect that most people will run reasonably contemporaneous kernel and user space code, and so three years after the switch, it continues to surprise us when someone actually tries UMS.

We are planning upgrade to KMS too. The kernel is upgraded more often compared to userspace, because of already mentioned (thanks!) "no regression" rule. Userspace is more complex and more work in my context, so it is lagging, but eventually we'll get there.

Quoted 9 lines · Kirill Smelkov

Quoted 6 lines · Keith Packard

You're right, of course -- UMS is a huge wart on the kernel driver at this point, keeping it working while also adding new functionality continues to cause challenges. We tend to expect that most people will run reasonably contemporaneous kernel and user space code, and so three years after the switch, it continues to surprise us when someone actually tries UMS.

Quoted 4 lines

We are planning upgrade to KMS too. The kernel is upgraded more often compared to userspace, because of already mentioned (thanks!) "no regression" rule. Userspace is more complex and more work in my context, so it is lagging, but eventually we'll get there.

Also wanted to say, that if whole X could be built, like the kernel, from one repo without multirepo-setup tool, with 100% reliable working incremental rebuild, etc... it would be a bit easier to upgrade X too.

Sorry for being a bit offtopic, could not resist. I was keeping that though in my head for ~ 2 years already, and now had a chance to mention it.

Quoted 19 lines · Kirill Smelkov

Quoted 9 lines · Kirill Smelkov

Quoted 6 lines · Keith Packard

You're right, of course -- UMS is a huge wart on the kernel driver at this point, keeping it working while also adding new functionality continues to cause challenges. We tend to expect that most people will run reasonably contemporaneous kernel and user space code, and so three years after the switch, it continues to surprise us when someone actually tries UMS.

Quoted 4 lines

We are planning upgrade to KMS too. The kernel is upgraded more often compared to userspace, because of already mentioned (thanks!) "no regression" rule. Userspace is more complex and more work in my context, so it is lagging, but eventually we'll get there.

Also wanted to say, that if whole X could be built, like the kernel, from one repo without multirepo-setup tool, with 100% reliable working incremental rebuild, etc... it would be a bit easier to upgrade X too.

Quoted 2 lines

Sorry for being a bit offtopic, could not resist. I was keeping that though in my head for ~ 2 years already, and now had a chance to mention it.

You don't have to rebuild all of X to use KMS. In most cases, you just need to update the ddx for your card.

Quoted 1 line

Sorry, to me it all looked like "UMS is being ignored forever".

You're right, of course -- UMS is a huge wart on the kernel driver at this point, keeping it working while also adding new functionality continues to cause challenges. We tend to expect that most people will run reasonably contemporaneous kernel and user space code, and so three years after the switch, it continues to surprise us when someone actually tries UMS.

I know I sound like a broken record but I really wish you i915 devs were little more eager to revert broken patches early rather than late. I mean, this particular breakage was already bisected but nobody said or did anything - and it's not like it's the first time either!

I suppose I need to bribe Linus somehow to be more strict with you folks.

Keith,

first of all thanks for your prompt reply. Then...

Quoted 11 lines · Keith Packard

Quoted 3 lines · Kirill Smelkov

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 74 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001
From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in
intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_dma.c +2 -4

index 1271282..8a3942c 100644
@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct drm_device *dev)
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct drm_device *dev)
 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 	}
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah->vaddr;
 
-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem *)dev_priv->status_page_dmah->vaddr,
+		  0, PAGE_SIZE);
 
 	i915_write_hws_pga(dev);
 

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size) · line 1329

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +3 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 		ring->get_seqno = pc_render_get_seqno;
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr = dev_priv->status_page_dmah->vaddr;
+
 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index 03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

From the outside it looks like the offending patch was done as a quick

Quoted 109 lines · Kirill Smelkov

Keith,

first of all thanks for your prompt reply. Then...

Quoted 11 lines · Keith Packard

Quoted 3 lines · Kirill Smelkov

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 74 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001
From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in
intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---
 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/i915_dma.c +2 -4

index 1271282..8a3942c 100644
@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct drm_device *dev)
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct drm_device *dev)
 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 	}
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah->vaddr;
 
-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem *)dev_priv->status_page_dmah->vaddr,
+		  0, PAGE_SIZE);
 
 	i915_write_hws_pga(dev);
 

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size) · line 1329

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +3 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
 		ring->get_seqno = pc_render_get_seqno;
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr = dev_priv->status_page_dmah->vaddr;
+
 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Keith, thanks again for the patch. As promised I've tested it on the hardware in question and yes, bad_access is gone and X seems to work, so thank you, but...

I see there are more such bugs in introduced-in-guilty-patch intel_render_ring_init_dri(). For example ring->irq_queue is left uninitialized and also ring->irq_lock etc...

I'm X newbie, so if here is something stupid X-wise, please don't beat me too hard, but to me the gist of the problem is the original patch, where Chris does

( git show e8616b6ced6137085e6657cc63bc2fe3900b8616 )

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

index 03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_init_render_ring_buffer(struct drm_device *dev) · line 1299

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +6 -0

@@ -1291,6 +1291,48 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
        return intel_init_ring_buffer(dev, ring);
 }
 
+int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size)
+{
+       drm_i915_private_t *dev_priv = dev->dev_private;
+       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+
+       *ring = render_ring;

          ^^^^^^^^^^^^^^^^^^^
          here resets

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_init_render_ring_buffer(struct drm_device *dev) · line 1307

Quoted 8 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +8 -0

+       if (INTEL_INFO(dev)->gen >= 6) {
+               ring->add_request = gen6_add_request;
+               ring->irq_get = gen6_render_ring_get_irq;
+               ring->irq_put = gen6_render_ring_put_irq;
+       } else if (IS_GEN5(dev)) {
+               ring->add_request = pc_render_add_request;
+               ring->get_seqno = pc_render_get_seqno;
+       }

and then the rest of the ring is initialized seemingly copy-pasted from intel_init_ring_buffer():

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_init_render_ring_buffer(struct drm_device *dev) · line 1322

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +10 -0

+       ring->size = size;
+       ring->effective_size = ring->size;
+       if (IS_I830(ring->dev))
+               ring->effective_size -= 128;
+
+       ring->map.offset = start;
+       ring->map.size = size;
+       ring->map.type = 0;
+       ring->map.flags = 0;
+       ring->map.mtrr = 0;

...

where both 3 chunks go almost exactly from intel_init_ring_buffer(), and ring->effective_size tweak even stripped original comment:

# original version from intel_init_ring_buffer():
        /* Workaround an erratum on the i830 which causes a hang if
         * the TAIL pointer points to within the last 2 cachelines
         * of the buffer.
         */
        ring->effective_size = ring->size;
        if (IS_I830(ring->dev))
                ring->effective_size -= 128;

...

The line marked "here resets" resets all the fields, and maybe it's not a good idea to re-initialize them all afterwards (missing some as this thread show), or at least if it is really needed, share initialization code between intel_render_ring_init_dri() and intel_init_ring_buffer() ?

Quoted 1 line

From the outside it looks like the offending patch was done as a quick

fix in a hurry (lots of copy-paste), and maybe it would be better to re-do it properly...

Quoted 7 lines · Kirill Smelkov

Quoted 6 lines · Kirill Smelkov

Keith,

first of all thanks for your prompt reply. Then...

Quoted 1 line · Keith Packard

On Fri, 22 Jul 2011 15:08:06 +0400, Kirill Smelkov <[email protected]>

Quoted 214 lines

Quoted 120 lines

Quoted 10 lines

Quoted 5 lines

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first

bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 97 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in

intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---

 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_dma.c index 1271282..8a3942c 100644

drivers/gpu/drm/i915/i915_dma.c +1 -4

@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct drm_device
 *dev)
 
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct drm_device
 *dev)
 
 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 
 	}
 
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah->vaddr;
 
-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem

*)dev_priv->status_page_dmah->vaddr, + 0, PAGE_SIZE);

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	i915_write_hws_pga(dev);

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +3 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct
 drm_device *dev, u64 start, u32 size)
 
 		ring->get_seqno = pc_render_get_seqno;
 
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr = dev_priv->status_page_dmah->vaddr;
+

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size) · line 1329

Quoted 3 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Keith, thanks again for the patch. As promised I've tested it on the hardware in question and yes, bad_access is gone and X seems to work, so thank you, but...

I see there are more such bugs in introduced-in-guilty-patch intel_render_ring_init_dri(). For example ring->irq_queue is left uninitialized and also ring->irq_lock etc...

I'm X newbie, so if here is something stupid X-wise, please don't beat me too hard, but to me the gist of the problem is the original patch, where Chris does

( git show e8616b6ced6137085e6657cc63bc2fe3900b8616 )

Quoted 15 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index 03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -0

@@ -1291,6 +1291,48 @@ int intel_init_render_ring_buffer(struct
 drm_device *dev)
 
        return intel_init_ring_buffer(dev, ring);
 
 }
 
+int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32

size) +{

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +4 -0

+       drm_i915_private_t *dev_priv = dev->dev_private;
+       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+
+       *ring = render_ring;

          ^^^^^^^^^^^^^^^^^^^
          here resets

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +8 -0

+       if (INTEL_INFO(dev)->gen >= 6) {
+               ring->add_request = gen6_add_request;
+               ring->irq_get = gen6_render_ring_get_irq;
+               ring->irq_put = gen6_render_ring_put_irq;
+       } else if (IS_GEN5(dev)) {
+               ring->add_request = pc_render_add_request;
+               ring->get_seqno = pc_render_get_seqno;
+       }

and then the rest of the ring is initialized seemingly copy-pasted

from intel_init_ring_buffer():

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +10 -0

+       ring->size = size;
+       ring->effective_size = ring->size;
+       if (IS_I830(ring->dev))
+               ring->effective_size -= 128;
+
+       ring->map.offset = start;
+       ring->map.size = size;
+       ring->map.type = 0;
+       ring->map.flags = 0;
+       ring->map.mtrr = 0;

...

where both 3 chunks go almost exactly from intel_init_ring_buffer(), and ring->effective_size tweak even stripped original comment:

# original version from intel_init_ring_buffer():
        /* Workaround an erratum on the i830 which causes a hang if

         * the TAIL pointer points to within the last 2 cachelines
         * of the buffer.
         */
        
        ring->effective_size = ring->size;
        if (IS_I830(ring->dev))
        
                ring->effective_size -= 128;

...

The line marked "here resets" resets all the fields, and maybe it's not a good idea to re-initialize them all afterwards (missing some as this thread show), or at least if it is really needed, share initialization code between intel_render_ring_init_dri() and intel_init_ring_buffer() ?

Quoted 4 lines

Quoted 1 line

From the outside it looks like the offending patch was done as a quick

fix in a hurry (lots of copy-paste), and maybe it would be better to re-do it properly...

Silence... ?

I read UMS is still ignored, because e.g. that uninitialized ring->irq_lock which I've wrote about above is for sure used e.g. in gen6_render_ring_get_irq() added to ring vtable in intel_render_ring_init_dri().

Quoted 8 lines · On Tuesday 09 August 2011 15:08:03 Kirill Smelkov wrote:

Quoted 7 lines · Kirill Smelkov

Quoted 6 lines · Kirill Smelkov

Keith,

first of all thanks for your prompt reply. Then...

Quoted 1 line · Keith Packard

On Fri, 22 Jul 2011 15:08:06 +0400, Kirill Smelkov <[email protected]>

Quoted 219 lines · wrote:

Quoted 218 lines

Quoted 120 lines

Quoted 10 lines

Quoted 5 lines

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first

bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 97 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in

intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---

 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_dma.c index 1271282..8a3942c 100644

drivers/gpu/drm/i915/i915_dma.c +1 -4

@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct drm_device
 *dev)
 
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct drm_device
 *dev)
 
 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 
 	}
 
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah->vaddr;
 
-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem

*)dev_priv->status_page_dmah->vaddr, + 0, PAGE_SIZE);

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	i915_write_hws_pga(dev);

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +3 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct
 drm_device *dev, u64 start, u32 size)
 
 		ring->get_seqno = pc_render_get_seqno;
 
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr = dev_priv->status_page_dmah->vaddr;
+

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size) · line 1329

Quoted 3 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Keith, thanks again for the patch. As promised I've tested it on the hardware in question and yes, bad_access is gone and X seems to work, so thank you, but...

I see there are more such bugs in introduced-in-guilty-patch intel_render_ring_init_dri(). For example ring->irq_queue is left uninitialized and also ring->irq_lock etc...

I'm X newbie, so if here is something stupid X-wise, please don't beat me too hard, but to me the gist of the problem is the original patch, where Chris does

( git show e8616b6ced6137085e6657cc63bc2fe3900b8616 )

Quoted 15 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index 03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -0

@@ -1291,6 +1291,48 @@ int intel_init_render_ring_buffer(struct
 drm_device *dev)
 
        return intel_init_ring_buffer(dev, ring);
 
 }
 
+int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32

size) +{

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +4 -0

+       drm_i915_private_t *dev_priv = dev->dev_private;
+       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+
+       *ring = render_ring;

          ^^^^^^^^^^^^^^^^^^^
          here resets

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +8 -0

+       if (INTEL_INFO(dev)->gen >= 6) {
+               ring->add_request = gen6_add_request;
+               ring->irq_get = gen6_render_ring_get_irq;
+               ring->irq_put = gen6_render_ring_put_irq;
+       } else if (IS_GEN5(dev)) {
+               ring->add_request = pc_render_add_request;
+               ring->get_seqno = pc_render_get_seqno;
+       }

and then the rest of the ring is initialized seemingly copy-pasted

from intel_init_ring_buffer():

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +10 -0

+       ring->size = size;
+       ring->effective_size = ring->size;
+       if (IS_I830(ring->dev))
+               ring->effective_size -= 128;
+
+       ring->map.offset = start;
+       ring->map.size = size;
+       ring->map.type = 0;
+       ring->map.flags = 0;
+       ring->map.mtrr = 0;

...

where both 3 chunks go almost exactly from intel_init_ring_buffer(), and ring->effective_size tweak even stripped original comment:

# original version from intel_init_ring_buffer():
        /* Workaround an erratum on the i830 which causes a hang if

         * the TAIL pointer points to within the last 2 cachelines
         * of the buffer.
         */
        
        ring->effective_size = ring->size;
        if (IS_I830(ring->dev))
        
                ring->effective_size -= 128;

...

The line marked "here resets" resets all the fields, and maybe it's not a good idea to re-initialize them all afterwards (missing some as this thread show), or at least if it is really needed, share initialization code between intel_render_ring_init_dri() and intel_init_ring_buffer() ?

Quoted 1 line

From the outside it looks like the offending patch was done as a quick

fix in a hurry (lots of copy-paste), and maybe it would be better to re-do it properly...

Quoted 6 lines

Silence... ?

I read UMS is still ignored, because e.g. that uninitialized ring->irq_lock which I've wrote about above is for sure used e.g. in gen6_render_ring_get_irq() added to ring vtable in intel_render_ring_init_dri().

I really doubt that UMS supports gen6 hardware.

Quoted 237 lines · Vasily Khoruzhick

Quoted 9 lines · On Tuesday 09 August 2011 15:08:03 Kirill Smelkov wrote:

Quoted 8 lines · Kirill Smelkov

Quoted 7 lines · Kirill Smelkov

Keith,

first of all thanks for your prompt reply. Then...

Quoted 2 lines · Keith Packard

On Fri, 22 Jul 2011 15:08:06 +0400, Kirill Smelkov <[email protected]>

Quoted 225 lines · wrote:

Quoted 224 lines

Quoted 124 lines

Quoted 11 lines

Quoted 5 lines

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first

bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 100 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in

intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including
ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---

 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_dma.c index 1271282..8a3942c 100644

drivers/gpu/drm/i915/i915_dma.c +1 -4

@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct
 drm_device *dev)
 
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct
 drm_device *dev)
 
 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 
 	}
 
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah->vaddr;
 
-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem

*)dev_priv->status_page_dmah->vaddr, + 0, PAGE_SIZE);

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	i915_write_hws_pga(dev);

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct
 drm_device *dev, u64 start, u32 size)
 
 		ring->get_seqno = pc_render_get_seqno;
 
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr =

dev_priv->status_page_dmah->vaddr; +

drivers/gpu/drm/i915/intel_ringbuffer.c int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size) · line 1329

Quoted 3 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Keith, thanks again for the patch. As promised I've tested it on the hardware in question and yes, bad_access is gone and X seems to work, so thank you, but...

I see there are more such bugs in introduced-in-guilty-patch intel_render_ring_init_dri(). For example ring->irq_queue is left uninitialized and also ring->irq_lock etc...

I'm X newbie, so if here is something stupid X-wise, please don't beat me too hard, but to me the gist of the problem is the original patch, where Chris does

( git show e8616b6ced6137085e6657cc63bc2fe3900b8616 )

Quoted 15 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index 03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -0

@@ -1291,6 +1291,48 @@ int intel_init_render_ring_buffer(struct
 drm_device *dev)
 
        return intel_init_ring_buffer(dev, ring);
 
 }
 
+int intel_render_ring_init_dri(struct drm_device *dev, u64 start,

u32 size) +{

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +4 -0

+       drm_i915_private_t *dev_priv = dev->dev_private;
+       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+
+       *ring = render_ring;

          ^^^^^^^^^^^^^^^^^^^
          here resets

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +8 -0

+       if (INTEL_INFO(dev)->gen >= 6) {
+               ring->add_request = gen6_add_request;
+               ring->irq_get = gen6_render_ring_get_irq;
+               ring->irq_put = gen6_render_ring_put_irq;
+       } else if (IS_GEN5(dev)) {
+               ring->add_request = pc_render_add_request;
+               ring->get_seqno = pc_render_get_seqno;
+       }

and then the rest of the ring is initialized seemingly copy-pasted

from intel_init_ring_buffer():

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +10 -0

+       ring->size = size;
+       ring->effective_size = ring->size;
+       if (IS_I830(ring->dev))
+               ring->effective_size -= 128;
+
+       ring->map.offset = start;
+       ring->map.size = size;
+       ring->map.type = 0;
+       ring->map.flags = 0;
+       ring->map.mtrr = 0;

...

where both 3 chunks go almost exactly from intel_init_ring_buffer(), and ring->effective_size tweak even stripped original comment:

# original version from intel_init_ring_buffer():
        /* Workaround an erratum on the i830 which causes a hang if

         * the TAIL pointer points to within the last 2 cachelines
         * of the buffer.
         */
        
        ring->effective_size = ring->size;
        if (IS_I830(ring->dev))
        
                ring->effective_size -= 128;

...

The line marked "here resets" resets all the fields, and maybe it's not a good idea to re-initialize them all afterwards (missing some as this thread show), or at least if it is really needed, share initialization code between intel_render_ring_init_dri() and intel_init_ring_buffer() ?

Quoted 2 lines

From the outside it looks like the offending patch was done as a quick

fix in a hurry (lots of copy-paste), and maybe it would be better to re-do it properly...

Quoted 8 lines

Quoted 6 lines

Silence... ?

I read UMS is still ignored, because e.g. that uninitialized ring->irq_lock which I've wrote about above is for sure used e.g. in gen6_render_ring_get_irq() added to ring vtable in intel_render_ring_init_dri().

I really doubt that UMS supports gen6 hardware.

Then why it is there in intel_render_ring_init_dri():

int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32

size) {
    	drm_i915_private_t *dev_priv = dev->dev_private;
    	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];

    	*ring = render_ring;
    	if (INTEL_INFO(dev)->gen >= 6) {

    		ring->add_request = gen6_add_request;
    		ring->irq_get = gen6_render_ring_get_irq;
    		ring->irq_put = gen6_render_ring_put_irq;
    	} else if (IS_GEN5(dev)) {
    		ring->add_request = pc_render_add_request;
    		ring->get_seqno = pc_render_get_seqno;
    	}

Quoted 254 lines · On Tuesday 09 August 2011 17:47:56 Kirill Smelkov wrote:

Quoted 237 lines · Vasily Khoruzhick

Quoted 9 lines · On Tuesday 09 August 2011 15:08:03 Kirill Smelkov wrote:

Quoted 8 lines · Kirill Smelkov

Quoted 7 lines · Kirill Smelkov

Keith,

first of all thanks for your prompt reply. Then...

Quoted 2 lines · Keith Packard

On Fri, 22 Jul 2011 15:08:06 +0400, Kirill Smelkov <[email protected]>

Quoted 225 lines · wrote:

Quoted 224 lines

Quoted 124 lines

Quoted 11 lines

Quoted 5 lines

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first

bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 100 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in

intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including
ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---

 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_dma.c index 1271282..8a3942c 100644

drivers/gpu/drm/i915/i915_dma.c +1 -4

@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct
 drm_device *dev)
 
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct
 drm_device *dev)
 
 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 
 	}
 
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah->vaddr;
 
-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem

*)dev_priv->status_page_dmah->vaddr, + 0, PAGE_SIZE);

drivers/gpu/drm/i915/i915_dma.c +0 -0

 	i915_write_hws_pga(dev);

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct
 drm_device *dev, u64 start, u32 size)
 
 		ring->get_seqno = pc_render_get_seqno;
 
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr =

dev_priv->status_page_dmah->vaddr; +

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 3 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Keith, thanks again for the patch. As promised I've tested it on the hardware in question and yes, bad_access is gone and X seems to work, so thank you, but...

I see there are more such bugs in introduced-in-guilty-patch intel_render_ring_init_dri(). For example ring->irq_queue is left uninitialized and also ring->irq_lock etc...

I'm X newbie, so if here is something stupid X-wise, please don't beat me too hard, but to me the gist of the problem is the original patch, where Chris does

( git show e8616b6ced6137085e6657cc63bc2fe3900b8616 )

Quoted 15 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index 03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -0

@@ -1291,6 +1291,48 @@ int intel_init_render_ring_buffer(struct
 drm_device *dev)
 
        return intel_init_ring_buffer(dev, ring);
 
 }
 
+int intel_render_ring_init_dri(struct drm_device *dev, u64 start,

u32 size) +{

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +4 -0

+       drm_i915_private_t *dev_priv = dev->dev_private;
+       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+
+       *ring = render_ring;

          ^^^^^^^^^^^^^^^^^^^
          here resets

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +8 -0

+       if (INTEL_INFO(dev)->gen >= 6) {
+               ring->add_request = gen6_add_request;
+               ring->irq_get = gen6_render_ring_get_irq;
+               ring->irq_put = gen6_render_ring_put_irq;
+       } else if (IS_GEN5(dev)) {
+               ring->add_request = pc_render_add_request;
+               ring->get_seqno = pc_render_get_seqno;
+       }

and then the rest of the ring is initialized seemingly copy-pasted

from intel_init_ring_buffer():

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +10 -0

+       ring->size = size;
+       ring->effective_size = ring->size;
+       if (IS_I830(ring->dev))
+               ring->effective_size -= 128;
+
+       ring->map.offset = start;
+       ring->map.size = size;
+       ring->map.type = 0;
+       ring->map.flags = 0;
+       ring->map.mtrr = 0;

...

where both 3 chunks go almost exactly from intel_init_ring_buffer(), and ring->effective_size tweak even stripped original comment:

# original version from intel_init_ring_buffer():
        /* Workaround an erratum on the i830 which causes a hang if

         * the TAIL pointer points to within the last 2 cachelines
         * of the buffer.
         */
        
        ring->effective_size = ring->size;
        if (IS_I830(ring->dev))
        
                ring->effective_size -= 128;

...

The line marked "here resets" resets all the fields, and maybe it's not a good idea to re-initialize them all afterwards (missing some as this thread show), or at least if it is really needed, share initialization code between intel_render_ring_init_dri() and intel_init_ring_buffer() ?

Quoted 2 lines

From the outside it looks like the offending patch was done as a quick

fix in a hurry (lots of copy-paste), and maybe it would be better to re-do it properly...

Quoted 8 lines

Quoted 6 lines

Silence... ?

I read UMS is still ignored, because e.g. that uninitialized ring->irq_lock which I've wrote about above is for sure used e.g. in gen6_render_ring_get_irq() added to ring vtable in intel_render_ring_init_dri().

I really doubt that UMS supports gen6 hardware.

Then why it is there in intel_render_ring_init_dri():

int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32

size) {
    	drm_i915_private_t *dev_priv = dev->dev_private;
    	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];

Quoted 2 lines

    	*ring = render_ring;
    	if (INTEL_INFO(dev)->gen >= 6) {

This branch executes only when hw generation is 6 or newer.

Quoted 7 lines

    		ring->add_request = gen6_add_request;
    		ring->irq_get = gen6_render_ring_get_irq;
    		ring->irq_put = gen6_render_ring_put_irq;
    	} else if (IS_GEN5(dev)) {
    		ring->add_request = pc_render_add_request;
    		ring->get_seqno = pc_render_get_seqno;
    	}

Quoted 101 lines · Vasily Khoruzhick

Quoted 100 lines · On Tuesday 09 August 2011 17:47:56 Kirill Smelkov wrote:

Quoted 99 lines · Vasily Khoruzhick

Quoted 9 lines · On Tuesday 09 August 2011 15:08:03 Kirill Smelkov wrote:

Quoted 8 lines · Kirill Smelkov

Quoted 7 lines · Kirill Smelkov

Keith,

first of all thanks for your prompt reply. Then...

Quoted 2 lines · Keith Packard

On Fri, 22 Jul 2011 15:08:06 +0400, Kirill Smelkov <[email protected]>

Quoted 87 lines · wrote:

Quoted 87 lines

Quoted 87 lines

Quoted 11 lines

Quoted 5 lines

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first

bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 71 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in

intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---

 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_dma.c index 1271282..8a3942c 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c

(quoted diff) +0 -1

@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct
 drm_device *dev)
 
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct
 drm_device *dev)
 

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Review static int i915_init_phys_hws(struct · line 79

Quoted 7 lines

(quoted diff) +0 -2

 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 
 	}
 
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah-

vaddr;

Quoted 156 lines

Quoted 155 lines

Quoted 135 lines

Quoted 135 lines

Quoted 134 lines

Quoted 31 lines

Quoted 23 lines

(quoted diff) +1 -1

-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem

*)dev_priv->status_page_dmah->vaddr, + 0, PAGE_SIZE);

(quoted diff) +0 -0

 	i915_write_hws_pga(dev);

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index
e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct
 drm_device *dev, u64 start, u32 size)
 
 		ring->get_seqno = pc_render_get_seqno;
 
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr =

dev_priv->status_page_dmah->vaddr; +

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 3 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Keith, thanks again for the patch. As promised I've tested it on the hardware in question and yes, bad_access is gone and X seems to work, so thank you, but...

I see there are more such bugs in introduced-in-guilty-patch intel_render_ring_init_dri(). For example ring->irq_queue is left uninitialized and also ring->irq_lock etc...

I'm X newbie, so if here is something stupid X-wise, please don't beat me too hard, but to me the gist of the problem is the original patch, where Chris does

( git show e8616b6ced6137085e6657cc63bc2fe3900b8616 )

Quoted 15 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index
03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -0

@@ -1291,6 +1291,48 @@ int intel_init_render_ring_buffer(struct
 drm_device *dev)
 
        return intel_init_ring_buffer(dev, ring);
 
 }
 
+int intel_render_ring_init_dri(struct drm_device *dev, u64

start, u32 size) +{

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +4 -0

+       drm_i915_private_t *dev_priv = dev->dev_private;
+       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+
+       *ring = render_ring;

          ^^^^^^^^^^^^^^^^^^^
          here resets

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +8 -0

+       if (INTEL_INFO(dev)->gen >= 6) {
+               ring->add_request = gen6_add_request;
+               ring->irq_get = gen6_render_ring_get_irq;
+               ring->irq_put = gen6_render_ring_put_irq;
+       } else if (IS_GEN5(dev)) {
+               ring->add_request = pc_render_add_request;
+               ring->get_seqno = pc_render_get_seqno;
+       }

and then the rest of the ring is initialized seemingly copy-pasted

from intel_init_ring_buffer():

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +10 -0

+       ring->size = size;
+       ring->effective_size = ring->size;
+       if (IS_I830(ring->dev))
+               ring->effective_size -= 128;
+
+       ring->map.offset = start;
+       ring->map.size = size;
+       ring->map.type = 0;
+       ring->map.flags = 0;
+       ring->map.mtrr = 0;

...

where both 3 chunks go almost exactly from intel_init_ring_buffer(), and ring->effective_size tweak even stripped original comment:

# original version from intel_init_ring_buffer():
        /* Workaround an erratum on the i830 which causes a hang
        if

         * the TAIL pointer points to within the last 2
         cachelines * of the buffer.
         */
        
        ring->effective_size = ring->size;
        if (IS_I830(ring->dev))
        
                ring->effective_size -= 128;

...

The line marked "here resets" resets all the fields, and maybe it's not a good idea to re-initialize them all afterwards (missing some as this thread show), or at least if it is really needed, share initialization code between intel_render_ring_init_dri() and intel_init_ring_buffer() ?

Quoted 2 lines

From the outside it looks like the offending patch was done as a quick

fix in a hurry (lots of copy-paste), and maybe it would be better to re-do it properly...

Quoted 8 lines

Quoted 6 lines

Silence... ?

I read UMS is still ignored, because e.g. that uninitialized ring->irq_lock which I've wrote about above is for sure used e.g. in gen6_render_ring_get_irq() added to ring vtable in intel_render_ring_init_dri().

I really doubt that UMS supports gen6 hardware.

Then why it is there in intel_render_ring_init_dri():
int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32

size) {

    	drm_i915_private_t *dev_priv = dev->dev_private;
    	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
    	
    	*ring = render_ring;
    	if (INTEL_INFO(dev)->gen >= 6) {

Quoted 1 line

This branch executes only when hw generation is 6 or newer.

and adds gen6_render_ring_get_irq() to vtable which uses ring->irq_lock which is left uninitialized.

I don't understand what you were trying to say. How does it matter if some branch executes only for such-and-such hardware, when this branch contains bugs? Could you please clarify?

Quoted 271 lines · On Tuesday 09 August 2011 18:34:46 Kirill Smelkov wrote:

Quoted 101 lines · Vasily Khoruzhick

Quoted 100 lines · On Tuesday 09 August 2011 17:47:56 Kirill Smelkov wrote:

Quoted 99 lines · Vasily Khoruzhick

Quoted 9 lines · On Tuesday 09 August 2011 15:08:03 Kirill Smelkov wrote:

Quoted 8 lines · Kirill Smelkov

Quoted 7 lines · Kirill Smelkov

Keith,

first of all thanks for your prompt reply. Then...

Quoted 2 lines · Keith Packard

On Fri, 22 Jul 2011 15:08:06 +0400, Kirill Smelkov <[email protected]>

Quoted 87 lines · wrote:

Quoted 87 lines

Quoted 87 lines

Quoted 11 lines

Quoted 5 lines

And now after v3.0 is out, I've tested it again, and yes, like it was broken on v3.0-rc5, it is (now even more) broken on v3.0 -- after first

bad io access the system freezes completely:

I looked at this when I first saw it (a couple of weeks ago), and I couldn't see any obvious reason this patch would cause this particular problem. I didn't want to revert the patch at that point as I feared it would cause other subtle problems. Given that you've got a work-around, it seemed best to just push this off past 3.0.

What kind of a workaround are you talking about? Sorry, to me it all looked like "UMS is being ignored forever". Anyway, let's move on to try to solve the issue.

Quoted 71 lines

Given the failing address passed to ioread32, this seems like it's probably the call to READ_BREADCRUMB -- I915_BREADCRUMB_INDEX is 0x21, which is an offset in 32-bit units within the hardware status page. If the status_page.page_addr value was zero, then the computed address would end up being 0x84.

And, it looks like status_page.page_addr *will* end up being zero as a result of the patch in question. The patch resets the entire ring structure contents back to the initial values, which includes smashing the status_page structure to zero, clearing the value of status_page.page_addr set in i915_init_phys_hws.

Here's an untested patch which moves the initialization of status_page.page_addr into intel_render_ring_init_dri. I note that intel_init_render_ring_buffer *already* has the setting of the status_page.page_addr value, and so I've removed the setting of status_page.page_addr from i915_init_phys_hws.

I suspect we could remove the memset from intel_init_render_ring_buffer; it seems entirely superfluous given the memset in i915_init_phys_hws.

From 159ba1dd207fc52590ce8a3afd83f40bd2cedf46 Mon Sep 17 00:00:00 2001 From: Keith Packard <[email protected]>
Date: Fri, 22 Jul 2011 10:44:39 -0700
Subject: [PATCH] drm/i915: Initialize RCS ring status page address in

intel_render_ring_init_dri

Physically-addressed hardware status pages are initialized early in the driver load process by i915_init_phys_hws. For UMS environments, the ring structure is not initialized until the X server starts. At that point, the entire ring structure is re-initialized with all new values. Any values set in the ring structure (including ring->status_page.page_addr) will be lost when the ring is re-initialized.

This patch moves the initialization of the status_page.page_addr value to intel_render_ring_init_dri.

Signed-off-by Keith Packard <[email protected]>

---

 drivers/gpu/drm/i915/i915_dma.c         |    6 ++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_dma.c index 1271282..8a3942c 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c

(quoted diff) +0 -1

@@ -61,7 +61,6 @@ static void i915_write_hws_pga(struct
 drm_device *dev)
 
 static int i915_init_phys_hws(struct drm_device *dev)
 {
 
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	struct intel_ring_buffer *ring = LP_RING(dev_priv);
 
 	/* Program Hardware Status Page */
 	dev_priv->status_page_dmah =

@@ -71,10 +70,9 @@ static int i915_init_phys_hws(struct
 drm_device *dev)
 

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Review static int i915_init_phys_hws(struct · line 79

Quoted 7 lines

(quoted diff) +0 -2

 		DRM_ERROR("Can not allocate hardware status page\n");
 		return -ENOMEM;
 
 	}
 
-	ring->status_page.page_addr =
-		(void __force __iomem *)dev_priv->status_page_dmah-

vaddr;

Quoted 150 lines

Quoted 150 lines

Quoted 135 lines

Quoted 135 lines

Quoted 134 lines

Quoted 31 lines

Quoted 23 lines

(quoted diff) +1 -1

-	memset_io(ring->status_page.page_addr, 0, PAGE_SIZE);
+	memset_io((void __force __iomem

*)dev_priv->status_page_dmah->vaddr, + 0, PAGE_SIZE);

(quoted diff) +0 -0

 	i915_write_hws_pga(dev);

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index
e961568..47b9b27 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +2 -0

@@ -1321,6 +1321,9 @@ int intel_render_ring_init_dri(struct
 drm_device *dev, u64 start, u32 size)
 
 		ring->get_seqno = pc_render_get_seqno;
 
 	}
 
+	if (!I915_NEED_GFX_HWS(dev))
+		ring->status_page.page_addr =

dev_priv->status_page_dmah->vaddr; +

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 3 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +0 -0

 	ring->dev = dev;
 	INIT_LIST_HEAD(&ring->active_list);
 	INIT_LIST_HEAD(&ring->request_list);

I can't tell whether this is correct, because intel gfx driver is unknown to me, but from the first glance your description sounds reasonable.

Quoted 2 lines

I'm out of office till ~ next week's tuesday, and on return I'll try to test it on the hardware in question.

Keith, thanks again for the patch. As promised I've tested it on the hardware in question and yes, bad_access is gone and X seems to work, so thank you, but...

I see there are more such bugs in introduced-in-guilty-patch intel_render_ring_init_dri(). For example ring->irq_queue is left uninitialized and also ring->irq_lock etc...

I'm X newbie, so if here is something stupid X-wise, please don't beat me too hard, but to me the gist of the problem is the original patch, where Chris does

( git show e8616b6ced6137085e6657cc63bc2fe3900b8616 )

Quoted 15 lines

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/intel_ringbuffer.c index
03e3370..51fbc5e 100644

drivers/gpu/drm/i915/intel_ringbuffer.c +1 -0

@@ -1291,6 +1291,48 @@ int intel_init_render_ring_buffer(struct
 drm_device *dev)
 
        return intel_init_ring_buffer(dev, ring);
 
 }
 
+int intel_render_ring_init_dri(struct drm_device *dev, u64

start, u32 size) +{

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 4 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +4 -0

+       drm_i915_private_t *dev_priv = dev->dev_private;
+       struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
+
+       *ring = render_ring;

          ^^^^^^^^^^^^^^^^^^^
          here resets

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 9 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +8 -0

+       if (INTEL_INFO(dev)->gen >= 6) {
+               ring->add_request = gen6_add_request;
+               ring->irq_get = gen6_render_ring_get_irq;
+               ring->irq_put = gen6_render_ring_put_irq;
+       } else if (IS_GEN5(dev)) {
+               ring->add_request = pc_render_add_request;
+               ring->get_seqno = pc_render_get_seqno;
+       }

and then the rest of the ring is initialized seemingly copy-pasted

from intel_init_ring_buffer():

Quoted 5 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +5 -0

+       ring->dev = dev;
+       INIT_LIST_HEAD(&ring->active_list);
+       INIT_LIST_HEAD(&ring->request_list);
+       INIT_LIST_HEAD(&ring->gpu_write_list);
+

drivers/gpu/drm/i915/intel_ringbuffer.c

Quoted 10 lines

drivers/gpu/drm/i915/intel_ringbuffer.c +10 -0

+       ring->size = size;
+       ring->effective_size = ring->size;
+       if (IS_I830(ring->dev))
+               ring->effective_size -= 128;
+
+       ring->map.offset = start;
+       ring->map.size = size;
+       ring->map.type = 0;
+       ring->map.flags = 0;
+       ring->map.mtrr = 0;

...

where both 3 chunks go almost exactly from intel_init_ring_buffer(), and ring->effective_size tweak even stripped original comment:

# original version from intel_init_ring_buffer():
        /* Workaround an erratum on the i830 which causes a hang
        if

         * the TAIL pointer points to within the last 2
         cachelines * of the buffer.
         */
        
        ring->effective_size = ring->size;
        if (IS_I830(ring->dev))
        
                ring->effective_size -= 128;

...

The line marked "here resets" resets all the fields, and maybe it's not a good idea to re-initialize them all afterwards (missing some as this thread show), or at least if it is really needed, share initialization code between intel_render_ring_init_dri() and intel_init_ring_buffer() ?

Quoted 2 lines

From the outside it looks like the offending patch was done as a quick

fix in a hurry (lots of copy-paste), and maybe it would be better to re-do it properly...

Quoted 8 lines

Quoted 6 lines

Silence... ?

I read UMS is still ignored, because e.g. that uninitialized ring->irq_lock which I've wrote about above is for sure used e.g. in gen6_render_ring_get_irq() added to ring vtable in intel_render_ring_init_dri().

I really doubt that UMS supports gen6 hardware.

Then why it is there in intel_render_ring_init_dri():
int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32

size) {

Quoted 7 lines

Quoted 5 lines

    	drm_i915_private_t *dev_priv = dev->dev_private;
    	struct intel_ring_buffer *ring = &dev_priv->ring[RCS];
    	
    	*ring = render_ring;
    	if (INTEL_INFO(dev)->gen >= 6) {

This branch executes only when hw generation is 6 or newer.

and adds gen6_render_ring_get_irq() to vtable which uses ring->irq_lock which is left uninitialized.

Quoted 3 lines

I don't understand what you were trying to say. How does it matter if some branch executes only for such-and-such hardware, when this branch contains bugs? Could you please clarify?

I want to say that xf86-video-intel with gen6 support does not support UMS. So you can't even hit this "bug".

Quite frankly, I don't understand intel-gfx developers attitude: why is it me, just random user who is nitpicking here? Why there is no interest/will to analyze now obviously buggy/duplicate code and fix it?

Quoted 3 lines · Kirill Smelkov

Quite frankly, I don't understand intel-gfx developers attitude: why is it me, just random user who is nitpicking here? Why there is no interest/will to analyze now obviously buggy/duplicate code and fix it?

Because they don't have an infinite amount of manpower. Actual bugs hitting actual users take precedence over 'cleanups' which always have a chance of causing regressions, as you're well aware. Code churn for the sake of abstract prettiness is discouraged, as it has a potential cost for little potential gain.

If you like, submit a patch. You may now be more up-to-date on those particular code paths than most of the intel-gfx developers.

Quoted 2 lines

If you like, submit a patch. You may now be more up-to-date on those particular code paths than most of the intel-gfx developers.

Ray, I'd agree with you if the topic was about cleanups.

Quoted 4 lines · Kirill Smelkov

Quoted 2 lines

If you like, submit a patch. You may now be more up-to-date on those particular code paths than most of the intel-gfx developers.

Ray, I'd agree with you if the topic was about cleanups.

At this point it is about cleanups unless Keith's patch upthread does not work for you. Does it or not?

Sorry, I won't submit a patch. If there is a need to find/fix/cleanup obvious things after company's developers, I have better things to do, and a todo item to re-evaluate hardware for my next project.

Quoted 3 lines

Sorry, I won't submit a patch. If there is a need to find/fix/cleanup obvious things after company's developers, I have better things to do, and a todo item to re-evaluate hardware for my next project.

You seem confused. If you have a support contract of some form with a Linux supplier or Intel please contact your support. This mailing list isn't for support services.