[Intel-gfx] [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Shashank Sharma]

> Userspace always sets force. Are you sure this actually improves anything?
Yes we do. We have had this code for commercial projects, and that's really
 important to have proper interrupt handling as well as to avoid race
condition between multiple HDMI detects from interrupt handler and
userspace detect calls. This is a must for HDMI compliance also.

Actually the plan is to use this force for GEN < 6 HW only, where the
hotplug doesn't work reliably (I remember our last conversation on some old
HW which doesn't support HPD properly). For vlv+, we can (will) use only
the cached EDID.

> Also the goal should be to keep things cache for a few calls from
> userspace (since often it pokes a few times in a row unfortuantely), for
> which we need a proper timeout to clear the edid again.

Can you please let us know why ? Why do we need to clear this EDID caching
? We should clear it only in the next hot-unplug, and maintain this cached
EDID for all userspace detect operations. I believe as long as we have the
state machine maintained, we need not to clear it.

>-Daniel

Regards
Shashank

On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel at ffwll.ch> wrote:

> On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
> > From: Shashank Sharma <contactshashanksharma at gmail.com>
> >
> > This patch makes sure that the HDMI detect function
> > reads EDID only when its forced to do it. All the other
> > times, it uses the connector->detect_edid which was cached
> > during hotplug handling in the hdmi_probe() function. As the
> > probe function gets called before detect in the interrupt handler
> > and handles the EDID cacheing part, its absolutely safe to assume
> > that presence of EDID reflects monitor connected and viceversa.
> >
> > This will save us from many race conditions between hotplug/unplug
> > detect call handler threads and userspace calls for the same.
> > The previous patch in this patch series explains this in detail.
> >
> > Signed-off-by: Shashank Sharma <contactshashanksharma at gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c |   26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 064ddd8..1fb6919 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1362,19 +1362,33 @@ 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 on 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 until we are forced, check connector status
> > +      * based on availability of cached EDID. This will avoid many of
> > +      * these race conditions and timing problems.
> > +      */
> > +     if (force)
>
> Userspace always sets force. Are you sure this actually improves anything?
> Also the goal should be to keep things cache for a few calls from
> userspace (since often it pokes a few times in a row unfortuantely), for
> which we need a proper timeout to clear the edid again.
> -Daniel
>
> > +             intel_hdmi_probe(intel_connector->encoder);
> >
> > -     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;
> >  }
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20150630/cddeb012/attachment-0001.html>