| Message ID | 1449722741-24763-1-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 10, 2015 at 10:15:41AM +0530, Sonika Jindal wrote: > From: Shashank Sharma <shashank.sharma@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@intel.com> > Signed-off-by: Sonika Jindal <sonika.jindal@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. Regards, Sonika -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Thursday, December 10, 2015 2:00 PM To: Jindal, Sonika <sonika.jindal@intel.com> Cc: intel-gfx@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@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@intel.com> > Signed-off-by: Sonika Jindal <sonika.jindal@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
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@ffwll.ch] On Behalf Of Daniel Vetter > Sent: Thursday, December 10, 2015 2:00 PM > To: Jindal, Sonika <sonika.jindal@intel.com> > Cc: intel-gfx@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@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@intel.com> > > Signed-off-by: Sonika Jindal <sonika.jindal@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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@ffwll.ch] On Behalf Of Daniel Vetter Sent: Thursday, December 10, 2015 2:24 PM To: Jindal, Sonika <sonika.jindal@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch>; intel-gfx@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@ffwll.ch] On Behalf Of > Daniel Vetter > Sent: Thursday, December 10, 2015 2:00 PM > To: Jindal, Sonika <sonika.jindal@intel.com> > Cc: intel-gfx@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@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@intel.com> > > Signed-off-by: Sonika Jindal <sonika.jindal@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@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
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;