* [PATCH 1/5] drm/i915: redisable VGA when we disable the power well
2013-09-26 23:05 [PATCH 0/5] module_reload fixes Paulo Zanoni
@ 2013-09-26 23:05 ` Paulo Zanoni
2013-10-01 13:47 ` Ville Syrjälä
2013-09-26 23:05 ` [PATCH 2/5] drm/i915: destroy connector sysfs files earlier Paulo Zanoni
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Paulo Zanoni @ 2013-09-26 23:05 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, dri-devel
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
VGA whack-a-mole!
We need VGA to be disabled whenever our driver is working. So even
without reproducible bugs this patch makes sense, but we do have a bug
solved by this patch.
If you boot a Haswell machine with eDP+HDMI, then kill your display
manager and run:
echo 0 > /sys/class/vtconsole/vtcon1/bind
you'll get thousands of "unclaimed register" messages. Notice that
since we have eDP+HDMI, the power well is *enabled* when we run the
command, but if you look at dmesg you'll see that at some point during
the boot we disabled it and then reenabled it. This patch solves this
problem.
I didn't do a deep analysis of the problem, but I guess vgacon gets
seriously confused when it sees that the VGA plane is enabled.
Besides the command above, this problem can also be reproduced by the
"module_reload" test from intel-gpu-tools.
Fixes regression introduced by:
commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
drm/i915: switch disable_power_well default value to 1
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 2 +-
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 665fa8f..065ffed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10232,7 +10232,7 @@ static void intel_init_quirks(struct drm_device *dev)
}
/* Disable the VGA plane that we never use */
-static void i915_disable_vga(struct drm_device *dev)
+void i915_disable_vga(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
u8 sr1;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a17a86a..e63646a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -678,6 +678,7 @@ void
ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
int dotclock);
bool intel_crtc_active(struct drm_crtc *crtc);
+void i915_disable_vga(struct drm_device *dev);
void i915_disable_vga_mem(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2ac1c2f..7a8af95 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5329,6 +5329,12 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
HSW_PWR_WELL_STATE_ENABLED), 20))
DRM_ERROR("Timeout enabling power well\n");
+
+ if (I915_READ(i915_vgacntrl_reg(dev)) !=
+ VGA_DISP_DISABLE) {
+ i915_disable_vga(dev);
+ i915_disable_vga_mem(dev);
+ }
}
} else {
if (enable_requested) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 1/5] drm/i915: redisable VGA when we disable the power well
2013-09-26 23:05 ` [PATCH 1/5] drm/i915: redisable VGA when we disable the power well Paulo Zanoni
@ 2013-10-01 13:47 ` Ville Syrjälä
2013-10-01 18:18 ` Ville Syrjälä
0 siblings, 1 reply; 17+ messages in thread
From: Ville Syrjälä @ 2013-10-01 13:47 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni, dri-devel
On Thu, Sep 26, 2013 at 08:05:58PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> VGA whack-a-mole!
>
> We need VGA to be disabled whenever our driver is working. So even
> without reproducible bugs this patch makes sense, but we do have a bug
> solved by this patch.
>
> If you boot a Haswell machine with eDP+HDMI, then kill your display
> manager and run:
> echo 0 > /sys/class/vtconsole/vtcon1/bind
> you'll get thousands of "unclaimed register" messages. Notice that
> since we have eDP+HDMI, the power well is *enabled* when we run the
> command, but if you look at dmesg you'll see that at some point during
> the boot we disabled it and then reenabled it. This patch solves this
> problem.
>
> I didn't do a deep analysis of the problem, but I guess vgacon gets
> seriously confused when it sees that the VGA plane is enabled.
>
> Besides the command above, this problem can also be reproduced by the
> "module_reload" test from intel-gpu-tools.
>
> Fixes regression introduced by:
> commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> drm/i915: switch disable_power_well default value to 1
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 665fa8f..065ffed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10232,7 +10232,7 @@ static void intel_init_quirks(struct drm_device *dev)
> }
>
> /* Disable the VGA plane that we never use */
> -static void i915_disable_vga(struct drm_device *dev)
> +void i915_disable_vga(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> u8 sr1;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a17a86a..e63646a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -678,6 +678,7 @@ void
> ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
> int dotclock);
> bool intel_crtc_active(struct drm_crtc *crtc);
> +void i915_disable_vga(struct drm_device *dev);
> void i915_disable_vga_mem(struct drm_device *dev);
>
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2ac1c2f..7a8af95 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5329,6 +5329,12 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> HSW_PWR_WELL_STATE_ENABLED), 20))
> DRM_ERROR("Timeout enabling power well\n");
> +
> + if (I915_READ(i915_vgacntrl_reg(dev)) !=
> + VGA_DISP_DISABLE) {
> + i915_disable_vga(dev);
> + i915_disable_vga_mem(dev);
> + }
Hmm. That makes no sense.
So somehow the thrice cursed BIOS re-enabled the VGA plane behind our
back? That really shouldn't cause any problems other than maybe
corrupting the display or something. In any case it has nothing to do
with vga resource decode.
The real problem seems to be that vgacon is poking at vga resources we
apparently decode but can't handle. If your patch really fixes it then
it would seem to BIOS also enabled VGA memory decode. But that still
doesn't explain the unclaimed register errors, as that would require
the power well to be off. But since you say HDMI is enabled, the power
well must be on as well. So all I can say is wtf?
> }
> } else {
> if (enable_requested) {
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/5] drm/i915: redisable VGA when we disable the power well
2013-10-01 13:47 ` Ville Syrjälä
@ 2013-10-01 18:18 ` Ville Syrjälä
2013-11-29 13:36 ` [PATCH] drm/i915: disable VGA mem when reenabling " Paulo Zanoni
0 siblings, 1 reply; 17+ messages in thread
From: Ville Syrjälä @ 2013-10-01 18:18 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni, dri-devel
On Tue, Oct 01, 2013 at 04:47:04PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2013 at 08:05:58PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > VGA whack-a-mole!
> >
> > We need VGA to be disabled whenever our driver is working. So even
> > without reproducible bugs this patch makes sense, but we do have a bug
> > solved by this patch.
> >
> > If you boot a Haswell machine with eDP+HDMI, then kill your display
> > manager and run:
> > echo 0 > /sys/class/vtconsole/vtcon1/bind
> > you'll get thousands of "unclaimed register" messages. Notice that
> > since we have eDP+HDMI, the power well is *enabled* when we run the
> > command, but if you look at dmesg you'll see that at some point during
> > the boot we disabled it and then reenabled it. This patch solves this
> > problem.
> >
> > I didn't do a deep analysis of the problem, but I guess vgacon gets
> > seriously confused when it sees that the VGA plane is enabled.
> >
> > Besides the command above, this problem can also be reproduced by the
> > "module_reload" test from intel-gpu-tools.
> >
> > Fixes regression introduced by:
> > commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > drm/i915: switch disable_power_well default value to 1
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> > 3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 665fa8f..065ffed 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10232,7 +10232,7 @@ static void intel_init_quirks(struct drm_device *dev)
> > }
> >
> > /* Disable the VGA plane that we never use */
> > -static void i915_disable_vga(struct drm_device *dev)
> > +void i915_disable_vga(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u8 sr1;
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a17a86a..e63646a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -678,6 +678,7 @@ void
> > ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
> > int dotclock);
> > bool intel_crtc_active(struct drm_crtc *crtc);
> > +void i915_disable_vga(struct drm_device *dev);
> > void i915_disable_vga_mem(struct drm_device *dev);
> >
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 2ac1c2f..7a8af95 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5329,6 +5329,12 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> > if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> > HSW_PWR_WELL_STATE_ENABLED), 20))
> > DRM_ERROR("Timeout enabling power well\n");
> > +
> > + if (I915_READ(i915_vgacntrl_reg(dev)) !=
> > + VGA_DISP_DISABLE) {
> > + i915_disable_vga(dev);
> > + i915_disable_vga_mem(dev);
> > + }
>
> Hmm. That makes no sense.
>
> So somehow the thrice cursed BIOS re-enabled the VGA plane behind our
> back? That really shouldn't cause any problems other than maybe
> corrupting the display or something. In any case it has nothing to do
> with vga resource decode.
>
> The real problem seems to be that vgacon is poking at vga resources we
> apparently decode but can't handle. If your patch really fixes it then
> it would seem to BIOS also enabled VGA memory decode. But that still
> doesn't explain the unclaimed register errors, as that would require
> the power well to be off. But since you say HDMI is enabled, the power
> well must be on as well. So all I can say is wtf?
Actually I tried it on my HSW, and the vga_cntrl reg always comes back
as 0x80002900 after the power well has been off, which is the reset
value. So your code w/ the '!=' comparoson would assume that the VGA plane
is enabled even though it might not be. We should fix all the VGA plane
enabled checks to use '&' instead of '!='.
But if your patch actually fixes things, then I guess the BIOS really
must have re-enabled vga memory decode and that's where the errors
are coming from. Otherwise if it's just about vga registers, then
redisabling vga memory decode and vga plane should have no effect on
the outcome.
One idea would be to lift the lid_notify hack from intel_lvds.c and plug
it into eDP and see if that makes a difference...
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH] drm/i915: disable VGA mem when reenabling the power well
2013-10-01 18:18 ` Ville Syrjälä
@ 2013-11-29 13:36 ` Paulo Zanoni
2013-12-11 17:44 ` Paulo Zanoni
0 siblings, 1 reply; 17+ messages in thread
From: Paulo Zanoni @ 2013-11-29 13:36 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Fixes regression introduced by:
commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
Date: Wed Jul 3 17:12:13 2013 -0300
drm/i915: switch disable_power_well default value to 1
The bug I'm seeing can be reproduced with:
- Boot your Haswell machine
- Make sure the power well gets disabled, then enabled. You can check
this by seeing the messages print by hsw_set_power_well
- Stop your display manager
- echo 0 > /sys/class/vtconsole/vtcon1/bind
I can easily reproduce this by blacklising snd_hda_intel and booting
with eDP+HDMI.
If you do this and then look at dmesg, you'll see we're printing
infinite "Unclaimed register" messages. This is happening because
we're stuck on an infinite loop inside console_unlock(), which is
calling many functions from vgacon.c. And the code that's triggering
the error messages is from vgacon_set_cursor_size().
For some reason, what fixes the problem is the code that clears
VGA_MSR_MEM_EN from VGA_MSR_WRITE. If you add some debug messages,
you'll see that we're reading a value of 0 and writing 0 back to it. I
really don't know why this fixes the problem, but I am sure that if
you remove the code line that writes 0 back the bug won't stop. I
suspect this is probably some problem with how the hardware gets
reinitialized when we reenable the power well.
Also notice that function i915_disable_vga_mem already existed and was
removed on commit 6e1b4fdad5157bb9e88777d525704aba24389bee, I just
brought it back. Notice that the intel_drv.h declaration was still
there too.
I know some people will probably request to deeply investigate why
exactly vgacon is doing the wrong thing, and ask me to replace the
"for (;;)" that exists inside console_unlock() with something else,
but my goal here is to just fix the regression. I don't think it's
worth spending that much time on vgacon code. And this is our
regression anyway.
Notice that this is just a partial fix to fd.o #67813. This fixes the
case where the power well is already enabled when we unbind, not when
it's disabled when we unbind.
V2: - Rebase (first version was sent in September).
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++
drivers/gpu/drm/i915/intel_pm.c | 2 ++
2 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 45a87d1..bf22bea 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11064,6 +11064,18 @@ void i915_redisable_vga(struct drm_device *dev)
}
}
+void i915_disable_vga_mem(struct drm_device *dev)
+{
+ if (HAS_PCH_SPLIT(dev)) {
+ vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
+ outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
+ vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
+ VGA_RSRC_NORMAL_IO |
+ VGA_RSRC_NORMAL_MEM);
+ vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
+ }
+}
+
static void intel_modeset_readout_hw_state(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e6d98fe..7ebe1bd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5703,6 +5703,8 @@ static void hsw_set_power_well(struct drm_device *dev,
if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
HSW_PWR_WELL_STATE_ENABLED), 20))
DRM_ERROR("Timeout enabling power well\n");
+
+ i915_disable_vga_mem(dev);
}
if (IS_BROADWELL(dev)) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH] drm/i915: disable VGA mem when reenabling the power well
2013-11-29 13:36 ` [PATCH] drm/i915: disable VGA mem when reenabling " Paulo Zanoni
@ 2013-12-11 17:44 ` Paulo Zanoni
2013-12-11 18:39 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Paulo Zanoni @ 2013-12-11 17:44 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Paulo Zanoni
Hi
2013/11/29 Paulo Zanoni <przanoni@gmail.com>:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Fixes regression introduced by:
> commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
> Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Date: Wed Jul 3 17:12:13 2013 -0300
> drm/i915: switch disable_power_well default value to 1
>
> The bug I'm seeing can be reproduced with:
> - Boot your Haswell machine
> - Make sure the power well gets disabled, then enabled. You can check
> this by seeing the messages print by hsw_set_power_well
> - Stop your display manager
> - echo 0 > /sys/class/vtconsole/vtcon1/bind
>
> I can easily reproduce this by blacklising snd_hda_intel and booting
> with eDP+HDMI.
>
> If you do this and then look at dmesg, you'll see we're printing
> infinite "Unclaimed register" messages. This is happening because
> we're stuck on an infinite loop inside console_unlock(), which is
> calling many functions from vgacon.c. And the code that's triggering
> the error messages is from vgacon_set_cursor_size().
>
> For some reason, what fixes the problem is the code that clears
> VGA_MSR_MEM_EN from VGA_MSR_WRITE. If you add some debug messages,
> you'll see that we're reading a value of 0 and writing 0 back to it. I
> really don't know why this fixes the problem, but I am sure that if
> you remove the code line that writes 0 back the bug won't stop. I
> suspect this is probably some problem with how the hardware gets
> reinitialized when we reenable the power well.
Due to even more questioning, I decided to investigate more.
After we re-enable the power well, every time we read/write the VGA
address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
print error messages. If we write anything to the VGA MSR register (it
doesn't really matter which value you write to bit 0), any
reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
anymore (even if MSR bit 0 is zero). So what happens with the current
code is that when we unbind i915 and bind vgacon, we call
console_unlock(). Function console_unlock() is responsible for
printing any messages that were supposed to be print when the console
was locked, so it calls the TTY layer, which calls the console layer,
which calls vgacon to print the messages. At this point, vgacon
eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
triggers unclaimed register interrupts. The problem is that when we
get these interrupts, we print the error messages, so we add more work
to console_unlock(), which will try to print it again, and then call
vgacon again, trigger a new interrupt, which will put more stuff to
the buffer, and then we'll be stuck at console_unlock() forever.
If you patch intel_uncore.c to not print anything when we detect
unclaimed registers, we won't get into the console_unlock() infinite
loop and the driver unbind will work just fine. We will still be
getting interrupts every time vgacon touches those registers, but we
will survive. This is a valid experiment, but IMHO it's not the real
fix: if we don't print any error messages we will still keep getting
the interrupts, and if we disable ERR_INT we won't get the interrupt
anymore, but we will also stop getting all the other error interrupts.
I talked about this problem with the HW engineer and his
recommendation is "So don't do any VGA I/O or memory access while the
power well is disabled, and make to re-program MSR after enabling the
power well and before using VGA I/O or memory accesses.".
Based on this, I could write an even-smaller patch that just touches
the MSR register to avoid the problem, also providing a nice comment
and an improved commit message. Or I could also write another
implementation that actually saves the value of MSR before we disable
the power well, then restores the correct value after we re-enable it:
but that wouldn't help vgacon too much. IMHO one of these new patches
would be the appropriate way to solve the problem, but it seems that
Daniel/Ville didn't like the previous patches, so I'm not sure they
will be willing to accept the next version either. Since so far I
haven't seen any alternative implementation/suggestion that actually
fixes the problem, and since module_reload is not working on Haswell
for months (possibly hiding other module_reload bugs), my vote is
obviously to accept the next patch because at least it fixes the
6-months-old regression. But if I don't get an "ack" on this I won't
spend my time working on this. What do you think?
Please also notice that this only fixes the case where the power well
is already enabled when we unbind. The fix for when the power well is
_disabled_ when we unbind depends on this, and IMHO we should discuss
it later.
Thanks,
Paulo
>
> Also notice that function i915_disable_vga_mem already existed and was
> removed on commit 6e1b4fdad5157bb9e88777d525704aba24389bee, I just
> brought it back. Notice that the intel_drv.h declaration was still
> there too.
>
> I know some people will probably request to deeply investigate why
> exactly vgacon is doing the wrong thing, and ask me to replace the
> "for (;;)" that exists inside console_unlock() with something else,
> but my goal here is to just fix the regression. I don't think it's
> worth spending that much time on vgacon code. And this is our
> regression anyway.
>
> Notice that this is just a partial fix to fd.o #67813. This fixes the
> case where the power well is already enabled when we unbind, not when
> it's disabled when we unbind.
>
> V2: - Rebase (first version was sent in September).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++
> drivers/gpu/drm/i915/intel_pm.c | 2 ++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 45a87d1..bf22bea 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11064,6 +11064,18 @@ void i915_redisable_vga(struct drm_device *dev)
> }
> }
>
> +void i915_disable_vga_mem(struct drm_device *dev)
> +{
> + if (HAS_PCH_SPLIT(dev)) {
> + vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> + outb(inb(VGA_MSR_READ) & ~VGA_MSR_MEM_EN, VGA_MSR_WRITE);
> + vga_set_legacy_decoding(dev->pdev, VGA_RSRC_LEGACY_IO |
> + VGA_RSRC_NORMAL_IO |
> + VGA_RSRC_NORMAL_MEM);
> + vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> + }
> +}
> +
> static void intel_modeset_readout_hw_state(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e6d98fe..7ebe1bd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5703,6 +5703,8 @@ static void hsw_set_power_well(struct drm_device *dev,
> if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) &
> HSW_PWR_WELL_STATE_ENABLED), 20))
> DRM_ERROR("Timeout enabling power well\n");
> +
> + i915_disable_vga_mem(dev);
> }
>
> if (IS_BROADWELL(dev)) {
> --
> 1.8.3.1
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] drm/i915: disable VGA mem when reenabling the power well
2013-12-11 17:44 ` Paulo Zanoni
@ 2013-12-11 18:39 ` Daniel Vetter
2013-12-11 18:50 ` Paulo Zanoni
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Vetter @ 2013-12-11 18:39 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni
On Wed, Dec 11, 2013 at 6:44 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>
> After we re-enable the power well, every time we read/write the VGA
> address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
> print error messages. If we write anything to the VGA MSR register (it
> doesn't really matter which value you write to bit 0), any
> reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
> anymore (even if MSR bit 0 is zero). So what happens with the current
> code is that when we unbind i915 and bind vgacon, we call
> console_unlock(). Function console_unlock() is responsible for
> printing any messages that were supposed to be print when the console
> was locked, so it calls the TTY layer, which calls the console layer,
> which calls vgacon to print the messages. At this point, vgacon
> eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
> triggers unclaimed register interrupts. The problem is that when we
> get these interrupts, we print the error messages, so we add more work
> to console_unlock(), which will try to print it again, and then call
> vgacon again, trigger a new interrupt, which will put more stuff to
> the buffer, and then we'll be stuck at console_unlock() forever.
>
> If you patch intel_uncore.c to not print anything when we detect
> unclaimed registers, we won't get into the console_unlock() infinite
> loop and the driver unbind will work just fine. We will still be
> getting interrupts every time vgacon touches those registers, but we
> will survive. This is a valid experiment, but IMHO it's not the real
> fix: if we don't print any error messages we will still keep getting
> the interrupts, and if we disable ERR_INT we won't get the interrupt
> anymore, but we will also stop getting all the other error interrupts.
>
> I talked about this problem with the HW engineer and his
> recommendation is "So don't do any VGA I/O or memory access while the
> power well is disabled, and make to re-program MSR after enabling the
> power well and before using VGA I/O or memory accesses.".
Impressive piece of debugging. And a great story ;-)
> Based on this, I could write an even-smaller patch that just touches
> the MSR register to avoid the problem, also providing a nice comment
> and an improved commit message. Or I could also write another
> implementation that actually saves the value of MSR before we disable
> the power well, then restores the correct value after we re-enable it:
> but that wouldn't help vgacon too much. IMHO one of these new patches
> would be the appropriate way to solve the problem, but it seems that
> Daniel/Ville didn't like the previous patches, so I'm not sure they
> will be willing to accept the next version either. Since so far I
> haven't seen any alternative implementation/suggestion that actually
> fixes the problem, and since module_reload is not working on Haswell
> for months (possibly hiding other module_reload bugs), my vote is
> obviously to accept the next patch because at least it fixes the
> 6-months-old regression. But if I don't get an "ack" on this I won't
> spend my time working on this. What do you think?
I'd go with re-disabling vga unconditionally through the msr. We don't
bother with restoring the vga plane anyway at module unload, so vga is
hosed no matter what. And it sounds like this is the more minimal fix.
So ack for the minimal fix to just poke the MSR at the right times.
> Please also notice that this only fixes the case where the power well
> is already enabled when we unbind. The fix for when the power well is
> _disabled_ when we unbind depends on this, and IMHO we should discuss
> it later.
I guess I need to resurrect my "kill vgacon" patch and figure out why
it doesn't work. Or could we just force-enable the power well when we
start our module unload sequence instead? I'd prefer the most minimal
fix, and even better if we don't have to touch the vgacon hell at all
...
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: disable VGA mem when reenabling the power well
2013-12-11 18:39 ` Daniel Vetter
@ 2013-12-11 18:50 ` Paulo Zanoni
2013-12-11 19:02 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Paulo Zanoni @ 2013-12-11 18:50 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Paulo Zanoni
2013/12/11 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Dec 11, 2013 at 6:44 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>
>> After we re-enable the power well, every time we read/write the VGA
>> address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
>> print error messages. If we write anything to the VGA MSR register (it
>> doesn't really matter which value you write to bit 0), any
>> reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
>> anymore (even if MSR bit 0 is zero). So what happens with the current
>> code is that when we unbind i915 and bind vgacon, we call
>> console_unlock(). Function console_unlock() is responsible for
>> printing any messages that were supposed to be print when the console
>> was locked, so it calls the TTY layer, which calls the console layer,
>> which calls vgacon to print the messages. At this point, vgacon
>> eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
>> triggers unclaimed register interrupts. The problem is that when we
>> get these interrupts, we print the error messages, so we add more work
>> to console_unlock(), which will try to print it again, and then call
>> vgacon again, trigger a new interrupt, which will put more stuff to
>> the buffer, and then we'll be stuck at console_unlock() forever.
>>
>> If you patch intel_uncore.c to not print anything when we detect
>> unclaimed registers, we won't get into the console_unlock() infinite
>> loop and the driver unbind will work just fine. We will still be
>> getting interrupts every time vgacon touches those registers, but we
>> will survive. This is a valid experiment, but IMHO it's not the real
>> fix: if we don't print any error messages we will still keep getting
>> the interrupts, and if we disable ERR_INT we won't get the interrupt
>> anymore, but we will also stop getting all the other error interrupts.
>>
>> I talked about this problem with the HW engineer and his
>> recommendation is "So don't do any VGA I/O or memory access while the
>> power well is disabled, and make to re-program MSR after enabling the
>> power well and before using VGA I/O or memory accesses.".
>
> Impressive piece of debugging. And a great story ;-)
>
>> Based on this, I could write an even-smaller patch that just touches
>> the MSR register to avoid the problem, also providing a nice comment
>> and an improved commit message. Or I could also write another
>> implementation that actually saves the value of MSR before we disable
>> the power well, then restores the correct value after we re-enable it:
>> but that wouldn't help vgacon too much. IMHO one of these new patches
>> would be the appropriate way to solve the problem, but it seems that
>> Daniel/Ville didn't like the previous patches, so I'm not sure they
>> will be willing to accept the next version either. Since so far I
>> haven't seen any alternative implementation/suggestion that actually
>> fixes the problem, and since module_reload is not working on Haswell
>> for months (possibly hiding other module_reload bugs), my vote is
>> obviously to accept the next patch because at least it fixes the
>> 6-months-old regression. But if I don't get an "ack" on this I won't
>> spend my time working on this. What do you think?
>
> I'd go with re-disabling vga unconditionally through the msr. We don't
> bother with restoring the vga plane anyway at module unload, so vga is
> hosed no matter what. And it sounds like this is the more minimal fix.
> So ack for the minimal fix to just poke the MSR at the right times.
Just to be clear: by "at the right times" you mean "when re-enabling
the power well", right?
>
>> Please also notice that this only fixes the case where the power well
>> is already enabled when we unbind. The fix for when the power well is
>> _disabled_ when we unbind depends on this, and IMHO we should discuss
>> it later.
>
> I guess I need to resurrect my "kill vgacon" patch and figure out why
> it doesn't work.
See comment #45 of the bug report for an explanation on how it fails.
I'm not sure if it's possible to do what you want.
> Or could we just force-enable the power well when we
> start our module unload sequence instead?
As I mentioned before, driver unbind (a requirement for module unload)
happens before we call any of the unload functions, so we never even
get to run i915_driver_unload.
> I'd prefer the most minimal
> fix, and even better if we don't have to touch the vgacon hell at all
Me too :)
> ...
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: disable VGA mem when reenabling the power well
2013-12-11 18:50 ` Paulo Zanoni
@ 2013-12-11 19:02 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2013-12-11 19:02 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni
On Wed, Dec 11, 2013 at 7:50 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/12/11 Daniel Vetter <daniel@ffwll.ch>:
>> On Wed, Dec 11, 2013 at 6:44 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>>>
>>> After we re-enable the power well, every time we read/write the VGA
>>> address 0x3d5 we get an "unclaimed register" interrupt (ERR_INT) and
>>> print error messages. If we write anything to the VGA MSR register (it
>>> doesn't really matter which value you write to bit 0), any
>>> reads/writes to 0x3d5 _don't_ trigger the "unclaimed register" errors
>>> anymore (even if MSR bit 0 is zero). So what happens with the current
>>> code is that when we unbind i915 and bind vgacon, we call
>>> console_unlock(). Function console_unlock() is responsible for
>>> printing any messages that were supposed to be print when the console
>>> was locked, so it calls the TTY layer, which calls the console layer,
>>> which calls vgacon to print the messages. At this point, vgacon
>>> eventually calls vgacon_set_cursor_size(), which touches 0x3d5, which
>>> triggers unclaimed register interrupts. The problem is that when we
>>> get these interrupts, we print the error messages, so we add more work
>>> to console_unlock(), which will try to print it again, and then call
>>> vgacon again, trigger a new interrupt, which will put more stuff to
>>> the buffer, and then we'll be stuck at console_unlock() forever.
>>>
>>> If you patch intel_uncore.c to not print anything when we detect
>>> unclaimed registers, we won't get into the console_unlock() infinite
>>> loop and the driver unbind will work just fine. We will still be
>>> getting interrupts every time vgacon touches those registers, but we
>>> will survive. This is a valid experiment, but IMHO it's not the real
>>> fix: if we don't print any error messages we will still keep getting
>>> the interrupts, and if we disable ERR_INT we won't get the interrupt
>>> anymore, but we will also stop getting all the other error interrupts.
>>>
>>> I talked about this problem with the HW engineer and his
>>> recommendation is "So don't do any VGA I/O or memory access while the
>>> power well is disabled, and make to re-program MSR after enabling the
>>> power well and before using VGA I/O or memory accesses.".
>>
>> Impressive piece of debugging. And a great story ;-)
>>
>>> Based on this, I could write an even-smaller patch that just touches
>>> the MSR register to avoid the problem, also providing a nice comment
>>> and an improved commit message. Or I could also write another
>>> implementation that actually saves the value of MSR before we disable
>>> the power well, then restores the correct value after we re-enable it:
>>> but that wouldn't help vgacon too much. IMHO one of these new patches
>>> would be the appropriate way to solve the problem, but it seems that
>>> Daniel/Ville didn't like the previous patches, so I'm not sure they
>>> will be willing to accept the next version either. Since so far I
>>> haven't seen any alternative implementation/suggestion that actually
>>> fixes the problem, and since module_reload is not working on Haswell
>>> for months (possibly hiding other module_reload bugs), my vote is
>>> obviously to accept the next patch because at least it fixes the
>>> 6-months-old regression. But if I don't get an "ack" on this I won't
>>> spend my time working on this. What do you think?
>>
>> I'd go with re-disabling vga unconditionally through the msr. We don't
>> bother with restoring the vga plane anyway at module unload, so vga is
>> hosed no matter what. And it sounds like this is the more minimal fix.
>> So ack for the minimal fix to just poke the MSR at the right times.
>
> Just to be clear: by "at the right times" you mean "when re-enabling
> the power well", right?
Yup.
>>> Please also notice that this only fixes the case where the power well
>>> is already enabled when we unbind. The fix for when the power well is
>>> _disabled_ when we unbind depends on this, and IMHO we should discuss
>>> it later.
>>
>> I guess I need to resurrect my "kill vgacon" patch and figure out why
>> it doesn't work.
>
> See comment #45 of the bug report for an explanation on how it fails.
> I'm not sure if it's possible to do what you want.
>
>
>> Or could we just force-enable the power well when we
>> start our module unload sequence instead?
>
> As I mentioned before, driver unbind (a requirement for module unload)
> happens before we call any of the unload functions, so we never even
> get to run i915_driver_unload.
Meh, I always forget that. And thanks to the awesome ownership model
in fbcon we can't use anything existing to pluck that power well
enabling code in. I hate this code ... So I guess preventing vgacon
from ever reloading on our systems would be the right solution then
for the case where the power well is down at console unbind time. Or
just CONFIG_FB=n ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/5] drm/i915: destroy connector sysfs files earlier
2013-09-26 23:05 [PATCH 0/5] module_reload fixes Paulo Zanoni
2013-09-26 23:05 ` [PATCH 1/5] drm/i915: redisable VGA when we disable the power well Paulo Zanoni
@ 2013-09-26 23:05 ` Paulo Zanoni
2013-09-30 21:10 ` Daniel Vetter
2013-09-26 23:06 ` [PATCH 3/5] tty/vt: add con_bind and con_unbind functions Paulo Zanoni
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Paulo Zanoni @ 2013-09-26 23:05 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, dri-devel
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
For some reason, every single time I try to run module_reload
something tries to read the connector sysfs files. This happens
after we destroy the encoders and before we destroy the connectors, so
when the sysfs read triggers the connector detect() function,
intel_conector->encoder points to memory that was already freed.
The bad backtrace is just:
[<ffffffff8163ca9a>] dump_stack+0x54/0x74
[<ffffffffa00c2c8e>] intel_dp_detect+0x1e/0x4b0 [i915]
[<ffffffffa001913d>] status_show+0x3d/0x80 [drm]
[<ffffffff813d5340>] dev_attr_show+0x20/0x60
[<ffffffff81221f50>] ? sysfs_read_file+0x80/0x1b0
[<ffffffff81221f79>] sysfs_read_file+0xa9/0x1b0
[<ffffffff811aaf1e>] vfs_read+0x9e/0x170
[<ffffffff811aba4c>] SyS_read+0x4c/0xa0
[<ffffffff8164e392>] system_call_fastpath+0x16/0x1b
But if you add tons of memory checking debug options to your Kernel
you'll also see:
- general protection fault: 0000
- BUG kmalloc-4096 (Tainted: G D W ): Poison overwritten
- INFO: Allocated in intel_ddi_init+0x65/0x270 [i915]
- INFO: Freed in intel_dp_encoder_destroy+0x69/0xb0 [i915]
Among a bunch of other error messages.
So this commit just destroys the sysfs files before both the encoder
and connectors are freed.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_crt.c | 1 -
drivers/gpu/drm/i915/intel_display.c | 5 +++++
drivers/gpu/drm/i915/intel_dp.c | 1 -
drivers/gpu/drm/i915/intel_dsi.c | 1 -
drivers/gpu/drm/i915/intel_dvo.c | 1 -
drivers/gpu/drm/i915/intel_hdmi.c | 1 -
drivers/gpu/drm/i915/intel_lvds.c | 1 -
drivers/gpu/drm/i915/intel_sdvo.c | 7 +++++--
drivers/gpu/drm/i915/intel_tv.c | 1 -
9 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 0263629..942b9ac 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -677,7 +677,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
static void intel_crt_destroy(struct drm_connector *connector)
{
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(connector);
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 065ffed..793061f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10726,6 +10726,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_crtc *crtc;
+ struct drm_connector *connector;
/*
* Interrupts and polling as the first thing to avoid creating havoc.
@@ -10768,6 +10769,10 @@ void intel_modeset_cleanup(struct drm_device *dev)
/* destroy backlight, if any, before the connectors */
intel_panel_destroy_backlight(dev);
+ /* destroy the sysfs files before encoders/connectors */
+ list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+ drm_sysfs_connector_remove(connector);
+
drm_mode_config_cleanup(dev);
intel_cleanup_overlay(dev);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 95a3159..7bdbb36 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3139,7 +3139,6 @@ intel_dp_connector_destroy(struct drm_connector *connector)
if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
intel_panel_fini(&intel_connector->panel);
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(connector);
}
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 674fd49..9a2fdd2 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -504,7 +504,6 @@ static void intel_dsi_destroy(struct drm_connector *connector)
DRM_DEBUG_KMS("\n");
intel_panel_fini(&intel_connector->panel);
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(connector);
}
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 91287d1..1b64145 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -367,7 +367,6 @@ static int intel_dvo_get_modes(struct drm_connector *connector)
static void intel_dvo_destroy(struct drm_connector *connector)
{
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(connector);
}
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 6004f9c..4f4d346 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1181,7 +1181,6 @@ static void intel_hdmi_post_disable(struct intel_encoder *encoder)
static void intel_hdmi_destroy(struct drm_connector *connector)
{
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(connector);
}
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index fb3f8ef..ae0c843 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -474,7 +474,6 @@ static void intel_lvds_destroy(struct drm_connector *connector)
intel_panel_fini(&lvds_connector->base.panel);
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(connector);
}
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5e59d64..a583e8f 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2009,7 +2009,6 @@ static void intel_sdvo_destroy(struct drm_connector *connector)
intel_sdvo_connector->tv_format);
intel_sdvo_destroy_enhance_property(connector);
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(intel_sdvo_connector);
}
@@ -2482,6 +2481,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
return true;
err:
+ drm_sysfs_connector_remove(connector);
intel_sdvo_destroy(connector);
return false;
}
@@ -2553,6 +2553,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
return true;
err:
+ drm_sysfs_connector_remove(connector);
intel_sdvo_destroy(connector);
return false;
}
@@ -2624,8 +2625,10 @@ static void intel_sdvo_output_cleanup(struct intel_sdvo *intel_sdvo)
list_for_each_entry_safe(connector, tmp,
&dev->mode_config.connector_list, head) {
- if (intel_attached_encoder(connector) == &intel_sdvo->base)
+ if (intel_attached_encoder(connector) == &intel_sdvo->base) {
+ drm_sysfs_connector_remove(connector);
intel_sdvo_destroy(connector);
+ }
}
}
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 75925a1..92895f9 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1433,7 +1433,6 @@ intel_tv_get_modes(struct drm_connector *connector)
static void
intel_tv_destroy(struct drm_connector *connector)
{
- drm_sysfs_connector_remove(connector);
drm_connector_cleanup(connector);
kfree(connector);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/5] drm/i915: destroy connector sysfs files earlier
2013-09-26 23:05 ` [PATCH 2/5] drm/i915: destroy connector sysfs files earlier Paulo Zanoni
@ 2013-09-30 21:10 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2013-09-30 21:10 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni, dri-devel
On Thu, Sep 26, 2013 at 08:05:59PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> For some reason, every single time I try to run module_reload
> something tries to read the connector sysfs files. This happens
> after we destroy the encoders and before we destroy the connectors, so
> when the sysfs read triggers the connector detect() function,
> intel_conector->encoder points to memory that was already freed.
>
> The bad backtrace is just:
> [<ffffffff8163ca9a>] dump_stack+0x54/0x74
> [<ffffffffa00c2c8e>] intel_dp_detect+0x1e/0x4b0 [i915]
> [<ffffffffa001913d>] status_show+0x3d/0x80 [drm]
> [<ffffffff813d5340>] dev_attr_show+0x20/0x60
> [<ffffffff81221f50>] ? sysfs_read_file+0x80/0x1b0
> [<ffffffff81221f79>] sysfs_read_file+0xa9/0x1b0
> [<ffffffff811aaf1e>] vfs_read+0x9e/0x170
> [<ffffffff811aba4c>] SyS_read+0x4c/0xa0
> [<ffffffff8164e392>] system_call_fastpath+0x16/0x1b
>
> But if you add tons of memory checking debug options to your Kernel
> you'll also see:
> - general protection fault: 0000
> - BUG kmalloc-4096 (Tainted: G D W ): Poison overwritten
> - INFO: Allocated in intel_ddi_init+0x65/0x270 [i915]
> - INFO: Freed in intel_dp_encoder_destroy+0x69/0xb0 [i915]
> Among a bunch of other error messages.
>
> So this commit just destroys the sysfs files before both the encoder
> and connectors are freed.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch. I'm unsure what to do with the
first patch, maybe Ville can take a look. For the next three patches to
wire up bind/unbind hooks I think the right approach would be to just
disallow the vgaconsole once drm/i915 is loaded:
- We don't want to even tighter integrate with the locking madness that is
fbcon.
- We want to kick out the vgacon anyway, even when e.g. fbcon isn't
loaded.
David Herrmann might have an idea how to solve this.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_crt.c | 1 -
> drivers/gpu/drm/i915/intel_display.c | 5 +++++
> drivers/gpu/drm/i915/intel_dp.c | 1 -
> drivers/gpu/drm/i915/intel_dsi.c | 1 -
> drivers/gpu/drm/i915/intel_dvo.c | 1 -
> drivers/gpu/drm/i915/intel_hdmi.c | 1 -
> drivers/gpu/drm/i915/intel_lvds.c | 1 -
> drivers/gpu/drm/i915/intel_sdvo.c | 7 +++++--
> drivers/gpu/drm/i915/intel_tv.c | 1 -
> 9 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 0263629..942b9ac 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -677,7 +677,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>
> static void intel_crt_destroy(struct drm_connector *connector)
> {
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 065ffed..793061f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10726,6 +10726,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> + struct drm_connector *connector;
>
> /*
> * Interrupts and polling as the first thing to avoid creating havoc.
> @@ -10768,6 +10769,10 @@ void intel_modeset_cleanup(struct drm_device *dev)
> /* destroy backlight, if any, before the connectors */
> intel_panel_destroy_backlight(dev);
>
> + /* destroy the sysfs files before encoders/connectors */
> + list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> + drm_sysfs_connector_remove(connector);
> +
> drm_mode_config_cleanup(dev);
>
> intel_cleanup_overlay(dev);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 95a3159..7bdbb36 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3139,7 +3139,6 @@ intel_dp_connector_destroy(struct drm_connector *connector)
> if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> intel_panel_fini(&intel_connector->panel);
>
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 674fd49..9a2fdd2 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -504,7 +504,6 @@ static void intel_dsi_destroy(struct drm_connector *connector)
>
> DRM_DEBUG_KMS("\n");
> intel_panel_fini(&intel_connector->panel);
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 91287d1..1b64145 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -367,7 +367,6 @@ static int intel_dvo_get_modes(struct drm_connector *connector)
>
> static void intel_dvo_destroy(struct drm_connector *connector)
> {
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 6004f9c..4f4d346 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1181,7 +1181,6 @@ static void intel_hdmi_post_disable(struct intel_encoder *encoder)
>
> static void intel_hdmi_destroy(struct drm_connector *connector)
> {
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index fb3f8ef..ae0c843 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -474,7 +474,6 @@ static void intel_lvds_destroy(struct drm_connector *connector)
>
> intel_panel_fini(&lvds_connector->base.panel);
>
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 5e59d64..a583e8f 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -2009,7 +2009,6 @@ static void intel_sdvo_destroy(struct drm_connector *connector)
> intel_sdvo_connector->tv_format);
>
> intel_sdvo_destroy_enhance_property(connector);
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(intel_sdvo_connector);
> }
> @@ -2482,6 +2481,7 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> return true;
>
> err:
> + drm_sysfs_connector_remove(connector);
> intel_sdvo_destroy(connector);
> return false;
> }
> @@ -2553,6 +2553,7 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> return true;
>
> err:
> + drm_sysfs_connector_remove(connector);
> intel_sdvo_destroy(connector);
> return false;
> }
> @@ -2624,8 +2625,10 @@ static void intel_sdvo_output_cleanup(struct intel_sdvo *intel_sdvo)
>
> list_for_each_entry_safe(connector, tmp,
> &dev->mode_config.connector_list, head) {
> - if (intel_attached_encoder(connector) == &intel_sdvo->base)
> + if (intel_attached_encoder(connector) == &intel_sdvo->base) {
> + drm_sysfs_connector_remove(connector);
> intel_sdvo_destroy(connector);
> + }
> }
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 75925a1..92895f9 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1433,7 +1433,6 @@ intel_tv_get_modes(struct drm_connector *connector)
> static void
> intel_tv_destroy(struct drm_connector *connector)
> {
> - drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> kfree(connector);
> }
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/5] tty/vt: add con_bind and con_unbind functions
2013-09-26 23:05 [PATCH 0/5] module_reload fixes Paulo Zanoni
2013-09-26 23:05 ` [PATCH 1/5] drm/i915: redisable VGA when we disable the power well Paulo Zanoni
2013-09-26 23:05 ` [PATCH 2/5] drm/i915: destroy connector sysfs files earlier Paulo Zanoni
@ 2013-09-26 23:06 ` Paulo Zanoni
2013-10-01 13:50 ` Ville Syrjälä
2013-09-26 23:06 ` [PATCH 4/5] console/fbcon: implement con_bind and con_unbind Paulo Zanoni
2013-09-26 23:06 ` [PATCH 5/5] drm/i915: put/get the power well at the FB bind/unbind functions Paulo Zanoni
4 siblings, 1 reply; 17+ messages in thread
From: Paulo Zanoni @ 2013-09-26 23:06 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, dri-devel
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
The consoles who need to do something when unbinding or binding can
optionally implement these functions.
The current problem I'm trying to solve is that when i915+fbcon is
loaded on Haswell, if we disable the power well (to save power) the
VGA interface gets completely disabled, so when we unbind fbcon we
need to restore the VGA interface to allow vgacon to work.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/tty/vt/vt.c | 6 ++++++
include/linux/console.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9a8e8c5..beb5986 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3014,6 +3014,9 @@ static int do_bind_con_driver(const struct consw *csw, int first, int last,
if (retval)
goto err;
+ if (csw->con_bind)
+ csw->con_bind();
+
if (!(con_driver->flag & CON_DRIVER_FLAG_INIT)) {
csw->con_startup();
con_driver->flag |= CON_DRIVER_FLAG_INIT;
@@ -3152,6 +3155,9 @@ int do_unbind_con_driver(const struct consw *csw, int first, int last, int deflt
if (!con_is_bound(csw))
goto err;
+ if (csw->con_unbind)
+ csw->con_unbind();
+
first = max(first, con_driver->first);
last = min(last, con_driver->last);
diff --git a/include/linux/console.h b/include/linux/console.h
index 7571a16..5cd2c35 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -65,6 +65,8 @@ struct consw {
* Restore the console to its pre-debug state as closely as possible.
*/
int (*con_debug_leave)(struct vc_data *);
+ void (*con_bind)(void);
+ void (*con_unbind)(void);
};
extern const struct consw *conswitchp;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] tty/vt: add con_bind and con_unbind functions
2013-09-26 23:06 ` [PATCH 3/5] tty/vt: add con_bind and con_unbind functions Paulo Zanoni
@ 2013-10-01 13:50 ` Ville Syrjälä
2013-10-08 21:12 ` Paulo Zanoni
0 siblings, 1 reply; 17+ messages in thread
From: Ville Syrjälä @ 2013-10-01 13:50 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni, dri-devel
On Thu, Sep 26, 2013 at 08:06:00PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The consoles who need to do something when unbinding or binding can
> optionally implement these functions.
>
> The current problem I'm trying to solve is that when i915+fbcon is
> loaded on Haswell, if we disable the power well (to save power) the
> VGA interface gets completely disabled, so when we unbind fbcon we
> need to restore the VGA interface to allow vgacon to work.
We don't need to make it work. No one else does, and in the general case
it's even impossible since on some hardware that would definitely
corrupt the state that the real driver is attempting to use. The only
case where it might be nice to restore vgacon is on i915 unload, but no
one else does that either AFAIK, so I would not waste any cycles on
attempting that.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/tty/vt/vt.c | 6 ++++++
> include/linux/console.h | 2 ++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 9a8e8c5..beb5986 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3014,6 +3014,9 @@ static int do_bind_con_driver(const struct consw *csw, int first, int last,
> if (retval)
> goto err;
>
> + if (csw->con_bind)
> + csw->con_bind();
> +
> if (!(con_driver->flag & CON_DRIVER_FLAG_INIT)) {
> csw->con_startup();
> con_driver->flag |= CON_DRIVER_FLAG_INIT;
> @@ -3152,6 +3155,9 @@ int do_unbind_con_driver(const struct consw *csw, int first, int last, int deflt
> if (!con_is_bound(csw))
> goto err;
>
> + if (csw->con_unbind)
> + csw->con_unbind();
> +
> first = max(first, con_driver->first);
> last = min(last, con_driver->last);
>
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 7571a16..5cd2c35 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -65,6 +65,8 @@ struct consw {
> * Restore the console to its pre-debug state as closely as possible.
> */
> int (*con_debug_leave)(struct vc_data *);
> + void (*con_bind)(void);
> + void (*con_unbind)(void);
> };
>
> extern const struct consw *conswitchp;
> --
> 1.8.3.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] tty/vt: add con_bind and con_unbind functions
2013-10-01 13:50 ` Ville Syrjälä
@ 2013-10-08 21:12 ` Paulo Zanoni
2013-10-09 8:10 ` Ville Syrjälä
0 siblings, 1 reply; 17+ messages in thread
From: Paulo Zanoni @ 2013-10-08 21:12 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Intel Graphics Development, Paulo Zanoni, DRI Development
2013/10/1 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Sep 26, 2013 at 08:06:00PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> The consoles who need to do something when unbinding or binding can
>> optionally implement these functions.
>>
>> The current problem I'm trying to solve is that when i915+fbcon is
>> loaded on Haswell, if we disable the power well (to save power) the
>> VGA interface gets completely disabled, so when we unbind fbcon we
>> need to restore the VGA interface to allow vgacon to work.
>
> We don't need to make it work. No one else does, and in the general case
> it's even impossible since on some hardware that would definitely
> corrupt the state that the real driver is attempting to use. The only
> case where it might be nice to restore vgacon is on i915 unload, but no
> one else does that either AFAIK, so I would not waste any cycles on
> attempting that.
I don't understand your point. Without patches 3-4-5, module_reload
doesn't work at all if the power well is disabled: we need these
patches to fix it. The plan is not to restore everything to make
vgacon actually work, the plan is just to prevent it from breaking
module_reload.
>
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> drivers/tty/vt/vt.c | 6 ++++++
>> include/linux/console.h | 2 ++
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
>> index 9a8e8c5..beb5986 100644
>> --- a/drivers/tty/vt/vt.c
>> +++ b/drivers/tty/vt/vt.c
>> @@ -3014,6 +3014,9 @@ static int do_bind_con_driver(const struct consw *csw, int first, int last,
>> if (retval)
>> goto err;
>>
>> + if (csw->con_bind)
>> + csw->con_bind();
>> +
>> if (!(con_driver->flag & CON_DRIVER_FLAG_INIT)) {
>> csw->con_startup();
>> con_driver->flag |= CON_DRIVER_FLAG_INIT;
>> @@ -3152,6 +3155,9 @@ int do_unbind_con_driver(const struct consw *csw, int first, int last, int deflt
>> if (!con_is_bound(csw))
>> goto err;
>>
>> + if (csw->con_unbind)
>> + csw->con_unbind();
>> +
>> first = max(first, con_driver->first);
>> last = min(last, con_driver->last);
>>
>> diff --git a/include/linux/console.h b/include/linux/console.h
>> index 7571a16..5cd2c35 100644
>> --- a/include/linux/console.h
>> +++ b/include/linux/console.h
>> @@ -65,6 +65,8 @@ struct consw {
>> * Restore the console to its pre-debug state as closely as possible.
>> */
>> int (*con_debug_leave)(struct vc_data *);
>> + void (*con_bind)(void);
>> + void (*con_unbind)(void);
>> };
>>
>> extern const struct consw *conswitchp;
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/5] tty/vt: add con_bind and con_unbind functions
2013-10-08 21:12 ` Paulo Zanoni
@ 2013-10-09 8:10 ` Ville Syrjälä
0 siblings, 0 replies; 17+ messages in thread
From: Ville Syrjälä @ 2013-10-09 8:10 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni, DRI Development
On Tue, Oct 08, 2013 at 06:12:15PM -0300, Paulo Zanoni wrote:
> 2013/10/1 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Thu, Sep 26, 2013 at 08:06:00PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> The consoles who need to do something when unbinding or binding can
> >> optionally implement these functions.
> >>
> >> The current problem I'm trying to solve is that when i915+fbcon is
> >> loaded on Haswell, if we disable the power well (to save power) the
> >> VGA interface gets completely disabled, so when we unbind fbcon we
> >> need to restore the VGA interface to allow vgacon to work.
> >
> > We don't need to make it work. No one else does, and in the general case
> > it's even impossible since on some hardware that would definitely
> > corrupt the state that the real driver is attempting to use. The only
> > case where it might be nice to restore vgacon is on i915 unload, but no
> > one else does that either AFAIK, so I would not waste any cycles on
> > attempting that.
>
> I don't understand your point. Without patches 3-4-5, module_reload
> doesn't work at all if the power well is disabled: we need these
> patches to fix it. The plan is not to restore everything to make
> vgacon actually work, the plan is just to prevent it from breaking
> module_reload.
How does the power well vs. vgacon break module_reload?
BTW module_reload seems to be busted on ILK and IVB too currently.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 4/5] console/fbcon: implement con_bind and con_unbind
2013-09-26 23:05 [PATCH 0/5] module_reload fixes Paulo Zanoni
` (2 preceding siblings ...)
2013-09-26 23:06 ` [PATCH 3/5] tty/vt: add con_bind and con_unbind functions Paulo Zanoni
@ 2013-09-26 23:06 ` Paulo Zanoni
2013-09-26 23:06 ` [PATCH 5/5] drm/i915: put/get the power well at the FB bind/unbind functions Paulo Zanoni
4 siblings, 0 replies; 17+ messages in thread
From: Paulo Zanoni @ 2013-09-26 23:06 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, dri-devel
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
And create fb_bind and fb_unbind fb_ops that the drivers can
implement.
The current problem I'm trying to solve is that when i915+fbcon is
loaded on Haswell, if we disable the power well (to save power) the
VGA interface gets completely disabled, so when we unbind fbcon we
need to restore the VGA interface to allow vgacon to work.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/video/console/fbcon.c | 26 ++++++++++++++++++++++++++
include/linux/fb.h | 4 ++++
2 files changed, 30 insertions(+)
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index cd8a802..68d316a 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3298,6 +3298,30 @@ done:
return ret;
}
+static void fbcon_con_bind(void)
+{
+ struct fb_info *info = NULL;
+
+ info = registered_fb[info_idx];
+ if (!info)
+ return;
+
+ if (info->fbops->fb_bind)
+ info->fbops->fb_bind(info);
+}
+
+static void fbcon_con_unbind(void)
+{
+ struct fb_info *info = NULL;
+
+ info = registered_fb[info_idx];
+ if (!info)
+ return;
+
+ if (info->fbops->fb_unbind)
+ info->fbops->fb_unbind(info);
+}
+
/*
* The console `switch' structure for the frame buffer based console
*/
@@ -3328,6 +3352,8 @@ static const struct consw fb_con = {
.con_resize = fbcon_resize,
.con_debug_enter = fbcon_debug_enter,
.con_debug_leave = fbcon_debug_leave,
+ .con_bind = fbcon_con_bind,
+ .con_unbind = fbcon_con_unbind,
};
static struct notifier_block fbcon_event_notifier = {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index ffac70a..8074bd5 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -304,6 +304,10 @@ struct fb_ops {
/* called at KDB enter and leave time to prepare the console */
int (*fb_debug_enter)(struct fb_info *info);
int (*fb_debug_leave)(struct fb_info *info);
+
+ /* called when binding/unbinding */
+ void (*fb_bind)(struct fb_info *info);
+ void (*fb_unbind)(struct fb_info *info);
};
#ifdef CONFIG_FB_TILEBLITTING
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 5/5] drm/i915: put/get the power well at the FB bind/unbind functions
2013-09-26 23:05 [PATCH 0/5] module_reload fixes Paulo Zanoni
` (3 preceding siblings ...)
2013-09-26 23:06 ` [PATCH 4/5] console/fbcon: implement con_bind and con_unbind Paulo Zanoni
@ 2013-09-26 23:06 ` Paulo Zanoni
4 siblings, 0 replies; 17+ messages in thread
From: Paulo Zanoni @ 2013-09-26 23:06 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, dri-devel
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
If we run the following command on Haswell when the power well is
disabled:
echo 0 > /sys/class/vtconsole/vtcon1/bind
then we'll get thousands of consecutive interrupts because something
is trying to touch registers that are on the disabled power well. So
before we unbind the console, we need to enable the power well to make
the VGA interface work correctly. With this, whoever is using it
doesn't trigger interrupts on i915.ko.
Fixes regression introduced by:
commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
drm/i915: switch disable_power_well default value to 1
Besides the command above, the "module_reload" test from
intel-gpu-tools can also be used to reproduce the bug fixed by this
patch.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67813
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_drv.h | 1 +
drivers/gpu/drm/i915/intel_fb.c | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e63646a..a643c48 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -113,6 +113,7 @@ struct intel_fbdev {
struct intel_framebuffer ifb;
struct list_head fbdev_list;
struct drm_display_mode *our_mode;
+ bool first_bind_done;
};
struct intel_encoder {
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index d883b77..8c6ed1c 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -43,6 +43,27 @@
#include <drm/i915_drm.h>
#include "i915_drv.h"
+static void intel_fb_bind(struct fb_info *info)
+{
+ struct drm_fb_helper *drm_helper = info->par;
+ struct drm_device *dev = drm_helper->dev;
+ struct intel_fbdev *ifbdev =
+ container_of(drm_helper, struct intel_fbdev, helper);
+
+ if (ifbdev->first_bind_done)
+ intel_display_power_put(dev, POWER_DOMAIN_VGA);
+ else
+ ifbdev->first_bind_done = true;
+}
+
+static void intel_fb_unbind(struct fb_info *info)
+{
+ struct drm_fb_helper *fb_helper = info->par;
+ struct drm_device *dev = fb_helper->dev;
+
+ intel_display_power_get(dev, POWER_DOMAIN_VGA);
+}
+
static struct fb_ops intelfb_ops = {
.owner = THIS_MODULE,
.fb_check_var = drm_fb_helper_check_var,
@@ -55,6 +76,8 @@ static struct fb_ops intelfb_ops = {
.fb_setcmap = drm_fb_helper_setcmap,
.fb_debug_enter = drm_fb_helper_debug_enter,
.fb_debug_leave = drm_fb_helper_debug_leave,
+ .fb_bind = intel_fb_bind,
+ .fb_unbind = intel_fb_unbind,
};
static int intelfb_create(struct drm_fb_helper *helper,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread