[Intel-gfx] [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation
Todd Previte
tprevite at gmail.com
Tue Apr 14 10:36:12 PDT 2015
On 4/14/15 4:29 AM, Paulo Zanoni wrote:
> 2015-04-10 13:12 GMT-03:00 Todd Previte <tprevite at 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 <tprevite at 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. 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
More information about the Intel-gfx
mailing list