| Message ID | 1407847101-21654-3-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote: > From: Shashank Sharma <shashank.sharma@intel.com> > > During the HDMI complaince tests, most of the HDMI analyzers > issue a soft HPD, to automate the tests. This process keeps > the HDMI cable connected, and DDC chhanel alive. > > HDMI detect() logic relies on EDID readability, to decide if > its a HDMI connect or disconnect event. But in this case, the > EDID is always readable, as HDMI cable is physically connected > and DDC channel is UP, so detect() always reports a HDMI connect > even if its intended to be a HDMI disconnect call. > > So if HDMI compliance is enabled, we should rely on the live status > register, than EDID availability. This patch adds: > 1. One kernel command line parameter for i915 module, which indicates > if we want to support HDMI compliance, for this platform. I would rather have this as an output property. In fact, I would like the hotplug detection method exposed (and even selectable, but other than this compliance testing, I can't think of a scenario where the kernel shouldn't be able to figure things out for itself). > 2. A new function to read EDID, which gets called only in case of > HDMI compliance support is required. This function reads EDID only > if live status register is up. The normal code flow doesn't get effected > if kernel command line parameter is not enabled. > 3. After various experiments on VLV2 board, with various HDMI monitors > we have seen that, with few monitors, the live status register gets > set after a slight delay, and then stays reliably. To support such > monitors, there is a busy-loop added, with a max delay upto 50ms, > with a status check after every 10ms. Please see the comment in > intel_hdmi_get_edid. Wouldn't a tidier solution be to delay the hpd by 50-100ms after the hotplug interrupt? That may overcome the issue with the live status for all connectors... -Chris
Hello Chris,
Thanks for your time and comments.
I would like to give a brief history of the patch.
We tried to apply this optimization by default, and check all the EDID
read based on the live status. But not all developers agreed to have
this by default, with following reasons:
1. live_status was not very reliable for all platforms, so live_status
based solution shouldn't be added.
2. they dint want EDID caching to be by default, as few old platforms
were not even HPD capable.
So we came up with this intermediate solution to have:
1. Timeout based EDID caching, where cached EDID will be cleared after
one minute.
2. An alternative code flow to support HDMI compliance (will be based
on the live_status check, and will be only enabled for commercial
platforms like android) which need HDMI compliance support. So the
kernel command line parameter is to support the need to add this
alternative EDID read method.
Please le me know your opinion about this, considering the background.
Regards
Shashank
On 8/12/2014 6:17 PM, Chris Wilson wrote:
> On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote:
>> From: Shashank Sharma <shashank.sharma@intel.com>
>>
>> During the HDMI complaince tests, most of the HDMI analyzers
>> issue a soft HPD, to automate the tests. This process keeps
>> the HDMI cable connected, and DDC chhanel alive.
>>
>> HDMI detect() logic relies on EDID readability, to decide if
>> its a HDMI connect or disconnect event. But in this case, the
>> EDID is always readable, as HDMI cable is physically connected
>> and DDC channel is UP, so detect() always reports a HDMI connect
>> even if its intended to be a HDMI disconnect call.
>>
>> So if HDMI compliance is enabled, we should rely on the live status
>> register, than EDID availability. This patch adds:
>> 1. One kernel command line parameter for i915 module, which indicates
>> if we want to support HDMI compliance, for this platform.
>
> I would rather have this as an output property. In fact, I would like
> the hotplug detection method exposed (and even selectable, but other
> than this compliance testing, I can't think of a scenario where the
> kernel shouldn't be able to figure things out for itself).
>
>> 2. A new function to read EDID, which gets called only in case of
>> HDMI compliance support is required. This function reads EDID only
>> if live status register is up. The normal code flow doesn't get effected
>> if kernel command line parameter is not enabled.
>> 3. After various experiments on VLV2 board, with various HDMI monitors
>> we have seen that, with few monitors, the live status register gets
>> set after a slight delay, and then stays reliably. To support such
>> monitors, there is a busy-loop added, with a max delay upto 50ms,
>> with a status check after every 10ms. Please see the comment in
>> intel_hdmi_get_edid.
>
> Wouldn't a tidier solution be to delay the hpd by 50-100ms after the
> hotplug interrupt? That may overcome the issue with the live status for
> all connectors...
> -Chris
>
On Tue, Aug 12, 2014 at 08:56:44PM +0530, Sharma, Shashank wrote: > Hello Chris, > > Thanks for your time and comments. > I would like to give a brief history of the patch. > > We tried to apply this optimization by default, and check all the > EDID read based on the live status. But not all developers agreed to > have this by default, with following reasons: > 1. live_status was not very reliable for all platforms, so live_status > based solution shouldn't be added. > 2. they dint want EDID caching to be by default, as few old platforms > were not even HPD capable. > > So we came up with this intermediate solution to have: > 1. Timeout based EDID caching, where cached EDID will be cleared after > one minute. > 2. An alternative code flow to support HDMI compliance (will be based > on the live_status check, and will be only enabled for commercial > platforms like android) which need HDMI compliance support. So the > kernel command line parameter is to support the need to add this > alternative EDID read method. > > Please le me know your opinion about this, considering the background. I know. That is orthogonal to the tweaks I was suggesting. Also if you feel you need to add details to your rationale, then your changelog is incomplete. -Chris
> I know. That is orthogonal to the tweaks I was suggesting. Also if you > feel you need to add details to your rationale, then your changelog is > incomplete. > -Chris Thanks Chris, Please find my comments inline to your previous mail, with suggestions. On 8/12/2014 6:17 PM, Chris Wilson wrote: > On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote: >> From: Shashank Sharma <shashank.sharma@intel.com> >> >> During the HDMI complaince tests, most of the HDMI analyzers >> issue a soft HPD, to automate the tests. This process keeps >> the HDMI cable connected, and DDC chhanel alive. >> >> HDMI detect() logic relies on EDID readability, to decide if >> its a HDMI connect or disconnect event. But in this case, the >> EDID is always readable, as HDMI cable is physically connected >> and DDC channel is UP, so detect() always reports a HDMI connect >> even if its intended to be a HDMI disconnect call. >> >> So if HDMI compliance is enabled, we should rely on the live status >> register, than EDID availability. This patch adds: >> 1. One kernel command line parameter for i915 module, which indicates >> if we want to support HDMI compliance, for this platform. > > I would rather have this as an output property. In fact, I would like > the hotplug detection method exposed (and even selectable, but other > than this compliance testing, I can't think of a scenario where the > kernel shouldn't be able to figure things out for itself). Considering the history of the case, can you please elaborate this suggestion ? I dont think I am getting it right. > >> 2. A new function to read EDID, which gets called only in case of >> HDMI compliance support is required. This function reads EDID only >> if live status register is up. The normal code flow doesn't get effected >> if kernel command line parameter is not enabled. >> 3. After various experiments on VLV2 board, with various HDMI monitors >> we have seen that, with few monitors, the live status register gets >> set after a slight delay, and then stays reliably. To support such >> monitors, there is a busy-loop added, with a max delay upto 50ms, >> with a status check after every 10ms. Please see the comment in >> intel_hdmi_get_edid. > > Wouldn't a tidier solution be to delay the hpd by 50-100ms after the > hotplug interrupt? That may overcome the issue with the live status for > all connectors... > -Chris > There would be few problems in this case: 1. We dont want this scenario to come into picture for DP, as DP HPD pulse can be as small as 2ms. 2. Not all the HDMI monitors show this problem, but a significant subset of popular monitors do. 3. In HDCP compliance testing, we send a HPD pulse train of UP and Down, where down pulse can be as small as 100ms. If we increase the delay by 100ms, we will definitely miss the HPD down pulse. 4. The method what we are using is a busy waiting check, where we delay the pulse for 50ms, but take a sample of live_status per 10ms, so if the live status is up with a delay of 20ms, we needn't to waste another 30. 5. We want this code routine only to be executed for commercial (like android) platforms, whereas others get the routine code. @ Danvet: Do you want to add something here ? Regards Shashank
On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote: > > I know. That is orthogonal to the tweaks I was suggesting. Also if you > > feel you need to add details to your rationale, then your changelog is > > incomplete. > > -Chris > > Thanks Chris, > Please find my comments inline to your previous mail, with suggestions. > > On 8/12/2014 6:17 PM, Chris Wilson wrote: > >On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote: > >>From: Shashank Sharma <shashank.sharma@intel.com> > >> > >>During the HDMI complaince tests, most of the HDMI analyzers > >>issue a soft HPD, to automate the tests. This process keeps > >>the HDMI cable connected, and DDC chhanel alive. > >> > >>HDMI detect() logic relies on EDID readability, to decide if > >>its a HDMI connect or disconnect event. But in this case, the > >>EDID is always readable, as HDMI cable is physically connected > >>and DDC channel is UP, so detect() always reports a HDMI connect > >>even if its intended to be a HDMI disconnect call. > >> > >>So if HDMI compliance is enabled, we should rely on the live status > >>register, than EDID availability. This patch adds: > >>1. One kernel command line parameter for i915 module, which indicates > >> if we want to support HDMI compliance, for this platform. > > > >I would rather have this as an output property. In fact, I would like > >the hotplug detection method exposed (and even selectable, but other > >than this compliance testing, I can't think of a scenario where the > >kernel shouldn't be able to figure things out for itself). > Considering the history of the case, can you please elaborate this > suggestion ? I dont think I am getting it right. Instead of (or in addition to) adding a kernel parameter, you add an output property so that it can be adjusted on the fly for individual monitors. > > > >>2. A new function to read EDID, which gets called only in case of > >> HDMI compliance support is required. This function reads EDID only > >> if live status register is up. The normal code flow doesn't get effected > >> if kernel command line parameter is not enabled. > >>3. After various experiments on VLV2 board, with various HDMI monitors > >> we have seen that, with few monitors, the live status register gets > >> set after a slight delay, and then stays reliably. To support such > >> monitors, there is a busy-loop added, with a max delay upto 50ms, > >> with a status check after every 10ms. Please see the comment in > >> intel_hdmi_get_edid. > > > >Wouldn't a tidier solution be to delay the hpd by 50-100ms after the > >hotplug interrupt? That may overcome the issue with the live status for > >all connectors... > >-Chris > > > There would be few problems in this case: > 1. We dont want this scenario to come into picture for DP, as DP HPD > pulse can be as small as 2ms. If the live status is asserted and deasserted within 2ms, do you care? Or perhaps you are talking about something else entirely. > 2. Not all the HDMI monitors show this problem, but a significant > subset of popular monitors do. > 3. In HDCP compliance testing, we send a HPD pulse train of UP and > Down, where down pulse can be as small as 100ms. If we increase the > delay by 100ms, we will definitely miss the HPD down pulse. And? If the monitor is only plugged in for less than 0.1s do I really want to waste 1-2s of user time reconfiguring the desktop and applications before undoing all the changes? There is no point in compliance testing if it does not actually test the code going to be used. > 4. The method what we are using is a busy waiting check, where we delay > the pulse for 50ms, but take a sample of live_status per 10ms, so if > the live status is up with a delay of 20ms, we needn't to waste > another 30. Yes. You block using 100% of the cpu in an uninterruptable context for a significant period of time. DO NOT DO THIS. > 5. We want this code routine only to be executed for commercial (like > android) platforms, whereas others get the routine code. In other words, you want to ignore years of real world compatibity testing and larger user bases. -Chris
On 8/13/2014 11:46 AM, Chris Wilson wrote: > On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote: >>> I know. That is orthogonal to the tweaks I was suggesting. Also if you >>> feel you need to add details to your rationale, then your changelog is >>> incomplete. >>> -Chris >> >> Thanks Chris, >> Please find my comments inline to your previous mail, with suggestions. >> >> On 8/12/2014 6:17 PM, Chris Wilson wrote: >>> On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sharma@intel.com wrote: >>>> From: Shashank Sharma <shashank.sharma@intel.com> >>>> >>>> During the HDMI complaince tests, most of the HDMI analyzers >>>> issue a soft HPD, to automate the tests. This process keeps >>>> the HDMI cable connected, and DDC chhanel alive. >>>> >>>> HDMI detect() logic relies on EDID readability, to decide if >>>> its a HDMI connect or disconnect event. But in this case, the >>>> EDID is always readable, as HDMI cable is physically connected >>>> and DDC channel is UP, so detect() always reports a HDMI connect >>>> even if its intended to be a HDMI disconnect call. >>>> >>>> So if HDMI compliance is enabled, we should rely on the live status >>>> register, than EDID availability. This patch adds: >>>> 1. One kernel command line parameter for i915 module, which indicates >>>> if we want to support HDMI compliance, for this platform. >>> >>> I would rather have this as an output property. In fact, I would like >>> the hotplug detection method exposed (and even selectable, but other >>> than this compliance testing, I can't think of a scenario where the >>> kernel shouldn't be able to figure things out for itself). >> Considering the history of the case, can you please elaborate this >> suggestion ? I dont think I am getting it right. > > Instead of (or in addition to) adding a kernel parameter, you add an > output property so that it can be adjusted on the fly for individual > monitors. > Yes, this can be done. This might cause some problems with the first modeset from driver (like fb_console), where there is no userspace to set a property yet, but I think its manageable. >>> >>>> 2. A new function to read EDID, which gets called only in case of >>>> HDMI compliance support is required. This function reads EDID only >>>> if live status register is up. The normal code flow doesn't get effected >>>> if kernel command line parameter is not enabled. >>>> 3. After various experiments on VLV2 board, with various HDMI monitors >>>> we have seen that, with few monitors, the live status register gets >>>> set after a slight delay, and then stays reliably. To support such >>>> monitors, there is a busy-loop added, with a max delay upto 50ms, >>>> with a status check after every 10ms. Please see the comment in >>>> intel_hdmi_get_edid. >>> >>> Wouldn't a tidier solution be to delay the hpd by 50-100ms after the >>> hotplug interrupt? That may overcome the issue with the live status for >>> all connectors... >>> -Chris >>> >> There would be few problems in this case: >> 1. We dont want this scenario to come into picture for DP, as DP HPD >> pulse can be as small as 2ms. > > If the live status is asserted and deasserted within 2ms, do you care? > Or perhaps you are talking about something else entirely. > Actually what I mean was, in case of compliance, there would be an expectation for Port Up/Down/UP, but what we will get is UP/UP This can fail the test case. >> 2. Not all the HDMI monitors show this problem, but a significant >> subset of popular monitors do. >> 3. In HDCP compliance testing, we send a HPD pulse train of UP and >> Down, where down pulse can be as small as 100ms. If we increase the >> delay by 100ms, we will definitely miss the HPD down pulse. > > And? If the monitor is only plugged in for less than 0.1s do I really > want to waste 1-2s of user time reconfiguring the desktop and > applications before undoing all the changes? > > There is no point in compliance testing if it does not actually test the > code going to be used. > I completely agree, but unfortunately if the test case fails, this would be blocker for commercial projects. The test clearly mentions we should respond properly to the HPD pulse train. >> 4. The method what we are using is a busy waiting check, where we delay >> the pulse for 50ms, but take a sample of live_status per 10ms, so if >> the live status is up with a delay of 20ms, we needn't to waste >> another 30. > > Yes. You block using 100% of the cpu in an uninterruptable context for a > significant period of time. DO NOT DO THIS. > I can very well switch to a sleep, but was not sure if the context switching 5 times for 10ms is beneficial :), please correct me if this is not ok. >> 5. We want this code routine only to be executed for commercial (like >> android) platforms, whereas others get the routine code. > > In other words, you want to ignore years of real world compatibity testing > and larger user bases. > -Chris > I do not mean this for sure, in fact we would be happy if the community accepts this for regular code flow also, but due to their previous objections and stand for not to go for a live_status based solution, made to to come via this route. Please suggest how can we do this in better way. Regards Shashank
On Wed, Aug 13, 2014 at 01:12:12PM +0530, Sharma, Shashank wrote: > On 8/13/2014 11:46 AM, Chris Wilson wrote: > >On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote: > >>5. We want this code routine only to be executed for commercial (like > >> android) platforms, whereas others get the routine code. > > > >In other words, you want to ignore years of real world compatibity testing > >and larger user bases. > > > I do not mean this for sure, in fact we would be happy if the community > accepts this for regular code flow also, but due to their previous > objections and stand for not to go for a live_status based solution, made to > to come via this route. Please suggest how can we do this in better way. So I'm grumpily ok with a special validation mode to get a sticker. Occasionally there's a business need for those stickers, and it kinda makes sense to merge that code upstream. But in the end reality in the form of existing broken hdmi screens out there wins. Always. And you actually learned that already by adding hacks to your validation-only path to make it actually work in reality. Sooner or later you need to add all the other crap we've accumulated too, or the driver will simply not work. Which means we'll actually not end up with a restricted validation-only hack, but duplicated code&bugs. I'm _not_ going to have 2 separate paths with real-world quirks. If there's anything actually broken with the current code in upstream, we need to fix that one, not add a new one because that's easier. Because it's not. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2a372f2..19e4f97 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2162,6 +2162,7 @@ struct i915_params { bool disable_vtd_wa; int use_mmio_flip; bool mmio_debug; + bool hdmi_compliance_support; }; extern struct i915_params i915 __read_mostly; diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 62ee830..27dcc1a 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -50,6 +50,7 @@ struct i915_params i915 __read_mostly = { .disable_vtd_wa = 0, .use_mmio_flip = 0, .mmio_debug = 0, + .hdmi_compliance_support = 0, }; module_param_named(modeset, i915.modeset, int, 0400); @@ -167,3 +168,8 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600); MODULE_PARM_DESC(mmio_debug, "Enable the MMIO debug code (default: false). This may negatively " "affect performance."); + +module_param_named(hdmi_compliance_support, i915.hdmi_compliance_support, + bool, 0400); +MODULE_PARM_DESC(hdmi_compliance_support, + "Support for HDMI compliance in i915 driver 1=On 0=Off"); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 185a45a..0b3798a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -111,6 +111,7 @@ #define INTEL_DSI_COMMAND_MODE 1 #define INTEL_HDMI_EDID_CACHING_SEC 60 +#define INTEL_HDMI_LIVE_STATUS_DELAY_MS 10 struct intel_framebuffer { struct drm_framebuffer base; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 8dc3970..06eb9de 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -978,6 +978,90 @@ static void intel_hdmi_invalidate_edid(struct work_struct *work) DRM_DEBUG_DRIVER("cleaned up cached EDID\n"); } +/* Get HDMI live status */ +static bool intel_hdmi_live_status(struct drm_device *dev, + struct intel_hdmi *intel_hdmi) +{ + uint32_t status_bit; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_digital_port *intel_dig_port = + hdmi_to_dig_port(intel_hdmi); + + DRM_DEBUG_KMS("Reading Live status\n"); + + /* Live status bit is 29 for PORT_B, 28 for C and 27 for D */ + status_bit = (PORTB_HOTPLUG_LIVE_STATUS_VLV >> + (PORT_B - intel_dig_port->port)); + + return I915_READ(PORT_HOTPLUG_STAT) & status_bit; +} + +/* Read DDC and get EDID */ +struct edid *intel_hdmi_get_edid(struct drm_connector *connector, bool force) +{ + bool live_status = false; + struct edid *new_edid = NULL; + struct i2c_adapter *adapter; + struct drm_device *dev = connector->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); + u8 retry = 5; + + if (!intel_hdmi) { + DRM_ERROR("Invalid input to get hdmi\n"); + return NULL; + } + + /* + * Read EDID only when live status is up. + * This condition is required for HDMI analyzers, where + * there is no physical disconnect of HDMI cable, but + * only a HPD is issued. In this case, if we decide the status of + * HDMI based on EDID availability from DDC channel, its always + * available. So rely on live status. + */ + while (retry--) { + /* + * Many HDMI monitors set the live status after some delay + * Add some tolerance for them. From few experiments on VLV + * with a set of monitors, we got to know that a delay close to + * 50 ms can cover this gap. + * Retry reading live status 5 times, in 10ms duration each + * But make sure that you be well within limit of a HDCP unplug + * which can be as small as 100ms + */ + live_status = intel_hdmi_live_status(dev, intel_hdmi); + if (!live_status) + mdelay(INTEL_HDMI_LIVE_STATUS_DELAY_MS); + else + break; + } + + /* + * Read EDID only if live status is up OR we are forced + * with a knife + */ + if (live_status || force) { + DRM_DEBUG_DRIVER("HDMI Live status is up\n"); + adapter = intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus); + + if (!adapter) { + DRM_ERROR("Get_hdmi cant get adapter\n"); + return NULL; + } + + new_edid = drm_get_edid(connector, adapter); + if (!new_edid) { + DRM_ERROR("Get_hdmi cant read edid\n"); + return NULL; + } + + DRM_DEBUG_KMS("Live status up, got EDID"); + } + return new_edid; +} + static enum drm_connector_status intel_hdmi_detect(struct drm_connector *connector, bool force) { @@ -1035,9 +1119,12 @@ skip_optimization: * You are well deserving, dear code, as you have survived * all the optimizations. Now go and enjoy reading EDID */ - edid = drm_get_edid(connector, + if (i915.hdmi_compliance_support && (INTEL_INFO(dev)->gen >= 6)) + edid = intel_hdmi_get_edid(connector, false); + else + edid = drm_get_edid(connector, intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + intel_hdmi->ddc_bus)); /* * Now when we have read new EDID, update cached EDID with * latest (both NULL or non NULL). Cancel the delayed work