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@xxxxxxxx> wrote:
> >>
> >>>On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
> >>>>From: Shashank Sharma <contactshashanksharma@xxxxxxxxx>
> >>>>
> >>>>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@xxxxxxxxx>
> >>>>---
> >>>> 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@xxxxxxxxxxxxxxxxxxxxx
> >>>>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> >>>--
> >>>Daniel Vetter
> >>>Software Engineer, Intel Corporation
> >>>http://blog.ffwll.ch
> >>>
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
