[PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.

The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.

A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
 	ssize_t ret;
 	int i;
 
+	/*
+	 * Sometime we just get the same incorrect byte repeated
+	 * over the entire buffer. Doing just one throw away read
+	 * initially seems to "solve" it.
+	 */
+	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
 	for (i = 0; i < 3; i++) {
 		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
 		if (ret == size)
-- 
2.0.4

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

[PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
to handle hpd we would need to turn on vdd to perform aux transfers.
This would lead to an endless cycle of
"vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."

So ignore long hpd pulses on eDP ports. eDP panels should be physically
tied to the machine anyway so they should not actually disappear and
thus don't need long hpd handling. Short hpds are still needed for link
re-train and whatnot so we can't just turn off the hpd interrupt
entirely for eDP ports. Perhaps we could turn it off whenever the panel
is disabled, but just ignoring the long hpd seems sufficient.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f07f02c..4455009 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
 		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
 
+	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
+		/*
+		 * vdd off can generate a long pulse on eDP which
+		 * would require vdd on to handle it, and thus we
+		 * would end up in an endless cycle of
+		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
+		 */
+		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
+			      port_name(intel_dig_port->port));
+		return false;
+	}
+
 	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
 		      port_name(intel_dig_port->port),
 		      long_hpd ? "long" : "short");
-- 
2.0.4

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

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Todd Previte]

On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
> to handle hpd we would need to turn on vdd to perform aux transfers.
> This would lead to an endless cycle of
> "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
>
> So ignore long hpd pulses on eDP ports. eDP panels should be physically
> tied to the machine anyway so they should not actually disappear and
> thus don't need long hpd handling. Short hpds are still needed for link
> re-train and whatnot so we can't just turn off the hpd interrupt
> entirely for eDP ports. Perhaps we could turn it off whenever the panel
> is disabled, but just ignoring the long hpd seems sufficient.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f07f02c..4455009 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>   	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
>   		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
>   
> +	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> +		/*
> +		 * vdd off can generate a long pulse on eDP which
> +		 * would require vdd on to handle it, and thus we
> +		 * would end up in an endless cycle of
> +		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> +		 */
> +		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> +			      port_name(intel_dig_port->port));
> +		return false;
> +	}
> +
>   	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
>   		      port_name(intel_dig_port->port),
>   		      long_hpd ? "long" : "short");
I'm not sure that ignoring a long pulse is the best way to handle it. 
eDP does not appear to differentiate between short and long pulses per 
the specification (not to mention that HPD support for eDP is optional 
in the first place). It seems to me that it would probably be better to 
handle them as a normal (short) HPD pulse and just do the regular link 
maintenance stuff. As I said, the spec doesn't differentiate between the 
long and short pulses for eDP so it's a safer bet to handle them as a 
short pulse than to ignore them entirely.

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

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Ville Syrjälä]

On Thu, Oct 16, 2014 at 12:38:55PM -0700, Todd Previte wrote:
> 
> On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
> > to handle hpd we would need to turn on vdd to perform aux transfers.
> > This would lead to an endless cycle of
> > "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> >
> > So ignore long hpd pulses on eDP ports. eDP panels should be physically
> > tied to the machine anyway so they should not actually disappear and
> > thus don't need long hpd handling. Short hpds are still needed for link
> > re-train and whatnot so we can't just turn off the hpd interrupt
> > entirely for eDP ports. Perhaps we could turn it off whenever the panel
> > is disabled, but just ignoring the long hpd seems sufficient.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f07f02c..4455009 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >   	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
> >   		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
> >   
> > +	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> > +		/*
> > +		 * vdd off can generate a long pulse on eDP which
> > +		 * would require vdd on to handle it, and thus we
> > +		 * would end up in an endless cycle of
> > +		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> > +		 */
> > +		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> > +			      port_name(intel_dig_port->port));
> > +		return false;
> > +	}
> > +
> >   	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
> >   		      port_name(intel_dig_port->port),
> >   		      long_hpd ? "long" : "short");
> I'm not sure that ignoring a long pulse is the best way to handle it. 
> eDP does not appear to differentiate between short and long pulses per 
> the specification (not to mention that HPD support for eDP is optional 
> in the first place). It seems to me that it would probably be better to 
> handle them as a normal (short) HPD pulse and just do the regular link 
> maintenance stuff. As I said, the spec doesn't differentiate between the 
> long and short pulses for eDP so it's a safer bet to handle them as a 
> short pulse than to ignore them entirely.

