| Message ID | 1435643038-3078-3-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote: > From: Shashank Sharma <contactshashanksharma@gmail.com> > > This patch makes sure that the HDMI detect function > reads EDID only when its forced to do it. All the other > times, it uses the connector->detect_edid which was cached > during hotplug handling in the hdmi_probe() function. As the > probe function gets called before detect in the interrupt handler > and handles the EDID cacheing part, its absolutely safe to assume > that presence of EDID reflects monitor connected and viceversa. > > This will save us from many race conditions between hotplug/unplug > detect call handler threads and userspace calls for the same. > The previous patch in this patch series explains this in detail. > > Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 26 ++++++++++++++++++++------ > 1 file changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 064ddd8..1fb6919 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1362,19 +1362,33 @@ static enum drm_connector_status > intel_hdmi_detect(struct drm_connector *connector, bool force) > { > enum drm_connector_status status; > + struct intel_connector *intel_connector = > + to_intel_connector(connector); > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > + /* > + * There are many userspace calls which probe EDID from > + * detect path. In case on multiple hotplug/unplug, these > + * can cause race conditions while probing EDID. Also its > + * waste of CPU cycles to read the EDID again and again > + * unless there is a real hotplug. > + * So until we are forced, check connector status > + * based on availability of cached EDID. This will avoid many of > + * these race conditions and timing problems. > + */ > + if (force) Userspace always sets force. Are you sure this actually improves anything? Also the goal should be to keep things cache for a few calls from userspace (since often it pokes a few times in a row unfortuantely), for which we need a proper timeout to clear the edid again. -Daniel > + intel_hdmi_probe(intel_connector->encoder); > > - intel_hdmi_unset_edid(connector); > - > - if (intel_hdmi_set_edid(connector)) { > + if (intel_connector->detect_edid) { > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > - > - hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; > status = connector_status_connected; > - } else > + hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; > + DRM_DEBUG_DRIVER("hdmi status = connected\n"); > + } else { > status = connector_status_disconnected; > + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); > + } > > return status; > } > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> Userspace always sets force. Are you sure this actually improves anything? Yes we do. We have had this code for commercial projects, and that's really important to have proper interrupt handling as well as to avoid race condition between multiple HDMI detects from interrupt handler and userspace detect calls. This is a must for HDMI compliance also. Actually the plan is to use this force for GEN < 6 HW only, where the hotplug doesn't work reliably (I remember our last conversation on some old HW which doesn't support HPD properly). For vlv+, we can (will) use only the cached EDID. > Also the goal should be to keep things cache for a few calls from > userspace (since often it pokes a few times in a row unfortuantely), for > which we need a proper timeout to clear the edid again. Can you please let us know why ? Why do we need to clear this EDID caching ? We should clear it only in the next hot-unplug, and maintain this cached EDID for all userspace detect operations. I believe as long as we have the state machine maintained, we need not to clear it. >-Daniel Regards Shashank On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote: > > From: Shashank Sharma <contactshashanksharma@gmail.com> > > > > This patch makes sure that the HDMI detect function > > reads EDID only when its forced to do it. All the other > > times, it uses the connector->detect_edid which was cached > > during hotplug handling in the hdmi_probe() function. As the > > probe function gets called before detect in the interrupt handler > > and handles the EDID cacheing part, its absolutely safe to assume > > that presence of EDID reflects monitor connected and viceversa. > > > > This will save us from many race conditions between hotplug/unplug > > detect call handler threads and userspace calls for the same. > > The previous patch in this patch series explains this in detail. > > > > Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com> > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 26 ++++++++++++++++++++------ > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > > index 064ddd8..1fb6919 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1362,19 +1362,33 @@ static enum drm_connector_status > > intel_hdmi_detect(struct drm_connector *connector, bool force) > > { > > enum drm_connector_status status; > > + struct intel_connector *intel_connector = > > + to_intel_connector(connector); > > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > connector->base.id, connector->name); > > + /* > > + * There are many userspace calls which probe EDID from > > + * detect path. In case on multiple hotplug/unplug, these > > + * can cause race conditions while probing EDID. Also its > > + * waste of CPU cycles to read the EDID again and again > > + * unless there is a real hotplug. > > + * So until we are forced, check connector status > > + * based on availability of cached EDID. This will avoid many of > > + * these race conditions and timing problems. > > + */ > > + if (force) > > Userspace always sets force. Are you sure this actually improves anything? > Also the goal should be to keep things cache for a few calls from > userspace (since often it pokes a few times in a row unfortuantely), for > which we need a proper timeout to clear the edid again. > -Daniel > > > + intel_hdmi_probe(intel_connector->encoder); > > > > - intel_hdmi_unset_edid(connector); > > - > > - if (intel_hdmi_set_edid(connector)) { > > + if (intel_connector->detect_edid) { > > struct intel_hdmi *intel_hdmi = > intel_attached_hdmi(connector); > > - > > - hdmi_to_dig_port(intel_hdmi)->base.type = > INTEL_OUTPUT_HDMI; > > status = connector_status_connected; > > - } else > > + hdmi_to_dig_port(intel_hdmi)->base.type = > INTEL_OUTPUT_HDMI; > > + DRM_DEBUG_DRIVER("hdmi status = connected\n"); > > + } else { > > status = connector_status_disconnected; > > + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); > > + } > > > > return status; > > } > > -- > > 1.7.10.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote: > > Userspace always sets force. Are you sure this actually improves anything? > Yes we do. We have had this code for commercial projects, and that's really > important to have proper interrupt handling as well as to avoid race > condition between multiple HDMI detects from interrupt handler and > userspace detect calls. This is a must for HDMI compliance also. There's no race, we have locks for this. And we already have a little bit of edid caching, and you're code won't add more caching for the force = true case. Which is why I wondered whether you've really seen improvements with this on latest upstream, and not just an older android tree. > Actually the plan is to use this force for GEN < 6 HW only, where the > hotplug doesn't work reliably (I remember our last conversation on some old > HW which doesn't support HPD properly). For vlv+, we can (will) use only > the cached EDID. Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which is half-busted it seems, and we've found examples for every single platform that supports hdmi out there. The problem isn't necessarily spec compliant HDMI sinks, but all the other crap ppl like to plug in. Yes this means we'll not be spec compliant, but if we have reality vs. spec, reality wins. At least in upstream. > > Also the goal should be to keep things cache for a few calls from > > userspace (since often it pokes a few times in a row unfortuantely), for > > which we need a proper timeout to clear the edid again. > > Can you please let us know why ? Why do we need to clear this EDID caching > ? We should clear it only in the next hot-unplug, and maintain this cached > EDID for all userspace detect operations. I believe as long as we have the > state machine maintained, we need not to clear it. hotplug is not reliable, at least not outside of labs and validation testers. And your code here throws the cached edid away every time force = true is set, which is pretty much always. At least on upstream. The only place where we don't set force is in the poll worker, and that's only run when we have a hpd storm. -Daniel > > >-Daniel > > Regards > Shashank > > On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote: > > > From: Shashank Sharma <contactshashanksharma@gmail.com> > > > > > > This patch makes sure that the HDMI detect function > > > reads EDID only when its forced to do it. All the other > > > times, it uses the connector->detect_edid which was cached > > > during hotplug handling in the hdmi_probe() function. As the > > > probe function gets called before detect in the interrupt handler > > > and handles the EDID cacheing part, its absolutely safe to assume > > > that presence of EDID reflects monitor connected and viceversa. > > > > > > This will save us from many race conditions between hotplug/unplug > > > detect call handler threads and userspace calls for the same. > > > The previous patch in this patch series explains this in detail. > > > > > > Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com> > > > --- > > > drivers/gpu/drm/i915/intel_hdmi.c | 26 ++++++++++++++++++++------ > > > 1 file changed, 20 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > > b/drivers/gpu/drm/i915/intel_hdmi.c > > > index 064ddd8..1fb6919 100644 > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > @@ -1362,19 +1362,33 @@ static enum drm_connector_status > > > intel_hdmi_detect(struct drm_connector *connector, bool force) > > > { > > > enum drm_connector_status status; > > > + struct intel_connector *intel_connector = > > > + to_intel_connector(connector); > > > > > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > > > connector->base.id, connector->name); > > > + /* > > > + * There are many userspace calls which probe EDID from > > > + * detect path. In case on multiple hotplug/unplug, these > > > + * can cause race conditions while probing EDID. Also its > > > + * waste of CPU cycles to read the EDID again and again > > > + * unless there is a real hotplug. > > > + * So until we are forced, check connector status > > > + * based on availability of cached EDID. This will avoid many of > > > + * these race conditions and timing problems. > > > + */ > > > + if (force) > > > > Userspace always sets force. Are you sure this actually improves anything? > > Also the goal should be to keep things cache for a few calls from > > userspace (since often it pokes a few times in a row unfortuantely), for > > which we need a proper timeout to clear the edid again. > > -Daniel > > > > > + intel_hdmi_probe(intel_connector->encoder); > > > > > > - intel_hdmi_unset_edid(connector); > > > - > > > - if (intel_hdmi_set_edid(connector)) { > > > + if (intel_connector->detect_edid) { > > > struct intel_hdmi *intel_hdmi = > > intel_attached_hdmi(connector); > > > - > > > - hdmi_to_dig_port(intel_hdmi)->base.type = > > INTEL_OUTPUT_HDMI; > > > status = connector_status_connected; > > > - } else > > > + hdmi_to_dig_port(intel_hdmi)->base.type = > > INTEL_OUTPUT_HDMI; > > > + DRM_DEBUG_DRIVER("hdmi status = connected\n"); > > > + } else { > > > status = connector_status_disconnected; > > > + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); > > > + } > > > > > > return status; > > > } > > > -- > > > 1.7.10.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > >
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6674
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK -1 302/302 301/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT 289/289 289/289
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt@kms_flip@flip-vs-rmfb-interruptible PASS(1) DMESG_WARN(1)
(dmesg patch applied)drm:intel_pch_fifo_underrun_irq_handler[i915]]*ERROR*PCH_transcoder_A_FIFO_underrun@PCH transcoder A FIFO underrun
Note: You need to pay more attention to line start with '*'
Regards Shashank On 7/1/2015 6:26 PM, Daniel Vetter wrote: > On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote: >>> Userspace always sets force. Are you sure this actually improves anything? >> Yes we do. We have had this code for commercial projects, and that's really >> important to have proper interrupt handling as well as to avoid race >> condition between multiple HDMI detects from interrupt handler and >> userspace detect calls. This is a must for HDMI compliance also. > > There's no race, we have locks for this. And we already have a little bit > of edid caching, and you're code won't add more caching for the force = > true case. Which is why I wondered whether you've really seen improvements > with this on latest upstream, and not just an older android tree. This caching is not useful, as in every detect call, we are doing a unset EDID, and doing a set EDID again. The actual reason behind multiple HDMI EDID reads is detect() getting called from user space only.So every time there is a detect call, there is HDMI EDID read. We should read EDID only on hotplug, cache it, and reuse it until hot unplug. > >> Actually the plan is to use this force for GEN < 6 HW only, where the >> hotplug doesn't work reliably (I remember our last conversation on some old >> HW which doesn't support HPD properly). For vlv+, we can (will) use only >> the cached EDID. > > Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which > is half-busted it seems, and we've found examples for every single > platform that supports hdmi out there. The problem isn't necessarily spec > compliant HDMI sinks, but all the other crap ppl like to plug in. > > Yes this means we'll not be spec compliant, but if we have reality vs. > spec, reality wins. At least in upstream. We have tested HPD with several HDMI monitors for VLV, CHV SKL and now for BXT also. We are getting reliable hotplugs and unplugs across these platforms, with accurate information on long/short pulses. Can you please give some details about what is your observation ? Its very important for the Android projects to comply with the spec due to certification pressure from customers. And we can get a common path for us, if we know what exactly is the problem. But for sure we cant ignore this factor that compliance is essential. > >>> Also the goal should be to keep things cache for a few calls from >>> userspace (since often it pokes a few times in a row unfortuantely), for >>> which we need a proper timeout to clear the edid again. >> >> Can you please let us know why ? Why do we need to clear this EDID caching >> ? We should clear it only in the next hot-unplug, and maintain this cached >> EDID for all userspace detect operations. I believe as long as we have the >> state machine maintained, we need not to clear it. > > hotplug is not reliable, at least not outside of labs and validation > testers. And your code here throws the cached edid away every time force = > true is set, which is pretty much always. At least on upstream. > > The only place where we don't set force is in the poll worker, and that's > only run when we have a hpd storm. > -Daniel The new code doesn't throw away cached EDID for platform's > GEN6 but the current code does that, in every detect call. The current state machine is: ============================= 1. Hotplug -> Unset EDID, Read EDID, Set edid 2. all detect calls -> Unset EDID, read EDID, Set EDID 3. Hotunplug -> Unset EDID The state machine I am suggesting is: ===================================== 1. Hotplug -> throw away cached EDID, cache new one probing DDC 2. all detect() calls -> use cached EDID only 3. hot unplug -> throw away cached EDID, OR if you want to support some old platforms: ============================================= 1. Hotplug -> throw away cached EDID, cache new one probing DDC 2. all detect() calls -> (Support old HW with unstable HPD) if (gen > GEN6) use cached EDID only else probe DDC and read EDID, update cache 3. hot unplug -> throw away cached EDID, > >> >>> -Daniel >> >> Regards >> Shashank >> >> On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> >>> On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote: >>>> From: Shashank Sharma <contactshashanksharma@gmail.com> >>>> >>>> This patch makes sure that the HDMI detect function >>>> reads EDID only when its forced to do it. All the other >>>> times, it uses the connector->detect_edid which was cached >>>> during hotplug handling in the hdmi_probe() function. As the >>>> probe function gets called before detect in the interrupt handler >>>> and handles the EDID cacheing part, its absolutely safe to assume >>>> that presence of EDID reflects monitor connected and viceversa. >>>> >>>> This will save us from many race conditions between hotplug/unplug >>>> detect call handler threads and userspace calls for the same. >>>> The previous patch in this patch series explains this in detail. >>>> >>>> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_hdmi.c | 26 ++++++++++++++++++++------ >>>> 1 file changed, 20 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c >>> b/drivers/gpu/drm/i915/intel_hdmi.c >>>> index 064ddd8..1fb6919 100644 >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>> @@ -1362,19 +1362,33 @@ static enum drm_connector_status >>>> intel_hdmi_detect(struct drm_connector *connector, bool force) >>>> { >>>> enum drm_connector_status status; >>>> + struct intel_connector *intel_connector = >>>> + to_intel_connector(connector); >>>> >>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>> connector->base.id, connector->name); >>>> + /* >>>> + * There are many userspace calls which probe EDID from >>>> + * detect path. In case on multiple hotplug/unplug, these >>>> + * can cause race conditions while probing EDID. Also its >>>> + * waste of CPU cycles to read the EDID again and again >>>> + * unless there is a real hotplug. >>>> + * So until we are forced, check connector status >>>> + * based on availability of cached EDID. This will avoid many of >>>> + * these race conditions and timing problems. >>>> + */ >>>> + if (force) >>> >>> Userspace always sets force. Are you sure this actually improves anything? >>> Also the goal should be to keep things cache for a few calls from >>> userspace (since often it pokes a few times in a row unfortuantely), for >>> which we need a proper timeout to clear the edid again. >>> -Daniel >>> >>>> + intel_hdmi_probe(intel_connector->encoder); >>>> >>>> - intel_hdmi_unset_edid(connector); >>>> - >>>> - if (intel_hdmi_set_edid(connector)) { >>>> + if (intel_connector->detect_edid) { >>>> struct intel_hdmi *intel_hdmi = >>> intel_attached_hdmi(connector); >>>> - >>>> - hdmi_to_dig_port(intel_hdmi)->base.type = >>> INTEL_OUTPUT_HDMI; >>>> status = connector_status_connected; >>>> - } else >>>> + hdmi_to_dig_port(intel_hdmi)->base.type = >>> INTEL_OUTPUT_HDMI; >>>> + DRM_DEBUG_DRIVER("hdmi status = connected\n"); >>>> + } else { >>>> status = connector_status_disconnected; >>>> + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); >>>> + } >>>> >>>> return status; >>>> } >>>> -- >>>> 1.7.10.4 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>> >>> -- >>> Daniel Vetter >>> Software Engineer, Intel Corporation >>> http://blog.ffwll.ch >>> >
On Thu, Jul 02, 2015 at 08:24:12AM +0530, Sharma, Shashank wrote: > Regards > Shashank > > On 7/1/2015 6:26 PM, Daniel Vetter wrote: > >On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote: > >>>Userspace always sets force. Are you sure this actually improves anything? > >>Yes we do. We have had this code for commercial projects, and that's really > >> important to have proper interrupt handling as well as to avoid race > >>condition between multiple HDMI detects from interrupt handler and > >>userspace detect calls. This is a must for HDMI compliance also. > > > >There's no race, we have locks for this. And we already have a little bit > >of edid caching, and you're code won't add more caching for the force = > >true case. Which is why I wondered whether you've really seen improvements > >with this on latest upstream, and not just an older android tree. > This caching is not useful, as in every detect call, we are doing a unset > EDID, and doing a set EDID again. The actual reason behind multiple HDMI > EDID reads is detect() getting called from user space only.So every time > there is a detect call, there is HDMI EDID read. > > We should read EDID only on hotplug, cache it, and reuse it until hot > unplug. But that's not what your patch is doing. You drop the cache when force is set in ->detect, which is always the case when called on userspace's behalf. That's why I wondered whether you've managed to measure any real improvement here. > >>Actually the plan is to use this force for GEN < 6 HW only, where the > >>hotplug doesn't work reliably (I remember our last conversation on some old > >>HW which doesn't support HPD properly). For vlv+, we can (will) use only > >>the cached EDID. > > > >Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which > >is half-busted it seems, and we've found examples for every single > >platform that supports hdmi out there. The problem isn't necessarily spec > >compliant HDMI sinks, but all the other crap ppl like to plug in. > > > >Yes this means we'll not be spec compliant, but if we have reality vs. > >spec, reality wins. At least in upstream. > > We have tested HPD with several HDMI monitors for VLV, CHV SKL and now for > BXT also. We are getting reliable hotplugs and unplugs across these > platforms, with accurate information on long/short pulses. Can you please > give some details about what is your observation ? > > Its very important for the Android projects to comply with the spec due to > certification pressure from customers. And we can get a common path for us, > if we know what exactly is the problem. But for sure we cant ignore this > factor that compliance is essential. Well we've tried this a few years back and had to revert on all machines because somehow hdp signalling isn't reliable. It might have been trouble with non-spec compliant sinks, but people where still annoyed about them no longer working. And I have a HP LP2475w here which is affected it seems: When unpluggint the hpd irq sometimes doesn't happen. And yes this is with a hdmi sink connector, not some hdmi->dvi cable or some other horror show. Last time I looked at it the hpd status bits where also unreliable (i.e. hpd indicated disconnected when the screen was clearly connected and working). Another hdmi screen I have here works. So yeah it's a sink problem, it's probably shitty/broken hw, but it means that it's completely independant of the platform. This means we _have_ to be spec incompliant to make real world hw word, and for upstream real world hw always beats specs. Which means I really can't merge any patch which assumes that hdmi hpd works. Or we figure out what has been broken with these screens and try to re-enable, but then we need to try this on every platfrom we support hdmi on since it's a clearly a sink problem. > >>>Also the goal should be to keep things cache for a few calls from > >>>userspace (since often it pokes a few times in a row unfortuantely), for > >>>which we need a proper timeout to clear the edid again. > >> > >>Can you please let us know why ? Why do we need to clear this EDID caching > >>? We should clear it only in the next hot-unplug, and maintain this cached > >>EDID for all userspace detect operations. I believe as long as we have the > >>state machine maintained, we need not to clear it. > > > >hotplug is not reliable, at least not outside of labs and validation > >testers. And your code here throws the cached edid away every time force = > >true is set, which is pretty much always. At least on upstream. > > > >The only place where we don't set force is in the poll worker, and that's > >only run when we have a hpd storm. > >-Daniel > The new code doesn't throw away cached EDID for platform's > GEN6 > but the current code does that, in every detect call. Hm, I didn't see that > gen6 check in your patches, where is it? Thanks, Daniel > > The current state machine is: > ============================= > 1. Hotplug -> Unset EDID, Read EDID, Set edid > 2. all detect calls -> Unset EDID, read EDID, Set EDID > 3. Hotunplug -> Unset EDID > > The state machine I am suggesting is: > ===================================== > 1. Hotplug -> throw away cached EDID, cache new one probing DDC > 2. all detect() calls -> > use cached EDID only > 3. hot unplug -> throw away cached EDID, > > OR if you want to support some old platforms: > ============================================= > 1. Hotplug -> throw away cached EDID, cache new one probing DDC > 2. all detect() calls -> (Support old HW with unstable HPD) > if (gen > GEN6) > use cached EDID only > else > probe DDC and read EDID, update cache > 3. hot unplug -> throw away cached EDID, > > > >> > >>>-Daniel > >> > >>Regards > >>Shashank > >> > >>On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > >> > >>>On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote: > >>>>From: Shashank Sharma <contactshashanksharma@gmail.com> > >>>> > >>>>This patch makes sure that the HDMI detect function > >>>>reads EDID only when its forced to do it. All the other > >>>>times, it uses the connector->detect_edid which was cached > >>>>during hotplug handling in the hdmi_probe() function. As the > >>>>probe function gets called before detect in the interrupt handler > >>>>and handles the EDID cacheing part, its absolutely safe to assume > >>>>that presence of EDID reflects monitor connected and viceversa. > >>>> > >>>>This will save us from many race conditions between hotplug/unplug > >>>>detect call handler threads and userspace calls for the same. > >>>>The previous patch in this patch series explains this in detail. > >>>> > >>>>Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com> > >>>>--- > >>>> drivers/gpu/drm/i915/intel_hdmi.c | 26 ++++++++++++++++++++------ > >>>> 1 file changed, 20 insertions(+), 6 deletions(-) > >>>> > >>>>diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > >>>b/drivers/gpu/drm/i915/intel_hdmi.c > >>>>index 064ddd8..1fb6919 100644 > >>>>--- a/drivers/gpu/drm/i915/intel_hdmi.c > >>>>+++ b/drivers/gpu/drm/i915/intel_hdmi.c > >>>>@@ -1362,19 +1362,33 @@ static enum drm_connector_status > >>>> intel_hdmi_detect(struct drm_connector *connector, bool force) > >>>> { > >>>> enum drm_connector_status status; > >>>>+ struct intel_connector *intel_connector = > >>>>+ to_intel_connector(connector); > >>>> > >>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > >>>> connector->base.id, connector->name); > >>>>+ /* > >>>>+ * There are many userspace calls which probe EDID from > >>>>+ * detect path. In case on multiple hotplug/unplug, these > >>>>+ * can cause race conditions while probing EDID. Also its > >>>>+ * waste of CPU cycles to read the EDID again and again > >>>>+ * unless there is a real hotplug. > >>>>+ * So until we are forced, check connector status > >>>>+ * based on availability of cached EDID. This will avoid many of > >>>>+ * these race conditions and timing problems. > >>>>+ */ > >>>>+ if (force) > >>> > >>>Userspace always sets force. Are you sure this actually improves anything? > >>>Also the goal should be to keep things cache for a few calls from > >>>userspace (since often it pokes a few times in a row unfortuantely), for > >>>which we need a proper timeout to clear the edid again. > >>>-Daniel > >>> > >>>>+ intel_hdmi_probe(intel_connector->encoder); > >>>> > >>>>- intel_hdmi_unset_edid(connector); > >>>>- > >>>>- if (intel_hdmi_set_edid(connector)) { > >>>>+ if (intel_connector->detect_edid) { > >>>> struct intel_hdmi *intel_hdmi = > >>>intel_attached_hdmi(connector); > >>>>- > >>>>- hdmi_to_dig_port(intel_hdmi)->base.type = > >>>INTEL_OUTPUT_HDMI; > >>>> status = connector_status_connected; > >>>>- } else > >>>>+ hdmi_to_dig_port(intel_hdmi)->base.type = > >>>INTEL_OUTPUT_HDMI; > >>>>+ DRM_DEBUG_DRIVER("hdmi status = connected\n"); > >>>>+ } else { > >>>> status = connector_status_disconnected; > >>>>+ DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); > >>>>+ } > >>>> > >>>> return status; > >>>> } > >>>>-- > >>>>1.7.10.4 > >>>> > >>>>_______________________________________________ > >>>>Intel-gfx mailing list > >>>>Intel-gfx@lists.freedesktop.org > >>>>http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >>> > >>>-- > >>>Daniel Vetter > >>>Software Engineer, Intel Corporation > >>>http://blog.ffwll.ch > >>> > >
On 7/6/2015 1:08 PM, Daniel Vetter wrote: > On Thu, Jul 02, 2015 at 08:24:12AM +0530, Sharma, Shashank wrote: >> Regards >> Shashank >> >> On 7/1/2015 6:26 PM, Daniel Vetter wrote: >>> On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote: >>>>> Userspace always sets force. Are you sure this actually improves anything? >>>> Yes we do. We have had this code for commercial projects, and that's really >>>> important to have proper interrupt handling as well as to avoid race >>>> condition between multiple HDMI detects from interrupt handler and >>>> userspace detect calls. This is a must for HDMI compliance also. >>> >>> There's no race, we have locks for this. And we already have a little bit >>> of edid caching, and you're code won't add more caching for the force = >>> true case. Which is why I wondered whether you've really seen improvements >>> with this on latest upstream, and not just an older android tree. >> This caching is not useful, as in every detect call, we are doing a unset >> EDID, and doing a set EDID again. The actual reason behind multiple HDMI >> EDID reads is detect() getting called from user space only.So every time >> there is a detect call, there is HDMI EDID read. >> >> We should read EDID only on hotplug, cache it, and reuse it until hot >> unplug. > > But that's not what your patch is doing. You drop the cache when force is > set in ->detect, which is always the case when called on userspace's > behalf. That's why I wondered whether you've managed to measure any real > improvement here. > >>>> Actually the plan is to use this force for GEN < 6 HW only, where the >>>> hotplug doesn't work reliably (I remember our last conversation on some old >>>> HW which doesn't support HPD properly). For vlv+, we can (will) use only >>>> the cached EDID. >>> >>> Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which >>> is half-busted it seems, and we've found examples for every single >>> platform that supports hdmi out there. The problem isn't necessarily spec >>> compliant HDMI sinks, but all the other crap ppl like to plug in. >>> >>> Yes this means we'll not be spec compliant, but if we have reality vs. >>> spec, reality wins. At least in upstream. >> >> We have tested HPD with several HDMI monitors for VLV, CHV SKL and now for >> BXT also. We are getting reliable hotplugs and unplugs across these >> platforms, with accurate information on long/short pulses. Can you please >> give some details about what is your observation ? >> >> Its very important for the Android projects to comply with the spec due to >> certification pressure from customers. And we can get a common path for us, >> if we know what exactly is the problem. But for sure we cant ignore this >> factor that compliance is essential. > > Well we've tried this a few years back and had to revert on all machines > because somehow hdp signalling isn't reliable. It might have been trouble > with non-spec compliant sinks, but people where still annoyed about them > no longer working. > > And I have a HP LP2475w here which is affected it seems: When unpluggint > the hpd irq sometimes doesn't happen. And yes this is with a hdmi sink > connector, not some hdmi->dvi cable or some other horror show. Last time I > looked at it the hpd status bits where also unreliable (i.e. hpd indicated > disconnected when the screen was clearly connected and working). Another > hdmi screen I have here works. So yeah it's a sink problem, it's probably > shitty/broken hw, but it means that it's completely independant of the > platform. > I see this problem with SKL HPD also.. and I figured out writing again and again to the port_hpd_hotplug stat somehow screws the state of hotplug. Currently we update to that reg for any interrupt. After putting a check to write to that reg only if a real hpd is triggered, it works good. Please check: [PATCH] [RFC] drm/i915: Handle HPD when it has actually occurred > This means we _have_ to be spec incompliant to make real world hw word, > and for upstream real world hw always beats specs. Which means I really > can't merge any patch which assumes that hdmi hpd works. Or we figure out > what has been broken with these screens and try to re-enable, but then we > need to try this on every platfrom we support hdmi on since it's a clearly > a sink problem. > >>>>> Also the goal should be to keep things cache for a few calls from >>>>> userspace (since often it pokes a few times in a row unfortuantely), for >>>>> which we need a proper timeout to clear the edid again. >>>> >>>> Can you please let us know why ? Why do we need to clear this EDID caching >>>> ? We should clear it only in the next hot-unplug, and maintain this cached >>>> EDID for all userspace detect operations. I believe as long as we have the >>>> state machine maintained, we need not to clear it. >>> >>> hotplug is not reliable, at least not outside of labs and validation >>> testers. And your code here throws the cached edid away every time force = >>> true is set, which is pretty much always. At least on upstream. >>> >>> The only place where we don't set force is in the poll worker, and that's >>> only run when we have a hpd storm. >>> -Daniel >> The new code doesn't throw away cached EDID for platform's > GEN6 >> but the current code does that, in every detect call. > > Hm, I didn't see that > gen6 check in your patches, where is it? > > Thanks, Daniel >> >> The current state machine is: >> ============================= >> 1. Hotplug -> Unset EDID, Read EDID, Set edid >> 2. all detect calls -> Unset EDID, read EDID, Set EDID >> 3. Hotunplug -> Unset EDID >> >> The state machine I am suggesting is: >> ===================================== >> 1. Hotplug -> throw away cached EDID, cache new one probing DDC >> 2. all detect() calls -> >> use cached EDID only >> 3. hot unplug -> throw away cached EDID, >> >> OR if you want to support some old platforms: >> ============================================= >> 1. Hotplug -> throw away cached EDID, cache new one probing DDC >> 2. all detect() calls -> (Support old HW with unstable HPD) >> if (gen > GEN6) >> use cached EDID only >> else >> probe DDC and read EDID, update cache >> 3. hot unplug -> throw away cached EDID, >>> >>>> >>>>> -Daniel >>>> >>>> Regards >>>> Shashank >>>> >>>> On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> >>>>> On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote: >>>>>> From: Shashank Sharma <contactshashanksharma@gmail.com> >>>>>> >>>>>> This patch makes sure that the HDMI detect function >>>>>> reads EDID only when its forced to do it. All the other >>>>>> times, it uses the connector->detect_edid which was cached >>>>>> during hotplug handling in the hdmi_probe() function. As the >>>>>> probe function gets called before detect in the interrupt handler >>>>>> and handles the EDID cacheing part, its absolutely safe to assume >>>>>> that presence of EDID reflects monitor connected and viceversa. >>>>>> >>>>>> This will save us from many race conditions between hotplug/unplug >>>>>> detect call handler threads and userspace calls for the same. >>>>>> The previous patch in this patch series explains this in detail. >>>>>> >>>>>> Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 26 ++++++++++++++++++++------ >>>>>> 1 file changed, 20 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c >>>>> b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> index 064ddd8..1fb6919 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> @@ -1362,19 +1362,33 @@ static enum drm_connector_status >>>>>> intel_hdmi_detect(struct drm_connector *connector, bool force) >>>>>> { >>>>>> enum drm_connector_status status; >>>>>> + struct intel_connector *intel_connector = >>>>>> + to_intel_connector(connector); >>>>>> >>>>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>>>> connector->base.id, connector->name); >>>>>> + /* >>>>>> + * There are many userspace calls which probe EDID from >>>>>> + * detect path. In case on multiple hotplug/unplug, these >>>>>> + * can cause race conditions while probing EDID. Also its >>>>>> + * waste of CPU cycles to read the EDID again and again >>>>>> + * unless there is a real hotplug. >>>>>> + * So until we are forced, check connector status >>>>>> + * based on availability of cached EDID. This will avoid many of >>>>>> + * these race conditions and timing problems. >>>>>> + */ >>>>>> + if (force) >>>>> >>>>> Userspace always sets force. Are you sure this actually improves anything? >>>>> Also the goal should be to keep things cache for a few calls from >>>>> userspace (since often it pokes a few times in a row unfortuantely), for >>>>> which we need a proper timeout to clear the edid again. >>>>> -Daniel >>>>> >>>>>> + intel_hdmi_probe(intel_connector->encoder); >>>>>> >>>>>> - intel_hdmi_unset_edid(connector); >>>>>> - >>>>>> - if (intel_hdmi_set_edid(connector)) { >>>>>> + if (intel_connector->detect_edid) { >>>>>> struct intel_hdmi *intel_hdmi = >>>>> intel_attached_hdmi(connector); >>>>>> - >>>>>> - hdmi_to_dig_port(intel_hdmi)->base.type = >>>>> INTEL_OUTPUT_HDMI; >>>>>> status = connector_status_connected; >>>>>> - } else >>>>>> + hdmi_to_dig_port(intel_hdmi)->base.type = >>>>> INTEL_OUTPUT_HDMI; >>>>>> + DRM_DEBUG_DRIVER("hdmi status = connected\n"); >>>>>> + } else { >>>>>> status = connector_status_disconnected; >>>>>> + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); >>>>>> + } >>>>>> >>>>>> return status; >>>>>> } >>>>>> -- >>>>>> 1.7.10.4 >>>>>> >>>>>> _______________________________________________ >>>>>> Intel-gfx mailing list >>>>>> Intel-gfx@lists.freedesktop.org >>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >>>>> >>>>> -- >>>>> Daniel Vetter >>>>> Software Engineer, Intel Corporation >>>>> http://blog.ffwll.ch >>>>> >>> >
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 064ddd8..1fb6919 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1362,19 +1362,33 @@ static enum drm_connector_status intel_hdmi_detect(struct drm_connector *connector, bool force) { enum drm_connector_status status; + struct intel_connector *intel_connector = + to_intel_connector(connector); DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); + /* + * There are many userspace calls which probe EDID from + * detect path. In case on multiple hotplug/unplug, these + * can cause race conditions while probing EDID. Also its + * waste of CPU cycles to read the EDID again and again + * unless there is a real hotplug. + * So until we are forced, check connector status + * based on availability of cached EDID. This will avoid many of + * these race conditions and timing problems. + */ + if (force) + intel_hdmi_probe(intel_connector->encoder); - intel_hdmi_unset_edid(connector); - - if (intel_hdmi_set_edid(connector)) { + if (intel_connector->detect_edid) { struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - - hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; status = connector_status_connected; - } else + hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; + DRM_DEBUG_DRIVER("hdmi status = connected\n"); + } else { status = connector_status_disconnected; + DRM_DEBUG_DRIVER("hdmi status = disconnected\n"); + } return status; }