drm/i915: only disable default vga device

Commit Message

Emil Velikov May 16, 2021, 5:14 p.m. UTC

From: Vivek Das Mohapatra <[e-mail]>

This patch is to do with seamless handover, eg when the sequence is
bootloader → plymouth → desktop.

It switches the vga arbiter from the "other" GPU to the default one
(intel in this case), so the driver can issue some io().

Signed-off-by: Vivek Das Mohapatra <[e-mail]>
Signed-off-by: Emil Velikov <[e-mail]>
---
Greetings all,

This patch has been downstream for a while now yet it seems perfectly
reasonable thing to have in the Linux kernel.

https://github.com/ValveSoftware/steamos_kernel/commit/5431b5b1999c3d3b5efee817fb3373fbbd473063


 drivers/gpu/drm/i915/display/intel_vga.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Ville Syrjälä May 17, 2021, 5:24 p.m. UTC | #1

On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> From: Vivek Das Mohapatra <[e-mail]>
> 
> This patch is to do with seamless handover, eg when the sequence is
> bootloader → plymouth → desktop.
> 
> It switches the vga arbiter from the "other" GPU to the default one
> (intel in this case), so the driver can issue some io().

I don't understand what this commit message is trying to say.

> 
> Signed-off-by: Vivek Das Mohapatra <[e-mail]>
> Signed-off-by: Emil Velikov <[e-mail]>
> ---
> Greetings all,
> 
> This patch has been downstream for a while now yet it seems perfectly
> reasonable thing to have in the Linux kernel.
> 
> https://github.com/ValveSoftware/steamos_kernel/commit/5431b5b1999c3d3b5efee817fb3373fbbd473063
> 
> 
>  drivers/gpu/drm/i915/display/intel_vga.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
> index be333699c515..4f07b5ad5fa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -30,12 +30,14 @@ void intel_vga_disable(struct drm_i915_private *dev_priv)
>  	u8 sr1;
>  
>  	/* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
> -	vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> -	outb(SR01, VGA_SR_INDEX);
> -	sr1 = inb(VGA_SR_DATA);
> -	outb(sr1 | 1 << 5, VGA_SR_DATA);
> -	vga_put(pdev, VGA_RSRC_LEGACY_IO);
> -	udelay(300);
> +	if (pdev == vga_default_device()) {
> +		vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> +		outb(SR01, VGA_SR_INDEX);
> +		sr1 = inb(VGA_SR_DATA);
> +		outb(sr1 | 1 << 5, VGA_SR_DATA);
> +		vga_put(pdev, VGA_RSRC_LEGACY_IO);
> +		udelay(300);
> +	}
>  
>  	intel_de_write(dev_priv, vga_reg, VGA_DISP_DISABLE);
>  	intel_de_posting_read(dev_priv, vga_reg);
> -- 
> 2.27.0

Emil Velikov May 18, 2021, 11:09 a.m. UTC | #2

Hi Ville,

On Mon, 17 May 2021 at 18:24, Ville Syrjälä
<[e-mail]> wrote:
>
> On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> > From: Vivek Das Mohapatra <[e-mail]>
> >
> > This patch is to do with seamless handover, eg when the sequence is
> > bootloader → plymouth → desktop.
> >
> > It switches the vga arbiter from the "other" GPU to the default one
> > (intel in this case), so the driver can issue some io().
>
> I don't understand what this commit message is trying to say.
>
Bunch of context is lost due to the patch age, so I'm not 100% sure of
the actual hardware setup where this occurs.
Does the following make sense?

Currently on dual GPU systems, we do not get seamless handover as the
output flickers during the transition bootloader -> plymouth ->
desktop.
This happens as a result of switching (via the VGA arbiter) from the
"other" GPU back to the default i915 one and issuing io() commands.

-Emil

Ville Syrjälä May 18, 2021, 11:17 a.m. UTC | #3

On Tue, May 18, 2021 at 12:09:56PM +0100, Emil Velikov wrote:
> Hi Ville,
> 
> On Mon, 17 May 2021 at 18:24, Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> > > From: Vivek Das Mohapatra <[e-mail]>
> > >
> > > This patch is to do with seamless handover, eg when the sequence is
> > > bootloader → plymouth → desktop.
> > >
> > > It switches the vga arbiter from the "other" GPU to the default one
> > > (intel in this case), so the driver can issue some io().
> >
> > I don't understand what this commit message is trying to say.
> >
> Bunch of context is lost due to the patch age, so I'm not 100% sure of
> the actual hardware setup where this occurs.
> Does the following make sense?
> 
> Currently on dual GPU systems, we do not get seamless handover as the
> output flickers during the transition bootloader -> plymouth ->
> desktop.
> This happens as a result of switching (via the VGA arbiter) from the
> "other" GPU back to the default i915 one and issuing io() commands.

Hmm. Does this work?

--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -29,6 +29,9 @@ void intel_vga_disable(struct drm_i915_private *dev_priv)
        i915_reg_t vga_reg = intel_vga_cntrl_reg(dev_priv);
        u8 sr1;
 
+       if (intel_de_read(dev_priv, vga_reg) & VGA_DISP_DISABLE)
+               return;
+
        /* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
        vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
        outb(SR01, VGA_SR_INDEX);

Emil Velikov May 18, 2021, 12:39 p.m. UTC | #4

On Tue, 18 May 2021 at 12:17, Ville Syrjälä
<[e-mail]> wrote:
>
> On Tue, May 18, 2021 at 12:09:56PM +0100, Emil Velikov wrote:
> > Hi Ville,
> >
> > On Mon, 17 May 2021 at 18:24, Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> > > > From: Vivek Das Mohapatra <[e-mail]>
> > > >
> > > > This patch is to do with seamless handover, eg when the sequence is
> > > > bootloader → plymouth → desktop.
> > > >
> > > > It switches the vga arbiter from the "other" GPU to the default one
> > > > (intel in this case), so the driver can issue some io().
> > >
> > > I don't understand what this commit message is trying to say.
> > >
> > Bunch of context is lost due to the patch age, so I'm not 100% sure of
> > the actual hardware setup where this occurs.
> > Does the following make sense?
> >
> > Currently on dual GPU systems, we do not get seamless handover as the
> > output flickers during the transition bootloader -> plymouth ->
> > desktop.
> > This happens as a result of switching (via the VGA arbiter) from the
> > "other" GPU back to the default i915 one and issuing io() commands.
>
> Hmm. Does this work?
>
Thanks I'll it give it a try. Might need a few days to find the right
hardware/software combination.

-Emil

Emil Velikov May 26, 2021, 4:21 p.m. UTC | #5

Hi Ville,

On Tue, 18 May 2021 at 12:17, Ville Syrjälä
<[e-mail]> wrote:
>
> On Tue, May 18, 2021 at 12:09:56PM +0100, Emil Velikov wrote:
> > Hi Ville,
> >
> > On Mon, 17 May 2021 at 18:24, Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> > > > From: Vivek Das Mohapatra <[e-mail]>
> > > >
> > > > This patch is to do with seamless handover, eg when the sequence is
> > > > bootloader → plymouth → desktop.
> > > >
> > > > It switches the vga arbiter from the "other" GPU to the default one
> > > > (intel in this case), so the driver can issue some io().
> > >
> > > I don't understand what this commit message is trying to say.
> > >
> > Bunch of context is lost due to the patch age, so I'm not 100% sure of
> > the actual hardware setup where this occurs.
> > Does the following make sense?
> >
> > Currently on dual GPU systems, we do not get seamless handover as the
> > output flickers during the transition bootloader -> plymouth ->
> > desktop.
> > This happens as a result of switching (via the VGA arbiter) from the
> > "other" GPU back to the default i915 one and issuing io() commands.
>
> Hmm. Does this work?
>
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -29,6 +29,9 @@ void intel_vga_disable(struct drm_i915_private *dev_priv)
>         i915_reg_t vga_reg = intel_vga_cntrl_reg(dev_priv);
>         u8 sr1;
>
> +       if (intel_de_read(dev_priv, vga_reg) & VGA_DISP_DISABLE)
> +               return;
> +
>         /* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
>         vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
>         outb(SR01, VGA_SR_INDEX);
>
Was able to replicate the issue somewhat and the above does help quite a lot.
Feel free to add my:
Reviewed-by: Emil Velikov <[e-mail]>
Tested-by: Emil Velikov <[e-mail]>

Also feel free to reuse as much/little of the following setup details.

To reproduce the issue:

Get a dual GPU system - Intel+Nvidia in my case. Set the other
(Nvidia) as default in UEFI and connect monitors to it.
Ensure the bootloader (and if using splash manager like plymouth) are
set to display the UEFI BGRT. Personally I tested systemd-boot,
although GRUB should also work. I couldn't get plymouth to work/behave
here :shrug:

Note: Having the Nvidia drivers in the initramfs can lead to extra
flicker so leave them out. Include the i915 drivers in initramfs.

Without the patch, the existing bootslash is wiped clean almost
instantaneously as the i915 driver calls intel_vga_disable().
With your patch the call is a no-op, and the bootsplash stays around
until the login manager (and X) is spawned.

HTH
Emil

Emil Velikov June 4, 2021, 1:47 p.m. UTC | #6

On Wed, 26 May 2021 at 17:21, Emil Velikov <[e-mail]> wrote:
>
> Hi Ville,
>
> On Tue, 18 May 2021 at 12:17, Ville Syrjälä
> <[e-mail]> wrote:
> >
> > On Tue, May 18, 2021 at 12:09:56PM +0100, Emil Velikov wrote:
> > > Hi Ville,
> > >
> > > On Mon, 17 May 2021 at 18:24, Ville Syrjälä
> > > <[e-mail]> wrote:
> > > >
> > > > On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> > > > > From: Vivek Das Mohapatra <[e-mail]>
> > > > >
> > > > > This patch is to do with seamless handover, eg when the sequence is
> > > > > bootloader → plymouth → desktop.
> > > > >
> > > > > It switches the vga arbiter from the "other" GPU to the default one
> > > > > (intel in this case), so the driver can issue some io().
> > > >
> > > > I don't understand what this commit message is trying to say.
> > > >
> > > Bunch of context is lost due to the patch age, so I'm not 100% sure of
> > > the actual hardware setup where this occurs.
> > > Does the following make sense?
> > >
> > > Currently on dual GPU systems, we do not get seamless handover as the
> > > output flickers during the transition bootloader -> plymouth ->
> > > desktop.
> > > This happens as a result of switching (via the VGA arbiter) from the
> > > "other" GPU back to the default i915 one and issuing io() commands.
> >
> > Hmm. Does this work?
> >
> > --- a/drivers/gpu/drm/i915/display/intel_vga.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> > @@ -29,6 +29,9 @@ void intel_vga_disable(struct drm_i915_private *dev_priv)
> >         i915_reg_t vga_reg = intel_vga_cntrl_reg(dev_priv);
> >         u8 sr1;
> >
> > +       if (intel_de_read(dev_priv, vga_reg) & VGA_DISP_DISABLE)
> > +               return;
> > +
> >         /* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
> >         vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> >         outb(SR01, VGA_SR_INDEX);
> >
> Was able to replicate the issue somewhat and the above does help quite a lot.
> Feel free to add my:
> Reviewed-by: Emil Velikov <[e-mail]>
> Tested-by: Emil Velikov <[e-mail]>
>
Humble poke.

Cannot see this patch in the drm-misc or drm-intel trees. Is there
anything I do to help move it forward?

Thanks
-Emil

Ville Syrjälä June 4, 2021, 2:08 p.m. UTC | #7

On Fri, Jun 04, 2021 at 02:47:16PM +0100, Emil Velikov wrote:
> On Wed, 26 May 2021 at 17:21, Emil Velikov <[e-mail]> wrote:
> >
> > Hi Ville,
> >
> > On Tue, 18 May 2021 at 12:17, Ville Syrjälä
> > <[e-mail]> wrote:
> > >
> > > On Tue, May 18, 2021 at 12:09:56PM +0100, Emil Velikov wrote:
> > > > Hi Ville,
> > > >
> > > > On Mon, 17 May 2021 at 18:24, Ville Syrjälä
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> > > > > > From: Vivek Das Mohapatra <[e-mail]>
> > > > > >
> > > > > > This patch is to do with seamless handover, eg when the sequence is
> > > > > > bootloader → plymouth → desktop.
> > > > > >
> > > > > > It switches the vga arbiter from the "other" GPU to the default one
> > > > > > (intel in this case), so the driver can issue some io().
> > > > >
> > > > > I don't understand what this commit message is trying to say.
> > > > >
> > > > Bunch of context is lost due to the patch age, so I'm not 100% sure of
> > > > the actual hardware setup where this occurs.
> > > > Does the following make sense?
> > > >
> > > > Currently on dual GPU systems, we do not get seamless handover as the
> > > > output flickers during the transition bootloader -> plymouth ->
> > > > desktop.
> > > > This happens as a result of switching (via the VGA arbiter) from the
> > > > "other" GPU back to the default i915 one and issuing io() commands.
> > >
> > > Hmm. Does this work?
> > >
> > > --- a/drivers/gpu/drm/i915/display/intel_vga.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> > > @@ -29,6 +29,9 @@ void intel_vga_disable(struct drm_i915_private *dev_priv)
> > >         i915_reg_t vga_reg = intel_vga_cntrl_reg(dev_priv);
> > >         u8 sr1;
> > >
> > > +       if (intel_de_read(dev_priv, vga_reg) & VGA_DISP_DISABLE)
> > > +               return;
> > > +
> > >         /* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
> > >         vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> > >         outb(SR01, VGA_SR_INDEX);
> > >
> > Was able to replicate the issue somewhat and the above does help quite a lot.
> > Feel free to add my:
> > Reviewed-by: Emil Velikov <[e-mail]>
> > Tested-by: Emil Velikov <[e-mail]>
> >
> Humble poke.
> 
> Cannot see this patch in the drm-misc or drm-intel trees. Is there
> anything I do to help move it forward?

Can you just respin the patch with this?

Emil Velikov June 4, 2021, 3:53 p.m. UTC | #8

On Fri, 4 Jun 2021 at 15:08, Ville Syrjälä
<[e-mail]> wrote:
>
> On Fri, Jun 04, 2021 at 02:47:16PM +0100, Emil Velikov wrote:
> > On Wed, 26 May 2021 at 17:21, Emil Velikov <[e-mail]> wrote:
> > >
> > > Hi Ville,
> > >
> > > On Tue, 18 May 2021 at 12:17, Ville Syrjälä
> > > <[e-mail]> wrote:
> > > >
> > > > On Tue, May 18, 2021 at 12:09:56PM +0100, Emil Velikov wrote:
> > > > > Hi Ville,
> > > > >
> > > > > On Mon, 17 May 2021 at 18:24, Ville Syrjälä
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > On Sun, May 16, 2021 at 06:14:32PM +0100, Emil Velikov wrote:
> > > > > > > From: Vivek Das Mohapatra <[e-mail]>
> > > > > > >
> > > > > > > This patch is to do with seamless handover, eg when the sequence is
> > > > > > > bootloader → plymouth → desktop.
> > > > > > >
> > > > > > > It switches the vga arbiter from the "other" GPU to the default one
> > > > > > > (intel in this case), so the driver can issue some io().
> > > > > >
> > > > > > I don't understand what this commit message is trying to say.
> > > > > >
> > > > > Bunch of context is lost due to the patch age, so I'm not 100% sure of
> > > > > the actual hardware setup where this occurs.
> > > > > Does the following make sense?
> > > > >
> > > > > Currently on dual GPU systems, we do not get seamless handover as the
> > > > > output flickers during the transition bootloader -> plymouth ->
> > > > > desktop.
> > > > > This happens as a result of switching (via the VGA arbiter) from the
> > > > > "other" GPU back to the default i915 one and issuing io() commands.
> > > >
> > > > Hmm. Does this work?
> > > >
> > > > --- a/drivers/gpu/drm/i915/display/intel_vga.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> > > > @@ -29,6 +29,9 @@ void intel_vga_disable(struct drm_i915_private *dev_priv)
> > > >         i915_reg_t vga_reg = intel_vga_cntrl_reg(dev_priv);
> > > >         u8 sr1;
> > > >
> > > > +       if (intel_de_read(dev_priv, vga_reg) & VGA_DISP_DISABLE)
> > > > +               return;
> > > > +
> > > >         /* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
> > > >         vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> > > >         outb(SR01, VGA_SR_INDEX);
> > > >
> > > Was able to replicate the issue somewhat and the above does help quite a lot.
> > > Feel free to add my:
> > > Reviewed-by: Emil Velikov <[e-mail]>
> > > Tested-by: Emil Velikov <[e-mail]>
> > >
> > Humble poke.
> >
> > Cannot see this patch in the drm-misc or drm-intel trees. Is there
> > anything I do to help move it forward?
>
> Can you just respin the patch with this?
>
I've used your suggestion and redone the commit message completely.
Hope it's far less cryptic now.
Patch is on the ML - realised I didn't CC you, just as it went out.

https://lore.kernel.org/dri-devel/[e-mail]/T/#u

Thanks
-Emil

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
index be333699c515..4f07b5ad5fa0 100644
--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -30,12 +30,14 @@  void intel_vga_disable(struct drm_i915_private *dev_priv)
 	u8 sr1;
 
 	/* WaEnableVGAAccessThroughIOPort:ctg,elk,ilk,snb,ivb,vlv,hsw */
-	vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
-	outb(SR01, VGA_SR_INDEX);
-	sr1 = inb(VGA_SR_DATA);
-	outb(sr1 | 1 << 5, VGA_SR_DATA);
-	vga_put(pdev, VGA_RSRC_LEGACY_IO);
-	udelay(300);
+	if (pdev == vga_default_device()) {
+		vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
+		outb(SR01, VGA_SR_INDEX);
+		sr1 = inb(VGA_SR_DATA);
+		outb(sr1 | 1 << 5, VGA_SR_DATA);
+		vga_put(pdev, VGA_RSRC_LEGACY_IO);
+		udelay(300);
+	}
 
 	intel_de_write(dev_priv, vga_reg, VGA_DISP_DISABLE);
 	intel_de_posting_read(dev_priv, vga_reg);