The spec does talk a lot about IRQ_HPD (which is the short pulse) and
the power sequencing diagrams explicitly show that HPD should be asserted
after the T3 delay and deasserted immediately on VDD off, so those would
be the long pulses. So based on that my patch seems to make sense.

It seems HPD is optional for the source only, in the sink it's madatory.
But that doesn't really matter anyway because if either end doesn't have
it it won't work. The spec does go on to say that we should periodically
poll the sink status if HPD_IRQ isn't available. We don't do that
currently, but it does sound like a sane idea in case the link drops out.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Todd Previte]

On 10/17/2014 1:43 AM, Ville Syrjälä wrote:
> On Thu, Oct 16, 2014 at 12:38:55PM -0700, Todd Previte wrote:
>> On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
>>> to handle hpd we would need to turn on vdd to perform aux transfers.
>>> This would lead to an endless cycle of
>>> "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
>>>
>>> So ignore long hpd pulses on eDP ports. eDP panels should be physically
>>> tied to the machine anyway so they should not actually disappear and
>>> thus don't need long hpd handling. Short hpds are still needed for link
>>> re-train and whatnot so we can't just turn off the hpd interrupt
>>> entirely for eDP ports. Perhaps we could turn it off whenever the panel
>>> is disabled, but just ignoring the long hpd seems sufficient.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index f07f02c..4455009 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>>    	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
>>>    		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
>>>    
>>> +	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
>>> +		/*
>>> +		 * vdd off can generate a long pulse on eDP which
>>> +		 * would require vdd on to handle it, and thus we
>>> +		 * would end up in an endless cycle of
>>> +		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
>>> +		 */
>>> +		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
>>> +			      port_name(intel_dig_port->port));
>>> +		return false;
>>> +	}
>>> +
>>>    	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
>>>    		      port_name(intel_dig_port->port),
>>>    		      long_hpd ? "long" : "short");
>> I'm not sure that ignoring a long pulse is the best way to handle it.
>> eDP does not appear to differentiate between short and long pulses per
>> the specification (not to mention that HPD support for eDP is optional
>> in the first place). It seems to me that it would probably be better to
>> handle them as a normal (short) HPD pulse and just do the regular link
>> maintenance stuff. As I said, the spec doesn't differentiate between the
>> long and short pulses for eDP so it's a safer bet to handle them as a
>> short pulse than to ignore them entirely.
> The spec does talk a lot about IRQ_HPD (which is the short pulse) and
> the power sequencing diagrams explicitly show that HPD should be asserted
> after the T3 delay and deasserted immediately on VDD off, so those would
> be the long pulses. So based on that my patch seems to make sense.
>
> It seems HPD is optional for the source only, in the sink it's madatory.
> But that doesn't really matter anyway because if either end doesn't have
> it it won't work. The spec does go on to say that we should periodically
> poll the sink status if HPD_IRQ isn't available. We don't do that
> currently, but it does sound like a sane idea in case the link drops out.
>

Yeah I saw some of the info on IRQ_HPD but didn't see the long pulse 
stuff. In any case, my concern was with missing a valid HPD event. In 
light of the above, that doesn't look like it's an issue, so I'm good 
with this patch.

It looks like HPD support in an eDP sink device is optional as well, at 
least according to the table on pg.34 of the eDP 1.4 spec. As you 
pointed out though, unless both source and sink devices support HPD, it 
doesn't really matter. I saw the bit about polling in there, too. It 
might be worth implementing a polling mechanism as a backup, but I don't 
know how useful it would be. I don't recall running across a sink device 
that doesn't support HPD, but that's not to say they don't exist.

Reviewed-by: Todd Previte <tprevite@gmail.com>

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Daniel Vetter]

