[01/12] drm/i915: Ignore initial lid state for eDP
diff mbox

Message ID 1469717448-4297-2-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä July 28, 2016, 2:50 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we try to check the lid status to see whether eDP is connected
or not. However we ignore the result anyway, and we have no lid notifier
to fire off uevents, so let's just ignore the lid stuff for eDP
entirely.

While at it, rip out the eDP case from intel_dp_detect_dpcd() since we
never call that thing with eDP displays anyway.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

Comments

Chris Wilson July 28, 2016, 3:47 p.m. UTC | #1
On Thu, Jul 28, 2016 at 05:50:37PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we try to check the lid status to see whether eDP is connected
> or not. However we ignore the result anyway, and we have no lid notifier
> to fire off uevents, so let's just ignore the lid stuff for eDP
> entirely.

You mean the OpRegion no longer contains lid_state? I don't see it being
disabled... The lid notifier is from acpi/button.c. I haven't looked at
why the result would be ignored, but you haven't convinced me why eDP is
special.
-Chris
Ville Syrjälä July 28, 2016, 4:01 p.m. UTC | #2
On Thu, Jul 28, 2016 at 04:47:07PM +0100, Chris Wilson wrote:
> On Thu, Jul 28, 2016 at 05:50:37PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we try to check the lid status to see whether eDP is connected
> > or not. However we ignore the result anyway, and we have no lid notifier
> > to fire off uevents, so let's just ignore the lid stuff for eDP
> > entirely.
> 
> You mean the OpRegion no longer contains lid_state? I don't see it being
> disabled... The lid notifier is from acpi/button.c. I haven't looked at
> why the result would be ignored, but you haven't convinced me why eDP is
> special.

We don't register a lid notifier for eDP. So if the initial detect would
happen with lid closed we'd mark eDP as disconnected, and when the lid
gets opened we won't even notice, and thus the connector never becomes
connected.

IIRC on my ILK the lid state in OpRegion didn't work anyway. But I
should probably double check.
Chris Wilson July 28, 2016, 4:36 p.m. UTC | #3
On Thu, Jul 28, 2016 at 07:01:29PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 28, 2016 at 04:47:07PM +0100, Chris Wilson wrote:
> > On Thu, Jul 28, 2016 at 05:50:37PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Currently we try to check the lid status to see whether eDP is connected
> > > or not. However we ignore the result anyway, and we have no lid notifier
> > > to fire off uevents, so let's just ignore the lid stuff for eDP
> > > entirely.
> > 
> > You mean the OpRegion no longer contains lid_state? I don't see it being
> > disabled... The lid notifier is from acpi/button.c. I haven't looked at
> > why the result would be ignored, but you haven't convinced me why eDP is
> > special.
> 
> We don't register a lid notifier for eDP. So if the initial detect would
> happen with lid closed we'd mark eDP as disconnected, and when the lid
> gets opened we won't even notice, and thus the connector never becomes
> connected.

That was meant to be part of the intel_panel so that we have consistent
behaviour on any laptop. I would still rather we have consistent
behaviour, but eDP has been different for so long that any change now is
going to be painful.
 
> IIRC on my ILK the lid state in OpRegion didn't work anyway. But I
> should probably double check.

Yup, the lid button was unreliable as every other piece of hw. Looking
at intel_lid_notify(), would it not be easier just to send the hotplug
uevent.
-Chris
Ville Syrjälä July 28, 2016, 4:48 p.m. UTC | #4
On Thu, Jul 28, 2016 at 05:36:27PM +0100, Chris Wilson wrote:
> On Thu, Jul 28, 2016 at 07:01:29PM +0300, Ville Syrjälä wrote:
> > On Thu, Jul 28, 2016 at 04:47:07PM +0100, Chris Wilson wrote:
> > > On Thu, Jul 28, 2016 at 05:50:37PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Currently we try to check the lid status to see whether eDP is connected
> > > > or not. However we ignore the result anyway, and we have no lid notifier
> > > > to fire off uevents, so let's just ignore the lid stuff for eDP
> > > > entirely.
> > > 
> > > You mean the OpRegion no longer contains lid_state? I don't see it being
> > > disabled... The lid notifier is from acpi/button.c. I haven't looked at
> > > why the result would be ignored, but you haven't convinced me why eDP is
> > > special.
> > 
> > We don't register a lid notifier for eDP. So if the initial detect would
> > happen with lid closed we'd mark eDP as disconnected, and when the lid
> > gets opened we won't even notice, and thus the connector never becomes
> > connected.
> 
> That was meant to be part of the intel_panel so that we have consistent
> behaviour on any laptop. I would still rather we have consistent
> behaviour, but eDP has been different for so long that any change now is
> going to be painful.
>  
> > IIRC on my ILK the lid state in OpRegion didn't work anyway. But I
> > should probably double check.
> 
> Yup, the lid button was unreliable as every other piece of hw. Looking
> at intel_lid_notify(), would it not be easier just to send the hotplug
> uevent.

I thought the main point of the lid notifier was to restore any state
trampled by the VBIOS. I've been hoping that I'd find some magic thing
to get the VBIOS to not do that. IIRC there are some semi-interesting
looking things in OpRegion that might just do it. But to know for sure
I'd first have to get my hands on a machine that suffers from this
affliction.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 21b04c3eda41..1fe9bb745de5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3965,9 +3965,6 @@  intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	if (!intel_dp_get_dpcd(intel_dp))
 		return connector_status_disconnected;
 
-	if (is_edp(intel_dp))
-		return connector_status_connected;
-
 	/* if there's no downstream port, we're done */
 	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
 		return connector_status_connected;
@@ -4003,19 +4000,6 @@  intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	return connector_status_disconnected;
 }
 
-static enum drm_connector_status
-edp_detect(struct intel_dp *intel_dp)
-{
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	enum drm_connector_status status;
-
-	status = intel_panel_detect(dev);
-	if (status == connector_status_unknown)
-		status = connector_status_connected;
-
-	return status;
-}
-
 static bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
 				       struct intel_digital_port *port)
 {
@@ -4223,9 +4207,8 @@  intel_dp_long_pulse(struct intel_connector *intel_connector)
 	power_domain = intel_display_port_aux_power_domain(intel_encoder);
 	intel_display_power_get(to_i915(dev), power_domain);
 
-	/* Can't disconnect eDP, but you can close the lid... */
 	if (is_edp(intel_dp))
-		status = edp_detect(intel_dp);
+		status = connector_status_connected;
 	else if (intel_digital_port_connected(to_i915(dev),
 					      dp_to_dig_port(intel_dp)))
 		status = intel_dp_detect_dpcd(intel_dp);