2015-04-14 14:36 GMT-03:00 Todd Previte <tprev...@gmail.com>:
>
>
> On 4/14/15 4:29 AM, Paulo Zanoni wrote:
>>
>> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprev...@gmail.com>:
>>>
>>> Update the hot plug function to handle the SST case. Instead of placing
>>> the SST case within the long/short pulse block, it is now handled after
>>> determining that MST mode is not in use. This way, the topology
>>> management
>>> layer can handle any MST-related operations while SST operations are
>>> still
>>> correctly handled afterwards.
>>>
>>> This patch also corrects the problem of SST mode only being handled in
>>> the
>>> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing
>>> purposes
>>> both short and long pulses are used by the different tests, thus both
>>> cases
>>> need to be addressed for SST.
>>>
>>> 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.
>>>
>>> For the SST case, the main stream is disabled for long HPD pulses as this
>>> generally indicates either a connect/disconnect event or link failure.
>>> For
>>> a number of case in compliance testing, the source is required to disable
>>> the main link upon detection of a long HPD.
>>>
>>> V2:
>>> - N/A
>>> V3:
>>> - Place the SST mode link status check into the mst_fail case
>>> - Remove obsolete comment regarding SST mode operation
>>> - Removed an erroneous line of code that snuck in during rebasing
>>> V4:
>>> - Added a disable of the main stream (DP transport) for the long pulse
>>> case
>>> for SST to support compliance testing
>>> V5:
>>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>>> V6:
>>> - Reformatted a comment
>>>
>>> Signed-off-by: Todd Previte <tprev...@gmail.com>
>>> ---
>>> drivers/gpu/drm/i915/intel_dp.c | 19 ++++++++-----------
>>> 1 file changed, 8 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 77b6b15..ba2da44 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4572,29 +4572,26 @@ 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
>>> - */
>>> -
>>> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> - intel_dp_check_link_status(intel_dp);
>>> -
>>> drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> - }
>>> }
>>
>> Shouldn't the code be moved to exactly this spot instead of after the
>> put_power label? Why would we want to call check_link_status in case
>> we goto mst_fail? In case there is a valid reason, maybe it would be
>> better to do a big reorganization of this function because it's going
>> to start looking very weird - or at least rename the labels.
>
> No because then you don't get long pulses, only short ones.
No, what I mean is:
if (long_hpd) {
... code ...
} else {
... code ....
}
if (!intel_dp->is_mst) {
drm_modeset_lock(...)
... code ...
}
mst_fail:
... code ...
The other problem I point is: imagine we're SST and we get a long_hpd.
Then we run ibx_digital_port_connected(), and since the monitor is
disconnected we "goto mst_fail". We'll end up running
intel_dp_check_link_status() before returning, but we really shouldn't
run it since we know the monitor is disconnected.
> The put_power
> case is where this belongs, unless you want to duplicate code in both the
> long_pulse and the else clause. There is a separate mst_check_link_status
> call so this one is specific to SST mode. There is also a check to make sure
> it doesn't get called when MST is active and MST has hit a failure mode, so
> that is a non-issue.
>>
>> Also, for the long_hpd case, I see that check_link_status() will redo
>> some of the stuff we already did on this function, such as get_dpcd().
>> And if you follow my advice on patch 2, you will end up having even
>> more repeated code. I think you could try to do a careful analysis
>> here to make sure we're not calling stuff twice here, especially since
>> some of those operations are potentially slow.
>
> I see a couple places where the code is duplicated, specifically the
> connection check (which I encapsulated in a function and I'll likely roll
> forward into this one since it makes things more clear) and the DPCD read in
> the long pulse case. I removed the code in check_link_status for both of
> these things and it still passes compliance. Good catch Paulo. This has been
> fixed and tested and will be in the updated patch posted shortly.
>>>
>>> ret = IRQ_HANDLED;
>>>
>>> goto put_power;
>>> mst_fail:
>>> - /* if we were in MST mode, and device is not there get out of MST
>>> mode */
>>> if (intel_dp->is_mst) {
>>> + /* if we were in MST mode, and device is not there get
>>> out of MST mode */
>>
>> I don't see the need for changes such as the one above - I saw similar
>> cases in other patches you submitted. I often use git blame on
>> comments in order to be able to see the whole context of the change,
>> and a simple change like this makes it harder to blame. Also, you're
>> not even fixing the 80 column problem here. And I do prefer the
>> comment on top of the if statement.
>
> This is just an artifact of moving things around, as it likely was in the
> other patches. The only reason I will move comments is to clarify what they
> pertain to if code is moving around it. It's back where it belongs now so it
> doesn't even show up in the patch. Fixed for the next version.
>
>>
>>> DRM_DEBUG_KMS("MST device may have disappeared %d vs
>>> %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>>> intel_dp->is_mst = false;
>>> drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
>>> intel_dp->is_mst);
>>> }
>>> put_power:
>>> + /* SST mode - handle short/long pulses here */
>>> + if (!intel_dp->is_mst) {
>>> + 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 = IRQ_HANDLED;
>>> + }
>>> intel_display_power_put(dev_priv, power_domain);
>>>
>>> return ret;
>>> --
>>> 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