On Fri, Oct 17, 2014 at 09:08:28AM -0700, Todd Previte wrote:
> 
> On 10/17/2014 1:43 AM, Ville Syrjälä wrote:
> >On Thu, Oct 16, 2014 at 12:38:55PM -0700, Todd Previte wrote:
> >>On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
> >>>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>>Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
> >>>to handle hpd we would need to turn on vdd to perform aux transfers.
> >>>This would lead to an endless cycle of
> >>>"vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> >>>
> >>>So ignore long hpd pulses on eDP ports. eDP panels should be physically
> >>>tied to the machine anyway so they should not actually disappear and
> >>>thus don't need long hpd handling. Short hpds are still needed for link
> >>>re-train and whatnot so we can't just turn off the hpd interrupt
> >>>entirely for eDP ports. Perhaps we could turn it off whenever the panel
> >>>is disabled, but just ignoring the long hpd seems sufficient.
> >>>
> >>>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>---
> >>>   drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
> >>>   1 file changed, 12 insertions(+)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>index f07f02c..4455009 100644
> >>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>@@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >>>   	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
> >>>   		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
> >>>+	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> >>>+		/*
> >>>+		 * vdd off can generate a long pulse on eDP which
> >>>+		 * would require vdd on to handle it, and thus we
> >>>+		 * would end up in an endless cycle of
> >>>+		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> >>>+		 */
> >>>+		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> >>>+			      port_name(intel_dig_port->port));
> >>>+		return false;
> >>>+	}
> >>>+
> >>>   	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
> >>>   		      port_name(intel_dig_port->port),
> >>>   		      long_hpd ? "long" : "short");
> >>I'm not sure that ignoring a long pulse is the best way to handle it.
> >>eDP does not appear to differentiate between short and long pulses per
> >>the specification (not to mention that HPD support for eDP is optional
> >>in the first place). It seems to me that it would probably be better to
> >>handle them as a normal (short) HPD pulse and just do the regular link
> >>maintenance stuff. As I said, the spec doesn't differentiate between the
> >>long and short pulses for eDP so it's a safer bet to handle them as a
> >>short pulse than to ignore them entirely.
> >The spec does talk a lot about IRQ_HPD (which is the short pulse) and
> >the power sequencing diagrams explicitly show that HPD should be asserted
> >after the T3 delay and deasserted immediately on VDD off, so those would
> >be the long pulses. So based on that my patch seems to make sense.
> >
> >It seems HPD is optional for the source only, in the sink it's madatory.
> >But that doesn't really matter anyway because if either end doesn't have
> >it it won't work. The spec does go on to say that we should periodically
> >poll the sink status if HPD_IRQ isn't available. We don't do that
> >currently, but it does sound like a sane idea in case the link drops out.
> >
> 
> Yeah I saw some of the info on IRQ_HPD but didn't see the long pulse stuff.
> In any case, my concern was with missing a valid HPD event. In light of the
> above, that doesn't look like it's an issue, so I'm good with this patch.
> 
> It looks like HPD support in an eDP sink device is optional as well, at
> least according to the table on pg.34 of the eDP 1.4 spec. As you pointed
> out though, unless both source and sink devices support HPD, it doesn't
> really matter. I saw the bit about polling in there, too. It might be worth
> implementing a polling mechanism as a backup, but I don't know how useful it
> would be. I don't recall running across a sink device that doesn't support
> HPD, but that's not to say they don't exist.
> 
> Reviewed-by: Todd Previte <tprevite@gmail.com>

Aside: We don't handle hpd for port A anyway, so for most panels this
doesn't matter all that much. Or we'd have piles more bug reports I think.

Anyway, Cc: stable@vger.kernel.org and one for Jani.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Dave Airlie]

> Aside: We don't handle hpd for port A anyway, so for most panels this
> doesn't matter all that much. Or we'd have piles more bug reports I think.
>

https://bugzilla.redhat.com/show_bug.cgi?id=1118448

probably like this.

Dave.

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Ville Syrjälä]

