[PATCH 1/2] drm/i915: Propagate PCI read/write errors during vga_set_state()

[PATCH 1/2] drm/i915: Propagate PCI read/write errors during vga_set_state()

[Chris Wilson]

This has very little effect other than log the errors in case of failure,
and we then hope for the best.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d0082a025c83..7db292c469af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11336,12 +11336,21 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u16 gmch_ctrl;
 
-	pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl);
+	if (pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl)) {
+		DRM_ERROR("failed to read control word\n");
+		return -EIO;
+	}
+
 	if (state)
 		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
 	else
 		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
-	pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl);
+
+	if (pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl)) {
+		DRM_ERROR("failed to write control word\n");
+		return -EIO;
+	}
+
 	return 0;
 }
 
-- 
1.8.5.1

[PATCH 2/2] drm/i915: Short-circuit no-op vga_set_state()

[Chris Wilson]

Touching the VGA registers risks a hard machine hang, at least on this
ivb machine after removing a conflicting efifb. This is more than likely
related to the discovery that VGA IO decode on the more recent PCH
platforms is terminally broken.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7db292c469af..d9b91a585dbf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11341,6 +11341,9 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 		return -EIO;
 	}
 
+	if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !state)
+		return 0;
+
 	if (state)
 		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
 	else
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Short-circuit no-op vga_set_state()

[Jani Nikula]

On Mon, 16 Dec 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Touching the VGA registers risks a hard machine hang, at least on this
> ivb machine after removing a conflicting efifb. This is more than likely
> related to the discovery that VGA IO decode on the more recent PCH
> platforms is terminally broken.

Please double check the config space address. At a glance, it looks like
we should be using SNB_GMCH_CTRL since SNB. Might explain the issues?

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7db292c469af..d9b91a585dbf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11341,6 +11341,9 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
>  		return -EIO;
>  	}
>  
> +	if (!!(gmch_ctrl & INTEL_GMCH_VGA_DISABLE) == !state)
> +		return 0;
> +

I think I'd use a temp gmch_ctrl, make the change below on that, and
then compare the new and old values. Just to avoid the use of negatives
so much. Either way, the patch is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

*iff* it's still needed with the verified config space address.


BR,
Jani.

>  	if (state)
>  		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
>  	else
> -- 
> 1.8.5.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Short-circuit no-op vga_set_state()

[Chris Wilson]

On Tue, Dec 17, 2013 at 11:21:27AM +0200, Jani Nikula wrote:
> On Mon, 16 Dec 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Touching the VGA registers risks a hard machine hang, at least on this
> > ivb machine after removing a conflicting efifb. This is more than likely
> > related to the discovery that VGA IO decode on the more recent PCH
> > platforms is terminally broken.
> 
> Please double check the config space address. At a glance, it looks like
> we should be using SNB_GMCH_CTRL since SNB. Might explain the issues?

Hmm, indeed. This bit is at a different location on SNB, but it is
locked. Which is where all the vgaarb madness began last time.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Short-circuit no-op vga_set_state()

[Chris Wilson]

On Tue, Dec 17, 2013 at 11:45:45AM +0000, Chris Wilson wrote:
> On Tue, Dec 17, 2013 at 11:21:27AM +0200, Jani Nikula wrote:
> > On Mon, 16 Dec 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > Touching the VGA registers risks a hard machine hang, at least on this
> > > ivb machine after removing a conflicting efifb. This is more than likely
> > > related to the discovery that VGA IO decode on the more recent PCH
> > > platforms is terminally broken.
> > 
> > Please double check the config space address. At a glance, it looks like
> > we should be using SNB_GMCH_CTRL since SNB. Might explain the issues?
> 
> Hmm, indeed. This bit is at a different location on SNB, but it is
> locked. Which is where all the vgaarb madness began last time.

Ok, that feels at least as stable as not writing to the invalid address.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Use the correct GMCH_CTRL register for Sandybridge+

[Chris Wilson]

The GMCH_CTRL register (or MGCC in the spec) is at a different address
on Sandybridge, and the address to which we currently write to is
undefined. These stray writes appear to upset (hard hang) my Ivybridge
machine whilst it is in UEFI mode.

Note that the register is still marked as locked RO on Sandybridge, so
vgaarb is still dysfunctional.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7db292c469af..9caf6a879f31 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11334,9 +11334,10 @@ void intel_connector_attach_encoder(struct intel_connector *connector,
 int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned reg = INTEL_INFO(dev)->gen >= 6 ? SNB_GMCH_CTRL : INTEL_GMCH_CTRL;
 	u16 gmch_ctrl;
 
-	if (pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl)) {
+	if (pci_read_config_word(dev_priv->bridge_dev, reg, &gmch_ctrl)) {
 		DRM_ERROR("failed to read control word\n");
 		return -EIO;
 	}
@@ -11346,7 +11347,7 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 	else
 		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
 
-	if (pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl)) {
+	if (pci_write_config_word(dev_priv->bridge_dev, reg, gmch_ctrl)) {
 		DRM_ERROR("failed to write control word\n");
 		return -EIO;
 	}
-- 
1.8.5.1

Re: [PATCH] drm/i915: Use the correct GMCH_CTRL register for Sandybridge+

[Jani Nikula]

On Tue, 17 Dec 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The GMCH_CTRL register (or MGCC in the spec) is at a different address
> on Sandybridge, and the address to which we currently write to is
> undefined. These stray writes appear to upset (hard hang) my Ivybridge
> machine whilst it is in UEFI mode.
>
> Note that the register is still marked as locked RO on Sandybridge, so
> vgaarb is still dysfunctional.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7db292c469af..9caf6a879f31 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11334,9 +11334,10 @@ void intel_connector_attach_encoder(struct intel_connector *connector,
>  int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned reg = INTEL_INFO(dev)->gen >= 6 ? SNB_GMCH_CTRL : INTEL_GMCH_CTRL;
>  	u16 gmch_ctrl;
>  
> -	if (pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl)) {
> +	if (pci_read_config_word(dev_priv->bridge_dev, reg, &gmch_ctrl)) {
>  		DRM_ERROR("failed to read control word\n");
>  		return -EIO;
>  	}
> @@ -11346,7 +11347,7 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
>  	else
>  		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
>  
> -	if (pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl)) {
> +	if (pci_write_config_word(dev_priv->bridge_dev, reg, gmch_ctrl)) {
>  		DRM_ERROR("failed to write control word\n");
>  		return -EIO;
>  	}
> -- 
> 1.8.5.1
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Propagate PCI read/write errors during vga_set_state()

[Jani Nikula]

On Mon, 16 Dec 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This has very little effect other than log the errors in case of failure,
> and we then hope for the best.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d0082a025c83..7db292c469af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11336,12 +11336,21 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u16 gmch_ctrl;
>  
> -	pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl);
> +	if (pci_read_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, &gmch_ctrl)) {
> +		DRM_ERROR("failed to read control word\n");
> +		return -EIO;
> +	}
> +
>  	if (state)
>  		gmch_ctrl &= ~INTEL_GMCH_VGA_DISABLE;
>  	else
>  		gmch_ctrl |= INTEL_GMCH_VGA_DISABLE;
> -	pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl);
> +
> +	if (pci_write_config_word(dev_priv->bridge_dev, INTEL_GMCH_CTRL, gmch_ctrl)) {
> +		DRM_ERROR("failed to write control word\n");
> +		return -EIO;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 1.8.5.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center