| Message ID | 1436443470-28890-5-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote: > Adding this for SKL onwards. > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> I think this is the critical piece really, and I'd like to roll it out for all platforms. git has the code for all the old ones, so no big deal to digg that out. Also we need your fix for the interrupt handling first ofc, otherwise this won't work. Then we can apply this and give it some good testing before we start relying on it with caching hdmi probe status. I know this means that things will be slower, but I've been burned a bit too much the last few times we've tried this. And I really think we need the most amount of testing we can get (hence rolling this out for all platforms). If your hpd irq handling bugfix is indeed the bug that will be confirmed quickly, otherwise it means back to debugging. > --- > drivers/gpu/drm/i915/intel_hdmi.c | 49 ++++++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 1fb6919..769cf4f 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1300,6 +1300,46 @@ intel_hdmi_unset_edid(struct drm_connector *connector) > to_intel_connector(connector)->detect_edid = NULL; > } > > +static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port) > +{ > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > + enum port port = intel_dig_port->port; > + > + if (IS_SKYLAKE(dev)) { > + u32 temp = I915_READ(SDEISR); > + > + switch (port) { > + case PORT_B: > + return temp & SDE_PORTB_HOTPLUG_CPT; > + > + case PORT_C: > + return temp & SDE_PORTC_HOTPLUG_CPT; > + > + case PORT_D: > + return temp & SDE_PORTD_HOTPLUG_CPT; > + > + default: > + return false; > + } The old code had per-platform helper functions for this instead of a big if-ladder. I think that would look better. Also when you digg out these old versions please also cite the commit sha1 of the patches where we had to last revert this (and explain why it's now save). -Daniel > + } else if (IS_BROXTON(dev)) { > + u32 temp = I915_READ(GEN8_DE_PORT_ISR); > + > + switch (port) { > + case PORT_B: > + return temp & BXT_DE_PORT_HP_DDIB; > + > + case PORT_C: > + return temp & BXT_DE_PORT_HP_DDIC; > + > + default: > + return false; > + > + } > + } > + return true; > +} > + > static bool > intel_hdmi_set_edid(struct drm_connector *connector) > { > @@ -1308,15 +1348,16 @@ intel_hdmi_set_edid(struct drm_connector *connector) > struct intel_encoder *intel_encoder = > &hdmi_to_dig_port(intel_hdmi)->base; > enum intel_display_power_domain power_domain; > - struct edid *edid; > + struct edid *edid = NULL; > bool connected = false; > > power_domain = intel_display_port_power_domain(intel_encoder); > intel_display_power_get(dev_priv, power_domain); > > - edid = drm_get_edid(connector, > - intel_gmbus_get_adapter(dev_priv, > - intel_hdmi->ddc_bus)); > + if (intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi))) > + edid = drm_get_edid(connector, > + intel_gmbus_get_adapter(dev_priv, > + intel_hdmi->ddc_bus)); > > intel_display_power_put(dev_priv, power_domain); > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 7/9/2015 10:57 PM, Daniel Vetter wrote: > On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote: >> Adding this for SKL onwards. >> >> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > I think this is the critical piece really, and I'd like to roll it out for > all platforms. git has the code for all the old ones, so no big deal to > digg that out. Also we need your fix for the interrupt handling first ofc, > otherwise this won't work. > We have tested this with VLV/CHV too, and it works fine. I'l add those platforms also in the next version of this patch and change the gen check in the next patch as well. > Then we can apply this and give it some good testing before we start > relying on it with caching hdmi probe status. I know this means that > things will be slower, but I've been burned a bit too much the last few > times we've tried this. And I really think we need the most amount of > testing we can get (hence rolling this out for all platforms). If your > hpd irq handling bugfix is indeed the bug that will be confirmed quickly, > otherwise it means back to debugging. > >> --- >> drivers/gpu/drm/i915/intel_hdmi.c | 49 ++++++++++++++++++++++++++++++++++--- >> 1 file changed, 45 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index 1fb6919..769cf4f 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1300,6 +1300,46 @@ intel_hdmi_unset_edid(struct drm_connector *connector) >> to_intel_connector(connector)->detect_edid = NULL; >> } >> >> +static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port) >> +{ >> + struct drm_device *dev = intel_dig_port->base.base.dev; >> + struct drm_i915_private *dev_priv = to_i915(dev); >> + enum port port = intel_dig_port->port; >> + >> + if (IS_SKYLAKE(dev)) { >> + u32 temp = I915_READ(SDEISR); >> + >> + switch (port) { >> + case PORT_B: >> + return temp & SDE_PORTB_HOTPLUG_CPT; >> + >> + case PORT_C: >> + return temp & SDE_PORTC_HOTPLUG_CPT; >> + >> + case PORT_D: >> + return temp & SDE_PORTD_HOTPLUG_CPT; >> + >> + default: >> + return false; >> + } > > The old code had per-platform helper functions for this instead of a big > if-ladder. I think that would look better. > Hmm, I see g4x_digital_port_connected and ibx_digital_port_connected which already checks for these bits. We can reuse that here instead. I'l send the next version. Thanks, Sonika > Also when you digg out these old versions please also cite the commit sha1 > of the patches where we had to last revert this (and explain why it's now > save). > -Daniel > >> + } else if (IS_BROXTON(dev)) { >> + u32 temp = I915_READ(GEN8_DE_PORT_ISR); >> + >> + switch (port) { >> + case PORT_B: >> + return temp & BXT_DE_PORT_HP_DDIB; >> + >> + case PORT_C: >> + return temp & BXT_DE_PORT_HP_DDIC; >> + >> + default: >> + return false; >> + >> + } >> + } >> + return true; >> +} >> + >> static bool >> intel_hdmi_set_edid(struct drm_connector *connector) >> { >> @@ -1308,15 +1348,16 @@ intel_hdmi_set_edid(struct drm_connector *connector) >> struct intel_encoder *intel_encoder = >> &hdmi_to_dig_port(intel_hdmi)->base; >> enum intel_display_power_domain power_domain; >> - struct edid *edid; >> + struct edid *edid = NULL; >> bool connected = false; >> >> power_domain = intel_display_port_power_domain(intel_encoder); >> intel_display_power_get(dev_priv, power_domain); >> >> - edid = drm_get_edid(connector, >> - intel_gmbus_get_adapter(dev_priv, >> - intel_hdmi->ddc_bus)); >> + if (intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi))) >> + edid = drm_get_edid(connector, >> + intel_gmbus_get_adapter(dev_priv, >> + intel_hdmi->ddc_bus)); >> >> intel_display_power_put(dev_priv, power_domain); >> >> -- >> 1.7.10.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Jul 09, 2015 at 07:27:57PM +0200, Daniel Vetter wrote: > On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote: > > Adding this for SKL onwards. > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > I think this is the critical piece really, and I'd like to roll it out for > all platforms. git has the code for all the old ones, so no big deal to > digg that out. Also we need your fix for the interrupt handling first ofc, > otherwise this won't work. > > Then we can apply this and give it some good testing before we start > relying on it with caching hdmi probe status. I know this means that > things will be slower, but I've been burned a bit too much the last few > times we've tried this. And I really think we need the most amount of > testing we can get (hence rolling this out for all platforms). If your > hpd irq handling bugfix is indeed the bug that will be confirmed quickly, > otherwise it means back to debugging. "things will be slower" is a bit of an understatement, sadly. On machines with no external display connected (the typical usecase for any device with an eDP, such as a laptop, tablet, etc.), the approach to repeat live status reads until buggy displays can be bothered to reply makes us spend twice as long as needed on resume. While it's nice that we can support buggy hardware, I think that we also, at the very least, should allow for skipping said those workarounds when not needed. Either by allow for disabling the lvie status reads (proven to work on older platforms since that was the default behaviour for a long, long time, and tested to work on at least Broadwell & Skylake ThinkPads) or by making the number of LSR reads configurable. The former would be the simplest solution, the latter would meet the letter of the spec, and allow for more precise tuning of behaviour; people who have a display that needs -- say -- for LSR reads to work reliably shouldn't have to wait for 9. While allowing for unusual use-cases / buggy hardware is great, we shouldn't miss out on the benefits that working hardware can give to the common use-cases. Just my 2¢. I'm feeling sorely tempted to treat this as a proper regression, and simply submit a revert patch, seeing as it slows down resume by something like 200ms, but seeing as there was mention of a case where incorrect EDID-information might end up being read after resume on some Chromebooks, I'll just try to open a discussion instead. Kind regards, David Weinehall
On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote: > On Thu, Jul 09, 2015 at 07:27:57PM +0200, Daniel Vetter wrote: > > On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote: > > > Adding this for SKL onwards. > > > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > > I think this is the critical piece really, and I'd like to roll it out for > > all platforms. git has the code for all the old ones, so no big deal to > > digg that out. Also we need your fix for the interrupt handling first ofc, > > otherwise this won't work. > > > > Then we can apply this and give it some good testing before we start > > relying on it with caching hdmi probe status. I know this means that > > things will be slower, but I've been burned a bit too much the last few > > times we've tried this. And I really think we need the most amount of > > testing we can get (hence rolling this out for all platforms). If your > > hpd irq handling bugfix is indeed the bug that will be confirmed quickly, > > otherwise it means back to debugging. > > "things will be slower" is a bit of an understatement, sadly. > On machines with no external display connected (the typical usecase for > any device with an eDP, such as a laptop, tablet, etc.), the approach to > repeat live status reads until buggy displays can be bothered to reply > makes us spend twice as long as needed on resume. > > While it's nice that we can support buggy hardware, I think that we > also, at the very least, should allow for skipping said those > workarounds when not needed. Either by allow for disabling the > lvie status reads (proven to work on older platforms since that was > the default behaviour for a long, long time, and tested to work > on at least Broadwell & Skylake ThinkPads) or by making the number of > LSR reads configurable. Nack on making probing configurable, that just plain doesn't work. It's the same hole war I've been fighting against adding the live status check until vpg realized that they have this hack to make broken sinks work. > The former would be the simplest solution, the latter would meet the > letter of the spec, and allow for more precise tuning of behaviour; > people who have a display that needs -- say -- for LSR reads to work > reliably shouldn't have to wait for 9. > > While allowing for unusual use-cases / buggy hardware is great, > we shouldn't miss out on the benefits that working hardware can > give to the common use-cases. > > Just my 2¢. > > I'm feeling sorely tempted to treat this as a proper regression, > and simply submit a revert patch, seeing as it slows down resume by > something like 200ms, but seeing as there was mention of a case where > incorrect EDID-information might end up being read after resume on some > Chromebooks, I'll just try to open a discussion instead. +1 on reverts from me on principle. No opinion on this case specifically, but given that we fail the suspend/resume limits since forever I'm inclined to say "meh" here. Probably we should add this as a basic/BAT performance metric, but atm igt/BAT/Jenkins aren't set up to track performance measurements and automatically report/bisect regressions. I think the proper solution should be to make all the probing and fbdev restoring async. As soon as we have working async atomic commit for modeset we should be able to do all that. -Daniel
On Wed, Jul 13, 2016 at 01:59:52PM +0200, Daniel Vetter wrote: > I think the proper solution should be to make all the probing and fbdev > restoring async. As soon as we have working async atomic commit for > modeset we should be able to do all that. We're not just blocked on the mode change here (indeed, we shouldn't be changing modes at resume at all, right?) but we appear to be doing a full detection cycle whereas the intent was just to tell userspace everything had changed, and for it to go figure out what to do about it. Also note that we can simply move this all out of the blocking resume path and still run this in parallel to userspace resuming (and of course serialised with whatever userspace wants to do). -Chris
On Wed, Jul 13, 2016 at 01:09:28PM +0100, Chris Wilson wrote: > On Wed, Jul 13, 2016 at 01:59:52PM +0200, Daniel Vetter wrote: > > I think the proper solution should be to make all the probing and fbdev > > restoring async. As soon as we have working async atomic commit for > > modeset we should be able to do all that. > > We're not just blocked on the mode change here (indeed, we shouldn't be > changing modes at resume at all, right?) but we appear to be doing a > full detection cycle whereas the intent was just to tell userspace > everything had changed, and for it to go figure out what to do about it. > > Also note that we can simply move this all out of the blocking resume > path and still run this in parallel to userspace resuming (and of course > serialised with whatever userspace wants to do). And to remind myself of conversations going on elsewhere, the more async we make resume the more inaccurate the current method of measuing resume time becomes (which more or less just looks at the initcall graph). We need a metric that measures the time from resume to the time of first pixel (first flip to a lit display preferably). I've shown how we can get our "resume time" down to about 10ms - all because the metric is subject to abuse. -Chris
On Wed, Jul 13, 2016 at 01:17:40PM +0100, Chris Wilson wrote: > On Wed, Jul 13, 2016 at 01:09:28PM +0100, Chris Wilson wrote: > > On Wed, Jul 13, 2016 at 01:59:52PM +0200, Daniel Vetter wrote: > > > I think the proper solution should be to make all the probing and fbdev > > > restoring async. As soon as we have working async atomic commit for > > > modeset we should be able to do all that. > > > > We're not just blocked on the mode change here (indeed, we shouldn't be > > changing modes at resume at all, right?) but we appear to be doing a > > full detection cycle whereas the intent was just to tell userspace > > everything had changed, and for it to go figure out what to do about it. > > > > Also note that we can simply move this all out of the blocking resume > > path and still run this in parallel to userspace resuming (and of course > > serialised with whatever userspace wants to do). > > And to remind myself of conversations going on elsewhere, the more async > we make resume the more inaccurate the current method of measuing resume > time becomes (which more or less just looks at the initcall graph). We > need a metric that measures the time from resume to the time of first > pixel (first flip to a lit display preferably). I've shown how we can > get our "resume time" down to about 10ms - all because the metric is > subject to abuse. Good news on this front -- it seems that the SuspendResume tool can be adapted to measure our resume-time even if we "cheat", and the author offered to help with this. So we "just" need to decide what actually constitutes being done with resume. Kind regards, David
On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote: > On Thu, Jul 09, 2015 at 07:27:57PM +0200, Daniel Vetter wrote: > > On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote: > > > Adding this for SKL onwards. > > > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > > > I think this is the critical piece really, and I'd like to roll it out for > > all platforms. git has the code for all the old ones, so no big deal to > > digg that out. Also we need your fix for the interrupt handling first ofc, > > otherwise this won't work. > > > > Then we can apply this and give it some good testing before we start > > relying on it with caching hdmi probe status. I know this means that > > things will be slower, but I've been burned a bit too much the last few > > times we've tried this. And I really think we need the most amount of > > testing we can get (hence rolling this out for all platforms). If your > > hpd irq handling bugfix is indeed the bug that will be confirmed quickly, > > otherwise it means back to debugging. > > "things will be slower" is a bit of an understatement, sadly. > On machines with no external display connected (the typical usecase for > any device with an eDP, such as a laptop, tablet, etc.), the approach to > repeat live status reads until buggy displays can be bothered to reply > makes us spend twice as long as needed on resume. Also this is causing stall during runtime hotplug. ~90ms stall causes 3-4 frames to be dropped in 24Hz movie. Even a single frame dropped during movie playback is enough to be noticed. This coupled with commit f8d03ea0053b23de42c828d559016eabe0b91523 Author: Gary Wang <gary.c.wang@intel.com> Date: Wed Dec 23 16:11:35 2015 +0800 drm/i915: increase the tries for HDMI hotplug live status checking which purports to "fix" live status for bdw+. Afaict using live status is as broken as we told you it would be from earlier experience. -Chris
On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote: > I'm feeling sorely tempted to treat this as a proper regression, > and simply submit a revert patch, seeing as it slows down resume by > something like 200ms, but seeing as there was mention of a case where > incorrect EDID-information might end up being read after resume on some > Chromebooks, I'll just try to open a discussion instead. Wrt https://bugs.freedesktop.org/show_bug.cgi?id=97139 this is a regression and I'll ack the revert. Yes, we need to resolve exactly how we get a stall between intel_hdmi_detect() and pageflip/cursor, but the onus is on the submitter of the patch to fix existing issues first to prevent such regressions (if possible, or else mitigate them). -Chris
Yes we had the issue of incorrect edid read. But as of now you can go ahead with the revert. I have moved to a different group, so I am not working on this anymore. Regards, Sonika -----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Tuesday, August 2, 2016 8:02 PM To: Daniel Vetter <daniel@ffwll.ch>; Jindal, Sonika <sonika.jindal@intel.com>; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 4/5] drm/i915: Check live status before reading edid On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote: > I'm feeling sorely tempted to treat this as a proper regression, and > simply submit a revert patch, seeing as it slows down resume by > something like 200ms, but seeing as there was mention of a case where > incorrect EDID-information might end up being read after resume on > some Chromebooks, I'll just try to open a discussion instead. Wrt https://bugs.freedesktop.org/show_bug.cgi?id=97139 this is a regression and I'll ack the revert. Yes, we need to resolve exactly how we get a stall between intel_hdmi_detect() and pageflip/cursor, but the onus is on the submitter of the patch to fix existing issues first to prevent such regressions (if possible, or else mitigate them). -Chris -- Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 1fb6919..769cf4f 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1300,6 +1300,46 @@ intel_hdmi_unset_edid(struct drm_connector *connector) to_intel_connector(connector)->detect_edid = NULL; } +static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port) +{ + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = to_i915(dev); + enum port port = intel_dig_port->port; + + if (IS_SKYLAKE(dev)) { + u32 temp = I915_READ(SDEISR); + + switch (port) { + case PORT_B: + return temp & SDE_PORTB_HOTPLUG_CPT; + + case PORT_C: + return temp & SDE_PORTC_HOTPLUG_CPT; + + case PORT_D: + return temp & SDE_PORTD_HOTPLUG_CPT; + + default: + return false; + } + } else if (IS_BROXTON(dev)) { + u32 temp = I915_READ(GEN8_DE_PORT_ISR); + + switch (port) { + case PORT_B: + return temp & BXT_DE_PORT_HP_DDIB; + + case PORT_C: + return temp & BXT_DE_PORT_HP_DDIC; + + default: + return false; + + } + } + return true; +} + static bool intel_hdmi_set_edid(struct drm_connector *connector) { @@ -1308,15 +1348,16 @@ intel_hdmi_set_edid(struct drm_connector *connector) struct intel_encoder *intel_encoder = &hdmi_to_dig_port(intel_hdmi)->base; enum intel_display_power_domain power_domain; - struct edid *edid; + struct edid *edid = NULL; bool connected = false; power_domain = intel_display_port_power_domain(intel_encoder); intel_display_power_get(dev_priv, power_domain); - edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + if (intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi))) + edid = drm_get_edid(connector, + intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus)); intel_display_power_put(dev_priv, power_domain);
Adding this for SKL onwards. Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/intel_hdmi.c | 49 ++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 4 deletions(-)