[Intel-gfx] [PATCH 2/6] drm/i915: Add HDMI probe function
Sharma, Shashank
shashank.sharma at intel.com
Mon Sep 14 06:45:23 PDT 2015
Regards
Shashank
On 9/14/2015 6:37 PM, Daniel Vetter wrote:
> On Mon, Sep 14, 2015 at 09:55:33AM +0000, Sharma, Shashank wrote:
>>> You need to inject a fake hpd or similar everytime the world might have changed from under us, which isn't just init but also system resume.
>> This is a fake hpd only, just without wrapper, if you want, we can
>> create a wrapper function like (intel_hdmi_insert_fake_hpd), and call
>> this function from init() And call HDMI probe(hot_plug) function from
>> here. This would be just adding some CPU cycles in the code.
>
> We already have fake hpd handling, and it's done at a higher level (and in
> all the correct places) using drm_helper_hpd_irq_event.
>
Thanks, this sounds like a good suggestion here.
We can call the hot_plug() from drm_helper_hpd_irq_event, just before it
calls the detect(), and our state machine is still intact.
> Your patch here breaks that, but then only partially fixes things up again
> (you're missing system resume). That's not a good design, since it means
> we need bandaids all over also in the future for anything else (like DP).
>
I guess, in this was we wont break anything existing.
> Hence imo why we should create a new overall design with how this is
> supposed to be handled, and then change the core bits (so probably we need
> an intel version of drm_helper_hpd_irq_event and use that instead of the
> current calls). Eventually we want to make that async to speed up resume.
>
>>> Adding hacks like these in the init code is certainly the wrong approach, since we never done detection this early (so surprising from a high-level
>>> ordering) and it also misses resume (which is why it should be done in code shared between driver load and resume).
>> During the ADF code development, we saw that fb_console comes up very
>> soon, as soon as the intel_fbdev is ready, causing a detect() on a
>> connected HDMI. Do you think intel_hpd_init() would be the right place
>> to probe, coz as you rightly mentioned, we will need this on
>> suspend/resume also.
>
> Nope. fbdev is optional and you can't rely upon that, or write the code
> assuming it's there. The initial probing at driver load time is atm not
> done at all, we rely upon fbdev or early userspace to do the probing.
>
> What you probably want to do instead is reset the edid cache on driver
> load and on resume to make sure we don't cache a potentially bogus edid.
> We have the encoder->reset hook for that kind of work. That might be
> another approach.
>
Yes, thats exactly what hot_plug function does. It checks the live
status and if we have cached EDID, it releases the EDID. Now its just
matter of calling this from the right place, in the right way.
> But doing probe in _init for dynamic ports isnt' the right approach imo.
> -Daniel
>
Yep, agree. Let us work on this part.
>>
>> Regards
>> Shashank
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Daniel Vetter
>> Sent: Monday, September 14, 2015 2:00 PM
>> To: Jindal, Sonika
>> Cc: intel-gfx at lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH 2/6] drm/i915: Add HDMI probe function
>>
>> On Fri, Sep 11, 2015 at 04:15:39PM +0530, Jindal, Sonika wrote:
>>>
>>>
>>> On 9/10/2015 12:25 AM, Rodrigo Vivi wrote:
>>>> I liked the approach and agree with Daniel, so with his suggestions
>>>> feel free to use:
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com
>>>> <mailto:rodrigo.vivi at intel.com>>
>>>>
>>>> On Fri, Sep 4, 2015 at 7:46 AM Daniel Vetter <daniel at ffwll.ch
>>>> <mailto:daniel at ffwll.ch>> wrote:
>>>>
>>>> On Fri, Sep 04, 2015 at 06:56:12PM +0530, Sonika Jindal wrote:
>>>> > From: Shashank Sharma <shashank.sharma at intel.com
>>>> <mailto:shashank.sharma at intel.com>>
>>>> >
>>>> > This patch adds a separate probe function for HDMI
>>>> > EDID read over DDC channel. This function has been
>>>> > registered as a .hot_plug handler for HDMI encoder.
>>>> >
>>>> > The current implementation of hdmi_detect()
>>>> > function re-sets the cached HDMI edid (in connector->detect_edid) in
>>>> > every detect call.This function gets called many times, sometimes
>>>> > directly from userspace probes, forcing drivers to read EDID every
>>>> > detect function call.This causes several problems like:
>>>> >
>>>> > 1. Race conditions in multiple hot_plug / unplug cases, between
>>>> > interrupts bottom halves and userspace detections.
>>>> > 2. Many Un-necessary EDID reads for single hotplug/unplug
>>>> > 3. HDMI complaince failures which expects only one EDID read per
>>>> hotplug
>>>> >
>>>> > This function will be serving the purpose of really reading the EDID
>>>> > by really probing the DDC channel, and updating the cached EDID.
>>>> >
>>>> > The plan is to:
>>>> > 1. i915 IRQ handler bottom half function already calls
>>>> > intel_encoder->hotplug() function. Adding This probe function
>>>> which
>>>> > will read the EDID only in case of a hotplug / unplug.
>>>> > 2. During init_connector his probe will be called to read the edid
>>>> > 3. Reuse the cached EDID in hdmi_detect() function.
>>>> >
>>>> > The "< gen7" check is there because this was tested only for >=gen7
>>>> > platforms. For older platforms the hotplug/reading edid path
>>>> remains same.
>>>> >
>>>> > v2: Calling set_edid instead of hdmi_probe during init.
>>>> > Also, for platforms having DDI, intel_encoder for DP and HDMI is same
>>>> > (taken from intel_dig_port), so for DP also, hot_plug function
>>>> gets called
>>>> > which is not intended here. So, check for HDMI in intel_hdmi_probe
>>>> > Rely on HPD for updating edid only for platforms gen > 8 and also
>>>> for VLV.
>>>> >
>>>> > v3: Dropping the gen < 8 || !VLV check. Now all platforms should
>>>> rely on
>>>> > hotplug or init for updating the edid.(Daniel)
>>>> > Also, calling hdmi_probe in init instead of set_edid
>>>> >
>>>> > Signed-off-by: Shashank Sharma <shashank.sharma at intel.com
>>>> <mailto:shashank.sharma at intel.com>>
>>>> > Signed-off-by: Sonika Jindal <sonika.jindal at intel.com
>>>> <mailto:sonika.jindal at intel.com>>
>>>> > ---
>>>> > drivers/gpu/drm/i915/intel_hdmi.c | 46
>>>> ++++++++++++++++++++++++++++++++-----
>>>> > 1 file changed, 40 insertions(+), 6 deletions(-)
>>>> >
>>>> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> > index 5bdb386..1eda71a 100644
>>>> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> > @@ -1368,23 +1368,53 @@ intel_hdmi_set_edid(struct drm_connector
>>>> *connector)
>>>> > return connected;
>>>> > }
>>>> >
>>>> > +void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>>>>
>>>> Please call it _hot_plug if it's for the _hot_plug path.
>>>>
>>> Hmm, initial plan was to call it from other places if any..
>>> But now it seems more logical to name it _hot_plug.
>>> I will do that.
>>>
>>>> > +{
>>>> > + struct intel_hdmi *intel_hdmi =
>>>> > + enc_to_intel_hdmi(&intel_encoder->base);
>>>> > + struct intel_connector *intel_connector =
>>>> > + intel_hdmi->attached_connector;
>>>> > +
>>>> > + /*
>>>> > + * We are here, means there is a hotplug or a force
>>>> > + * detection. Clear the cached EDID and probe the
>>>> > + * DDC bus to check the current status of HDMI.
>>>> > + */
>>>> > + intel_hdmi_unset_edid(&intel_connector->base);
>>>> > + if (intel_hdmi_set_edid(&intel_connector->base))
>>>> > + DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
>>>> > + else
>>>> > + DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
>>>> > +}
>>>> > +
>>>> > 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 <http://base.id>,
>>>> connector->name);
>>>> > + /*
>>>> > + * There are many userspace calls which probe EDID from
>>>> > + * detect path. In case of 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, rely on hotplugs and init to read edid.
>>>> > + * Check connector status based on availability of cached EDID.
>>>> > + */
>>>> >
>>>> > - 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;
>>>> > }
>>>> > @@ -2104,11 +2134,15 @@ void intel_hdmi_init_connector(struct
>>>> intel_digital_port *intel_dig_port,
>>>> > intel_connector->unregister = intel_connector_unregister;
>>>> >
>>>> > intel_hdmi_add_properties(intel_hdmi, connector);
>>>> > + intel_encoder->hot_plug = intel_hdmi_probe;
>>>> >
>>>> > intel_connector_attach_encoder(intel_connector, intel_encoder);
>>>> > drm_connector_register(connector);
>>>> > intel_hdmi->attached_connector = intel_connector;
>>>> >
>>>> > + /* Set edid during init */
>>>> > + intel_hdmi_probe(intel_encoder);
>>>>
>>>> Separate patch, but we really don't want this, initial probe should be
>>>> done async to avoid stalling the driver.
>>>> -Daniel
>>>
>>> Where else do you suggest to do this? detect call is one place which
>>> does this for DP and used to do for HDMI also.
>>> But since we need to take the edid reading part out of edid, I dont
>>> see any other place.
>>> Please suggest.
>>
>> You need to inject a fake hpd or similar everytime the world might have changed from under us, which isn't just init but also system resume.
>>
>> We do that already, so you just have to make sure that keeps on working.
>> Probably another reason why it would be better to rework that entire hpd handling/detection code, to make sure it's clean and can be understood.
>>
>> Adding hacks like these in the init code is certainly the wrong approach, since we never done detection this early (so surprising from a high-level
>> ordering) and it also misses resume (which is why it should be done in code shared between driver load and resume).
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list