[Intel-gfx] [PATCH 3/3] drm/i915: Read HDMI EDID only when required
Jindal, Sonika
sonika.jindal at intel.com
Mon Jul 6 01:01:58 PDT 2015
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 at ffwll.ch> wrote:
>>>>
>>>>> On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
>>>>>> From: Shashank Sharma <contactshashanksharma at 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 at 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 at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>
>>>>> --
>>>>> Daniel Vetter
>>>>> Software Engineer, Intel Corporation
>>>>> http://blog.ffwll.ch
>>>>>
>>>
>
More information about the Intel-gfx
mailing list