On Tue, Oct 21, 2014 at 06:00:05PM +0200, Daniel Vetter wrote:
> On Fri, Oct 17, 2014 at 09:08:28AM -0700, Todd Previte wrote:
> > 
> > On 10/17/2014 1:43 AM, Ville Syrjälä wrote:
> > >On Thu, Oct 16, 2014 at 12:38:55PM -0700, Todd Previte wrote:
> > >>On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
> > >>>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>
> > >>>Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
> > >>>to handle hpd we would need to turn on vdd to perform aux transfers.
> > >>>This would lead to an endless cycle of
> > >>>"vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> > >>>
> > >>>So ignore long hpd pulses on eDP ports. eDP panels should be physically
> > >>>tied to the machine anyway so they should not actually disappear and
> > >>>thus don't need long hpd handling. Short hpds are still needed for link
> > >>>re-train and whatnot so we can't just turn off the hpd interrupt
> > >>>entirely for eDP ports. Perhaps we could turn it off whenever the panel
> > >>>is disabled, but just ignoring the long hpd seems sufficient.
> > >>>
> > >>>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>>---
> > >>>   drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
> > >>>   1 file changed, 12 insertions(+)
> > >>>
> > >>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > >>>index f07f02c..4455009 100644
> > >>>--- a/drivers/gpu/drm/i915/intel_dp.c
> > >>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> > >>>@@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> > >>>   	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
> > >>>   		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
> > >>>+	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> > >>>+		/*
> > >>>+		 * vdd off can generate a long pulse on eDP which
> > >>>+		 * would require vdd on to handle it, and thus we
> > >>>+		 * would end up in an endless cycle of
> > >>>+		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> > >>>+		 */
> > >>>+		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> > >>>+			      port_name(intel_dig_port->port));
> > >>>+		return false;
> > >>>+	}
> > >>>+
> > >>>   	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
> > >>>   		      port_name(intel_dig_port->port),
> > >>>   		      long_hpd ? "long" : "short");
> > >>I'm not sure that ignoring a long pulse is the best way to handle it.
> > >>eDP does not appear to differentiate between short and long pulses per
> > >>the specification (not to mention that HPD support for eDP is optional
> > >>in the first place). It seems to me that it would probably be better to
> > >>handle them as a normal (short) HPD pulse and just do the regular link
> > >>maintenance stuff. As I said, the spec doesn't differentiate between the
> > >>long and short pulses for eDP so it's a safer bet to handle them as a
> > >>short pulse than to ignore them entirely.
> > >The spec does talk a lot about IRQ_HPD (which is the short pulse) and
> > >the power sequencing diagrams explicitly show that HPD should be asserted
> > >after the T3 delay and deasserted immediately on VDD off, so those would
> > >be the long pulses. So based on that my patch seems to make sense.
> > >
> > >It seems HPD is optional for the source only, in the sink it's madatory.
> > >But that doesn't really matter anyway because if either end doesn't have
> > >it it won't work. The spec does go on to say that we should periodically
> > >poll the sink status if HPD_IRQ isn't available. We don't do that
> > >currently, but it does sound like a sane idea in case the link drops out.
> > >
> > 
> > Yeah I saw some of the info on IRQ_HPD but didn't see the long pulse stuff.
> > In any case, my concern was with missing a valid HPD event. In light of the
> > above, that doesn't look like it's an issue, so I'm good with this patch.
> > 
> > It looks like HPD support in an eDP sink device is optional as well, at
> > least according to the table on pg.34 of the eDP 1.4 spec. As you pointed
> > out though, unless both source and sink devices support HPD, it doesn't
> > really matter. I saw the bit about polling in there, too. It might be worth
> > implementing a polling mechanism as a backup, but I don't know how useful it
> > would be. I don't recall running across a sink device that doesn't support
> > HPD, but that's not to say they don't exist.
> > 
> > Reviewed-by: Todd Previte <tprevite@gmail.com>
> 
> Aside: We don't handle hpd for port A anyway, so for most panels this
> doesn't matter all that much. Or we'd have piles more bug reports I think.

Yeah, but we should. Then we could actually enable PSR main-link off
mode and the fallback to manual retrain would work if/when the automagic
training fails.

> 
> Anyway, Cc: stable@vger.kernel.org and one for Jani.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Jani Nikula]

On Tue, 21 Oct 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Oct 17, 2014 at 09:08:28AM -0700, Todd Previte wrote:
>> 
>> On 10/17/2014 1:43 AM, Ville Syrjälä wrote:
>> >On Thu, Oct 16, 2014 at 12:38:55PM -0700, Todd Previte wrote:
>> >>On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
>> >>>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >>>
>> >>>Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
>> >>>to handle hpd we would need to turn on vdd to perform aux transfers.
>> >>>This would lead to an endless cycle of
>> >>>"vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
>> >>>
>> >>>So ignore long hpd pulses on eDP ports. eDP panels should be physically
>> >>>tied to the machine anyway so they should not actually disappear and
>> >>>thus don't need long hpd handling. Short hpds are still needed for link
>> >>>re-train and whatnot so we can't just turn off the hpd interrupt
>> >>>entirely for eDP ports. Perhaps we could turn it off whenever the panel
>> >>>is disabled, but just ignoring the long hpd seems sufficient.
>> >>>
>> >>>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

...

> Anyway, Cc: stable@vger.kernel.org and one for Jani.

Pushed both patches to drm-intel-fixes, thanks for the patches and
review.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Dave Airlie]

On 17 October 2014 03:46,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
> to handle hpd we would need to turn on vdd to perform aux transfers.
> This would lead to an endless cycle of
> "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
>
> So ignore long hpd pulses on eDP ports. eDP panels should be physically
> tied to the machine anyway so they should not actually disappear and
> thus don't need long hpd handling. Short hpds are still needed for link
> re-train and whatnot so we can't just turn off the hpd interrupt
> entirely for eDP ports. Perhaps we could turn it off whenever the panel
> is disabled, but just ignoring the long hpd seems sufficient.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Hell yes,

I think nouveau just disables the whole irq when they know the outputs
isn't being used,

but this looks good to me.

