From: Kirill Smelkov <kirr@mns.spb.ru>
To: Keith Packard <keithp@keithp.com>
Cc: Pekka Enberg <penberg@kernel.org>,
Chris Wilson <chris@chris-wilson.co.uk>,
Luke-Jr <luke@dashjr.org>,
intel-gfx@lists.freedesktop.org,
LKML <linux-kernel@vger.kernel.org>,
dri-devel@lists.freedesktop.org,
"Rafael J. Wysocki" <rjw@sisk.pl>, Ray Lee <ray-lk@madrabbit.org>,
Herbert Xu <herbert@gondor.apana.org.au>,
Linus Torvalds <torvalds@linux-foundation.org>,
Andrew Morton <akpm@linux-foundation.org>,
Florian Mickler <florian@mickler.org>
Subject: Re: Major 2.6.38 / 2.6.39 / 3.0 regression ignored?
Date: Sat, 23 Jul 2011 00:23:36 +0400 [thread overview]
Message-ID: <20110722202336.GA14375@tugrik.mns.mnsspb.ru> (raw)
In-Reply-To: <yun7h7atcty.fsf@aiko.keithp.com>
Keith,
first of all thanks for your prompt reply. Then...
On Fri, Jul 22, 2011 at 11:00:41AM -0700, Keith Packard wrote:
> On Fri, 22 Jul 2011 15:08:06 +0400, Kirill Smelkov <kirr@mns.spb.ru> wrote:
>
> > 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.
> 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 <keithp@keithp.com>
> 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 <keithp@keithp.com>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 6 ++----
> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c 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
> @@ -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);
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e961568..47b9b27 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -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.
Thanks again,
Kirill
next prev parent reply other threads:[~2011-07-22 20:23 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-20 17:06 Major 2.6.38 regression ignored? Luke-Jr
2011-05-20 18:08 ` Ray Lee
2011-05-20 20:24 ` Rafael J. Wysocki
2011-05-20 21:11 ` Ray Lee
2011-05-21 8:41 ` Chris Wilson
2011-05-21 15:23 ` Luke-Jr
2011-05-21 15:40 ` Chris Wilson
2011-05-21 19:33 ` Luke-Jr
2011-05-28 13:19 ` Major 2.6.38 / 2.6.39 " Kirill Smelkov
2011-07-12 17:17 ` [Intel-gfx] " Kirill Smelkov
2011-07-12 18:07 ` Pekka Enberg
[not found] ` <CA+55aFxakA2U+oMJ1T7awTYa+p6xp9N0aCbfrUqgkF7BJ8gnQw@mail.gmail.com>
2011-07-22 11:08 ` Major 2.6.38 / 2.6.39 / 3.0 " Kirill Smelkov
2011-07-22 14:12 ` Herbert Xu
2011-07-22 18:00 ` Keith Packard
2011-07-22 20:23 ` Kirill Smelkov [this message]
2011-07-22 20:50 ` Keith Packard
2011-07-22 21:08 ` Kirill Smelkov
2011-07-22 21:31 ` Kirill Smelkov
2011-07-23 15:10 ` [Intel-gfx] " Alex Deucher
2011-07-23 18:19 ` Kirill Smelkov
2011-07-23 15:55 ` Pekka Enberg
2011-07-25 4:29 ` Keith Packard
2011-07-26 13:48 ` [Intel-gfx] " Kirill Smelkov
2011-08-09 12:08 ` Kirill Smelkov
2011-08-09 14:00 ` Vasily Khoruzhick
2011-08-09 14:47 ` Kirill Smelkov
2011-08-09 15:09 ` Vasily Khoruzhick
2011-08-09 15:34 ` Kirill Smelkov
2011-08-09 16:02 ` Vasily Khoruzhick
2011-08-09 16:32 ` [Intel-gfx] " Kirill Smelkov
2011-08-09 16:56 ` Ray Lee
2011-08-09 17:40 ` Kirill Smelkov
2011-08-09 17:43 ` [Intel-gfx] " Ray Lee
2011-08-10 8:36 ` Kirill Smelkov
2011-08-10 9:41 ` [Intel-gfx] " Alan Cox
2011-08-10 11:37 ` Kirill Smelkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110722202336.GA14375@tugrik.mns.mnsspb.ru \
--to=kirr@mns.spb.ru \
--cc=akpm@linux-foundation.org \
--cc=chris@chris-wilson.co.uk \
--cc=dri-devel@lists.freedesktop.org \
--cc=florian@mickler.org \
--cc=herbert@gondor.apana.org.au \
--cc=intel-gfx@lists.freedesktop.org \
--cc=keithp@keithp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luke@dashjr.org \
--cc=penberg@kernel.org \
--cc=ray-lk@madrabbit.org \
--cc=rjw@sisk.pl \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).