Re: [PATCH 2/6] drm/i915: Add HDMI probe function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I liked the approach and agree with Daniel, so with his suggestions feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>

On Fri, Sep 4, 2015 at 7:46 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
On Fri, Sep 04, 2015 at 06:56:12PM +0530, Sonika Jindal wrote:
> From: Shashank Sharma <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>
> Signed-off-by: Sonika Jindal <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.

> +{
> +     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, 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

> +
>       /* 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
> 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
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux