lkml.org 
[lkml]   [2011]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [Intel-gfx] Major 2.6.38 / 2.6.39 / 3.0 regression ignored?
    Date
    On Tuesday 09 August 2011 15:08:03 Kirill Smelkov wrote:
    > On Tue, Jul 26, 2011 at 05:48:27PM +0400, Kirill Smelkov wrote:
    > > On Sat, Jul 23, 2011 at 12:23:36AM +0400, Kirill Smelkov wrote:
    > > > 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.
    > >
    > > 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 )
    > >
    > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
    > > > b/drivers/gpu/drm/i915/intel_ringbuffer.c index 03e3370..51fbc5e
    > > > 100644
    > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
    > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
    > > > @@ -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
    > > >
    > > > + 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():
    > > > + ring->dev = dev;
    > > > + INIT_LIST_HEAD(&ring->active_list);
    > > > + INIT_LIST_HEAD(&ring->request_list);
    > > > + INIT_LIST_HEAD(&ring->gpu_write_list);
    > > > +
    > > > + 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() ?
    > >
    > > >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().

    I really doubt that UMS supports gen6 hardware.

    Regards
    Vasily


    \
     
     \ /
      Last update: 2011-08-09 16:03    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site