[Intel-gfx] [PATCH] drm/i915: Add hot_plug hook for hdmi encoder
Jindal, Sonika
sonika.jindal at intel.com
Thu Dec 10 01:04:50 PST 2015
By "we" I meant me :)
Also, since it isn't possible to check all the platforms for live status, it is safer to put a gen check to platforms which we are sure.
Anyways, lets see if increasing the delay helps there, then you can take a call.
Regards,
Sonika
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Thursday, December 10, 2015 2:24 PM
To: Jindal, Sonika <sonika.jindal at intel.com>
Cc: Daniel Vetter <daniel at ffwll.ch>; intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add hot_plug hook for hdmi encoder
On Thu, Dec 10, 2015 at 08:35:30AM +0000, Jindal, Sonika wrote:
> Sure, I will start a new thread for this.
>
> Regarding the regression, since it is a sandybridge system, I am not
> sure if I can debug it anyhow because of the unavailability of the
> system. For the live status check, we earlier suggested to use
> platforms gen8 and above.
> But for wider testing, we removed the check.
> If required we can add a check there.
It's your patch, you need to own this. And until we've confirmed that it's a source issue I won't add a genAnything check but instead just revert the patch.
I hope expectations from my side here are clear.
Thanks, Daniel
>
> Regards,
> Sonika
>
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Thursday, December 10, 2015 2:00 PM
> To: Jindal, Sonika <sonika.jindal at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add hot_plug hook for hdmi
> encoder
>
> On Thu, Dec 10, 2015 at 10:15:41AM +0530, Sonika Jindal wrote:
> > From: Shashank Sharma <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 this 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
> >
> > v4: Renaming intel_hdmi_probe to intel_hdmi_hot_plug.
> > Also calling this hotplug handler from intel_hpd_init to take care
> > of init resume scenarios.
> >
> > v5: Moved the call to encoder hotplug during init to separate
> > patch(Daniel)
> > v6: Rebased and maintaining authorship.
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> > Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
>
> Iirc we had to revert an earlier version of this. Commit message should cite that revert and explain why this version is better.
>
> Also after a few days please start a new thread when resubmitting a patch series. The idea behind in-reply-to is to keep the discussion together while it's still ongoing. That's not the case when the discussion has died down since weeks.
>
> Also, your patch to take hpd sense into account for hdmi detection has causes a regression. Your on the cc. Handling that regression needs to be absolute top priority (otherwise I need to revert that patch too).
>
> Thanks, Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_hdmi.c | 58
> > ++++++++++++++++++++++++++++++---------
> > 1 file changed, 45 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index bdd462e..16dd2a7 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1381,18 +1381,16 @@ intel_hdmi_set_edid(struct drm_connector *connector, bool force)
> > return connected;
> > }
> >
> > -static enum drm_connector_status
> > -intel_hdmi_detect(struct drm_connector *connector, bool force)
> > +void intel_hdmi_hot_plug(struct intel_encoder *intel_encoder)
> > {
> > - enum drm_connector_status status;
> > - struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> > - struct drm_i915_private *dev_priv = to_i915(connector->dev);
> > + struct intel_hdmi *intel_hdmi =
> > + enc_to_intel_hdmi(&intel_encoder->base);
> > + struct intel_connector *intel_connector =
> > + intel_hdmi->attached_connector;
> > + struct drm_i915_private *dev_priv =
> > +to_i915(intel_encoder->base.dev);
> > bool live_status = false;
> > unsigned int retry = 3;
> >
> > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > - connector->base.id, connector->name);
> > -
> > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> >
> > while (!live_status && --retry) {
> > @@ -1400,21 +1398,53 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> > hdmi_to_dig_port(intel_hdmi));
> > mdelay(10);
> > }
> > + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> >
> > if (!live_status)
> > DRM_DEBUG_KMS("Live status not up!");
> >
> > - intel_hdmi_unset_edid(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, live_status))
> > + 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);
> >
> > - if (intel_hdmi_set_edid(connector, live_status)) {
> > + 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 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.
> > + */
> > +
> > + 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
> > + DRM_DEBUG_DRIVER("hdmi status = connected\n");
> > + } else {
> > status = connector_status_disconnected;
> > -
> > - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> > + DRM_DEBUG_DRIVER("hdmi status = disconnected\n");
> > + }
> >
> > return status;
> > }
> > @@ -2131,6 +2161,8 @@ void intel_hdmi_init_connector(struct
> > intel_digital_port *intel_dig_port,
> >
> > intel_hdmi_add_properties(intel_hdmi, connector);
> >
> > + intel_encoder->hot_plug = intel_hdmi_hot_plug;
> > +
> > intel_connector_attach_encoder(intel_connector, intel_encoder);
> > drm_connector_register(connector);
> > intel_hdmi->attached_connector = intel_connector;
> > --
> > 1.9.1
> >
> > _______________________________________________
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list