Reviewed-by: Dave Airlie <airlied@redhat.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f07f02c..4455009 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>         if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
>                 intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
>
> +       if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> +               /*
> +                * vdd off can generate a long pulse on eDP which
> +                * would require vdd on to handle it, and thus we
> +                * would end up in an endless cycle of
> +                * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> +                */
> +               DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> +                             port_name(intel_dig_port->port));
> +               return false;
> +       }
> +
>         DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
>                       port_name(intel_dig_port->port),
>                       long_hpd ? "long" : "short");
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Jani Nikula]

On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
> to handle hpd we would need to turn on vdd to perform aux transfers.
> This would lead to an endless cycle of
> "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
>
> So ignore long hpd pulses on eDP ports. eDP panels should be physically
> tied to the machine anyway so they should not actually disappear and
> thus don't need long hpd handling. Short hpds are still needed for link
> re-train and whatnot so we can't just turn off the hpd interrupt
> entirely for eDP ports. Perhaps we could turn it off whenever the panel
> is disabled, but just ignoring the long hpd seems sufficient.

Did you test this with my short vs. long hpd fix applied?

In any case, makes sense,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>





>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f07f02c..4455009 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
>  		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
>  
> +	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> +		/*
> +		 * vdd off can generate a long pulse on eDP which
> +		 * would require vdd on to handle it, and thus we
> +		 * would end up in an endless cycle of
> +		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> +		 */
> +		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> +			      port_name(intel_dig_port->port));
> +		return false;
> +	}
> +
>  	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
>  		      port_name(intel_dig_port->port),
>  		      long_hpd ? "long" : "short");
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Ignore long hpds on eDP ports

[Ville Syrjälä]

On Fri, Oct 17, 2014 at 11:49:54AM +0300, Jani Nikula wrote:
> On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Turning vdd on/off can generate a long hpd pulse on eDP ports. In order
> > to handle hpd we would need to turn on vdd to perform aux transfers.
> > This would lead to an endless cycle of
> > "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> >
> > So ignore long hpd pulses on eDP ports. eDP panels should be physically
> > tied to the machine anyway so they should not actually disappear and
> > thus don't need long hpd handling. Short hpds are still needed for link
> > re-train and whatnot so we can't just turn off the hpd interrupt
> > entirely for eDP ports. Perhaps we could turn it off whenever the panel
> > is disabled, but just ignoring the long hpd seems sufficient.
> 
> Did you test this with my short vs. long hpd fix applied?

Yes. Well, with my own version of it :)

Now someone should go and implement HPD support for port A.

> 
> In any case, makes sense,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> 
> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f07f02c..4455009 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4611,6 +4611,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >  	if (intel_dig_port->base.type != INTEL_OUTPUT_EDP)
> >  		intel_dig_port->base.type = INTEL_OUTPUT_DISPLAYPORT;
> >  
> > +	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> > +		/*
> > +		 * vdd off can generate a long pulse on eDP which
> > +		 * would require vdd on to handle it, and thus we
> > +		 * would end up in an endless cycle of
> > +		 * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..."
> > +		 */
> > +		DRM_DEBUG_KMS("ignoring long hpd on eDP port %c\n",
> > +			      port_name(intel_dig_port->port));
> > +		return false;
> > +	}
> > +
> >  	DRM_DEBUG_KMS("got hpd irq on port %c - %s\n",
> >  		      port_name(intel_dig_port->port),
> >  		      long_hpd ? "long" : "short");
> > -- 
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Todd Previte]

On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Sometimes we seem to get utter garbage from DPCD reads. The resulting
> buffer is filled with the same byte, and the operation completed without
> errors. My HP ZR24w monitor seems particularly susceptible to this
> problem once it's gone into a sleep mode.
>
> The issue seems to happen only for the first AUX message that wakes the
> sink up. But as the first AUX read we often do is the DPCD receiver
> cap it does wreak a bit of havoc with subsequent link training etc. when
> the receiver cap bw/lane/etc. information is garbage.
>
> A sufficient workaround seems to be to perform a single byte dummy read
> before reading the actual data. I suppose that just wakes up the sink
> sufficiently and we can just throw away the returned data in case it's
> crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 64c8e04..f07f02c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>   	ssize_t ret;
>   	int i;
>   
> +	/*
> +	 * Sometime we just get the same incorrect byte repeated
> +	 * over the entire buffer. Doing just one throw away read
> +	 * initially seems to "solve" it.
> +	 */
> +	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> +
>   	for (i = 0; i < 3; i++) {
>   		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>   		if (ret == size)
Seems like a reasonable workaround for this problem, though 
investigating the actual root cause might be worthwhile.

Reviewed-by: Todd Previte <tprevite@gmail.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Ville Syrjälä]

