| Message ID | 20180717174216.22252-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 17, 2018 at 08:42:15PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We never registered any kind of lid notifier for eDP, so looking at the > lid status is pretty much bonkers. Let's just consider eDP always > connected instead. hm.... I've seen in the past, specially on rvps, so many times that we detect edp or assume edp is there and it is not... that generates a flood of aux timeout reads... I never investigate that, but I wonder if this change here couldn't make that case even more common... > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b45b08420c1f..61657b8b109c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4532,14 +4532,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > static enum drm_connector_status > edp_detect(struct intel_dp *intel_dp) > { > - struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); > - enum drm_connector_status status; > - > - status = intel_panel_detect(dev_priv); > - if (status == connector_status_unknown) > - status = connector_status_connected; > - > - return status; > + return connector_status_connected; > } > > static bool ibx_digital_port_connected(struct intel_encoder *encoder) > @@ -4802,7 +4795,7 @@ intel_dp_long_pulse(struct intel_connector *connector) > > intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > - /* Can't disconnect eDP, but you can close the lid... */ > + /* Can't disconnect eDP */ > if (intel_dp_is_edp(intel_dp)) > status = edp_detect(intel_dp); > else if (intel_digital_port_connected(&dp_to_dig_port(intel_dp)->base)) > -- > 2.16.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jul 17, 2018 at 01:00:25PM -0700, Rodrigo Vivi wrote: > On Tue, Jul 17, 2018 at 08:42:15PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > We never registered any kind of lid notifier for eDP, so looking at the > > lid status is pretty much bonkers. Let's just consider eDP always > > connected instead. > > hm.... > I've seen in the past, specially on rvps, so many times that > we detect edp or assume edp is there and it is not... > that generates a flood of aux timeout reads... > > I never investigate that, but I wonder if this change here > couldn't make that case even more common... I don't know if RVPs would have any of the lid stuff implemented. I somewhat doubt it. If dpcd reads fail we won't register edp so any timeouts and whatnot should only be logged during driver load. > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 11 ++--------- > > 1 file changed, 2 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index b45b08420c1f..61657b8b109c 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4532,14 +4532,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > > static enum drm_connector_status > > edp_detect(struct intel_dp *intel_dp) > > { > > - struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); > > - enum drm_connector_status status; > > - > > - status = intel_panel_detect(dev_priv); > > - if (status == connector_status_unknown) > > - status = connector_status_connected; > > - > > - return status; > > + return connector_status_connected; > > } > > > > static bool ibx_digital_port_connected(struct intel_encoder *encoder) > > @@ -4802,7 +4795,7 @@ intel_dp_long_pulse(struct intel_connector *connector) > > > > intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > > > - /* Can't disconnect eDP, but you can close the lid... */ > > + /* Can't disconnect eDP */ > > if (intel_dp_is_edp(intel_dp)) > > status = edp_detect(intel_dp); > > else if (intel_digital_port_connected(&dp_to_dig_port(intel_dp)->base)) > > -- > > 2.16.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 18, 2018 at 01:03:59PM +0300, Ville Syrjälä wrote: > On Tue, Jul 17, 2018 at 01:00:25PM -0700, Rodrigo Vivi wrote: > > On Tue, Jul 17, 2018 at 08:42:15PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > We never registered any kind of lid notifier for eDP, so looking at the > > > lid status is pretty much bonkers. Let's just consider eDP always > > > connected instead. > > > > hm.... > > I've seen in the past, specially on rvps, so many times that > > we detect edp or assume edp is there and it is not... > > that generates a flood of aux timeout reads... > > > > I never investigate that, but I wonder if this change here > > couldn't make that case even more common... > > I don't know if RVPs would have any of the lid stuff implemented. > I somewhat doubt it. I think they do... and with some hard toggles buttons, but anyways I believe we might safe, otherwise our CI would have complained about it right? > > If dpcd reads fail we won't register edp so any timeouts and whatnot > should only be logged during driver load. yeap, but I'm afraid that without any kind of check we would start seeing those aux failures everywhere... even on regular desktop units where we naver saw that kind of failure would start to having logs poluted. But if CI is happy and you checked that this is not poluting the logs of platforms without eDP then the patch looks correct: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 11 ++--------- > > > 1 file changed, 2 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > index b45b08420c1f..61657b8b109c 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -4532,14 +4532,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > > > static enum drm_connector_status > > > edp_detect(struct intel_dp *intel_dp) > > > { > > > - struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); > > > - enum drm_connector_status status; > > > - > > > - status = intel_panel_detect(dev_priv); > > > - if (status == connector_status_unknown) > > > - status = connector_status_connected; > > > - > > > - return status; > > > + return connector_status_connected; > > > } > > > > > > static bool ibx_digital_port_connected(struct intel_encoder *encoder) > > > @@ -4802,7 +4795,7 @@ intel_dp_long_pulse(struct intel_connector *connector) > > > > > > intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > > > > > - /* Can't disconnect eDP, but you can close the lid... */ > > > + /* Can't disconnect eDP */ > > > if (intel_dp_is_edp(intel_dp)) > > > status = edp_detect(intel_dp); > > > else if (intel_digital_port_connected(&dp_to_dig_port(intel_dp)->base)) > > > -- > > > 2.16.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 18, 2018 at 02:00:42PM -0700, Rodrigo Vivi wrote: > On Wed, Jul 18, 2018 at 01:03:59PM +0300, Ville Syrjälä wrote: > > On Tue, Jul 17, 2018 at 01:00:25PM -0700, Rodrigo Vivi wrote: > > > On Tue, Jul 17, 2018 at 08:42:15PM +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > We never registered any kind of lid notifier for eDP, so looking at the > > > > lid status is pretty much bonkers. Let's just consider eDP always > > > > connected instead. > > > > > > hm.... > > > I've seen in the past, specially on rvps, so many times that > > > we detect edp or assume edp is there and it is not... > > > that generates a flood of aux timeout reads... > > > > > > I never investigate that, but I wonder if this change here > > > couldn't make that case even more common... > > > > I don't know if RVPs would have any of the lid stuff implemented. > > I somewhat doubt it. > > I think they do... and with some hard toggles buttons, Oh right, some do. I think I have one SKL with those switches on the front. Don't think I've seen those buttons on any Atom RVPs though. > > but anyways I believe we might safe, > otherwise our CI would have complained about it right? Not sure. Hmm. Actually none of this should matter for any machine that doesn't actually have eDP. We don't check this stuff when we initialize the encoder/connector so we'd still get the same amount of DPCD fail as before. > > > > > If dpcd reads fail we won't register edp so any timeouts and whatnot > > should only be logged during driver load. > > yeap, but I'm afraid that without any kind of check we would > start seeing those aux failures everywhere... even on regular desktop > units where we naver saw that kind of failure would start to having logs > poluted. > > But if CI is happy and you checked that this is not poluting the logs > of platforms without eDP then the patch looks correct: > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Thanks. > > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 11 ++--------- > > > > 1 file changed, 2 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > > > index b45b08420c1f..61657b8b109c 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -4532,14 +4532,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > > > > static enum drm_connector_status > > > > edp_detect(struct intel_dp *intel_dp) > > > > { > > > > - struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); > > > > - enum drm_connector_status status; > > > > - > > > > - status = intel_panel_detect(dev_priv); > > > > - if (status == connector_status_unknown) > > > > - status = connector_status_connected; > > > > - > > > > - return status; > > > > + return connector_status_connected; > > > > } > > > > > > > > static bool ibx_digital_port_connected(struct intel_encoder *encoder) > > > > @@ -4802,7 +4795,7 @@ intel_dp_long_pulse(struct intel_connector *connector) > > > > > > > > intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > > > > > > > - /* Can't disconnect eDP, but you can close the lid... */ > > > > + /* Can't disconnect eDP */ > > > > if (intel_dp_is_edp(intel_dp)) > > > > status = edp_detect(intel_dp); > > > > else if (intel_digital_port_connected(&dp_to_dig_port(intel_dp)->base)) > > > > -- > > > > 2.16.4 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b45b08420c1f..61657b8b109c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4532,14 +4532,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) static enum drm_connector_status edp_detect(struct intel_dp *intel_dp) { - struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp)); - enum drm_connector_status status; - - status = intel_panel_detect(dev_priv); - if (status == connector_status_unknown) - status = connector_status_connected; - - return status; + return connector_status_connected; } static bool ibx_digital_port_connected(struct intel_encoder *encoder) @@ -4802,7 +4795,7 @@ intel_dp_long_pulse(struct intel_connector *connector) intel_display_power_get(dev_priv, intel_dp->aux_power_domain); - /* Can't disconnect eDP, but you can close the lid... */ + /* Can't disconnect eDP */ if (intel_dp_is_edp(intel_dp)) status = edp_detect(intel_dp); else if (intel_digital_port_connected(&dp_to_dig_port(intel_dp)->base))