[PATCH 1/3] drm/i915: add attached connector to hdmi container

[PATCH 1/3] drm/i915: add attached connector to hdmi container

[Sonika Jindal]

From: Shashank Sharma <contactshashanksharma@gmail.com>

This patch adds the intel_connector initialized to intel_hdmi
display, during the init phase, just like the other encoders do.
This attachment is very useful when we need to extract the connector
pointer during the hotplug handler function

Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e90c743..a47fbc3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -659,6 +659,7 @@ struct intel_hdmi {
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
 	enum hdmi_picture_aspect aspect_ratio;
+	struct intel_connector *attached_connector;
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				enum hdmi_infoframe_type type,
 				const void *frame, ssize_t len);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 00c4b40..af4d1e6 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2000,6 +2000,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 	drm_connector_register(connector);
+	intel_hdmi->attached_connector = intel_connector;
 
 	/* 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
-- 
1.7.10.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sonika Jindal]

From: Shashank Sharma <contactshashanksharma@gmail.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 reason behind addition of this separate function needs
brief explaination. 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. Reuse the cached EDID in hdmi_detect() function UNLESS we are forced
   to probe the EDID by the force=1 variable.The next patch in the
   series does this.
3. Make sure that we are using this force variable properly.

Signed-off-by: Shashank Sharma <contactshashanksharma@gmail.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index af4d1e6..064ddd8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1340,6 +1340,24 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	return connected;
 }
 
+void intel_hdmi_probe(struct intel_encoder *intel_encoder)
+{
+	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)
 {
@@ -1997,6 +2015,7 @@ 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);
-- 
1.7.10.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sonika Jindal]

From: Shashank Sharma <contactshashanksharma@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@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)
+		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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Daniel Vetter]

On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
> From: Shashank Sharma <contactshashanksharma@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Shashank Sharma]

[-- Attachment #1.1: Type: text/plain, Size: 4719 bytes --]

> 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@ffwll.ch> wrote:

> On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
> > From: Shashank Sharma <contactshashanksharma@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@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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #1.2: Type: text/html, Size: 6897 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Daniel Vetter]

On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote:
> > 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.

There's no race, we have locks for this. And we already have a little bit
of edid caching, and you're code won't add more caching for the force =
true case. Which is why I wondered whether you've really seen improvements
with this on latest upstream, and not just an older android tree.

> 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.

Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which
is half-busted it seems, and we've found examples for every single
platform that supports hdmi out there. The problem isn't necessarily spec
compliant HDMI sinks, but all the other crap ppl like to plug in.

Yes this means we'll not be spec compliant, but if we have reality vs.
spec, reality wins. At least in upstream.

> > 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.

hotplug is not reliable, at least not outside of labs and validation
testers. And your code here throws the cached edid away every time force =
true is set, which is pretty much always. At least on upstream.

The only place where we don't set force is in the poll worker, and that's
only run when we have a hpd storm.
-Daniel

> 
> >-Daniel
> 
> Regards
> Shashank
> 
> On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
> > > From: Shashank Sharma <contactshashanksharma@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6674
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -1              302/302              301/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                                  289/289              289/289
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-rmfb-interruptible      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)drm:intel_pch_fifo_underrun_irq_handler[i915]]*ERROR*PCH_transcoder_A_FIFO_underrun@PCH transcoder A FIFO underrun
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Sharma, Shashank]

Regards
Shashank

On 7/1/2015 6:26 PM, Daniel Vetter wrote:
> On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote:
>>> 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.
>
> There's no race, we have locks for this. And we already have a little bit
> of edid caching, and you're code won't add more caching for the force =
> true case. Which is why I wondered whether you've really seen improvements
> with this on latest upstream, and not just an older android tree.
This caching is not useful, as in every detect call, we are doing a 
unset EDID, and doing a set EDID again. The actual reason behind 
multiple HDMI EDID reads is detect() getting called from user space 
only.So every time there is a detect call, there is HDMI EDID read.

We should read EDID only on hotplug, cache it, and reuse it until hot 
unplug.
>
>> 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.
>
> Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which
> is half-busted it seems, and we've found examples for every single
> platform that supports hdmi out there. The problem isn't necessarily spec
> compliant HDMI sinks, but all the other crap ppl like to plug in.
>
> Yes this means we'll not be spec compliant, but if we have reality vs.
> spec, reality wins. At least in upstream.

We have tested HPD with several HDMI monitors for VLV, CHV SKL and now 
for BXT also. We are getting reliable hotplugs and unplugs across these 
platforms, with accurate information on long/short pulses. Can you 
please give some details about what is your observation ?

Its very important for the Android projects to comply with the spec due 
to certification pressure from customers. And we can get a common path 
for us, if we know what exactly is the problem. But for sure we cant 
ignore this factor that compliance is essential.
>
>>> 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.
>
> hotplug is not reliable, at least not outside of labs and validation
> testers. And your code here throws the cached edid away every time force =
> true is set, which is pretty much always. At least on upstream.
>
> The only place where we don't set force is in the poll worker, and that's
> only run when we have a hpd storm.
> -Daniel
The new code doesn't throw away cached EDID for platform's > GEN6
but the current code does that, in every detect call.

The current state machine is:
=============================
1. Hotplug -> Unset EDID, Read EDID, Set edid
2. all detect calls -> Unset EDID, read EDID, Set EDID
3. Hotunplug -> Unset EDID

The state machine I am suggesting is:
=====================================
1. Hotplug -> throw away cached EDID, cache new one probing DDC
2. all detect() calls ->
	use cached EDID only
3. hot unplug -> throw away cached EDID,

OR if you want to support some old platforms:
=============================================
1. Hotplug -> throw away cached EDID, cache new one probing DDC
2. all detect() calls -> (Support old HW with unstable HPD)
	if (gen > GEN6)
		use cached EDID only
	else
		probe DDC and read EDID, update cache
3. hot unplug -> throw away cached EDID,
>
>>
>>> -Daniel
>>
>> Regards
>> Shashank
>>
>> On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>>> On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
>>>> From: Shashank Sharma <contactshashanksharma@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@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@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> http://blog.ffwll.ch
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Daniel Vetter]

On Thu, Jul 02, 2015 at 08:24:12AM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 7/1/2015 6:26 PM, Daniel Vetter wrote:
> >On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote:
> >>>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.
> >
> >There's no race, we have locks for this. And we already have a little bit
> >of edid caching, and you're code won't add more caching for the force =
> >true case. Which is why I wondered whether you've really seen improvements
> >with this on latest upstream, and not just an older android tree.
> This caching is not useful, as in every detect call, we are doing a unset
> EDID, and doing a set EDID again. The actual reason behind multiple HDMI
> EDID reads is detect() getting called from user space only.So every time
> there is a detect call, there is HDMI EDID read.
> 
> We should read EDID only on hotplug, cache it, and reuse it until hot
> unplug.

But that's not what your patch is doing. You drop the cache when force is
set in ->detect, which is always the case when called on userspace's
behalf. That's why I wondered whether you've managed to measure any real
improvement here.

> >>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.
> >
> >Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which
> >is half-busted it seems, and we've found examples for every single
> >platform that supports hdmi out there. The problem isn't necessarily spec
> >compliant HDMI sinks, but all the other crap ppl like to plug in.
> >
> >Yes this means we'll not be spec compliant, but if we have reality vs.
> >spec, reality wins. At least in upstream.
> 
> We have tested HPD with several HDMI monitors for VLV, CHV SKL and now for
> BXT also. We are getting reliable hotplugs and unplugs across these
> platforms, with accurate information on long/short pulses. Can you please
> give some details about what is your observation ?
> 
> Its very important for the Android projects to comply with the spec due to
> certification pressure from customers. And we can get a common path for us,
> if we know what exactly is the problem. But for sure we cant ignore this
> factor that compliance is essential.

Well we've tried this a few years back and had to revert on all machines
because somehow hdp signalling isn't reliable. It might have been trouble
with non-spec compliant sinks, but people where still annoyed about them
no longer working.

And I have a HP LP2475w here which is affected it seems: When unpluggint
the hpd irq sometimes doesn't happen. And yes this is with a hdmi sink
connector, not some hdmi->dvi cable or some other horror show. Last time I
looked at it the hpd status bits where also unreliable (i.e. hpd indicated
disconnected when the screen was clearly connected and working). Another
hdmi screen I have here works. So yeah it's a sink problem, it's probably
shitty/broken hw, but it means that it's completely independant of the
platform.

This means we _have_ to be spec incompliant to make real world hw word,
and for upstream real world hw always beats specs. Which means I really
can't merge any patch which assumes that hdmi hpd works. Or we figure out
what has been broken with these screens and try to re-enable, but then we
need to try this on every platfrom we support hdmi on since it's a clearly
a sink problem.

> >>>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.
> >
> >hotplug is not reliable, at least not outside of labs and validation
> >testers. And your code here throws the cached edid away every time force =
> >true is set, which is pretty much always. At least on upstream.
> >
> >The only place where we don't set force is in the poll worker, and that's
> >only run when we have a hpd storm.
> >-Daniel
> The new code doesn't throw away cached EDID for platform's > GEN6
> but the current code does that, in every detect call.

Hm, I didn't see that > gen6 check in your patches, where is it?

Thanks, Daniel
> 
> The current state machine is:
> =============================
> 1. Hotplug -> Unset EDID, Read EDID, Set edid
> 2. all detect calls -> Unset EDID, read EDID, Set EDID
> 3. Hotunplug -> Unset EDID
> 
> The state machine I am suggesting is:
> =====================================
> 1. Hotplug -> throw away cached EDID, cache new one probing DDC
> 2. all detect() calls ->
> 	use cached EDID only
> 3. hot unplug -> throw away cached EDID,
> 
> OR if you want to support some old platforms:
> =============================================
> 1. Hotplug -> throw away cached EDID, cache new one probing DDC
> 2. all detect() calls -> (Support old HW with unstable HPD)
> 	if (gen > GEN6)
> 		use cached EDID only
> 	else
> 		probe DDC and read EDID, update cache
> 3. hot unplug -> throw away cached EDID,
> >
> >>
> >>>-Daniel
> >>
> >>Regards
> >>Shashank
> >>
> >>On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >>>On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
> >>>>From: Shashank Sharma <contactshashanksharma@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Read HDMI EDID only when required

[Jindal, Sonika]

On 7/6/2015 1:08 PM, Daniel Vetter wrote:
> On Thu, Jul 02, 2015 at 08:24:12AM +0530, Sharma, Shashank wrote:
>> Regards
>> Shashank
>>
>> On 7/1/2015 6:26 PM, Daniel Vetter wrote:
>>> On Tue, Jun 30, 2015 at 09:49:57PM +0530, Shashank Sharma wrote:
>>>>> 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.
>>>
>>> There's no race, we have locks for this. And we already have a little bit
>>> of edid caching, and you're code won't add more caching for the force =
>>> true case. Which is why I wondered whether you've really seen improvements
>>> with this on latest upstream, and not just an older android tree.
>> This caching is not useful, as in every detect call, we are doing a unset
>> EDID, and doing a set EDID again. The actual reason behind multiple HDMI
>> EDID reads is detect() getting called from user space only.So every time
>> there is a detect call, there is HDMI EDID read.
>>
>> We should read EDID only on hotplug, cache it, and reuse it until hot
>> unplug.
>
> But that's not what your patch is doing. You drop the cache when force is
> set in ->detect, which is always the case when called on userspace's
> behalf. That's why I wondered whether you've managed to measure any real
> improvement here.
>
>>>> 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.
>>>
>>> Ok, once more: HDMI hpd is unreliable everywhere. I have a gen7 here which
>>> is half-busted it seems, and we've found examples for every single
>>> platform that supports hdmi out there. The problem isn't necessarily spec
>>> compliant HDMI sinks, but all the other crap ppl like to plug in.
>>>
>>> Yes this means we'll not be spec compliant, but if we have reality vs.
>>> spec, reality wins. At least in upstream.
>>
>> We have tested HPD with several HDMI monitors for VLV, CHV SKL and now for
>> BXT also. We are getting reliable hotplugs and unplugs across these
>> platforms, with accurate information on long/short pulses. Can you please
>> give some details about what is your observation ?
>>
>> Its very important for the Android projects to comply with the spec due to
>> certification pressure from customers. And we can get a common path for us,
>> if we know what exactly is the problem. But for sure we cant ignore this
>> factor that compliance is essential.
>
> Well we've tried this a few years back and had to revert on all machines
> because somehow hdp signalling isn't reliable. It might have been trouble
> with non-spec compliant sinks, but people where still annoyed about them
> no longer working.
>
> And I have a HP LP2475w here which is affected it seems: When unpluggint
> the hpd irq sometimes doesn't happen. And yes this is with a hdmi sink
> connector, not some hdmi->dvi cable or some other horror show. Last time I
> looked at it the hpd status bits where also unreliable (i.e. hpd indicated
> disconnected when the screen was clearly connected and working). Another
> hdmi screen I have here works. So yeah it's a sink problem, it's probably
> shitty/broken hw, but it means that it's completely independant of the
> platform.
>
I see this problem with SKL HPD also.. and I figured out writing again 
and again to the port_hpd_hotplug stat somehow screws the state of hotplug.
Currently we update to that reg for any interrupt. After putting a check 
to write to that reg only if a real hpd is triggered, it works good.
Please check:
[PATCH] [RFC] drm/i915: Handle HPD when it has actually occurred

> This means we _have_ to be spec incompliant to make real world hw word,
> and for upstream real world hw always beats specs. Which means I really
> can't merge any patch which assumes that hdmi hpd works. Or we figure out
> what has been broken with these screens and try to re-enable, but then we
> need to try this on every platfrom we support hdmi on since it's a clearly
> a sink problem.
>
>>>>> 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.
>>>
>>> hotplug is not reliable, at least not outside of labs and validation
>>> testers. And your code here throws the cached edid away every time force =
>>> true is set, which is pretty much always. At least on upstream.
>>>
>>> The only place where we don't set force is in the poll worker, and that's
>>> only run when we have a hpd storm.
>>> -Daniel
>> The new code doesn't throw away cached EDID for platform's > GEN6
>> but the current code does that, in every detect call.
>
> Hm, I didn't see that > gen6 check in your patches, where is it?
>
> Thanks, Daniel
>>
>> The current state machine is:
>> =============================
>> 1. Hotplug -> Unset EDID, Read EDID, Set edid
>> 2. all detect calls -> Unset EDID, read EDID, Set EDID
>> 3. Hotunplug -> Unset EDID
>>
>> The state machine I am suggesting is:
>> =====================================
>> 1. Hotplug -> throw away cached EDID, cache new one probing DDC
>> 2. all detect() calls ->
>> 	use cached EDID only
>> 3. hot unplug -> throw away cached EDID,
>>
>> OR if you want to support some old platforms:
>> =============================================
>> 1. Hotplug -> throw away cached EDID, cache new one probing DDC
>> 2. all detect() calls -> (Support old HW with unstable HPD)
>> 	if (gen > GEN6)
>> 		use cached EDID only
>> 	else
>> 		probe DDC and read EDID, update cache
>> 3. hot unplug -> throw away cached EDID,
>>>
>>>>
>>>>> -Daniel
>>>>
>>>> Regards
>>>> Shashank
>>>>
>>>> On Tue, Jun 30, 2015 at 4:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>
>>>>> On Tue, Jun 30, 2015 at 11:13:58AM +0530, Sonika Jindal wrote:
>>>>>> From: Shashank Sharma <contactshashanksharma@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@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@lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>
>>>>> --
>>>>> Daniel Vetter
>>>>> Software Engineer, Intel Corporation
>>>>> http://blog.ffwll.ch
>>>>>
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915: add attached connector to hdmi container

[Sonika Jindal]

From: Shashank Sharma <shashank.sharma@intel.com>

This patch adds the intel_connector initialized to intel_hdmi
display, during the init phase, just like the other encoders do.
This attachment is very useful when we need to extract the connector
pointer during the hotplug handler function

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e90c743..a47fbc3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -659,6 +659,7 @@ struct intel_hdmi {
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
 	enum hdmi_picture_aspect aspect_ratio;
+	struct intel_connector *attached_connector;
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				enum hdmi_infoframe_type type,
 				const void *frame, ssize_t len);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 00c4b40..af4d1e6 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2000,6 +2000,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 	drm_connector_register(connector);
+	intel_hdmi->attached_connector = intel_connector;
 
 	/* 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
-- 
1.7.10.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx