[PATCH] Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[PATCH] Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[Jean Delvare]

Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
hang when loading the eeprom driver (see bug #35572.) GMBUS will be
re-enabled later, differently.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Reported-by: Marek Otahal <markotahal@gmail.com>
Tested-by: Yermandu Patapitafious <yermandu.dev@gmail.com>
Tested-by: Andrew Lutomirski <luto@mit.edu>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: David Airlie <airlied@linux.ie>
---
 drivers/gpu/drm/i915/intel_i2c.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- linux-2.6.39.orig/drivers/gpu/drm/i915/intel_i2c.c	2011-05-20 10:42:40.000000000 +0200
+++ linux-2.6.39/drivers/gpu/drm/i915/intel_i2c.c	2011-06-02 16:26:56.000000000 +0200
@@ -401,8 +401,7 @@ int intel_setup_gmbus(struct drm_device
 		bus->reg0 = i | GMBUS_RATE_100KHZ;
 
 		/* XXX force bit banging until GMBUS is fully debugged */
-		if (IS_GEN2(dev))
-			bus->force_bit = intel_gpio_create(dev_priv, i);
+		bus->force_bit = intel_gpio_create(dev_priv, i);
 	}
 
 	intel_i2c_reset(dev_priv->dev);


-- 
Jean Delvare

Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[Florian Mickler]

On Sat, 04 Jun 2011 19:34:56 -0000
Jean Delvare <khali@linux-fr.org> wrote:

> Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
> hang when loading the eeprom driver (see bug #35572.) GMBUS will be
> re-enabled later, differently.
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Reported-by: Marek Otahal <markotahal@gmail.com>
> Tested-by: Yermandu Patapitafious <yermandu.dev@gmail.com>
> Tested-by: Andrew Lutomirski <luto@mit.edu>
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Airlie <airlied@linux.ie>



Hi,

is this[1] resolved some other way in the meantime? 

Regards,
Flo

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=35572

Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[Jean Delvare]

Hi Florian,

On Sat, 11 Jun 2011 13:28:15 +0200, Florian Mickler wrote:
> On Sat, 04 Jun 2011 19:34:56 -0000
> Jean Delvare <khali@linux-fr.org> wrote:
> 
> > Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
> > hang when loading the eeprom driver (see bug #35572.) GMBUS will be
> > re-enabled later, differently.
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > Reported-by: Marek Otahal <markotahal@gmail.com>
> > Tested-by: Yermandu Patapitafious <yermandu.dev@gmail.com>
> > Tested-by: Andrew Lutomirski <luto@mit.edu>
> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: David Airlie <airlied@linux.ie>
> 
> is this[1] resolved some other way in the meantime? 
> 
> Regards,
> Flo
> 
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=35572

Not that I know of (and I don't see any other way at least for 2.6.39.)
This is a shame, really, my revert patch should have been applied
several days ago already.

Keith, Chris, David, can you please get it rolling? This is a
regression presumably affecting a lot of users, we should really fix it
quickly, both in 2.6.39.x and 3.0-rc.

-- 
Jean Delvare

Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[Dave Airlie]

On Sat, Jun 11, 2011 at 10:58 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Florian,
>
> On Sat, 11 Jun 2011 13:28:15 +0200, Florian Mickler wrote:
>> On Sat, 04 Jun 2011 19:34:56 -0000
>> Jean Delvare <khali@linux-fr.org> wrote:
>>
>> > Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
>> > hang when loading the eeprom driver (see bug #35572.) GMBUS will be
>> > re-enabled later, differently.
>> >
>> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
>> > Reported-by: Marek Otahal <markotahal@gmail.com>
>> > Tested-by: Yermandu Patapitafious <yermandu.dev@gmail.com>
>> > Tested-by: Andrew Lutomirski <luto@mit.edu>
>> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: David Airlie <airlied@linux.ie>
>>
>> is this[1] resolved some other way in the meantime?
>>
>> Regards,
>> Flo
>>
>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=35572
>
> Not that I know of (and I don't see any other way at least for 2.6.39.)
> This is a shame, really, my revert patch should have been applied
> several days ago already.
>
> Keith, Chris, David, can you please get it rolling? This is a
> regression presumably affecting a lot of users, we should really fix it
> quickly, both in 2.6.39.x and 3.0-rc.

This patch really had no info other than the bug link to tell me wtf its doing,

I actually don't think reverting this is the correct fix, since it looks like
the code path thats screws with the mutex still happens on GEN2 machines
which unlucky for them.

Chris, also I don't see an ack anywhere on the list, only some
discussion in the bug,
I suspect the correct fix is to remove all the offending code instead
of just putting back the piece
of plaster that fell out of the wall.

Dave.

Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[Jean Delvare]

Hi Dave,

On Tue, 14 Jun 2011 13:39:35 +1000, Dave Airlie wrote:
> On Sat, Jun 11, 2011 at 10:58 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi Florian,
> >
> > On Sat, 11 Jun 2011 13:28:15 +0200, Florian Mickler wrote:
> >> On Sat, 04 Jun 2011 19:34:56 -0000
> >> Jean Delvare <khali@linux-fr.org> wrote:
> >>
> >> > Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
> >> > hang when loading the eeprom driver (see bug #35572.) GMBUS will be
> >> > re-enabled later, differently.
> >> >
> >> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> >> > Reported-by: Marek Otahal <markotahal@gmail.com>
> >> > Tested-by: Yermandu Patapitafious <yermandu.dev@gmail.com>
> >> > Tested-by: Andrew Lutomirski <luto@mit.edu>
> >> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Cc: David Airlie <airlied@linux.ie>
> >>
> >> is this[1] resolved some other way in the meantime?
> >>
> >> Regards,
> >> Flo
> >>
> >> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=35572
> >
> > Not that I know of (and I don't see any other way at least for 2.6.39.)
> > This is a shame, really, my revert patch should have been applied
> > several days ago already.
> >
> > Keith, Chris, David, can you please get it rolling? This is a
> > regression presumably affecting a lot of users, we should really fix it
> > quickly, both in 2.6.39.x and 3.0-rc.
> 
> This patch really had no info other than the bug link to tell me wtf its doing,

The patch I sent on June 4th (Message-ID:
<20110604213456.7ac5588e@endymion.delvare>) says:

Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
hang when loading the eeprom driver (see bug #35572.) GMBUS will be
re-enabled later, differently.

Seems clear enough to me.

> I actually don't think reverting this is the correct fix, since it looks like
> the code path thats screws with the mutex still happens on GEN2 machines
> which unlucky for them.

No. Without the revert, the problem happens on every chip except GEN2.
With the revert, the problems shouldn't happen on any chip. At least
this is how I understand the code.

Anyway, you may not like the fix, but the fact is that commit
8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f caused a regression, so at
least for kernel 2.6.39, reverting it is the right thing to do. For
3.0.0, either someone comes up with an alternative fix and we can apply
it, or reverting the faulty commit is the way to go too.

We have a known regression affecting many users, we have the fix, let's
please apply it quickly. Further discussions should happen _later_.

> Chris, also I don't see an ack anywhere on the list, only some
> discussion in the bug,

I would indeed love to get a ack from Chris.

> I suspect the correct fix is to remove all the offending code instead
> of just putting back the piece
> of plaster that fell out of the wall.

Which "offending code" are you talking about?

We are fixing a regression here. It has to do with being fast and safe,
not with "the correct fix".

Thanks,
-- 
Jean Delvare

Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[Chris Wilson]

On Thu, 16 Jun 2011 22:11:07 +0200, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Dave,
> 
> On Tue, 14 Jun 2011 13:39:35 +1000, Dave Airlie wrote:
> > Chris, also I don't see an ack anywhere on the list, only some
> > discussion in the bug,
> 
> I would indeed love to get a ack from Chris.

Sure, let me ack it again. Hmm, was the last ack on linux-kernel, not
dri-devel?
 
> > I suspect the correct fix is to remove all the offending code instead
> > of just putting back the piece
> > of plaster that fell out of the wall.
> 
> Which "offending code" are you talking about?

Dave and myself chatted on irc and hopefully we cleared up the confusion
that this patch is indeed just restoring the code to 2.6.39. And that
further works needs to be done before trying GMBUS again.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"

[Dave Airlie]

On Fri, Jun 17, 2011 at 6:11 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Dave,
>
> On Tue, 14 Jun 2011 13:39:35 +1000, Dave Airlie wrote:
>> On Sat, Jun 11, 2011 at 10:58 PM, Jean Delvare <khali@linux-fr.org> wrote:
>> > Hi Florian,
>> >
>> > On Sat, 11 Jun 2011 13:28:15 +0200, Florian Mickler wrote:
>> >> On Sat, 04 Jun 2011 19:34:56 -0000
>> >> Jean Delvare <khali@linux-fr.org> wrote:
>> >>
>> >> > Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
>> >> > hang when loading the eeprom driver (see bug #35572.) GMBUS will be
>> >> > re-enabled later, differently.
>> >> >
>> >> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
>> >> > Reported-by: Marek Otahal <markotahal@gmail.com>
>> >> > Tested-by: Yermandu Patapitafious <yermandu.dev@gmail.com>
>> >> > Tested-by: Andrew Lutomirski <luto@mit.edu>
>> >> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >> > Cc: David Airlie <airlied@linux.ie>
>> >>
>> >> is this[1] resolved some other way in the meantime?
>> >>
>> >> Regards,
>> >> Flo
>> >>
>> >> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=35572
>> >
>> > Not that I know of (and I don't see any other way at least for 2.6.39.)
>> > This is a shame, really, my revert patch should have been applied
>> > several days ago already.
>> >
>> > Keith, Chris, David, can you please get it rolling? This is a
>> > regression presumably affecting a lot of users, we should really fix it
>> > quickly, both in 2.6.39.x and 3.0-rc.
>>
>> This patch really had no info other than the bug link to tell me wtf its doing,
>
> The patch I sent on June 4th (Message-ID:
> <20110604213456.7ac5588e@endymion.delvare>) says:
>
> Revert commit 8f9a3f9b63b8cd3f03be9dc53533f90bd4120e5f. This fixes a
> hang when loading the eeprom driver (see bug #35572.) GMBUS will be
> re-enabled later, differently.
>
> Seems clear enough to me.

Hi Jean,

yeah I talked to Chris and I then spotted I was applying this in the
wrong place when
looking at the code by hand,

Once Chris explained I got it, will push soon.

Dave.