On Thu, Oct 16, 2014 at 12:39:29PM -0700, Todd Previte wrote:
> 
> On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Sometimes we seem to get utter garbage from DPCD reads. The resulting
> > buffer is filled with the same byte, and the operation completed without
> > errors. My HP ZR24w monitor seems particularly susceptible to this
> > problem once it's gone into a sleep mode.
> >
> > The issue seems to happen only for the first AUX message that wakes the
> > sink up. But as the first AUX read we often do is the DPCD receiver
> > cap it does wreak a bit of havoc with subsequent link training etc. when
> > the receiver cap bw/lane/etc. information is garbage.
> >
> > A sufficient workaround seems to be to perform a single byte dummy read
> > before reading the actual data. I suppose that just wakes up the sink
> > sufficiently and we can just throw away the returned data in case it's
> > crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 64c8e04..f07f02c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> >   	ssize_t ret;
> >   	int i;
> >   
> > +	/*
> > +	 * Sometime we just get the same incorrect byte repeated
> > +	 * over the entire buffer. Doing just one throw away read
> > +	 * initially seems to "solve" it.
> > +	 */
> > +	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> > +
> >   	for (i = 0; i < 3; i++) {
> >   		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> >   		if (ret == size)
> Seems like a reasonable workaround for this problem, though 
> investigating the actual root cause might be worthwhile.

Sure. If someone has an AUX analyzer and a HP ZR24w monitor it should
be trivial to look at the traffic and see if there's something bogus in
our AUX communication. Sadly I don't have an AUX analyzer.

> 
> Reviewed-by: Todd Previte <tprevite@gmail.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Todd Previte]

On 10/17/2014 2:06 AM, Ville Syrjälä wrote:
> On Thu, Oct 16, 2014 at 12:39:29PM -0700, Todd Previte wrote:
>> On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Sometimes we seem to get utter garbage from DPCD reads. The resulting
>>> buffer is filled with the same byte, and the operation completed without
>>> errors. My HP ZR24w monitor seems particularly susceptible to this
>>> problem once it's gone into a sleep mode.
>>>
>>> The issue seems to happen only for the first AUX message that wakes the
>>> sink up. But as the first AUX read we often do is the DPCD receiver
>>> cap it does wreak a bit of havoc with subsequent link training etc. when
>>> the receiver cap bw/lane/etc. information is garbage.
>>>
>>> A sufficient workaround seems to be to perform a single byte dummy read
>>> before reading the actual data. I suppose that just wakes up the sink
>>> sufficiently and we can just throw away the returned data in case it's
>>> crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 64c8e04..f07f02c 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>>>    	ssize_t ret;
>>>    	int i;
>>>    
>>> +	/*
>>> +	 * Sometime we just get the same incorrect byte repeated
>>> +	 * over the entire buffer. Doing just one throw away read
>>> +	 * initially seems to "solve" it.
>>> +	 */
>>> +	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
>>> +
>>>    	for (i = 0; i < 3; i++) {
>>>    		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>>>    		if (ret == size)
>> Seems like a reasonable workaround for this problem, though
>> investigating the actual root cause might be worthwhile.
> Sure. If someone has an AUX analyzer and a HP ZR24w monitor it should
> be trivial to look at the traffic and see if there's something bogus in
> our AUX communication. Sadly I don't have an AUX analyzer.

I've got the monitor on my desk but no AUX analyzer to use. I'll see if 
I can track one down.

-T

>> Reviewed-by: Todd Previte <tprevite@gmail.com>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Daniel Vetter]

On Thu, Oct 16, 2014 at 12:39:29PM -0700, Todd Previte wrote:
> 
> On 10/16/2014 10:46 AM, ville.syrjala@linux.intel.com wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Sometimes we seem to get utter garbage from DPCD reads. The resulting
> >buffer is filled with the same byte, and the operation completed without
> >errors. My HP ZR24w monitor seems particularly susceptible to this
> >problem once it's gone into a sleep mode.
> >
> >The issue seems to happen only for the first AUX message that wakes the
> >sink up. But as the first AUX read we often do is the DPCD receiver
> >cap it does wreak a bit of havoc with subsequent link training etc. when
> >the receiver cap bw/lane/etc. information is garbage.
> >
> >A sufficient workaround seems to be to perform a single byte dummy read
> >before reading the actual data. I suppose that just wakes up the sink
> >sufficiently and we can just throw away the returned data in case it's
> >crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >index 64c8e04..f07f02c 100644
> >--- a/drivers/gpu/drm/i915/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/intel_dp.c
> >@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> >  	ssize_t ret;
> >  	int i;
> >+	/*
> >+	 * Sometime we just get the same incorrect byte repeated
> >+	 * over the entire buffer. Doing just one throw away read
> >+	 * initially seems to "solve" it.
> >+	 */
> >+	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> >+
> >  	for (i = 0; i < 3; i++) {
> >  		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> >  		if (ret == size)
> Seems like a reasonable workaround for this problem, though investigating
> the actual root cause might be worthwhile.
> 
> Reviewed-by: Todd Previte <tprevite@gmail.com>

Cc: stable@vger.kernel.org

... one for Jani I guess. But I'm suspicious here too, so maybe we should
extract this read_wake function to the core dp helpers and convince that
some other driver should use it? Adding more people and lists.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Jani Nikula]

