Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

Todd Previte
2015-02-18T17:06:54Z

On 12/17/14 1:30 PM, Daniel Vetter wrote:
> On Wed, Dec 17, 2014 at 03:57:21PM -0200, Paulo Zanoni wrote:
>> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>>> Moves the non-MST case out of the if-statement and places it at the beginning
>>> of the function to handle HPD events for SST mode. The reasoning behind this
>>> is to accommodate link status checks for compliance testing. Some test devices
>>> use long pulses to perform test requests so link status must be checked
>>> regardless of the pulse width for the SST operational mode.
>> Can you please elaborate a little more on what do you see on these
>> devices? The test spec is very clear about short vs long HPD pulses,
>> so it's hard to believe a test device would get this wrong. We have
>> some registers on the PCH that allow us to redefine short vs long
>> durations. Have you tried to play with them?
>>
>> More below:
>>
>>> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
>>> previous compliance testing patch sequence. Review feedback on that patch
>>> indicated that updating intel_dp_hot_plug() was not the correct place for
>>> the test handler.
>>>
>>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
>>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 4a55ca6..73014d8 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>> power_domain = intel_display_port_power_domain(intel_encoder);
>>> intel_display_power_get(dev_priv, power_domain);
>>>
>>> + if (!intel_dp->is_mst) {
>>> + /*
>>> + * Pulse width doesn't matter for SST mode
>>> + * Handle the HPD event now
>>> + */
>>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> + intel_dp_check_link_status(intel_dp);
>> The very first thing intel_dp_check_link_status() does is to return in
>> case "connector->base.status != connected". If we're getting a long
>> HPD, it doesn't seem make sense to check this field because the status
>> might be changing due to the long HPD.
> I don't think we can unconditionally run SST hpd logic before we've
> correctly handled mst mode. It likely screws up the accounting.

Upon further review, it looks like the best solution here is to place
the SST code in the mst_fail case as the 'else' clause. That way SST
mode stuff gets handled regardless and if a need arises in the future
where differentiating between short and long becomes necessary, there's
a place to handle it. This will be in V3.

>>> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> + ret = false;
>>> + goto put_power;
>>> + }
>>> +
>>> if (long_hpd) {
>>>
>>> if (HAS_PCH_SPLIT(dev)) {
>>> @@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>> if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>>> goto mst_fail;
>>> }
>>> -
>>> - if (!intel_dp->is_mst) {
>>> - /*
>>> - * we'll check the link status via the normal hot plug path later -
>>> - * but for short hpds we should check it now
>>> - */
> Just aside: The above comment is outdated and can be remove. This is now
> the only place where we handle link retraining. The function could be made
> static and dropped from headers, too.
> -Daniel
Can't be static and pulled from the header. It's called in intel_ddi.c
as well.

But the comment and the code is gone, in light of the above changes.

>>> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> - intel_dp_check_link_status(intel_dp);
>>> - drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> - }
>>> }
>>> ret = false;
>>> goto put_power;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> --
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx