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@xxxxxxxxx
<mailto:rodrigo.vivi@xxxxxxxxx>>
On Fri, Sep 4, 2015 at 7:46 AM Daniel Vetter <daniel@xxxxxxxx
<mailto:daniel@xxxxxxxx>> wrote:
On Fri, Sep 04, 2015 at 06:56:12PM +0530, Sonika Jindal wrote:
> From: Shashank Sharma <shashank.sharma@xxxxxxxxx
<mailto:shashank.sharma@xxxxxxxxx>>
>
> 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@xxxxxxxxx
<mailto:shashank.sharma@xxxxxxxxx>>
> Signed-off-by: Sonika Jindal <sonika.jindal@xxxxxxxxx
<mailto:sonika.jindal@xxxxxxxxx>>
> ---
> 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.
Thanks,
Sonika
> +
> /* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first
be written
> * 0xd. Failure to do so will result in spurious
interrupts being
> * generated on the port when a cable is not attached.
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
<mailto:Intel-gfx@xxxxxxxxxxxxxxxxxxxxx>
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx <mailto:Intel-gfx@xxxxxxxxxxxxxxxxxxxxx>
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