On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Sometimes we seem to get utter garbage from DPCD reads. The resulting
> buffer is filled with the same byte, and the operation completed without
> errors. My HP ZR24w monitor seems particularly susceptible to this
> problem once it's gone into a sleep mode.
>
> The issue seems to happen only for the first AUX message that wakes the
> sink up. But as the first AUX read we often do is the DPCD receiver
> cap it does wreak a bit of havoc with subsequent link training etc. when
> the receiver cap bw/lane/etc. information is garbage.

This makes me suspect our sink dpms and wake handling even more than I
already did. Someone(tm) should dig into the DP and hw specs again with
fresh eyes...

> A sufficient workaround seems to be to perform a single byte dummy read
> before reading the actual data. I suppose that just wakes up the sink
> sufficiently and we can just throw away the returned data in case it's
> crap. DP_DPCD_REV seems like a sufficiently safe location to read here.

Seems like a pretty harmless thing to do, and we already do loads of
dpcd reads anyway. We should throw this at some related bugs.

So ack.

BR,
Jani.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 64c8e04..f07f02c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>  	ssize_t ret;
>  	int i;
>  
> +	/*
> +	 * Sometime we just get the same incorrect byte repeated
> +	 * over the entire buffer. Doing just one throw away read
> +	 * initially seems to "solve" it.
> +	 */
> +	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> +
>  	for (i = 0; i < 3; i++) {
>  		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>  		if (ret == size)
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Ville Syrjälä]

On Fri, Oct 17, 2014 at 11:43:21AM +0300, Jani Nikula wrote:
> On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Sometimes we seem to get utter garbage from DPCD reads. The resulting
> > buffer is filled with the same byte, and the operation completed without
> > errors. My HP ZR24w monitor seems particularly susceptible to this
> > problem once it's gone into a sleep mode.
> >
> > The issue seems to happen only for the first AUX message that wakes the
> > sink up. But as the first AUX read we often do is the DPCD receiver
> > cap it does wreak a bit of havoc with subsequent link training etc. when
> > the receiver cap bw/lane/etc. information is garbage.
> 
> This makes me suspect our sink dpms and wake handling even more than I
> already did. Someone(tm) should dig into the DP and hw specs again with
> fresh eyes...

Yeah when I last looked at the spec it was rather vague on the subject.
On the other hand it seems to suggest that you're supposed to wake up
the sink via DP_SET_POWER before any other AUX transfer, but on the
other hand it seems to say AUX is fine as long as you remember to do
the extended retry in case the AUX circuitry was asleep in the sink.

However DPCD 1.0 doesn't even support DP_SET_POWER so that does suggest
that it's not really mandatory to wake things up first. Well, assuming
DPCD 1.0 devices even exist.

I was a bit suspicious of our AUX code as well, but IIRC didn't spot
anything obviously incorrect there when I had a look. So might be this
is just the sink being a bit buggy.

> 
> > A sufficient workaround seems to be to perform a single byte dummy read
> > before reading the actual data. I suppose that just wakes up the sink
> > sufficiently and we can just throw away the returned data in case it's
> > crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
> 
> Seems like a pretty harmless thing to do, and we already do loads of
> dpcd reads anyway. We should throw this at some related bugs.
> 
> So ack.
> 
> BR,
> Jani.
> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 64c8e04..f07f02c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> >  	ssize_t ret;
> >  	int i;
> >  
> > +	/*
> > +	 * Sometime we just get the same incorrect byte repeated
> > +	 * over the entire buffer. Doing just one throw away read
> > +	 * initially seems to "solve" it.
> > +	 */
> > +	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> > +
> >  	for (i = 0; i < 3; i++) {
> >  		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> >  		if (ret == size)
> > -- 
> > 2.0.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Todd Previte]

On 10/17/2014 1:59 AM, Ville Syrjälä wrote:
> On Fri, Oct 17, 2014 at 11:43:21AM +0300, Jani Nikula wrote:
>> On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Sometimes we seem to get utter garbage from DPCD reads. The resulting
>>> buffer is filled with the same byte, and the operation completed without
>>> errors. My HP ZR24w monitor seems particularly susceptible to this
>>> problem once it's gone into a sleep mode.
>>>
>>> The issue seems to happen only for the first AUX message that wakes the
>>> sink up. But as the first AUX read we often do is the DPCD receiver
>>> cap it does wreak a bit of havoc with subsequent link training etc. when
>>> the receiver cap bw/lane/etc. information is garbage.
>> This makes me suspect our sink dpms and wake handling even more than I
>> already did. Someone(tm) should dig into the DP and hw specs again with
>> fresh eyes...
> Yeah when I last looked at the spec it was rather vague on the subject.
> On the other hand it seems to suggest that you're supposed to wake up
> the sink via DP_SET_POWER before any other AUX transfer, but on the
> other hand it seems to say AUX is fine as long as you remember to do
> the extended retry in case the AUX circuitry was asleep in the sink.

The sink is supposed to exit power-down mode within 1ms of detecting a 
differential signal voltage on the AUX lines, according to the spec. So 
in theory, any AUX transactions should be able to wake up the sink 
device. As you pointed out, the AUX retries will catch the case where 
the AUX channel is powered down. I know of one instance where the write 
to SET_POWER is required, and that's in one of the compliance tests. 
Outside of that though, I haven't seen anything where it's mandatory.

-T

>
> However DPCD 1.0 doesn't even support DP_SET_POWER so that does suggest
> that it's not really mandatory to wake things up first. Well, assuming
> DPCD 1.0 devices even exist.
>
> I was a bit suspicious of our AUX code as well, but IIRC didn't spot
> anything obviously incorrect there when I had a look. So might be this
> is just the sink being a bit buggy.
>
>>> A sufficient workaround seems to be to perform a single byte dummy read
>>> before reading the actual data. I suppose that just wakes up the sink
>>> sufficiently and we can just throw away the returned data in case it's
>>> crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
>> Seems like a pretty harmless thing to do, and we already do loads of
>> dpcd reads anyway. We should throw this at some related bugs.
>>
>> So ack.
>>
>> BR,
>> Jani.
>>
>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 64c8e04..f07f02c 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>>>   	ssize_t ret;
>>>   	int i;
>>>   
>>> +	/*
>>> +	 * Sometime we just get the same incorrect byte repeated
>>> +	 * over the entire buffer. Doing just one throw away read
>>> +	 * initially seems to "solve" it.
>>> +	 */
>>> +	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
>>> +
>>>   	for (i = 0; i < 3; i++) {
>>>   		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>>>   		if (ret == size)
>>> -- 
>>> 2.0.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read

[Todd Previte]

On 10/17/2014 1:43 AM, Jani Nikula wrote:
> On Thu, 16 Oct 2014, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Sometimes we seem to get utter garbage from DPCD reads. The resulting
>> buffer is filled with the same byte, and the operation completed without
>> errors. My HP ZR24w monitor seems particularly susceptible to this
>> problem once it's gone into a sleep mode.
>>
>> The issue seems to happen only for the first AUX message that wakes the
>> sink up. But as the first AUX read we often do is the DPCD receiver
>> cap it does wreak a bit of havoc with subsequent link training etc. when
>> the receiver cap bw/lane/etc. information is garbage.
> This makes me suspect our sink dpms and wake handling even more than I
> already did. Someone(tm) should dig into the DP and hw specs again with
> fresh eyes...
I can go look into this today/next week. This problem sounds vaguely 
similar to one I've run across before.

-T

>> A sufficient workaround seems to be to perform a single byte dummy read
>> before reading the actual data. I suppose that just wakes up the sink
>> sufficiently and we can just throw away the returned data in case it's
>> crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
> Seems like a pretty harmless thing to do, and we already do loads of
> dpcd reads anyway. We should throw this at some related bugs.
>
> So ack.
>
> BR,
> Jani.
>
>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 64c8e04..f07f02c 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
>>   	ssize_t ret;
>>   	int i;
>>   
>> +	/*
>> +	 * Sometime we just get the same incorrect byte repeated
>> +	 * over the entire buffer. Doing just one throw away read
>> +	 * initially seems to "solve" it.
>> +	 */
>> +	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
>> +
>>   	for (i = 0; i < 3; i++) {
>>   		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
>>   		if (ret == size)
>> -- 
>> 2.0.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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